On Tue, 1 Oct 2019 at 16:25, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > > Hi, > Hi, > > > 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. > > :-) > > > 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. > > > > 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
