Re: [PATCH v2 02/15] IB/srp: Fix race between srp_queuecommand() and srp_claim_req()

[Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>]

On 06/28/13 16:42, Sebastian Riemer wrote:
> On 28.06.2013 14:48, Bart Van Assche wrote:
>> Avoid that srp_claim_command() can claim a command while
>> srp_queuecommand() is still busy queueing the same command.
>> Found this via source reading.
>
> Nice, that's much less re-acquiring of the target lock in error case in
> srp_queuecommand().
> But if we have to change that many locations for srp_put_tx_iu() anyway,
> wouldn't it make sense to rename it into __srp_put_tx_iu() as well?
>
> Then we can also put a little description to it and it looks familiar
> compared to __srp_get_tx_iu().
>
> The description could look like follows:
> /*
>   * Return an IU and possible credit to the free pool
>   *
>   * Must be called with target->lock held to protect free_tx.
>   */
>
> I'm not sure if we still need that lockdep_assert_held() then. There is
> no other location with lock debugging in ib_srp.

Hello Sebastian,

I don't have a strong opinion about either of these two topics.

If a function like __srp_get_tx_iu() would be introduced that would 
allow to drop only two spin_lock/spin_unlock call pairs. So introducing 
that function would probably add more lines of code than adding the 
spin_lock/spin_unlock pairs. Hence my choice not to introduce 
__srp_get_tx_iu().

Regarding the lockdep_assert_held() statement: the reason I introduced 
it instead of adding a comment above the function telling which locking 
is required is because a lockdep_assert_held() statement is verified at 
runtime on a system with a kernel in which lockdep support has been enabled.

Bart.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html