[ofa-general] Re: Incorrect atomic usage in ipath driver

[Tom Mitchell]

Thank you for the feedback.
Part this code path is necessary for an early revision
of the hardware.  It may be important on ppc.  As it is now,
it is a don't care on x86_64.

The responsible engineers here have seen this and will investigate further.


On May 08 01:21, Benjamin Herrenschmidt wrote:
> Hi !
> 
> So I see this construct:
> 
> 	/* There is already a thread processing this queue. */
> 	if (test_and_set_bit(0, &dd->ipath_rcv_pending))
> 		goto bail;
> 
> 	.../...
> 
> done:
> 	clear_bit(0, &dd->ipath_rcv_pending);
> 	smp_mb__after_clear_bit();
> 
> So that's basically an attempt at doing a spinlock. The problem is your
> barrier is wrong at the end. Better would be:
> 
> 
> done:
> 	smp_mb__before_clear_bit();
> 	clear_bit(0, &dd->ipath_rcv_pending);
> 
> 
> Though it's still less optimal that doing:
> 
> 	if (!spin_trylock(...))
> 		goto bail;
> 
> 	.../...
> 
> done:
> 	spin_unlock(...)
> 
> If you really want to stick to bitops, then you may want to look at
> Nick's upcoming patches adding some bitops with appropriate lock
> semantics.
> 
> Cheers,
> Ben.
> 
> 

-- 
	T o m   M i t c h e l l
	Host Solutions Group
	QLogic Corp.  http://www.qlogic.com