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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <DM5PR1301MB2172C8E090C289F2E7971D86E7379@DM5PR1301MB2172.namprd13.prod.outlook.com>
Date:   Fri, 18 Feb 2022 08:26:24 +0000
From:   Baowen Zheng <baowen.zheng@...igine.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Simon Horman <simon.horman@...igine.com>
CC:     David Miller <davem@...emloft.net>,
        Louis Peens <louis.peens@...igine.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        oss-drivers <oss-drivers@...igine.com>
Subject: RE: [PATCH net-next 3/6] nfp: add hash table to store meter table

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@...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
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)