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 863CA43688; Wed, 6 Dec 2023 11:21:33 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1C69D402CF; Wed, 6 Dec 2023 11:21:33 +0100 (CET) Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by mails.dpdk.org (Postfix) with ESMTP id 4EBD5402C6 for ; Wed, 6 Dec 2023 11:21:31 +0100 (CET) Received: from mail.maildlp.com (unknown [172.18.186.231]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4SlYL86Hx3z6K8wt; Wed, 6 Dec 2023 18:19:44 +0800 (CST) Received: from frapeml100008.china.huawei.com (unknown [7.182.85.131]) by mail.maildlp.com (Postfix) with ESMTPS id 3245314038F; Wed, 6 Dec 2023 18:21:30 +0800 (CST) Received: from frapeml500007.china.huawei.com (7.182.85.172) by frapeml100008.china.huawei.com (7.182.85.131) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Wed, 6 Dec 2023 11:21:30 +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; Wed, 6 Dec 2023 11:21:29 +0100 From: Konstantin Ananyev To: Bruce Richardson , Stephen Hemminger CC: =?iso-8859-1?Q?Morten_Br=F8rup?= , Feifei Wang , "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+YrCYrQIAgABJ9MCAAgNpAIABAXqAgAAR/SA= Date: Wed, 6 Dec 2023 10:21:29 +0000 Message-ID: References: <20231204023910.1714667-1-feifei.wang2@arm.com> <98CBD80474FA8B44BF855DF32C47DC35E9F081@smartserver.smartshare.dk> <20231205105032.4b81e3da@hermes.local> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.48.148.249] 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 > > > > > > > > NAK. > > > > > > > > This patch is not race safe. > > > > > > +1, It is a bad idea. > > > > The patch does raise a couple of issues that could be addressed by > > rearranging. There is duplicate code, and there are no comments > > to explain the rationale. > > > > Maybe something like the following (untested): > > > > diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h > > index 286b32b788a5..b43c055fbe3f 100644 > > --- a/lib/mbuf/rte_mbuf.h > > +++ b/lib/mbuf/rte_mbuf.h > > @@ -1342,42 +1342,32 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m) > > { > > __rte_mbuf_sanity_check(m, 0); > > > > - if (likely(rte_mbuf_refcnt_read(m) =3D=3D 1)) { > > - > > - 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; > > - > > - 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; > > + if (likely(rte_mbuf_refcnt_read(m) !=3D 1) ) { >=20 > =3D=3D 1 >=20 > > + /* If this is the only reference to the mbuf it can just > > + * be setup for reuse without modifying reference count. > > + */ > > + } else if (unlikely(__rte_mbuf_refcnt_update(m, -1) =3D=3D 0)) { > > + /* This was last reference reset to 1 for recycling/free. */ > > rte_mbuf_refcnt_set(m, 1); > > + } else { > > + /* mbuf is still in use */ > > + return NULL; > > + } > > >=20 > This seems much clearer with good comments. Then, it could be just: /* put whatever likely/unlikely we believe would be the most common case */ if (rte_mbuf_refcnt_read(m) !=3D 1 && __rte_mbuf_refcnt_update(m, -1) !=3D = 0) return NULL; /* do whatever cleanup is necessary */ =20 >=20 > > - return m; > > + 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; > > } > > - return NULL; > > + > > + if (m->next !=3D NULL) > > + m->next =3D NULL; > > + if (m->nb_segs !=3D 1) > > + m->nb_segs =3D 1; > > + return m; > > } > > > > /**