[PATCH net 0/2] nfp: MPLS and shared blocks TC offload fixes

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH net 0/2] nfp: MPLS and shared blocks TC offload fixes
@ 2018-06-25 20:22 Jakub Kicinski
  2018-06-25 20:22 ` [PATCH net 1/2] nfp: flower: fix mpls ether type detection Jakub Kicinski
  2018-06-25 20:22 ` [PATCH net 2/2] nfp: reject binding to shared blocks Jakub Kicinski
  0 siblings, 2 replies; 8+ messages in thread
From: Jakub Kicinski @ 2018-06-25 20:22 UTC (permalink / raw)
  To: davem; +Cc: oss-drivers, netdev, Jakub Kicinski

Hi!

This series brings two fixes to TC filter/action offload code.
Pieter fixes matching MPLS packets when the match is purely on
the MPLS ethertype and none of the MPLS fields are used.
John provides a fix for offload of shared blocks.  Unfortunately,
with shared blocks there is currently no guarantee that filters
which were added by the core will be removed before block unbind.
Our simple fix is to not support offload of rules on shared blocks
at all, a revert of this fix will be send for -next once the
reoffload infrastructure lands.  The shared blocks became important
as we are trying to use them for bonding offload (managed from user
space) and lack of remove calls leads to resource leaks.

John Hurley (1):
  nfp: reject binding to shared blocks

Pieter Jansen van Vuuren (1):
  nfp: flower: fix mpls ether type detection

 drivers/net/ethernet/netronome/nfp/bpf/main.c      |  3 +++
 drivers/net/ethernet/netronome/nfp/flower/match.c  | 14 ++++++++++++++
 .../net/ethernet/netronome/nfp/flower/offload.c    | 12 ++++++++++++
 3 files changed, 29 insertions(+)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net 1/2] nfp: flower: fix mpls ether type detection
  2018-06-25 20:22 [PATCH net 0/2] nfp: MPLS and shared blocks TC offload fixes Jakub Kicinski
@ 2018-06-25 20:22 ` Jakub Kicinski
  2018-06-25 20:22 ` [PATCH net 2/2] nfp: reject binding to shared blocks Jakub Kicinski
  1 sibling, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2018-06-25 20:22 UTC (permalink / raw)
  To: davem; +Cc: oss-drivers, netdev, Pieter Jansen van Vuuren

From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

Previously it was not possible to distinguish between mpls ether types and
other ether types. This leads to incorrect classification of offloaded
filters that match on mpls ether type. For example the following two
filters overlap:

 # tc filter add dev eth0 parent ffff: \
    protocol 0x8847 flower \
    action mirred egress redirect dev eth1

 # tc filter add dev eth0 parent ffff: \
    protocol 0x0800 flower \
    action mirred egress redirect dev eth2

The driver now correctly includes the mac_mpls layer where HW stores mpls
fields, when it detects an mpls ether type. It also sets the MPLS_Q bit to
indicate that the filter should match mpls packets.

Fixes: bb055c198d9b ("nfp: add mpls match offloading support")
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/match.c  | 14 ++++++++++++++
 .../net/ethernet/netronome/nfp/flower/offload.c    |  8 ++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/match.c b/drivers/net/ethernet/netronome/nfp/flower/match.c
index 91935405f586..84f7a5dbea9d 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/match.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/match.c
@@ -123,6 +123,20 @@ nfp_flower_compile_mac(struct nfp_flower_mac_mpls *frame,
 			 NFP_FLOWER_MASK_MPLS_Q;
 
 		frame->mpls_lse = cpu_to_be32(t_mpls);
+	} else if (dissector_uses_key(flow->dissector,
+				      FLOW_DISSECTOR_KEY_BASIC)) {
+		/* Check for mpls ether type and set NFP_FLOWER_MASK_MPLS_Q
+		 * bit, which indicates an mpls ether type but without any
+		 * mpls fields.
+		 */
+		struct flow_dissector_key_basic *key_basic;
+
+		key_basic = skb_flow_dissector_target(flow->dissector,
+						      FLOW_DISSECTOR_KEY_BASIC,
+						      flow->key);
+		if (key_basic->n_proto == cpu_to_be16(ETH_P_MPLS_UC) ||
+		    key_basic->n_proto == cpu_to_be16(ETH_P_MPLS_MC))
+			frame->mpls_lse = cpu_to_be32(NFP_FLOWER_MASK_MPLS_Q);
 	}
 }
 
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index c42e64f32333..477f584f6d28 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -264,6 +264,14 @@ nfp_flower_calculate_key_layers(struct nfp_app *app,
 		case cpu_to_be16(ETH_P_ARP):
 			return -EOPNOTSUPP;
 
+		case cpu_to_be16(ETH_P_MPLS_UC):
+		case cpu_to_be16(ETH_P_MPLS_MC):
+			if (!(key_layer & NFP_FLOWER_LAYER_MAC)) {
+				key_layer |= NFP_FLOWER_LAYER_MAC;
+				key_size += sizeof(struct nfp_flower_mac_mpls);
+			}
+			break;
+
 		/* Will be included in layer 2. */
 		case cpu_to_be16(ETH_P_8021Q):
 			break;
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net 2/2] nfp: reject binding to shared blocks
  2018-06-25 20:22 [PATCH net 0/2] nfp: MPLS and shared blocks TC offload fixes Jakub Kicinski
  2018-06-25 20:22 ` [PATCH net 1/2] nfp: flower: fix mpls ether type detection Jakub Kicinski
@ 2018-06-25 20:22 ` Jakub Kicinski
  2018-06-25 20:40   ` Jiri Pirko
                     ` (2 more replies)
  1 sibling, 3 replies; 8+ messages in thread
From: Jakub Kicinski @ 2018-06-25 20:22 UTC (permalink / raw)
  To: davem; +Cc: oss-drivers, netdev, John Hurley, Jakub Kicinski

From: John Hurley <john.hurley@netronome.com>

TC shared blocks allow multiple qdiscs to be grouped together and filters
shared between them. Currently the chains of filters attached to a block
are only flushed when the block is removed. If a qdisc is removed from a
block but the block still exists, flow del messages are not passed to the
callback registered for that qdisc. For the NFP, this presents the
possibility of rules still existing in hw when they should be removed.

Prevent binding to shared blocks until the kernel can send per qdisc del
messages when block unbinds occur.

Fixes: 4861738775d7 ("net: sched: introduce shared filter blocks infrastructure")
Signed-off-by: John Hurley <john.hurley@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/main.c       | 3 +++
 drivers/net/ethernet/netronome/nfp/flower/offload.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index fcdfb8e7fdea..6b15e3b11956 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
@@ -202,6 +202,9 @@ static int nfp_bpf_setup_tc_block(struct net_device *netdev,
 	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
 		return -EOPNOTSUPP;
 
+	if (tcf_block_shared(f->block))
+		return -EOPNOTSUPP;
+
 	switch (f->command) {
 	case TC_BLOCK_BIND:
 		return tcf_block_cb_register(f->block,
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 477f584f6d28..525057bee0ed 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -631,6 +631,9 @@ static int nfp_flower_setup_tc_block(struct net_device *netdev,
 	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
 		return -EOPNOTSUPP;
 
+	if (tcf_block_shared(f->block))
+		return -EOPNOTSUPP;
+
 	switch (f->command) {
 	case TC_BLOCK_BIND:
 		return tcf_block_cb_register(f->block,
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net 2/2] nfp: reject binding to shared blocks
  2018-06-25 20:22 ` [PATCH net 2/2] nfp: reject binding to shared blocks Jakub Kicinski
@ 2018-06-25 20:40   ` Jiri Pirko
  2018-06-25 20:50     ` Jakub Kicinski
  2018-06-25 23:37   ` kbuild test robot
  2018-06-26  2:04   ` kbuild test robot
  2 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2018-06-25 20:40 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: davem, oss-drivers, netdev, John Hurley

Mon, Jun 25, 2018 at 10:22:46PM CEST, jakub.kicinski@netronome.com wrote:
>From: John Hurley <john.hurley@netronome.com>
>
>TC shared blocks allow multiple qdiscs to be grouped together and filters
>shared between them. Currently the chains of filters attached to a block
>are only flushed when the block is removed. If a qdisc is removed from a
>block but the block still exists, flow del messages are not passed to the
>callback registered for that qdisc. For the NFP, this presents the
>possibility of rules still existing in hw when they should be removed.
>
>Prevent binding to shared blocks until the kernel can send per qdisc del
>messages when block unbinds occur.

This is not nfp-specific problem. Should be handled differently. The
driver has information about offloaded filters. On unbind, it have
enough info to do the flush, doesn't it?


>
>Fixes: 4861738775d7 ("net: sched: introduce shared filter blocks infrastructure")
>Signed-off-by: John Hurley <john.hurley@netronome.com>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>Reviewed-by: Simon Horman <simon.horman@netronome.com>
>---
> drivers/net/ethernet/netronome/nfp/bpf/main.c       | 3 +++
> drivers/net/ethernet/netronome/nfp/flower/offload.c | 3 +++
> 2 files changed, 6 insertions(+)
>
>diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
>index fcdfb8e7fdea..6b15e3b11956 100644
>--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
>+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
>@@ -202,6 +202,9 @@ static int nfp_bpf_setup_tc_block(struct net_device *netdev,
> 	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
> 		return -EOPNOTSUPP;
> 
>+	if (tcf_block_shared(f->block))
>+		return -EOPNOTSUPP;
>+
> 	switch (f->command) {
> 	case TC_BLOCK_BIND:
> 		return tcf_block_cb_register(f->block,
>diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>index 477f584f6d28..525057bee0ed 100644
>--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
>+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>@@ -631,6 +631,9 @@ static int nfp_flower_setup_tc_block(struct net_device *netdev,
> 	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
> 		return -EOPNOTSUPP;
> 
>+	if (tcf_block_shared(f->block))
>+		return -EOPNOTSUPP;
>+
> 	switch (f->command) {
> 	case TC_BLOCK_BIND:
> 		return tcf_block_cb_register(f->block,
>-- 
>2.17.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 2/2] nfp: reject binding to shared blocks
  2018-06-25 20:40   ` Jiri Pirko
@ 2018-06-25 20:50     ` Jakub Kicinski
  2018-06-25 20:51       ` Jiri Pirko
  0 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2018-06-25 20:50 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: davem, oss-drivers, netdev, John Hurley

On Mon, 25 Jun 2018 22:40:21 +0200, Jiri Pirko wrote:
> Mon, Jun 25, 2018 at 10:22:46PM CEST, jakub.kicinski@netronome.com wrote:
> >From: John Hurley <john.hurley@netronome.com>
> >
> >TC shared blocks allow multiple qdiscs to be grouped together and filters
> >shared between them. Currently the chains of filters attached to a block
> >are only flushed when the block is removed. If a qdisc is removed from a
> >block but the block still exists, flow del messages are not passed to the
> >callback registered for that qdisc. For the NFP, this presents the
> >possibility of rules still existing in hw when they should be removed.
> >
> >Prevent binding to shared blocks until the kernel can send per qdisc del
> >messages when block unbinds occur.  
> 
> This is not nfp-specific problem. Should be handled differently. The
> driver has information about offloaded filters. On unbind, it have
> enough info to do the flush, doesn't it?

Certainly.  But this fix is simpler and sufficient.  We need to
backport it back to 4.16.  If we have to go through driver tables 
and flush filters we may as well merge the reoffload series to net.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 2/2] nfp: reject binding to shared blocks
  2018-06-25 20:50     ` Jakub Kicinski
@ 2018-06-25 20:51       ` Jiri Pirko
  0 siblings, 0 replies; 8+ messages in thread
From: Jiri Pirko @ 2018-06-25 20:51 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: davem, oss-drivers, netdev, John Hurley

Mon, Jun 25, 2018 at 10:50:14PM CEST, jakub.kicinski@netronome.com wrote:
>On Mon, 25 Jun 2018 22:40:21 +0200, Jiri Pirko wrote:
>> Mon, Jun 25, 2018 at 10:22:46PM CEST, jakub.kicinski@netronome.com wrote:
>> >From: John Hurley <john.hurley@netronome.com>
>> >
>> >TC shared blocks allow multiple qdiscs to be grouped together and filters
>> >shared between them. Currently the chains of filters attached to a block
>> >are only flushed when the block is removed. If a qdisc is removed from a
>> >block but the block still exists, flow del messages are not passed to the
>> >callback registered for that qdisc. For the NFP, this presents the
>> >possibility of rules still existing in hw when they should be removed.
>> >
>> >Prevent binding to shared blocks until the kernel can send per qdisc del
>> >messages when block unbinds occur.  
>> 
>> This is not nfp-specific problem. Should be handled differently. The
>> driver has information about offloaded filters. On unbind, it have
>> enough info to do the flush, doesn't it?
>
>Certainly.  But this fix is simpler and sufficient.  We need to
>backport it back to 4.16.  If we have to go through driver tables 
>and flush filters we may as well merge the reoffload series to net.

Oh, I missed this is for net. Sorry.

Acked-by: Jiri Pirko <jiri@mellanox.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 2/2] nfp: reject binding to shared blocks
  2018-06-25 20:22 ` [PATCH net 2/2] nfp: reject binding to shared blocks Jakub Kicinski
  2018-06-25 20:40   ` Jiri Pirko
@ 2018-06-25 23:37   ` kbuild test robot
  2018-06-26  2:04   ` kbuild test robot
  2 siblings, 0 replies; 8+ messages in thread
From: kbuild test robot @ 2018-06-25 23:37 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: kbuild-all, davem, oss-drivers, netdev, John Hurley,
	Jakub Kicinski

[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]

Hi John,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/nfp-MPLS-and-shared-blocks-TC-offload-fixes/20180626-042358
config: i386-randconfig-x001-201825 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/net/ethernet/netronome/nfp/flower/offload.c: In function 'nfp_flower_setup_tc_block':
>> drivers/net/ethernet/netronome/nfp/flower/offload.c:634:6: error: implicit declaration of function 'tcf_block_shared'; did you mean 'tcf_block_dev'? [-Werror=implicit-function-declaration]
     if (tcf_block_shared(f->block))
         ^~~~~~~~~~~~~~~~
         tcf_block_dev
   cc1: some warnings being treated as errors

vim +634 drivers/net/ethernet/netronome/nfp/flower/offload.c

   625	
   626	static int nfp_flower_setup_tc_block(struct net_device *netdev,
   627					     struct tc_block_offload *f)
   628	{
   629		struct nfp_repr *repr = netdev_priv(netdev);
   630	
   631		if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
   632			return -EOPNOTSUPP;
   633	
 > 634		if (tcf_block_shared(f->block))
   635			return -EOPNOTSUPP;
   636	
   637		switch (f->command) {
   638		case TC_BLOCK_BIND:
   639			return tcf_block_cb_register(f->block,
   640						     nfp_flower_setup_tc_block_cb,
   641						     repr, repr);
   642		case TC_BLOCK_UNBIND:
   643			tcf_block_cb_unregister(f->block,
   644						nfp_flower_setup_tc_block_cb,
   645						repr);
   646			return 0;
   647		default:
   648			return -EOPNOTSUPP;
   649		}
   650	}
   651	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33423 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net 2/2] nfp: reject binding to shared blocks
  2018-06-25 20:22 ` [PATCH net 2/2] nfp: reject binding to shared blocks Jakub Kicinski
  2018-06-25 20:40   ` Jiri Pirko
  2018-06-25 23:37   ` kbuild test robot
@ 2018-06-26  2:04   ` kbuild test robot
  2 siblings, 0 replies; 8+ messages in thread
From: kbuild test robot @ 2018-06-26  2:04 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: kbuild-all, davem, oss-drivers, netdev, John Hurley,
	Jakub Kicinski

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

Hi John,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/nfp-MPLS-and-shared-blocks-TC-offload-fixes/20180626-042358
config: x86_64-randconfig-u0-06260533 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/net/ethernet/netronome/nfp/bpf/main.c: In function 'nfp_bpf_setup_tc_block':
>> drivers/net/ethernet/netronome/nfp/bpf/main.c:205:6: error: implicit declaration of function 'tcf_block_shared' [-Werror=implicit-function-declaration]
     if (tcf_block_shared(f->block))
         ^
   cc1: some warnings being treated as errors

vim +/tcf_block_shared +205 drivers/net/ethernet/netronome/nfp/bpf/main.c

   196	
   197	static int nfp_bpf_setup_tc_block(struct net_device *netdev,
   198					  struct tc_block_offload *f)
   199	{
   200		struct nfp_net *nn = netdev_priv(netdev);
   201	
   202		if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
   203			return -EOPNOTSUPP;
   204	
 > 205		if (tcf_block_shared(f->block))
   206			return -EOPNOTSUPP;
   207	
   208		switch (f->command) {
   209		case TC_BLOCK_BIND:
   210			return tcf_block_cb_register(f->block,
   211						     nfp_bpf_setup_tc_block_cb,
   212						     nn, nn);
   213		case TC_BLOCK_UNBIND:
   214			tcf_block_cb_unregister(f->block,
   215						nfp_bpf_setup_tc_block_cb,
   216						nn);
   217			return 0;
   218		default:
   219			return -EOPNOTSUPP;
   220		}
   221	}
   222	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33599 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-06-26  2:05 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-25 20:22 [PATCH net 0/2] nfp: MPLS and shared blocks TC offload fixes Jakub Kicinski
2018-06-25 20:22 ` [PATCH net 1/2] nfp: flower: fix mpls ether type detection Jakub Kicinski
2018-06-25 20:22 ` [PATCH net 2/2] nfp: reject binding to shared blocks Jakub Kicinski
2018-06-25 20:40   ` Jiri Pirko
2018-06-25 20:50     ` Jakub Kicinski
2018-06-25 20:51       ` Jiri Pirko
2018-06-25 23:37   ` kbuild test robot
2018-06-26  2:04   ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).