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/08/14 09:45), David L Stevens wrote:

First off, please don't drop email ids from the original email. 
I've re-added Raghuram Kothakota to fix that initial omission.

Second, this thread originally came up with your suggestion that
we shold move the wmb() down one line. I looked at this, and consulted
a few folks, all of whom agree that that is probably incorrect. 

As Bob Picco pointed out to me:
" Placing the wmb() after probably won't have any negative consequences
  BUT could. Suppose the compiler reorders the stores and the d->hdr.state
  store occurs before another part of the descriptor. We HV trap,
  and the trap doesn't flush the store buffer. The consumer could then
  see VIO_DESC_READY before the cookies arrray is updated. It would depend
  on how many cache lines are spanned by vio_net_desc and the store buffer
  organization."
thus I would be cautious about doing that.

> I'm no mb expert, and I know of no symptoms, but it appears to be saying that
> load reordering could result in a race where the READY flag could be set with
> old data in other descriptor fields due to loading them in a different order --
> something it says wmb() on another CPU explicitly does not prevent.

Please see the explanation that I, and Raghuram offered.
The wmb() assures that the stores of the cookie state are
ordered correctly by the consumer. The next store is for VIO_DESC_READY
and the consumer will not proceed to read cookie state until this
is visible. Thus no rmb() is needed. 

On its side, the consumer resets the hdr.state to DONE after it
consumes the cookies, and the DRING_STOPPLED announcement further
informs the producer that the descriptor is available. 

Hope that helps. If you think there is some sequence where this
is insufficient, please explain with details.

>
> The particular case would be adding to the ring at the same time the
> other side
> is removing from the ring, so no locks or LDC traffic would affect it.
> 
> So, it appears to me we have a missing rmb() that is needed and I
> don't know what
> leads you to believe it isn't.
> 
> 							+-DLS