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
<pbhagavat...@marvell.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Nitin Saxena <nsax...@marvell.com>
> > Sent: Wednesday, April 9, 2025 7:26 PM
> > To: Nithin Kumar Dabilpuram <ndabilpu...@marvell.com>; Pavan Nikhilesh
> > Bhagavatula <pbhagavat...@marvell.com>; Robin Jarry
> > <rja...@redhat.com>; Christophe Fontaine <cfont...@redhat.com>
> > Cc: dev@dpdk.org; Jerin Jacob <jer...@marvell.com>; Nitin Saxena
> > <nsaxen...@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 <nsax...@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
>
