On Thu, Mar 19, 2020 at 3:33 PM Mattias Rönnblom
<mattias.ronnb...@ericsson.com> wrote:
>
> On 2020-03-18 20:02, jer...@marvell.com wrote:
> > From: Jerin Jacob <jer...@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.
