On Sun, 29 Sep 2019 at 04:15, Johannes Berg [off-list ref] 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?

quoted

@@ -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