Re: [dpdk-dev] [PATCH] mbuf: support dynamic fields and flags

[Olivier Matz <olivier.matz@6wind.com>]

Hi Konstantin,

On Thu, Oct 17, 2019 at 11:58:52AM +0000, Ananyev, Konstantin wrote:
> 
> 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.matz@6wind.com>
> > > > Acked-by: Thomas Monjalon <thomas@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.

I changed it.


> > 
> > >
> > > >  } __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),
> },

Yes, but honnestly, I'm not sure that it will be more readable than
the macro, knowing that we could add fields in the future.


> ....
> 
> > 
> > > > +
> > > > +	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(&params, 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(&params, 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.
> > 
> >