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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
