Re: [PATCH v5 1/2] node: add global node mbuf dynfield

[Nitin Saxena <nsaxena16@gmail.com>]

Hi Pavan,

I have incorporated your comments in patch 6
Thanks for reviewing

Nitin

On Sat, Apr 19, 2025 at 12:33 AM Pavan Nikhilesh Bhagavatula
<pbhagavatula@marvell.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Nitin Saxena <nsaxena@marvell.com>
> > Sent: Wednesday, April 9, 2025 7:26 PM
> > To: Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>; Pavan Nikhilesh
> > Bhagavatula <pbhagavatula@marvell.com>; Robin Jarry
> > <rjarry@redhat.com>; Christophe Fontaine <cfontain@redhat.com>
> > Cc: dev@dpdk.org; Jerin Jacob <jerinj@marvell.com>; Nitin Saxena
> > <nsaxena16@gmail.com>
> > Subject: [PATCH v5 1/2] node: add global node mbuf dynfield
> >
> > This patch defines rte_node specific dynamic field structure
> > (rte_node_mbuf_dynfield_t)
> >
> > rte_node_mbuf_dynfield_t structure holds two types of fields
> > - Persistent data fields which are preserved across graph walk.
> >   Currently size of persistent data fields is zero.
> > - Overloadable data fields which are used by any two adjacent nodes.
> >   Same fields can be repurposed by any other adjacent nodes
> >
> > This dynfield can be also be used by out-of-tree nodes.
> >
> > Signed-off-by: Nitin Saxena <nsaxena@marvell.com>
> > ---
> >  doc/api/doxy-api-index.md              |   3 +-
> >  doc/guides/rel_notes/release_25_07.rst |   6 ++
> >  lib/node/meson.build                   |   2 +
> >  lib/node/node_mbuf_dynfield.c          |  57 +++++++++++
> >  lib/node/rte_node_mbuf_dynfield.h      | 132
> > +++++++++++++++++++++++++
> >  5 files changed, 199 insertions(+), 1 deletion(-)
> >  create mode 100644 lib/node/node_mbuf_dynfield.c
> >  create mode 100644 lib/node/rte_node_mbuf_dynfield.h
> >
> > diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> > index 5c425a2cb9..763cfb3f3c 100644
> > --- a/doc/api/doxy-api-index.md
> > +++ b/doc/api/doxy-api-index.md
> > @@ -219,7 +219,8 @@ The public API headers are grouped by topics:
> >      [ip4_node](@ref rte_node_ip4_api.h),
> >      [ip6_node](@ref rte_node_ip6_api.h),
> >      [udp4_input_node](@ref rte_node_udp4_input_api.h)
> > -
> > +  * graph_nodes_mbuf:
> > +    [node_mbuf_dynfield](@ref rte_node_mbuf_dynfield.h)
>
> Missing blank line here.
Done

>
> >  - **basic**:
> >    [bitops](@ref rte_bitops.h),
> >    [approx fraction](@ref rte_approx.h),
> > diff --git a/doc/guides/rel_notes/release_25_07.rst
> > b/doc/guides/rel_notes/release_25_07.rst
> > index 093b85d206..2dc888e65d 100644
> > --- a/doc/guides/rel_notes/release_25_07.rst
> > +++ b/doc/guides/rel_notes/release_25_07.rst
> > @@ -55,6 +55,12 @@ New Features
> >       Also, make sure to start the actual text at the margin.
> >       =======================================================
> >
> > +* **Added rte_node specific global mbuf dynamic field.**
> > +
> > +  Instead each node registering mbuf dynamic field for its own purpose, a
> > +  global structure is added which can be used/overloaded by all nodes
> > +  (including out-of-tree nodes). This minimizes footprint of node specific
> > mbuf
> > +  dynamic field.
> >
> >  Removed Items
> >  -------------
> > diff --git a/lib/node/meson.build b/lib/node/meson.build
> > index 0bed97a96c..152fe41129 100644
> > --- a/lib/node/meson.build
> > +++ b/lib/node/meson.build
> > @@ -8,6 +8,7 @@ if is_windows
> >  endif
> >
> >  sources = files(
> > +        'node_mbuf_dynfield.c',
> >          'ethdev_ctrl.c',
> >          'ethdev_rx.c',
> >          'ethdev_tx.c',
> > @@ -30,6 +31,7 @@ headers = files(
> >          'rte_node_ip4_api.h',
> >          'rte_node_ip6_api.h',
> >          'rte_node_udp4_input_api.h',
> > +        'rte_node_mbuf_dynfield.h',
> >  )
> >
> >  # Strict-aliasing rules are violated by uint8_t[] to context size casts.
> > diff --git a/lib/node/node_mbuf_dynfield.c b/lib/node/node_mbuf_dynfield.c
> > new file mode 100644
> > index 0000000000..6005e72ed6
> > --- /dev/null
> > +++ b/lib/node/node_mbuf_dynfield.c
> > @@ -0,0 +1,57 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2025 Marvell International Ltd.
> > + */
> > +#include <rte_memzone.h>
> > +#include <rte_node_mbuf_dynfield.h>
> > +#include <node_private.h>
> > +#include <eal_export.h>
> > +
> > +#define NODE_MBUF_DYNFIELD_MEMZONE_NAME
> > "__rte_node_mbuf_dynfield"
> > +
> > +struct node_mbuf_dynfield_mz {
> > +     int dynfield_offset;
> > +};
> > +
> > +static const struct rte_mbuf_dynfield node_mbuf_dynfield_desc = {
> > +     .name = "rte_node_mbuf_dynfield",
> > +     .size = sizeof(rte_node_mbuf_dynfield_t),
> > +     .align = alignof(rte_node_mbuf_dynfield_t),
> > +};
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_node_mbuf_dynfield_register,
> > 25.07);
> > +int rte_node_mbuf_dynfield_register(void)
> > +{
> > +     struct node_mbuf_dynfield_mz *f = NULL;
> > +     const struct rte_memzone *mz = NULL;
> > +     int dyn_offset;
> > +
> > +     RTE_BUILD_BUG_ON(sizeof(rte_node_mbuf_dynfield_t) <
> > RTE_NODE_MBUF_DYNFIELD_SIZE);
> > +     RTE_BUILD_BUG_ON(sizeof(rte_node_mbuf_overload_fields_t) <
> > +                      RTE_NODE_MBUF_OVERLOADABLE_FIELDS_SIZE);
> > +
> > +     mz =
> > rte_memzone_lookup(NODE_MBUF_DYNFIELD_MEMZONE_NAME);
> > +
> > +     if (!mz) {
> > +             mz =
> > rte_memzone_reserve_aligned(NODE_MBUF_DYNFIELD_MEMZONE_NAME,
> > +                                              sizeof(struct
> > node_mbuf_dynfield_mz),
> > +                                              SOCKET_ID_ANY, 0,
> > +                                              RTE_CACHE_LINE_SIZE);
> > +             if (!mz) {
> > +                     node_err("node_mbuf_dyn",
> > "rte_memzone_reserve_aligned failed");
> > +                     return -1;
>
> Please return proper error codes, or set rte_errno.
Done

>
> > +             }
> > +             dyn_offset =
> > rte_mbuf_dynfield_register(&node_mbuf_dynfield_desc);
> > +             if (dyn_offset < 0) {
> > +                     node_err("node_mbuf_dyn",
> > "rte_mbuf_dynfield_register failed");
> > +                     return -1;
>
> Please return proper error codes, or set rte_errno.
Done

>
> > +             }
> > +             f = (struct node_mbuf_dynfield_mz *)mz->addr;
> > +             f->dynfield_offset = dyn_offset;
> > +
> > +             node_dbg("node_mbuf_dyn", "memzone: %s of size: %zu at
> > offset : %d",
> > +                      mz->name, mz->len, f->dynfield_offset);
> > +     } else {
> > +             f = (struct node_mbuf_dynfield_mz *)mz->addr;
> > +     }
> > +     return f->dynfield_offset;
> > +}
> > diff --git a/lib/node/rte_node_mbuf_dynfield.h
> > b/lib/node/rte_node_mbuf_dynfield.h
> > new file mode 100644
> > index 0000000000..b5a3d6db3f
> > --- /dev/null
> > +++ b/lib/node/rte_node_mbuf_dynfield.h
> > @@ -0,0 +1,132 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2025 Marvell International Ltd.
> > + */
> > +
> > +#ifndef _RTE_GRAPH_MBUF_DYNFIELD_H_
> > +#define _RTE_GRAPH_MBUF_DYNFIELD_H_
> > +
> > +#include <rte_common.h>
> > +#include <rte_mbuf.h>
> > +#include <rte_mbuf_dyn.h>
> > +
> > +/**
> > + * @file: rte_node_mbuf_dynfield.h
> > + *
>
> Please mark the file as experimental
> i.e.,
>  * @warning
>  * @b EXPERIMENTAL:
Done

>
> > + * Defines rte_node specific mbuf dynamic field region
> > [rte_node_mbuf_dynfield_t] which
> > + * can used by both DPDK in-built and out-of-tree nodes for storing per-mbuf
> > + * fields for graph walk
> > + */
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/** Size of persistent mbuf fields */
> > +#define RTE_NODE_MBUF_PERSISTENT_FIELDS_SIZE          (0)
> > +/** Size of overloadable mbuf fields */
> > +#define RTE_NODE_MBUF_OVERLOADABLE_FIELDS_SIZE        (8)
> > +/** Size of node mbuf dynamic field */
> > +#define RTE_NODE_MBUF_DYNFIELD_SIZE     \
> > +     (RTE_NODE_MBUF_PERSISTENT_FIELDS_SIZE +
> > RTE_NODE_MBUF_OVERLOADABLE_FIELDS_SIZE)
>
> New line here.
Done

>
> > +/**
> > + * Node mbuf overloadable data.
> > + *
> > + * Out-of-tree nodes can repurpose overloadable fields via
> > + * rte_node_mbuf_overload_fields_get(mbuf). Overloadable fields are not
> > + * preserved and typically can be used with-in two adjacent nodes in the
> > graph.
> > + */
> > +typedef struct rte_node_mbuf_overload_fields {
> > +     union {
> > +             uint8_t
> > data[RTE_NODE_MBUF_OVERLOADABLE_FIELDS_SIZE];
> > +     };
> > +} rte_node_mbuf_overload_fields_t;
> > +
> > +/**
> > + * rte_node specific mbuf dynamic field structure
> > [rte_node_mbuf_dynfield_t]
> > + *
> > + * It holds two types of fields:
> > + * 1. Persistent fields: Fields which are preserved across nodes during graph
> > walk.
> > + *    - Eg: rx/tx interface etc
> > + * 2. Overloadable fields: Fields which can be repurposed by two adjacent
> > nodes.
> > + */
> > +typedef struct rte_node_mbuf_dynfield {
> > +     /**
> > +      * Persistent mbuf region across nodes in graph walk
> > +      *
> > +      * These fields are preserved across graph walk and can be accessed by
> > +      * rte_node_mbuf_dynfield_get() in fast path.
> > +      */
> > +     union {
> > +             uint8_t
> > persistent_data[RTE_NODE_MBUF_PERSISTENT_FIELDS_SIZE];
> > +     };
> > +
> > +     /**
> > +      * Overloadable mbuf fields across graph walk. Fields which can
> > change
> > +      *
> > +      * Two adjacent nodes (producer, consumer) can use this memory
> > region for
> > +      * sharing per-mbuf specific information. Once mbuf leaves a
> > consumer node,
> > +      * this region can be repurposed by another sets of [producer,
> > consumer] node.
> > +      *
> > +      * In fast path, this region can be accessed by
> > rte_node_mbuf_overload_fields_get()
> > +      */
> > +     rte_node_mbuf_overload_fields_t overloadable_data;
> > +} rte_node_mbuf_dynfield_t;
> > +
> > +/**
> > + * For a given mbuf and dynamic offset, return pointer to
> > rte_node_mbuf_dynfield_t *
> > + *
> > + * @param m
> > + *   Mbuf
> > + * @param offset
> > + *   Dynamic offset returned by @ref rte_node_mbuf_dynfield_register()
>
> Missing return field in doxygen.
Done

>
> > + */
> > +__rte_experimental
> > +static __rte_always_inline rte_node_mbuf_dynfield_t *
> > +rte_node_mbuf_dynfield_get(struct rte_mbuf *m, const int offset)
> > +{
> > +     return RTE_MBUF_DYNFIELD(m, offset, struct
> > rte_node_mbuf_dynfield *);
> > +}
> > +
> > +/**
> > + * For a given mbuf and dynamic offset, return pointer to overloadable fields
> > + * Nodes can typecast returned pointer to reuse for their own purpose
> > + *
> > + * @param m
> > + *   Mbuf
> > + * @param offset
> > + *   Dynamic offset returned by @ref rte_node_mbuf_dynfield_register()
>
> Missing return field.
Done

>
> > + */
> > +__rte_experimental
> > +static __rte_always_inline rte_node_mbuf_overload_fields_t *
> > +rte_node_mbuf_overload_fields_get(struct rte_mbuf *m, const int offset)
> > +{
> > +     struct rte_node_mbuf_dynfield *f = NULL;
>
> rte_node_mbuf_dynfield_t
Done

>
> > +
> > +     f = RTE_MBUF_DYNFIELD(m, offset, struct rte_node_mbuf_dynfield
> > *);
>
> rte_node_mbuf_dynfield_t
Done

>
> > +
> > +     return &(f->overloadable_data);
> > +}
> > +
> > +/**
> > + *  Register rte_node specific common mbuf dynamic field region. Can be
> > called
> > + *  in rte_node_register()->init() function to save offset in node->ctx
> > + *
> > + *  In process() function, node->ctx can be passed to
> > + *  - rte_node_mbuf_dynfield_get(mbuf, offset)
> > + *  - rte_node_mbuf_overload_fields_get(mbuf, offset)
> > + *
> > + *  Can be called multiple times by any number of nodes in init() function.
> > + *  - Very first call registers dynamic field and returns offset
> > + *  - Subsequent calls return same offset
> > + *
> > + *  @return
> > + *   -1 on error
>
> < 0 on error.
> Please return and document proper error codes.
>
Done

> > + *   dynamic field offset on success
> > + */
> > +__rte_experimental
> > +int rte_node_mbuf_dynfield_register(void);
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif
> > --
> > 2.43.0
>