RE: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Chris Mi]

> -Original Message-
> From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
> Sent: Monday, October 23, 2017 11:40 PM
> To: Chris Mi <chr...@mellanox.com>
> Cc: Jamal Hadi Salim <j...@mojatatu.com>; Linux Kernel Network Developers
> <netdev@vger.kernel.org>; Lucas Bates <luc...@mojatatu.com>; Jiri Pirko
> <j...@resnulli.us>; David Miller <da...@davemloft.net>
> Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and
> bindcnt to atomic
> 
> On Sun, Oct 22, 2017 at 7:47 PM, Chris Mi <chr...@mellanox.com> wrote:
> >
> > It seems it is not easy to discard call_rcu().  I'm afraid even if we
> > have a final solution without call_rcu(), it is not mature at the
> > beginning as well. I mean we also need time
> 
> Why do you believe it is not easy? RTNL lock is already there,
> list_splice_init_rcu() is there too. I can naturally divide my patches for 
> each
> module so that they are much easier to backport than yours.
I tested your patches. It takes about 17 seconds to run the same test.
I believe your code is simpler and easy to maintain. If it is stable,
we will also get benefit. Thanks!
> 
> 
> > to fix the possible bugs of the new design. And maybe to destroy the
> > filters in parallel is the right direction. If this bug is the last
> > bug brought by call_rcu(), then changing it may not be a good idea.
> 
> Again, you have to prove this is the last bug, I seriously doubt it is.
> 
> 
> >
> > Patch 1 is straightforward to use atomic. Patch 2 is to convert the list to
> array.
> 
> Both are big in size.
> 
> 
> > I think there is no harm to the new design.  Patch 3 and 4 are useful test
> case.
> 
> It definitely doesn't harm, but why do we waste time on it when we know
> there is a better way? It is clearly not easy for backport either, in fact it 
> is
> harder w.r.t. size.
> 
> 
> > We also need it with new design to make sure there is no regression.
> >
> 
> Are you saying I can't trust your test cases? ;)
> 
> 
> > So I think my patch set should not be held so long time.
> 
> I think your patches should be dropped except the last two, I will take the
> last two for you.
> 
> Thanks!

Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Cong Wang]

On Sun, Oct 22, 2017 at 7:47 PM, Chris Mi  wrote:
>
> It seems it is not easy to discard call_rcu().  I'm afraid even if we have a 
> final solution
> without call_rcu(), it is not mature at the beginning as well. I mean we also 
> need time

Why do you believe it is not easy? RTNL lock is already there,
list_splice_init_rcu() is there too. I can naturally divide my patches
for each module so that they are much easier to backport than
yours.


> to fix the possible bugs of the new design. And maybe to destroy the filters 
> in parallel
> is the right direction. If this bug is the last bug brought by call_rcu(), 
> then changing it
> may not be a good idea.

Again, you have to prove this is the last bug, I seriously doubt
it is.


>
> Patch 1 is straightforward to use atomic. Patch 2 is to convert the list to 
> array.

Both are big in size.


> I think there is no harm to the new design.  Patch 3 and 4 are useful test 
> case.

It definitely doesn't harm, but why do we waste time on it when we
know there is a better way? It is clearly not easy for backport either,
in fact it is harder w.r.t. size.


> We also need it with new design to make sure there is no regression.
>

Are you saying I can't trust your test cases? ;)


> So I think my patch set should not be held so long time.

I think your patches should be dropped except the last two,
I will take the last two for you.

Thanks!

RE: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Chris Mi]

> -Original Message-
> From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
> Sent: Friday, October 20, 2017 11:00 AM
> To: Jamal Hadi Salim <j...@mojatatu.com>
> Cc: Chris Mi <chr...@mellanox.com>; Linux Kernel Network Developers
> <netdev@vger.kernel.org>; Lucas Bates <luc...@mojatatu.com>; Jiri Pirko
> <j...@resnulli.us>; David Miller <da...@davemloft.net>
> Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and
> bindcnt to atomic
> 
> On Thu, Oct 19, 2017 at 7:21 AM, Jamal Hadi Salim <j...@mojatatu.com>
> wrote:
> > On 17-10-18 12:43 PM, Cong Wang wrote:
> >>
> >> On Tue, Oct 17, 2017 at 6:03 PM, Chris Mi <chr...@mellanox.com> wrote:
> >>>>
> >>>> -Original Message-
> >
> >
> >>
> >> You listed 3 problems, and you think they are 3 different ones, here
> >> I argue problem 3 (using RCU callbacks) is the cause of problem 1
> >> (refcnt not atomic). This is why I mentioned I have been thinking
> >> about removing RCU callbacks, because it probably could fix all of them.
> >>
> >
> > Cong,
> > Given this is a known bug (the test case Chris presented crashes the
> > kernel) - would it make sense to have a patch that goes to -net to fix
> > this while your approach and discussion outcome goes into net-next?
> 
> I am not sure. Because Chris' patchset is large too and I don't think it 
> could fix
> all crashes, so it has little value to just apply them for -net.

It seems it is not easy to discard call_rcu().  I'm afraid even if we have a 
final solution
without call_rcu(), it is not mature at the beginning as well. I mean we also 
need time
to fix the possible bugs of the new design. And maybe to destroy the filters in 
parallel
is the right direction. If this bug is the last bug brought by call_rcu(), then 
changing it
may not be a good idea.
 
Patch 1 is straightforward to use atomic. Patch 2 is to convert the list to 
array.
I think there is no harm to the new design.  Patch 3 and 4 are useful test case.
We also need it with new design to make sure there is no regression.

So I think my patch set should not be held so long time.

My two cents.

Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Cong Wang]

On Thu, Oct 19, 2017 at 7:21 AM, Jamal Hadi Salim  wrote:
> On 17-10-18 12:43 PM, Cong Wang wrote:
>>
>> On Tue, Oct 17, 2017 at 6:03 PM, Chris Mi  wrote:

 -Original Message-
>
>
>>
>> You listed 3 problems, and you think they are 3 different ones, here
>> I argue problem 3 (using RCU callbacks) is the cause of problem 1
>> (refcnt not atomic). This is why I mentioned I have been thinking about
>> removing RCU callbacks, because it probably could fix all of them.
>>
>
> Cong,
> Given this is a known bug (the test case Chris presented crashes the
> kernel) - would it make sense to have a patch that goes to -net
> to fix this while your approach and discussion outcome goes into
> net-next?

I am not sure. Because Chris' patchset is large too and I don't think
it could fix all crashes, so it has little value to just apply them for -net.

Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Jamal Hadi Salim]

On 17-10-18 12:43 PM, Cong Wang wrote:

On Tue, Oct 17, 2017 at 6:03 PM, Chris Mi  wrote:

-Original Message-




You listed 3 problems, and you think they are 3 different ones, here
I argue problem 3 (using RCU callbacks) is the cause of problem 1
(refcnt not atomic). This is why I mentioned I have been thinking about
removing RCU callbacks, because it probably could fix all of them.



Cong,
Given this is a known bug (the test case Chris presented crashes the
kernel) - would it make sense to have a patch that goes to -net
to fix this while your approach and discussion outcome goes into
net-next?

cheers,
jamal

Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Cong Wang]

On Tue, Oct 17, 2017 at 6:03 PM, Chris Mi <chr...@mellanox.com> wrote:
>> -Original Message-
>> From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
>> Sent: Tuesday, October 17, 2017 11:53 PM
>> To: Chris Mi <chr...@mellanox.com>
>> Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; Jamal Hadi
>> Salim <j...@mojatatu.com>; Lucas Bates <luc...@mojatatu.com>; Jiri Pirko
>> <j...@resnulli.us>; David Miller <da...@davemloft.net>
>> Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and
>> bindcnt to atomic
>>
>> On Mon, Oct 16, 2017 at 6:14 PM, Chris Mi <chr...@mellanox.com> wrote:
>> > I don't think this bug were introduced by above two commits only.
>> > Actually, this bug were introduced by several commits, at least the
>> following:
>> > 1. refcnt and bindcnt are not atomic
>>
>> Nope, it is perfectly okay with non-atomic as long as no parallel, and 
>> without
>> RCU callback they are perfectly serialized by RTNL.
> Agree.
>>
>>
>> > 2. passing actions using list instead of arrays (I think initially we
>> > are using arrays)
>>
>> We are discussing patch 1/4, this is patch 2/4, so irrelevant.
> Agree.
>>
>>
>> > 3. using RCU callbacks
>>
>> This introduces problem 1.
> I think this patch set only fixes one problem, that's the race and the panic.
> What do you mean by problem 1.


You listed 3 problems, and you think they are 3 different ones, here
I argue problem 3 (using RCU callbacks) is the cause of problem 1
(refcnt not atomic). This is why I mentioned I have been thinking about
removing RCU callbacks, because it probably could fix all of them.


>>
>>
>> > So instead of blaming the latest commit, it is better to say it is a 
>> > pre-git error.
>>
>> You are wrong.
> OK, you are right. But could I know what's your suggestion for this patch set?
> 1. reject it?
> 2. change the "Fixes" as you suggested?
> 3. something else?

In my opinion we need to think about removing RCU callbacks
rather than fixing all bugs they introduce, because it is really hard
to prove we can fix all of them. In your patchset, you fix 2 bugs.
Before, we fixed 2 bugs (I already list them in the other reply to you).
In total, we have 4 bugs... Are we totally race-free even after
your patches? It seems not at all without a lock, but as I said locking
itself is hard...

I will start a new thread to discuss this and keep you Cc'ed. So
please hold your patches until we have a conclusion.

Thanks.

RE: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Chris Mi]

> -Original Message-
> From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
> Sent: Tuesday, October 17, 2017 11:53 PM
> To: Chris Mi <chr...@mellanox.com>
> Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; Jamal Hadi
> Salim <j...@mojatatu.com>; Lucas Bates <luc...@mojatatu.com>; Jiri Pirko
> <j...@resnulli.us>; David Miller <da...@davemloft.net>
> Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and
> bindcnt to atomic
> 
> On Mon, Oct 16, 2017 at 6:14 PM, Chris Mi <chr...@mellanox.com> wrote:
> > I don't think this bug were introduced by above two commits only.
> > Actually, this bug were introduced by several commits, at least the
> following:
> > 1. refcnt and bindcnt are not atomic
> 
> Nope, it is perfectly okay with non-atomic as long as no parallel, and without
> RCU callback they are perfectly serialized by RTNL.
Agree.
> 
> 
> > 2. passing actions using list instead of arrays (I think initially we
> > are using arrays)
> 
> We are discussing patch 1/4, this is patch 2/4, so irrelevant.
Agree.
> 
> 
> > 3. using RCU callbacks
> 
> This introduces problem 1.
I think this patch set only fixes one problem, that's the race and the panic.
What do you mean by problem 1.
> 
> 
> > So instead of blaming the latest commit, it is better to say it is a 
> > pre-git error.
> 
> You are wrong.
OK, you are right. But could I know what's your suggestion for this patch set?
1. reject it?
2. change the "Fixes" as you suggested?
3. something else?

Thanks,
Chris

Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Cong Wang]

On Mon, Oct 16, 2017 at 6:14 PM, Chris Mi  wrote:
> I don't think this bug were introduced by above two commits only.
> Actually, this bug were introduced by several commits, at least the following:
> 1. refcnt and bindcnt are not atomic

Nope, it is perfectly okay with non-atomic as long as no parallel,
and without RCU callback they are perfectly serialized by RTNL.


> 2. passing actions using list instead of arrays (I think initially we are 
> using arrays)

We are discussing patch 1/4, this is patch 2/4, so irrelevant.


> 3. using RCU callbacks

This introduces problem 1.


> So instead of blaming the latest commit, it is better to say it is a pre-git 
> error.

You are wrong.

RE: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Chris Mi]

> -Original Message-
> From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
> Sent: Tuesday, October 17, 2017 1:06 AM
> To: Chris Mi <chr...@mellanox.com>
> Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; Jamal Hadi
> Salim <j...@mojatatu.com>; Lucas Bates <luc...@mojatatu.com>; Jiri Pirko
> <j...@resnulli.us>; David Miller <da...@davemloft.net>
> Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and
> bindcnt to atomic
> 
> On Mon, Oct 16, 2017 at 4:18 AM, Chris Mi <chr...@mellanox.com> wrote:
> > If many filters share the same action. That action's refcnt and
> > bindcnt could be manipulated by many RCU callbacks at the same time.
> > This patch makes these operations atomic.
> 
> Actually I have been thinking about removing these RCU callbacks, they are
> not necessary AFAIK, callers hold RTNL lock so they are allowed to block. The
> only drawback is that adding a synchronize_rcu(), but these are slow paths
> anyway...
> 
> I am not sure, it is arguable anyway, essentially it is:
> 
> synchronize_rcu() in slow path vs.  multiple RCU callback races
> 
> 
> >
> > Fixes commit in pre-git era.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
> This is not true, the action RCU callbacks were introduced
> by:
> 
> commit c7de2cf053420d63bac85133469c965d4b1083e1
> Author: Eric Dumazet <eric.duma...@gmail.com>
> Date:   Wed Jun 9 02:09:23 2010 +
> 
> pkt_sched: gen_kill_estimator() rcu fixes
> 
> 
> and the filter RCU callbacks were introduced by the patchset like this one:
> 
> 
> commit 1ce87720d456e471de0fbd814dc5d1fe10fc1c44
> Author: John Fastabend <john.fastab...@gmail.com>
> Date:   Fri Sep 12 20:09:16 2014 -0700
> 
> net: sched: make cls_u32 lockless
I don't think this bug were introduced by above two commits only.
Actually, this bug were introduced by several commits, at least the following:
1. refcnt and bindcnt are not atomic
2. passing actions using list instead of arrays (I think initially we are using 
arrays)
3. using RCU callbacks
So instead of blaming the latest commit, it is better to say it is a pre-git 
error.

Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Cong Wang]

On Mon, Oct 16, 2017 at 4:18 AM, Chris Mi  wrote:
> If many filters share the same action. That action's refcnt and bindcnt
> could be manipulated by many RCU callbacks at the same time. This patch
> makes these operations atomic.

Actually I have been thinking about removing these RCU callbacks,
they are not necessary AFAIK, callers hold RTNL lock so they are
allowed to block. The only drawback is that adding a synchronize_rcu(),
but these are slow paths anyway...

I am not sure, it is arguable anyway, essentially it is:

synchronize_rcu() in slow path vs.  multiple RCU callback races


>
> Fixes commit in pre-git era.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

This is not true, the action RCU callbacks were introduced
by:

commit c7de2cf053420d63bac85133469c965d4b1083e1
Author: Eric Dumazet 
Date:   Wed Jun 9 02:09:23 2010 +

pkt_sched: gen_kill_estimator() rcu fixes


and the filter RCU callbacks were introduced by the
patchset like this one:


commit 1ce87720d456e471de0fbd814dc5d1fe10fc1c44
Author: John Fastabend 
Date:   Fri Sep 12 20:09:16 2014 -0700

net: sched: make cls_u32 lockless

[patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Chris Mi]

If many filters share the same action. That action's refcnt and bindcnt
could be manipulated by many RCU callbacks at the same time. This patch
makes these operations atomic.

Fixes commit in pre-git era.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Chris Mi 
Acked-by: Jamal Hadi Salim 
---
 include/net/act_api.h  |  4 ++--
 net/sched/act_api.c| 21 +++--
 net/sched/act_bpf.c|  4 ++--
 net/sched/act_connmark.c   |  4 ++--
 net/sched/act_csum.c   |  4 ++--
 net/sched/act_gact.c   |  4 ++--
 net/sched/act_ife.c|  4 ++--
 net/sched/act_ipt.c|  4 ++--
 net/sched/act_mirred.c |  4 ++--
 net/sched/act_nat.c|  4 ++--
 net/sched/act_pedit.c  |  4 ++--
 net/sched/act_police.c |  4 ++--
 net/sched/act_sample.c |  4 ++--
 net/sched/act_simple.c |  4 ++--
 net/sched/act_skbedit.c|  4 ++--
 net/sched/act_skbmod.c |  4 ++--
 net/sched/act_tunnel_key.c |  4 ++--
 net/sched/act_vlan.c   |  4 ++--
 18 files changed, 45 insertions(+), 44 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index b944e0eb..a469ab6 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -25,8 +25,8 @@ struct tc_action {
struct tcf_idrinfo  *idrinfo;
 
u32 tcfa_index;
-   int tcfa_refcnt;
-   int tcfa_bindcnt;
+   atomic_ttcfa_refcnt;
+   atomic_ttcfa_bindcnt;
u32 tcfa_capab;
int tcfa_action;
struct tcf_ttcfa_tm;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index da6fa82..9c0224d 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -88,12 +88,13 @@ int __tcf_idr_release(struct tc_action *p, bool bind, bool 
strict)
 
if (p) {
if (bind)
-   p->tcfa_bindcnt--;
-   else if (strict && p->tcfa_bindcnt > 0)
+   atomic_dec(>tcfa_bindcnt);
+   else if (strict && atomic_read(>tcfa_bindcnt) > 0)
return -EPERM;
 
-   p->tcfa_refcnt--;
-   if (p->tcfa_bindcnt <= 0 && p->tcfa_refcnt <= 0) {
+   atomic_dec(>tcfa_refcnt);
+   if (atomic_read(>tcfa_bindcnt) == 0 &&
+   atomic_read(>tcfa_refcnt) == 0) {
if (p->ops->cleanup)
p->ops->cleanup(p, bind);
tcf_idr_remove(p->idrinfo, p);
@@ -245,8 +246,8 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, 
struct tc_action **a,
 
if (index && p) {
if (bind)
-   p->tcfa_bindcnt++;
-   p->tcfa_refcnt++;
+   atomic_inc(>tcfa_bindcnt);
+   atomic_inc(>tcfa_refcnt);
*a = p;
return true;
}
@@ -274,9 +275,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, 
struct nlattr *est,
 
if (unlikely(!p))
return -ENOMEM;
-   p->tcfa_refcnt = 1;
+   atomic_set(>tcfa_refcnt, 1);
if (bind)
-   p->tcfa_bindcnt = 1;
+   atomic_set(>tcfa_bindcnt, 1);
 
if (cpustats) {
p->cpu_bstats = netdev_alloc_pcpu_stats(struct 
gnet_stats_basic_cpu);
@@ -727,7 +728,7 @@ static void cleanup_a(struct list_head *actions, int ovr)
return;
 
list_for_each_entry(a, actions, list)
-   a->tcfa_refcnt--;
+   atomic_dec(>tcfa_refcnt);
 }
 
 int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
@@ -751,7 +752,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, 
struct nlattr *nla,
}
act->order = i;
if (ovr)
-   act->tcfa_refcnt++;
+   atomic_inc(>tcfa_refcnt);
list_add_tail(>list, actions);
}
 
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index c0c707e..4ddf281 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -141,8 +141,8 @@ static int tcf_bpf_dump(struct sk_buff *skb, struct 
tc_action *act,
struct tcf_bpf *prog = to_bpf(act);
struct tc_act_bpf opt = {
.index   = prog->tcf_index,
-   .refcnt  = prog->tcf_refcnt - ref,
-   .bindcnt = prog->tcf_bindcnt - bind,
+   .refcnt  = atomic_read(>tcf_refcnt) - ref,
+   .bindcnt = atomic_read(>tcf_bindcnt) - bind,
.action  = prog->tcf_action,
};
struct tcf_t tm;
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 10b7a88..d2cf658 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -153,8 +153,8 @@ static inline