[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20070907142538.GC8864@linux.vnet.ibm.com>
Date: Fri, 7 Sep 2007 07:25:38 -0700
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: Johannes Berg <johannes@...solutions.net>
Cc: Herbert Xu <herbert@...dor.apana.org.au>, satyam@...radead.org,
flo@...822.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
michal.k.k.piotrowski@...il.com,
ipw3945-devel@...ts.sourceforge.net, yi.zhu@...el.com,
flamingice@...rmilk.net
Subject: 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:
>
> > 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!!!
>
> :)
>
> > 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!
> > > - 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.
> > > @@ -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?)
> > > ASSERT_RTNL();
> >
> > I -like- this!!! ;-)
>
> :)
Thanx, Paul
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists