Message ID | 1436485068-30609-2-git-send-email-stephen@networkplumber.org (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id B44A468A5; Fri, 10 Jul 2015 01:37:44 +0200 (CEST) Received: from mail-pd0-f174.google.com (mail-pd0-f174.google.com [209.85.192.174]) by dpdk.org (Postfix) with ESMTP id 50D6F5A74 for <dev@dpdk.org>; Fri, 10 Jul 2015 01:37:40 +0200 (CEST) Received: by pdbep18 with SMTP id ep18so173107328pdb.1 for <dev@dpdk.org>; Thu, 09 Jul 2015 16:37:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=FaEfvhlfupNDIsTjLCR7C4aqp50X05jxZ2TS+8E6ZBM=; b=fZ4w+gsNImAv+KO/hFv7+nGxNcSjtHttzpzKbl/kNPm+VlPuVNx9ZTxsNBpAkuqmkf zGeKrVV8Sjl4uezkxet/kKResJQEGHJ4mMbd2pnptrSNJ1nNZL7tUP9T4e6kZRSsFjTO acghf8jh+PB0R5D2kRVCLeNHqTvpU4osCijhEJMnkK5SSjB3NMu3fWMYn26/F18pYYNt cghkUZe9BJ4TLJcN5UAxFjorOvsZ94FENfGwzyF3ud8XMROBq3cu7Fi7LIC9m9F5KX2I BpPv1SgBsrre5UWwCFD13dK8lqVVc+apCAMDfGeka0BGQlpJaEp/qYHv4Y8TF0VdBmA6 lmUw== X-Gm-Message-State: ALoCoQkcouCJJg0ut+wYEeTTLjiZVoxnI6e0PBjlW1K7cLEAMhEZcmVwIEYSR0/fqwZZ3/P1m9bd X-Received: by 10.68.167.66 with SMTP id zm2mr36192589pbb.164.1436485059750; Thu, 09 Jul 2015 16:37:39 -0700 (PDT) Received: from urahara.home.lan (static-50-53-82-155.bvtn.or.frontiernet.net. [50.53.82.155]) by smtp.gmail.com with ESMTPSA id cq5sm7232038pad.11.2015.07.09.16.37.38 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 09 Jul 2015 16:37:39 -0700 (PDT) From: Stephen Hemminger <stephen@networkplumber.org> To: dev@dpdk.org Date: Thu, 9 Jul 2015 16:37:47 -0700 Message-Id: <1436485068-30609-2-git-send-email-stephen@networkplumber.org> X-Mailer: git-send-email 2.1.4 In-Reply-To: <1436485068-30609-1-git-send-email-stephen@networkplumber.org> References: <1436485068-30609-1-git-send-email-stephen@networkplumber.org> Cc: Mike Davison <mdavison@brocade.com>, Stephen Hemminger <shemming@brocade.com> Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Stephen Hemminger
July 9, 2015, 11:37 p.m. UTC
From: Stephen Hemminger <shemming@brocade.com> Added rte_pktmbuf_copy() function since copying multi-part segments is common issue and can be problematic. Signed-off-by: Mike Davison <mdavison@brocade.com> Reviewed-by: Stephen Hemminger <shemming@brocade.com> --- lib/librte_mbuf/rte_mbuf.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)
Comments
On 07/10/2015 01:37 AM, Stephen Hemminger wrote: > From: Stephen Hemminger <shemming@brocade.com> > > Added rte_pktmbuf_copy() function since copying multi-part > segments is common issue and can be problematic. > > Signed-off-by: Mike Davison <mdavison@brocade.com> > Reviewed-by: Stephen Hemminger <shemming@brocade.com> > --- > lib/librte_mbuf/rte_mbuf.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > index 80419df..f0a543b 100644 > --- a/lib/librte_mbuf/rte_mbuf.h > +++ b/lib/librte_mbuf/rte_mbuf.h > @@ -60,6 +60,7 @@ > #include <rte_atomic.h> > #include <rte_prefetch.h> > #include <rte_branch_prediction.h> > +#include <rte_memcpy.h> > > #ifdef __cplusplus > extern "C" { > @@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m) > return !!(m->nb_segs == 1); > } > > +/* > + * Creates a copy of the given packet mbuf. > + * > + * Walks through all segments of the given packet mbuf, and for each of them: > + * - Creates a new packet mbuf from the given pool. > + * - Copy segment to newly created mbuf. > + * Then updates pkt_len and nb_segs of the new packet mbuf to match values > + * from the original packet mbuf. > + * > + * @param md > + * The packet mbuf to be copied. > + * @param mp > + * The mempool from which the mbufs are allocated. > + * @return > + * - The pointer to the new mbuf on success. > + * - NULL if allocation fails. > + */ > +static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md, > + struct rte_mempool *mp) > +{ > + struct rte_mbuf *mc = NULL; > + struct rte_mbuf **prev = &mc; > + > + do { > + struct rte_mbuf *mi; > + > + mi = rte_pktmbuf_alloc(mp); > + if (unlikely(mi == NULL)) { > + rte_pktmbuf_free(mc); > + return NULL; > + } > + I think we should check that the destination mbuf is large enough to store the segment data. Then we have 2 options on failure: - split the original segment into several destination segments - return an error > + mi->data_off = md->data_off; > + mi->data_len = md->data_len; > + mi->port = md->port; > + mi->vlan_tci = md->vlan_tci; > + mi->tx_offload = md->tx_offload; > + mi->hash = md->hash; > + > + mi->next = NULL; > + mi->pkt_len = md->pkt_len; > + mi->nb_segs = md->nb_segs; > + mi->ol_flags = md->ol_flags; > + mi->packet_type = md->packet_type; > + > + rte_memcpy(rte_pktmbuf_mtod(mi, char *), > + rte_pktmbuf_mtod(md, char *), > + md->data_len); > + > + *prev = mi; > + prev = &mi->next; > + } while ((md = md->next) != NULL); > + > + *prev = NULL; > + __rte_mbuf_sanity_check(mc, 1); > + return mc; > +} > + > /** > * Dump an mbuf structure to the console. > * >
>-----Original Message----- >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger >Sent: Friday, July 10, 2015 1:38 AM >To: dev@dpdk.org >Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger ><shemming@brocade.com> >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy > >From: Stephen Hemminger <shemming@brocade.com> > >Added rte_pktmbuf_copy() function since copying multi-part >segments is common issue and can be problematic. > >Signed-off-by: Mike Davison <mdavison@brocade.com> >Reviewed-by: Stephen Hemminger <shemming@brocade.com> >--- > lib/librte_mbuf/rte_mbuf.h | 59 >++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > >diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h >index 80419df..f0a543b 100644 >--- a/lib/librte_mbuf/rte_mbuf.h >+++ b/lib/librte_mbuf/rte_mbuf.h >@@ -60,6 +60,7 @@ > #include <rte_atomic.h> > #include <rte_prefetch.h> > #include <rte_branch_prediction.h> >+#include <rte_memcpy.h> > > #ifdef __cplusplus > extern "C" { >@@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const >struct rte_mbuf *m) > return !!(m->nb_segs == 1); > } > >+/* >+ * Creates a copy of the given packet mbuf. >+ * >+ * Walks through all segments of the given packet mbuf, and for each of them: >+ * - Creates a new packet mbuf from the given pool. >+ * - Copy segment to newly created mbuf. >+ * Then updates pkt_len and nb_segs of the new packet mbuf to match values >+ * from the original packet mbuf. >+ * >+ * @param md >+ * The packet mbuf to be copied. >+ * @param mp >+ * The mempool from which the mbufs are allocated. >+ * @return >+ * - The pointer to the new mbuf on success. >+ * - NULL if allocation fails. >+ */ >+static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md, >+ struct rte_mempool *mp) >+{ >+ struct rte_mbuf *mc = NULL; >+ struct rte_mbuf **prev = &mc; >+ >+ do { >+ struct rte_mbuf *mi; >+ >+ mi = rte_pktmbuf_alloc(mp); >+ if (unlikely(mi == NULL)) { >+ rte_pktmbuf_free(mc); >+ return NULL; >+ } >+ >+ mi->data_off = md->data_off; >+ mi->data_len = md->data_len; >+ mi->port = md->port; >+ mi->vlan_tci = md->vlan_tci; >+ mi->tx_offload = md->tx_offload; >+ mi->hash = md->hash; >+ >+ mi->next = NULL; >+ mi->pkt_len = md->pkt_len; >+ mi->nb_segs = md->nb_segs; >+ mi->ol_flags = md->ol_flags; >+ mi->packet_type = md->packet_type; >+ >+ rte_memcpy(rte_pktmbuf_mtod(mi, char *), >+ rte_pktmbuf_mtod(md, char *), >+ md->data_len); >+ >+ *prev = mi; >+ prev = &mi->next; >+ } while ((md = md->next) != NULL); >+ >+ *prev = NULL; >+ __rte_mbuf_sanity_check(mc, 1); >+ return mc; >+} >+ > /** > * Dump an mbuf structure to the console. > * >-- >2.1.4 Hi Stephen :> This patch look useful in case of backup buffs. There will be second approach ?
On Fri, 22 Jan 2016 13:40:44 +0000 "Mrzyglod, DanielX T" <danielx.t.mrzyglod@intel.com> wrote: > > > >-----Original Message----- > >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger > >Sent: Friday, July 10, 2015 1:38 AM > >To: dev@dpdk.org > >Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger > ><shemming@brocade.com> > >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy > > > >From: Stephen Hemminger <shemming@brocade.com> > > > >Added rte_pktmbuf_copy() function since copying multi-part > >segments is common issue and can be problematic. > > > >Signed-off-by: Mike Davison <mdavison@brocade.com> > >Reviewed-by: Stephen Hemminger <shemming@brocade.com> > >--- > > lib/librte_mbuf/rte_mbuf.h | 59 > >++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > > >diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > >index 80419df..f0a543b 100644 > >--- a/lib/librte_mbuf/rte_mbuf.h > >+++ b/lib/librte_mbuf/rte_mbuf.h > >@@ -60,6 +60,7 @@ > > #include <rte_atomic.h> > > #include <rte_prefetch.h> > > #include <rte_branch_prediction.h> > >+#include <rte_memcpy.h> > > > > #ifdef __cplusplus > > extern "C" { > >@@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const > >struct rte_mbuf *m) > > return !!(m->nb_segs == 1); > > } > > > >+/* > >+ * Creates a copy of the given packet mbuf. > >+ * > >+ * Walks through all segments of the given packet mbuf, and for each of them: > >+ * - Creates a new packet mbuf from the given pool. > >+ * - Copy segment to newly created mbuf. > >+ * Then updates pkt_len and nb_segs of the new packet mbuf to match values > >+ * from the original packet mbuf. > >+ * > >+ * @param md > >+ * The packet mbuf to be copied. > >+ * @param mp > >+ * The mempool from which the mbufs are allocated. > >+ * @return > >+ * - The pointer to the new mbuf on success. > >+ * - NULL if allocation fails. > >+ */ > >+static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md, > >+ struct rte_mempool *mp) > >+{ > >+ struct rte_mbuf *mc = NULL; > >+ struct rte_mbuf **prev = &mc; > >+ > >+ do { > >+ struct rte_mbuf *mi; > >+ > >+ mi = rte_pktmbuf_alloc(mp); > >+ if (unlikely(mi == NULL)) { > >+ rte_pktmbuf_free(mc); > >+ return NULL; > >+ } > >+ > >+ mi->data_off = md->data_off; > >+ mi->data_len = md->data_len; > >+ mi->port = md->port; > >+ mi->vlan_tci = md->vlan_tci; > >+ mi->tx_offload = md->tx_offload; > >+ mi->hash = md->hash; > >+ > >+ mi->next = NULL; > >+ mi->pkt_len = md->pkt_len; > >+ mi->nb_segs = md->nb_segs; > >+ mi->ol_flags = md->ol_flags; > >+ mi->packet_type = md->packet_type; > >+ > >+ rte_memcpy(rte_pktmbuf_mtod(mi, char *), > >+ rte_pktmbuf_mtod(md, char *), > >+ md->data_len); > >+ > >+ *prev = mi; > >+ prev = &mi->next; > >+ } while ((md = md->next) != NULL); > >+ > >+ *prev = NULL; > >+ __rte_mbuf_sanity_check(mc, 1); > >+ return mc; > >+} > >+ > > /** > > * Dump an mbuf structure to the console. > > * > >-- > >2.1.4 > > Hi Stephen :> > This patch look useful in case of backup buffs. > There will be second approach ? I did bother resubmitting it since unless there are users in current code base it would likely rot.
Hi Stephen, Please find some comments below. On 01/22/2016 02:40 PM, Mrzyglod, DanielX T wrote: >> +/* >> + * Creates a copy of the given packet mbuf. >> + * >> + * Walks through all segments of the given packet mbuf, and for each of them: >> + * - Creates a new packet mbuf from the given pool. >> + * - Copy segment to newly created mbuf. >> + * Then updates pkt_len and nb_segs of the new packet mbuf to match values >> + * from the original packet mbuf. >> + * >> + * @param md >> + * The packet mbuf to be copied. >> + * @param mp >> + * The mempool from which the mbufs are allocated. >> + * @return >> + * - The pointer to the new mbuf on success. >> + * - NULL if allocation fails. >> + */ >> +static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md, >> + struct rte_mempool *mp) You are usually against inline functions. Any reason why it's better to inline the code in this case? Also, I think the mbuf argument should be const. >> +{ >> + struct rte_mbuf *mc = NULL; >> + struct rte_mbuf **prev = &mc; >> + >> + do { >> + struct rte_mbuf *mi; >> + >> + mi = rte_pktmbuf_alloc(mp); >> + if (unlikely(mi == NULL)) { >> + rte_pktmbuf_free(mc); >> + return NULL; >> + } >> + >> + mi->data_off = md->data_off; I'm not a big fan of 'mi' and 'md' (and 'mc'). In rte_pktmbuf_attach(), 'md' means direct and 'mi' means indirect. Here, all mbufs are direct (i.e. they points to their own data buffer). I think that using 'm' and 'm2' (or m_dup) is clearer. Also, 'seg' looks clearer for a mbuf that points to a rte_mbuf inside the chain. >> + mi->data_len = md->data_len; >> + mi->port = md->port; We don't need to update port for all the segments here. Same for vlan_tci, tx_offload, hash, pkt_len, nb_segs, ol_flags, packet_type. >> + mi->vlan_tci = md->vlan_tci; >> + mi->tx_offload = md->tx_offload; >> + mi->hash = md->hash; >> + >> + mi->next = NULL; >> + mi->pkt_len = md->pkt_len; >> + mi->nb_segs = md->nb_segs; >> + mi->ol_flags = md->ol_flags; >> + mi->packet_type = md->packet_type; >> + >> + rte_memcpy(rte_pktmbuf_mtod(mi, char *), >> + rte_pktmbuf_mtod(md, char *), >> + md->data_len); >> + >> + *prev = mi; >> + prev = &mi->next; Using a double mbuf pointer here looks a bit overkill to me. I suggest to do something like (just an example, not even compiled): struct rte_mbuf *rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp) { struct rte_mbuf *m2, *seg, *seg2; m2 = rte_pktmbuf_alloc(mp); if (unlikely(m2 == NULL)) { rte_pktmbuf_free(m2); return NULL; } m2->data_off = m->data_off; m2->data_len = m->data_len; m2->pkt_len = m->pkt_len; m2->tx_offload = m->tx_offload; /* ... */ for (seg = m->next; seg != NULL; seg = seg->next) { seg2 = rte_pktmbuf_alloc(mp); if (unlikely(seg2 == NULL)) { rte_pktmbuf_free(m2); return NULL; } seg2->data_off = seg->data_off; /* ... */ seg = seg->next; } return m2; } >> + } while ((md = md->next) != NULL); >> + >> + *prev = NULL; why this? >> + __rte_mbuf_sanity_check(mc, 1); >> + return mc; >> +} >> + I agree this kind of function could be useful. But if there is no user of this code inside the dpdk, I think we must at least have a test function for it in app/test/test_mbuf.c Regards, Olivier
On Wed, 3 Feb 2016 17:23:05 +0000 Olivier MATZ <olivier.matz@6wind.com> wrote: > + } while ((md = md->next) != NULL); > >> + > >> + *prev = NULL; > > why this? This is null terminating the linked list of segments.
Hi, > > >-----Original Message----- > > >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen > > >Hemminger > > >Sent: Friday, July 10, 2015 1:38 AM > > >To: dev@dpdk.org > > >Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger > > ><shemming@brocade.com> > > >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy > > > > > >From: Stephen Hemminger <shemming@brocade.com> > > > > > >Added rte_pktmbuf_copy() function since copying multi-part segments > > >is common issue and can be problematic. > > > > > >Signed-off-by: Mike Davison <mdavison@brocade.com> > > >Reviewed-by: Stephen Hemminger <shemming@brocade.com> > > >--- > > > > Hi Stephen :> > > This patch look useful in case of backup buffs. > > There will be second approach ? > > I did bother resubmitting it since unless there are users in current code base it > would likely rot. I was writing similar mbuf copy functionality which is required for tcpdump feature. But, It was brought to my notice that this patch with similar functionality already exists, so I would like to take this patch and work on further. Also, if you have any further code changes on this patch, would you please send the latest one. I will work further. Thanks, Reshma
On Fri, 18 Mar 2016 17:03:51 +0000 "Pattan, Reshma" <reshma.pattan@intel.com> wrote: > Hi, > > > > >-----Original Message----- > > > >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen > > > >Hemminger > > > >Sent: Friday, July 10, 2015 1:38 AM > > > >To: dev@dpdk.org > > > >Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger > > > ><shemming@brocade.com> > > > >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy > > > > > > > >From: Stephen Hemminger <shemming@brocade.com> > > > > > > > >Added rte_pktmbuf_copy() function since copying multi-part segments > > > >is common issue and can be problematic. > > > > > > > >Signed-off-by: Mike Davison <mdavison@brocade.com> > > > >Reviewed-by: Stephen Hemminger <shemming@brocade.com> > > > >--- > > > > > > Hi Stephen :> > > > This patch look useful in case of backup buffs. > > > There will be second approach ? > > > > I did bother resubmitting it since unless there are users in current code base it > > would likely rot. > > I was writing similar mbuf copy functionality which is required for tcpdump feature. > But, It was brought to my notice that this patch with similar functionality already exists, so I would like to take this patch and work on further. > Also, if you have any further code changes on this patch, would you please send the latest one. I will work further. > > Thanks, > Reshma We have a newer version that handles different size pools.
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h index 80419df..f0a543b 100644 --- a/lib/librte_mbuf/rte_mbuf.h +++ b/lib/librte_mbuf/rte_mbuf.h @@ -60,6 +60,7 @@ #include <rte_atomic.h> #include <rte_prefetch.h> #include <rte_branch_prediction.h> +#include <rte_memcpy.h> #ifdef __cplusplus extern "C" { @@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m) return !!(m->nb_segs == 1); } +/* + * Creates a copy of the given packet mbuf. + * + * Walks through all segments of the given packet mbuf, and for each of them: + * - Creates a new packet mbuf from the given pool. + * - Copy segment to newly created mbuf. + * Then updates pkt_len and nb_segs of the new packet mbuf to match values + * from the original packet mbuf. + * + * @param md + * The packet mbuf to be copied. + * @param mp + * The mempool from which the mbufs are allocated. + * @return + * - The pointer to the new mbuf on success. + * - NULL if allocation fails. + */ +static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md, + struct rte_mempool *mp) +{ + struct rte_mbuf *mc = NULL; + struct rte_mbuf **prev = &mc; + + do { + struct rte_mbuf *mi; + + mi = rte_pktmbuf_alloc(mp); + if (unlikely(mi == NULL)) { + rte_pktmbuf_free(mc); + return NULL; + } + + mi->data_off = md->data_off; + mi->data_len = md->data_len; + mi->port = md->port; + mi->vlan_tci = md->vlan_tci; + mi->tx_offload = md->tx_offload; + mi->hash = md->hash; + + mi->next = NULL; + mi->pkt_len = md->pkt_len; + mi->nb_segs = md->nb_segs; + mi->ol_flags = md->ol_flags; + mi->packet_type = md->packet_type; + + rte_memcpy(rte_pktmbuf_mtod(mi, char *), + rte_pktmbuf_mtod(md, char *), + md->data_len); + + *prev = mi; + prev = &mi->next; + } while ((md = md->next) != NULL); + + *prev = NULL; + __rte_mbuf_sanity_check(mc, 1); + return mc; +} + /** * Dump an mbuf structure to the console. *