Re: [dpdk-dev] [PATCH v5 00/33] DPDK Trace support

[Jerin Jacob <jerinjacobk@gmail.com>]

On Wed, Apr 15, 2020 at 6:56 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> Thanks Jerin for this new version.
> New round of comments.

Thanks for the review.

>
> - What do you think of splitting the API in two headers, thinking
> about who will use them?
> * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for
> users of the trace framework that want to
>  * get the status of the whole trace subsystem,
>  * enable/disable tracepoints by pattern/regexp,
>  * dump the current events,
> * rte_tracepoint.h (rte_tracepoint_ prefix for all
> functions/macros/types) for developers that want to add tracepoints to
> their code

# Initially, I thought of doing the same.
Later realized that some of the definitions such as following

1)
/** The trace object. The trace APIs are based on this opaque object. */
typedef uint64_t rte_trace_t;

2) rte_trace_fp_is_enabled()

Used in both the header file. i.e rte_tracepoint.h needed to include
rte_trace.h.
If that's is the case, I don't see any value.

# Regarding the API change the following to rte_tracepoint_*

#define rte_trace_ctf_u64(val)
#define rte_trace_ctf_i64(val)
#define rte_trace_ctf_u32(val)
#define rte_trace_ctf_i32(val)
#define rte_trace_ctf_u16(val)
#define rte_trace_ctf_i16(val)
#define rte_trace_ctf_u8(val)
#define rte_trace_ctf_i8(val)
#define rte_trace_ctf_int(val)
#define rte_trace_ctf_long(val)
#define rte_trace_ctf_float(val)
#define rte_trace_ctf_double(val)
#define rte_trace_ctf_ptr(val)
#define rte_trace_ctf_string(val)

It could be done. Just concerned the length of API will be more. like
rte_trace_point_ctf_u64
If you have a strong opinion on this then I can change it.


>
>
> - Having functions "is_disabled" has little value when a "is_enabled"
> counterpart exists.

I thought so. I like to have "is_enabled". But in the code,
"is_disabled" has been used more.
Then finally added both in API. Please share your preference, I will
do the same in v6.


>
>
> - What is the value of having a _public_ rte_trace_is_invalid() ?
> A final user would need to lookup by name to get a trace descriptor
> and we should hope that such a lookup returns a valid descriptor :-).
> A developer would already have the descriptor hook point in his own
> code: by construction, if the tracepoint is registered, then the
> descriptor is valid, else, it is unknown.

Yep. I will make rte_trace_is_invalid as internal trace_is_invalid() API.

>
>
> - I did not get why we put the trace descriptors in a specific elf
> section, can you explain the benefits?

Those are global variables of size 8B. Since it is used in fastpath.
I just want all of them to same area for
1) It is a mostly a read-only area. Not mix with other "write" global variables
2) Chances that same subsystem FP variables comes same fastpath cache line.
In short, We will be more predictable performance number from build to build i.e
not depended on other global variables from build to build.

>
> - I can see no protection on the tracepoint list. Could we have issues
> with control/application threads that dpdk does not control, dynamic
> loading of libraries.. ?

Based on my understanding, all constructors in the .so file should be
called in eal_plugins_init()
one by one. So there is no synchronization issue. eal_trace_init()
which is called after
eal_plugins_init(). So all the .so files with constructor should be
called in eal_plugins_init()
be it DPDK shared lib or non DPDK shared lib.
rte_log_register() does the similar thing.


>
>
> - Following comment on v4 and the removal of the mode per tracepoint
> api, don't we need to put the current select mode in each tracepoint
> descriptor when registering a trace point ?

RTE_TRACE_POINT_REGISTER has only trace and name.
RTE_TRACE_POINT_REGISTER(trace, name)

Not sure why we need to add "mode" in the trace register.


Reason for adding in the descriptor we discussed last time.
---------------------------------------------------------------

> In fastpath, I tried to avoid reading multiple control variables to
> improve performance.
> i.e the tracepoint(uint64_t) has all control information. So it has
> given a feature to control mode per tracepoint.
> I thought it may be useful. No strong option here.
>
> If you think, it is not usefull and creates more problems, I will
> change to the following:
>
> 1) Remove int rte_trace_mode_set(rte_trace_t trace, enum rte_trace_mode_e mode);
> 2) Change
> From:
> void rte_trace_global_mode_set(enum rte_trace_mode_e mode);
> to:
> void rte_trace_mode_set(enum rte_trace_mode_e mode);

Yes, you can keep the information in the tracepoint descriptor and
expose a single api that impacts all tracepoints.
+1 for me.
--------------------------------------------------------------------




>
>
> --
> David Marchand
>