diff mbox series

[v3,2/7] app/proc-info: eliminate useless borders

Message ID 20200715212228.28010-3-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series app/proc-info enhancments |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger July 15, 2020, 9:22 p.m. UTC 
  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

Thomas Monjalon July 17, 2020, 2:06 p.m. UTC | #1 
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 :)
Varghese, Vipin Aug. 13, 2020, 4:10 a.m. UTC | #2 
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.
Stephen Hemminger Aug. 13, 2020, 4:11 p.m. UTC | #3 
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?
Varghese, Vipin Aug. 14, 2020, 12:45 a.m. UTC | #4 
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?
Varghese, Vipin Aug. 14, 2020, 1:14 a.m. UTC | #5 
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.
Stephen Hemminger Aug. 14, 2020, 3:18 a.m. UTC | #6 
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?
Varghese, Vipin Aug. 14, 2020, 11:08 a.m. UTC | #7 
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?
Stephen Hemminger Aug. 14, 2020, 2:56 p.m. UTC | #8 
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.
Varghese, Vipin Aug. 15, 2020, 2:48 a.m. UTC | #9 
> 
> > 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.
diff mbox series

Patch

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 69e3000616c0..049818bad51f 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -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;
 }