Re: [PATCH net-next v3 3/5] tc/act: remove unneeded RCU lock in action callback

2018-07-25 Thread Marcelo Ricardo Leitner
On Wed, Jul 25, 2018 at 07:59:48AM -0400, Jamal Hadi Salim wrote:
> On 24/07/18 04:06 PM, Paolo Abeni wrote:
> > Each lockless action currently does its own RCU locking in ->act().
> > This is allows using plain RCU accessor, even if the context
> > is really RCU BH.
> > 
> > This change drops the per action RCU lock, replace the accessors
> > with _bh variant, cleans up a bit the surronding code and documents
> > the RCU status in the relevant header.
> > No functional nor performance change is intended.
> > 
> > The goal of this patch is clarifying that the RCU critical section
> > used by the tc actions extends up to the classifier's caller.
> > 
> 
> This and 2/5 seems to stand on their own merit.

So does 1/5, I think.

> 
> cheers,
> jamal


Re: [PATCH net-next v3 3/5] tc/act: remove unneeded RCU lock in action callback

2018-07-25 Thread Jiri Pirko
Tue, Jul 24, 2018 at 10:06:41PM CEST, pab...@redhat.com wrote:
>Each lockless action currently does its own RCU locking in ->act().
>This is allows using plain RCU accessor, even if the context
>is really RCU BH.
>
>This change drops the per action RCU lock, replace the accessors
>with _bh variant, cleans up a bit the surronding code and documents

s/surronding/surrounding/


>the RCU status in the relevant header.
>No functional nor performance change is intended.
>
>The goal of this patch is clarifying that the RCU critical section
>used by the tc actions extends up to the classifier's caller.
>
>v1 -> v2:
> - preserve rcu lock in act_bpf: it's needed by eBPF helpers,
>   as pointed out by Daniel
>
>Signed-off-by: Paolo Abeni 

Acked-by: Jiri Pirko 


Re: [PATCH net-next v3 3/5] tc/act: remove unneeded RCU lock in action callback

2018-07-25 Thread Jamal Hadi Salim

On 24/07/18 04:06 PM, Paolo Abeni wrote:

Each lockless action currently does its own RCU locking in ->act().
This is allows using plain RCU accessor, even if the context
is really RCU BH.

This change drops the per action RCU lock, replace the accessors
with _bh variant, cleans up a bit the surronding code and documents
the RCU status in the relevant header.
No functional nor performance change is intended.

The goal of this patch is clarifying that the RCU critical section
used by the tc actions extends up to the classifier's caller.



This and 2/5 seems to stand on their own merit.

cheers,
jamal


[PATCH net-next v3 3/5] tc/act: remove unneeded RCU lock in action callback

2018-07-24 Thread Paolo Abeni
Each lockless action currently does its own RCU locking in ->act().
This is allows using plain RCU accessor, even if the context
is really RCU BH.

This change drops the per action RCU lock, replace the accessors
with _bh variant, cleans up a bit the surronding code and documents
the RCU status in the relevant header.
No functional nor performance change is intended.

The goal of this patch is clarifying that the RCU critical section
used by the tc actions extends up to the classifier's caller.

v1 -> v2:
 - preserve rcu lock in act_bpf: it's needed by eBPF helpers,
   as pointed out by Daniel

Signed-off-by: Paolo Abeni 
---
 include/net/act_api.h  |  2 +-
 include/net/sch_generic.h  |  2 ++
 net/sched/act_csum.c   | 12 +++-
 net/sched/act_ife.c|  5 +
 net/sched/act_mirred.c |  4 +---
 net/sched/act_sample.c |  4 +---
 net/sched/act_skbedit.c| 10 +++---
 net/sched/act_skbmod.c | 21 +
 net/sched/act_tunnel_key.c |  6 +-
 net/sched/act_vlan.c   | 19 +++
 10 files changed, 29 insertions(+), 56 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 683ce41053d9..8c9bc02d05e1 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -85,7 +85,7 @@ struct tc_action_ops {
size_t  size;
struct module   *owner;
int (*act)(struct sk_buff *, const struct tc_action *,
-  struct tcf_result *);
+  struct tcf_result *); /* called under RCU BH lock*/
int (*dump)(struct sk_buff *, struct tc_action *, int, int);
void(*cleanup)(struct tc_action *);
int (*lookup)(struct net *net, struct tc_action **a, u32 index,
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 7432100027b7..056dc1083aa3 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -275,6 +275,8 @@ struct tcf_proto {
/* Fast access part */
struct tcf_proto __rcu  *next;
void __rcu  *root;
+
+   /* called under RCU BH lock*/
int (*classify)(struct sk_buff *,
const struct tcf_proto *,
struct tcf_result *);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index bd232d3bd022..4f092fbd1e20 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -561,15 +561,14 @@ static int tcf_csum(struct sk_buff *skb, const struct 
tc_action *a,
u32 update_flags;
int action;
 
-   rcu_read_lock();
-   params = rcu_dereference(p->params);
+   params = rcu_dereference_bh(p->params);
 
tcf_lastuse_update(>tcf_tm);
bstats_cpu_update(this_cpu_ptr(p->common.cpu_bstats), skb);
 
action = params->action;
if (unlikely(action == TC_ACT_SHOT))
-   goto drop_stats;
+   goto drop;
 
update_flags = params->update_flags;
switch (tc_skb_protocol(skb)) {
@@ -583,16 +582,11 @@ static int tcf_csum(struct sk_buff *skb, const struct 
tc_action *a,
break;
}
 
-unlock:
-   rcu_read_unlock();
return action;
 
 drop:
-   action = TC_ACT_SHOT;
-
-drop_stats:
qstats_drop_inc(this_cpu_ptr(p->common.cpu_qstats));
-   goto unlock;
+   return TC_ACT_SHOT;
 }
 
 static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 3d6e265758c0..df4060e32d43 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -820,14 +820,11 @@ static int tcf_ife_act(struct sk_buff *skb, const struct 
tc_action *a,
struct tcf_ife_params *p;
int ret;
 
-   rcu_read_lock();
-   p = rcu_dereference(ife->params);
+   p = rcu_dereference_bh(ife->params);
if (p->flags & IFE_ENCODE) {
ret = tcf_ife_encode(skb, a, res, p);
-   rcu_read_unlock();
return ret;
}
-   rcu_read_unlock();
 
return tcf_ife_decode(skb, a, res);
 }
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 6afd89a36c69..eeb335f03102 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -181,11 +181,10 @@ static int tcf_mirred(struct sk_buff *skb, const struct 
tc_action *a,
tcf_lastuse_update(>tcf_tm);
bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
 
-   rcu_read_lock();
m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
m_eaction = READ_ONCE(m->tcfm_eaction);
retval = READ_ONCE(m->tcf_action);
-   dev = rcu_dereference(m->tcfm_dev);
+   dev = rcu_dereference_bh(m->tcfm_dev);
if (unlikely(!dev)) {
pr_notice_once("tc mirred: target device is gone\n");
goto out;
@@ -236,7 +235,6 @@ static int tcf_mirred(struct sk_buff *skb, const struct 
tc_action *a,
if