On Tue, 1 Oct 2019 at 16:25, Johannes Berg [off-list ref] wrote:

Hi,

Hi,

quoted

quoted

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.

:-)

quoted

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.

That makes *sense*, but it seems to me that for example in virt_wifi we
just missed this part completely, so addressing it in the generic code
would still reduce overall code and complexity?

Yes, you're right,
Virt_wifi has the same problem. I will fix this in a v5 patch!

Actually, looking at net-next, we already have
netdev_lockdep_set_classes() as a macro there that handles all this. I
guess having it as a macro makes some sense so it "evaporates" when
lockdep isn't enabled.


I'd probably try that but maybe somebody else can chime in and say what
they think about applying that to *every* netdev instead though.

If we place lockdep keys into "struct net_device", this macro would be a
little bit modified and reused. And driver code shape will not be huge
changed. I think this way is better than this v4 way.
So I will try it.

quoted

quoted

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.

I didn't really think hard about it when I wrote this ...

But it seems to me the whole nesting also has to be applied here?

__dev_xmit_skb:
 * qdisc_run_begin()
 * sch_direct_xmit()
   * HARD_TX_LOCK(dev, txq, smp_processor_id());
   * dev_hard_start_xmit() // say this is VLAN
     * dev_queue_xmit() // on real_dev
       * __dev_xmit_skb // recursion on another netdev

Now if you have VLAN-in-VLAN the whole thing will recurse right?

I have checked on this routine.
Only xmit_lock(HARD_TX_LOCK) could be nested. other
qdisc locks(runinng, busylock) will not be nested. This patch already
handles the _xmit_lock key. so I think there is no problem.
But I would like to place four lockdep keys(busylock, address,
running, _xmit_lock) into "struct net_device" because of code complexity.

Let me know if I misunderstood anything.

johannes

Thank you,
Taehee