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>,
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: Sat, 5 Oct 2019 18:13:30 +0900 [thread overview]
Message-ID: <CAMArcTVeFGqA2W26=rBD5KkjRpFB6gjSgXj8dp+WWrrwJ7pr-A@mail.gmail.com> (raw)
In-Reply-To: <72bc9727d0943c56403eac03b6de69c00b0f53f6.camel@sipsolutions.net>
On Tue, 1 Oct 2019 at 16:25, Johannes Berg <johannes@sipsolutions.net> 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
next prev parent reply other threads:[~2019-10-05 9:13 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-28 16:48 [PATCH net v4 00/12] net: fix nested device bugs Taehee Yoo
2019-09-28 16:48 ` [PATCH net v4 01/12] net: core: limit nested device depth Taehee Yoo
2019-09-28 19:36 ` Johannes Berg
2019-09-29 11:05 ` Taehee Yoo
2019-10-01 7:11 ` Johannes Berg
2019-10-01 13:53 ` Taehee Yoo
2019-10-01 13:57 ` Johannes Berg
2019-10-01 18:23 ` Taehee Yoo
2019-10-10 10:19 ` Sabrina Dubroca
2019-10-12 11:42 ` Taehee Yoo
2019-09-28 16:48 ` [PATCH net v4 02/12] vlan: use dynamic lockdep key instead of subclass Taehee Yoo
2019-09-28 16:48 ` [PATCH net v4 03/12] bonding: fix unexpected IFF_BONDING bit unset Taehee Yoo
2019-09-30 20:48 ` Jay Vosburgh
2019-09-28 16:48 ` [PATCH net v4 04/12] bonding: use dynamic lockdep key instead of subclass Taehee Yoo
2019-09-28 16:48 ` [PATCH net v4 05/12] team: use dynamic lockdep key instead of static key Taehee Yoo
2019-09-28 16:48 ` [PATCH net v4 06/12] macsec: use dynamic lockdep key instead of subclass Taehee Yoo
2019-09-28 16:48 ` [PATCH net v4 07/12] macvlan: " Taehee Yoo
2019-09-28 19:14 ` Johannes Berg
2019-09-29 8:03 ` Taehee Yoo
2019-10-01 7:25 ` Johannes Berg
2019-10-05 9:13 ` Taehee Yoo [this message]
2019-10-07 11:41 ` Johannes Berg
2019-10-08 8:13 ` Taehee Yoo
2019-10-21 16:00 ` Taehee Yoo
2019-09-28 16:48 ` [PATCH net v4 08/12] macsec: fix refcnt leak in module exit routine Taehee Yoo
2019-09-28 16:48 ` [PATCH net v4 09/12] net: core: add ignore flag to netdev_adjacent structure Taehee Yoo
2019-09-28 16:48 ` [PATCH net v4 10/12] vxlan: add adjacent link to limit depth level Taehee Yoo
2019-09-28 16:48 ` [PATCH net v4 11/12] net: remove unnecessary variables and callback Taehee Yoo
2019-09-28 19:42 ` Johannes Berg
2019-09-28 16:48 ` [PATCH net v4 12/12] virt_wifi: fix refcnt leak in module exit routine Taehee Yoo
2019-09-28 18:57 ` Johannes Berg
2019-10-07 11:22 ` Sabrina Dubroca
2019-10-08 6:53 ` Taehee Yoo
2019-09-28 19:20 ` [PATCH net v4 00/12] net: fix nested device bugs Johannes Berg
2019-09-29 8:31 ` Taehee Yoo
2019-10-01 7:39 ` Johannes Berg
2019-10-05 9:40 ` Taehee Yoo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMArcTVeFGqA2W26=rBD5KkjRpFB6gjSgXj8dp+WWrrwJ7pr-A@mail.gmail.com' \
--to=ap420073@gmail.com \
--cc=andy@greyhouse.net \
--cc=bjorn@mork.no \
--cc=davem@davemloft.net \
--cc=haiyangz@microsoft.com \
--cc=hare@suse.de \
--cc=j.vosburgh@gmail.com \
--cc=jakub.kicinski@netronome.com \
--cc=jay.vosburgh@canonical.com \
--cc=jiri@resnulli.us \
--cc=johannes@sipsolutions.net \
--cc=kgraul@linux.ibm.com \
--cc=kys@microsoft.com \
--cc=linux-wireless@vger.kernel.org \
--cc=manishc@marvell.com \
--cc=netdev@vger.kernel.org \
--cc=rahulv@marvell.com \
--cc=roopa@cumulusnetworks.com \
--cc=saeedm@mellanox.com \
--cc=sashal@kernel.org \
--cc=schuffelen@google.com \
--cc=sd@queasysnail.net \
--cc=stephen@networkplumber.org \
--cc=ubraun@linux.ibm.com \
--cc=varun@chelsio.com \
--cc=vfalico@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).