From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id 30B8B532D for ; Tue, 10 Jan 2017 17:51:05 +0100 (CET) Received: by mail-wm0-f42.google.com with SMTP id k184so168938089wme.1 for ; Tue, 10 Jan 2017 08:51:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=from:date:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=VkNeSRC6wYqdIx0s/6pfJVMHaQH31AXHI/cFxc6zeF4=; b=t1+Cs1PeP6osFFqnpgg46lznvT2gd0AnPY1JaYNT9oYzfvcZU752ZN64zjkHzVwtxS 2PdymJzzCi/phpGsp1UeAdapLkEJcihGUyBGZn/uSet8qzSy8QgLa3VDygKpw15cjUui VJHl3lwxWG4wuXikI77MXhWjU/2BcQdDqLrodWNHUGND5dhG7jBrq5dP90wM+4EXT9UV Fd6Q3WfIyP2iyvYLtcUtUyz9ylhR2Aqw9Befs+WTusDShsKMboQv2mktAHSVOYSbWPtk 0cDZwFn8W3Wx6wCtnSWOLNN1BFLbwBjcIHZZULO7aDM1ppXtfniEvBpQz0iBxAUrlCBw CCZg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=VkNeSRC6wYqdIx0s/6pfJVMHaQH31AXHI/cFxc6zeF4=; b=GEFVEVSsIy7N9hgKsT3iZ2V+obwbDGREWmF46xMDcyx52RRrX9dBQsVuaLa5Dw3HNq qYmZW6nt0Bs/hUfy7klZlRHgY2mOOUKVanyMDWhoLikXKhAef5IWO9LbrGEc5jIN3Edj A7uiIyfFEo+ojANhDDxDydqZ5gbft743KJT65r+H7Y5AVWlHjExNc9+Ke/DYmig7CWXM SWSySLX1O96bdmFvBNqS2dFQNiKu8L1C3QzZK+8Lt9VrEgQSOVF0WTwhHERV5JnLtTNI iYKdET5QV0Jl8vI7qWxZmMsxvRZIC/nJ76Ez6akdQeHPDETkNnrpUVvivSH8gkglMWj1 otEg== X-Gm-Message-State: AIkVDXLcjG0x6oLsSsVzU0QfBc43VfAOhzH0Qntdg3d8CqHbuJHDZRH7qNcivXycyg9zwqLo X-Received: by 10.28.180.132 with SMTP id d126mr504197wmf.123.1484067064717; Tue, 10 Jan 2017 08:51:04 -0800 (PST) Received: from glumotte.dev.6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id c187sm4639071wmd.13.2017.01.10.08.51.03 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 10 Jan 2017 08:51:04 -0800 (PST) From: Olivier MATZ X-Google-Original-From: Olivier MATZ Date: Tue, 10 Jan 2017 17:50:56 +0100 To: Tomasz Kulasek Cc: dev@dpdk.org, Pablo de Lara , Olivier Matz Message-ID: <20170110175056.051b6b8a@glumotte.dev.6wind.com> In-Reply-To: <1483634640-33892-1-git-send-email-tomaszx.kulasek@intel.com> References: <1483607556-21460-1-git-send-email-tomaszx.kulasek@intel.com> <1483634640-33892-1-git-send-email-tomaszx.kulasek@intel.com> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4] rte_mbuf: add rte_pktmbuf_linearize X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Jan 2017 16:51:05 -0000 Hi Tomasz, On Thu, 5 Jan 2017 17:44:00 +0100, Tomasz Kulasek 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 > Cc: Olivier Matz > Signed-off-by: Tomasz Kulasek 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