diff mbox series

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

Message ID 20220217105652.14451-4-simon.horman@corigine.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series nfp: flow-independent tc action hardware offload | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 2 maintainers not CCed: yinjun.zhang@corigine.com peng.zhang@corigine.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 95 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Simon Horman Feb. 17, 2022, 10:56 a.m. UTC 
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(-)

Comments

Jakub Kicinski Feb. 18, 2022, 4:47 a.m. UTC | #1 
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)
Baowen Zheng Feb. 18, 2022, 8:26 a.m. UTC | #2 
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)
diff mbox series

Patch

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;
 }