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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Fri, Jun 23, 2023 at 5:23 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> 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'?

dump_* commands dumping on stdout or FILE.
Trace is mostly saving "current trace buffer" it and internally it
figure out the FILE.
But no strong opinion, if testpmd user thinks "dump" is better.


>