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

[Jerin Jacob]

On Thu, Mar 19, 2020 at 3:33 PM Mattias Rönnblom
<mattias.ronnblom at ericsson.com> wrote:
>
> On 2020-03-18 20:02, jerinj at marvell.com wrote:
> > From: Jerin Jacob <jerinj at marvell.com>
> >

> > +int
> > +__rte_trace_point_register(rte_trace_t handle, const char *name, uint32_t level,
> > +                      void (*fn)(void))
> Maybe a more descriptive name than 'fn' would be in order.

OK. I will change to "register_fn" in v2.


> > +{
> > +     char *field = RTE_PER_LCORE(ctf_field);
> > +     struct trace_point *tp;
> > +     uint16_t sz;
> > +
> > +     /* Sanity checks of arguments */
> > +     if (name == NULL || fn == NULL || handle == NULL) {
> > +             trace_err("invalid arguments");
> > +             rte_errno = EINVAL; goto fail;
> > +     }
> > +
> > +     /* Sanity check of level */
> > +     if (level > RTE_LOG_DEBUG || level > UINT8_MAX) {
>
> Consider a #define for the max level. If the type was uint8_t, you
> wouldn't need to check max at all.

The reason for keeping level as uint32_t to keep compatibility
with rte_log level datatype. For some reason, rte_log defined level
as uint32_t, So I thought of sticking to that so that we can migrate the
rte_log to rte_trace in future if needed and also making semantics
similar to rte_log.

>
> > +             trace_err("invalid log level=%d", level);
> > +             rte_errno = EINVAL; goto fail;
> > +
> > +     }
> > +
> > +     /* Check the size of the trace point object */
> > +     RTE_PER_LCORE(trace_point_sz) = 0;
> > +     RTE_PER_LCORE(ctf_count) = 0;
> > +     fn();
> > +     if (RTE_PER_LCORE(trace_point_sz) == 0) {
> > +             trace_err("missing rte_trace_emit_header() in register fn");
> > +             rte_errno = EBADF; goto fail;
> > +     }
> > +
> > +     /* Is size overflowed */
> > +     if (RTE_PER_LCORE(trace_point_sz) > UINT16_MAX) {
> > +             trace_err("trace point size overflowed");
> > +             rte_errno = ENOSPC; goto fail;
> > +     }
> > +
> > +     /* Are we running out of space to store trace points? */
> > +     if (trace.nb_trace_points > UINT16_MAX) {
> > +             trace_err("trace point exceeds the max count");
> > +             rte_errno = ENOSPC; goto fail;
> > +     }
> > +
> > +     /* Get the size of the trace point */
> > +     sz = RTE_PER_LCORE(trace_point_sz);
> > +     tp = calloc(1, sizeof(struct trace_point));
> Not rte_zmalloc()? Are secondary processes accessing this memory?

This been called by the constructor at that time memory
services are not enabled and it is for the per-process like rte_log
scheme.


> > +     if (tp == NULL) {
> > +             trace_err("fail to allocate trace point memory");
> > +             rte_errno = ENOMEM; goto fail;
> Missing newline.

I will fix it in v2.

> > +     }
> > +
> > +     /* 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
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.