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

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

[akpm]

From: Andrew Morton <akpm@linux-foundation.org>

ASSERT_RTNL() uses mutex_trylock(), but it's better to use mutex_is_locked().

Make that change, and remove rtnl_trylock() altogether.

(not tested yet!)

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/net/cxgb3/cxgb3_main.c |    3 +--
 include/linux/rtnetlink.h      |    5 ++---
 net/core/rtnetlink.c           |    7 ++++---
 3 files changed, 7 insertions(+), 8 deletions(-)

diff -puN drivers/net/cxgb3/cxgb3_main.c~net-use-mutex_is_locked-for-assert_rtnl drivers/net/cxgb3/cxgb3_main.c
--- a/drivers/net/cxgb3/cxgb3_main.c~net-use-mutex_is_locked-for-assert_rtnl
+++ a/drivers/net/cxgb3/cxgb3_main.c
@@ -2191,7 +2191,7 @@ static void check_t3b2_mac(struct adapte
 {
 	int i;
 
-	if (!rtnl_trylock())	/* synchronize with ifdown */
+	if (rtnl_is_locked())	/* synchronize with ifdown */
 		return;
 
 	for_each_port(adapter, i) {
@@ -2219,7 +2219,6 @@ static void check_t3b2_mac(struct adapte
 			p->mac.stats.num_resets++;
 		}
 	}
-	rtnl_unlock();
 }
 
 
diff -puN include/linux/rtnetlink.h~net-use-mutex_is_locked-for-assert_rtnl include/linux/rtnetlink.h
--- a/include/linux/rtnetlink.h~net-use-mutex_is_locked-for-assert_rtnl
+++ a/include/linux/rtnetlink.h
@@ -751,14 +751,13 @@ extern void rtmsg_ifinfo(int type, struc
 /* RTNL is used as a global lock for all changes to network configuration  */
 extern void rtnl_lock(void);
 extern void rtnl_unlock(void);
-extern int rtnl_trylock(void);
+extern int rtnl_is_locked(void);
 
 extern void rtnetlink_init(void);
 extern void __rtnl_unlock(void);
 
 #define ASSERT_RTNL() do { \
-	if (unlikely(rtnl_trylock())) { \
-		rtnl_unlock(); \
+	if (unlikely(!rtnl_is_locked())) { \
 		printk(KERN_ERR "RTNL: assertion failed at %s (%d)\n", \
 		       __FILE__,  __LINE__); \
 		dump_stack(); \
diff -puN net/core/rtnetlink.c~net-use-mutex_is_locked-for-assert_rtnl net/core/rtnetlink.c
--- a/net/core/rtnetlink.c~net-use-mutex_is_locked-for-assert_rtnl
+++ a/net/core/rtnetlink.c
@@ -77,9 +77,10 @@ void rtnl_unlock(void)
 	netdev_run_todo();
 }
 
-int rtnl_trylock(void)
+/* Return non-zero if rtnl_mutex is presently held */
+int rtnl_is_locked(void)
 {
-	return mutex_trylock(&rtnl_mutex);
+	return mutex_is_locked(&rtnl_mutex);
 }
 
 int rtattr_parse(struct rtattr *tb[], int maxattr, struct rtattr *rta, int len)
@@ -1424,7 +1425,7 @@ EXPORT_SYMBOL(rtattr_parse);
 EXPORT_SYMBOL(__rtattr_parse_nested_compat);
 EXPORT_SYMBOL(rtnetlink_put_metrics);
 EXPORT_SYMBOL(rtnl_lock);
-EXPORT_SYMBOL(rtnl_trylock);
+EXPORT_SYMBOL(rtnl_is_locked);
 EXPORT_SYMBOL(rtnl_unlock);
 EXPORT_SYMBOL(rtnl_unicast);
 EXPORT_SYMBOL(rtnl_notify);
_

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

[Herbert Xu]

akpm@linux-foundation.org wrote:
>
> diff -puN drivers/net/cxgb3/cxgb3_main.c~net-use-mutex_is_locked-for-assert_rtnl drivers/net/cxgb3/cxgb3_main.c
> --- a/drivers/net/cxgb3/cxgb3_main.c~net-use-mutex_is_locked-for-assert_rtnl
> +++ a/drivers/net/cxgb3/cxgb3_main.c
> @@ -2191,7 +2191,7 @@ static void check_t3b2_mac(struct adapte
> {
>        int i;
> 
> -       if (!rtnl_trylock())    /* synchronize with ifdown */
> +       if (rtnl_is_locked())   /* synchronize with ifdown */
>                return;
> 
>        for_each_port(adapter, i) {
> @@ -2219,7 +2219,6 @@ static void check_t3b2_mac(struct adapte
>                        p->mac.stats.num_resets++;
>                }
>        }
> -       rtnl_unlock();

This doesn't look right.  It seems that they really want trylock
here so we should just fix it by removing the bang.

Also, does ASSERT_RTNL still warn when someone calls it from an
atomic context? We definitely don't want to lose that check.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Andrew Morton]

On Fri, 14 Dec 2007 16:10:44 +0800 Herbert Xu <herbert@gondor.apana.org.au> wrote:

> akpm@linux-foundation.org wrote:
> >
> > diff -puN drivers/net/cxgb3/cxgb3_main.c~net-use-mutex_is_locked-for-assert_rtnl drivers/net/cxgb3/cxgb3_main.c
> > --- a/drivers/net/cxgb3/cxgb3_main.c~net-use-mutex_is_locked-for-assert_rtnl
> > +++ a/drivers/net/cxgb3/cxgb3_main.c
> > @@ -2191,7 +2191,7 @@ static void check_t3b2_mac(struct adapte
> > {
> >        int i;
> > 
> > -       if (!rtnl_trylock())    /* synchronize with ifdown */
> > +       if (rtnl_is_locked())   /* synchronize with ifdown */
> >                return;
> > 
> >        for_each_port(adapter, i) {
> > @@ -2219,7 +2219,6 @@ static void check_t3b2_mac(struct adapte
> >                        p->mac.stats.num_resets++;
> >                }
> >        }
> > -       rtnl_unlock();
> 
> This doesn't look right.  It seems that they really want trylock
> here so we should just fix it by removing the bang.

doh.

> Also, does ASSERT_RTNL still warn when someone calls it from an
> atomic context? We definitely don't want to lose that check.

I don't see how it could warn about that.  Nor should it - one might want
to check that rtnl_lock is held inside preempt_disable() or spin_lock or
whatever.

It might make sense to warn if ASSERT_RTNL is called in in_interrupt()
contexts though.

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

[Herbert Xu]

On Fri, Dec 14, 2007 at 12:22:09AM -0800, Andrew Morton wrote:
>
> I don't see how it could warn about that.  Nor should it - one might want
> to check that rtnl_lock is held inside preempt_disable() or spin_lock or
> whatever.
> 
> It might make sense to warn if ASSERT_RTNL is called in in_interrupt()
> contexts though.

Well the paths where ASSERT_RTNL is used should never be in an
atomic context.  In the past it has been quite useful in pointing
out bogus locking practices.

There is currently one path where it's known to warn because of
this and it (promiscuous mode) is on my todo list.

Oh and it only warns when you have mutex debugging enabled.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 14 Dec 2007 16:30:37 +0800

> On Fri, Dec 14, 2007 at 12:22:09AM -0800, Andrew Morton wrote:
> >
> > I don't see how it could warn about that.  Nor should it - one might want
> > to check that rtnl_lock is held inside preempt_disable() or spin_lock or
> > whatever.
> > 
> > It might make sense to warn if ASSERT_RTNL is called in in_interrupt()
> > contexts though.
> 
> Well the paths where ASSERT_RTNL is used should never be in an
> atomic context.  In the past it has been quite useful in pointing
> out bogus locking practices.
> 
> There is currently one path where it's known to warn because of
> this and it (promiscuous mode) is on my todo list.
> 
> Oh and it only warns when you have mutex debugging enabled.

Right, this change is just totally bogus.

I'm all for using existing facilities to replace hand-crafted copies,
but this case is removing useful debugging functionality so it's
wrong.

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

[Andrew Morton]

On Fri, 14 Dec 2007 11:15:14 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Fri, 14 Dec 2007 16:30:37 +0800
> 
> > On Fri, Dec 14, 2007 at 12:22:09AM -0800, Andrew Morton wrote:
> > >
> > > I don't see how it could warn about that.  Nor should it - one might want
> > > to check that rtnl_lock is held inside preempt_disable() or spin_lock or
> > > whatever.
> > > 
> > > It might make sense to warn if ASSERT_RTNL is called in in_interrupt()
> > > contexts though.
> > 
> > Well the paths where ASSERT_RTNL is used should never be in an
> > atomic context.  In the past it has been quite useful in pointing
> > out bogus locking practices.
> > 
> > There is currently one path where it's known to warn because of
> > this and it (promiscuous mode) is on my todo list.
> > 
> > Oh and it only warns when you have mutex debugging enabled.
> 
> Right, this change is just totally bogus.
> 
> I'm all for using existing facilities to replace hand-crafted copies,
> but this case is removing useful debugging functionality so it's
> wrong.

I don't believe that ASSERT_RTNL() presently warns when called from atomic
contexts.  If it does then I missed it.

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

[Herbert Xu]

On Fri, Dec 14, 2007 at 03:11:36PM -0800, Andrew Morton wrote:
>
> I don't believe that ASSERT_RTNL() presently warns when called from atomic
> contexts.  If it does then I missed it.

It does when mutex debugging is enabled.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Andrew Morton]

On Sat, 15 Dec 2007 12:18:27 +0800 Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Fri, Dec 14, 2007 at 03:11:36PM -0800, Andrew Morton wrote:
> >
> > I don't believe that ASSERT_RTNL() presently warns when called from atomic
> > contexts.  If it does then I missed it.
> 
> It does when mutex debugging is enabled.
> 

That sounds like a bug in mutex_trylock() to me.

Where in the tangled forest of the mutex implementation is the code
which does this?

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

[Herbert Xu]

On Fri, Dec 14, 2007 at 09:44:18PM -0800, Andrew Morton wrote:
>
> That sounds like a bug in mutex_trylock() to me.

I was relying on

	http://kerneltrap.org/mailarchive/linux-netdev/2007/9/28/325129

which seems to be a bogus claim now that I actually look at the
source code.  So in that case I'm OK with your patch as long as
it warns about hard IRQ usage.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Andrew Morton]

On Sat, 15 Dec 2007 14:10:21 +0800 Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Fri, Dec 14, 2007 at 09:44:18PM -0800, Andrew Morton wrote:
> >
> > That sounds like a bug in mutex_trylock() to me.
> 
> I was relying on
> 
> 	http://kerneltrap.org/mailarchive/linux-netdev/2007/9/28/325129
> 
> which seems to be a bogus claim now that I actually look at the
> source code.  So in that case I'm OK with your patch as long as
> it warns about hard IRQ usage.

When Eric said

> Way way deep in mutex debugging on the slowpath there is a unreadable
> and incomprehensible WARN_ON in muxtex_trylock that will trigger if
> you have 10 tons of debugging turned on, and you are in,
> interrupt context, and you manage to hit the slow path.  I think that
> is a pretty unlikely scenario.

I think he's still right.  That's if the warning which he managed to find
even still exists.

I think the change which Eric proposed is a good one: it converts
ASSERT_RTNL() from an atomic rmw which dirties a cacheline which will
sometimes be owned by a different CPU into a plain old read.  It's going to
make ASSERT_RTNL() heaps cheaper.

<looks at mutex_is_locked(), rofls at "static inline fastcall", fixes it>

Now as a separate issue we (ie: you) need to work out what _other_ things
you want ASSERT_RTNL to check apart from "rtnl must be held".

If you want to check that no locks are held (which I think is a bit weird,
but whatever) then add might_sleep().

If you want to check that we're not in interrupt context or whatever, then
add the checks and be happy.  might_sleep() will of course check for
in_interrupt(), in_irq(), etc so if you go with a might_sleep() then
nothing else needs to be added.

While doing this I'd also suggest that the thing should be uninlined -
it'll probably generate less text and it'll give considerably more
flexibility for adding new debug fetures.  Ones which might be controlled at
compile time or runtime. ie:

void __assert_rtnl(const char *file, int line);
#define ASSERT_RTNL() __assert_rtnl(__FILE__, __LINE__)

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

[Herbert Xu]

On Sat, Dec 15, 2007 at 02:48:10AM -0800, Andrew Morton wrote:
>
> When Eric said
> 
> > Way way deep in mutex debugging on the slowpath there is a unreadable
> > and incomprehensible WARN_ON in muxtex_trylock that will trigger if
> > you have 10 tons of debugging turned on, and you are in,
> > interrupt context, and you manage to hit the slow path.  I think that
> > is a pretty unlikely scenario.
> 
> I think he's still right.  That's if the warning which he managed to find
> even still exists.

Well at the very start he said:

: When mutex debugging is on taking a mutex complains if we are not
: allowed to sleep.  At that point we have called netif_tx_lock_bh
: so we are clearly not allowed to sleep.  Arguably this is not a
: problem for mutex_trylock.

This is what threw me off as it implied that we were warning about
ASSERT_RTNL calls in any atomic context, including those with just
soft IRQs disabled, which is an important distinction for the
networking subsystem.

Had he said that it was just IRQ handlers rather than soft IRQs
then we wouldn't be having this discussion now :)

> I think the change which Eric proposed is a good one: it converts
> ASSERT_RTNL() from an atomic rmw which dirties a cacheline which will
> sometimes be owned by a different CPU into a plain old read.  It's going to
> make ASSERT_RTNL() heaps cheaper.

I agree.  Although paths using ASSERT_RTNL shouldn't be performance
critical since the RTNL should only be held for control operations
as opposed to data transport.

> Now as a separate issue we (ie: you) need to work out what _other_ things
> you want ASSERT_RTNL to check apart from "rtnl must be held".

Since we have now established that ASSERT_RTNL never actually
warned about usage on paths with BH off, I think Eric's original
patch is fine as it is and I owe him an apology.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 15 Dec 2007 21:10:16 +0800

> On Sat, Dec 15, 2007 at 02:48:10AM -0800, Andrew Morton wrote:
> >
> > Now as a separate issue we (ie: you) need to work out what _other_ things
> > you want ASSERT_RTNL to check apart from "rtnl must be held".
> 
> Since we have now established that ASSERT_RTNL never actually
> warned about usage on paths with BH off, I think Eric's original
> patch is fine as it is and I owe him an apology.

Ok, same here...

Such situations (ASSERT_RTNL() in atomic context) have always
been bugs though, and that continues to be true and I think
the check should be added somehow.

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

[Herbert Xu]

On Sat, Dec 15, 2007 at 09:44:29PM -0800, David Miller wrote:
>
> Such situations (ASSERT_RTNL() in atomic context) have always
> been bugs though, and that continues to be true and I think
> the check should be added somehow.

OK once I've fixed the set_multicast path I'll do an audit of
the existing ASSERT_RTNL users and then add the might_sleep
check.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Jarek Poplawski]

Andrew Morton wrote, On 12/15/2007 11:48 AM:

> On Sat, 15 Dec 2007 14:10:21 +0800 Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
>> On Fri, Dec 14, 2007 at 09:44:18PM -0800, Andrew Morton wrote:
>>> That sounds like a bug in mutex_trylock() to me.
>> I was relying on
>>
>> 	http://kerneltrap.org/mailarchive/linux-netdev/2007/9/28/325129
>>
>> which seems to be a bogus claim now that I actually look at the
>> source code.  So in that case I'm OK with your patch as long as
>> it warns about hard IRQ usage.
> 
> When Eric said
> 
>> Way way deep in mutex debugging on the slowpath there is a unreadable
>> and incomprehensible WARN_ON in muxtex_trylock that will trigger if
>> you have 10 tons of debugging turned on, and you are in,
>> interrupt context, and you manage to hit the slow path.  I think that
>> is a pretty unlikely scenario.
> 
> I think he's still right.  That's if the warning which he managed to find
> even still exists.


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.

Regards,
Jarek P.

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

[Herbert Xu]

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.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Jarek Poplawski]

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.

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

[Herbert Xu]

On Mon, Dec 17, 2007 at 08:26:01AM +0100, Jarek Poplawski wrote:
>
> 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?

On some paths this may be buried a conditional.  Also if you
replace it with a mutex_is_locked without the may_sleep it won't
catch the case that started all this, namely the promiscuous path.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Jarek Poplawski]

On Mon, Dec 17, 2007 at 03:31:33PM +0800, Herbert Xu wrote:
> On Mon, Dec 17, 2007 at 08:26:01AM +0100, Jarek Poplawski wrote:
> >
> > 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?
> 
> On some paths this may be buried a conditional.  Also if you
> replace it with a mutex_is_locked without the may_sleep it won't
> catch the case that started all this, namely the promiscuous path.
> 

Right! There is only a question how much is such cases vs. checked
already, because then a might_sleep is really more readable when
added explicitly.

Jarek P.

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

[Jarek Poplawski]

On Mon, Dec 17, 2007 at 08:26:01AM +0100, Jarek Poplawski wrote:
> On Mon, Dec 17, 2007 at 09:26:32AM +0800, Herbert Xu wrote:
...
> > I retract what I've said in this thread and continue to oppose
> > this change without a might_sleep.
...
> 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.

OOPS! I've looped again! Of course, ASSERT_RTNL with might_sleep()
would be explicit enough by itself (if we don't believe atomicity
is debugged enough). So, this atomic version could be usable for
other reasons.
 
Jarek P.

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

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Fri, 14 Dec 2007 21:44:18 -0800

> That sounds like a bug in mutex_trylock() to me.

I disagree, I have yet to see a legitimate case where doing a trylock
on a mutex lock didn't turn out to be a bug when performed in an
atomic context.

This is particularly true in the networking, the bonding driver
has provided some excellent examples over the years :-)

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

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]


> I don't see how it could warn about that.  Nor should it - one might want
> to check that rtnl_lock is held inside preempt_disable() or spin_lock or
> whatever.

I agree with this. IIRC I removed some ASSERT_RTNL()s in the wireless
code (or maybe it was only during testing patches) where we had a
function that required only the rtnl to be held but in certain contexts
was called from within an RCU section.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

[Herbert Xu]

On Fri, Dec 14, 2007 at 01:37:40PM +0100, Johannes Berg wrote:
> 
> I agree with this. IIRC I removed some ASSERT_RTNL()s in the wireless
> code (or maybe it was only during testing patches) where we had a
> function that required only the rtnl to be held but in certain contexts
> was called from within an RCU section.

Please point me to the actual code so I can see if this is legit
or not.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]


> > I agree with this. IIRC I removed some ASSERT_RTNL()s in the wireless
> > code (or maybe it was only during testing patches) where we had a
> > function that required only the rtnl to be held but in certain contexts
> > was called from within an RCU section.
> 
> Please point me to the actual code so I can see if this is legit
> or not.

I don't think I have that case any more since now my interface list is
either protected by RCU or the rtnl.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

[David Miller]

From: akpm@linux-foundation.org
Date: Thu, 13 Dec 2007 16:02:36 -0800

> From: Andrew Morton <akpm@linux-foundation.org>
> 
> ASSERT_RTNL() uses mutex_trylock(), but it's better to use mutex_is_locked().
> 
> Make that change, and remove rtnl_trylock() altogether.
> 
> (not tested yet!)
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

NACK, as explained please remove this until the replacement
doesn't remove valid checks which are done currently.