From: Daniel Borkmann <daniel@iogearbox•net>
To: Cong Wang <xiyou.wangcong@gmail•com>
Cc: David Miller <davem@davemloft•net>,
Alexei Starovoitov <alexei.starovoitov@gmail•com>,
john fastabend <john.fastabend@gmail•com>,
Jamal Hadi Salim <jhs@mojatatu•com>,
Linux Kernel Network Developers <netdev@vger•kernel.org>,
tgraf@suug•ch
Subject: Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent
Date: Wed, 08 Jun 2016 23:30:14 +0200 [thread overview]
Message-ID: <57588E66.8070509@iogearbox.net> (raw)
In-Reply-To: <CAM_iQpXJAa5B=4ez9z+-Q66rFj+EUTGwrYWMhYoCK=3rp65gYw@mail.gmail.com>
On 06/06/2016 09:52 PM, Cong Wang wrote:
> On Mon, Jun 6, 2016 at 12:25 PM, Daniel Borkmann <daniel@iogearbox•net> wrote:
[...]
> This is fundamental for libnl to update caches.
>
> I don't understand why it should be separated, since notification is
> not a feature, we already have notifications in other paths.
>
>> Looking into this, I would probably make this a single notification that
>> denotes this 'wild-card' removal for that parent instead of calling
>> tfilter_notify() for each filter separately (which allocs skb, dumps it,
>> etc), qdisc del doesn't loop through it either, so probably fine this way.
>
> Makes sense.
I've been playing around with both options and am actually currently
leaning towards the tfilter_notify() for each proto for the reason
that user space tc monitors can simply stay as is. F.e., if someone
keeps an older libnl binary that wouldn't understand such a wildcard
message, then elements in libnl cache won't receive updates since the
meta data won't match on them (average case, there probably are only
one up to a handful of classifiers per parent) ... hm, different topic
but still wondering whether libnl relying on such messages is a good
idea in general since under stress tfilter_notify() can also fail and
user space won't get the updates (except for queries with RTM_GETTFILTER).
Thanks,
Daniel
net/sched/cls_api.c | 37 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a75864d..f873bbc 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -103,6 +103,17 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
struct nlmsghdr *n, struct tcf_proto *tp,
unsigned long fh, int event);
+static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
+ struct nlmsghdr *n,
+ struct tcf_proto __rcu **chain, int event)
+{
+ struct tcf_proto __rcu **it_chain;
+ struct tcf_proto *tp;
+
+ for (it_chain = chain; (tp = rtnl_dereference(*it_chain)) != NULL;
+ it_chain = &tp->next)
+ tfilter_notify(net, oskb, n, tp, 0, event);
+}
/* Select new prio value from the range, managed by kernel. */
@@ -156,11 +167,23 @@ replay:
cl = 0;
if (prio == 0) {
- /* If no priority is given, user wants we allocated it. */
- if (n->nlmsg_type != RTM_NEWTFILTER ||
- !(n->nlmsg_flags & NLM_F_CREATE))
+ switch (n->nlmsg_type) {
+ case RTM_DELTFILTER:
+ if (protocol || t->tcm_handle)
+ return -ENOENT;
+ break;
+ case RTM_NEWTFILTER:
+ /* If no priority is provided by the user,
+ * we allocate one.
+ */
+ if (n->nlmsg_flags & NLM_F_CREATE) {
+ prio = TC_H_MAKE(0x80000000U, 0U);
+ break;
+ }
+ /* fall-through */
+ default:
return -ENOENT;
- prio = TC_H_MAKE(0x80000000U, 0U);
+ }
}
/* Find head of filter chain. */
@@ -200,6 +223,12 @@ replay:
err = -EINVAL;
if (chain == NULL)
goto errout;
+ if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
+ tfilter_notify_chain(net, skb, n, chain, RTM_DELTFILTER);
+ tcf_destroy_chain(chain);
+ err = 0;
+ goto errout;
+ }
/* Check the chain for existence of proto-tcf with this priority */
for (back = chain;
--
1.9.3
next prev parent reply other threads:[~2016-06-08 21:30 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-04 16:24 [PATCH net-next] net, cls: allow for deleting all filters for given parent Daniel Borkmann
2016-06-05 12:24 ` Jamal Hadi Salim
2016-06-05 12:53 ` Sergei Shtylyov
2016-06-05 17:47 ` Daniel Borkmann
2016-06-06 16:41 ` Sergei Shtylyov
2016-06-06 17:12 ` Cong Wang
2016-06-06 19:25 ` Daniel Borkmann
2016-06-06 19:52 ` Cong Wang
2016-06-06 20:32 ` Daniel Borkmann
2016-06-08 21:30 ` Daniel Borkmann [this message]
2016-06-09 22:54 ` Cong Wang
2016-06-10 11:49 ` Jamal Hadi Salim
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=57588E66.8070509@iogearbox.net \
--to=daniel@iogearbox$(echo .)net \
--cc=alexei.starovoitov@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=jhs@mojatatu$(echo .)com \
--cc=john.fastabend@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=tgraf@suug$(echo .)ch \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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