Re: [dpdk-dev] [PATCH v1 03/32] eal/trace: implement trace register API

[Jerin Jacob <jerinjacobk@gmail.com>]

On Mon, Mar 23, 2020 at 8:13 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> On 2020-03-23 14:37, Jerin Jacob wrote:
> >>> +     }
> >>> +
> >>> +     /* Initialize the trace point */
> >>> +     if (rte_strscpy(tp->name, name, TRACE_POINT_NAME_SIZE) < 0) {
> >>> +             trace_err("name is too long");
> >>> +             rte_errno = E2BIG;
> >>> +             goto free;
> >>> +     }
> >>> +
> >>> +     /* Copy the field data for future use */
> >>> +     if (rte_strscpy(tp->ctf_field, field, TRACE_CTF_FIELD_SIZE) < 0) {
> >>> +             trace_err("CTF field size is too long");
> >>> +             rte_errno = E2BIG;
> >>> +             goto free;
> >>> +     }
> >>> +
> >>> +     /* Clear field memory for the next event */
> >>> +     memset(field, 0, TRACE_CTF_FIELD_SIZE);
> >>> +
> >>> +     /* Form the trace handle */
> >>> +     *handle = sz;
> >>> +     *handle |= trace.nb_trace_points << __RTE_TRACE_FIELD_ID_SHIFT;
> >>> +     *handle |= (uint64_t)level << __RTE_TRACE_FIELD_LEVEL_SHIFT;
> >> If *handle would be a struct, you could use a bitfield instead, and much
> >> simplify this code.
> > I thought that initially, Two reasons why I did not do that
> > 1) The flags have been used in fastpath, I prefer to work with flags
> > in fastpath so that
> Is it really that obvious that flags are faster than bitfield
> operations? I think most modern architectures have machine instructions
> for bitfield manipulation.

Add x86 maintainers.

There were comments in ml about bitfield inefficiency usage with x86.

http://patches.dpdk.org/patch/16482/

Search for: Bitfileds are efficient on Octeon. What's about other CPUs
you have in
mind? x86 is not as efficient.

Thoughts from x86 folks.

> > there is no performance impact using bitfields from the compiler _if any_.
> > 2) In some of the places, I can simply operate on APIs like
> > __atomic_and_fetch() with flags.
>
> I think you may still use such atomic operations. Just convert the
> struct to a uint64_t, which will essentially be a no-operation, and fire
> away.

Not sure, We think about the atomic "and" and fetch here.
That memcpy may translate additional load/store based on the compiler
optimization level.(say compiled with -O0)

>
>
> static uint64_t
>
> __rte_trace_raw(struct trace *t)
>
> {
>
>      uint64_t raw;
>
>      memcpy(&raw, t, sizeof(struct trace));
>
>      return raw;
>
> }
>
>