[dpdk-dev] [PATCH v4] rte_mbuf: add rte_pktmbuf_linearize

Olivier MATZ olivier.matz at 6wind.com
Tue Jan 10 17:50:56 CET 2017


Hi Tomasz,

On Thu,  5 Jan 2017 17:44:00 +0100, Tomasz Kulasek
<tomaszx.kulasek at intel.com> wrote:
> This patch adds function rte_pktmbuf_linearize to let crypto PMD
> coalesce chained mbuf before crypto operation and extend their
> capabilities to support segmented mbufs when device cannot handle
> them natively.
> 
> Included unit tests for rte_pktmbuf_linearize functionality:
> 
>  1) Creates banch of segmented mbufs with different size and number of
>     segments.
>  2) Generates pkt_len bytes of random data.
>  3) Fills noncontigouos mbuf with randomly generated data.
>  4) Uses rte_pktmbuf_linearize to coalesce segmented buffer into one
>     contiguous.
>  5) Verifies data in linearized buffer.
> 
> Dependencies:
> 
> This patch is rebased to the dpdk-next-crypto and should be applied
> before "Chained Mbufs support in SW PMDs" patchset.
> 
> 
> changes in v4:
>  - separated from "Chained Mbufs support in SW PMDs" patch set for
>    better reviewing,
>  - merged "rte_pktmbuf_linearize" implementation with unit tests,
> 
> changes in v3:
>  - rebased to dpdk-next-crypto
> 
> changes in v2:
>  - rte_pktmbuf_coalesce replaced with rte_pktmbuf_linearize
> 
> 
> Cc: Pablo de Lara <pablo.de.lara.guarch at intel.com>
> Cc: Olivier Matz <olivier.matz at 6wind.com>
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek at intel.com>

The overall idea and code looks good to me. I have some minor
comments, please see below.

To avoid the warning of check-git-log.sh, the title of the commit could
be change to:
  mbuf: add a function to linearize a packet


> diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> index c0823ea..39577e7 100644
> --- a/app/test/test_mbuf.c
> +++ b/app/test/test_mbuf.c
> @@ -930,6 +930,124 @@
>  	return 0;
>  }
>  
> +static int
> +test_mbuf_linearize(int pkt_len, int nb_segs) {
> +
> +	struct rte_mbuf *m = NULL, *mbuf_src = NULL;
> +	uint8_t data[pkt_len], *src, *dst;
> +	int data_len = 0;
> +	int i, size;
> +	int t_len;
> +
> +	if (pkt_len < 1) {
> +		printf("Packet size must be 1 or more (is %d)\n",
> pkt_len);
> +		return -1;
> +	}
> +
> +	if (nb_segs < 1) {
> +		printf("Number of segments must be 1 or more (is
> %d)\n",
> +				nb_segs);
> +		return -1;
> +	}
> +
> +	/* Setup buffer */
> +	for (i = 0; i < pkt_len; i++)
> +		data[i] = (uint8_t) rte_rand();

This is maybe a bit overkill.
What about data[i] = i ?

And with this assumption, the local buffer would become useless.

> +
> +	t_len = pkt_len >= nb_segs ? pkt_len / nb_segs : 1;

Hmm, I think we could do clearer here. Maybe:

	seg_len = pkt_len / nb_segs;
	if (seg_len == 0)
		seg_len = 1;

> +	src = data;
> +	size = pkt_len;

rename 'size' as 'remain'?

> +
> +	/* Create chained mbuf_src and fill it generated data */
> +	for (i = 0; size > 0; i++) {
> +
> +		m = rte_pktmbuf_alloc(pktmbuf_pool);
> +		if (i == 0)
> +			mbuf_src = m;
> +
> +		if (!m) {

(x == NULL) should be preferred when the variable is a pointer (see
coding_style.rst).

> +			printf("Cannot create segment for source
> mbuf");
> +			goto fail;
> +		}
> +
> +		/* Make sure if tailroom is zeroed */
> +		memset(rte_pktmbuf_mtod(m, uint8_t *), 0,
> +				rte_pktmbuf_tailroom(m));
> +
> +		data_len = size > t_len ? t_len : size;

I think the following is easier to read:

	data_len = remain;
	if (data_len > seg_len)
		data_len = seg_len;

But for that one, maybe it's me, I let you decide ;)


> +		dst = (uint8_t *)rte_pktmbuf_append(m, data_len);
> +		if (!dst) {

if (dst == NULL)

> +			printf("Cannot append %d bytes to the
> mbuf\n",
> +					data_len);
> +			goto fail;
> +		}
> +
> +		rte_memcpy(dst, src, data_len);
> +		src += data_len;
> +
> +		if (mbuf_src != m)
> +			rte_pktmbuf_chain(mbuf_src, m);
> +
> +		size -= data_len;
> +
> +	}
> +
> +	/* Create destination buffer to store coalesced data */
> +	if (rte_pktmbuf_linearize(mbuf_src)) {
> +		printf("Mbuf linearization failed\n");
> +		goto fail;
> +	}
> +
> +	if (!rte_pktmbuf_is_contiguous(mbuf_src)) {
> +		printf("Source buffer should be contiguous after "
> +				"linearization\n");
> +		goto fail;
> +	}
> +
> +	src = rte_pktmbuf_mtod(mbuf_src, uint8_t *);
> +
> +	if (memcmp(src, data, rte_pktmbuf_pkt_len(mbuf_src))) {
> +		printf("Incorrect data in coalesced mbuf\n");
> +		goto fail;
> +	}
> +
> +	if (mbuf_src)
> +		rte_pktmbuf_free(mbuf_src);

Is it possible that mbuf_src is NULL here?

> +	return 0;
> +
> +fail:
> +	if (mbuf_src)
> +		rte_pktmbuf_free(mbuf_src);
> +	return -1;
> +}
> +
> +static int
> +test_mbuf_linearize_check(void)
> +{
> +	struct test_mbuf_array {
> +		int size;
> +		int nb_segs;
> +	} mbuf_array[5] = {

you could change [5] by []

> +			{ 128, 1 },
> +			{ 64, 64 },
> +			{ 512, 10 },
> +			{ 250, 11 },
> +			{ 123, 8 },
> +	};
> +	unsigned int i;
> +
> +	printf("Test mbuf linearize API\n");
> +
> +	for (i = 0; i < RTE_DIM(mbuf_array); i++)
> +		if (test_mbuf_linearize(mbuf_array[i].size,
> +				mbuf_array[i].nb_segs)) {
> +			printf("Test failed for %d, %d\n",
> mbuf_array[i].size,
> +					mbuf_array[i].nb_segs);
> +			return -1;
> +		}
> +
> +	return 0;
> +}
>  
>  static int
>  test_mbuf(void)
> @@ -1023,6 +1141,11 @@
>  		printf("test_failing_mbuf_sanity_check() failed\n");
>  		return -1;
>  	}
> +
> +	if (test_mbuf_linearize_check() < 0) {
> +		printf("test_mbuf_linearize_check() failed\n");
> +		return -1;
> +	}
>  	return 0;
>  }
>  
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index ead7c6e..b11a31d 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1647,6 +1647,62 @@ static inline int rte_pktmbuf_chain(struct
> rte_mbuf *head, struct rte_mbuf *tail }
>  
>  /**
> + * Linearize data in mbuf.
> + *
> + * This function coalesce mbuf merging data in the first segment,
> unchaining
> + * rest, and then frees them.

I suggest this description instead:

This function moves the mbuf data in the first segment if there is
enough tailroom. The subsequent segments are unchained and freed.

> + *
> + * All operations are done in-place, so the structure of incoming
> mbuf
> + * is changed.

Not sure what you mean here. Are you talking about pointers to data
that could become invalid?

> + *
> + * @param mbuf
> + *   mbuf to linearize
> + * @return
> + *   - 0, on success
> + *   - -1, on error
> + */
> +static inline int
> +rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
> +{
> +	int l, n;
> +	struct rte_mbuf *m;
> +	struct rte_mbuf *m_next;
> +	char *buffer;
> +
> +	if (rte_pktmbuf_is_contiguous(mbuf))
> +		return 0;
> +
> +	/* Extend first segment to the total packet length
> +	 */

For one-line comments, it can be:
	/* foo bar */

> +	n = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);

rename 'n' as 'copy_len'?

> +
> +	if (unlikely(n > rte_pktmbuf_tailroom(mbuf)))
> +		return -1;
> +
> +	buffer = rte_pktmbuf_mtod_offset(mbuf, char *,
> mbuf->data_len);
> +	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
> +
> +	/* Append data from next segments to the first one
> +	 */

/* foo bar */

> +	m = mbuf->next;
> +	while (m != NULL) {
> +		m_next = m->next;
> +
> +		l = rte_pktmbuf_data_len(m);

rename 'l' as 'seg_len'?

> +		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), l);
> +		buffer += l;
> +
> +		rte_pktmbuf_free_seg(m);
> +		m = m_next;
> +	}
> +
> +	mbuf->next = NULL;
> +	mbuf->nb_segs = 1;
> +
> +	return 0;
> +}


Thank you Tomasz , I think it could be useful in other scenarii. In the
future it could be extended to memmove() the data of the first segment
if the tailroom is too small.

Regards,
Olivier





More information about the dev mailing list