Hi Olivier,
> > > 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.
> >
> > Looks ok to me in general.
> > Some comments/suggestions inline.
> > Konstantin
> >
> > >
> > > 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
> > >
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -198,9 +198,12 @@ extern "C" {
> > > #define PKT_RX_OUTER_L4_CKSUM_GOOD (1ULL << 22)
> > > #define PKT_RX_OUTER_L4_CKSUM_INVALID ((1ULL << 21) | (1ULL << 22))
> > >
> > > -/* add new RX flags here */
> > > +/* add new RX flags here, don't forget to update PKT_FIRST_FREE */
> > >
> > > -/* add new TX flags here */
> > > +#define PKT_FIRST_FREE (1ULL << 23)
> > > +#define PKT_LAST_FREE (1ULL << 39)
> > > +
> > > +/* add new TX flags here, don't forget to update PKT_LAST_FREE */
> > >
> > > /**
> > > * Indicate that the metadata field in the mbuf is in use.
> > > @@ -738,6 +741,8 @@ struct rte_mbuf {
> > > */
> > > struct rte_mbuf_ext_shared_info *shinfo;
> > >
> > > + uint64_t dynfield1; /**< Reserved for dynamic fields. */
> > > + uint64_t dynfield2; /**< Reserved for dynamic fields. */
> >
> > Wonder why just not one field:
> > union {
> > uint8_t u8[16];
> > ...
> > uint64_t u64[2];
> > } dyn_field1;
> > ?
> > Probably would be a bit handy, to refer, register, etc. no?
>
> I didn't find any place where we need an access through u8, so I
> just changed it into uint64_t dynfield1[2].
My thought was - if you'll have all dynamic stuff as one field (uint64_t
dyn_field[2]),
then you woulnd't need any cycles at register() at all.
But up to you.
>
>
> >
> > > } __rte_cache_aligned;
> > >
> > > /**
> > > @@ -1684,6 +1689,21 @@ rte_pktmbuf_attach_extbuf(struct rte_mbuf *m, void
> > > *buf_addr,
> > > */
> > > #define rte_pktmbuf_detach_extbuf(m) rte_pktmbuf_detach(m)
> > >
> > > +/**
> > > + * Copy dynamic fields from m_src to m_dst.
> > > + *
> > > + * @param m_dst
> > > + * The destination mbuf.
> > > + * @param m_src
> > > + * The source mbuf.
> > > + */
> > > +static inline void
> > > +rte_mbuf_dynfield_copy(struct rte_mbuf *m_dst, const struct rte_mbuf
> > > *m_src)
> > > +{
> > > + m_dst->dynfield1 = m_src->dynfield1;
> > > + m_dst->dynfield2 = m_src->dynfield2;
> > > +}
> > > +
> > > /**
> > > * Attach packet mbuf to another packet mbuf.
> > > *
> > > @@ -1732,6 +1752,7 @@ static inline void rte_pktmbuf_attach(struct
> > > rte_mbuf *mi, struct rte_mbuf *m)
> > > mi->vlan_tci_outer = m->vlan_tci_outer;
> > > mi->tx_offload = m->tx_offload;
> > > mi->hash = m->hash;
> > > + rte_mbuf_dynfield_copy(mi, m);
> > >
> > > mi->next = NULL;
> > > mi->pkt_len = mi->data_len;
> > > diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c
> > > b/lib/librte_mbuf/rte_mbuf_dyn.c
> > > new file mode 100644
> > > index 000000000..13b8742d0
> > > --- /dev/null
> > > +++ b/lib/librte_mbuf/rte_mbuf_dyn.c
> > > @@ -0,0 +1,408 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > + * Copyright 2019 6WIND S.A.
> > > + */
> > > +
> > > +#include <sys/queue.h>
> > > +
> > > +#include <rte_common.h>
> > > +#include <rte_eal.h>
> > > +#include <rte_eal_memconfig.h>
> > > +#include <rte_tailq.h>
> > > +#include <rte_errno.h>
> > > +#include <rte_malloc.h>
> > > +#include <rte_string_fns.h>
> > > +#include <rte_mbuf.h>
> > > +#include <rte_mbuf_dyn.h>
> > > +
> > > +#define RTE_MBUF_DYN_MZNAME "rte_mbuf_dyn"
> > > +
> > > +struct mbuf_dynfield_elt {
> > > + TAILQ_ENTRY(mbuf_dynfield_elt) next;
> > > + struct rte_mbuf_dynfield params;
> > > + int offset;
> >
> > Why not 'size_t offset', to avoid any explicit conversions, etc?
>
> Fixed
>
>
> > > +};
> > > +TAILQ_HEAD(mbuf_dynfield_list, rte_tailq_entry);
> > > +
> > > +static struct rte_tailq_elem mbuf_dynfield_tailq = {
> > > + .name = "RTE_MBUF_DYNFIELD",
> > > +};
> > > +EAL_REGISTER_TAILQ(mbuf_dynfield_tailq);
> > > +
> > > +struct mbuf_dynflag_elt {
> > > + TAILQ_ENTRY(mbuf_dynflag_elt) next;
> > > + struct rte_mbuf_dynflag params;
> > > + int bitnum;
> > > +};
> > > +TAILQ_HEAD(mbuf_dynflag_list, rte_tailq_entry);
> > > +
> > > +static struct rte_tailq_elem mbuf_dynflag_tailq = {
> > > + .name = "RTE_MBUF_DYNFLAG",
> > > +};
> > > +EAL_REGISTER_TAILQ(mbuf_dynflag_tailq);
> > > +
> > > +struct mbuf_dyn_shm {
> > > + /** For each mbuf byte, free_space[i] == 1 if space is free. */
> > > + uint8_t free_space[sizeof(struct rte_mbuf)];
> > > + /** Bitfield of available flags. */
> > > + uint64_t free_flags;
> > > +};
> > > +static struct mbuf_dyn_shm *shm;
> > > +
> > > +/* allocate and initialize the shared memory */
> > > +static int
> > > +init_shared_mem(void)
> > > +{
> > > + const struct rte_memzone *mz;
> > > + uint64_t mask;
> > > +
> > > + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > > + mz = rte_memzone_reserve_aligned(RTE_MBUF_DYN_MZNAME,
> > > + sizeof(struct mbuf_dyn_shm),
> > > + SOCKET_ID_ANY, 0,
> > > + RTE_CACHE_LINE_SIZE);
> > > + } else {
> > > + mz = rte_memzone_lookup(RTE_MBUF_DYN_MZNAME);
> > > + }
> > > + if (mz == NULL)
> > > + return -1;
> > > +
> > > + shm = mz->addr;
> > > +
> > > +#define mark_free(field) \
> > > + memset(&shm->free_space[offsetof(struct rte_mbuf, field)], \
> > > + 0xff, sizeof(((struct rte_mbuf *)0)->field))
> >
> > I think you can avoid defining/unedifying macros here by something like
> > that:
> >
> > static const struct {
> > size_t offset;
> > size_t size;
> > } dyn_syms[] = {
> > [0] = {.offset = offsetof(struct rte_mbuf, dynfield1), sizeof((struct
> > rte_mbuf *)0)->dynfield1),
> > [1] = {.offset = offsetof(struct rte_mbuf, dynfield2), sizeof((struct
> > rte_mbuf *)0)->dynfield2),
> > };
> > ...
> >
> > for (i = 0; i != RTE_DIM(dyn_syms); i++)
> > memset(shm->free_space + dym_syms[i].offset, UINT8_MAX,
> > dym_syms[i].size);
> >
>
> I tried it, but the following lines are too long
> [0] = {offsetof(struct rte_mbuf, dynfield1), sizeof((struct rte_mbuf
> *)0)->dynfield1),
> [1] = {offsetof(struct rte_mbuf, dynfield2), sizeof((struct rte_mbuf
> *)0)->dynfield2),
> To make them shorter, we can use a macro... but... wait :)
Guess what, you can put offset ans size on different lines :)
[0] = {
.offset = offsetof(struct rte_mbuf, dynfield1),
.size= sizeof((struct rte_mbuf *)0)->dynfield1),
},
....
>
> > > +
> > > + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > > + /* init free_space, keep it sync'd with
> > > + * rte_mbuf_dynfield_copy().
> > > + */
> > > + memset(shm, 0, sizeof(*shm));
> > > + mark_free(dynfield1);
> > > + mark_free(dynfield2);
> > > +
> > > + /* init free_flags */
> > > + for (mask = PKT_FIRST_FREE; mask <= PKT_LAST_FREE; mask <<= 1)
> > > + shm->free_flags |= mask;
> > > + }
> > > +#undef mark_free
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* check if this offset can be used */
> > > +static int
> > > +check_offset(size_t offset, size_t size, size_t align, unsigned int
> > > flags)
> > > +{
> > > + size_t i;
> > > +
> > > + (void)flags;
> >
> >
> > We have RTE_SET_USED() for such cases...
> > Though as it is an internal function probably better not to introduce
> > unused parameters at all.
>
> I removed the flag parameter as you suggested.
>
>
> > > +
> > > + if ((offset & (align - 1)) != 0)
> > > + return -1;
> > > + if (offset + size > sizeof(struct rte_mbuf))
> > > + return -1;
> > > +
> > > + for (i = 0; i < size; i++) {
> > > + if (!shm->free_space[i + offset])
> > > + return -1;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* assume tailq is locked */
> > > +static struct mbuf_dynfield_elt *
> > > +__mbuf_dynfield_lookup(const char *name)
> > > +{
> > > + struct mbuf_dynfield_list *mbuf_dynfield_list;
> > > + struct mbuf_dynfield_elt *mbuf_dynfield;
> > > + struct rte_tailq_entry *te;
> > > +
> > > + mbuf_dynfield_list = RTE_TAILQ_CAST(
> > > + mbuf_dynfield_tailq.head, mbuf_dynfield_list);
> > > +
> > > + TAILQ_FOREACH(te, mbuf_dynfield_list, next) {
> > > + mbuf_dynfield = (struct mbuf_dynfield_elt *)te->data;
> > > + if (strcmp(name, mbuf_dynfield->params.name) == 0)
> > > + break;
> > > + }
> > > +
> > > + if (te == NULL) {
> > > + rte_errno = ENOENT;
> > > + return NULL;
> > > + }
> > > +
> > > + return mbuf_dynfield;
> > > +}
> > > +
> > > +int
> > > +rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield
> > > *params)
> > > +{
> > > + struct mbuf_dynfield_elt *mbuf_dynfield;
> > > +
> > > + if (shm == NULL) {
> > > + rte_errno = ENOENT;
> > > + return -1;
> > > + }
> > > +
> > > + rte_mcfg_tailq_read_lock();
> > > + mbuf_dynfield = __mbuf_dynfield_lookup(name);
> > > + rte_mcfg_tailq_read_unlock();
> > > +
> > > + if (mbuf_dynfield == NULL) {
> > > + rte_errno = ENOENT;
> > > + return -1;
> > > + }
> > > +
> > > + if (params != NULL)
> > > + memcpy(params, &mbuf_dynfield->params, sizeof(*params));
> > > +
> > > + return mbuf_dynfield->offset;
> > > +}
> > > +
> > > +static int mbuf_dynfield_cmp(const struct rte_mbuf_dynfield *params1,
> > > + const struct rte_mbuf_dynfield *params2)
> > > +{
> > > + if (strcmp(params1->name, params2->name))
> > > + return -1;
> > > + if (params1->size != params2->size)
> > > + return -1;
> > > + if (params1->align != params2->align)
> > > + return -1;
> > > + if (params1->flags != params2->flags)
> > > + return -1;
> > > + return 0;
> > > +}
> > > +
> > > +int
> > > +rte_mbuf_dynfield_register(const struct rte_mbuf_dynfield *params)
> >
> > What I meant at user-space - if we can also have another function that
> > would allow
> > user to specify required offset for dynfield explicitly, then user can
> > define it as constant
> > value and let compiler do optimization work and hopefully generate faster
> > code to access
> > this field.
> > Something like that:
> >
> > int rte_mbuf_dynfiled_register_offset(const struct rte_mbuf_dynfield
> > *params, size_t offset);
> >
> > #define RTE_MBUF_DYNFIELD_OFFSET(fld, off) (offsetof(struct rte_mbuf, fld)
> > + (off))
> >
> > And then somewhere in user code:
> >
> > /* to let say reserve first 4B in dynfield1*/
> > #define MBUF_DYNFIELD_A RTE_MBUF_DYNFIELD_OFFSET(dynfiled1, 0)
> > ...
> > params.name = RTE_STR(MBUF_DYNFIELD_A);
> > params.size = sizeof(uint32_t);
> > params.align = sizeof(uint32_t);
> > ret = rte_mbuf_dynfiled_register_offset(¶ms, MBUF_DYNFIELD_A);
> > if (ret != MBUF_DYNFIELD_A) {
> > /* handle it somehow, probably just terminate gracefully... */
> > }
> > ...
> >
> > /* to let say reserve last 2B in dynfield2*/
> > #define MBUF_DYNFIELD_B RTE_MBUF_DYNFIELD_OFFSET(dynfiled2, 6)
> > ...
> > params.name = RTE_STR(MBUF_DYNFIELD_B);
> > params.size = sizeof(uint16_t);
> > params.align = sizeof(uint16_t);
> > ret = rte_mbuf_dynfiled_register_offset(¶ms, MBUF_DYNFIELD_B);
> >
> > After that user can use constant offsets MBUF_DYNFIELD_A/ MBUF_DYNFIELD_B
> > to access these fields.
> > Same thoughts for DYNFLAG.
>
> I added the feature in v2.
>
>
> > > + struct mbuf_dynfield_list *mbuf_dynfield_list;
> > > + struct mbuf_dynfield_elt *mbuf_dynfield = NULL;
> > > + struct rte_tailq_entry *te = NULL;
> > > + int offset, ret;
> >
> > size_t offset
> > to avoid explicit conversions, etc.?
> >
>
> Fixed.
>
>
> > > + size_t i;
> > > +
> > > + if (shm == NULL && init_shared_mem() < 0)
> > > + goto fail;
> >
> > As I understand, here you allocate/initialize your shm without any lock
> > protection,
> > though later you protect it via rte_mcfg_tailq_write_lock().
> > That seems a bit flakey to me.
> > Why not to store information about free dynfield bytes inside
> > mbuf_dynfield_tailq?
> > Let say at init() create and add an entry into that list with some
> > reserved name.
> > Then at register - grab mcfg_tailq_write_lock and do lookup
> > for such entry and then read/update it as needed.
> > It would help to avoid racing problem, plus you wouldn't need to
> > allocate/lookup for memzone.
>
> I don't quite like the idea of having a special entry with a different type
> in an element list. Despite it is simpler for a locking perspective, it is
> less obvious for the developper.
>
> Also, I changed the way a zone is reserved to return the one that have the
> less impact on next reservation, and I feel it is easier to implement with
> the shared memory.
>
> So, I just moved the init_shared_mem() inside the rte_mcfg_tailq_write_lock(),
> it should do the job.
Yep, that should work too, I think.
>
>
> > > + if (params->size >= sizeof(struct rte_mbuf)) {
> > > + rte_errno = EINVAL;
> > > + goto fail;
> > > + }
> > > + if (!rte_is_power_of_2(params->align)) {
> > > + rte_errno = EINVAL;
> > > + goto fail;
> > > + }
> > > + if (params->flags != 0) {
> > > + rte_errno = EINVAL;
> > > + goto fail;
> > > + }
> > > +
> > > + rte_mcfg_tailq_write_lock();
> > > +
> >
> > I think it probably would be cleaner and easier to read/maintain, if you'll
> > put actual
> > code under lock protection into a separate function - as you did for
> > __mbuf_dynfield_lookup().
>
> Yes, I did that, it should be clearer now.
>
>
