On 28.06.2013 16:51, Bart Van Assche wrote: >> 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.
Hi Bart, I just meant a rename into "__srp_put_tx_iu()" to show that locking is required and not introducing a further wrapper function. The other function of that kind "__srp_get_tx_iu()" also doesn't have a wrapper function "srp_get_tx_iu()". For me it doesn't make much difference how it is marked that locking is required. I just wanted to point out that this method is new to ib_srp. Cheers, Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
