Re: [dpdk-dev] [PATCH] ixgbe: fix icc issue with mbuf initializer

[Bruce Richardson <bruce.richardson@intel.com>]

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