[PATCH net-next 0/6] nfp: flow-independent tc action hardware offload

[PATCH net-next 0/6] nfp: flow-independent tc action hardware offload

[Simon Horman]

Baowen Zheng says:

Allow nfp NIC to offload tc actions independent of flows.

The motivation for this work is to offload tc actions independent of flows
for nfp NIC. We allow nfp driver to provide hardware offload of OVS
metering feature - which calls for policers that may be used by multiple
flows and whose lifecycle is independent of any flows that use them.

When nfp driver tries to offload a flow table using the independent action,
the driver will search if the action is already offloaded to the hardware.
If not, the flow table offload will fail.

When the nfp NIC successes to offload an action, the user can check
in_hw_count when dumping the tc action.

Tc cli command to offload and dump an action:

 # tc actions add action police rate 100mbit burst 10000k index 200 skip_sw

 # tc -s -d actions list action police

 total acts 1

      action order 0:  police 0xc8 rate 100Mbit burst 10000Kb mtu 2Kb action reclassify
      overhead 0b linklayer ethernet
      ref 1 bind 0  installed 142 sec used 0 sec
      Action statistics:
      Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
      backlog 0b 0p requeues 0
      skip_sw in_hw in_hw_count 1
      used_hw_stats delayed

Baowen Zheng (6):
  nfp: refactor policer config to support ingress/egress meter
  nfp: add support to offload tc action to hardware
  nfp: add hash table to store meter table
  nfp: add process to get action stats from hardware
  nfp: add support to offload police action from flower table
  nfp: add NFP_FL_FEATS_QOS_METER to host features to enable meter
    offload

 .../ethernet/netronome/nfp/flower/action.c    |  58 +++
 .../net/ethernet/netronome/nfp/flower/cmsg.h  |   7 +
 .../net/ethernet/netronome/nfp/flower/main.h  |  50 +-
 .../ethernet/netronome/nfp/flower/offload.c   |  16 +-
 .../ethernet/netronome/nfp/flower/qos_conf.c  | 440 +++++++++++++++++-
 5 files changed, 545 insertions(+), 26 deletions(-)

-- 
2.20.1

[PATCH net-next 1/6] nfp: refactor policer config to support ingress/egress meter

[Simon Horman]

From: Baowen Zheng <baowen.zheng@corigine.com>

Add an policer API to support ingress/egress meter.

Change ingress police to compatible with the new API.

Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../net/ethernet/netronome/nfp/flower/main.h  |  2 +
 .../ethernet/netronome/nfp/flower/qos_conf.c  | 74 ++++++++++++++-----
 2 files changed, 56 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index 917c450a7aad..7720403e79fb 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -569,4 +569,6 @@ nfp_flower_xmit_flow(struct nfp_app *app, struct nfp_fl_payload *nfp_flow,
 void
 nfp_flower_update_merge_stats(struct nfp_app *app,
 			      struct nfp_fl_payload *sub_flow);
+int nfp_flower_offload_one_police(struct nfp_app *app, bool ingress,
+				  bool pps, u32 id, u32 rate, u32 burst);
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
index 784c6dbf8bc4..68a92a28d7fa 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
@@ -11,10 +11,14 @@
 
 #define NFP_FL_QOS_UPDATE		msecs_to_jiffies(1000)
 #define NFP_FL_QOS_PPS  BIT(15)
+#define NFP_FL_QOS_METER  BIT(10)
 
 struct nfp_police_cfg_head {
 	__be32 flags_opts;
-	__be32 port;
+	union {
+		__be32 meter_id;
+		__be32 port;
+	};
 };
 
 enum NFP_FL_QOS_TYPES {
@@ -46,7 +50,15 @@ enum NFP_FL_QOS_TYPES {
  * |                    Committed Information Rate                 |
  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  * Word[0](FLag options):
- * [15] p(pps) 1 for pps ,0 for bps
+ * [15] p(pps) 1 for pps, 0 for bps
+ *
+ * Meter control message
+ *  1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
+ * +-------------------------------+-+---+-----+-+---------+-+---+-+
+ * |            Reserved           |p| Y |TYPE |E|TSHFV    |P| PC|R|
+ * +-------------------------------+-+---+-----+-+---------+-+---+-+
+ * |                            meter ID                           |
+ * +-------------------------------+-------------------------------+
  *
  */
 struct nfp_police_config {
@@ -67,6 +79,40 @@ struct nfp_police_stats_reply {
 	__be64 drop_pkts;
 };
 
+int nfp_flower_offload_one_police(struct nfp_app *app, bool ingress,
+				  bool pps, u32 id, u32 rate, u32 burst)
+{
+	struct nfp_police_config *config;
+	struct sk_buff *skb;
+
+	skb = nfp_flower_cmsg_alloc(app, sizeof(struct nfp_police_config),
+				    NFP_FLOWER_CMSG_TYPE_QOS_MOD, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	config = nfp_flower_cmsg_get_data(skb);
+	memset(config, 0, sizeof(struct nfp_police_config));
+	if (pps)
+		config->head.flags_opts |= cpu_to_be32(NFP_FL_QOS_PPS);
+	if (!ingress)
+		config->head.flags_opts |= cpu_to_be32(NFP_FL_QOS_METER);
+
+	if (ingress)
+		config->head.port = cpu_to_be32(id);
+	else
+		config->head.meter_id = cpu_to_be32(id);
+
+	config->bkt_tkn_p = cpu_to_be32(burst);
+	config->bkt_tkn_c = cpu_to_be32(burst);
+	config->pbs = cpu_to_be32(burst);
+	config->cbs = cpu_to_be32(burst);
+	config->pir = cpu_to_be32(rate);
+	config->cir = cpu_to_be32(rate);
+	nfp_ctrl_tx(app->ctrl, skb);
+
+	return 0;
+}
+
 static int
 nfp_flower_install_rate_limiter(struct nfp_app *app, struct net_device *netdev,
 				struct tc_cls_matchall_offload *flow,
@@ -77,14 +123,13 @@ nfp_flower_install_rate_limiter(struct nfp_app *app, struct net_device *netdev,
 	struct nfp_flower_priv *fl_priv = app->priv;
 	struct flow_action_entry *action = NULL;
 	struct nfp_flower_repr_priv *repr_priv;
-	struct nfp_police_config *config;
 	u32 netdev_port_id, i;
 	struct nfp_repr *repr;
-	struct sk_buff *skb;
 	bool pps_support;
 	u32 bps_num = 0;
 	u32 pps_num = 0;
 	u32 burst;
+	bool pps;
 	u64 rate;
 
 	if (!nfp_netdev_is_nfp_repr(netdev)) {
@@ -169,23 +214,12 @@ nfp_flower_install_rate_limiter(struct nfp_app *app, struct net_device *netdev,
 		}
 
 		if (rate != 0) {
-			skb = nfp_flower_cmsg_alloc(repr->app, sizeof(struct nfp_police_config),
-						    NFP_FLOWER_CMSG_TYPE_QOS_MOD, GFP_KERNEL);
-			if (!skb)
-				return -ENOMEM;
-
-			config = nfp_flower_cmsg_get_data(skb);
-			memset(config, 0, sizeof(struct nfp_police_config));
+			pps = false;
 			if (action->police.rate_pkt_ps > 0)
-				config->head.flags_opts = cpu_to_be32(NFP_FL_QOS_PPS);
-			config->head.port = cpu_to_be32(netdev_port_id);
-			config->bkt_tkn_p = cpu_to_be32(burst);
-			config->bkt_tkn_c = cpu_to_be32(burst);
-			config->pbs = cpu_to_be32(burst);
-			config->cbs = cpu_to_be32(burst);
-			config->pir = cpu_to_be32(rate);
-			config->cir = cpu_to_be32(rate);
-			nfp_ctrl_tx(repr->app->ctrl, skb);
+				pps = true;
+			nfp_flower_offload_one_police(repr->app, true,
+						      pps, netdev_port_id,
+						      rate, burst);
 		}
 	}
 	repr_priv->qos_table.netdev_port_id = netdev_port_id;
-- 
2.20.1

[PATCH net-next 2/6] nfp: add support to offload tc action to hardware

[Simon Horman]

From: Baowen Zheng <baowen.zheng@corigine.com>

Add process to offload tc action to hardware.

Currently we only support to offload police action.

Add meter capability to check if firmware supports
meter offload.

Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../net/ethernet/netronome/nfp/flower/main.h  |   6 +
 .../ethernet/netronome/nfp/flower/offload.c   |  16 ++-
 .../ethernet/netronome/nfp/flower/qos_conf.c  | 103 ++++++++++++++++++
 3 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index 7720403e79fb..a880f7684600 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -12,7 +12,9 @@
 #include <linux/rhashtable.h>
 #include <linux/time64.h>
 #include <linux/types.h>
+#include <net/flow_offload.h>
 #include <net/pkt_cls.h>
+#include <net/pkt_sched.h>
 #include <net/tcp.h>
 #include <linux/workqueue.h>
 #include <linux/idr.h>
@@ -48,6 +50,7 @@ struct nfp_app;
 #define NFP_FL_FEATS_IPV6_TUN		BIT(7)
 #define NFP_FL_FEATS_VLAN_QINQ		BIT(8)
 #define NFP_FL_FEATS_QOS_PPS		BIT(9)
+#define NFP_FL_FEATS_QOS_METER		BIT(10)
 #define NFP_FL_FEATS_HOST_ACK		BIT(31)
 
 #define NFP_FL_ENABLE_FLOW_MERGE	BIT(0)
@@ -569,6 +572,9 @@ nfp_flower_xmit_flow(struct nfp_app *app, struct nfp_fl_payload *nfp_flow,
 void
 nfp_flower_update_merge_stats(struct nfp_app *app,
 			      struct nfp_fl_payload *sub_flow);
+
+int nfp_setup_tc_act_offload(struct nfp_app *app,
+			     struct flow_offload_action *fl_act);
 int nfp_flower_offload_one_police(struct nfp_app *app, bool ingress,
 				  bool pps, u32 id, u32 rate, u32 burst);
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index f97eff5afd12..92e8ade4854e 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -1861,6 +1861,20 @@ nfp_flower_setup_indr_tc_block(struct net_device *netdev, struct Qdisc *sch, str
 	return 0;
 }
 
+static int
+nfp_setup_tc_no_dev(struct nfp_app *app, enum tc_setup_type type, void *data)
+{
+	if (!data)
+		return -EOPNOTSUPP;
+
+	switch (type) {
+	case TC_SETUP_ACT:
+		return nfp_setup_tc_act_offload(app, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 int
 nfp_flower_indr_setup_tc_cb(struct net_device *netdev, struct Qdisc *sch, void *cb_priv,
 			    enum tc_setup_type type, void *type_data,
@@ -1868,7 +1882,7 @@ nfp_flower_indr_setup_tc_cb(struct net_device *netdev, struct Qdisc *sch, void *
 			    void (*cleanup)(struct flow_block_cb *block_cb))
 {
 	if (!netdev)
-		return -EOPNOTSUPP;
+		return nfp_setup_tc_no_dev(cb_priv, type, data);
 
 	if (!nfp_fl_is_netdev_to_offload(netdev))
 		return -EOPNOTSUPP;
diff --git a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
index 68a92a28d7fa..3ec63217fb19 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
@@ -475,3 +475,106 @@ int nfp_flower_setup_qos_offload(struct nfp_app *app, struct net_device *netdev,
 		return -EOPNOTSUPP;
 	}
 }
+
+/* offload tc action, currently only for tc police */
+
+static int
+nfp_act_install_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
+			struct netlink_ext_ack *extack)
+{
+	struct flow_action_entry *paction = &fl_act->action.entries[0];
+	u32 action_num = fl_act->action.num_entries;
+	struct nfp_flower_priv *fl_priv = app->priv;
+	struct flow_action_entry *action = NULL;
+	u32 burst, i, meter_id;
+	bool pps_support, pps;
+	bool add = false;
+	u64 rate;
+
+	pps_support = !!(fl_priv->flower_ext_feats & NFP_FL_FEATS_QOS_PPS);
+
+	for (i = 0 ; i < action_num; i++) {
+		/*set qos associate data for this interface */
+		action = paction + i;
+		if (action->id != FLOW_ACTION_POLICE) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "unsupported offload: qos rate limit offload requires police action");
+			continue;
+		}
+		if (action->police.rate_bytes_ps > 0) {
+			rate = action->police.rate_bytes_ps;
+			burst = action->police.burst;
+		} else if (action->police.rate_pkt_ps > 0 && pps_support) {
+			rate = action->police.rate_pkt_ps;
+			burst = action->police.burst_pkt;
+		} else {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "unsupported offload: unsupported qos rate limit");
+			continue;
+		}
+
+		if (rate != 0) {
+			pps = false;
+			if (action->police.rate_pkt_ps > 0)
+				pps = true;
+			meter_id = action->hw_index;
+			nfp_flower_offload_one_police(app, false, pps, meter_id,
+						      rate, burst);
+			add = true;
+		}
+	}
+
+	if (add)
+		return 0;
+	else
+		return -EOPNOTSUPP;
+}
+
+static int
+nfp_act_remove_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
+		       struct netlink_ext_ack *extack)
+{
+	struct nfp_police_config *config;
+	struct sk_buff *skb;
+	u32 meter_id;
+
+	/*delete qos associate data for this interface */
+	if (fl_act->id != FLOW_ACTION_POLICE) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "unsupported offload: qos rate limit offload requires police action");
+		return -EOPNOTSUPP;
+	}
+
+	meter_id = fl_act->index;
+	skb = nfp_flower_cmsg_alloc(app, sizeof(struct nfp_police_config),
+				    NFP_FLOWER_CMSG_TYPE_QOS_DEL, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	config = nfp_flower_cmsg_get_data(skb);
+	memset(config, 0, sizeof(struct nfp_police_config));
+	config->head.flags_opts = cpu_to_be32(NFP_FL_QOS_METER);
+	config->head.meter_id = cpu_to_be32(meter_id);
+	nfp_ctrl_tx(app->ctrl, skb);
+
+	return 0;
+}
+
+int nfp_setup_tc_act_offload(struct nfp_app *app,
+			     struct flow_offload_action *fl_act)
+{
+	struct netlink_ext_ack *extack = fl_act->extack;
+	struct nfp_flower_priv *fl_priv = app->priv;
+
+	if (!(fl_priv->flower_ext_feats & NFP_FL_FEATS_QOS_METER))
+		return -EOPNOTSUPP;
+
+	switch (fl_act->command) {
+	case FLOW_ACT_REPLACE:
+		return nfp_act_install_actions(app, fl_act, extack);
+	case FLOW_ACT_DESTROY:
+		return nfp_act_remove_actions(app, fl_act, extack);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
-- 
2.20.1

[PATCH net-next 3/6] nfp: add hash table to store meter table

[Simon Horman]

From: Baowen Zheng <baowen.zheng@corigine.com>

Add a hash table to store meter table.

This meter table will also be used by flower action.

Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../net/ethernet/netronome/nfp/flower/main.h  |  36 +++++
 .../ethernet/netronome/nfp/flower/qos_conf.c  | 148 +++++++++++++++++-
 2 files changed, 183 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index a880f7684600..0c28e3414b7f 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -194,6 +194,8 @@ struct nfp_fl_internal_ports {
  * @qos_stats_work:	Workqueue for qos stats processing
  * @qos_rate_limiters:	Current active qos rate limiters
  * @qos_stats_lock:	Lock on qos stats updates
+ * @meter_stats_lock:   Lock on meter stats updates
+ * @meter_table:	Hash table used to store the meter table
  * @pre_tun_rule_cnt:	Number of pre-tunnel rules offloaded
  * @merge_table:	Hash table to store merged flows
  * @ct_zone_table:	Hash table used to store the different zones
@@ -231,6 +233,8 @@ struct nfp_flower_priv {
 	struct delayed_work qos_stats_work;
 	unsigned int qos_rate_limiters;
 	spinlock_t qos_stats_lock; /* Protect the qos stats */
+	struct mutex meter_stats_lock; /* Protect the meter stats */
+	struct rhashtable meter_table;
 	int pre_tun_rule_cnt;
 	struct rhashtable merge_table;
 	struct rhashtable ct_zone_table;
@@ -377,6 +381,32 @@ struct nfp_fl_stats_frame {
 	__be64 stats_cookie;
 };
 
+struct nfp_meter_stats_entry {
+	u64 pkts;
+	u64 bytes;
+	u64 drops;
+};
+
+struct nfp_meter_entry {
+	struct rhash_head ht_node;
+	u32 meter_id;
+	bool bps;
+	u32 rate;
+	u32 burst;
+	u64 used;
+	struct nfp_meter_stats {
+		u64 update;
+		struct nfp_meter_stats_entry curr;
+		struct nfp_meter_stats_entry prev;
+	} stats;
+};
+
+enum nfp_meter_op {
+	NFP_METER_ADD,
+	NFP_METER_SET,
+	NFP_METER_DEL,
+};
+
 static inline bool
 nfp_flower_internal_port_can_offload(struct nfp_app *app,
 				     struct net_device *netdev)
@@ -575,6 +605,12 @@ nfp_flower_update_merge_stats(struct nfp_app *app,
 
 int nfp_setup_tc_act_offload(struct nfp_app *app,
 			     struct flow_offload_action *fl_act);
+int nfp_init_meter_table(struct nfp_app *app);
+
 int nfp_flower_offload_one_police(struct nfp_app *app, bool ingress,
 				  bool pps, u32 id, u32 rate, u32 burst);
+int nfp_flower_setup_meter_entry(struct nfp_app *app,
+				 const struct flow_action_entry *action,
+				 enum nfp_meter_op op,
+				 u32 meter_id);
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
index 3ec63217fb19..f9f9e506b303 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
@@ -1,7 +1,11 @@
 // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
 /* Copyright (C) 2019 Netronome Systems, Inc. */
 
+#include <linux/hash.h>
+#include <linux/hashtable.h>
+#include <linux/jhash.h>
 #include <linux/math64.h>
+#include <linux/vmalloc.h>
 #include <net/pkt_cls.h>
 #include <net/pkt_sched.h>
 
@@ -440,6 +444,9 @@ void nfp_flower_qos_init(struct nfp_app *app)
 	struct nfp_flower_priv *fl_priv = app->priv;
 
 	spin_lock_init(&fl_priv->qos_stats_lock);
+	mutex_init(&fl_priv->meter_stats_lock);
+	nfp_init_meter_table(app);
+
 	INIT_DELAYED_WORK(&fl_priv->qos_stats_work, &update_stats_cache);
 }
 
@@ -478,6 +485,128 @@ int nfp_flower_setup_qos_offload(struct nfp_app *app, struct net_device *netdev,
 
 /* offload tc action, currently only for tc police */
 
+static const struct rhashtable_params stats_meter_table_params = {
+	.key_offset	= offsetof(struct nfp_meter_entry, meter_id),
+	.head_offset	= offsetof(struct nfp_meter_entry, ht_node),
+	.key_len	= sizeof(u32),
+};
+
+static struct nfp_meter_entry *
+nfp_flower_search_meter_entry(struct nfp_app *app, u32 meter_id)
+{
+	struct nfp_flower_priv *priv = app->priv;
+
+	return rhashtable_lookup_fast(&priv->meter_table, &meter_id,
+				      stats_meter_table_params);
+}
+
+static struct nfp_meter_entry *
+nfp_flower_add_meter_entry(struct nfp_app *app, u32 meter_id)
+{
+	struct nfp_meter_entry *meter_entry = NULL;
+	struct nfp_flower_priv *priv = app->priv;
+
+	meter_entry = rhashtable_lookup_fast(&priv->meter_table,
+					     &meter_id,
+					     stats_meter_table_params);
+
+	if (meter_entry)
+		return meter_entry;
+
+	meter_entry = kzalloc(sizeof(*meter_entry), GFP_ATOMIC);
+	if (!meter_entry)
+		goto err;
+
+	meter_entry->meter_id = meter_id;
+	meter_entry->used = jiffies;
+	if (rhashtable_insert_fast(&priv->meter_table, &meter_entry->ht_node,
+				   stats_meter_table_params)) {
+		goto err_free_meter_entry;
+	}
+	priv->qos_rate_limiters++;
+	if (priv->qos_rate_limiters == 1)
+		schedule_delayed_work(&priv->qos_stats_work,
+				      NFP_FL_QOS_UPDATE);
+	return meter_entry;
+
+err_free_meter_entry:
+	kfree(meter_entry);
+err:
+	return NULL;
+}
+
+static void nfp_flower_del_meter_entry(struct nfp_app *app, u32 meter_id)
+{
+	struct nfp_meter_entry *meter_entry = NULL;
+	struct nfp_flower_priv *priv = app->priv;
+
+	meter_entry = rhashtable_lookup_fast(&priv->meter_table, &meter_id,
+					     stats_meter_table_params);
+
+	if (meter_entry) {
+		rhashtable_remove_fast(&priv->meter_table,
+				       &meter_entry->ht_node,
+				       stats_meter_table_params);
+		kfree(meter_entry);
+		priv->qos_rate_limiters--;
+		if (!priv->qos_rate_limiters)
+			cancel_delayed_work_sync(&priv->qos_stats_work);
+	}
+}
+
+int nfp_flower_setup_meter_entry(struct nfp_app *app,
+				 const struct flow_action_entry *action,
+				 enum nfp_meter_op op,
+				 u32 meter_id)
+{
+	struct nfp_flower_priv *fl_priv = app->priv;
+	struct nfp_meter_entry *meter_entry = NULL;
+	int err = 0;
+
+	mutex_lock(&fl_priv->meter_stats_lock);
+
+	switch (op) {
+	case NFP_METER_DEL:
+		nfp_flower_del_meter_entry(app, meter_id);
+		goto ret;
+	case NFP_METER_ADD:
+		meter_entry = nfp_flower_add_meter_entry(app, meter_id);
+		break;
+	default:
+		meter_entry = nfp_flower_search_meter_entry(app, meter_id);
+		break;
+	}
+
+	if (!meter_entry) {
+		err = -ENOMEM;
+		goto ret;
+	}
+
+	if (!action) {
+		err = -EINVAL;
+		goto ret;
+	}
+
+	if (action->police.rate_bytes_ps > 0) {
+		meter_entry->bps = true;
+		meter_entry->rate = action->police.rate_bytes_ps;
+		meter_entry->burst = action->police.burst;
+	} else {
+		meter_entry->bps = false;
+		meter_entry->rate = action->police.rate_pkt_ps;
+		meter_entry->burst = action->police.burst_pkt;
+	}
+ret:
+	mutex_unlock(&fl_priv->meter_stats_lock);
+	return err;
+}
+
+int nfp_init_meter_table(struct nfp_app *app)
+{
+	struct nfp_flower_priv *priv = app->priv;
+
+	return rhashtable_init(&priv->meter_table, &stats_meter_table_params);
+}
 static int
 nfp_act_install_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
 			struct netlink_ext_ack *extack)
@@ -514,10 +643,13 @@ nfp_act_install_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
 		}
 
 		if (rate != 0) {
+			meter_id = action->hw_index;
+			if (nfp_flower_setup_meter_entry(app, action, NFP_METER_ADD, meter_id))
+				continue;
+
 			pps = false;
 			if (action->police.rate_pkt_ps > 0)
 				pps = true;
-			meter_id = action->hw_index;
 			nfp_flower_offload_one_police(app, false, pps, meter_id,
 						      rate, burst);
 			add = true;
@@ -534,9 +666,11 @@ static int
 nfp_act_remove_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
 		       struct netlink_ext_ack *extack)
 {
+	struct nfp_meter_entry *meter_entry = NULL;
 	struct nfp_police_config *config;
 	struct sk_buff *skb;
 	u32 meter_id;
+	bool pps;
 
 	/*delete qos associate data for this interface */
 	if (fl_act->id != FLOW_ACTION_POLICE) {
@@ -546,6 +680,14 @@ nfp_act_remove_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
 	}
 
 	meter_id = fl_act->index;
+	meter_entry = nfp_flower_search_meter_entry(app, meter_id);
+	if (!meter_entry) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "no meter entry when delete the action index.\n");
+		return -ENOENT;
+	}
+	pps = !meter_entry->bps;
+
 	skb = nfp_flower_cmsg_alloc(app, sizeof(struct nfp_police_config),
 				    NFP_FLOWER_CMSG_TYPE_QOS_DEL, GFP_KERNEL);
 	if (!skb)
@@ -555,7 +697,11 @@ nfp_act_remove_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
 	memset(config, 0, sizeof(struct nfp_police_config));
 	config->head.flags_opts = cpu_to_be32(NFP_FL_QOS_METER);
 	config->head.meter_id = cpu_to_be32(meter_id);
+	if (pps)
+		config->head.flags_opts |= cpu_to_be32(NFP_FL_QOS_PPS);
+
 	nfp_ctrl_tx(app->ctrl, skb);
+	nfp_flower_setup_meter_entry(app, NULL, NFP_METER_DEL, meter_id);
 
 	return 0;
 }
-- 
2.20.1

[PATCH net-next 4/6] nfp: add process to get action stats from hardware

[Simon Horman]

From: Baowen Zheng <baowen.zheng@corigine.com>

Add a process to update action stats from hardware.

This stats data will be updated to tc action when dumping actions
or filters.

Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../net/ethernet/netronome/nfp/flower/main.h  |   3 +-
 .../ethernet/netronome/nfp/flower/qos_conf.c  | 117 +++++++++++++++++-
 2 files changed, 115 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index 0c28e3414b7f..73bb76a938a2 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -606,7 +606,8 @@ nfp_flower_update_merge_stats(struct nfp_app *app,
 int nfp_setup_tc_act_offload(struct nfp_app *app,
 			     struct flow_offload_action *fl_act);
 int nfp_init_meter_table(struct nfp_app *app);
-
+void nfp_flower_stats_meter_request_all(struct nfp_flower_priv *fl_priv);
+void nfp_act_stats_reply(struct nfp_app *app, void *pmsg);
 int nfp_flower_offload_one_police(struct nfp_app *app, bool ingress,
 				  bool pps, u32 id, u32 rate, u32 burst);
 int nfp_flower_setup_meter_entry(struct nfp_app *app,
diff --git a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
index f9f9e506b303..632513b4f121 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
@@ -304,6 +304,9 @@ void nfp_flower_stats_rlim_reply(struct nfp_app *app, struct sk_buff *skb)
 	u32 netdev_port_id;
 
 	msg = nfp_flower_cmsg_get_data(skb);
+	if (be32_to_cpu(msg->head.flags_opts) & NFP_FL_QOS_METER)
+		return nfp_act_stats_reply(app, msg);
+
 	netdev_port_id = be32_to_cpu(msg->head.port);
 	rcu_read_lock();
 	netdev = nfp_app_dev_get(app, netdev_port_id, NULL);
@@ -335,7 +338,7 @@ void nfp_flower_stats_rlim_reply(struct nfp_app *app, struct sk_buff *skb)
 
 static void
 nfp_flower_stats_rlim_request(struct nfp_flower_priv *fl_priv,
-			      u32 netdev_port_id)
+			      u32 id, bool ingress)
 {
 	struct nfp_police_cfg_head *head;
 	struct sk_buff *skb;
@@ -346,10 +349,15 @@ nfp_flower_stats_rlim_request(struct nfp_flower_priv *fl_priv,
 				    GFP_ATOMIC);
 	if (!skb)
 		return;
-
 	head = nfp_flower_cmsg_get_data(skb);
+
 	memset(head, 0, sizeof(struct nfp_police_cfg_head));
-	head->port = cpu_to_be32(netdev_port_id);
+	if (ingress) {
+		head->port = cpu_to_be32(id);
+	} else {
+		head->flags_opts = cpu_to_be32(NFP_FL_QOS_METER);
+		head->meter_id = cpu_to_be32(id);
+	}
 
 	nfp_ctrl_tx(fl_priv->app->ctrl, skb);
 }
@@ -379,7 +387,8 @@ nfp_flower_stats_rlim_request_all(struct nfp_flower_priv *fl_priv)
 			if (!netdev_port_id)
 				continue;
 
-			nfp_flower_stats_rlim_request(fl_priv, netdev_port_id);
+			nfp_flower_stats_rlim_request(fl_priv,
+						      netdev_port_id, true);
 		}
 	}
 
@@ -397,6 +406,8 @@ static void update_stats_cache(struct work_struct *work)
 			       qos_stats_work);
 
 	nfp_flower_stats_rlim_request_all(fl_priv);
+	nfp_flower_stats_meter_request_all(fl_priv);
+
 	schedule_delayed_work(&fl_priv->qos_stats_work, NFP_FL_QOS_UPDATE);
 }
 
@@ -607,6 +618,29 @@ int nfp_init_meter_table(struct nfp_app *app)
 
 	return rhashtable_init(&priv->meter_table, &stats_meter_table_params);
 }
+
+void
+nfp_flower_stats_meter_request_all(struct nfp_flower_priv *fl_priv)
+{
+	struct nfp_meter_entry *meter_entry = NULL;
+	struct rhashtable_iter iter;
+
+	mutex_lock(&fl_priv->meter_stats_lock);
+	rhashtable_walk_enter(&fl_priv->meter_table, &iter);
+	rhashtable_walk_start(&iter);
+
+	while ((meter_entry = rhashtable_walk_next(&iter)) != NULL) {
+		if (IS_ERR(meter_entry))
+			continue;
+		nfp_flower_stats_rlim_request(fl_priv,
+					      meter_entry->meter_id, false);
+	}
+
+	rhashtable_walk_stop(&iter);
+	rhashtable_walk_exit(&iter);
+	mutex_unlock(&fl_priv->meter_stats_lock);
+}
+
 static int
 nfp_act_install_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
 			struct netlink_ext_ack *extack)
@@ -706,6 +740,79 @@ nfp_act_remove_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
 	return 0;
 }
 
+void
+nfp_act_stats_reply(struct nfp_app *app, void *pmsg)
+{
+	struct nfp_flower_priv *fl_priv = app->priv;
+	struct nfp_meter_entry *meter_entry = NULL;
+	struct nfp_police_stats_reply *msg = pmsg;
+	u32 meter_id;
+
+	meter_id = be32_to_cpu(msg->head.meter_id);
+	mutex_lock(&fl_priv->meter_stats_lock);
+
+	meter_entry = nfp_flower_search_meter_entry(app, meter_id);
+	if (!meter_entry)
+		goto ret;
+
+	meter_entry->stats.curr.pkts = be64_to_cpu(msg->pass_pkts) +
+				       be64_to_cpu(msg->drop_pkts);
+	meter_entry->stats.curr.bytes = be64_to_cpu(msg->pass_bytes) +
+					be64_to_cpu(msg->drop_bytes);
+	meter_entry->stats.curr.drops = be64_to_cpu(msg->drop_pkts);
+	if (!meter_entry->stats.update) {
+		meter_entry->stats.prev.pkts = meter_entry->stats.curr.pkts;
+		meter_entry->stats.prev.bytes = meter_entry->stats.curr.bytes;
+		meter_entry->stats.prev.drops = meter_entry->stats.curr.drops;
+	}
+
+	meter_entry->stats.update = jiffies;
+
+ret:
+	mutex_unlock(&fl_priv->meter_stats_lock);
+}
+
+static int
+nfp_act_stats_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
+		      struct netlink_ext_ack *extack)
+{
+	struct nfp_flower_priv *fl_priv = app->priv;
+	struct nfp_meter_entry *meter_entry = NULL;
+	u64 diff_bytes, diff_pkts, diff_drops;
+	int err = 0;
+
+	if (fl_act->id != FLOW_ACTION_POLICE) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "unsupported offload: qos rate limit offload requires police action");
+		return -EOPNOTSUPP;
+	}
+
+	mutex_lock(&fl_priv->meter_stats_lock);
+	meter_entry = nfp_flower_search_meter_entry(app, fl_act->index);
+	if (!meter_entry) {
+		err = -ENOENT;
+		goto ret;
+	}
+	diff_pkts = meter_entry->stats.curr.pkts > meter_entry->stats.prev.pkts ?
+		    meter_entry->stats.curr.pkts - meter_entry->stats.prev.pkts : 0;
+	diff_bytes = meter_entry->stats.curr.bytes > meter_entry->stats.prev.bytes ?
+		     meter_entry->stats.curr.bytes - meter_entry->stats.prev.bytes : 0;
+	diff_drops = meter_entry->stats.curr.drops > meter_entry->stats.prev.drops ?
+		     meter_entry->stats.curr.drops - meter_entry->stats.prev.drops : 0;
+
+	flow_stats_update(&fl_act->stats, diff_bytes, diff_pkts, diff_drops,
+			  meter_entry->stats.update,
+			  FLOW_ACTION_HW_STATS_DELAYED);
+
+	meter_entry->stats.prev.pkts = meter_entry->stats.curr.pkts;
+	meter_entry->stats.prev.bytes = meter_entry->stats.curr.bytes;
+	meter_entry->stats.prev.drops = meter_entry->stats.curr.drops;
+ret:
+	mutex_unlock(&fl_priv->meter_stats_lock);
+
+	return err;
+}
+
 int nfp_setup_tc_act_offload(struct nfp_app *app,
 			     struct flow_offload_action *fl_act)
 {
@@ -720,6 +827,8 @@ int nfp_setup_tc_act_offload(struct nfp_app *app,
 		return nfp_act_install_actions(app, fl_act, extack);
 	case FLOW_ACT_DESTROY:
 		return nfp_act_remove_actions(app, fl_act, extack);
+	case FLOW_ACT_STATS:
+		return nfp_act_stats_actions(app, fl_act, extack);
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.20.1

[PATCH net-next 5/6] nfp: add support to offload police action from flower table

[Simon Horman]

From: Baowen Zheng <baowen.zheng@corigine.com>

Offload flow table if the action is already offloaded to hardware when
flow table uses this action.

Change meter id to type of u32 to support all the action index.

Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../ethernet/netronome/nfp/flower/action.c    | 58 +++++++++++++++++++
 .../net/ethernet/netronome/nfp/flower/cmsg.h  |  7 +++
 .../net/ethernet/netronome/nfp/flower/main.h  |  2 +
 .../ethernet/netronome/nfp/flower/qos_conf.c  |  2 +-
 4 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/action.c b/drivers/net/ethernet/netronome/nfp/flower/action.c
index a3242b36e216..2c40a3959f94 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/action.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/action.c
@@ -922,6 +922,51 @@ nfp_fl_pedit(const struct flow_action_entry *act,
 	}
 }
 
+static struct nfp_fl_meter *nfp_fl_meter(char *act_data)
+{
+	size_t act_size = sizeof(struct nfp_fl_meter);
+	struct nfp_fl_meter *meter_act;
+
+	meter_act = (struct nfp_fl_meter *)act_data;
+
+	memset(meter_act, 0, act_size);
+
+	meter_act->head.jump_id = NFP_FL_ACTION_OPCODE_METER;
+	meter_act->head.len_lw = act_size >> NFP_FL_LW_SIZ;
+
+	return meter_act;
+}
+
+static int
+nfp_flower_meter_action(struct nfp_app *app,
+			const struct flow_action_entry *action,
+			struct nfp_fl_payload *nfp_fl, int *a_len,
+			struct net_device *netdev,
+			struct netlink_ext_ack *extack)
+{
+	struct nfp_fl_meter *fl_meter;
+	u32 meter_id;
+
+	if (*a_len + sizeof(struct nfp_fl_meter) > NFP_FL_MAX_A_SIZ) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "unsupported offload:meter action size beyond the allowed maximum");
+		return -EOPNOTSUPP;
+	}
+
+	meter_id = action->hw_index;
+	if (!nfp_flower_search_meter_entry(app, meter_id)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "can not offload flow table with unsupported police action.\n");
+		return -EOPNOTSUPP;
+	}
+
+	fl_meter = nfp_fl_meter(&nfp_fl->action_data[*a_len]);
+	*a_len += sizeof(struct nfp_fl_meter);
+	fl_meter->meter_id = cpu_to_be32(meter_id);
+
+	return 0;
+}
+
 static int
 nfp_flower_output_action(struct nfp_app *app,
 			 const struct flow_action_entry *act,
@@ -985,6 +1030,7 @@ nfp_flower_loop_action(struct nfp_app *app, const struct flow_action_entry *act,
 		       struct nfp_flower_pedit_acts *set_act, bool *pkt_host,
 		       struct netlink_ext_ack *extack, int act_idx)
 {
+	struct nfp_flower_priv *fl_priv = app->priv;
 	struct nfp_fl_pre_tunnel *pre_tun;
 	struct nfp_fl_set_tun *set_tun;
 	struct nfp_fl_push_vlan *psh_v;
@@ -1149,6 +1195,18 @@ nfp_flower_loop_action(struct nfp_app *app, const struct flow_action_entry *act,
 
 		*pkt_host = true;
 		break;
+	case FLOW_ACTION_POLICE:
+		if (!(fl_priv->flower_ext_feats & NFP_FL_FEATS_QOS_METER)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "unsupported offload: unsupported police action in action list");
+			return -EOPNOTSUPP;
+		}
+
+		err = nfp_flower_meter_action(app, act, nfp_fl, a_len, netdev,
+					      extack);
+		if (err)
+			return err;
+		break;
 	default:
 		/* Currently we do not handle any other actions. */
 		NL_SET_ERR_MSG_MOD(extack, "unsupported offload: unsupported action in action list");
diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
index 784292b16290..4518ee90afef 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
@@ -85,6 +85,7 @@
 #define NFP_FL_ACTION_OPCODE_SET_TCP		15
 #define NFP_FL_ACTION_OPCODE_PRE_LAG		16
 #define NFP_FL_ACTION_OPCODE_PRE_TUNNEL		17
+#define NFP_FL_ACTION_OPCODE_METER		24
 #define NFP_FL_ACTION_OPCODE_PUSH_GENEVE	26
 #define NFP_FL_ACTION_OPCODE_NUM		32
 
@@ -260,6 +261,12 @@ struct nfp_fl_set_mpls {
 	__be32 lse;
 };
 
+struct nfp_fl_meter {
+	struct nfp_fl_act_head head;
+	__be16 reserved;
+	__be32 meter_id;
+};
+
 /* Metadata with L2 (1W/4B)
  * ----------------------------------------------------------------
  *    3                   2                   1
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index 73bb76a938a2..898abb68b80b 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -614,4 +614,6 @@ int nfp_flower_setup_meter_entry(struct nfp_app *app,
 				 const struct flow_action_entry *action,
 				 enum nfp_meter_op op,
 				 u32 meter_id);
+struct nfp_meter_entry *
+nfp_flower_search_meter_entry(struct nfp_app *app, u32 meter_id);
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
index 632513b4f121..6da24c457ab3 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
@@ -502,7 +502,7 @@ static const struct rhashtable_params stats_meter_table_params = {
 	.key_len	= sizeof(u32),
 };
 
-static struct nfp_meter_entry *
+struct nfp_meter_entry *
 nfp_flower_search_meter_entry(struct nfp_app *app, u32 meter_id)
 {
 	struct nfp_flower_priv *priv = app->priv;
-- 
2.20.1

[PATCH net-next 6/6] nfp: add NFP_FL_FEATS_QOS_METER to host features to enable meter offload

[Simon Horman]

From: Baowen Zheng <baowen.zheng@corigine.com>

Add NFP_FL_FEATS_QOS_METER to host features to enable meter
offload in driver.

Before adding this feature, we will not offload any police action
since we will check the host features before offloading any police
action.

Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/flower/main.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index 898abb68b80b..627d5155e376 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -66,7 +66,8 @@ struct nfp_app;
 	NFP_FL_FEATS_PRE_TUN_RULES | \
 	NFP_FL_FEATS_IPV6_TUN | \
 	NFP_FL_FEATS_VLAN_QINQ | \
-	NFP_FL_FEATS_QOS_PPS)
+	NFP_FL_FEATS_QOS_PPS | \
+	NFP_FL_FEATS_QOS_METER)
 
 struct nfp_fl_mask_id {
 	struct circ_buf mask_id_free_list;
-- 
2.20.1

Re: [PATCH net-next 2/6] nfp: add support to offload tc action to hardware

[Jakub Kicinski]

On Thu, 17 Feb 2022 11:56:48 +0100 Simon Horman wrote:
> +	if (add)
> +		return 0;
> +	else
> +		return -EOPNOTSUPP;

	return add ? 0 : -EOPNOTSUPP;

or at least remove the else, everything after if () return; is in an
'else' branch.

Re: [PATCH net-next 3/6] nfp: add hash table to store meter table

[Jakub Kicinski]

On Thu, 17 Feb 2022 11:56:49 +0100 Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
> 
> Add a hash table to store meter table.
> 
> This meter table will also be used by flower action.

> +static struct nfp_meter_entry *
> +nfp_flower_add_meter_entry(struct nfp_app *app, u32 meter_id)
> +{
> +	struct nfp_meter_entry *meter_entry = NULL;
> +	struct nfp_flower_priv *priv = app->priv;
> +
> +	meter_entry = rhashtable_lookup_fast(&priv->meter_table,
> +					     &meter_id,
> +					     stats_meter_table_params);
> +

unnecessary new line

> +	if (meter_entry)
> +		return meter_entry;
> +
> +	meter_entry = kzalloc(sizeof(*meter_entry), GFP_ATOMIC);

why is this ATOMIC?

> +	if (!meter_entry)
> +		goto err;
> +
> +	meter_entry->meter_id = meter_id;
> +	meter_entry->used = jiffies;
> +	if (rhashtable_insert_fast(&priv->meter_table, &meter_entry->ht_node,
> +				   stats_meter_table_params)) {
> +		goto err_free_meter_entry;
> +	}

unnecessary brackets

> +	priv->qos_rate_limiters++;
> +	if (priv->qos_rate_limiters == 1)
> +		schedule_delayed_work(&priv->qos_stats_work,
> +				      NFP_FL_QOS_UPDATE);
> +	return meter_entry;
> +
> +err_free_meter_entry:
> +	kfree(meter_entry);
> +err:

don't jump to return, just return directly instead of a goto

> +	return NULL;
> +}
> +
> +static void nfp_flower_del_meter_entry(struct nfp_app *app, u32 meter_id)
> +{
> +	struct nfp_meter_entry *meter_entry = NULL;
> +	struct nfp_flower_priv *priv = app->priv;
> +
> +	meter_entry = rhashtable_lookup_fast(&priv->meter_table, &meter_id,
> +					     stats_meter_table_params);
> +

unnecessary nl

> +	if (meter_entry) {

flip condition and return early

> +		rhashtable_remove_fast(&priv->meter_table,
> +				       &meter_entry->ht_node,
> +				       stats_meter_table_params);
> +		kfree(meter_entry);
> +		priv->qos_rate_limiters--;
> +		if (!priv->qos_rate_limiters)
> +			cancel_delayed_work_sync(&priv->qos_stats_work);
> +	}
> +}
> +
> +int nfp_flower_setup_meter_entry(struct nfp_app *app,
> +				 const struct flow_action_entry *action,
> +				 enum nfp_meter_op op,
> +				 u32 meter_id)
> +{
> +	struct nfp_flower_priv *fl_priv = app->priv;
> +	struct nfp_meter_entry *meter_entry = NULL;
> +	int err = 0;
> +
> +	mutex_lock(&fl_priv->meter_stats_lock);
> +
> +	switch (op) {
> +	case NFP_METER_DEL:
> +		nfp_flower_del_meter_entry(app, meter_id);
> +		goto ret;

try to avoid naming labels with common variable names, exit_unlock
would be most in line with the style of the driver here.

> +	case NFP_METER_ADD:
> +		meter_entry = nfp_flower_add_meter_entry(app, meter_id);
> +		break;
> +	default:

why default and not use _SET?

> +		meter_entry = nfp_flower_search_meter_entry(app, meter_id);
> +		break;
> +	}
> +
> +	if (!meter_entry) {
> +		err = -ENOMEM;
> +		goto ret;
> +	}
> +
> +	if (!action) {
> +		err = -EINVAL;
> +		goto ret;
> +	}

defensive programming is discouraged in the kernel, please drop the
action check if it can't happen in practice

> +	if (action->police.rate_bytes_ps > 0) {
> +		meter_entry->bps = true;
> +		meter_entry->rate = action->police.rate_bytes_ps;
> +		meter_entry->burst = action->police.burst;
> +	} else {
> +		meter_entry->bps = false;
> +		meter_entry->rate = action->police.rate_pkt_ps;
> +		meter_entry->burst = action->police.burst_pkt;
> +	}
> +ret:
> +	mutex_unlock(&fl_priv->meter_stats_lock);
> +	return err;
> +}
> +
> +int nfp_init_meter_table(struct nfp_app *app)
> +{
> +	struct nfp_flower_priv *priv = app->priv;
> +
> +	return rhashtable_init(&priv->meter_table, &stats_meter_table_params);
> +}

missing nl

>  static int
>  nfp_act_install_actions(struct nfp_app *app, struct flow_offload_action *fl_act,
>  			struct netlink_ext_ack *extack)

RE: [PATCH net-next 2/6] nfp: add support to offload tc action to hardware

[Baowen Zheng]

On Friday, February 18, 2022 12:40 PM, Jakub:
>To: Simon Horman <simon.horman@corigine.com>
>Cc: David Miller <davem@davemloft.net>; Baowen Zheng
><baowen.zheng@corigine.com>; Louis Peens <louis.peens@corigine.com>;
>netdev@vger.kernel.org; oss-drivers <oss-drivers@corigine.com>
>Subject: Re: [PATCH net-next 2/6] nfp: add support to offload tc action to
>hardware
>
>On Thu, 17 Feb 2022 11:56:48 +0100 Simon Horman wrote:
>> +	if (add)
>> +		return 0;
>> +	else
>> +		return -EOPNOTSUPP;
>
>	return add ? 0 : -EOPNOTSUPP;
>
>or at least remove the else, everything after if () return; is in an 'else' branch.
Thanks, will make the change in V2 patch.

RE: [PATCH net-next 3/6] nfp: add hash table to store meter table

[Baowen Zheng]

On February 18, 2022 12:47 PM, Jakub wrote:
>On Thu, 17 Feb 2022 11:56:49 +0100 Simon Horman wrote:
>> From: Baowen Zheng <baowen.zheng@corigine.com>
>>
>> Add a hash table to store meter table.
>>
>> This meter table will also be used by flower action.
>
>> +static struct nfp_meter_entry *
>> +nfp_flower_add_meter_entry(struct nfp_app *app, u32 meter_id) {
>> +	struct nfp_meter_entry *meter_entry = NULL;
>> +	struct nfp_flower_priv *priv = app->priv;
>> +
>> +	meter_entry = rhashtable_lookup_fast(&priv->meter_table,
>> +					     &meter_id,
>> +					     stats_meter_table_params);
>> +
>
>unnecessary new line
Thanks, will make the change in V2 patch.
>
>> +	if (meter_entry)
>> +		return meter_entry;
>> +
>> +	meter_entry = kzalloc(sizeof(*meter_entry), GFP_ATOMIC);
>
>why is this ATOMIC?
Previously, we use spin_lock to protect the meter entry, so we use the 
But now we change the mutex so it will make sense to use GFP_KERNEL memory alloc. 
We will make the change in V2 patch.
Thanks
>
>> +	if (!meter_entry)
>> +		goto err;
>> +
>> +	meter_entry->meter_id = meter_id;
>> +	meter_entry->used = jiffies;
>> +	if (rhashtable_insert_fast(&priv->meter_table, &meter_entry-
>>ht_node,
>> +				   stats_meter_table_params)) {
>> +		goto err_free_meter_entry;
>> +	}
>
>unnecessary brackets
Thanks, will make the change in V2 patch.
>
>> +	priv->qos_rate_limiters++;
>> +	if (priv->qos_rate_limiters == 1)
>> +		schedule_delayed_work(&priv->qos_stats_work,
>> +				      NFP_FL_QOS_UPDATE);
>> +	return meter_entry;
>> +
>> +err_free_meter_entry:
>> +	kfree(meter_entry);
>> +err:
>
>don't jump to return, just return directly instead of a goto
>
>> +	return NULL;
>> +}
>> +
>> +static void nfp_flower_del_meter_entry(struct nfp_app *app, u32
>> +meter_id) {
>> +	struct nfp_meter_entry *meter_entry = NULL;
>> +	struct nfp_flower_priv *priv = app->priv;
>> +
>> +	meter_entry = rhashtable_lookup_fast(&priv->meter_table,
>&meter_id,
>> +					     stats_meter_table_params);
>> +
>
>unnecessary nl
Thanks, will make the change in V2 patch.
>
>> +	if (meter_entry) {
>
>flip condition and return early
Thanks, will make the change in V2 patch.
>
>> +		rhashtable_remove_fast(&priv->meter_table,
>> +				       &meter_entry->ht_node,
>> +				       stats_meter_table_params);
>> +		kfree(meter_entry);
>> +		priv->qos_rate_limiters--;
>> +		if (!priv->qos_rate_limiters)
>> +			cancel_delayed_work_sync(&priv->qos_stats_work);
>> +	}
>> +}
>> +
>> +int nfp_flower_setup_meter_entry(struct nfp_app *app,
>> +				 const struct flow_action_entry *action,
>> +				 enum nfp_meter_op op,
>> +				 u32 meter_id)
>> +{
>> +	struct nfp_flower_priv *fl_priv = app->priv;
>> +	struct nfp_meter_entry *meter_entry = NULL;
>> +	int err = 0;
>> +
>> +	mutex_lock(&fl_priv->meter_stats_lock);
>> +
>> +	switch (op) {
>> +	case NFP_METER_DEL:
>> +		nfp_flower_del_meter_entry(app, meter_id);
>> +		goto ret;
>
>try to avoid naming labels with common variable names, exit_unlock would be
>most in line with the style of the driver here.
Thanks, will make the change in V2 patch.
>
>> +	case NFP_METER_ADD:
>> +		meter_entry = nfp_flower_add_meter_entry(app, meter_id);
>> +		break;
>> +	default:
>
>why default and not use _SET?
Actually, we will check if the meter entry exists and add in NFP_METER_ADD,  so we do not use the NFP_METER_SET. 
Maybe we will need to delete the NFP_METER_SET definition to omit the confusion.
Thanks
>
>> +		meter_entry = nfp_flower_search_meter_entry(app,
>meter_id);
>> +		break;
>> +	}
>> +
>> +	if (!meter_entry) {
>> +		err = -ENOMEM;
>> +		goto ret;
>> +	}
>> +
>> +	if (!action) {
>> +		err = -EINVAL;
>> +		goto ret;
>> +	}
>
>defensive programming is discouraged in the kernel, please drop the action
>check if it can't happen in practice
Thanks, will make the change in V2 patch.
>
>> +	if (action->police.rate_bytes_ps > 0) {
>> +		meter_entry->bps = true;
>> +		meter_entry->rate = action->police.rate_bytes_ps;
>> +		meter_entry->burst = action->police.burst;
>> +	} else {
>> +		meter_entry->bps = false;
>> +		meter_entry->rate = action->police.rate_pkt_ps;
>> +		meter_entry->burst = action->police.burst_pkt;
>> +	}
>> +ret:
>> +	mutex_unlock(&fl_priv->meter_stats_lock);
>> +	return err;
>> +}
>> +
>> +int nfp_init_meter_table(struct nfp_app *app) {
>> +	struct nfp_flower_priv *priv = app->priv;
>> +
>> +	return rhashtable_init(&priv->meter_table,
>> +&stats_meter_table_params); }
>
>missing nl
Thanks, will make the change in V2 patch.
>
>>  static int
>>  nfp_act_install_actions(struct nfp_app *app, struct flow_offload_action
>*fl_act,
>>  			struct netlink_ext_ack *extack)