Re: BUG: scheduling while atomic: ifconfig/0x00000002/4170

On Fri, Sep 07, 2007 at 03:27:15PM +0200, Johannes Berg wrote:

On Thu, 2007-09-06 at 08:46 -0700, Paul E. McKenney wrote:

quoted

Looks good to me from an RCU viewpoint.  I cannot claim familiarity with
this code.  I therefore especially like the indications of where RTNL
is held and not!!!

:)

quoted

Some questions below based on a quick scan.  And a global question:
should the comments about RTNL being held be replaced by ASSERT_RTNL()?

I don't like ASSERT_RTNL() much because it actually tries to lock it.
I'd be much happer if it was WARN_ON(!mutex_locked(&rtnl_mutex)) or
something equivalent.

Ah!  It would indeed be nice to have a lower-overhead ASSERT_RTNL_LIGHT()
or whatever.

In any case, I have an updated patch I'll be sending soon, and it
requires a new list walking primitive I'll also send.

Look forward to seeing it!

quoted

quoted

-	write_lock_bh(&local->sub_if_lock);
+	/* we're under RTNL so all this is fine */
 	if (unlikely(local->reg_state == IEEE80211_DEV_UNREGISTERED)) {
-		write_unlock_bh(&local->sub_if_lock);
 		__ieee80211_if_del(local, sdata);
 		return -ENODEV;
 	}
-	list_add(&sdata->list, &local->sub_if_list);
+	list_add_tail_rcu(&sdata->list, &local->interfaces);

The _rcu is required because this list isn't protected by RTNL?

Yes, not all walkers of the list are protected by the RTNL.

K.

quoted

quoted

@@ -226,22 +225,22 @@ void ieee80211_if_reinit(struct net_devi
 		/* Remove all virtual interfaces that use this BSS
 		 * as their sdata->bss */
 		struct ieee80211_sub_if_data *tsdata, *n;
-		LIST_HEAD(tmp_list);

-		write_lock_bh(&local->sub_if_lock);

This code is also protected by RTNL?

Yes.

Comment?  (Or is it in the function header?)

quoted

quoted

 	ASSERT_RTNL();

I -like- this!!!  ;-)

:)

							Thanx, Paul