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

From: Taehee Yoo <ap420073@gmail.com>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: "David Miller" <davem@davemloft.net>,
	Netdev <netdev@vger.kernel.org>,
	linux-wireless@vger.kernel.org,
	"Jakub Kicinski" <jakub.kicinski@netronome.com>,
	j.vosburgh@gmail.com, vfalico@gmail.com,
	"Andy Gospodarek" <andy@greyhouse.net>,
	"Jiří Pírko" <jiri@resnulli.us>,
	"Sabrina Dubroca" <sd@queasysnail.net>,
	"Roopa Prabhu" <roopa@cumulusnetworks.com>,
	saeedm@mellanox.com, manishc@marvell.com, rahulv@marvell.com,
	kys@microsoft.com, haiyangz@microsoft.com,
	"Stephen Hemminger" <stephen@networkplumber.org>,
	sashal@kernel.org, hare@suse.de, varun@chelsio.com,
	ubraun@linux.ibm.com, kgraul@linux.ibm.com,
	"Jay Vosburgh" <jay.vosburgh@canonical.com>,
	"Cody Schuffelen" <schuffelen@google.com>,
	bjorn@mork.no
Subject: Re: [PATCH net v4 07/12] macvlan: use dynamic lockdep key instead of subclass
Date: Tue, 22 Oct 2019 01:00:57 +0900	[thread overview]
Message-ID: <CAMArcTWvuqtUv-RKvmw17x4A2JXTkJMOPchRvJ0aRVKKCKemyw@mail.gmail.com> (raw)
In-Reply-To: <bb48fca5a5ffb0a877b2bff8de07ec8090b63427.camel@sipsolutions.net>

On Mon, 7 Oct 2019 at 20:41, Johannes Berg <johannes@sipsolutions.net> 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.
>
> > > 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.
>

"I have checked on this routine.
Only xmit_lock(HARD_TX_LOCK) could be nested. other
qdisc locks(runinng, busylock) will not be nested."

I'm so sorry, I think it's not true.
running lock could be nested.
But lockdep warning doesn't occur because of below code.

seqcount_acquire(&qdisc->running.dep_map, 0, 1, _RET_IP_);

The third argument means trylock.
If trylock is set, lockdep doesn't make lockdep chain.
So, running could be nested but lockdep warning doesn't occur even
these have the same lockdep key.
You can check on /proc/lockdep and /proc/lockdep_chain

> 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.
>
> johannes
>

Thank you
Taehee