Re: [PATCH v2 1/5] app/testpmd: add trace save command

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 6/23/2023 9:00 AM, Slava Ovsiienko wrote:
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Wednesday, June 21, 2023 2:16 PM
>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org; Aman Singh
>> <aman.deep.singh@intel.com>
>> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>> Subject: Re: [PATCH v2 1/5] app/testpmd: add trace save command
>>
>> On 6/13/2023 5:58 PM, Viacheslav Ovsiienko wrote:
>>> The "save_trace" CLI command is added to trigger saving the trace
>>> dumps to the trace directory.
>>>
>>
>> Hi Viacheslav,
>>
>> Trace is already saved when dpdk application terminated, I guess this is to
>> save the trace before exiting the application, what is the use case for this, can
>> you please detail in the commit log.
> 
> OK, will update the commit log. The command "save_trace" is useful in some
> dynamic debug scenarios to save the trace without restarting the entire application.
> 
>>
>> And what happens if this is called multiple times, or what happens on the
>> application exit, will it overwrite the file or fail?
> It overwrites.
> 
>> Again please explain in the commit log.
> Sure, will do.
> 
>>
>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>> ---
>>>  app/test-pmd/cmdline.c | 38 ++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 38 insertions(+)
>>>
>>
>> Can you please update documentation too?
>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>> a15a442a06..db71ce2028 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -39,6 +39,7 @@
>>>  #include <rte_gro.h>
>>>  #endif
>>>  #include <rte_mbuf_dyn.h>
>>> +#include <rte_trace.h>
>>>
>>>  #include <cmdline_rdline.h>
>>>  #include <cmdline_parse.h>
>>> @@ -12745,6 +12746,40 @@ static cmdline_parse_inst_t
>> cmd_config_tx_affinity_map = {
>>>  	},
>>>  };
>>>
>>> +#ifndef RTE_EXEC_ENV_WINDOWS
>>> +/* *** SAVE_TRACE *** */
>>> +
>>> +struct cmd_save_trace_result {
>>> +	cmdline_fixed_string_t save;
>>> +};
>>> +
>>> +static void cmd_save_trace_parsed(__rte_unused void *parsed_result,
>>> +				  __rte_unused struct cmdline *cl,
>>> +				  __rte_unused void *data)
>>> +{
>>> +	int rc;
>>> +
>>> +	rc = rte_trace_save();
>>> +	if (rc)
>>> +		printf("Save trace failed with error: %d\n", rc);
>>> +	else
>>> +		printf("Trace saved successfully\n"); }
>>> +
>>> +static cmdline_parse_token_string_t cmd_save_trace_save =
>>> +	TOKEN_STRING_INITIALIZER(struct cmd_save_trace_result, save,
>>> +"save_trace");
>>> +
>>
>> We have dump_* commands, what do you think to have 'dump_trace'
>> command for this?
> It was initially (in v1) with "dump_trace" command.
> And there is the comment by Jerin:
> https://inbox.dpdk.org/dev/CALBAE1Of79a_jHnFT3KX--Enhud-h5RzL02TMQBsmoW721ds7A@mail.gmail.com/#t
> 
> So, I have changed to "save_trace". I have no strong opinion about command name, any allowing trace save is OK for me.
> 

Ah, I missed that.


@Jerin,
I just saw your comment, agree more exact action can be 'save' but
'dump' also describes enough.
Since there are existing 'dump_*' commands, it makes command more
intuitive and easy to remember.

As an active user of testpmd myself, I am finding it hard to
remember/find the command I need as number of commands increased. That
is why I am paying extra attention to have more hierarchical, consistent
and intuitive commands.

For me "dump_trace" works better in that manner, what do you think, do
you have strong opinion on 'save_trace'?