Re: [PATCH net-next v2 2/2] Re-check for a VIO_DESC_READY data descriptor after short udelay()

[Sowmini Varadhan <sowmini.varadhan@oracle.com>]

On (09/04/14 22:36), David Miller wrote:
> 
> The memory barrier exists in order to make sure the cookies et al. are
> globally visible before the VIO_DESC_READY.  We don't want stores to
> be reordered such that the VIO_DESC_READY is seen too early.

Ok, though David (dls) was just pointing out that a rmb() might
be missing in vnet_walk_rx_one() before checking for READY descriptor

> I'm having a hard time imagining that putting the wmb() afterwards
> would help, except by adding a new delay in a different place.

correct.

> I say that because the TX trigger is going to make a hypervisor call,
> and I bet that hypervisor trap syncs the store buffer of invoking cpu
> thread.
> 
> Thinking further, another issue to consider is that the wmb() might no
> be necessary considering all of the above, because the remote entity
> should not look at this descriptor until the tx trigger occurs.

So, today the consumer already reads a series of descriptors based on
a single LDC start trigger already in vnet_walk_rx() - it doesnt
actually wait for an LDC "start" per descriptor.

Which is as it should be- the first "start" trigger of a burst 
is just an async signal from producer to consumer that data is ready 
for consumption.

A wmb() (and maybe a missing rmb()) should itself synchronize the critical
state more efficiently than an ldc exchange. The last bit
about VIO_DESC_READY may get flushed at some later point, and
the delay in patch 2/2 slightly helps work around that fudge factor,
but moving the wmb() before/after the VIO_DESC_READY (in my testing)
doesn't really make a difference to correctness - the consumer first
checks for VIO_DESC_READY, and by that time, the rest of the cookie
info should have been correctly sync'ed by the memory barrier.

If we get the memory-barriers right,
the trigger-per-vnet_start_xmit is superfluous, and only 
serves to flood the ldc_channel (and make the env ripe for
!tx_has_space_for()).

(And a side-effect of this is that avoids severl other udelay() loops
that we'd have otherwise undergone as part of __vnet_tx_trigger(),
which is also a healthy thing to avoid)

> Anyways, if the hypervisor trap to signal the tx trigger does not
> flush the local cpu thread's store buffer, then yes moving the memory
> barrier might make sense.

As such, it's expensive and unnecessary to cripple ourselves down to
one LDC exchange per descriptor - the wmb() itself should ensure this
correctly in a cheaper way.

BTW, even if you set the READY state before (not after) the wmb() 
there's still a small iperf-boost to be obtained by lingering
around in vnet_walk_rx() (as in patch 2/2)- that boost is from avoiding
what would otherwise be a ldc stop/start exchange between consumer
and producer. 

But I'm not too attached to this boost (aka patch 2/2)- 
benchmark-special code is always ugly.

--Sowmini