Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[kbuild test robot]

Hi Manish,

Thank you for the patch! Yet we hit a small issue.
[auto build test WARNING on net-next/master]

url:
https://github.com/0day-ci/linux/commits/Manish-Kurup/net-sched-act_vlan-Change-stats-update-to-use-per-core-stats/20171029-053449
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)


vim +70 drivers/net/ethernet/netronome/nfp/flower/action.c

1a1e586f Pieter Jansen van Vuuren 2017-06-29  55  
1a1e586f Pieter Jansen van Vuuren 2017-06-29  56  static void
1a1e586f Pieter Jansen van Vuuren 2017-06-29  57  nfp_fl_push_vlan(struct 
nfp_fl_push_vlan *push_vlan,
1a1e586f Pieter Jansen van Vuuren 2017-06-29  58 const struct 
tc_action *action)
1a1e586f Pieter Jansen van Vuuren 2017-06-29  59  {
1a1e586f Pieter Jansen van Vuuren 2017-06-29  60size_t act_size = 
sizeof(struct nfp_fl_push_vlan);
1a1e586f Pieter Jansen van Vuuren 2017-06-29  61struct tcf_vlan *vlan = 
to_vlan(action);
1a1e586f Pieter Jansen van Vuuren 2017-06-29  62u16 tmp_push_vlan_tci;
1a1e586f Pieter Jansen van Vuuren 2017-06-29  63  
62d3f60b Pieter Jansen van Vuuren 2017-10-20  64push_vlan->head.jump_id 
= NFP_FL_ACTION_OPCODE_PUSH_VLAN;
62d3f60b Pieter Jansen van Vuuren 2017-10-20  65push_vlan->head.len_lw 
= act_size >> NFP_FL_LW_SIZ;
1a1e586f Pieter Jansen van Vuuren 2017-06-29  66push_vlan->reserved = 0;
1a1e586f Pieter Jansen van Vuuren 2017-06-29  67push_vlan->vlan_tpid = 
tcf_vlan_push_proto(action);
1a1e586f Pieter Jansen van Vuuren 2017-06-29  68  
1a1e586f Pieter Jansen van Vuuren 2017-06-29  69tmp_push_vlan_tci =
1a1e586f Pieter Jansen van Vuuren 2017-06-29 @70
FIELD_PREP(NFP_FL_PUSH_VLAN_PRIO, vlan->tcfv_push_prio) |
1a1e586f Pieter Jansen van Vuuren 2017-06-29  71
FIELD_PREP(NFP_FL_PUSH_VLAN_VID, vlan->tcfv_push_vid) |
1a1e586f Pieter Jansen van Vuuren 2017-06-29  72
NFP_FL_PUSH_VLAN_CFI;
1a1e586f Pieter Jansen van Vuuren 2017-06-29  73push_vlan->vlan_tci = 
cpu_to_be16(tmp_push_vlan_tci);
1a1e586f Pieter Jansen van Vuuren 2017-06-29  74  }
1a1e586f Pieter Jansen van Vuuren 2017-06-29  75  

:: The code at line 70 was first introduced by commit
:: 1a1e586f54bfe54a0ba7ea0ac9b8c7b1d3e655f6 nfp: add basic action 
capabilities to flower offloads

:: TO: Pieter Jansen van Vuuren 
:: CC: David S. Miller 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[kbuild test robot]

Hi Manish,

Thank you for the patch! Yet we hit a small issue.
[auto build test ERROR on net-next/master]

url:
https://github.com/0day-ci/linux/commits/Manish-Kurup/net-sched-act_vlan-Change-stats-update-to-use-per-core-stats/20171029-053449
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64 

All errors (new ones prefixed by >>):

   In file included from include/asm-generic/bug.h:4:0,
from arch/ia64/include/asm/bug.h:12,
from include/linux/bug.h:4,
from include/linux/bitfield.h:18,
from drivers/net/ethernet/netronome/nfp/flower/action.c:34:
   drivers/net/ethernet/netronome/nfp/flower/action.c: In function 
'nfp_fl_push_vlan':
>> drivers/net/ethernet/netronome/nfp/flower/action.c:70:41: error: 'struct 
>> tcf_vlan' has no member named 'tcfv_push_prio'
  FIELD_PREP(NFP_FL_PUSH_VLAN_PRIO, vlan->tcfv_push_prio) |
^
   include/linux/compiler.h:553:19: note: in definition of macro 
'__compiletime_assert'
  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:576:2: note: in expansion of macro 
'_compiletime_assert'
 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
 ^~~
   include/linux/build_bug.h:46:37: note: in expansion of macro 
'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
   include/linux/bitfield.h:56:3: note: in expansion of macro 'BUILD_BUG_ON_MSG'
  BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?  \
  ^~~~
   include/linux/bitfield.h:88:3: note: in expansion of macro '__BF_FIELD_CHECK'
  __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
  ^~~~
   drivers/net/ethernet/netronome/nfp/flower/action.c:70:3: note: in expansion 
of macro 'FIELD_PREP'
  FIELD_PREP(NFP_FL_PUSH_VLAN_PRIO, vlan->tcfv_push_prio) |
  ^~
>> drivers/net/ethernet/netronome/nfp/flower/action.c:70:41: error: 'struct 
>> tcf_vlan' has no member named 'tcfv_push_prio'
  FIELD_PREP(NFP_FL_PUSH_VLAN_PRIO, vlan->tcfv_push_prio) |
^
   include/linux/compiler.h:553:19: note: in definition of macro 
'__compiletime_assert'
  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:576:2: note: in expansion of macro 
'_compiletime_assert'
 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
 ^~~
   include/linux/build_bug.h:46:37: note: in expansion of macro 
'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
   include/linux/bitfield.h:56:3: note: in expansion of macro 'BUILD_BUG_ON_MSG'
  BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?  \
  ^~~~
   include/linux/bitfield.h:88:3: note: in expansion of macro '__BF_FIELD_CHECK'
  __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
  ^~~~
   drivers/net/ethernet/netronome/nfp/flower/action.c:70:3: note: in expansion 
of macro 'FIELD_PREP'
  FIELD_PREP(NFP_FL_PUSH_VLAN_PRIO, vlan->tcfv_push_prio) |
  ^~
   In file included from 
drivers/net/ethernet/netronome/nfp/flower/action.c:34:0:
>> drivers/net/ethernet/netronome/nfp/flower/action.c:70:41: error: 'struct 
>> tcf_vlan' has no member named 'tcfv_push_prio'
  FIELD_PREP(NFP_FL_PUSH_VLAN_PRIO, vlan->tcfv_push_prio) |
^
   include/linux/bitfield.h:89:20: note: in definition of macro 'FIELD_PREP'
  ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
   ^~~~
   In file included from include/asm-generic/bug.h:4:0,
from arch/ia64/include/asm/bug.h:12,
from include/linux/bug.h:4,
from include/linux/bitfield.h:18,
from drivers/net/ethernet/netronome/nfp/flower/action.c:34:
>> drivers/net/ethernet/netronome/nfp/flower/action.c:71:40: error: 'struct 
>> tcf_vlan' has no member named 'tcfv_push_vid'
  FIELD_PREP(NFP_FL_PUSH_VLAN_VID, vlan->tcfv_push_vid) |
   ^
   include/linux/compiler.h:553:19: note: in definition of macro 
'__compiletime_assert'
  bool __cond = !(condition);\
  ^
   include/linux/compiler.h:576:2: note: in expansion of macro 
'_compiletime_assert'
 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
 ^~~
   

[PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[Manish Kurup]

Using a spinlock in the VLAN action causes performance issues when the VLAN
action is used on multiple cores. Rewrote the VLAN action to use RCU read
locking for reads and updates instead.

Acked-by: Jamal Hadi Salim 
Acked-by: Jiri Pirko 
Signed-off-by: Manish Kurup 
---
 include/net/tc_act/tc_vlan.h | 46 +--
 net/sched/act_vlan.c | 65 
 2 files changed, 78 insertions(+), 33 deletions(-)

diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
index c2090df..22ae260 100644
--- a/include/net/tc_act/tc_vlan.h
+++ b/include/net/tc_act/tc_vlan.h
@@ -13,12 +13,17 @@
 #include 
 #include 
 
+struct tcf_vlan_params {
+   int   tcfv_action;
+   u16   tcfv_push_vid;
+   __be16tcfv_push_proto;
+   u8tcfv_push_prio;
+   struct rcu_head   rcu;
+};
+
 struct tcf_vlan {
struct tc_actioncommon;
-   int tcfv_action;
-   u16 tcfv_push_vid;
-   __be16  tcfv_push_proto;
-   u8  tcfv_push_prio;
+   struct tcf_vlan_params __rcu *vlan_p;
 };
 #define to_vlan(a) ((struct tcf_vlan *)a)
 
@@ -33,22 +38,45 @@ static inline bool is_tcf_vlan(const struct tc_action *a)
 
 static inline u32 tcf_vlan_action(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_action;
+   u32 tcfv_action;
+
+   rcu_read_lock();
+   tcfv_action = rcu_dereference(to_vlan(a)->vlan_p)->tcfv_action;
+   rcu_read_unlock();
+
+   return tcfv_action;
 }
 
 static inline u16 tcf_vlan_push_vid(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_push_vid;
+   u16 tcfv_push_vid;
+
+   rcu_read_lock();
+   tcfv_push_vid = rcu_dereference(to_vlan(a)->vlan_p)->tcfv_push_vid;
+   rcu_read_unlock();
+
+   return tcfv_push_vid;
 }
 
 static inline __be16 tcf_vlan_push_proto(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_push_proto;
+   __be16 tcfv_push_proto;
+
+   rcu_read_lock();
+   tcfv_push_proto = rcu_dereference(to_vlan(a)->vlan_p)->tcfv_push_proto;
+   rcu_read_unlock();
+
+   return tcfv_push_proto;
 }
 
 static inline u8 tcf_vlan_push_prio(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_push_prio;
-}
+   u8 tcfv_push_prio;
 
+   rcu_read_lock();
+   tcfv_push_prio = rcu_dereference(to_vlan(a)->vlan_p)->tcfv_push_prio;
+   rcu_read_unlock();
+
+   return tcfv_push_prio;
+}
 #endif /* __NET_TC_VLAN_H */
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index b093bad..7f461f9 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -26,6 +26,7 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
tc_action *a,
struct tcf_result *res)
 {
struct tcf_vlan *v = to_vlan(a);
+   struct tcf_vlan_params *p;
int action;
int err;
u16 tci;
@@ -33,24 +34,27 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
tc_action *a,
tcf_lastuse_update(>tcf_tm);
bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
 
-   spin_lock(>tcf_lock);
-   action = v->tcf_action;
-
/* Ensure 'data' points at mac_header prior calling vlan manipulating
 * functions.
 */
if (skb_at_tc_ingress(skb))
skb_push_rcsum(skb, skb->mac_len);
 
-   switch (v->tcfv_action) {
+   rcu_read_lock();
+
+   action = READ_ONCE(v->tcf_action);
+
+   p = rcu_dereference(v->vlan_p);
+
+   switch (p->tcfv_action) {
case TCA_VLAN_ACT_POP:
err = skb_vlan_pop(skb);
if (err)
goto drop;
break;
case TCA_VLAN_ACT_PUSH:
-   err = skb_vlan_push(skb, v->tcfv_push_proto, v->tcfv_push_vid |
-   (v->tcfv_push_prio << VLAN_PRIO_SHIFT));
+   err = skb_vlan_push(skb, p->tcfv_push_proto, p->tcfv_push_vid |
+   (p->tcfv_push_prio << VLAN_PRIO_SHIFT));
if (err)
goto drop;
break;
@@ -69,14 +73,14 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
tc_action *a,
goto drop;
}
/* replace the vid */
-   tci = (tci & ~VLAN_VID_MASK) | v->tcfv_push_vid;
+   tci = (tci & ~VLAN_VID_MASK) | p->tcfv_push_vid;
/* replace prio bits, if tcfv_push_prio specified */
-   if (v->tcfv_push_prio) {
+   if (p->tcfv_push_prio) {
tci &= ~VLAN_PRIO_MASK;
-   tci |= v->tcfv_push_prio << VLAN_PRIO_SHIFT;
+   tci |= p->tcfv_push_prio << VLAN_PRIO_SHIFT;
}
/* put updated tci 

[PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[Manish Kurup]

Using a spinlock in the VLAN action causes performance issues when the VLAN
action is used on multiple cores. Rewrote the VLAN action to use RCU read
locking for reads and updates instead.

Signed-off-by: Manish Kurup 
---
 include/net/tc_act/tc_vlan.h | 46 +--
 net/sched/act_vlan.c | 65 
 2 files changed, 78 insertions(+), 33 deletions(-)

diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
index c2090df..22ae260 100644
--- a/include/net/tc_act/tc_vlan.h
+++ b/include/net/tc_act/tc_vlan.h
@@ -13,12 +13,17 @@
 #include 
 #include 
 
+struct tcf_vlan_params {
+   int   tcfv_action;
+   u16   tcfv_push_vid;
+   __be16tcfv_push_proto;
+   u8tcfv_push_prio;
+   struct rcu_head   rcu;
+};
+
 struct tcf_vlan {
struct tc_actioncommon;
-   int tcfv_action;
-   u16 tcfv_push_vid;
-   __be16  tcfv_push_proto;
-   u8  tcfv_push_prio;
+   struct tcf_vlan_params __rcu *vlan_p;
 };
 #define to_vlan(a) ((struct tcf_vlan *)a)
 
@@ -33,22 +38,45 @@ static inline bool is_tcf_vlan(const struct tc_action *a)
 
 static inline u32 tcf_vlan_action(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_action;
+   u32 tcfv_action;
+
+   rcu_read_lock();
+   tcfv_action = rcu_dereference(to_vlan(a)->vlan_p)->tcfv_action;
+   rcu_read_unlock();
+
+   return tcfv_action;
 }
 
 static inline u16 tcf_vlan_push_vid(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_push_vid;
+   u16 tcfv_push_vid;
+
+   rcu_read_lock();
+   tcfv_push_vid = rcu_dereference(to_vlan(a)->vlan_p)->tcfv_push_vid;
+   rcu_read_unlock();
+
+   return tcfv_push_vid;
 }
 
 static inline __be16 tcf_vlan_push_proto(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_push_proto;
+   __be16 tcfv_push_proto;
+
+   rcu_read_lock();
+   tcfv_push_proto = rcu_dereference(to_vlan(a)->vlan_p)->tcfv_push_proto;
+   rcu_read_unlock();
+
+   return tcfv_push_proto;
 }
 
 static inline u8 tcf_vlan_push_prio(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_push_prio;
-}
+   u8 tcfv_push_prio;
 
+   rcu_read_lock();
+   tcfv_push_prio = rcu_dereference(to_vlan(a)->vlan_p)->tcfv_push_prio;
+   rcu_read_unlock();
+
+   return tcfv_push_prio;
+}
 #endif /* __NET_TC_VLAN_H */
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index b093bad..7f461f9 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -26,6 +26,7 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
tc_action *a,
struct tcf_result *res)
 {
struct tcf_vlan *v = to_vlan(a);
+   struct tcf_vlan_params *p;
int action;
int err;
u16 tci;
@@ -33,24 +34,27 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
tc_action *a,
tcf_lastuse_update(>tcf_tm);
bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
 
-   spin_lock(>tcf_lock);
-   action = v->tcf_action;
-
/* Ensure 'data' points at mac_header prior calling vlan manipulating
 * functions.
 */
if (skb_at_tc_ingress(skb))
skb_push_rcsum(skb, skb->mac_len);
 
-   switch (v->tcfv_action) {
+   rcu_read_lock();
+
+   action = READ_ONCE(v->tcf_action);
+
+   p = rcu_dereference(v->vlan_p);
+
+   switch (p->tcfv_action) {
case TCA_VLAN_ACT_POP:
err = skb_vlan_pop(skb);
if (err)
goto drop;
break;
case TCA_VLAN_ACT_PUSH:
-   err = skb_vlan_push(skb, v->tcfv_push_proto, v->tcfv_push_vid |
-   (v->tcfv_push_prio << VLAN_PRIO_SHIFT));
+   err = skb_vlan_push(skb, p->tcfv_push_proto, p->tcfv_push_vid |
+   (p->tcfv_push_prio << VLAN_PRIO_SHIFT));
if (err)
goto drop;
break;
@@ -69,14 +73,14 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
tc_action *a,
goto drop;
}
/* replace the vid */
-   tci = (tci & ~VLAN_VID_MASK) | v->tcfv_push_vid;
+   tci = (tci & ~VLAN_VID_MASK) | p->tcfv_push_vid;
/* replace prio bits, if tcfv_push_prio specified */
-   if (v->tcfv_push_prio) {
+   if (p->tcfv_push_prio) {
tci &= ~VLAN_PRIO_MASK;
-   tci |= v->tcfv_push_prio << VLAN_PRIO_SHIFT;
+   tci |= p->tcfv_push_prio << VLAN_PRIO_SHIFT;
}
/* put updated tci as hwaccel tag */
-   __vlan_hwaccel_put_tag(skb, v->tcfv_push_proto, tci);

Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[Jiri Pirko]

Wed, Oct 11, 2017 at 06:27:07PM CEST, xiyou.wangc...@gmail.com wrote:
>On Tue, Oct 10, 2017 at 7:33 PM, Manish Kurup  wrote:

[...]


>> @@ -187,16 +196,33 @@ static int tcf_vlan_init(struct net *net, struct 
>> nlattr *nla,
>>
>> v = to_vlan(*a);
>>
>> -   spin_lock_bh(>tcf_lock);
>> -
>> -   v->tcfv_action = action;
>> -   v->tcfv_push_vid = push_vid;
>> -   v->tcfv_push_prio = push_prio;
>> -   v->tcfv_push_proto = push_proto;
>> +   ASSERT_RTNL();
>> +   p = kzalloc(sizeof(*p), GFP_KERNEL);
>> +   if (unlikely(!p)) {
>> +   if (ovr)
>> +   tcf_idr_release(*a, bind);
>> +   return -ENOMEM;
>> +   }
>>
>> v->tcf_action = parm->action;
>>
>> -   spin_unlock_bh(>tcf_lock);
>> +   p_old = rtnl_dereference(v->vlan_p);
>> +
>> +   if (ovr)
>> +   spin_lock_bh(>tcf_lock);
>
>Why still take spinlock when you already have RTNL lock?
>What's the point?

Yeah, I believe this is copy bug from act_skbmod

Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[Cong Wang]

On Tue, Oct 10, 2017 at 7:33 PM, Manish Kurup  wrote:
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index 14c262c..9bb0236 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -29,31 +29,37 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
> tc_action *a,
> int action;
> int err;
> u16 tci;
> +   struct tcf_vlan_params *p;
>
> tcf_lastuse_update(>tcf_tm);
> bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
>
> -   spin_lock(>tcf_lock);
> -   action = v->tcf_action;
> -

spin_lock() is removed here, see below.


> /* Ensure 'data' points at mac_header prior calling vlan manipulating
>  * functions.
>  */
> if (skb_at_tc_ingress(skb))
> skb_push_rcsum(skb, skb->mac_len);
>
> -   switch (v->tcfv_action) {
> +   rcu_read_lock();
> +
> +   action = READ_ONCE(v->tcf_action);
> +
> +   p = rcu_dereference(v->vlan_p);
> +
> +   switch (p->tcfv_action) {
> case TCA_VLAN_ACT_POP:
> err = skb_vlan_pop(skb);
> if (err)
> goto drop;
> break;
> +
> case TCA_VLAN_ACT_PUSH:
> -   err = skb_vlan_push(skb, v->tcfv_push_proto, v->tcfv_push_vid 
> |
> -   (v->tcfv_push_prio << VLAN_PRIO_SHIFT));
> +   err = skb_vlan_push(skb, p->tcfv_push_proto, p->tcfv_push_vid 
> |
> +   (p->tcfv_push_prio << VLAN_PRIO_SHIFT));
> if (err)
> goto drop;
> break;
> +
> case TCA_VLAN_ACT_MODIFY:
> /* No-op if no vlan tag (either hw-accel or in-payload) */
> if (!skb_vlan_tagged(skb))
> @@ -69,15 +75,16 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
> tc_action *a,
> goto drop;
> }
> /* replace the vid */
> -   tci = (tci & ~VLAN_VID_MASK) | v->tcfv_push_vid;
> +   tci = (tci & ~VLAN_VID_MASK) | p->tcfv_push_vid;
> /* replace prio bits, if tcfv_push_prio specified */
> -   if (v->tcfv_push_prio) {
> +   if (p->tcfv_push_prio) {
> tci &= ~VLAN_PRIO_MASK;
> -   tci |= v->tcfv_push_prio << VLAN_PRIO_SHIFT;
> +   tci |= p->tcfv_push_prio << VLAN_PRIO_SHIFT;
> }
> /* put updated tci as hwaccel tag */
> -   __vlan_hwaccel_put_tag(skb, v->tcfv_push_proto, tci);
> +   __vlan_hwaccel_put_tag(skb, p->tcfv_push_proto, tci);
> break;
> +
> default:
> BUG();
> }
> @@ -89,6 +96,7 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
> tc_action *a,
> qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
>
>  unlock:
> +   rcu_read_unlock();
> if (skb_at_tc_ingress(skb))
> skb_pull_rcsum(skb, skb->mac_len);
>


But here spin_unlock() is not removed... At least it doesn't show in diff
context. It's probably unbalanced spinlock.


> @@ -111,6 +119,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr 
> *nla,
> struct nlattr *tb[TCA_VLAN_MAX + 1];
> struct tc_vlan *parm;
> struct tcf_vlan *v;
> +   struct tcf_vlan_params *p, *p_old;
> int action;
> __be16 push_vid = 0;
> __be16 push_proto = 0;
> @@ -187,16 +196,33 @@ static int tcf_vlan_init(struct net *net, struct nlattr 
> *nla,
>
> v = to_vlan(*a);
>
> -   spin_lock_bh(>tcf_lock);
> -
> -   v->tcfv_action = action;
> -   v->tcfv_push_vid = push_vid;
> -   v->tcfv_push_prio = push_prio;
> -   v->tcfv_push_proto = push_proto;
> +   ASSERT_RTNL();
> +   p = kzalloc(sizeof(*p), GFP_KERNEL);
> +   if (unlikely(!p)) {
> +   if (ovr)
> +   tcf_idr_release(*a, bind);
> +   return -ENOMEM;
> +   }
>
> v->tcf_action = parm->action;
>
> -   spin_unlock_bh(>tcf_lock);
> +   p_old = rtnl_dereference(v->vlan_p);
> +
> +   if (ovr)
> +   spin_lock_bh(>tcf_lock);

Why still take spinlock when you already have RTNL lock?
What's the point?

Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[Eric Dumazet]

On Wed, 2017-10-11 at 06:10 -0700, Eric Dumazet wrote:
> On Tue, 2017-10-10 at 22:33 -0400, Manish Kurup wrote:
> > Using a spinlock in the VLAN action causes performance issues when the VLAN
> > action is used on multiple cores. Rewrote the VLAN action to use RCU read
> > locking for reads and updates instead.
> > 
> > Signed-off-by: Manish Kurup 
> > ---
> >  include/net/tc_act/tc_vlan.h | 21 -
> >  net/sched/act_vlan.c | 73 
> > ++--
> >  2 files changed, 63 insertions(+), 31 deletions(-)
> > 
> > diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
> > index c2090df..67fd355 100644
> > --- a/include/net/tc_act/tc_vlan.h
> > +++ b/include/net/tc_act/tc_vlan.h
> > @@ -13,12 +13,17 @@
> >  #include 
> >  #include 
> >  
> > +struct tcf_vlan_params {
> > +   struct rcu_head   rcu;
> > +   int   tcfv_action;
> > +   u16   tcfv_push_vid;
> > +   __be16tcfv_push_proto;
> > +   u8tcfv_push_prio;
> > +};
> > +
> >  struct tcf_vlan {
> > struct tc_actioncommon;
> > -   int tcfv_action;
> > -   u16 tcfv_push_vid;
> > -   __be16  tcfv_push_proto;
> > -   u8  tcfv_push_prio;
> > +   struct tcf_vlan_params __rcu *vlan_p;
> >  };
> >  #define to_vlan(a) ((struct tcf_vlan *)a)
> >  
> > @@ -33,22 +38,22 @@ static inline bool is_tcf_vlan(const struct tc_action 
> > *a)
> >  
> >  static inline u32 tcf_vlan_action(const struct tc_action *a)
> >  {
> > -   return to_vlan(a)->tcfv_action;
> > +   return to_vlan(a)->vlan_p->tcfv_action;
> 
> This is not proper RCU : ->vlan_p should be read once, and using
> rcu_dereference()
> 
> So the caller should provide to this helper the 'p' pointer instead of
> 'a'
> 
> 
> CONFIG_SPARSE_RCU_POINTER=y
> 
> and make C=2 would probably give you warnings about that.
> 


BTW no need to change your .config after :


commit 41a2901e7d220875752a8c870e0b53288a578c20
Author: Paul E. McKenney 
Date:   Fri May 12 15:56:35 2017 -0700

rcu: Remove SPARSE_RCU_POINTER Kconfig option

The sparse-based checking for non-RCU accesses to RCU-protected pointers
has been around for a very long time, and it is now the only type of
sparse-based checking that is optional.  This commit therefore makes
it unconditional.

Reported-by: Ingo Molnar 
Signed-off-by: Paul E. McKenney 
Cc: Fengguang Wu 

Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[Jiri Pirko]

Wed, Oct 11, 2017 at 04:33:39AM CEST, kurup.man...@gmail.com wrote:
>Using a spinlock in the VLAN action causes performance issues when the VLAN
>action is used on multiple cores. Rewrote the VLAN action to use RCU read
>locking for reads and updates instead.
>
>Signed-off-by: Manish Kurup 
>---
> include/net/tc_act/tc_vlan.h | 21 -
> net/sched/act_vlan.c | 73 ++--
> 2 files changed, 63 insertions(+), 31 deletions(-)
>
>diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
>index c2090df..67fd355 100644
>--- a/include/net/tc_act/tc_vlan.h
>+++ b/include/net/tc_act/tc_vlan.h
>@@ -13,12 +13,17 @@
> #include 
> #include 
> 
>+struct tcf_vlan_params {
>+  struct rcu_head   rcu;

Usually rcu_head is put at the very end of struct.


>+  int   tcfv_action;
>+  u16   tcfv_push_vid;
>+  __be16tcfv_push_proto;
>+  u8tcfv_push_prio;
>+};
>+
> struct tcf_vlan {
>   struct tc_actioncommon;
>-  int tcfv_action;
>-  u16 tcfv_push_vid;
>-  __be16  tcfv_push_proto;
>-  u8  tcfv_push_prio;
>+  struct tcf_vlan_params __rcu *vlan_p;
> };
> #define to_vlan(a) ((struct tcf_vlan *)a)
> 
>@@ -33,22 +38,22 @@ static inline bool is_tcf_vlan(const struct tc_action *a)
> 
> static inline u32 tcf_vlan_action(const struct tc_action *a)
> {
>-  return to_vlan(a)->tcfv_action;
>+  return to_vlan(a)->vlan_p->tcfv_action;
> }
> 
> static inline u16 tcf_vlan_push_vid(const struct tc_action *a)
> {
>-  return to_vlan(a)->tcfv_push_vid;
>+  return to_vlan(a)->vlan_p->tcfv_push_vid;
> }
> 
> static inline __be16 tcf_vlan_push_proto(const struct tc_action *a)
> {
>-  return to_vlan(a)->tcfv_push_proto;
>+  return to_vlan(a)->vlan_p->tcfv_push_proto;
> }
> 
> static inline u8 tcf_vlan_push_prio(const struct tc_action *a)
> {
>-  return to_vlan(a)->tcfv_push_prio;
>+  return to_vlan(a)->vlan_p->tcfv_push_prio;

All these helpers should use rtnl_dereference() as well


> }
> 
> #endif /* __NET_TC_VLAN_H */
>diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
>index 14c262c..9bb0236 100644
>--- a/net/sched/act_vlan.c
>+++ b/net/sched/act_vlan.c
>@@ -29,31 +29,37 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
>tc_action *a,
>   int action;
>   int err;
>   u16 tci;
>+  struct tcf_vlan_params *p;
> 
>   tcf_lastuse_update(>tcf_tm);
>   bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
> 
>-  spin_lock(>tcf_lock);
>-  action = v->tcf_action;
>-
>   /* Ensure 'data' points at mac_header prior calling vlan manipulating
>* functions.
>*/
>   if (skb_at_tc_ingress(skb))
>   skb_push_rcsum(skb, skb->mac_len);
> 
>-  switch (v->tcfv_action) {
>+  rcu_read_lock();
>+
>+  action = READ_ONCE(v->tcf_action);
>+

Too many empty lines. At least remove the one above this ^


>+  p = rcu_dereference(v->vlan_p);
>+
>+  switch (p->tcfv_action) {
>   case TCA_VLAN_ACT_POP:
>   err = skb_vlan_pop(skb);
>   if (err)
>   goto drop;
>   break;
>+
>   case TCA_VLAN_ACT_PUSH:
>-  err = skb_vlan_push(skb, v->tcfv_push_proto, v->tcfv_push_vid |
>-  (v->tcfv_push_prio << VLAN_PRIO_SHIFT));
>+  err = skb_vlan_push(skb, p->tcfv_push_proto, p->tcfv_push_vid |
>+  (p->tcfv_push_prio << VLAN_PRIO_SHIFT));

Again, indentation is odd. Please fix.



>   if (err)
>   goto drop;
>   break;
>+

Avoid adding this unrelated empty line.


>   case TCA_VLAN_ACT_MODIFY:
>   /* No-op if no vlan tag (either hw-accel or in-payload) */
>   if (!skb_vlan_tagged(skb))
>@@ -69,15 +75,16 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
>tc_action *a,
>   goto drop;
>   }
>   /* replace the vid */
>-  tci = (tci & ~VLAN_VID_MASK) | v->tcfv_push_vid;
>+  tci = (tci & ~VLAN_VID_MASK) | p->tcfv_push_vid;
>   /* replace prio bits, if tcfv_push_prio specified */
>-  if (v->tcfv_push_prio) {
>+  if (p->tcfv_push_prio) {
>   tci &= ~VLAN_PRIO_MASK;
>-  tci |= v->tcfv_push_prio << VLAN_PRIO_SHIFT;
>+  tci |= p->tcfv_push_prio << VLAN_PRIO_SHIFT;
>   }
>   /* put updated tci as hwaccel tag */
>-  __vlan_hwaccel_put_tag(skb, v->tcfv_push_proto, tci);
>+  __vlan_hwaccel_put_tag(skb, p->tcfv_push_proto, tci);
>   break;
>+

Again, avoid adding this unrelated empty line.


>   default:
>   BUG();
>   }
>@@ -89,6 +96,7 

Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[Eric Dumazet]

On Tue, 2017-10-10 at 22:33 -0400, Manish Kurup wrote:
> Using a spinlock in the VLAN action causes performance issues when the VLAN
> action is used on multiple cores. Rewrote the VLAN action to use RCU read
> locking for reads and updates instead.
> 
> Signed-off-by: Manish Kurup 
> ---
>  include/net/tc_act/tc_vlan.h | 21 -
>  net/sched/act_vlan.c | 73 
> ++--
>  2 files changed, 63 insertions(+), 31 deletions(-)
> 
> diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
> index c2090df..67fd355 100644
> --- a/include/net/tc_act/tc_vlan.h
> +++ b/include/net/tc_act/tc_vlan.h
> @@ -13,12 +13,17 @@
>  #include 
>  #include 
>  
> +struct tcf_vlan_params {
> + struct rcu_head   rcu;
> + int   tcfv_action;
> + u16   tcfv_push_vid;
> + __be16tcfv_push_proto;
> + u8tcfv_push_prio;
> +};
> +
>  struct tcf_vlan {
>   struct tc_actioncommon;
> - int tcfv_action;
> - u16 tcfv_push_vid;
> - __be16  tcfv_push_proto;
> - u8  tcfv_push_prio;
> + struct tcf_vlan_params __rcu *vlan_p;
>  };
>  #define to_vlan(a) ((struct tcf_vlan *)a)
>  
> @@ -33,22 +38,22 @@ static inline bool is_tcf_vlan(const struct tc_action *a)
>  
>  static inline u32 tcf_vlan_action(const struct tc_action *a)
>  {
> - return to_vlan(a)->tcfv_action;
> + return to_vlan(a)->vlan_p->tcfv_action;

This is not proper RCU : ->vlan_p should be read once, and using
rcu_dereference()

So the caller should provide to this helper the 'p' pointer instead of
'a'


CONFIG_SPARSE_RCU_POINTER=y

and make C=2 would probably give you warnings about that.

Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[Jamal Hadi Salim]

On 17-10-10 10:33 PM, Manish Kurup wrote:

Using a spinlock in the VLAN action causes performance issues when the VLAN
action is used on multiple cores. Rewrote the VLAN action to use RCU read
locking for reads and updates instead.

Signed-off-by: Manish Kurup 


Acked-by: Jamal Hadi Salim 

cheers,
jamal

[PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update

[Manish Kurup]

Using a spinlock in the VLAN action causes performance issues when the VLAN
action is used on multiple cores. Rewrote the VLAN action to use RCU read
locking for reads and updates instead.

Signed-off-by: Manish Kurup 
---
 include/net/tc_act/tc_vlan.h | 21 -
 net/sched/act_vlan.c | 73 ++--
 2 files changed, 63 insertions(+), 31 deletions(-)

diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
index c2090df..67fd355 100644
--- a/include/net/tc_act/tc_vlan.h
+++ b/include/net/tc_act/tc_vlan.h
@@ -13,12 +13,17 @@
 #include 
 #include 
 
+struct tcf_vlan_params {
+   struct rcu_head   rcu;
+   int   tcfv_action;
+   u16   tcfv_push_vid;
+   __be16tcfv_push_proto;
+   u8tcfv_push_prio;
+};
+
 struct tcf_vlan {
struct tc_actioncommon;
-   int tcfv_action;
-   u16 tcfv_push_vid;
-   __be16  tcfv_push_proto;
-   u8  tcfv_push_prio;
+   struct tcf_vlan_params __rcu *vlan_p;
 };
 #define to_vlan(a) ((struct tcf_vlan *)a)
 
@@ -33,22 +38,22 @@ static inline bool is_tcf_vlan(const struct tc_action *a)
 
 static inline u32 tcf_vlan_action(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_action;
+   return to_vlan(a)->vlan_p->tcfv_action;
 }
 
 static inline u16 tcf_vlan_push_vid(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_push_vid;
+   return to_vlan(a)->vlan_p->tcfv_push_vid;
 }
 
 static inline __be16 tcf_vlan_push_proto(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_push_proto;
+   return to_vlan(a)->vlan_p->tcfv_push_proto;
 }
 
 static inline u8 tcf_vlan_push_prio(const struct tc_action *a)
 {
-   return to_vlan(a)->tcfv_push_prio;
+   return to_vlan(a)->vlan_p->tcfv_push_prio;
 }
 
 #endif /* __NET_TC_VLAN_H */
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 14c262c..9bb0236 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -29,31 +29,37 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
tc_action *a,
int action;
int err;
u16 tci;
+   struct tcf_vlan_params *p;
 
tcf_lastuse_update(>tcf_tm);
bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
 
-   spin_lock(>tcf_lock);
-   action = v->tcf_action;
-
/* Ensure 'data' points at mac_header prior calling vlan manipulating
 * functions.
 */
if (skb_at_tc_ingress(skb))
skb_push_rcsum(skb, skb->mac_len);
 
-   switch (v->tcfv_action) {
+   rcu_read_lock();
+
+   action = READ_ONCE(v->tcf_action);
+
+   p = rcu_dereference(v->vlan_p);
+
+   switch (p->tcfv_action) {
case TCA_VLAN_ACT_POP:
err = skb_vlan_pop(skb);
if (err)
goto drop;
break;
+
case TCA_VLAN_ACT_PUSH:
-   err = skb_vlan_push(skb, v->tcfv_push_proto, v->tcfv_push_vid |
-   (v->tcfv_push_prio << VLAN_PRIO_SHIFT));
+   err = skb_vlan_push(skb, p->tcfv_push_proto, p->tcfv_push_vid |
+   (p->tcfv_push_prio << VLAN_PRIO_SHIFT));
if (err)
goto drop;
break;
+
case TCA_VLAN_ACT_MODIFY:
/* No-op if no vlan tag (either hw-accel or in-payload) */
if (!skb_vlan_tagged(skb))
@@ -69,15 +75,16 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
tc_action *a,
goto drop;
}
/* replace the vid */
-   tci = (tci & ~VLAN_VID_MASK) | v->tcfv_push_vid;
+   tci = (tci & ~VLAN_VID_MASK) | p->tcfv_push_vid;
/* replace prio bits, if tcfv_push_prio specified */
-   if (v->tcfv_push_prio) {
+   if (p->tcfv_push_prio) {
tci &= ~VLAN_PRIO_MASK;
-   tci |= v->tcfv_push_prio << VLAN_PRIO_SHIFT;
+   tci |= p->tcfv_push_prio << VLAN_PRIO_SHIFT;
}
/* put updated tci as hwaccel tag */
-   __vlan_hwaccel_put_tag(skb, v->tcfv_push_proto, tci);
+   __vlan_hwaccel_put_tag(skb, p->tcfv_push_proto, tci);
break;
+
default:
BUG();
}
@@ -89,6 +96,7 @@ static int tcf_vlan(struct sk_buff *skb, const struct 
tc_action *a,
qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
 
 unlock:
+   rcu_read_unlock();
if (skb_at_tc_ingress(skb))
skb_pull_rcsum(skb, skb->mac_len);
 
@@ -111,6 +119,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr 
*nla,
struct nlattr *tb[TCA_VLAN_MAX + 1];
struct tc_vlan *parm;
struct tcf_vlan