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

[Pieter Jansen van Vuuren]

On Tue, 7 Nov 2017 08:54:20 -0800
Alexander Duyck <alexander.du...@gmail.com> wrote:

> On Mon, Nov 6, 2017 at 10:37 AM, Pieter Jansen van Vuuren
> <pieter.jansenvanvuu...@netronome.com> wrote:
> > On Fri,  3 Nov 2017 11:50:47 -0400
> > Manish Kurup <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.
> >>
> >> Acked-by: Jamal Hadi Salim <j...@mojatatu.com>
> >> Acked-by: Jiri Pirko <j...@mellanox.com>
> >> Signed-off-by: Manish Kurup <manish.ku...@verizon.com>
> >> ---
> >>  include/net/tc_act/tc_vlan.h | 46 +--
> >>  net/sched/act_vlan.c | 75
> >> ++-- 2 files changed, 88
> >> insertions(+), 33 deletions(-)  
> > ...  
> >>
> >> +static void tcf_vlan_cleanup(struct tc_action *a, int bind)
> >> +{
> >> + struct tcf_vlan *v = to_vlan(a);
> >> + struct tcf_vlan_params *p;
> >> +
> >> + p = rcu_dereference_protected(v->vlan_p, 1);
> >> + kfree_rcu(p, rcu);
> >> +}
> >> +
> >>  static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
> >>int bind, int ref)
> >>  {
> >>   unsigned char *b = skb_tail_pointer(skb);
> >>   struct tcf_vlan *v = to_vlan(a);
> >> + struct tcf_vlan_params *p = rtnl_dereference(v->vlan_p);  
> > nack. This fails reverse xmas-tree.  
> 
> Are we really going to be so strict about the reverse xmas-tree that
> we won't allow for assignment w/ variable declaration because the
> dependency order won't fit into that format?
Okay, I think that is a fair point. I would be okay by making an exception
for this.
> 
> Last I knew this kind of setup was an exception to the reverse
> xmas-tree layout requirement because in this case 'p' relies on 'v' so
> we can't reorder these without having to kick the assignment of 'p'
> off onto a line by itself.
I was actually not aware of this, thank you for pointing it out. It does
make sense.
> 
> - Alex

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

[Pieter Jansen van Vuuren]

On Fri,  3 Nov 2017 11:50:47 -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.
> 
> 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 | 75
> ++-- 2 files changed, 88
> insertions(+), 33 deletions(-)
...
>  
> +static void tcf_vlan_cleanup(struct tc_action *a, int bind)
> +{
> + struct tcf_vlan *v = to_vlan(a);
> + struct tcf_vlan_params *p;
> +
> + p = rcu_dereference_protected(v->vlan_p, 1);
> + kfree_rcu(p, rcu);
> +}
> +
>  static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
>int bind, int ref)
>  {
>   unsigned char *b = skb_tail_pointer(skb);
>   struct tcf_vlan *v = to_vlan(a);
> + struct tcf_vlan_params *p = rtnl_dereference(v->vlan_p);
nack. This fails reverse xmas-tree.

Re: [PATCH net-next v6 2/3] nfp flower action: Modified to use VLAN helper functions

[Pieter Jansen van Vuuren]

On Fri,  3 Nov 2017 11:50:25 -0400
Manish Kurup <kurup.man...@gmail.com> wrote:

> Modified netronome nfp flower action to use VLAN helper functions instead
> of accessing the structure directly.
> 
> Signed-off-by: Manish Kurup <manish.ku...@verizon.com>
> ---
>  drivers/net/ethernet/netronome/nfp/flower/action.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/flower/action.c
> b/drivers/net/ethernet/netronome/nfp/flower/action.c index de64ced..c1c595f
> 100644 --- a/drivers/net/ethernet/netronome/nfp/flower/action.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/action.c
> @@ -58,7 +58,6 @@ nfp_fl_push_vlan(struct nfp_fl_push_vlan *push_vlan,
>const struct tc_action *action)
>  {
>   size_t act_size = sizeof(struct nfp_fl_push_vlan);
> - struct tcf_vlan *vlan = to_vlan(action);
>   u16 tmp_push_vlan_tci;
>  
>   push_vlan->head.jump_id = NFP_FL_ACTION_OPCODE_PUSH_VLAN;
> @@ -67,8 +66,8 @@ nfp_fl_push_vlan(struct nfp_fl_push_vlan *push_vlan,
>   push_vlan->vlan_tpid = tcf_vlan_push_proto(action);
>  
>   tmp_push_vlan_tci =
> - FIELD_PREP(NFP_FL_PUSH_VLAN_PRIO, vlan->tcfv_push_prio) |
> - FIELD_PREP(NFP_FL_PUSH_VLAN_VID, vlan->tcfv_push_vid) |
> + FIELD_PREP(NFP_FL_PUSH_VLAN_PRIO,
> tcf_vlan_push_prio(action)) |
> + FIELD_PREP(NFP_FL_PUSH_VLAN_VID, tcf_vlan_push_vid(action))
> | NFP_FL_PUSH_VLAN_CFI;
>   push_vlan->vlan_tci = cpu_to_be16(tmp_push_vlan_tci);
>  }
Thank you for this; you may consider this patch:
Reviewed-by: Pieter Jansen van Vuuren <pieter.jansenvanvuu...@netronome.com>

Re: [PATCH net-next v6 0/3] Incorporated all required changes

[Pieter Jansen van Vuuren]

On Mon, 6 Nov 2017 11:52:37 -0500
Manish Kurup  wrote:

> Hi Dave,
> 
> On Sun, Nov 5, 2017 at 8:08 AM, David Miller  wrote:
> 
> > From: Manish Kurup 
> > Date: Fri,  3 Nov 2017 11:49:19 -0400
> >  
> > > Modified the netronome drivers (flower action) to use the VLAN helper
> > > functions instead of dereferencing the structure directly. This is
> > > required for the VLAN action patch.
> > >
> > > Could you please review?  
> >
> > Please use a more appropriate patch series header posting than this.
> >
> > This subject shall describe what the patch series is about, in much
> > the same style as a normal commit, using appropriate subsystem
> > prefixes and so on.
> >
> > The commit message body must describe what the patch series is doing,
> > how it is doing it, and why it is doing it that way.
> >  
> 
> Mistakenly unicasted my reply (below) to Dave.
> 
> Currently, the body of the commit message describes what it is doing in
> each commit patch. Would you like me to add some detail to the
> description of each commit?
Hi Manish, Your patch series header is "Incorporated all required
changes", could you update this to something more descriptive?

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

[Pieter Jansen van Vuuren]

On Tue, 31 Oct 2017 10:16:05 -0400
Manish Kurup <kurup.man...@gmail.com> wrote:

> Hi Pieter,
> 
> On Tue, Oct 31, 2017 at 9:06 AM, Pieter Jansen van Vuuren <
> pieter.jansenvanvuu...@netronome.com> wrote:  
> 
> > On Sun, 29 Oct 2017 04:47:54 -0400
> > Manish Kurup <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.
> > > Fixed nxp flower action to use VLAN helper functions instead of  
> > accessing the  
> > > structure directly (build break error).  
> > Thank you Manish. One minor nit, I think you meant nfp flower action...
> > Also is it possible to split this patch into 2 patches, it seems to do 2
> > things:
> > 1. Update the VLAN action to use RCU.
> > 2. Fix the nfp flower action to use the VLAN helper.
> >
> > Can you please dump the build break error you are seeing here?
> > ...
> >  
> 
> Here's what the kbuild robot sent me (follows this mail).
> 
> I cannot split this into 2 patches, since this has to be part of a patch
> I'd sent out for review earlier (the one that fixe the act_vlan action).
> Please let me know if you agree with my changes, and I will go ahead and
> commit this.
> 
> Thanks,
> 
> -Manish
Thanks Manish. I don't see obvious issues with this patch. The changes to nfp
flower action could exist standalone without your other patches, no? If that is
the case I would prefer having them split into 2 patches but keep them part of
your patch set. But I guess this is not crucial. Also the build break error
mentioned only occurs after your patches are applied. I would reword the commit
message to "update nfp flower action to use VLAN helper accessing the structure
directly.". The original commit message made me think there might be an bug in
net.
...

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

[Pieter Jansen van Vuuren]

On Sun, 29 Oct 2017 04:47:54 -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.
> Fixed nxp flower action to use VLAN helper functions instead of accessing the
> structure directly (build break error).
Thank you Manish. One minor nit, I think you meant nfp flower action...
Also is it possible to split this patch into 2 patches, it seems to do 2 things:
1. Update the VLAN action to use RCU.
2. Fix the nfp flower action to use the VLAN helper.

Can you please dump the build break error you are seeing here?
...

[PATCH net 3/3] nfp: remove incorrect mask check for vlan matching

[Pieter Jansen van Vuuren]

Previously the vlan tci field was incorrectly exact matched. This patch
fixes this by using the flow dissector to populate the vlan tci field.

Fixes: 5571e8c9f241 ("nfp: extend flower matching capabilities")
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuu...@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicin...@netronome.com>
Reviewed-by: Simon Horman <simon.hor...@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/match.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/match.c 
b/drivers/net/ethernet/netronome/nfp/flower/match.c
index b365110..d25b503 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/match.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/match.c
@@ -42,6 +42,7 @@ nfp_flower_compile_meta_tci(struct nfp_flower_meta_two *frame,
struct tc_cls_flower_offload *flow, u8 key_type,
bool mask_version)
 {
+   struct fl_flow_key *target = mask_version ? flow->mask : flow->key;
struct flow_dissector_key_vlan *flow_vlan;
u16 tmp_tci;
 
@@ -50,15 +51,10 @@ nfp_flower_compile_meta_tci(struct nfp_flower_meta_two 
*frame,
frame->nfp_flow_key_layer = key_type;
frame->mask_id = ~0;
 
-   if (mask_version) {
-   frame->tci = cpu_to_be16(~0);
-   return;
-   }
-
if (dissector_uses_key(flow->dissector, FLOW_DISSECTOR_KEY_VLAN)) {
flow_vlan = skb_flow_dissector_target(flow->dissector,
  FLOW_DISSECTOR_KEY_VLAN,
- flow->key);
+ target);
/* Populate the tci field. */
if (flow_vlan->vlan_id) {
tmp_tci = FIELD_PREP(NFP_FLOWER_MASK_VLAN_PRIO,
-- 
2.7.4

[PATCH net 2/3] nfp: fix supported key layers calculation

[Pieter Jansen van Vuuren]

Previously when calculating the supported key layers MPLS, IPv4/6
TTL and TOS were not considered. This patch checks that the TTL and
TOS fields are masked out before offloading. Additionally this patch
checks that MPLS packets are correctly handled, by not offloading them.

Fixes: af9d842c1354 ("nfp: extend flower add flow offload")
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuu...@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicin...@netronome.com>
Reviewed-by: Simon Horman <simon.hor...@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/offload.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c 
b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 6c8ecc2..74a96d6 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -107,6 +107,7 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls 
*ret_key_ls,
 {
struct flow_dissector_key_basic *mask_basic = NULL;
struct flow_dissector_key_basic *key_basic = NULL;
+   struct flow_dissector_key_ip *mask_ip = NULL;
u32 key_layer_two;
u8 key_layer;
int key_size;
@@ -132,6 +133,11 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls 
*ret_key_ls,
  flow->key);
}
 
+   if (dissector_uses_key(flow->dissector, FLOW_DISSECTOR_KEY_IP))
+   mask_ip = skb_flow_dissector_target(flow->dissector,
+   FLOW_DISSECTOR_KEY_IP,
+   flow->mask);
+
key_layer_two = 0;
key_layer = NFP_FLOWER_LAYER_PORT | NFP_FLOWER_LAYER_MAC;
key_size = sizeof(struct nfp_flower_meta_one) +
@@ -142,11 +148,19 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls 
*ret_key_ls,
/* Ethernet type is present in the key. */
switch (key_basic->n_proto) {
case cpu_to_be16(ETH_P_IP):
+   if (mask_ip && mask_ip->tos)
+   return -EOPNOTSUPP;
+   if (mask_ip && mask_ip->ttl)
+   return -EOPNOTSUPP;
key_layer |= NFP_FLOWER_LAYER_IPV4;
key_size += sizeof(struct nfp_flower_ipv4);
break;
 
case cpu_to_be16(ETH_P_IPV6):
+   if (mask_ip && mask_ip->tos)
+   return -EOPNOTSUPP;
+   if (mask_ip && mask_ip->ttl)
+   return -EOPNOTSUPP;
key_layer |= NFP_FLOWER_LAYER_IPV6;
key_size += sizeof(struct nfp_flower_ipv6);
break;
@@ -157,6 +171,11 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls 
*ret_key_ls,
case cpu_to_be16(ETH_P_ARP):
return -EOPNOTSUPP;
 
+   /* Currently we do not offload MPLS. */
+   case cpu_to_be16(ETH_P_MPLS_UC):
+   case cpu_to_be16(ETH_P_MPLS_MC):
+   return -EOPNOTSUPP;
+
/* Will be included in layer 2. */
case cpu_to_be16(ETH_P_8021Q):
break;
-- 
2.7.4

[PATCH net 0/3] fix layer calculation and flow dissector use

[Pieter Jansen van Vuuren]

Hi,

Previously when calculating the supported key layers MPLS, IPv4/6
TTL and TOS were not considered. Formerly flow dissectors were referenced
without first checking that they are in use and correctly populated by TC.
Additionally this patch set fixes the incorrect use of mask field for vlan
matching.

Pieter Jansen van Vuuren (3):
  nfp: fix unchecked flow dissector use
  nfp: fix supported key layers calculation
  nfp: remove incorrect mask check for vlan matching

 drivers/net/ethernet/netronome/nfp/flower/match.c  | 139 +++--
 .../net/ethernet/netronome/nfp/flower/offload.c|  60 ++---
 2 files changed, 113 insertions(+), 86 deletions(-)

-- 
2.7.4

[PATCH net 1/3] nfp: fix unchecked flow dissector use

[Pieter Jansen van Vuuren]

Previously flow dissectors were referenced without first checking that
they are in use and correctly populated by TC. This patch fixes this by
checking each flow dissector key before referencing them.

Fixes: 5571e8c9f241 ("nfp: extend flower matching capabilities")
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuu...@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicin...@netronome.com>
Reviewed-by: Simon Horman <simon.hor...@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/match.c  | 133 +++--
 .../net/ethernet/netronome/nfp/flower/offload.c|  41 ---
 2 files changed, 93 insertions(+), 81 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/match.c 
b/drivers/net/ethernet/netronome/nfp/flower/match.c
index 0e08404..b365110 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/match.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/match.c
@@ -45,6 +45,7 @@ nfp_flower_compile_meta_tci(struct nfp_flower_meta_two *frame,
struct flow_dissector_key_vlan *flow_vlan;
u16 tmp_tci;
 
+   memset(frame, 0, sizeof(struct nfp_flower_meta_two));
/* Populate the metadata frame. */
frame->nfp_flow_key_layer = key_type;
frame->mask_id = ~0;
@@ -54,21 +55,20 @@ nfp_flower_compile_meta_tci(struct nfp_flower_meta_two 
*frame,
return;
}
 
-   flow_vlan = skb_flow_dissector_target(flow->dissector,
- FLOW_DISSECTOR_KEY_VLAN,
- flow->key);
-
-   /* Populate the tci field. */
-   if (!flow_vlan->vlan_id) {
-   tmp_tci = 0;
-   } else {
-   tmp_tci = FIELD_PREP(NFP_FLOWER_MASK_VLAN_PRIO,
-flow_vlan->vlan_priority) |
- FIELD_PREP(NFP_FLOWER_MASK_VLAN_VID,
-flow_vlan->vlan_id) |
- NFP_FLOWER_MASK_VLAN_CFI;
+   if (dissector_uses_key(flow->dissector, FLOW_DISSECTOR_KEY_VLAN)) {
+   flow_vlan = skb_flow_dissector_target(flow->dissector,
+ FLOW_DISSECTOR_KEY_VLAN,
+ flow->key);
+   /* Populate the tci field. */
+   if (flow_vlan->vlan_id) {
+   tmp_tci = FIELD_PREP(NFP_FLOWER_MASK_VLAN_PRIO,
+flow_vlan->vlan_priority) |
+ FIELD_PREP(NFP_FLOWER_MASK_VLAN_VID,
+flow_vlan->vlan_id) |
+ NFP_FLOWER_MASK_VLAN_CFI;
+   frame->tci = cpu_to_be16(tmp_tci);
+   }
}
-   frame->tci = cpu_to_be16(tmp_tci);
 }
 
 static void
@@ -99,17 +99,18 @@ nfp_flower_compile_mac(struct nfp_flower_mac_mpls *frame,
   bool mask_version)
 {
struct fl_flow_key *target = mask_version ? flow->mask : flow->key;
-   struct flow_dissector_key_eth_addrs *flow_mac;
-
-   flow_mac = skb_flow_dissector_target(flow->dissector,
-FLOW_DISSECTOR_KEY_ETH_ADDRS,
-target);
+   struct flow_dissector_key_eth_addrs *addr;
 
memset(frame, 0, sizeof(struct nfp_flower_mac_mpls));
 
-   /* Populate mac frame. */
-   ether_addr_copy(frame->mac_dst, _mac->dst[0]);
-   ether_addr_copy(frame->mac_src, _mac->src[0]);
+   if (dissector_uses_key(flow->dissector, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
+   addr = skb_flow_dissector_target(flow->dissector,
+FLOW_DISSECTOR_KEY_ETH_ADDRS,
+target);
+   /* Populate mac frame. */
+   ether_addr_copy(frame->mac_dst, >dst[0]);
+   ether_addr_copy(frame->mac_src, >src[0]);
+   }
 
if (mask_version)
frame->mpls_lse = cpu_to_be32(~0);
@@ -121,14 +122,17 @@ nfp_flower_compile_tport(struct nfp_flower_tp_ports 
*frame,
 bool mask_version)
 {
struct fl_flow_key *target = mask_version ? flow->mask : flow->key;
-   struct flow_dissector_key_ports *flow_tp;
+   struct flow_dissector_key_ports *tp;
 
-   flow_tp = skb_flow_dissector_target(flow->dissector,
-   FLOW_DISSECTOR_KEY_PORTS,
-   target);
+   memset(frame, 0, sizeof(struct nfp_flower_tp_ports));
 
-   frame->port_src = flow_tp->src;
-   frame->port_dst = flow_tp->dst;
+   if (dissector_uses_key(flow->dissector, FLOW_DISSECTOR_KEY_PORTS)