> -----Original Message----- > From: Olivier Matz [mailto:olivier.m...@6wind.com] > Sent: Monday, September 23, 2019 16:32 > To: Wang, Haiyue <haiyue.w...@intel.com> > Cc: dev@dpdk.org; Thomas Monjalon <tho...@monjalon.net>; Stephen Hemminger > <step...@networkplumber.org>; Andrew Rybchenko <arybche...@solarflare.com>; > Wiles, Keith > <keith.wi...@intel.com>; Jerin Jacob Kollanukkaran <jer...@marvell.com> > Subject: Re: [PATCH] mbuf: support dynamic fields and flags > > Hi, > > On Sat, Sep 21, 2019 at 04:54:39AM +0000, Wang, Haiyue wrote: > > > -----Original Message----- > > > From: Olivier Matz [mailto:olivier.m...@6wind.com] > > > Sent: Thursday, September 19, 2019 00:55 > > > To: dev@dpdk.org > > > Cc: Thomas Monjalon <tho...@monjalon.net>; Wang, Haiyue > > > <haiyue.w...@intel.com>; Stephen Hemminger > > > <step...@networkplumber.org>; Andrew Rybchenko > > > <arybche...@solarflare.com>; Wiles, Keith > > > <keith.wi...@intel.com>; Jerin Jacob Kollanukkaran <jer...@marvell.com> > > > Subject: [PATCH] mbuf: support dynamic fields and flags > > > > > > Many features require to store data inside the mbuf. As the room in mbuf > > > structure is limited, it is not possible to have a field for each > > > feature. Also, changing fields in the mbuf structure can break the API > > > or ABI. > > > > > > This commit addresses these issues, by enabling the dynamic registration > > > of fields or flags: > > > > > > - a dynamic field is a named area in the rte_mbuf structure, with a > > > given size (>= 1 byte) and alignment constraint. > > > - a dynamic flag is a named bit in the rte_mbuf structure. > > > > > > The typical use case is a PMD that registers space for an offload > > > feature, when the application requests to enable this feature. As > > > the space in mbuf is limited, the space should only be reserved if it > > > is going to be used (i.e when the application explicitly asks for it). > > > > > > The registration can be done at any moment, but it is not possible > > > to unregister fields or flags for now. > > > > > > Signed-off-by: Olivier Matz <olivier.m...@6wind.com> > > > Acked-by: Thomas Monjalon <tho...@monjalon.net> > > > --- > > > > > > rfc -> v1 > > > > > > * Rebase on top of master > > > * Change registration API to use a structure instead of > > > variables, getting rid of #defines (Stephen's comment) > > > * Update flag registration to use a similar API as fields. > > > * Change max name length from 32 to 64 (sugg. by Thomas) > > > * Enhance API documentation (Haiyue's and Andrew's comments) > > > * Add a debug log at registration > > > * Add some words in release note > > > * Did some performance tests (sugg. by Andrew): > > > On my platform, reading a dynamic field takes ~3 cycles more > > > than a static field, and ~2 cycles more for writing. > > > > > > app/test/test_mbuf.c | 114 ++++++- > > > doc/guides/rel_notes/release_19_11.rst | 7 + > > > lib/librte_mbuf/Makefile | 2 + > > > lib/librte_mbuf/meson.build | 6 +- > > > lib/librte_mbuf/rte_mbuf.h | 25 +- > > > lib/librte_mbuf/rte_mbuf_dyn.c | 408 +++++++++++++++++++++++++ > > > lib/librte_mbuf/rte_mbuf_dyn.h | 163 ++++++++++ > > > lib/librte_mbuf/rte_mbuf_version.map | 4 + > > > 8 files changed, 724 insertions(+), 5 deletions(-) > > > create mode 100644 lib/librte_mbuf/rte_mbuf_dyn.c > > > create mode 100644 lib/librte_mbuf/rte_mbuf_dyn.h > > > > > > > [snip] > > > > > +/** > > > + * Helper macro to access to a dynamic field. > > > + */ > > > +#define RTE_MBUF_DYNFIELD(m, offset, type) ((type)((uintptr_t)(m) + > > > (offset))) > > > > How about to change it as: ? > > #define RTE_MBUF_DYNFIELD(m, offset, type) ((type *)((uintptr_t)(m) + > > (offset))) > > ^ > > Then, > > *RTE_MBUF_DYNFIELD(mb, xxx, uint32_t) = yyy; > > > > Since we use 'type' like: sizeof(type), __alignof__(type), this makes > > 'type' be > > more consistent, not have to force cast 'type *' when using it. > > > > const struct rte_mbuf_dynfield dynfield2 = { > > .name = "test-dynfield2", > > .size = sizeof(uint16_t), > > .align = __alignof__(uint16_t), > > .flags = 0, > > }; > > Yes, I don't see use cases where the '*' is omitted, so it could be in the > macro. On the other hand, doing like in the patch is more consistent with > similar macros like rte_pktmbuf_mtod(), so I'll tend to keep it as is. > > This is maybe not that important, because this macro will often be hidden > in a wrapper, like below: > > static inline uint64_t rte_mbuf_dyn_timestamp_get(const struct rte_mbuf *m) > { > return *RTE_MBUF_DYNFIELD(m, rte_mbuf_dynfield_timestamp_offset, > uint64_t *); > } >
Thanks, yes, the same style as 'rte_pktmbuf_mtod', I didn't notice it. > > Thank you for the feedback! > > Olivier
