[v3,2/7] app/proc-info: eliminate useless borders
Checks
Commit Message
Printing extra borders does not improve readability, and is just
unnecessary. Putting TSC hz in header also makes no sense here.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
app/proc-info/main.c | 35 +++++++----------------------------
1 file changed, 7 insertions(+), 28 deletions(-)
Comments
15/07/2020 23:22, Stephen Hemminger:
> Printing extra borders does not improve readability, and is just
> unnecessary. Putting TSC hz in header also makes no sense here.
The CPU frequency on headers!
OK to remove :)
Hi Stephen,
snipped
>
> 15/07/2020 23:22, Stephen Hemminger:
> > Printing extra borders does not improve readability, and is just
> > unnecessary. Putting TSC hz in header also makes no sense here.
>
> The CPU frequency on headers!
> OK to remove :)
>
The rational of having Time Stamp Counter as the header for port info and stats, is to account for each iteration for an average `packets per second`. That is using `watch -d -n 1 ./dpdk-procinfo -- --xstats | grep -v ": 0"`.
But if there better way to do this or add as new feature, +1 to remove.
On Thu, 13 Aug 2020 04:10:39 +0000
"Varghese, Vipin" <vipin.varghese@intel.com> wrote:
> Hi Stephen,
>
> snipped
> >
> > 15/07/2020 23:22, Stephen Hemminger:
> > > Printing extra borders does not improve readability, and is just
> > > unnecessary. Putting TSC hz in header also makes no sense here.
> >
> > The CPU frequency on headers!
> > OK to remove :)
> >
>
> The rational of having Time Stamp Counter as the header for port info and stats, is to account for each iteration for an average `packets per second`. That is using `watch -d -n 1 ./dpdk-procinfo -- --xstats | grep -v ": 0"`.
>
> But if there better way to do this or add as new feature, +1 to remove.
Proc info should just use standard clock for its updates, not TSC?
snipped
> >
> > snipped
> > >
> > > 15/07/2020 23:22, Stephen Hemminger:
> > > > Printing extra borders does not improve readability, and is just
> > > > unnecessary. Putting TSC hz in header also makes no sense here.
> > >
> > > The CPU frequency on headers!
> > > OK to remove :)
> > >
> >
> > The rational of having Time Stamp Counter as the header for port info and
> stats, is to account for each iteration for an average `packets per second`. That
> is using `watch -d -n 1 ./dpdk-procinfo -- --xstats | grep -v ": 0"`.
> >
> > But if there better way to do this or add as new feature, +1 to remove.
>
> Proc info should just use standard clock for its updates, not TSC?
Can you please explain the rationale behind the (syscall for time) and not TSC?
snipped
>
> snipped
> > >
> > > snipped
> > > >
> > > > 15/07/2020 23:22, Stephen Hemminger:
> > > > > Printing extra borders does not improve readability, and is just
> > > > > unnecessary. Putting TSC hz in header also makes no sense here.
> > > >
> > > > The CPU frequency on headers!
> > > > OK to remove :)
> > > >
> > >
> > > The rational of having Time Stamp Counter as the header for port
> > > info and
> > stats, is to account for each iteration for an average `packets per
> > second`. That is using `watch -d -n 1 ./dpdk-procinfo -- --xstats | grep -v ": 0"`.
> > >
> > > But if there better way to do this or add as new feature, +1 to remove.
> >
> > Proc info should just use standard clock for its updates, not TSC?
>
> Can you please explain the rationale behind the (syscall for time) and not TSC?
Looking forward for the patch with clock change too.
On Fri, 14 Aug 2020 01:14:40 +0000
"Varghese, Vipin" <vipin.varghese@intel.com> wrote:
> snipped
> >
> > snipped
> > > >
> > > > snipped
> > > > >
> > > > > 15/07/2020 23:22, Stephen Hemminger:
> > > > > > Printing extra borders does not improve readability, and is just
> > > > > > unnecessary. Putting TSC hz in header also makes no sense here.
> > > > >
> > > > > The CPU frequency on headers!
> > > > > OK to remove :)
> > > > >
> > > >
> > > > The rational of having Time Stamp Counter as the header for port
> > > > info and
> > > stats, is to account for each iteration for an average `packets per
> > > second`. That is using `watch -d -n 1 ./dpdk-procinfo -- --xstats | grep -v ": 0"`.
> > > >
> > > > But if there better way to do this or add as new feature, +1 to remove.
> > >
> > > Proc info should just use standard clock for its updates, not TSC?
> >
> > Can you please explain the rationale behind the (syscall for time) and not TSC?
>
> Looking forward for the patch with clock change too.
There is no part of what proc-info is show with xstats that displays or uses tsc directly.
Do you have a driver that is putting TSC information in xstats?
Hi Stephen,
>
> > snipped
> > >
> > > snipped
> > > > >
> > > > > snipped
> > > > > >
> > > > > > 15/07/2020 23:22, Stephen Hemminger:
> > > > > > > Printing extra borders does not improve readability, and is
> > > > > > > just unnecessary. Putting TSC hz in header also makes no sense
> here.
> > > > > >
> > > > > > The CPU frequency on headers!
> > > > > > OK to remove :)
> > > > > >
> > > > >
> > > > > The rational of having Time Stamp Counter as the header for port
> > > > > info and
> > > > stats, is to account for each iteration for an average `packets
> > > > per second`. That is using `watch -d -n 1 ./dpdk-procinfo -- --xstats | grep -
> v ": 0"`.
> > > > >
> > > > > But if there better way to do this or add as new feature, +1 to remove.
> > > >
> > > > Proc info should just use standard clock for its updates, not TSC?
> > >
> > > Can you please explain the rationale behind the (syscall for time) and not
> TSC?
> >
> > Looking forward for the patch with clock change too.
>
> There is no part of what proc-info is show with xstats that displays or uses tsc
> directly.
> Do you have a driver that is putting TSC information in xstats?
I am sorry I did not follow you on this, In the earlier implementation with `proc-info` header we had TSC so coupling `--show --xstats`. But with the header removed out with TSC, how can we get this?
On Fri, 14 Aug 2020 11:08:31 +0000
"Varghese, Vipin" <vipin.varghese@intel.com> wrote:
> Hi Stephen,
>
> >
> > > snipped
> > > >
> > > > snipped
> > > > > >
> > > > > > snipped
> > > > > > >
> > > > > > > 15/07/2020 23:22, Stephen Hemminger:
> > > > > > > > Printing extra borders does not improve readability, and is
> > > > > > > > just unnecessary. Putting TSC hz in header also makes no sense
> > here.
> > > > > > >
> > > > > > > The CPU frequency on headers!
> > > > > > > OK to remove :)
> > > > > > >
> > > > > >
> > > > > > The rational of having Time Stamp Counter as the header for port
> > > > > > info and
> > > > > stats, is to account for each iteration for an average `packets
> > > > > per second`. That is using `watch -d -n 1 ./dpdk-procinfo -- --xstats | grep -
> > v ": 0"`.
> > > > > >
> > > > > > But if there better way to do this or add as new feature, +1 to remove.
> > > > >
> > > > > Proc info should just use standard clock for its updates, not TSC?
> > > >
> > > > Can you please explain the rationale behind the (syscall for time) and not
> > TSC?
> > >
> > > Looking forward for the patch with clock change too.
> >
> > There is no part of what proc-info is show with xstats that displays or uses tsc
> > directly.
> > Do you have a driver that is putting TSC information in xstats?
>
> I am sorry I did not follow you on this, In the earlier implementation with `proc-info` header we had TSC so coupling `--show --xstats`. But with the header removed out with TSC, how can we get this?
I am asking why is the TSC hz a useful piece of information here?
Sure, proc-info could print lots of things but unless it serves a useful purpose
for examining the interfaces, it is just noise.
Developers love to clutter output, but it just raises the signal to noise ratio.
>
> > Hi Stephen,
> >
> > >
> > > > snipped
> > > > >
> > > > > snipped
> > > > > > >
> > > > > > > snipped
> > > > > > > >
> > > > > > > > 15/07/2020 23:22, Stephen Hemminger:
> > > > > > > > > Printing extra borders does not improve readability, and
> > > > > > > > > is just unnecessary. Putting TSC hz in header also makes
> > > > > > > > > no sense
> > > here.
> > > > > > > >
> > > > > > > > The CPU frequency on headers!
> > > > > > > > OK to remove :)
> > > > > > > >
> > > > > > >
> > > > > > > The rational of having Time Stamp Counter as the header for
> > > > > > > port info and
> > > > > > stats, is to account for each iteration for an average
> > > > > > `packets per second`. That is using `watch -d -n 1
> > > > > > ./dpdk-procinfo -- --xstats | grep -
> > > v ": 0"`.
> > > > > > >
> > > > > > > But if there better way to do this or add as new feature, +1 to
> remove.
> > > > > >
> > > > > > Proc info should just use standard clock for its updates, not TSC?
> > > > >
> > > > > Can you please explain the rationale behind the (syscall for
> > > > > time) and not
> > > TSC?
> > > >
> > > > Looking forward for the patch with clock change too.
> > >
> > > There is no part of what proc-info is show with xstats that displays
> > > or uses tsc directly.
> > > Do you have a driver that is putting TSC information in xstats?
> >
> > I am sorry I did not follow you on this, In the earlier implementation with
> `proc-info` header we had TSC so coupling `--show --xstats`. But with the
> header removed out with TSC, how can we get this?
>
> I am asking why is the TSC hz a useful piece of information here?
> Sure, proc-info could print lots of things but unless it serves a useful purpose
> for examining the interfaces, it is just noise.
>
> Developers love to clutter output, but it just raises the signal to noise ratio.
Thanks for the update, as mentioned earlier from my end `+1` for cleaning up the console output interface. Also `+1` for adding option to get timestamp too.
But I am still not able to understand the suggestions like
`1. Why Proc info should just use standard clock for its updates, not TSC?
2. CPU frequency in header?`
As I understand standard clock is syscall and TSC is register fetch for clock ticks after reset and not CPU frequency.
@@ -660,8 +660,7 @@ show_port(void)
uint16_t i = 0;
int ret = 0, j, k;
- snprintf(bdr_str, MAX_STRING_LEN, " show - Port PMD %"PRIu64,
- rte_get_tsc_hz());
+ snprintf(bdr_str, MAX_STRING_LEN, " show - Port PMD ");
STATS_BDR_STR(10, bdr_str);
RTE_ETH_FOREACH_DEV(i) {
@@ -673,7 +672,7 @@ show_port(void)
memset(&rss_conf, 0, sizeof(rss_conf));
- snprintf(bdr_str, MAX_STRING_LEN, " Port (%u)", i);
+ snprintf(bdr_str, MAX_STRING_LEN, " Port %u ", i);
STATS_BDR_STR(5, bdr_str);
printf(" - generic config\n");
@@ -754,8 +753,6 @@ show_port(void)
}
#endif
}
-
- STATS_BDR_STR(50, "");
}
static void
@@ -841,8 +838,7 @@ show_tm(void)
unsigned int j, k;
uint16_t i = 0;
- snprintf(bdr_str, MAX_STRING_LEN, " show - TM PMD %"PRIu64,
- rte_get_tsc_hz());
+ snprintf(bdr_str, MAX_STRING_LEN, " show - TM PMD ");
STATS_BDR_STR(10, bdr_str);
RTE_ETH_FOREACH_DEV(i) {
@@ -1038,8 +1034,6 @@ show_tm(void)
stats.leaf.n_bytes_dropped[RTE_COLOR_RED]);
}
}
-
- STATS_BDR_STR(50, "");
}
static void
@@ -1085,8 +1079,7 @@ show_crypto(void)
{
uint8_t crypto_dev_count = rte_cryptodev_count(), i;
- snprintf(bdr_str, MAX_STRING_LEN, " show - CRYPTO PMD %"PRIu64,
- rte_get_tsc_hz());
+ snprintf(bdr_str, MAX_STRING_LEN, " show - CRYPTO PMD ");
STATS_BDR_STR(10, bdr_str);
for (i = 0; i < crypto_dev_count; i++) {
@@ -1121,15 +1114,12 @@ show_crypto(void)
stats.dequeue_err_count);
}
}
-
- STATS_BDR_STR(50, "");
}
static void
show_ring(char *name)
{
- snprintf(bdr_str, MAX_STRING_LEN, " show - RING %"PRIu64,
- rte_get_tsc_hz());
+ snprintf(bdr_str, MAX_STRING_LEN, " show - RING ");
STATS_BDR_STR(10, bdr_str);
if (name != NULL) {
@@ -1160,7 +1150,6 @@ show_ring(char *name)
}
rte_ring_list_dump(stdout);
- STATS_BDR_STR(50, "");
}
static void
@@ -1168,8 +1157,7 @@ show_mempool(char *name)
{
uint64_t flags = 0;
- snprintf(bdr_str, MAX_STRING_LEN, " show - MEMPOOL %"PRIu64,
- rte_get_tsc_hz());
+ snprintf(bdr_str, MAX_STRING_LEN, " show - MEMPOOL ");
STATS_BDR_STR(10, bdr_str);
if (name != NULL) {
@@ -1206,13 +1194,11 @@ show_mempool(char *name)
rte_mempool_avail_count(ptr),
rte_mempool_in_use_count(ptr));
- STATS_BDR_STR(50, "");
return;
}
}
rte_mempool_list_dump(stdout);
- STATS_BDR_STR(50, "");
}
static void
@@ -1230,8 +1216,7 @@ mempool_itr_obj(struct rte_mempool *mp, void *opaque,
static void
iter_mempool(char *name)
{
- snprintf(bdr_str, MAX_STRING_LEN, " iter - MEMPOOL %"PRIu64,
- rte_get_tsc_hz());
+ snprintf(bdr_str, MAX_STRING_LEN, " iter - MEMPOOL ");
STATS_BDR_STR(10, bdr_str);
if (name != NULL) {
@@ -1241,12 +1226,9 @@ iter_mempool(char *name)
uint32_t ret = rte_mempool_obj_iter(ptr,
mempool_itr_obj, NULL);
printf("\n - iterated %u objects\n", ret);
- STATS_BDR_STR(50, "");
return;
}
}
-
- STATS_BDR_STR(50, "");
}
int
@@ -1347,8 +1329,5 @@ main(int argc, char **argv)
if (ret)
printf("Error from rte_eal_cleanup(), %d\n", ret);
- strlcpy(bdr_str, " ", MAX_STRING_LEN);
- STATS_BDR_STR(50, bdr_str);
-
return 0;
}