On Tue, 16 Jun 2020 at 06:33, Cong Wang <xiyou.wangc...@gmail.com> wrote:
>
Hi Cong,
> On Sat, Jun 13, 2020 at 9:03 AM Taehee Yoo <ap420...@gmail.com> wrote:
> >
> > On Thu, 11 Jun 2020 at 08:21, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> > >
> >
> > Hi Cong :)
> >
> > > On Wed, Jun 10, 2020 at 7:48 AM Taehee Yoo <ap420...@gmail.com> wrote:
> > > >
> > > > On Tue, 9 Jun 2020 at 06:53, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> > > > >
> > > > > + lockdep_set_class_and_subclass(&dev->addr_list_lock,
> > > > > + &vlan_netdev_addr_lock_key,
> > > > > + subclass);
> >
> > In this patch, lockdep_set_class_and_subclass() is used.
> > As far as I know, this function initializes lockdep key and subclass
> > value with a given variable.
> > A dev->lower_level variable is used as a subclass value in this patch.
> > When dev->lower_level value is changed, the subclass value of this
> > lockdep key is not changed automatically.
> > If this value has to be changed, additional function is needed.
>
> Hmm, but we pass a dynamic subclass to spin_lock_nested().
>
> So I guess I should just remove all the
> lockdep_set_class_and_subclass() and leave subclass to 0?
>
I agree with that.
And, do you have any plan to replace netif_addr_lock_bh() with
netif_addr_lock_nested()?
(Of course, it needs BH handling code)
I'm not sure but I think it would be needed.
> >
> > >>> netif_addr_lock_bh(from);
> > In this function, internally spin_lock_bh() is used and this function
> > might use an 'initialized subclass value' not a current dev->lower_level.
> > At this point, I think the lockdep splat might occur.
> >
> > +static inline void netif_addr_lock_nested(struct net_device *dev)
> > +{
> > + spin_lock_nested(&dev->addr_list_lock, dev->lower_level);
> > +}
> > In this patch, you used netif_addr_lock_nested() too.
> > These two subclass values could be different.
> > But I'm not sure whether using spin_lock_nested with two different
> > subclass values are the right way or not.
>
> Yeah, as long as dev->lower_level is different, it should be different
> subclass. I assume dev->lower_level is automatically adjusted
> whenever the topology changes, like the vlan over bond case above.
>
Yes, you're right.
Thanks!
Taehee Yoo
