From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier Matz Subject: Re: [RFC 3/8] mbuf: set mbuf fields while in pool Date: Tue, 28 Feb 2017 15:51:11 +0100 Message-ID: <20170228155111.2dbee114@platinum> References: <1485271173-13408-1-git-send-email-olivier.matz@6wind.com> <1485271173-13408-4-git-send-email-olivier.matz@6wind.com> <20170124155049.GA172024@bricha3-MOBL3.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org To: Bruce Richardson Return-path: Received: from mail-wm0-f48.google.com (mail-wm0-f48.google.com [74.125.82.48]) by dpdk.org (Postfix) with ESMTP id B150F2B86 for ; Tue, 28 Feb 2017 15:51:13 +0100 (CET) Received: by mail-wm0-f48.google.com with SMTP id v186so87424259wmd.0 for ; Tue, 28 Feb 2017 06:51:13 -0800 (PST) In-Reply-To: <20170124155049.GA172024@bricha3-MOBL3.ger.corp.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Bruce, On Tue, 24 Jan 2017 15:50:49 +0000, Bruce Richardson wrote: > On Tue, Jan 24, 2017 at 04:19:28PM +0100, Olivier Matz wrote: > > Set the value of m->refcnt to 1, m->nb_segs to 1 and m->next > > to NULL when the mbuf is stored inside the mempool (unused). > > This is done in rte_pktmbuf_prefree_seg(), before freeing or > > recycling a mbuf. > > > > Before this patch, the value of m->refcnt was expected to be 0 > > while in pool. > > > > The objectives are: > > > > - to avoid drivers to set m->next to NULL in the early Rx path, > > since this field is in the second 64B of the mbuf and its access > > could trigger a cache miss > > > > - rationalize the behavior of raw_alloc/raw_free: one is now the > > symmetric of the other, and refcnt is never changed in these > > functions. > > > > Signed-off-by: Olivier Matz > > --- > > drivers/net/mlx5/mlx5_rxtx.c | 5 ++--- > > drivers/net/mpipe/mpipe_tilegx.c | 1 + > > lib/librte_mbuf/rte_mbuf.c | 2 ++ > > lib/librte_mbuf/rte_mbuf.h | 45 > > +++++++++++++++++++++++++++++----------- 4 files changed, 38 > > insertions(+), 15 deletions(-) > > > /* compat with older versions */ > > __rte_deprecated > > -static inline void __attribute__((always_inline)) > > +static inline void > > __rte_mbuf_raw_free(struct rte_mbuf *m) > > { > > rte_mbuf_raw_free(m); > > @@ -1218,8 +1232,12 @@ static inline void rte_pktmbuf_detach(struct > > rte_mbuf *m) m->data_len = 0; > > m->ol_flags = 0; > > > > - if (rte_mbuf_refcnt_update(md, -1) == 0) > > + if (rte_mbuf_refcnt_update(md, -1) == 0) { > > Minor nit, but in the case that we only have a single reference to the > mbufs, we are always setting that to zero just to re-increment it to 1 > again. > > > + md->next = NULL; > > + md->nb_segs = 1; > > + rte_mbuf_refcnt_set(md, 1); > > rte_mbuf_raw_free(md); > > + } > > } > > > > /** I'm trying to gather the comments that have been made on this patchset. About this one, I think it would be more complex to change the code to avoid to set the refcnt twice: - we would need to duplicate code from rte_mbuf_refcnt_update(), which I think is not a very good idea, due to the big comment - it would make the detach code less readable - it's even not sure that it will be more performant: since rte_mbuf_refcnt_update() is inline, the compiler is probably able to do the simplification by itself. Olivier