From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CCC5F4366B; Mon, 4 Dec 2023 12:07:11 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4468240DDE; Mon, 4 Dec 2023 12:07:11 +0100 (CET) Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by mails.dpdk.org (Postfix) with ESMTP id AFDE040DD8 for ; Mon, 4 Dec 2023 12:07:09 +0100 (CET) Received: from mail.maildlp.com (unknown [172.18.186.216]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4SkLNB6n5Sz67lZn; Mon, 4 Dec 2023 19:02:18 +0800 (CST) Received: from frapeml500008.china.huawei.com (unknown [7.182.85.71]) by mail.maildlp.com (Postfix) with ESMTPS id 01C89140D1A; Mon, 4 Dec 2023 19:07:09 +0800 (CST) Received: from frapeml500007.china.huawei.com (7.182.85.172) by frapeml500008.china.huawei.com (7.182.85.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Mon, 4 Dec 2023 12:07:08 +0100 Received: from frapeml500007.china.huawei.com ([7.182.85.172]) by frapeml500007.china.huawei.com ([7.182.85.172]) with mapi id 15.01.2507.035; Mon, 4 Dec 2023 12:07:08 +0100 From: Konstantin Ananyev To: =?iso-8859-1?Q?Morten_Br=F8rup?= , Feifei Wang CC: "dev@dpdk.org" , "nd@arm.com" , Ruifeng Wang Subject: RE: [PATCH v1] mbuf: remove the redundant code for mbuf prefree Thread-Topic: [PATCH v1] mbuf: remove the redundant code for mbuf prefree Thread-Index: AQHaJlsfC3gCbwH7q0iB9dYk9Mt+YrCYrQIAgABJ9MA= Date: Mon, 4 Dec 2023 11:07:08 +0000 Message-ID: References: <20231204023910.1714667-1-feifei.wang2@arm.com> <98CBD80474FA8B44BF855DF32C47DC35E9F081@smartserver.smartshare.dk> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F081@smartserver.smartshare.dk> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.206.138.42] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > > For 'rte_pktmbuf_prefree_seg' function, 'rte_mbuf_refcnt_read(m) =3D=3D= 1' > > and '__rte_mbuf_refcnt_update(m, -1) =3D=3D 0' are the same cases where > > mbuf's refcnt value should be 1. Thus we can simplify the code and > > remove the redundant part. > > > > Furthermore, according to [1], when the mbuf is stored inside the > > mempool, the m->refcnt value should be 1. And then it is detached > > from its parent for an indirect mbuf. Thus change the description of > > 'rte_pktmbuf_prefree_seg' function. > > > > [1] https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4- > > olivier.matz@6wind.com/ > > > > Suggested-by: Ruifeng Wang > > Signed-off-by: Feifei Wang > > --- > > lib/mbuf/rte_mbuf.h | 22 +++------------------- > > 1 file changed, 3 insertions(+), 19 deletions(-) > > > > diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h > > index 286b32b788..42e9b50d51 100644 > > --- a/lib/mbuf/rte_mbuf.h > > +++ b/lib/mbuf/rte_mbuf.h > > @@ -1328,7 +1328,7 @@ static inline int > > __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m) > > * > > * This function does the same than a free, except that it does not > > * return the segment to its pool. > > - * It decreases the reference counter, and if it reaches 0, it is > > + * It decreases the reference counter, and if it reaches 1, it is >=20 > No, the original description is correct. > However, the reference counter is set to 1 when put back in the pool, as = a shortcut so it isn't needed to be set back to 1 when > allocated from the pool. >=20 > > * detached from its parent for an indirect mbuf. > > * > > * @param m > > @@ -1358,25 +1358,9 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m) >=20 > The preceding "if (likely(rte_mbuf_refcnt_read(m) =3D=3D 1)) {" is only a= shortcut for the likely case. >=20 > > m->nb_segs =3D 1; > > > > return m; > > - > > - } else if (__rte_mbuf_refcnt_update(m, -1) =3D=3D 0) { > > - > > - if (!RTE_MBUF_DIRECT(m)) { > > - rte_pktmbuf_detach(m); > > - if (RTE_MBUF_HAS_EXTBUF(m) && > > - RTE_MBUF_HAS_PINNED_EXTBUF(m) && > > - __rte_pktmbuf_pinned_extbuf_decref(m)) > > - return NULL; > > - } > > - > > - if (m->next !=3D NULL) > > - m->next =3D NULL; > > - if (m->nb_segs !=3D 1) > > - m->nb_segs =3D 1; > > - rte_mbuf_refcnt_set(m, 1); > > - > > - return m; > > } > > + > > + __rte_mbuf_refcnt_update(m, -1); > > return NULL; > > } > > > > -- > > 2.25.1 >=20 > NAK. >=20 > This patch is not race safe.=20 +1, It is a bad idea. > With the patch: >=20 > This thread: > if (likely(rte_mbuf_refcnt_read(m) =3D=3D 1)) { // Assume it's 2. >=20 > The other thread: > if (likely(rte_mbuf_refcnt_read(m) =3D=3D 1)) { // It's 2. > __rte_mbuf_refcnt_update(m, -1); // Now it's 1. > return NULL; >=20 > This thread: > __rte_mbuf_refcnt_update(m, -1); // Now it's 0. > return NULL; >=20 > None of the threads have done the "prefree" work.