[ofa-general] Re: [PATCH 24/28] IB/ipath - ipath_poll fixups and enhancements

[Arthur Jones]

hi roland, ...

On Wed, Jun 27, 2007 at 12:13:54PM -0700, Roland Dreier wrote:
>  > > OK, fair enough, although it seems you may be missing some memory
>  > > barriers to make sure you don't run into the CPU reordering accesses
>  > > to the head/tail pointers.
>  > 
>  > i had a quick look at the patch and the surrounding
>  > code and i did not catch the problem.  can you be a
>  > little more specific about the suspect code?
> 
> I'm not sure there's a bug there.  But the patch in question does
> 
>  > +       tail = *(volatile u64 *)pd->port_rcvhdrtail_kvaddr;
> 
> with no memory ordering.  The volatile makes sure the compiler puts
> that read where you wrote it, but there's no guarantee that the CPU
> executes it anywhere remotely close to where it is in the code.  Later
> on you have

agreed.

>  > +       if (tail != head ||
>  > +           test_bit(IPATH_PORT_WAITING_RCV, &pd->int_flag)) {
> 
> etc., and the CPU might speculate those test far ahead of actually
> reading the port_rcvhdrttail_kvaddr value, which means you might end
> up executing code based on a guess about tail != head that is not true
> at the time it speculates the branch, but by the time it does get to
> actually check its speculation, the guess has become true.

i agree that the &pd->int_flag result could be
valid before tail has become valid and hence
when waiting for tail to be valid we're out of
order wrt the int_flag load.  but this logic is
completely async to the head != tail test, so
the out-of-order result there can not hurt us...

arthur