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

[Ralph Campbell]

On Wed, 2007-06-27 at 12:13 -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
> 
>  > +       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.
> 
> Just something to think about...

Most of the places where the receive header tail is checked is
for queue full/non-full so the read barriers aren't needed.
The one place where we might need a rmb() is in ipath_kreceive()
where we check the tail and then read the queue entry.