Re: [patch 2/4] net: use mutex_is_locked() for ASSERT_RTNL()

[Jarek Poplawski <jarkao2@gmail.com>]

On Mon, Dec 17, 2007 at 09:26:32AM +0800, Herbert Xu wrote:
> On Sun, Dec 16, 2007 at 07:06:41PM +0100, Jarek Poplawski wrote:
> >
> > It seemed to exist a few days ago:
> > http://kerneltrap.org/mailarchive/linux-netdev/2007/12/4/473123
> > 
> > Btw., I don't know which of the patches: Eric's or yours will be chosen,
> > but, IMHO, there is no reason to remove rtnl_trylock(), which can be still
> > useful, just like mutex_trylock() is.
> 
> Doh! Andrew was too convincing :) I misread the grep result on
> in_interrupt.  Of course that function returns true if we're
> either in an IRQ handler or have BH off.
> 
> I retract what I've said in this thread and continue to oppose
> this change without a might_sleep.
> 

...And I think some change is needed here. Btw., I proposed to change
this long time ago too. There were no response - only Ben Greear
mentioned about some applications, which could rely on the trylock
way. I didn't understand what he was talking about at all - and it
didn't change until I've read this and Eric's patch thread!

So, I was surprised, probably just like Eric, ASSERT_RTNL is 2 in 1,
with this atomic somewhere deep in mind. IMHO this should be better
commented at least. But it's still dubious to me: using trylock this
way makes impossible to verify (eg. by lockdep) recursion cases,
when lock is taken with trylock in a loop.

So, I think using might_sleep() explicitly would be much more
readable or, otherwise, Patrick's proposal with adding
ASSERT_RTNL_ATOMIC would implicitly signal the real meaning of the
other one.

Btw. #2: David Miller gave this example of ASSERT_RTNL use:

	ASSERT_RTNL();
	page = alloc_page(GFP_KERNEL);

But isn't there a debugging duplication: it seems alloc_page() is used
in so many places and this check for GFP is/should_be there already?

Regards,
Jarek P.