[v2,1/3] app/testpmd: add profiling flags set command
diff mbox series

Message ID 1584625851-10291-2-git-send-email-viacheslavo@mellanox.com
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • app/testpmd: qualify Rx/Tx profiling data on burst size
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Slava Ovsiienko March 19, 2020, 1:50 p.m. UTC
This commit is preparation step before adding the separated profiling
of Rx and Tx burst routines. The new testpmd command is introduced:

  set fwdprof (flags)

This command controls which profiling statistics is being gathered
in runtime:

- bit 0 - enables profiling the entire forward routine, counts the ticks
          spent in the forwarding routine, is set by default. Provides the
	  same data as previous implementation.

- bit 1 - enables gathering the profiling data for the transmit datapath,
          counts the ticks spent within rte_eth_tx_burst() routine,
          is cleared by default, extends the existing statistics.

- bit 2 - enables gathering the profiling data for the receive datapath,
          counts the ticks spent within rte_eth_rx_burst() routine,
          is cleared by default, extends the existing statistics.

The new counters for the cycles spent into rx/tx burst
routines are introduced. The feature is engaged only if
CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES configured to 'Y'.

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 app/test-pmd/cmdline.c                      | 15 +++++++++++++++
 app/test-pmd/config.c                       | 10 ++++++++++
 app/test-pmd/testpmd.c                      |  3 +++
 app/test-pmd/testpmd.h                      |  8 ++++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst | 22 ++++++++++++++++++++++
 5 files changed, 58 insertions(+)

Comments

Thomas Monjalon April 2, 2020, 11:15 a.m. UTC | #1
19/03/2020 14:50, Viacheslav Ovsiienko:
> +set fwdprof
> +~~~~~~~~~~~
> +
> +Set the flags controlling the datapath profiling statistics::
> +
> +   testpmd> set fwdprof (flags)
> +
> +This command is available only if ``CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES`` is
> +configured to Y, enabling the profiling code generation.
> +
> +The bitmask flag controls the gathering profiling statistics for datapath:
> +
> +* ``bit 0`` enables gathering the profiling data for the entire
> +  forwarding routine, counts the ticks spent in the forwarding routine,
> +  is set by default.
> +* ``bit 1`` enables gathering the profiling data for the transmit datapath,
> +  counts the ticks spent within rte_eth_tx_burst() routine, is cleared by
> +  default.
> +* ``bit 2`` enables gathering the profiling data for the receive datapath,
> +  counts the ticks spent within rte_eth_rx_burst() routine, is cleared by
> +  default.

As commented in the cover letter, please replace flags with Rx/Tx text tokens.
Ferruh Yigit April 9, 2020, 11:56 a.m. UTC | #2
On 3/19/2020 1:50 PM, Viacheslav Ovsiienko wrote:
> This commit is preparation step before adding the separated profiling
> of Rx and Tx burst routines. The new testpmd command is introduced:
> 
>   set fwdprof (flags)
> 
> This command controls which profiling statistics is being gathered
> in runtime:
> 
> - bit 0 - enables profiling the entire forward routine, counts the ticks
>           spent in the forwarding routine, is set by default. Provides the
> 	  same data as previous implementation.
> 
> - bit 1 - enables gathering the profiling data for the transmit datapath,
>           counts the ticks spent within rte_eth_tx_burst() routine,
>           is cleared by default, extends the existing statistics.
> 
> - bit 2 - enables gathering the profiling data for the receive datapath,
>           counts the ticks spent within rte_eth_rx_burst() routine,
>           is cleared by default, extends the existing statistics.
> 
> The new counters for the cycles spent into rx/tx burst
> routines are introduced. The feature is engaged only if
> CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES configured to 'Y'.

Hi Slava,

Thanks for improving the testpmd performance measuring, unfortunately these
features are not documented at all, unless I miss it, and now you are improving
it but still there is no documentation.

Would you mind adding a section for performance measures, document how to enable
and use it, and how to read the output?

> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

<...>

> +#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
> +void
> +set_fwdprof_flags(uint16_t pf_flags)
> +{
> +	printf("Change forward profiling flags from %u to %u\n",
> +	       (unsigned int) fwdprof_flags, (unsigned int) pf_flags);

To reduce the 'RTE_TEST_PMD_RECORD_CORE_CYCLES' define usage, what do you think
have it only in this function, if it is defined work as expected and if not
print an error and don't update anything?

<...>

> @@ -647,6 +647,28 @@ Regexes can also be used for type. To change log level of user1, user2 and user3
>  
>  	testpmd> set log user[1-3] (level)
>  
> +set fwdprof
> +~~~~~~~~~~~
> +
> +Set the flags controlling the datapath profiling statistics::
> +
> +   testpmd> set fwdprof (flags)

What do you think a little longer command 'fwdprofile", which is more clear I think.

And what about having another command to show existing value, right now it is
'set' only?
Slava Ovsiienko April 13, 2020, 7:56 a.m. UTC | #3
Hi, Ferruh

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, April 9, 2020 14:56
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Cc: Thomas Monjalon <thomas@monjalon.net>;
> bernard.iremonger@intel.com
> Subject: Re: [PATCH v2 1/3] app/testpmd: add profiling flags set command
> 
> On 3/19/2020 1:50 PM, Viacheslav Ovsiienko wrote:
> > This commit is preparation step before adding the separated profiling
> > of Rx and Tx burst routines. The new testpmd command is introduced:
> >
> >   set fwdprof (flags)
> >
> > This command controls which profiling statistics is being gathered in
> > runtime:
> >
> > - bit 0 - enables profiling the entire forward routine, counts the ticks
> >           spent in the forwarding routine, is set by default. Provides the
> > 	  same data as previous implementation.
> >
> > - bit 1 - enables gathering the profiling data for the transmit datapath,
> >           counts the ticks spent within rte_eth_tx_burst() routine,
> >           is cleared by default, extends the existing statistics.
> >
> > - bit 2 - enables gathering the profiling data for the receive datapath,
> >           counts the ticks spent within rte_eth_rx_burst() routine,
> >           is cleared by default, extends the existing statistics.
> >
> > The new counters for the cycles spent into rx/tx burst routines are
> > introduced. The feature is engaged only if
> > CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES configured to 'Y'.
> 
> Hi Slava,
> 
> Thanks for improving the testpmd performance measuring, unfortunately
> these features are not documented at all, unless I miss it, and now you are
> improving it but still there is no documentation.
> 
> Would you mind adding a section for performance measures, document how
> to enable and use it, and how to read the output?

OK, I'll update the documentation either and explain the new extended stats.

> 
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> <...>
> 
> > +#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES void
> > +set_fwdprof_flags(uint16_t pf_flags) {
> > +	printf("Change forward profiling flags from %u to %u\n",
> > +	       (unsigned int) fwdprof_flags, (unsigned int) pf_flags);
> 
> To reduce the 'RTE_TEST_PMD_RECORD_CORE_CYCLES' define usage, what do
> you think have it only in this function, if it is defined work as expected and if
> not print an error and don't update anything?
Agree, it would simplify, going to fix.

> 
> <...>
> 
> > @@ -647,6 +647,28 @@ Regexes can also be used for type. To change log
> > level of user1, user2 and user3
> >
> >  	testpmd> set log user[1-3] (level)
> >
> > +set fwdprof
> > +~~~~~~~~~~~
> > +
> > +Set the flags controlling the datapath profiling statistics::
> > +
> > +   testpmd> set fwdprof (flags)
> 
> What do you think a little longer command 'fwdprofile", which is more clear I
> think.
Yes, it is clearer, but longer to type 😊
If you insist on - I will extend, please, let me know your final opinion.

> 
> And what about having another command to show existing value, right now it
> is 'set' only?
Do you mean it would be good to add "show fwdpro/fwdprofile" ?

With best regards, Slava
Thomas Monjalon April 13, 2020, 12:23 p.m. UTC | #4
13/04/2020 09:56, Slava Ovsiienko:
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > +set fwdprof
> > > +~~~~~~~~~~~
> > > +
> > > +Set the flags controlling the datapath profiling statistics::
> > > +
> > > +   testpmd> set fwdprof (flags)
> > 
> > What do you think a little longer command 'fwdprofile", which is more clear I
> > think.
> Yes, it is clearer, but longer to type 😊
> If you insist on - I will extend, please, let me know your final opinion.

I agree with Ferruh in general and especially about "fwdprof" not nice to read.
What about fwd-profiling?
I would be also OK with fwd-prof.

Is it common to use hyphen sign in testpmd commands?
Ferruh Yigit April 14, 2020, 9:07 a.m. UTC | #5
On 4/13/2020 8:56 AM, Slava Ovsiienko wrote:
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, April 9, 2020 14:56
>> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
>> Cc: Thomas Monjalon <thomas@monjalon.net>;
>> bernard.iremonger@intel.com
>> Subject: Re: [PATCH v2 1/3] app/testpmd: add profiling flags set command
>>
>> On 3/19/2020 1:50 PM, Viacheslav Ovsiienko wrote:
>>> This commit is preparation step before adding the separated profiling
>>> of Rx and Tx burst routines. The new testpmd command is introduced:
>>>
>>>   set fwdprof (flags)
>>>
>>> This command controls which profiling statistics is being gathered in
>>> runtime:
>>>
>>> - bit 0 - enables profiling the entire forward routine, counts the ticks
>>>           spent in the forwarding routine, is set by default. Provides the
>>> 	  same data as previous implementation.
>>>
>>> - bit 1 - enables gathering the profiling data for the transmit datapath,
>>>           counts the ticks spent within rte_eth_tx_burst() routine,
>>>           is cleared by default, extends the existing statistics.
>>>
>>> - bit 2 - enables gathering the profiling data for the receive datapath,
>>>           counts the ticks spent within rte_eth_rx_burst() routine,
>>>           is cleared by default, extends the existing statistics.
>>>
>>> The new counters for the cycles spent into rx/tx burst routines are
>>> introduced. The feature is engaged only if
>>> CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES configured to 'Y'.
>>
>> Hi Slava,
>>
>> Thanks for improving the testpmd performance measuring, unfortunately
>> these features are not documented at all, unless I miss it, and now you are
>> improving it but still there is no documentation.
>>
>> Would you mind adding a section for performance measures, document how
>> to enable and use it, and how to read the output?
> 
> OK, I'll update the documentation either and explain the new extended stats.
> 
>>
>>>
>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
>>
>> <...>
>>
>>> +#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES void
>>> +set_fwdprof_flags(uint16_t pf_flags) {
>>> +	printf("Change forward profiling flags from %u to %u\n",
>>> +	       (unsigned int) fwdprof_flags, (unsigned int) pf_flags);
>>
>> To reduce the 'RTE_TEST_PMD_RECORD_CORE_CYCLES' define usage, what do
>> you think have it only in this function, if it is defined work as expected and if
>> not print an error and don't update anything?
> Agree, it would simplify, going to fix.
> 
>>
>> <...>
>>
>>> @@ -647,6 +647,28 @@ Regexes can also be used for type. To change log
>>> level of user1, user2 and user3
>>>
>>>  	testpmd> set log user[1-3] (level)
>>>
>>> +set fwdprof
>>> +~~~~~~~~~~~
>>> +
>>> +Set the flags controlling the datapath profiling statistics::
>>> +
>>> +   testpmd> set fwdprof (flags)
>>
>> What do you think a little longer command 'fwdprofile", which is more clear I
>> think.
> Yes, it is clearer, but longer to type 😊
> If you insist on - I will extend, please, let me know your final opinion.

In the scope of this patch it may look OK, but if you look all testpmd commands
without knowing the code, 'fwdprof' may not be clear what it is. I am for making
it more clear.

> 
>>
>> And what about having another command to show existing value, right now it
>> is 'set' only?
> Do you mean it would be good to add "show fwdpro/fwdprofile" ?

Yes.

Patch
diff mbox series

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index a037a55..74dbb29 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -263,6 +263,9 @@  static void cmd_help_long_parsed(void *parsed_result,
 			"set verbose (level)\n"
 			"    Set the debug verbosity level X.\n\n"
 
+			"set fwdprof (flags)\n"
+			"    Set the flags to profile the forwarding.\n\n"
+
 			"set log global|(type) (level)\n"
 			"    Set the log level.\n\n"
 
@@ -3743,20 +3746,32 @@  static void cmd_set_parsed(void *parsed_result,
 		set_nb_pkt_per_burst(res->value);
 	else if (!strcmp(res->what, "verbose"))
 		set_verbose_level(res->value);
+#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
+	else if (!strcmp(res->what, "fwdprof"))
+		set_fwdprof_flags(res->value);
+#endif
 }
 
 cmdline_parse_token_string_t cmd_set_set =
 	TOKEN_STRING_INITIALIZER(struct cmd_set_result, set, "set");
 cmdline_parse_token_string_t cmd_set_what =
 	TOKEN_STRING_INITIALIZER(struct cmd_set_result, what,
+#ifndef RTE_TEST_PMD_RECORD_CORE_CYCLES
 				 "nbport#nbcore#burst#verbose");
+#else
+				 "nbport#nbcore#burst#verbose#fwdprof");
+#endif
 cmdline_parse_token_num_t cmd_set_value =
 	TOKEN_NUM_INITIALIZER(struct cmd_set_result, value, UINT16);
 
 cmdline_parse_inst_t cmd_set_numbers = {
 	.f = cmd_set_parsed,
 	.data = NULL,
+#ifndef RTE_TEST_PMD_RECORD_CORE_CYCLES
 	.help_str = "set nbport|nbcore|burst|verbose <value>",
+#else
+	.help_str = "set nbport|nbcore|burst|verbose|fwdprof <value>",
+#endif
 	.tokens = {
 		(void *)&cmd_set_set,
 		(void *)&cmd_set_what,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 8cf84cc..1d86250 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3151,6 +3151,16 @@  struct igb_ring_desc_16_bytes {
 	configure_rxtx_dump_callbacks(verbose_level);
 }
 
+#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
+void
+set_fwdprof_flags(uint16_t pf_flags)
+{
+	printf("Change forward profiling flags from %u to %u\n",
+	       (unsigned int) fwdprof_flags, (unsigned int) pf_flags);
+	fwdprof_flags = pf_flags;
+}
+#endif
+
 void
 vlan_extend_set(portid_t port_id, int on)
 {
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 035836a..c93fa35 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -81,6 +81,9 @@ 
 #define EXTBUF_ZONE_SIZE RTE_PGSIZE_2M
 
 uint16_t verbose_level = 0; /**< Silent by default. */
+#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
+uint16_t fwdprof_flags = RECORD_CORE_CYCLES_FWD; /**< Fwd only by default. */
+#endif
 int testpmd_logtype; /**< Log type for testpmd logs */
 
 /* use master core for command line ? */
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 7a7c73f..466e611 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -321,8 +321,15 @@  struct queue_stats_mappings {
 
 extern uint8_t xstats_hide_zero; /**< Hide zero values for xstats display */
 
+#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
+#define RECORD_CORE_CYCLES_FWD (1<<0)
+#define RECORD_CORE_CYCLES_RX (1<<1)
+#define RECORD_CORE_CYCLES_TX (1<<2)
+#endif
+
 /* globals used for configuration */
 extern uint16_t verbose_level; /**< Drives messages being displayed, if any. */
+extern uint16_t fwdprof_flags; /**< Controls the profiling statistics. */
 extern int testpmd_logtype; /**< Log type for testpmd logs */
 extern uint8_t  interactive;
 extern uint8_t  auto_start;
@@ -787,6 +794,7 @@  void vlan_tpid_set(portid_t port_id, enum rte_vlan_type vlan_type,
 void set_xstats_hide_zero(uint8_t on_off);
 
 void set_verbose_level(uint16_t vb_level);
+void set_fwdprof_flags(uint16_t pf_flags);
 void set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs);
 void show_tx_pkt_segments(void);
 void set_tx_pkt_split(const char *name);
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 5bb12a5..b6ec783 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -647,6 +647,28 @@  Regexes can also be used for type. To change log level of user1, user2 and user3
 
 	testpmd> set log user[1-3] (level)
 
+set fwdprof
+~~~~~~~~~~~
+
+Set the flags controlling the datapath profiling statistics::
+
+   testpmd> set fwdprof (flags)
+
+This command is available only if ``CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES`` is
+configured to Y, enabling the profiling code generation.
+
+The bitmask flag controls the gathering profiling statistics for datapath:
+
+* ``bit 0`` enables gathering the profiling data for the entire
+  forwarding routine, counts the ticks spent in the forwarding routine,
+  is set by default.
+* ``bit 1`` enables gathering the profiling data for the transmit datapath,
+  counts the ticks spent within rte_eth_tx_burst() routine, is cleared by
+  default.
+* ``bit 2`` enables gathering the profiling data for the receive datapath,
+  counts the ticks spent within rte_eth_rx_burst() routine, is cleared by
+  default.
+
 set nbport
 ~~~~~~~~~~