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

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

> Hi Stephen,
> 
> Thanks for commenting. See response inline.
> 
> Regards,
> Nitin
> 
> On Tue, Apr 1, 2025 at 7:45 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Tue, 1 Apr 2025 09:50:46 +0530
> > Nitin Saxena <nsaxena@marvell.com> wrote:
> >
> > > +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);
> >
> > Seems wasteful to have a whole memzone for this, the data is small.
> > Is there a reason it could not just be a global variable like timestamp.
> >
> Replaced usage of memzone with global variable in v2

We need to use memzone to share the offset between primary and secondary
processes I don’t see any other way.

> 
> > I would prefer this was a clone of timestamp code, and put in
> rte_mbuf_dynfield.c
> rte_node_mbuf_dynfield_register() is local to graph based rte_nodes
> and it is not targeted to be used by non-graph based applications
> 
> Do you still think we should move this API definition to rte_mbuf_dyn.c?