From: Olivier MATZ <olivier.matz@6wind.com>
To: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Cc: dev@dpdk.org, Pablo de Lara <pablo.de.lara.guarch@intel.com>,
Olivier Matz <olivier.matz@6wind.com>
Subject: Re: [dpdk-dev] [PATCH v4] rte_mbuf: add rte_pktmbuf_linearize
Date: Tue, 10 Jan 2017 17:50:56 +0100 [thread overview]
Message-ID: <20170110175056.051b6b8a@glumotte.dev.6wind.com> (raw)
In-Reply-To: <1483634640-33892-1-git-send-email-tomaszx.kulasek@intel.com>
Hi Tomasz,
On Thu, 5 Jan 2017 17:44:00 +0100, Tomasz Kulasek
<tomaszx.kulasek@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@intel.com>
> Cc: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@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
next prev parent reply other threads:[~2017-01-10 16:51 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-02 17:07 [dpdk-dev] [PATCH 0/4] Chained Mbufs support in SW PMDs Tomasz Kulasek
2016-12-02 17:07 ` [dpdk-dev] [PATCH 1/4] rte_mbuf: add rte_pktmbuf_coalesce Tomasz Kulasek
2016-12-16 10:06 ` Olivier Matz
2016-12-29 15:58 ` Kulasek, TomaszX
2016-12-02 17:07 ` [dpdk-dev] [PATCH 2/4] test: add rte_pktmbuf_coalesce unit tests Tomasz Kulasek
2016-12-02 17:07 ` [dpdk-dev] [PATCH 3/4] crypto: add sgl support for sw PMDs Tomasz Kulasek
2016-12-03 8:28 ` Michał Mirosław
2016-12-12 10:01 ` Kulasek, TomaszX
2016-12-02 17:07 ` [dpdk-dev] [PATCH 4/4] test: add sgl unit tests for crypto devices Tomasz Kulasek
2016-12-29 17:12 ` [dpdk-dev] [PATCH v2 0/5] Chained Mbufs support in SW PMDs Tomasz Kulasek
2016-12-29 17:12 ` [dpdk-dev] [PATCH v2 1/5] rte_mbuf: add rte_pktmbuf_linearize Tomasz Kulasek
2016-12-29 17:12 ` [dpdk-dev] [PATCH v2 2/5] test: add rte_pktmbuf_linearize unit tests Tomasz Kulasek
2016-12-29 17:12 ` [dpdk-dev] [PATCH v2 3/5] crypto: add sgl support in sw PMDs Tomasz Kulasek
2016-12-29 17:12 ` [dpdk-dev] [PATCH v2 4/5] crypto: add sgl support in openssl PMD Tomasz Kulasek
2016-12-29 17:12 ` [dpdk-dev] [PATCH v2 5/5] test: add sgl unit tests for crypto devices Tomasz Kulasek
2017-01-05 9:12 ` [dpdk-dev] [PATCH v3 0/5] Chained Mbufs support in SW PMDs Tomasz Kulasek
2017-01-05 9:12 ` [dpdk-dev] [PATCH v3 1/5] rte_mbuf: add rte_pktmbuf_linearize Tomasz Kulasek
2017-01-05 15:37 ` De Lara Guarch, Pablo
2017-01-05 16:52 ` Kulasek, TomaszX
2017-01-05 9:12 ` [dpdk-dev] [PATCH v3 2/5] test: add rte_pktmbuf_linearize unit tests Tomasz Kulasek
2017-01-05 9:12 ` [dpdk-dev] [PATCH v3 3/5] crypto: add sgl support in sw PMDs Tomasz Kulasek
2017-01-05 9:12 ` [dpdk-dev] [PATCH v3 4/5] crypto: add sgl support in openssl PMD Tomasz Kulasek
2017-01-05 9:12 ` [dpdk-dev] [PATCH v3 5/5] test: add sgl unit tests for crypto devices Tomasz Kulasek
2017-01-05 16:44 ` [dpdk-dev] [PATCH v4] rte_mbuf: add rte_pktmbuf_linearize Tomasz Kulasek
2017-01-10 16:50 ` Olivier MATZ [this message]
2017-01-12 9:40 ` [dpdk-dev] [PATCH v5] mbuf: add a function to linearize a packet Tomasz Kulasek
2017-01-13 15:32 ` Olivier Matz
2017-01-13 15:40 ` Kulasek, TomaszX
2017-01-15 18:32 ` Thomas Monjalon
2017-01-05 16:46 ` [dpdk-dev] [PATCH v4 0/3] Chained Mbufs support in SW PMDs Tomasz Kulasek
2017-01-05 16:46 ` [dpdk-dev] [PATCH v4 1/3] crypto: add sgl support in sw PMDs Tomasz Kulasek
2017-01-05 16:46 ` [dpdk-dev] [PATCH v4 2/3] crypto: add sgl support in openssl PMD Tomasz Kulasek
2017-01-05 16:46 ` [dpdk-dev] [PATCH v4 3/3] test: add sgl unit tests for crypto devices Tomasz Kulasek
2017-01-13 15:23 ` [dpdk-dev] [PATCH v5 0/3] Chained Mbufs support in SW PMDs Tomasz Kulasek
2017-01-13 15:23 ` [dpdk-dev] [PATCH v5 1/3] crypto: add sgl support in sw PMDs Tomasz Kulasek
2017-01-16 16:08 ` Declan Doherty
2017-01-13 15:23 ` [dpdk-dev] [PATCH v5 2/3] crypto: add sgl support in openssl PMD Tomasz Kulasek
2017-01-16 16:10 ` Declan Doherty
2017-01-13 15:23 ` [dpdk-dev] [PATCH v5 3/3] test: add sgl unit tests for crypto devices Tomasz Kulasek
2017-01-16 16:11 ` Declan Doherty
2017-01-16 19:00 ` [dpdk-dev] [PATCH v5 0/3] Chained Mbufs support in SW PMDs De Lara Guarch, Pablo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170110175056.051b6b8a@glumotte.dev.6wind.com \
--to=olivier.matz@6wind.com \
--cc=dev@dpdk.org \
--cc=pablo.de.lara.guarch@intel.com \
--cc=tomaszx.kulasek@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).