[PATCH net-next 0/3,v2] flow_offload hardware priority fixes

[PATCH net-next 0/3,v2] flow_offload hardware priority fixes

[Pablo Neira Ayuso]

Hi,

This patchset contains three updates for the flow_offload users:

1) Pass major tc priority to drivers so they do not have to
   lshift it. This is a preparation patch for the fix coming in
   patch 3/3.

2) Add a new structure to basechain objects to wrap the offload
   data. This is another preparation patch for the fix coming in
   patch 3/3.

3) Allocate priority field per rule from the commit path and
   pass it on to drivers. Currently this is set to zero and
   some drivers bail out with it, start by priority number 1
   and allocate new priority from there on. The priority field
   is limited to 8-bits at this stage for simplicity.

v2: address Jakub comments to not use the netfilter basechain
    priority for this mapping.

Please, apply, thank you.

Pablo Neira Ayuso (3):
  net: sched: use major priority number as hardware priority
  netfilter: nf_tables_offload: add offload field to basechain
  filter: nf_tables_offload: set priority field for rules

 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c    |  2 +-
 drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c |  2 +-
 drivers/net/ethernet/mscc/ocelot_flower.c          | 12 ++------
 .../net/ethernet/netronome/nfp/flower/qos_conf.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c    |  2 +-
 include/net/netfilter/nf_tables.h                  | 14 ++++++---
 include/net/netfilter/nf_tables_offload.h          |  6 ++++
 include/net/pkt_cls.h                              |  2 +-
 net/netfilter/nf_tables_api.c                      |  2 +-
 net/netfilter/nf_tables_offload.c                  | 34 +++++++++++++++++-----
 10 files changed, 51 insertions(+), 27 deletions(-)

-- 
2.11.0

[PATCH net-next 1/3,v2] net: sched: use major priority number as hardware priority

[Pablo Neira Ayuso]

tc transparently maps the software priority number to hardware. Update
it to pass the major priority which is what most drivers expect. Update
drivers too so they do not need to lshift the priority field of the
flow_cls_common_offload object. The stmmac driver is an exception, since
this code assumes the tc software priority is fine, therefore, lshift it
just to be conservative.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: no changes.

 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c      |  2 +-
 drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c   |  2 +-
 drivers/net/ethernet/mscc/ocelot_flower.c            | 12 +++---------
 drivers/net/ethernet/netronome/nfp/flower/qos_conf.c |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c      |  2 +-
 include/net/pkt_cls.h                                |  2 +-
 6 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index cc096f6011d9..744c0c640c10 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -3158,7 +3158,7 @@ mlx5e_flow_esw_attr_init(struct mlx5_esw_flow_attr *esw_attr,
 
 	esw_attr->parse_attr = parse_attr;
 	esw_attr->chain = f->common.chain_index;
-	esw_attr->prio = TC_H_MAJ(f->common.prio) >> 16;
+	esw_attr->prio = f->common.prio;
 
 	esw_attr->in_rep = in_rep;
 	esw_attr->in_mdev = in_mdev;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
index e8ac90564dbe..84a87d059333 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
@@ -471,7 +471,7 @@ int mlxsw_sp_acl_rulei_commit(struct mlxsw_sp_acl_rule_info *rulei)
 void mlxsw_sp_acl_rulei_priority(struct mlxsw_sp_acl_rule_info *rulei,
 				 unsigned int priority)
 {
-	rulei->priority = priority >> 16;
+	rulei->priority = priority;
 }
 
 void mlxsw_sp_acl_rulei_keymask_u32(struct mlxsw_sp_acl_rule_info *rulei,
diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 59487d446a09..b894bc0c9c16 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -13,12 +13,6 @@ struct ocelot_port_block {
 	struct ocelot_port *port;
 };
 
-static u16 get_prio(u32 prio)
-{
-	/* prio starts from 0x1000 while the ids starts from 0 */
-	return prio >> 16;
-}
-
 static int ocelot_flower_parse_action(struct flow_cls_offload *f,
 				      struct ocelot_ace_rule *rule)
 {
@@ -168,7 +162,7 @@ static int ocelot_flower_parse(struct flow_cls_offload *f,
 	}
 
 finished_key_parsing:
-	ocelot_rule->prio = get_prio(f->common.prio);
+	ocelot_rule->prio = f->common.prio;
 	ocelot_rule->id = f->cookie;
 	return ocelot_flower_parse_action(f, ocelot_rule);
 }
@@ -218,7 +212,7 @@ static int ocelot_flower_destroy(struct flow_cls_offload *f,
 	struct ocelot_ace_rule rule;
 	int ret;
 
-	rule.prio = get_prio(f->common.prio);
+	rule.prio = f->common.prio;
 	rule.port = port_block->port;
 	rule.id = f->cookie;
 
@@ -236,7 +230,7 @@ static int ocelot_flower_stats_update(struct flow_cls_offload *f,
 	struct ocelot_ace_rule rule;
 	int ret;
 
-	rule.prio = get_prio(f->common.prio);
+	rule.prio = f->common.prio;
 	rule.port = port_block->port;
 	rule.id = f->cookie;
 	ret = ocelot_ace_rule_stats_update(&rule);
diff --git a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
index 86e968cd5ffd..124a43dc136a 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
@@ -93,7 +93,7 @@ nfp_flower_install_rate_limiter(struct nfp_app *app, struct net_device *netdev,
 		return -EOPNOTSUPP;
 	}
 
-	if (flow->common.prio != (1 << 16)) {
+	if (flow->common.prio != 1) {
 		NL_SET_ERR_MSG_MOD(extack, "unsupported offload: qos rate limit offload requires highest priority");
 		return -EOPNOTSUPP;
 	}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 58ea18af9813..5cd040215469 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -94,7 +94,7 @@ static int tc_fill_entry(struct stmmac_priv *priv,
 	struct stmmac_tc_entry *entry, *frag = NULL;
 	struct tc_u32_sel *sel = cls->knode.sel;
 	u32 off, data, mask, real_off, rem;
-	u32 prio = cls->common.prio;
+	u32 prio = cls->common.prio << 16;
 	int ret;
 
 	/* Only 1 match per entry */
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index e429809ca90d..98be18ef1ed3 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -646,7 +646,7 @@ tc_cls_common_offload_init(struct flow_cls_common_offload *cls_common,
 {
 	cls_common->chain_index = tp->chain->index;
 	cls_common->protocol = tp->protocol;
-	cls_common->prio = tp->prio;
+	cls_common->prio = tp->prio >> 16;
 	if (tc_skip_sw(flags) || flags & TCA_CLS_FLAGS_VERBOSE)
 		cls_common->extack = extack;
 }
-- 
2.11.0

Re: [PATCH net-next 1/3,v2] net: sched: use major priority number as hardware priority

[Jiri Pirko]

Fri, Aug 02, 2019 at 03:28:44PM CEST, pablo@netfilter.org wrote:
>tc transparently maps the software priority number to hardware. Update
>it to pass the major priority which is what most drivers expect. Update
>drivers too so they do not need to lshift the priority field of the
>flow_cls_common_offload object. The stmmac driver is an exception, since
>this code assumes the tc software priority is fine, therefore, lshift it
>just to be conservative.
>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

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

[PATCH net-next 2/3] netfilter: nf_tables_offload: add offload field to basechain

[Pablo Neira Ayuso]

Wrap offload objects in struct nft_base_chain around structure.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v1: initial patch version.

 include/net/netfilter/nf_tables.h         | 6 ++++--
 include/net/netfilter/nf_tables_offload.h | 5 +++++
 net/netfilter/nf_tables_api.c             | 2 +-
 net/netfilter/nf_tables_offload.c         | 7 ++++---
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 9b624566b82d..87dbe62c0f27 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -952,7 +952,7 @@ struct nft_stats {
  *	@stats: per-cpu chain stats
  *	@chain: the chain
  *	@dev_name: device name that this base chain is attached to (if any)
- *	@flow_block: flow block (for hardware offload)
+ *	@offload: hardware offload data
  */
 struct nft_base_chain {
 	struct nf_hook_ops		ops;
@@ -962,7 +962,9 @@ struct nft_base_chain {
 	struct nft_stats __percpu	*stats;
 	struct nft_chain		chain;
 	char 				dev_name[IFNAMSIZ];
-	struct flow_block		flow_block;
+	struct {
+		struct flow_block	flow_block;
+	} offload;
 };
 
 static inline struct nft_base_chain *nft_base_chain(const struct nft_chain *chain)
diff --git a/include/net/netfilter/nf_tables_offload.h b/include/net/netfilter/nf_tables_offload.h
index 3196663a10e3..fb3db391ade8 100644
--- a/include/net/netfilter/nf_tables_offload.h
+++ b/include/net/netfilter/nf_tables_offload.h
@@ -73,4 +73,9 @@ int nft_flow_rule_offload_commit(struct net *net);
 	(__reg)->key		= __key;				\
 	memset(&(__reg)->mask, 0xff, (__reg)->len);
 
+static inline void nft_basechain_offload_init(struct nft_base_chain *basechain)
+{
+	flow_block_init(&basechain->offload.flow_block);
+}
+
 #endif
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 605a7cfe7ca7..a07d764c3555 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1662,7 +1662,7 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 
 		chain->flags |= NFT_BASE_CHAIN | flags;
 		basechain->policy = NF_ACCEPT;
-		flow_block_init(&basechain->flow_block);
+		nft_basechain_offload_init(basechain);
 	} else {
 		chain = kzalloc(sizeof(*chain), GFP_KERNEL);
 		if (chain == NULL)
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index 64f5fd5f240e..84615381b06f 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -113,10 +113,11 @@ static void nft_flow_offload_common_init(struct flow_cls_common_offload *common,
 static int nft_setup_cb_call(struct nft_base_chain *basechain,
 			     enum tc_setup_type type, void *type_data)
 {
+	struct flow_block *flow_block = &basechain->offload.flow_block;
 	struct flow_block_cb *block_cb;
 	int err;
 
-	list_for_each_entry(block_cb, &basechain->flow_block.cb_list, list) {
+	list_for_each_entry(block_cb, &flow_block->cb_list, list) {
 		err = block_cb->cb(type, type_data, block_cb->cb_priv);
 		if (err < 0)
 			return err;
@@ -154,7 +155,7 @@ static int nft_flow_offload_rule(struct nft_trans *trans,
 static int nft_flow_offload_bind(struct flow_block_offload *bo,
 				 struct nft_base_chain *basechain)
 {
-	list_splice(&bo->cb_list, &basechain->flow_block.cb_list);
+	list_splice(&bo->cb_list, &basechain->offload.flow_block.cb_list);
 	return 0;
 }
 
@@ -198,7 +199,7 @@ static int nft_flow_offload_chain(struct nft_trans *trans,
 		return -EOPNOTSUPP;
 
 	bo.command = cmd;
-	bo.block = &basechain->flow_block;
+	bo.block = &basechain->offload.flow_block;
 	bo.binder_type = FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS;
 	bo.extack = &extack;
 	INIT_LIST_HEAD(&bo.cb_list);
-- 
2.11.0

[PATCH net-next 3/3] filter: nf_tables_offload: set priority field for rules

[Pablo Neira Ayuso]

This patch allocates the priority per rule starting from priority 1
since some drivers assume priority 0 never happens.

This patch is restricting the rule priority range to 8-bit integer since
the nft_rule object has 7-bit spare bits plus one that is scratched from
the handle. It should be possible to extend this later on by placing the
priority after the userdata area to turn this into 32-bits priority
field, to put this data away from the packet path cachelines.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v1: formerly "netfilter: nf_tables: map basechain priority to hardware priority"
    address mapping to hardware based on comments from Jakub.

 include/net/netfilter/nf_tables.h         |  8 ++++++--
 include/net/netfilter/nf_tables_offload.h |  1 +
 net/netfilter/nf_tables_offload.c         | 27 ++++++++++++++++++++++-----
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 87dbe62c0f27..a6308fcf5bf0 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -9,6 +9,7 @@
 #include <linux/netfilter/nf_tables.h>
 #include <linux/u64_stats_sync.h>
 #include <linux/rhashtable.h>
+#include <linux/idr.h>
 #include <net/netfilter/nf_flow_table.h>
 #include <net/netlink.h>
 #include <net/flow_offload.h>
@@ -824,14 +825,16 @@ int nft_expr_dump(struct sk_buff *skb, unsigned int attr,
  *	@genmask: generation mask
  *	@dlen: length of expression data
  *	@udata: user data is appended to the rule
+ *	@prio: priority (for hardware offload)
  *	@data: expression data
  */
 struct nft_rule {
 	struct list_head		list;
-	u64				handle:42,
+	u64				handle:41,
 					genmask:2,
 					dlen:12,
-					udata:1;
+					udata:1,
+					prio:8;
 	unsigned char			data[]
 		__attribute__((aligned(__alignof__(struct nft_expr))));
 };
@@ -964,6 +967,7 @@ struct nft_base_chain {
 	char 				dev_name[IFNAMSIZ];
 	struct {
 		struct flow_block	flow_block;
+		struct idr		prio_idr;
 	} offload;
 };
 
diff --git a/include/net/netfilter/nf_tables_offload.h b/include/net/netfilter/nf_tables_offload.h
index fb3db391ade8..70f226568fe7 100644
--- a/include/net/netfilter/nf_tables_offload.h
+++ b/include/net/netfilter/nf_tables_offload.h
@@ -76,6 +76,7 @@ int nft_flow_rule_offload_commit(struct net *net);
 static inline void nft_basechain_offload_init(struct nft_base_chain *basechain)
 {
 	flow_block_init(&basechain->offload.flow_block);
+	idr_init(&basechain->offload.prio_idr);
 }
 
 #endif
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index 84615381b06f..21144938482a 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -103,10 +103,11 @@ void nft_offload_update_dependency(struct nft_offload_ctx *ctx,
 }
 
 static void nft_flow_offload_common_init(struct flow_cls_common_offload *common,
-					 __be16 proto,
-					struct netlink_ext_ack *extack)
+					 __be16 proto, u32 priority,
+					 struct netlink_ext_ack *extack)
 {
 	common->protocol = proto;
+	common->prio = priority;
 	common->extack = extack;
 }
 
@@ -125,6 +126,8 @@ static int nft_setup_cb_call(struct nft_base_chain *basechain,
 	return 0;
 }
 
+#define NFT_OFFLOAD_PRIO_MAX	U8_MAX
+
 static int nft_flow_offload_rule(struct nft_trans *trans,
 				 enum flow_cls_command command)
 {
@@ -134,22 +137,36 @@ static int nft_flow_offload_rule(struct nft_trans *trans,
 	struct nft_base_chain *basechain;
 	struct netlink_ext_ack extack;
 	__be16 proto = ETH_P_ALL;
+	u32 prio = 1;
+	int err;
 
 	if (!nft_is_base_chain(trans->ctx.chain))
 		return -EOPNOTSUPP;
 
 	basechain = nft_base_chain(trans->ctx.chain);
 
-	if (flow)
+	if (flow) {
+		if (idr_alloc_u32(&basechain->offload.prio_idr, NULL, &prio,
+				  NFT_OFFLOAD_PRIO_MAX, GFP_KERNEL) < 0)
+			return -E2BIG;
+
+		rule->prio = prio;
 		proto = flow->proto;
+	}
 
-	nft_flow_offload_common_init(&cls_flow.common, proto, &extack);
+	nft_flow_offload_common_init(&cls_flow.common, proto, rule->prio,
+				     &extack);
 	cls_flow.command = command;
 	cls_flow.cookie = (unsigned long) rule;
 	if (flow)
 		cls_flow.rule = flow->rule;
 
-	return nft_setup_cb_call(basechain, TC_SETUP_CLSFLOWER, &cls_flow);
+	err = nft_setup_cb_call(basechain, TC_SETUP_CLSFLOWER, &cls_flow);
+	if ((err < 0 && command == FLOW_CLS_REPLACE) ||
+	    (err == 0 && command == FLOW_CLS_DESTROY))
+		idr_remove(&basechain->offload.prio_idr, rule->prio);
+
+	return err;
 }
 
 static int nft_flow_offload_bind(struct flow_block_offload *bo,
-- 
2.11.0

Re: [PATCH net-next 0/3,v2] flow_offload hardware priority fixes

[Jakub Kicinski]

On Fri,  2 Aug 2019 15:28:43 +0200, Pablo Neira Ayuso wrote:
> v2: address Jakub comments to not use the netfilter basechain
>     priority for this mapping.

Hardly.