[openib-general] Re: [PATCH] sdp_conn_put/sdp_conn_hold race

[Michael S. Tsirkin]

Quoting r. Libor Michalek <limichal at cisco.com>:
> Subject: Re: [PATCH] sdp_conn_put/sdp_conn_hold race
> 
> On Wed, Jul 20, 2005 at 09:44:18PM +0300, Michael S. Tsirkin wrote:
> > Quoting r. Michael S. Tsirkin <mst at mellanox.co.il>:
> > > The current sdp_conn_put/sdp_conn_hold implementation
> > > seems to be subject to the following race condition:
> > 
> > Libor, did you have the time to review this patch?
> 
>   Yes, looks good. The only question I had was about making
> sdp_conn_put an inline, and calling to destruct only on the
> last decrement.

conn_put must first do
spin_lock_irqsave(&dev_root_s.sock_lock, flags);

so that would require making dev_root_s extern instead of static.

> Basically split the sdp_conn_put code in
> your patch, where sdp_conn_put is everything before the
> unlock of sock_lock, and destruct is everything after.

How do you mean? It seems unlock of sock_lock is the last operation
in sdp_conn_put. Must be careful not to reintroduce the race if its
changed.

>  Was
> there any reason you didn't do that? Is there a reasont to
> split it like this...

conn_put is done in places where we expect to have
the last reference. If we know its not, we can do _light.
So making it inline will increase code size
and wont save a function call.

>   Looks like all the right places have _light().
> 
> -Libor
> 

Right. How about we merge it, and then its easier to discuss
patches on top of it? BTW, my limited benchmarking showed
no effect on performance.

-- 
MST