From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Hanoch Haim (hhaim)" Subject: Re: [PATCH v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix atomic usage Date: Thu, 16 Nov 2017 09:06:56 +0000 Message-ID: References: <20171115091413.27119-1-hhaim@cisco.com> <1D98684F-B8A9-4037-8534-0D4E3A1FD34C@gmail.com> <20171115173058.mrkrv3usbl5sfw3h@platinum> <2fa9a7806c9e447995d6017c6def9894@XCH-RTP-017.cisco.com> <20171116084112.ockgmxnxews7coie@platinum> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Konstantin Ananyev , Ilya Matveychikov , "dev@dpdk.org" To: Olivier MATZ Return-path: Received: from rcdn-iport-4.cisco.com (rcdn-iport-4.cisco.com [173.37.86.75]) by dpdk.org (Postfix) with ESMTP id 6C5F11B1A9 for ; Thu, 16 Nov 2017 10:06:59 +0100 (CET) In-Reply-To: <20171116084112.ockgmxnxews7coie@platinum> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Understood=20 rte_mbuf_refcnt_update_blind()=20 should be good., it will take care the RTE_MBUF_REFCNT_ATOMIC=20 Hanoh -----Original Message----- From: Olivier MATZ [mailto:olivier.matz@6wind.com]=20 Sent: Thursday, November 16, 2017 10:42 AM To: Hanoch Haim (hhaim) Cc: Konstantin Ananyev; Ilya Matveychikov; dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix= atomic usage Hi Hanoh, On Thu, Nov 16, 2017 at 07:16:31AM +0000, Hanoch Haim (hhaim) wrote: > Hi Oliver, >=20 > It's hard for me to follow this thread.=20 Yes, here are some few tips to make it easier to follow: - avoid top-posting - prefix quoted lines with "> " - describe the problem and how you solve it in the commit log - one problem =3D one patch > 1) It is not about clear/not-clear, it is error prone to *replicate* cod= e that has the same logic. >=20 > "I'm not convinced that: >=20 > __rte_pktmbuf_reset_nb_segs(m); >=20 > is clearer than: >=20 > m->next =3D NULL; > m->nb_segs =3D 1; >=20 > Anyway, I agree this should not be part of this patch. We should only kee= p the fix. > " rte_mbuf_refcnt_update() was not used in rte_pktmbuf_prefree_seg() to avoid= reading the refcount twice. The problem of having clear or unclear is fundamental. I don't see the poin= t of having a function __rte_pktmbuf_reset_nb_segs(). Keeping the two affec= tations makes things explicit. > 2) This definitely does not look good.=20 > All the point in my patch is to move the ref-cnt operations to set of=20 > API that already taking care of RTE_MBUF_REFCNT_ATOMIC >=20 > + /* We don't use rte_mbuf_refcnt_update() because we alrea= dy > + * tested that refcnt !=3D 1. > + */ > +#ifdef RTE_MBUF_REFCNT_ATOMIC > + ret =3D rte_atomic16_add_return(&m->refcnt_atomic, -1);=20 > +#else > + ret =3D --m->refcnt; > +#endif > + if (ret !=3D 0) > + return NULL; >=20 We cannot use the existing API taking care of atomic refcount rte_mbuf_refcnt_update() because it would read the refcount twice. We cannot change the behavior of rte_mbuf_refcnt_update() because it's a pu= blic API. An option proposed by Konstantin is to introduce a new helper rte_mbuf_refcnt_update_blind() that does the same than rte_mbuf_refcnt_update() but without the first test. It think it is a bit = overkill to have this function for one caller. That's why I end up with this patch. I don't see why it would be an issue t= o have a mbuf ifdef inside the mbuf code. Olivier