From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=2660; q=dns/txt; s=iport; t=1510823219; x=1512032819; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=MxEDutYIVGZJJnr2ViyN5kfak46CE2z+UWc5lZpmkrM=; b=CZZhDHVXMYEC/QXFSa0pU/ds4iHUfO6TPpx5pB+aoqqfjd2QgZueD3Yr IkdmK/iI8J9+bo87mrtCygrTKA3Los6IvwnHYSgNDLABBc3t/zH9f9DRD 7UeGwn98W/R14Qin17GuWQ5+AlLs5WLPGXYyqQkUa7MS6TbWY/mRbxMbv Y=; X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0DtAACdVA1a/4QNJK1eGQEBAQEBAQEBA?= =?us-ascii?q?QEBAQcBAQEBAYM2gVInB44XjyCBfZFuhHGCEQqFOwKFFT8YAQEBAQEBAQEBayi?= =?us-ascii?q?FHgEBAQECAScTPwUHBAIBCBEEAQEBHgkHMhQJCAIEDgUIihQIq0s6ixMBAQEBA?= =?us-ascii?q?QEBAQEBAQEBAQEBAQEBAQEdgzSCB4FVgWmDK4sQBaI7ApR/gh6KEIcjSJU7AhE?= =?us-ascii?q?ZAYE4AR84gXR6FYMthF93ihwBgRABAQE?= X-IronPort-AV: E=Sophos;i="5.44,402,1505779200"; d="scan'208";a="321292759" Received: from alln-core-10.cisco.com ([173.36.13.132]) by rcdn-iport-4.cisco.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 16 Nov 2017 09:06:57 +0000 Received: from XCH-RTP-017.cisco.com (xch-rtp-017.cisco.com [64.101.220.157]) by alln-core-10.cisco.com (8.14.5/8.14.5) with ESMTP id vAG96vXY030845 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Thu, 16 Nov 2017 09:06:57 GMT Received: from xch-rtp-017.cisco.com (64.101.220.157) by XCH-RTP-017.cisco.com (64.101.220.157) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Thu, 16 Nov 2017 04:06:56 -0500 Received: from xch-rtp-017.cisco.com ([64.101.220.157]) by XCH-RTP-017.cisco.com ([64.101.220.157]) with mapi id 15.00.1320.000; Thu, 16 Nov 2017 04:06:56 -0500 From: "Hanoch Haim (hhaim)" To: Olivier MATZ CC: Konstantin Ananyev , Ilya Matveychikov , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix atomic usage Thread-Index: AQHTXg+5U6hJZK650Ei3mGGQZg+VYqMWBrKAgACRIhCAAG1sAP//supQ 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> In-Reply-To: <20171116084112.ockgmxnxews7coie@platinum> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [64.103.125.33] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix atomic usage X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Nov 2017 09:06:59 -0000 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