From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 76E681B29D for ; Thu, 16 Nov 2017 09:42:19 +0100 (CET) Received: from lfbn-1-6068-189.w90-110.abo.wanadoo.fr ([90.110.3.189] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1eFFqL-0001NK-Do; Thu, 16 Nov 2017 09:48:18 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Thu, 16 Nov 2017 09:42:04 +0100 Date: Thu, 16 Nov 2017 09:42:04 +0100 From: Olivier MATZ To: "Hanoch Haim (hhaim)" Cc: Konstantin Ananyev , Ilya Matveychikov , "dev@dpdk.org" Message-ID: <20171116084112.ockgmxnxews7coie@platinum> References: <20171115091413.27119-1-hhaim@cisco.com> <1D98684F-B8A9-4037-8534-0D4E3A1FD34C@gmail.com> <20171115173058.mrkrv3usbl5sfw3h@platinum> <2fa9a7806c9e447995d6017c6def9894@XCH-RTP-017.cisco.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2fa9a7806c9e447995d6017c6def9894@XCH-RTP-017.cisco.com> User-Agent: NeoMutt/20170113 (1.7.2) 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 08:42:19 -0000 Hi Hanoh, On Thu, Nov 16, 2017 at 07:16:31AM +0000, Hanoch Haim (hhaim) wrote: > Hi Oliver, > > It's hard for me to follow this thread. 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 = one patch > 1) It is not about clear/not-clear, it is error prone to *replicate* code that has the same logic. > > "I'm not convinced that: > > __rte_pktmbuf_reset_nb_segs(m); > > is clearer than: > > m->next = NULL; > m->nb_segs = 1; > > Anyway, I agree this should not be part of this patch. We should only keep 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 point of having a function __rte_pktmbuf_reset_nb_segs(). Keeping the two affectations makes things explicit. > 2) This definitely does not look good. > All the point in my patch is to move the ref-cnt operations to set of API that already taking care of RTE_MBUF_REFCNT_ATOMIC > > + /* We don't use rte_mbuf_refcnt_update() because we already > + * tested that refcnt != 1. > + */ > +#ifdef RTE_MBUF_REFCNT_ATOMIC > + ret = rte_atomic16_add_return(&m->refcnt_atomic, -1); > +#else > + ret = --m->refcnt; > +#endif > + if (ret != 0) > + return NULL; > 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 public 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 to have a mbuf ifdef inside the mbuf code. Olivier