From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 43E2E6968 for ; Tue, 17 May 2016 13:06:34 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP; 17 May 2016 04:06:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,324,1459839600"; d="scan'208";a="978656780" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga002.jf.intel.com with ESMTP; 17 May 2016 04:06:32 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.130]) by IRSMSX151.ger.corp.intel.com ([169.254.4.7]) with mapi id 14.03.0248.002; Tue, 17 May 2016 12:06:28 +0100 From: "Ananyev, Konstantin" To: Hiroyuki Mikita , "olivier.matz@6wind.com" CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2] mbuf: decrease refcnt when detaching Thread-Index: AQHRr5OGTZbJUl8op0mwi7XajqfNVp+892Eg Date: Tue, 17 May 2016 11:06:27 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836B51838@irsmsx105.ger.corp.intel.com> References: <1463327436-6863-1-git-send-email-h.mikita89@gmail.com> <1463417600-20943-1-git-send-email-h.mikita89@gmail.com> In-Reply-To: <1463417600-20943-1-git-send-email-h.mikita89@gmail.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2] 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: Tue, 17 May 2016 11:06:34 -0000 Hi Hiroyuki, >=20 > The rte_pktmbuf_detach() function should decrease refcnt on a direct > buffer. >=20 > Signed-off-by: Hiroyuki Mikita > --- > v2: > * introduced a new function rte_pktmbuf_detach2() which decrease refcnt. > * marked rte_pktmbuf_detach() as deprecated. > * added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_det= ach(). > * checked refcnt when detaching in unit tests. > * added this issue to release notes. >=20 > app/test/test_mbuf.c | 9 +++++-- > doc/guides/rel_notes/release_16_07.rst | 11 ++++----- > lib/librte_mbuf/rte_mbuf.h | 43 ++++++++++++++++++++++++++++= +++--- > 3 files changed, 51 insertions(+), 12 deletions(-) >=20 > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c > index 98ff93a..2bf05eb 100644 > --- a/app/test/test_mbuf.c > +++ b/app/test/test_mbuf.c > @@ -438,6 +438,7 @@ test_attach_from_different_pool(void) > struct rte_mbuf *clone =3D NULL; > struct rte_mbuf *clone2 =3D NULL; > char *data, *c_data, *c_data2; > + uint16_t refcnt; >=20 > /* alloc a mbuf */ > m =3D rte_pktmbuf_alloc(pktmbuf_pool); > @@ -508,13 +509,17 @@ test_attach_from_different_pool(void) > GOTO_FAIL("invalid refcnt in m\n"); >=20 > /* detach the clones */ > - rte_pktmbuf_detach(clone); > + refcnt =3D rte_pktmbuf_detach2(clone); > if (c_data !=3D rte_pktmbuf_mtod(clone, char *)) > GOTO_FAIL("clone was not detached properly\n"); > + if (refcnt !=3D 2 || rte_mbuf_refcnt_read(m) !=3D 2) > + GOTO_FAIL("invalid refcnt in m\n"); >=20 > - rte_pktmbuf_detach(clone2); > + refcnt =3D rte_pktmbuf_detach2(clone2); > if (c_data2 !=3D rte_pktmbuf_mtod(clone2, char *)) > GOTO_FAIL("clone2 was not detached properly\n"); > + if (refcnt !=3D 1 || rte_mbuf_refcnt_read(m) !=3D 1) > + GOTO_FAIL("invalid refcnt in m\n"); >=20 > /* free the clones and the initial mbuf */ > rte_pktmbuf_free(clone2); > diff --git a/doc/guides/rel_notes/release_16_07.rst b/doc/guides/rel_note= s/release_16_07.rst > index f6d543c..9678c1f 100644 > --- a/doc/guides/rel_notes/release_16_07.rst > +++ b/doc/guides/rel_notes/release_16_07.rst > @@ -77,13 +77,10 @@ Other > Known Issues > ------------ >=20 > -This section should contain new known issues in this release. Sample for= mat: > - > -* **Add title in present tense with full stop.** > - > - Add a short 1-2 sentence description of the known issue in the present > - tense. Add information on any known workarounds. > - > +* The ``rte_pktmbuf_detach()`` function does not decrement the direct > + mbuf's reference counter. It leads a memory leak of the direct > + mbuf. The workaround is to explicitly decrement the reference > + counter or use ``rte_pktmbuf_detach2()``. >=20 > API Changes > ----------- > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > index 529debb..c0a592d 100644 > --- a/lib/librte_mbuf/rte_mbuf.h > +++ b/lib/librte_mbuf/rte_mbuf.h > @@ -1408,6 +1408,7 @@ 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. > * 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 > @@ -1459,15 +1460,50 @@ static inline void rte_pktmbuf_attach(struct rte_= mbuf *mi, struct rte_mbuf *m) > /** > * Detach an indirect packet mbuf. > * > + * Note: It is deprecated. > + * The direct mbuf's reference counter is not decremented. > + * > + * - 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. > + * > + * @param m > + * The indirect attached packet mbuf. > + */ > +static inline void __rte_deprecated rte_pktmbuf_detach(struct rte_mbuf *= m) > +{ > + struct rte_mempool *mp =3D m->pool; > + uint32_t mbuf_size, buf_len, priv_size; > + > + priv_size =3D rte_pktmbuf_priv_size(mp); > + mbuf_size =3D sizeof(struct rte_mbuf) + priv_size; > + buf_len =3D rte_pktmbuf_data_room_size(mp); > + > + m->priv_size =3D priv_size; > + m->buf_addr =3D (char *)m + mbuf_size; > + m->buf_physaddr =3D rte_mempool_virt2phy(mp, m) + mbuf_size; > + m->buf_len =3D (uint16_t)buf_len; > + m->data_off =3D RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len); > + m->data_len =3D 0; > + m->ol_flags =3D 0; > +} I still think it would be good to have a separate function for what rte_pkt= mbuf_detach() Is doing right now: restore original values of indirect mbuf. Probably rename it to rte_pktmbuf_restore() or so and make _detach2() (unat= ach() ?) to call it internally.=20 > + > +/** > + * Detach an indirect packet mbuf. > + * > * - 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. > * > * @param m > * The indirect attached packet mbuf. > + * @return > + * The updated value of the direct mbuf's reference counter. > */ > -static inline void rte_pktmbuf_detach(struct rte_mbuf *m) > +static inline uint16_t rte_pktmbuf_detach2(struct rte_mbuf *m) > { > + struct rte_mbuf *md =3D rte_mbuf_from_indirect(m); > struct rte_mempool *mp =3D m->pool; > uint32_t mbuf_size, buf_len, priv_size; >=20 > @@ -1482,6 +1518,8 @@ static inline void rte_pktmbuf_detach(struct rte_mb= uf *m) > m->data_off =3D RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len); > m->data_len =3D 0; > m->ol_flags =3D 0; > + > + return rte_mbuf_refcnt_update(md, -1); > } >=20 > static inline struct rte_mbuf* __attribute__((always_inline)) > @@ -1497,8 +1535,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m) > */ > if (RTE_MBUF_INDIRECT(m)) { > struct rte_mbuf *md =3D rte_mbuf_from_indirect(m); I don't think there is a need to invoke rte_mbuf_from_indirect() twice. You can either pass md as a second parameter to _detach2(), or make detach2() to invoke __rte_mbuf_raw_free() if rte_mbuf_refcnt_update(md, -1) =3D=3D 0. Konstantin > - rte_pktmbuf_detach(m); > - if (rte_mbuf_refcnt_update(md, -1) =3D=3D 0) > + if (rte_pktmbuf_detach2(m) =3D=3D 0) > __rte_mbuf_raw_free(md); > } > return m; > -- > 1.9.1