netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Florian Westphal <fw@strlen.de>
To: Jiri Pirko <jiri@resnulli.us>
Cc: Cong Wang <xiyou.wangcong@gmail.com>,
	netdev@vger.kernel.org, jhs@mojatatu.com, fw@strlen.de
Subject: Re: [Patch net-next v2 3/4] net_sched: remove tc class reference counting
Date: Fri, 25 Aug 2017 11:18:50 +0200	[thread overview]
Message-ID: <20170825091850.GI15739@breakpoint.cc> (raw)
In-Reply-To: <20170825090021.GA4829@nanopsycho>

Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Aug 25, 2017 at 01:51:29AM CEST, xiyou.wangcong@gmail.com wrote:
> >For TC classes, their ->get() and ->put() are always paired, and the
> >reference counting is completely useless, because:
> >
> >1) For class modification and dumping paths, we already hold RTNL lock,
> >   so all of these ->get(),->change(),->put() are atomic.
> 
> There is ongoing initiative by Florian to avoid taking RTNL for some
> rtnetlink calls. I think that for dumping it could be done in tc as well.
> Don't we need the refcnt then?

Dumping is a problem at this time because several places depend on RTNL
to ensure we get a consistent state, even "simple" functions like
rtnl_fill_ifinfo, see e.g.  2907c35ff64708065e5a7fd54e8ded8263eb3074
(net: hold rtnl again in dump callbacks).

So for these places we already need some other way (e.g. seqlock)
to ensure we don't put garbage in netlink skb.

At this time I think that it is better if Congs patches go in
(Unless there are other problems of course) as they simplify
things quite a bit, and I am not sure that we need refcount.

It might be enough to use rcu and detect when the class we just read
from might have been in inconsistent state (so we can retry).

Does that make sense to you?

  reply	other threads:[~2017-08-25  9:21 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-24 23:51 [Patch net-next v2 0/4] net_sched: clean up tc classes and u32 filter Cong Wang
2017-08-24 23:51 ` [Patch net-next v2 1/4] net_sched: get rid of more forward declarations Cong Wang
2017-08-25  8:45   ` Jiri Pirko
2017-08-25 11:51   ` Jamal Hadi Salim
2017-08-24 23:51 ` [Patch net-next v2 2/4] net_sched: introduce tclass_del_notify() Cong Wang
2017-08-25  8:54   ` Jiri Pirko
2017-08-25 11:51   ` Jamal Hadi Salim
2017-08-24 23:51 ` [Patch net-next v2 3/4] net_sched: remove tc class reference counting Cong Wang
2017-08-25  9:00   ` Jiri Pirko
2017-08-25  9:18     ` Florian Westphal [this message]
2017-08-25  9:36       ` Jiri Pirko
2017-08-25  9:41   ` Jiri Pirko
2017-08-25 12:28   ` Jamal Hadi Salim
2017-08-24 23:51 ` [Patch net-next v2 4/4] net_sched: kill u32_node pointer in Qdisc Cong Wang
2017-08-25  9:06   ` Jiri Pirko
2017-08-25 12:29   ` Jamal Hadi Salim
2017-08-25 12:37     ` Jiri Pirko
2017-08-26  0:20 ` [Patch net-next v2 0/4] net_sched: clean up tc classes and u32 filter David Miller

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=20170825091850.GI15739@breakpoint.cc \
    --to=fw@strlen.de \
    --cc=jhs@mojatatu.com \
    --cc=jiri@resnulli.us \
    --cc=netdev@vger.kernel.org \
    --cc=xiyou.wangcong@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).