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
