Re: [openib-general] Re: [git patch review 1/4] IPoIB: Don't start send-only joins while multicast thread is stopped

[Michael S. Tsirkin]

Quoting r. Roland Dreier <rdreier@xxxxxxxxx>:
> Subject: [openib-general] Re: [git patch review 1/4] IPoIB: Don't start send-only joins while multicast thread is stopped
> 
>  > Roland Dreier <rolandd@xxxxxxxxx> wrote:
>  > >
>  > >  +	spin_lock_irq(&priv->lock);
>  > >  +	set_bit(IPOIB_MCAST_STARTED, &priv->flags);
>  > >  +	spin_unlock_irq(&priv->lock);
>  > 
>  > Strange to put a lock around an atomic op like that.
>  > 
>  > Sometimes it's valid.   If another cpu was doing:
>  > 
>  > 	spin_lock(lock);
>  > 
>  > 	if (test_bit(IPOIB_MCAST_STARTED))
>  > 		something();
>  > 	...
>  > 	if (test_bit(IPOIB_MCAST_STARTED))
>  > 		something_else();
>  > 
>  > 	spin_unlock(lock);
>  > 
>  > then the locked set_bit() makes sense.
>  > 
>  > But often it doesn't ;)
> 
> Good point.  Michael, any reason why the lock is there around the
> set_bit()?  (And similarly for the corresponding clear_bit())
> 
> Thanks,
>  Roland

Basically, its as Andrew said: the lock around clear_bit is there to ensure that
ipoib_mcast_send isnt running already when we stop the thread.  Thats why
test_bit has to be inside the lock, too.

This was discussed with Krishna Kumar when I posted the patch originally.
For more detail, please review this thread:
http://www.mail-archive.com/openib-general@xxxxxxxxxx/msg13206.html
or here
http://openib.org/pipermail/openib-general/2005-December/014370.html


-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/