From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id 74B593237 for ; Wed, 3 Feb 2016 18:23:20 +0100 (CET) Received: from [10.16.0.195] (unknown [10.16.0.195]) by proxy.6wind.com (Postfix) with ESMTP id 9480B25D40; Wed, 3 Feb 2016 18:22:36 +0100 (CET) Message-ID: <56B23779.3090407@6wind.com> Date: Wed, 03 Feb 2016 18:23:05 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0 MIME-Version: 1.0 To: "Mrzyglod, DanielX T" , Stephen Hemminger , "dev@dpdk.org" References: <1436485068-30609-1-git-send-email-stephen@networkplumber.org> <1436485068-30609-2-git-send-email-stephen@networkplumber.org> <7ADD74816B4C8A45B56203CBA65FE5A63346A5A4@IRSMSX107.ger.corp.intel.com> In-Reply-To: <7ADD74816B4C8A45B56203CBA65FE5A63346A5A4@IRSMSX107.ger.corp.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: Mike Davison , Stephen Hemminger Subject: Re: [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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Feb 2016 17:23:20 -0000 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