Re: [RFC Patch net-next 5/6] net_sched: use rcu in fast path

[John Fastabend]

On 16-09-08 10:54 PM, Cong Wang wrote:
> On Thu, Sep 8, 2016 at 8:49 AM, John Fastabend  
> wrote:
>> Agreed not sure why you would ever want to do a late binding and
>> replace on a tc_mirred actions. But it is supported...
> 
> I will let Jamal teach you on this, /me is really tired of explaining
> things to you John.
> 

This was a meta-comment on the use case for doing this with mirred
action. Not necessarily about the patch itself.

I was actually curious where this happens in practice. The only thing
I can think of is your external logging box moved so you need to send
out another port. Is there any open source software that manages 'tc'
like this. If so I would like to read it.

So do you know of any?

.John

Re: [RFC Patch net-next 5/6] net_sched: use rcu in fast path

[Cong Wang]

On Thu, Sep 8, 2016 at 8:49 AM, John Fastabend  wrote:
> Agreed not sure why you would ever want to do a late binding and
> replace on a tc_mirred actions. But it is supported...

I will let Jamal teach you on this, /me is really tired of explaining
things to you John.

Re: [RFC Patch net-next 5/6] net_sched: use rcu in fast path

[Cong Wang]

On Tue, Sep 6, 2016 at 7:52 AM, Eric Dumazet  wrote:
> On Thu, 2016-09-01 at 22:57 -0700, Cong Wang wrote:
>
>
>
> Missing changelog ?

I was too lazy to write a changelog for this RFC patch, surely
it will be added when removing RFC.


>
> Here I have no idea what you want to fix, since John already took care
> all this infra.
>
> Adding extra rcu_dereference() and rcu_read_lock() while the critical
> RCU dereferences already happen in callers is not needed.


Of course it is, TX and RX both have rcu read lock held.


>
>> Signed-off-by: Cong Wang 
>> ---
>>  net/sched/act_api.c | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 2f8db3c..fb6ff52 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -470,10 +470,14 @@ int tcf_action_exec(struct sk_buff *skb, struct 
>> tc_action **actions,
>>   goto exec_done;
>>   }
>>   for (i = 0; i < nr_actions; i++) {
>> - const struct tc_action *a = actions[i];
>> + const struct tc_action *a;
>>
>> + rcu_read_lock();
>
> But the caller already has rcu_read_lock() or rcu_read_lock_bh()
>
> This was done in commit 25d8c0d55f241ce2 ("net: rcu-ify tcf_proto")

More precisely, it is __dev_queue_xmit() or netif_receive_skb_internal().
Not directly related to John.

The reason why I made it is to make it explicit that I take rcu_read_lock()
for tc action fast path, since it can be called recursively.

>
>> + a = rcu_dereference(actions[i]);
>
>
> Add in your .config :
> CONFIG_SPARSE_RCU_POINTER=y
> make C=2 M=net/sched
>

Yeah, kbuild-robot already reported this to me, no worries,
fixing it is already in my TODO list.



>>  repeat:
>>   ret = a->ops->act(skb, a, res);
>> + rcu_read_unlock();
>> +
>>   if (ret == TC_ACT_REPEAT)
>>   goto repeat;/* we need a ttl - JHS */
>>   if (ret != TC_ACT_PIPE)
>
>
>
> I do not believe this patch is necessary.
>
> Please add John as reviewer next time.
>

You missed the rcu_dereference() part.

Re: [RFC Patch net-next 5/6] net_sched: use rcu in fast path

[John Fastabend]

[...]

>>
>> So its at least possible I think these could be interleaved on multiple
>> cpus.
> 
> Sure, this was very clear when I wrote this code. Otherwise I would have
> used an intermediate object and one rcu_dereference() instead of the
> READ_ONCE().
> 
> 
>>
>> Notice that some of the actions are fine though and don't have this
>> issue act_bpf for example is fine.
>>
>> I think we can either fix it in the hash table create part of the
>> list as this series does or just let each action handle it on its own.
> 
> If we want a fix for stable kernels we want a tc_mirred fix on its own,
> and I was planning to do so.
> 
> Given that a "tc qdisc replace ..." drops all packets sitting in the old
> qdisc, I never thought someone would actually depend on atomically
> switching tc_mirred parameters. I for sure did not care.
> 
> Thanks.
> 
> 

Agreed not sure why you would ever want to do a late binding and
replace on a tc_mirred actions. But it is supported...

Re: [RFC Patch net-next 5/6] net_sched: use rcu in fast path

[Eric Dumazet]

On Wed, 2016-09-07 at 23:04 -0700, John Fastabend wrote:

> So the actual issue as I see it is with the late binding actions the
> ones created with the ill documented 'tc action' syntax.
> 
> If you add a rule here and then bind it to a filter (stealing Jamals
> example),
> 
>   tc actions add action skbedit mark 10 index 1
>   tc filter add dev $DEV parent : protocol ip prio 1 u32\
>  match ip dst 17.0.0.1/32 flowid 1:10 action skbedit index 1
> 
> then you can modify this action later with the following
> 
>   tc actions replace action 
> 
> So for an action such as act_mirred we may end up running with a
> partially updated state from this,
> 
>   tcf_mirred_init(...)
>   [...]
> 
>   m->tcf_action = parm->action;
>   m->tcfm_eaction = parm->eaction;
>   
>   [...]
>   
> 
> and then in the execution of the action,
> 
> 
>   tcf_mirred(...)
>   [...]
>   retval = READ_ONCE(m->tcf_action);
>   [...]
> if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
>   [...]
> 
> 
> So its at least possible I think these could be interleaved on multiple
> cpus.

Sure, this was very clear when I wrote this code. Otherwise I would have
used an intermediate object and one rcu_dereference() instead of the
READ_ONCE().


> 
> Notice that some of the actions are fine though and don't have this
> issue act_bpf for example is fine.
> 
> I think we can either fix it in the hash table create part of the
> list as this series does or just let each action handle it on its own.

If we want a fix for stable kernels we want a tc_mirred fix on its own,
and I was planning to do so.

Given that a "tc qdisc replace ..." drops all packets sitting in the old
qdisc, I never thought someone would actually depend on atomically
switching tc_mirred parameters. I for sure did not care.

Thanks.

Re: [RFC Patch net-next 5/6] net_sched: use rcu in fast path

[John Fastabend]

On 16-09-06 07:52 AM, Eric Dumazet wrote:
> On Thu, 2016-09-01 at 22:57 -0700, Cong Wang wrote:
> 
> 
> 
> Missing changelog ?
> 

Yeah this stuff is a bit tricky more notes to walk us through
this would be helpful. (btw you can disregard my comment from
earlier this morning I've tracked most of this down now)

> Here I have no idea what you want to fix, since John already took care
> all this infra.
> 
> Adding extra rcu_dereference() and rcu_read_lock() while the critical
> RCU dereferences already happen in callers is not needed.
> 

Agreed drop all the extra rcu_read_lock/unlock here.

>> Signed-off-by: Cong Wang 
>> ---
>>  net/sched/act_api.c | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 2f8db3c..fb6ff52 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -470,10 +470,14 @@ int tcf_action_exec(struct sk_buff *skb, struct 
>> tc_action **actions,
>>  goto exec_done;
>>  }
>>  for (i = 0; i < nr_actions; i++) {
>> -const struct tc_action *a = actions[i];
>> +const struct tc_action *a;
>>  
>> +rcu_read_lock();
> 
> But the caller already has rcu_read_lock() or rcu_read_lock_bh()
> 
> This was done in commit 25d8c0d55f241ce2 ("net: rcu-ify tcf_proto")
> 
>> +a = rcu_dereference(actions[i]);
> 
> 
> Add in your .config :
> CONFIG_SPARSE_RCU_POINTER=y
> make C=2 M=net/sched
> 
>>  repeat:
>>  ret = a->ops->act(skb, a, res);
>> +rcu_read_unlock();
>> +
>>  if (ret == TC_ACT_REPEAT)
>>  goto repeat;/* we need a ttl - JHS */
>>  if (ret != TC_ACT_PIPE)
> 
> 
> 
> I do not believe this patch is necessary.
> 
> Please add John as reviewer next time.
> 
> Thanks.
> 
> 

So the actual issue as I see it is with the late binding actions the
ones created with the ill documented 'tc action' syntax.

If you add a rule here and then bind it to a filter (stealing Jamals
example),

  tc actions add action skbedit mark 10 index 1
  tc filter add dev $DEV parent : protocol ip prio 1 u32\
 match ip dst 17.0.0.1/32 flowid 1:10 action skbedit index 1

then you can modify this action later with the following

  tc actions replace action 

So for an action such as act_mirred we may end up running with a
partially updated state from this,

tcf_mirred_init(...)
[...]

m->tcf_action = parm->action;
m->tcfm_eaction = parm->eaction;

[...]


and then in the execution of the action,


tcf_mirred(...)
[...]
retval = READ_ONCE(m->tcf_action);
[...]
if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
[...]


So its at least possible I think these could be interleaved on multiple
cpus.

Notice that some of the actions are fine though and don't have this
issue act_bpf for example is fine.

I think we can either fix it in the hash table create part of the
list as this series does or just let each action handle it on its own.

.John

Re: [RFC Patch net-next 5/6] net_sched: use rcu in fast path

[Eric Dumazet]

On Thu, 2016-09-01 at 22:57 -0700, Cong Wang wrote:



Missing changelog ?

Here I have no idea what you want to fix, since John already took care
all this infra.

Adding extra rcu_dereference() and rcu_read_lock() while the critical
RCU dereferences already happen in callers is not needed.

> Signed-off-by: Cong Wang 
> ---
>  net/sched/act_api.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 2f8db3c..fb6ff52 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -470,10 +470,14 @@ int tcf_action_exec(struct sk_buff *skb, struct 
> tc_action **actions,
>   goto exec_done;
>   }
>   for (i = 0; i < nr_actions; i++) {
> - const struct tc_action *a = actions[i];
> + const struct tc_action *a;
>  
> + rcu_read_lock();

But the caller already has rcu_read_lock() or rcu_read_lock_bh()

This was done in commit 25d8c0d55f241ce2 ("net: rcu-ify tcf_proto")

> + a = rcu_dereference(actions[i]);


Add in your .config :
CONFIG_SPARSE_RCU_POINTER=y
make C=2 M=net/sched

>  repeat:
>   ret = a->ops->act(skb, a, res);
> + rcu_read_unlock();
> +
>   if (ret == TC_ACT_REPEAT)
>   goto repeat;/* we need a ttl - JHS */
>   if (ret != TC_ACT_PIPE)



I do not believe this patch is necessary.

Please add John as reviewer next time.

Thanks.

[RFC Patch net-next 5/6] net_sched: use rcu in fast path

[Cong Wang]

Signed-off-by: Cong Wang 
---
 net/sched/act_api.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 2f8db3c..fb6ff52 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -470,10 +470,14 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action 
**actions,
goto exec_done;
}
for (i = 0; i < nr_actions; i++) {
-   const struct tc_action *a = actions[i];
+   const struct tc_action *a;
 
+   rcu_read_lock();
+   a = rcu_dereference(actions[i]);
 repeat:
ret = a->ops->act(skb, a, res);
+   rcu_read_unlock();
+
if (ret == TC_ACT_REPEAT)
goto repeat;/* we need a ttl - JHS */
if (ret != TC_ACT_PIPE)
-- 
2.1.0