Re: [EXT] [PATCH 4/8] trace: fix dynamically enabling trace points

[David Marchand <david.marchand@redhat.com>]

On Thu, Sep 22, 2022 at 1:18 PM Sunil Kumar Kori <skori@marvell.com> wrote:
> >  int
> > -rte_trace_point_disable(rte_trace_point_t *trace)
> > +rte_trace_point_disable(rte_trace_point_t *t)
> >  {
> > -     if (trace_point_is_invalid(trace))
> > +     uint64_t prev;
> > +
> > +     if (trace_point_is_invalid(t))
> >               return -ERANGE;
> >
> > -     __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
> > -             __ATOMIC_RELEASE);
> > +     prev = __atomic_fetch_and(t, ~__RTE_TRACE_FIELD_ENABLE_MASK,
> > __ATOMIC_RELEASE);
> > +     if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) != 0)
> > +             __atomic_sub_fetch(&trace.status, 1, __ATOMIC_RELEASE);
> >       return 0;
> >  }
> >
> IMO, above change should go as a separate commit as it just replaces the variable name.

The count of enabled tracepoints (stored in the 'trace' global
variable) must be updated as part of the change.
So the renaming is necessary.


[snip]

> > @@ -375,6 +380,10 @@ trace_meta_save(struct trace *trace)
> >       FILE *f;
> >       int rc;
> >
> > +     rc = trace_mkdir();
> > +     if (rc < 0)
> > +             return rc;
> > +
>
> Trace directory is being created here and in trace_mem_save() function along with the logic to handle whether it is already done or not.
> Instead can't it be called in rte_trace_save() directly. That will suffice the intention, I believe.

Oh indeed, I had the false impression that there were other path than
rte_trace_save(), leading to trace_mkdir().
Will fix.


-- 
David Marchand