From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 8EEEFAD9F for ; Wed, 18 May 2016 13:59:07 +0200 (CEST) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1b30A0-0007kX-W8; Wed, 18 May 2016 14:01:12 +0200 To: Hiroyuki Mikita , thomas.monjalon@6wind.com, konstantin.ananyev@intel.com References: <1463417600-20943-1-git-send-email-h.mikita89@gmail.com> <1463502902-5295-1-git-send-email-h.mikita89@gmail.com> Cc: dev@dpdk.org From: Olivier Matz Message-ID: <573C58FE.3020101@6wind.com> Date: Wed, 18 May 2016 13:58:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: <1463502902-5295-1-git-send-email-h.mikita89@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3] mbuf: decrease refcnt when detaching 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, 18 May 2016 11:59:07 -0000 Hi Hiroyuki, Thanks for submitting a new version. There are some styling issues in the patch, I highlighted them below but you can check them by using checkpatch: DPDK_CHECKPATCH_PATH=/path/to/linux/checkpatch.pl \ scripts/checkpatches.sh file.patch On 05/17/2016 06:35 PM, Hiroyuki Mikita wrote: > The rte_pktmbuf_detach() function should decrease refcnt on a direct > buffer. > > Signed-off-by: Hiroyuki Mikita > > [...] > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > index 529debb..299b60e 100644 > --- a/lib/librte_mbuf/rte_mbuf.h > +++ b/lib/librte_mbuf/rte_mbuf.h > @@ -1408,6 +1408,8 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool, > * > * After attachment we refer the mbuf we attached as 'indirect', > * while mbuf we attached to as 'direct'. > + * The direct mbuf's reference counter is incremented. > + * ERROR:TRAILING_WHITESPACE: trailing whitespace #82: FILE: lib/librte_mbuf/rte_mbuf.h:1412: + * $ > * Right now, not supported: > * - attachment for already indirect mbuf (e.g. - mi has to be direct). > * - mbuf we trying to attach (mi) is used by someone else > @@ -1462,12 +1464,15 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m) > * - restore original mbuf address and length values. > * - reset pktmbuf data and data_len to their default values. > * All other fields of the given packet mbuf will be left intact. > + * - decrement the direct mbuf's reference counter. > + * When the reference counter becomes 0, the direct mbuf is freed. Minor comment here: I think something like that would be clearer: * - restore original mbuf address and length values. * - reset pktmbuf data and data_len to their default values. * - decrement the direct mbuf's reference counter. When the * reference counter becomes 0, the direct mbuf is freed. * * All other fields of the given packet mbuf will be left intact. > * > * @param m > * The indirect attached packet mbuf. > */ > static inline void rte_pktmbuf_detach(struct rte_mbuf *m) > { > + struct rte_mbuf *md = rte_mbuf_from_indirect(m); > struct rte_mempool *mp = m->pool; > uint32_t mbuf_size, buf_len, priv_size; > > @@ -1482,6 +1487,10 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m) > m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len); > m->data_len = 0; > m->ol_flags = 0; > + > + if (rte_mbuf_refcnt_update(md, -1) == 0) { > + __rte_mbuf_raw_free(md); > + } > } WARNING:BRACES: braces {} are not necessary for single statement blocks #107: FILE: lib/librte_mbuf/rte_mbuf.h:1491: + if (rte_mbuf_refcnt_update(md, -1) == 0) { + __rte_mbuf_raw_free(md); + } > > static inline struct rte_mbuf* __attribute__((always_inline)) > @@ -1491,15 +1500,9 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m) > > if (likely(rte_mbuf_refcnt_update(m, -1) == 0)) { > > - /* if this is an indirect mbuf, then > - * - detach mbuf > - * - free attached mbuf segment > - */ > + /* if this is an indirect mbuf, it is detached. */ > if (RTE_MBUF_INDIRECT(m)) { > - struct rte_mbuf *md = rte_mbuf_from_indirect(m); > rte_pktmbuf_detach(m); > - if (rte_mbuf_refcnt_update(md, -1) == 0) > - __rte_mbuf_raw_free(md); > } > return m; > } > It's not seen by checkpatch, but the braces could also be removed here. Apart from this, it looks good to me. I'm ok with not providing a compat function as we could consider it was a bug. Thanks for working on this. Olivier