From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [PATCH] ixgbe: fix icc issue with mbuf initializer Date: Mon, 3 Nov 2014 13:16:54 +0000 Message-ID: <20141103131654.GC4840@bricha3-MOBL3> References: <1415013076-30314-1-git-send-email-bruce.richardson@intel.com> <20141103124732.GB4840@bricha3-MOBL3> <2987294.1LlcqdoeYZ@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev-VfR2kkLFssw@public.gmane.org To: Thomas Monjalon Return-path: Content-Disposition: inline In-Reply-To: <2987294.1LlcqdoeYZ@xps13> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Mon, Nov 03, 2014 at 01:59:16PM +0100, Thomas Monjalon wrote: > 2014-11-03 12:47, Bruce Richardson: > > On Mon, Nov 03, 2014 at 01:31:10PM +0100, David Marchand wrote: > > > On Mon, Nov 3, 2014 at 12:11 PM, Bruce Richardson < > > > > +#ifdef RTE_MBUF_REFCNT > > > > + mb_def.refcnt = 1; > > > > +#endif > > > > > > I would expect we use rte_mbuf_refcnt_set / rte_mbuf_refcnt_read to access > > > this "refcnt" field. > > > This api handles both RTE_MBUF_REFCNT_ATOMIC and ! RTE_MBUF_REFCNT_ATOMIC > > > configs. > > > But I suppose this is fine at init time (since the union will initialize > > > properly the field). > > > > It's a good point, I'll update patch to use the appropriate macro which will clean up the code a bit. > > > > By the way, why do we have this RTE_MBUF_REFCNT_ATOMIC option ? > > > From my point of view, there is not much use of a refcnt that is not atomic > > > :-). > > Bruce, I think it's a good question but you didn't answer. > Maybe we should remove this option to keep only atomic mode. > I didn't answer just because it wasn't directly relevant to the patch. It was not meant as a snub. :-) As for why the option is there, it's purely for performance, I suspect. The cost of doing increments and decrements using atomic operations is far higher than doing a read-modify-write on a single core. However, the downside is obviously that you need to know what you are doing if you disable atomic refcnts and, given that atomic is the default, I reckon we can probably get rid of the option permanently - unless someone has a use case where they turn off the option, and can't take the performance hit of the atomic instructions. As a further asside, if we get the proposed changes made to the zero-copy vhost implementation - discussed previously[1] - we should hopefully be able to get rid of the refcnt option too, and leave it permanently enabled. /Bruce [1] Discussed in this thread: http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/7098