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 CA9624365D; Mon, 4 Dec 2023 08:41:12 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A7E75402E0; Mon, 4 Dec 2023 08:41:12 +0100 (CET) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 4A6A8402AE for ; Mon, 4 Dec 2023 08:41:11 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 50A4120CCC; Mon, 4 Dec 2023 08:41:10 +0100 (CET) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v1] mbuf: remove the redundant code for mbuf prefree X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Mon, 4 Dec 2023 08:41:08 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F081@smartserver.smartshare.dk> In-Reply-To: <20231204023910.1714667-1-feifei.wang2@arm.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v1] mbuf: remove the redundant code for mbuf prefree Thread-Index: AdomWxl/9+k7SGlHSvSs1W1J4dp0mgAKGuDg References: <20231204023910.1714667-1-feifei.wang2@arm.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Feifei Wang" Cc: , , "Ruifeng Wang" 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 > From: Feifei Wang [mailto:feifei.wang2@arm.com] > Sent: Monday, 4 December 2023 03.39 >=20 > 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. >=20 > 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. >=20 > [1] = https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4- > olivier.matz@6wind.com/ >=20 > Suggested-by: Ruifeng Wang > Signed-off-by: Feifei Wang > --- > lib/mbuf/rte_mbuf.h | 22 +++------------------- > 1 file changed, 3 insertions(+), 19 deletions(-) >=20 > 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 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. > * detached from its parent for an indirect mbuf. > * > * @param m > @@ -1358,25 +1358,9 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m) The preceding "if (likely(rte_mbuf_refcnt_read(m) =3D=3D 1)) {" is only = a shortcut for the likely case. > m->nb_segs =3D 1; >=20 > 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; > } >=20 > -- > 2.25.1 NAK. This patch is not race safe. With the patch: This thread: if (likely(rte_mbuf_refcnt_read(m) =3D=3D 1)) { // Assume it's 2. 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; This thread: __rte_mbuf_refcnt_update(m, -1); // Now it's 0. return NULL; None of the threads have done the "prefree" work.