Re: [PATCH net v4 07/12] macvlan: use dynamic lockdep key instead of subclass

[Taehee Yoo <ap420073@gmail.com>]

On Sun, 29 Sep 2019 at 04:15, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Hi,
>

Hi Johannes,
Thank you so much for the detailed reviews!

> I hadn't seen the previous patchsets of this, and looking briefly in the
> archives didn't really seem to say anything about this.
>
>
> However, I'm wondering now: patches 2-7 of this patchset look basically
> all identical in a way:
>  * you set the addr_list_lock's class to a newly registered key inside
>    the netdev (or rather the private struct, but doesn't make a big
>    difference)
>  * you set each TX queue's _xmit_lock's class similarly
>  * you set the qdisc_tx_busylock/qdisc_running_key
>
> The first two of these look pretty much completely identical.
>
> Would it perhaps make sense to just do that for *every* netdev? Many of
> those netdevs won't ever nest so it wouldn't really be useful, but I'm
> not convinced it would put that much more strain on lockdep - if
> anything, people are probably creating more VLANs than regular PF/VF
> netdevs anyway?
>
> I didn't see any discussion on this, but perhaps I missed it? The cost
> would be a bigger netdev struct (when lockdep is enabled), but we
> already have that for all the VLANs etc. it's just in the private data,
> so it's not a _huge_ difference really I'd think, and this is quite a
> bit of code for each device type now.
>

Actually I agree with your opinion.
The benefits of this way are to be able to make common helper functions.
That would reduce duplicate codes and we can maintain this more easily.
But I'm not sure about the overhead of this way. So I would like to ask
maintainers and more reviewers about this.

> Alternatively, maybe there could just be some common helper code:
>
> struct nested_netdev_lockdep {
>         struct lock_class_key xmit_lock_key;
>         struct lock_class_key addr_lock_key;
> };
>
> void netdev_init_nested_lockdep(struct net_device *dev,
>                                 struct netsted_netdev_lockdep *l)
> {
>         /* ... */
> }
>
> so you just have to embed a "struct nested_netdev_lockdep" in your
> private data structure and call the common code.
>
> Or maybe make that
>
> void netdev_init_nested_lockdep(
>         struct net_device *dev,
>         struct
> netsted_netdev_lockdep *l,
>         struct lock_class_key
> *qdisc_tx_busylock_key,
>         struct lock_class_key *qdisc_running_key)
>
> so you can't really get that part wrong either?
>
>
> > @@ -922,6 +938,9 @@ static void macvlan_uninit(struct net_device *dev)
> >       port->count -= 1;
> >       if (!port->count)
> >               macvlan_port_destroy(port->dev);
> > +
> > +     lockdep_unregister_key(&vlan->addr_lock_key);
> > +     lockdep_unregister_key(&vlan->xmit_lock_key);
> >  }
>
> OK, so I guess you need an equivalent "deinit" function too -
> netdev_deinit_nested_lockdep() or so.
>

Using "struct nested_netdev_lockdep" looks really good.
I will make common codes such as "struct nested_netdev_lockdep"
and "netdev_devinit_nested_lockdep" and others in a v5 patch.

>
> What's not really clear to me is why the qdisc locks can actually stay
> the same at all levels? Can they just never nest? But then why are they
> different per device type?

I didn't test about qdisc so I didn't modify code related to qdisc code.
If someone reviews this, I would really appreciate.

Thank you

>
> Thanks,
> johannes
>