[v4,3/5] mbuf: create packet pool with external memory buffers
diff mbox series

Message ID 1579179869-32508-4-git-send-email-viacheslavo@mellanox.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • mbuf: detach mbuf with pinned external buffer
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Slava Ovsiienko Jan. 16, 2020, 1:04 p.m. UTC
The dedicated routine rte_pktmbuf_pool_create_extbuf() is
provided to create mbuf pool with data buffers located in
the pinned external memory. The application provides the
external memory description and routine initializes each
mbuf with appropriate virtual and physical buffer address.
It is entirely application responsibility to register
external memory with rte_extmem_register() API, map this
memory, etc.

The new introduced flag RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF
is set in private pool structure, specifying the new special
pool type. The allocated mbufs from pool of this kind will
have the EXT_ATTACHED_MBUF flag set and initialiazed shared
info structure, allowing cloning with regular mbufs (without
attached external buffers of any kind).

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 lib/librte_mbuf/rte_mbuf.c           | 178 ++++++++++++++++++++++++++++++++++-
 lib/librte_mbuf/rte_mbuf.h           |  84 +++++++++++++++++
 lib/librte_mbuf/rte_mbuf_version.map |   1 +
 3 files changed, 260 insertions(+), 3 deletions(-)

Comments

Olivier Matz Jan. 20, 2020, 1:59 p.m. UTC | #1
On Thu, Jan 16, 2020 at 01:04:27PM +0000, Viacheslav Ovsiienko wrote:
> The dedicated routine rte_pktmbuf_pool_create_extbuf() is
> provided to create mbuf pool with data buffers located in
> the pinned external memory. The application provides the
> external memory description and routine initializes each
> mbuf with appropriate virtual and physical buffer address.
> It is entirely application responsibility to register
> external memory with rte_extmem_register() API, map this
> memory, etc.
> 
> The new introduced flag RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF
> is set in private pool structure, specifying the new special
> pool type. The allocated mbufs from pool of this kind will
> have the EXT_ATTACHED_MBUF flag set and initialiazed shared
> info structure, allowing cloning with regular mbufs (without
> attached external buffers of any kind).
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  lib/librte_mbuf/rte_mbuf.c           | 178 ++++++++++++++++++++++++++++++++++-
>  lib/librte_mbuf/rte_mbuf.h           |  84 +++++++++++++++++
>  lib/librte_mbuf/rte_mbuf_version.map |   1 +
>  3 files changed, 260 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 8fa7f49..b9d89d0 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -59,9 +59,12 @@
>  	}
>  
>  	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
> -		user_mbp_priv->mbuf_data_room_size +
> +		((user_mbp_priv->flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) ?
> +			sizeof(struct rte_mbuf_ext_shared_info) :
> +			user_mbp_priv->mbuf_data_room_size) +
>  		user_mbp_priv->mbuf_priv_size);
> -	RTE_ASSERT(user_mbp_priv->flags == 0);
> +	RTE_ASSERT((user_mbp_priv->flags &
> +		    ~RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) == 0);
>  
>  	mbp_priv = rte_mempool_get_priv(mp);
>  	memcpy(mbp_priv, user_mbp_priv, sizeof(*mbp_priv));
> @@ -107,6 +110,89 @@
>  	m->next = NULL;
>  }
>  
> +/*
> + * @internal
> + * The callback routine called when reference counter in shinfo for mbufs
> + * with pinned external buffer reaches zero. It means there is no more
> + * references to buffer backing mbuf and this one should be freed.
> + */
> +static void rte_pktmbuf_free_pinned_extmem(void *addr, void *opaque)
> +{
> +	struct rte_mbuf *m = opaque;
> +
> +	RTE_SET_USED(addr);
> +	RTE_ASSERT(RTE_MBUF_HAS_EXTBUF(m));
> +	RTE_ASSERT(RTE_MBUF_HAS_PINNED_EXTBUF(m));
> +	RTE_ASSERT(m->shinfo->fcb_opaque == m);
> +
> +	rte_mbuf_ext_refcnt_set(m->shinfo, 1);
> +	rte_pktmbuf_free_seg(m);

I think it should be rte_mbuf_raw_free(m) instead.

Else, the corresponding code can fail:

  - alloc packet from pinned_pool
  - increase ref counter (this is legal)
  - free packet on 2 cores at the same time

The refcnt will reach 0 on one of the core, then is calls
rte_pktmbuf_free_pinned_extmem(), then rte_pktmbuf_free_seg(), then
rte_pktmbuf_prefree_seg() which will do nothing because refcnt is not 1
-> mem leak



> +}
> +
> +/*
> + * pktmbuf constructor for the pool with pinned external buffer,
> + * given as a callback function to rte_mempool_obj_iter() in
> + * rte_pktmbuf_pool_create_extbuf(). Set the fields of a packet
> + * mbuf to their default values.
> + */
> +void
> +rte_pktmbuf_init_extmem(struct rte_mempool *mp,
> +			void *opaque_arg,
> +			void *_m,
> +			__attribute__((unused)) unsigned int i)
> +{

Can it be static?
If it is public, it should be experimental and in .map

> +	struct rte_mbuf *m = _m;
> +	struct rte_pktmbuf_extmem_init_ctx *ctx = opaque_arg;
> +	struct rte_pktmbuf_extmem *ext_mem;
> +	uint32_t mbuf_size, buf_len, priv_size;
> +	struct rte_mbuf_ext_shared_info *shinfo;
> +
> +	priv_size = rte_pktmbuf_priv_size(mp);
> +	mbuf_size = sizeof(struct rte_mbuf) + priv_size;
> +	buf_len = rte_pktmbuf_data_room_size(mp);
> +
> +	RTE_ASSERT(RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) == priv_size);
> +	RTE_ASSERT(mp->elt_size >= mbuf_size);
> +	RTE_ASSERT(buf_len <= UINT16_MAX);
> +
> +	memset(m, 0, mbuf_size);
> +	m->priv_size = priv_size;
> +	m->buf_len = (uint16_t)buf_len;
> +
> +	/* set the data buffer pointers to external memory */
> +	ext_mem = ctx->ext_mem + ctx->ext;
> +
> +	RTE_ASSERT(ctx->ext < ctx->ext_num);
> +	RTE_ASSERT(ctx->off < ext_mem->buf_len);
> +
> +	m->buf_addr = RTE_PTR_ADD(ext_mem->buf_ptr, ctx->off);
> +	m->buf_iova = ext_mem->buf_iova == RTE_BAD_IOVA ?
> +		      RTE_BAD_IOVA : (ext_mem->buf_iova + ctx->off);
> +
> +	ctx->off += ext_mem->elt_size;
> +	if (ctx->off >= ext_mem->buf_len) {
> +		ctx->off = 0;
> +		++ctx->ext;
> +	}
> +	/* keep some headroom between start of buffer and data */
> +	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
> +
> +	/* init some constant fields */
> +	m->pool = mp;
> +	m->nb_segs = 1;
> +	m->port = MBUF_INVALID_PORT;
> +	m->ol_flags = EXT_ATTACHED_MBUF;
> +	rte_mbuf_refcnt_set(m, 1);
> +	m->next = NULL;
> +
> +	/* init external buffer shared info items */
> +	shinfo = RTE_PTR_ADD(m, mbuf_size);

I think it should be
shinfo = RTE_PTR_ADD(m, mbuf_size + priv_size);

> +	m->shinfo = shinfo;
> +	shinfo->free_cb = rte_pktmbuf_free_pinned_extmem;
> +	shinfo->fcb_opaque = m;
> +	rte_mbuf_ext_refcnt_set(shinfo, 1);

To me, it is not very clear that free_cb() will only be called
for mbuf from non-pinned pools that are attached to a mbuf from
a pinned pool, i.e. the free_cb is not called at all if attach()
is not used.


> +}
> +
>  /* Helper to create a mbuf pool with given mempool ops name*/
>  struct rte_mempool *
>  rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
> @@ -169,6 +255,92 @@ struct rte_mempool *
>  			data_room_size, socket_id, NULL);
>  }
>  
> +/* Helper to create a mbuf pool with pinned external data buffers. */
> +struct rte_mempool *
> +rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
> +	unsigned int cache_size, uint16_t priv_size,
> +	uint16_t data_room_size, int socket_id,
> +	struct rte_pktmbuf_extmem *ext_mem, unsigned int ext_num)

struct rte_pktmbuf_extmem *ext_mem can be const

> +{
> +	struct rte_mempool *mp;
> +	struct rte_pktmbuf_pool_private mbp_priv;
> +	struct rte_pktmbuf_extmem_init_ctx init_ctx;
> +	const char *mp_ops_name;
> +	unsigned int elt_size;
> +	unsigned int i, n_elts = 0;
> +	int ret;
> +
> +	if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
> +		RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
> +			priv_size);
> +		rte_errno = EINVAL;
> +		return NULL;
> +	}
> +	/* Check the external memory descriptors. */
> +	for (i = 0; i < ext_num; i++) {
> +		struct rte_pktmbuf_extmem *extm = ext_mem + i;
> +
> +		if (!extm->elt_size || !extm->buf_len || !extm->buf_ptr) {
> +			RTE_LOG(ERR, MBUF, "invalid extmem descriptor\n");
> +			rte_errno = EINVAL;
> +			return NULL;
> +		}
> +		if (data_room_size > extm->elt_size) {
> +			RTE_LOG(ERR, MBUF, "ext elt_size=%u is too small\n",
> +				priv_size);
> +			rte_errno = EINVAL;
> +			return NULL;
> +		}
> +		n_elts += extm->buf_len / extm->elt_size;
> +	}

Maybe it's just me, but I think there is a source of confusion between:

- elt_size: the size of mempool element (i.e a sizeof(mbuf) +
  priv_size + sizeof(shinfo))
- extm->elt_size: the size of data buffer in external memory

The structure fields could be renamed like this:

struct rte_pktmbuf_extmem {
	void *ptr;		/**< The virtual address of data buffer. */
	rte_iova_t iova;	/**< The IO address of the data buffer. */
	size_t len;		/**< External buffer length in bytes. */
	uint16_t mbuf_data_size; /**< mbuf data size in bytes. */
};


> +	/* Check whether enough external memory provided. */
> +	if (n_elts < n) {
> +		RTE_LOG(ERR, MBUF, "not enough extmem\n");
> +		rte_errno = ENOMEM;
> +		return NULL;
> +	}
> +	elt_size = sizeof(struct rte_mbuf) +
> +		   (unsigned int)priv_size +
> +		   sizeof(struct rte_mbuf_ext_shared_info);
> +
> +	memset(&mbp_priv, 0, sizeof(mbp_priv));
> +	mbp_priv.mbuf_data_room_size = data_room_size;
> +	mbp_priv.mbuf_priv_size = priv_size;
> +	mbp_priv.flags = RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF;
> +
> +	mp = rte_mempool_create_empty(name, n, elt_size, cache_size,
> +		 sizeof(struct rte_pktmbuf_pool_private), socket_id, 0);
> +	if (mp == NULL)
> +		return NULL;
> +
> +	mp_ops_name = rte_mbuf_best_mempool_ops();
> +	ret = rte_mempool_set_ops_byname(mp, mp_ops_name, NULL);
> +	if (ret != 0) {
> +		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
> +		rte_mempool_free(mp);
> +		rte_errno = -ret;
> +		return NULL;
> +	}
> +	rte_pktmbuf_pool_init(mp, &mbp_priv);
> +
> +	ret = rte_mempool_populate_default(mp);
> +	if (ret < 0) {
> +		rte_mempool_free(mp);
> +		rte_errno = -ret;
> +		return NULL;
> +	}

I still think that these ~20 lines could be simplified into a call to
rte_pktmbuf_pool_create(). I don't think that using rte_mempool_obj_iter()
twice would cause a performance issue.

> +
> +	init_ctx = (struct rte_pktmbuf_extmem_init_ctx){
> +		.ext_mem = ext_mem,
> +		.ext_num = ext_num,
> +		.ext = 0,
> +		.off = 0,
> +	};
> +	rte_mempool_obj_iter(mp, rte_pktmbuf_init_extmem, &init_ctx);
> +
> +	return mp;
> +}
> +
>  /* do some sanity checks on a mbuf: panic if it fails */
>  void
>  rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
7> @@ -247,7 +419,7 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
>  	return 0;
>  }
>  
> -/**
> +/*
>   * @internal helper function for freeing a bulk of packet mbuf segments
>   * via an array holding the packet mbuf segments from the same mempool
>   * pending to be freed.

I agree with this change, but it should not go in that patch.

> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 52d57d1..093210e 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -637,6 +637,34 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
>  void rte_pktmbuf_init(struct rte_mempool *mp, void *opaque_arg,
>  		      void *m, unsigned i);
>  
> +/** The context to initialize the mbufs with pinned external buffers. */
> +struct rte_pktmbuf_extmem_init_ctx {
> +	struct rte_pktmbuf_extmem *ext_mem; /* pointer to descriptor array. */
> +	unsigned int ext_num; /* number of descriptors in array. */
> +	unsigned int ext; /* loop descriptor index. */
> +	size_t off; /* loop buffer offset. */
> +};

I think it could be private.

> +
> +/**
> + * The packet mbuf constructor for pools with pinned external memory.
> + *
> + * This function initializes some fields in the mbuf structure that are
> + * not modified by the user once created (origin pool, buffer start
> + * address, and so on). This function is given as a callback function to
> + * rte_mempool_obj_iter() called from rte_mempool_create_extmem().
> + *
> + * @param mp
> + *   The mempool from which mbufs originate.
> + * @param opaque_arg
> + *   A pointer to the rte_pktmbuf_extmem_init_ctx - initialization
> + *   context structure
> + * @param m
> + *   The mbuf to initialize.
> + * @param i
> + *   The index of the mbuf in the pool table.
> + */
> +void rte_pktmbuf_init_extmem(struct rte_mempool *mp, void *opaque_arg,
> +			     void *m, unsigned int i);
>  

Except if I missing something, it could be private too

>  /**
>   * A  packet mbuf pool constructor.
> @@ -738,6 +766,62 @@ struct rte_mempool *
>  	unsigned int cache_size, uint16_t priv_size, uint16_t data_room_size,
>  	int socket_id, const char *ops_name);
>  
> +/** A structure that describes the pinned external buffer segment. */
> +struct rte_pktmbuf_extmem {
> +	void *buf_ptr;		/**< The virtual address of data buffer. */
> +	rte_iova_t buf_iova;	/**< The IO address of the data buffer. */
> +	size_t buf_len;		/**< External buffer length in bytes. */
> +	uint16_t elt_size;	/**< mbuf element size in bytes. */
> +};

See my proposition above about fields renaming.

> +
> +/**
> + * Create a mbuf pool with external pinned data buffers.
> + *
> + * This function creates and initializes a packet mbuf pool that contains
> + * only mbufs with external buffer. It is a wrapper to rte_mempool functions.
> + *
> + * @param name
> + *   The name of the mbuf pool.
> + * @param n
> + *   The number of elements in the mbuf pool. The optimum size (in terms
> + *   of memory usage) for a mempool is when n is a power of two minus one:
> + *   n = (2^q - 1).
> + * @param cache_size
> + *   Size of the per-core object cache. See rte_mempool_create() for
> + *   details.
> + * @param priv_size
> + *   Size of application private are between the rte_mbuf structure
> + *   and the data buffer. This value must be aligned to RTE_MBUF_PRIV_ALIGN.
> + * @param data_room_size
> + *   Size of data buffer in each mbuf, including RTE_PKTMBUF_HEADROOM.
> + * @param socket_id
> + *   The socket identifier where the memory should be allocated. The
> + *   value can be *SOCKET_ID_ANY* if there is no NUMA constraint for the
> + *   reserved zone.
> + * @param ext_mem
> + *   Pointer to the array of structures describing the external memory
> + *   for data buffers. It is caller responsibility to register this memory
> + *   with rte_extmem_register() (if needed), map this memory to appropriate
> + *   physical device, etc.
> + * @param ext_num
> + *   Number of elements in the ext_mem array.
> + * @return
> + *   The pointer to the new allocated mempool, on success. NULL on error
> + *   with rte_errno set appropriately. Possible rte_errno values include:
> + *    - E_RTE_NO_CONFIG - function could not get pointer to rte_config structure
> + *    - E_RTE_SECONDARY - function was called from a secondary process instance
> + *    - EINVAL - cache size provided is too large, or priv_size is not aligned.
> + *    - ENOSPC - the maximum number of memzones has already been allocated
> + *    - EEXIST - a memzone with the same name already exists
> + *    - ENOMEM - no appropriate memory area found in which to create memzone
> + */
> +__rte_experimental
> +struct rte_mempool *
> +rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
> +	unsigned int cache_size, uint16_t priv_size,
> +	uint16_t data_room_size, int socket_id,
> +	struct rte_pktmbuf_extmem *ext_mem, unsigned int ext_num);
> +
>  /**
>   * Get the data room size of mbufs stored in a pktmbuf_pool
>   *
> diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
> index 3bbb476..ab161bc 100644
> --- a/lib/librte_mbuf/rte_mbuf_version.map
> +++ b/lib/librte_mbuf/rte_mbuf_version.map
> @@ -44,5 +44,6 @@ EXPERIMENTAL {
>  	rte_mbuf_dyn_dump;
>  	rte_pktmbuf_copy;
>  	rte_pktmbuf_free_bulk;
> +	rte_pktmbuf_pool_create_extbuf;
>  
>  };
> -- 
> 1.8.3.1
> 

One more thing: it would be nice to have a functional test for this new
feature. I did a very minimal one to check the basic alloc/free/attach
feature, you can restart from that if you want.

static int
test_ext_pinned(int test_case)
{
	struct rte_pktmbuf_extmem ext_mem;
	struct rte_mempool *pinned_pool = NULL;
	struct rte_mempool *std_pool = NULL;
	const struct rte_memzone *mz = NULL;
	struct rte_mbuf *m = NULL, *m2 = NULL;

	printf("Test mbuf pool with mbufs data pinned to external buffer (%d)\n", test_case);

	std_pool = rte_pktmbuf_pool_create("std_pool",
			NB_MBUF, MEMPOOL_CACHE_SIZE, 0, MBUF_DATA_SIZE,
			SOCKET_ID_ANY);
	if (std_pool == NULL)
		GOTO_FAIL("std_pool alloc failed");

	mz = rte_memzone_reserve("std_pool",
				NB_MBUF * sizeof(struct rte_mbuf),
				SOCKET_ID_ANY,
				RTE_MEMZONE_2MB|RTE_MEMZONE_SIZE_HINT_ONLY);
	if (mz == NULL)
		GOTO_FAIL("memzone alloc failed");

	ext_mem.buf_ptr = mz->addr;
	ext_mem.buf_iova = mz->iova;
	ext_mem.buf_len = mz->len;
	ext_mem.elt_size = sizeof(struct rte_mbuf);

	pinned_pool = rte_pktmbuf_pool_create_extbuf("pinned_pool",
					NB_MBUF, MEMPOOL_CACHE_SIZE,
					0, 0, SOCKET_ID_ANY, &ext_mem, 1);
	if (pinned_pool == NULL)
		GOTO_FAIL("pinned_pool alloc failed");

	m = rte_pktmbuf_alloc(pinned_pool);
	if (unlikely(m == NULL))
		goto fail;

	if (test_case != 0) {
		m2 = rte_pktmbuf_alloc(std_pool);
		if (unlikely(m == NULL))
			goto fail;
		rte_pktmbuf_attach(m2, m);
	}

	if (test_case == 0) {
		rte_pktmbuf_free(m);
	} else if (test_case == 1) {
		rte_pktmbuf_free(m);
		rte_pktmbuf_free(m2);
	} else if (test_case == 2) {
		rte_pktmbuf_free(m2);
		rte_pktmbuf_free(m);
	}


	rte_mempool_free(pinned_pool);
	rte_memzone_free(mz);
	rte_mempool_free(std_pool);
	return 0;

fail:
	rte_pktmbuf_free(m2);
	rte_pktmbuf_free(m);
	rte_mempool_free(pinned_pool);
	rte_memzone_free(mz);
	rte_mempool_free(std_pool);
	return -1;
}
Slava Ovsiienko Jan. 20, 2020, 5:33 p.m. UTC | #2
Hi, Olivier

Great! Thanks a lot for the unit test template, this is very nice starting point,
I will think what would be good to add for more thorough testing.

With best regards, Slava
 
> One more thing: it would be nice to have a functional test for this new
> feature. I did a very minimal one to check the basic alloc/free/attach
> feature, you can restart from that if you want.
> 
> static int
> test_ext_pinned(int test_case)
> {
> 	struct rte_pktmbuf_extmem ext_mem;
> 	struct rte_mempool *pinned_pool = NULL;
> 	struct rte_mempool *std_pool = NULL;
> 	const struct rte_memzone *mz = NULL;
> 	struct rte_mbuf *m = NULL, *m2 = NULL;
> 
> 	printf("Test mbuf pool with mbufs data pinned to external buffer
> (%d)\n", test_case);
> 
> 	std_pool = rte_pktmbuf_pool_create("std_pool",
> 			NB_MBUF, MEMPOOL_CACHE_SIZE, 0,
> MBUF_DATA_SIZE,
> 			SOCKET_ID_ANY);
> 	if (std_pool == NULL)
> 		GOTO_FAIL("std_pool alloc failed");
> 
> 	mz = rte_memzone_reserve("std_pool",
> 				NB_MBUF * sizeof(struct rte_mbuf),
> 				SOCKET_ID_ANY,
> 
> 	RTE_MEMZONE_2MB|RTE_MEMZONE_SIZE_HINT_ONLY);
> 	if (mz == NULL)
> 		GOTO_FAIL("memzone alloc failed");
> 
> 	ext_mem.buf_ptr = mz->addr;
> 	ext_mem.buf_iova = mz->iova;
> 	ext_mem.buf_len = mz->len;
> 	ext_mem.elt_size = sizeof(struct rte_mbuf);
> 
> 	pinned_pool = rte_pktmbuf_pool_create_extbuf("pinned_pool",
> 					NB_MBUF, MEMPOOL_CACHE_SIZE,
> 					0, 0, SOCKET_ID_ANY, &ext_mem, 1);
> 	if (pinned_pool == NULL)
> 		GOTO_FAIL("pinned_pool alloc failed");
> 
> 	m = rte_pktmbuf_alloc(pinned_pool);
> 	if (unlikely(m == NULL))
> 		goto fail;
> 
> 	if (test_case != 0) {
> 		m2 = rte_pktmbuf_alloc(std_pool);
> 		if (unlikely(m == NULL))
> 			goto fail;
> 		rte_pktmbuf_attach(m2, m);
> 	}
> 
> 	if (test_case == 0) {
> 		rte_pktmbuf_free(m);
> 	} else if (test_case == 1) {
> 		rte_pktmbuf_free(m);
> 		rte_pktmbuf_free(m2);
> 	} else if (test_case == 2) {
> 		rte_pktmbuf_free(m2);
> 		rte_pktmbuf_free(m);
> 	}
> 
> 
> 	rte_mempool_free(pinned_pool);
> 	rte_memzone_free(mz);
> 	rte_mempool_free(std_pool);
> 	return 0;
> 
> fail:
> 	rte_pktmbuf_free(m2);
> 	rte_pktmbuf_free(m);
> 	rte_mempool_free(pinned_pool);
> 	rte_memzone_free(mz);
> 	rte_mempool_free(std_pool);
> 	return -1;
> }

Patch
diff mbox series

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 8fa7f49..b9d89d0 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -59,9 +59,12 @@ 
 	}
 
 	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
-		user_mbp_priv->mbuf_data_room_size +
+		((user_mbp_priv->flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) ?
+			sizeof(struct rte_mbuf_ext_shared_info) :
+			user_mbp_priv->mbuf_data_room_size) +
 		user_mbp_priv->mbuf_priv_size);
-	RTE_ASSERT(user_mbp_priv->flags == 0);
+	RTE_ASSERT((user_mbp_priv->flags &
+		    ~RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) == 0);
 
 	mbp_priv = rte_mempool_get_priv(mp);
 	memcpy(mbp_priv, user_mbp_priv, sizeof(*mbp_priv));
@@ -107,6 +110,89 @@ 
 	m->next = NULL;
 }
 
+/*
+ * @internal
+ * The callback routine called when reference counter in shinfo for mbufs
+ * with pinned external buffer reaches zero. It means there is no more
+ * references to buffer backing mbuf and this one should be freed.
+ */
+static void rte_pktmbuf_free_pinned_extmem(void *addr, void *opaque)
+{
+	struct rte_mbuf *m = opaque;
+
+	RTE_SET_USED(addr);
+	RTE_ASSERT(RTE_MBUF_HAS_EXTBUF(m));
+	RTE_ASSERT(RTE_MBUF_HAS_PINNED_EXTBUF(m));
+	RTE_ASSERT(m->shinfo->fcb_opaque == m);
+
+	rte_mbuf_ext_refcnt_set(m->shinfo, 1);
+	rte_pktmbuf_free_seg(m);
+}
+
+/*
+ * pktmbuf constructor for the pool with pinned external buffer,
+ * given as a callback function to rte_mempool_obj_iter() in
+ * rte_pktmbuf_pool_create_extbuf(). Set the fields of a packet
+ * mbuf to their default values.
+ */
+void
+rte_pktmbuf_init_extmem(struct rte_mempool *mp,
+			void *opaque_arg,
+			void *_m,
+			__attribute__((unused)) unsigned int i)
+{
+	struct rte_mbuf *m = _m;
+	struct rte_pktmbuf_extmem_init_ctx *ctx = opaque_arg;
+	struct rte_pktmbuf_extmem *ext_mem;
+	uint32_t mbuf_size, buf_len, priv_size;
+	struct rte_mbuf_ext_shared_info *shinfo;
+
+	priv_size = rte_pktmbuf_priv_size(mp);
+	mbuf_size = sizeof(struct rte_mbuf) + priv_size;
+	buf_len = rte_pktmbuf_data_room_size(mp);
+
+	RTE_ASSERT(RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) == priv_size);
+	RTE_ASSERT(mp->elt_size >= mbuf_size);
+	RTE_ASSERT(buf_len <= UINT16_MAX);
+
+	memset(m, 0, mbuf_size);
+	m->priv_size = priv_size;
+	m->buf_len = (uint16_t)buf_len;
+
+	/* set the data buffer pointers to external memory */
+	ext_mem = ctx->ext_mem + ctx->ext;
+
+	RTE_ASSERT(ctx->ext < ctx->ext_num);
+	RTE_ASSERT(ctx->off < ext_mem->buf_len);
+
+	m->buf_addr = RTE_PTR_ADD(ext_mem->buf_ptr, ctx->off);
+	m->buf_iova = ext_mem->buf_iova == RTE_BAD_IOVA ?
+		      RTE_BAD_IOVA : (ext_mem->buf_iova + ctx->off);
+
+	ctx->off += ext_mem->elt_size;
+	if (ctx->off >= ext_mem->buf_len) {
+		ctx->off = 0;
+		++ctx->ext;
+	}
+	/* keep some headroom between start of buffer and data */
+	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
+
+	/* init some constant fields */
+	m->pool = mp;
+	m->nb_segs = 1;
+	m->port = MBUF_INVALID_PORT;
+	m->ol_flags = EXT_ATTACHED_MBUF;
+	rte_mbuf_refcnt_set(m, 1);
+	m->next = NULL;
+
+	/* init external buffer shared info items */
+	shinfo = RTE_PTR_ADD(m, mbuf_size);
+	m->shinfo = shinfo;
+	shinfo->free_cb = rte_pktmbuf_free_pinned_extmem;
+	shinfo->fcb_opaque = m;
+	rte_mbuf_ext_refcnt_set(shinfo, 1);
+}
+
 /* Helper to create a mbuf pool with given mempool ops name*/
 struct rte_mempool *
 rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
@@ -169,6 +255,92 @@  struct rte_mempool *
 			data_room_size, socket_id, NULL);
 }
 
+/* Helper to create a mbuf pool with pinned external data buffers. */
+struct rte_mempool *
+rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
+	unsigned int cache_size, uint16_t priv_size,
+	uint16_t data_room_size, int socket_id,
+	struct rte_pktmbuf_extmem *ext_mem, unsigned int ext_num)
+{
+	struct rte_mempool *mp;
+	struct rte_pktmbuf_pool_private mbp_priv;
+	struct rte_pktmbuf_extmem_init_ctx init_ctx;
+	const char *mp_ops_name;
+	unsigned int elt_size;
+	unsigned int i, n_elts = 0;
+	int ret;
+
+	if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
+		RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
+			priv_size);
+		rte_errno = EINVAL;
+		return NULL;
+	}
+	/* Check the external memory descriptors. */
+	for (i = 0; i < ext_num; i++) {
+		struct rte_pktmbuf_extmem *extm = ext_mem + i;
+
+		if (!extm->elt_size || !extm->buf_len || !extm->buf_ptr) {
+			RTE_LOG(ERR, MBUF, "invalid extmem descriptor\n");
+			rte_errno = EINVAL;
+			return NULL;
+		}
+		if (data_room_size > extm->elt_size) {
+			RTE_LOG(ERR, MBUF, "ext elt_size=%u is too small\n",
+				priv_size);
+			rte_errno = EINVAL;
+			return NULL;
+		}
+		n_elts += extm->buf_len / extm->elt_size;
+	}
+	/* Check whether enough external memory provided. */
+	if (n_elts < n) {
+		RTE_LOG(ERR, MBUF, "not enough extmem\n");
+		rte_errno = ENOMEM;
+		return NULL;
+	}
+	elt_size = sizeof(struct rte_mbuf) +
+		   (unsigned int)priv_size +
+		   sizeof(struct rte_mbuf_ext_shared_info);
+
+	memset(&mbp_priv, 0, sizeof(mbp_priv));
+	mbp_priv.mbuf_data_room_size = data_room_size;
+	mbp_priv.mbuf_priv_size = priv_size;
+	mbp_priv.flags = RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF;
+
+	mp = rte_mempool_create_empty(name, n, elt_size, cache_size,
+		 sizeof(struct rte_pktmbuf_pool_private), socket_id, 0);
+	if (mp == NULL)
+		return NULL;
+
+	mp_ops_name = rte_mbuf_best_mempool_ops();
+	ret = rte_mempool_set_ops_byname(mp, mp_ops_name, NULL);
+	if (ret != 0) {
+		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
+		rte_mempool_free(mp);
+		rte_errno = -ret;
+		return NULL;
+	}
+	rte_pktmbuf_pool_init(mp, &mbp_priv);
+
+	ret = rte_mempool_populate_default(mp);
+	if (ret < 0) {
+		rte_mempool_free(mp);
+		rte_errno = -ret;
+		return NULL;
+	}
+
+	init_ctx = (struct rte_pktmbuf_extmem_init_ctx){
+		.ext_mem = ext_mem,
+		.ext_num = ext_num,
+		.ext = 0,
+		.off = 0,
+	};
+	rte_mempool_obj_iter(mp, rte_pktmbuf_init_extmem, &init_ctx);
+
+	return mp;
+}
+
 /* do some sanity checks on a mbuf: panic if it fails */
 void
 rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
@@ -247,7 +419,7 @@  int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
-/**
+/*
  * @internal helper function for freeing a bulk of packet mbuf segments
  * via an array holding the packet mbuf segments from the same mempool
  * pending to be freed.
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 52d57d1..093210e 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -637,6 +637,34 @@  static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
 void rte_pktmbuf_init(struct rte_mempool *mp, void *opaque_arg,
 		      void *m, unsigned i);
 
+/** The context to initialize the mbufs with pinned external buffers. */
+struct rte_pktmbuf_extmem_init_ctx {
+	struct rte_pktmbuf_extmem *ext_mem; /* pointer to descriptor array. */
+	unsigned int ext_num; /* number of descriptors in array. */
+	unsigned int ext; /* loop descriptor index. */
+	size_t off; /* loop buffer offset. */
+};
+
+/**
+ * The packet mbuf constructor for pools with pinned external memory.
+ *
+ * This function initializes some fields in the mbuf structure that are
+ * not modified by the user once created (origin pool, buffer start
+ * address, and so on). This function is given as a callback function to
+ * rte_mempool_obj_iter() called from rte_mempool_create_extmem().
+ *
+ * @param mp
+ *   The mempool from which mbufs originate.
+ * @param opaque_arg
+ *   A pointer to the rte_pktmbuf_extmem_init_ctx - initialization
+ *   context structure
+ * @param m
+ *   The mbuf to initialize.
+ * @param i
+ *   The index of the mbuf in the pool table.
+ */
+void rte_pktmbuf_init_extmem(struct rte_mempool *mp, void *opaque_arg,
+			     void *m, unsigned int i);
 
 /**
  * A  packet mbuf pool constructor.
@@ -738,6 +766,62 @@  struct rte_mempool *
 	unsigned int cache_size, uint16_t priv_size, uint16_t data_room_size,
 	int socket_id, const char *ops_name);
 
+/** A structure that describes the pinned external buffer segment. */
+struct rte_pktmbuf_extmem {
+	void *buf_ptr;		/**< The virtual address of data buffer. */
+	rte_iova_t buf_iova;	/**< The IO address of the data buffer. */
+	size_t buf_len;		/**< External buffer length in bytes. */
+	uint16_t elt_size;	/**< mbuf element size in bytes. */
+};
+
+/**
+ * Create a mbuf pool with external pinned data buffers.
+ *
+ * This function creates and initializes a packet mbuf pool that contains
+ * only mbufs with external buffer. It is a wrapper to rte_mempool functions.
+ *
+ * @param name
+ *   The name of the mbuf pool.
+ * @param n
+ *   The number of elements in the mbuf pool. The optimum size (in terms
+ *   of memory usage) for a mempool is when n is a power of two minus one:
+ *   n = (2^q - 1).
+ * @param cache_size
+ *   Size of the per-core object cache. See rte_mempool_create() for
+ *   details.
+ * @param priv_size
+ *   Size of application private are between the rte_mbuf structure
+ *   and the data buffer. This value must be aligned to RTE_MBUF_PRIV_ALIGN.
+ * @param data_room_size
+ *   Size of data buffer in each mbuf, including RTE_PKTMBUF_HEADROOM.
+ * @param socket_id
+ *   The socket identifier where the memory should be allocated. The
+ *   value can be *SOCKET_ID_ANY* if there is no NUMA constraint for the
+ *   reserved zone.
+ * @param ext_mem
+ *   Pointer to the array of structures describing the external memory
+ *   for data buffers. It is caller responsibility to register this memory
+ *   with rte_extmem_register() (if needed), map this memory to appropriate
+ *   physical device, etc.
+ * @param ext_num
+ *   Number of elements in the ext_mem array.
+ * @return
+ *   The pointer to the new allocated mempool, on success. NULL on error
+ *   with rte_errno set appropriately. Possible rte_errno values include:
+ *    - E_RTE_NO_CONFIG - function could not get pointer to rte_config structure
+ *    - E_RTE_SECONDARY - function was called from a secondary process instance
+ *    - EINVAL - cache size provided is too large, or priv_size is not aligned.
+ *    - ENOSPC - the maximum number of memzones has already been allocated
+ *    - EEXIST - a memzone with the same name already exists
+ *    - ENOMEM - no appropriate memory area found in which to create memzone
+ */
+__rte_experimental
+struct rte_mempool *
+rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
+	unsigned int cache_size, uint16_t priv_size,
+	uint16_t data_room_size, int socket_id,
+	struct rte_pktmbuf_extmem *ext_mem, unsigned int ext_num);
+
 /**
  * Get the data room size of mbufs stored in a pktmbuf_pool
  *
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 3bbb476..ab161bc 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -44,5 +44,6 @@  EXPERIMENTAL {
 	rte_mbuf_dyn_dump;
 	rte_pktmbuf_copy;
 	rte_pktmbuf_free_bulk;
+	rte_pktmbuf_pool_create_extbuf;
 
 };