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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220217204714.33132c8a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 17 Feb 2022 20:47:14 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Simon Horman <simon.horman@...igine.com>
Cc:     David Miller <davem@...emloft.net>,
        Baowen Zheng <baowen.zheng@...igine.com>,
        Louis Peens <louis.peens@...igine.com>, netdev@...r.kernel.org,
        oss-drivers@...igine.com
Subject: Re: [PATCH net-next 3/6] nfp: add hash table to store meter table

On Thu, 17 Feb 2022 11:56:49 +0100 Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@...igine.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)