On Mon, 7 Oct 2019 at 20:41, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > Hi Johannes, > On Sat, 2019-10-05 at 18:13 +0900, Taehee Yoo wrote: > > > > 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 I was thinking was that if we can do this for every VLAN netdev, > why shouldn't we do it for *every* netdev unconditionally? Some code > could perhaps even be simplified if this was just a general part of > netdev allocation. > Your opinion makes sense. I think there is no critical reason that every netdev shouldn't have own lockdep keys. By comparison, the benefits are obvious. > > > 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. > > OK, I still didn't check it too closely I guess, or got confused which > lock I should look at. > > > This patch already > > handles the _xmit_lock key. so I think there is no problem. > > Right > > > 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. > > Nothing to misunderstand - I was just asking/wondering why the qdisc > locks were not treated the same way. > I'm always thankful for your detailed and careful reviews. > johannes > Thank you, Taehee Yoo
