netdev - Re: [PATCH net-next V3 04/11] net/mlx5: fs, add mlx5_fs

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <0c6d6368-85ab-4112-a423-828a51b703e1@intel.com>
Date: Thu, 19 Dec 2024 10:17:13 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Tariq Toukan <tariqt@...dia.com>, Moshe Shemesh <moshe@...dia.com>
CC: <netdev@...r.kernel.org>, Saeed Mahameed <saeedm@...dia.com>, Gal Pressman
	<gal@...dia.com>, Leon Romanovsky <leonro@...dia.com>, Mark Bloch
	<mbloch@...dia.com>, Yevgeny Kliteynik <kliteyn@...dia.com>, "David S.
 Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>, Andrew Lunn
	<andrew+netdev@...n.ch>
Subject: Re: [PATCH net-next V3 04/11] net/mlx5: fs, add mlx5_fs_pool API

On 12/18/24 16:09, Tariq Toukan wrote:
> From: Moshe Shemesh <moshe@...dia.com>
> 
> Refactor fc_pool API to create generic fs_pool API, as HW steering has
> more flow steering elements which can take advantage of the same pool of
> bulks API. Change fs_counters code to use the fs_pool API.
> 
> Note, removed __counted_by from struct mlx5_fc_bulk as bulk_len is now
> inner struct member. It will be added back once __counted_by can support
> inner struct members.
> 
> Signed-off-by: Moshe Shemesh <moshe@...dia.com>
> Reviewed-by: Yevgeny Kliteynik <kliteyn@...dia.com>
> Reviewed-by: Mark Bloch <mbloch@...dia.com>
> Signed-off-by: Tariq Toukan <tariqt@...dia.com>
> ---
>   .../net/ethernet/mellanox/mlx5/core/Makefile  |   2 +-
>   .../ethernet/mellanox/mlx5/core/fs_counters.c | 294 +++++-------------
>   .../net/ethernet/mellanox/mlx5/core/fs_pool.c | 194 ++++++++++++
>   .../net/ethernet/mellanox/mlx5/core/fs_pool.h |  54 ++++
>   4 files changed, 331 insertions(+), 213 deletions(-)
>   create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/fs_pool.c
>   create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/fs_pool.h
> 

[...]

> +static struct mlx5_fs_bulk *mlx5_fc_bulk_create(struct mlx5_core_dev *dev)
>   {
>   	enum mlx5_fc_bulk_alloc_bitmask alloc_bitmask;
> -	struct mlx5_fc_bulk *bulk;
> -	int err = -ENOMEM;
> +	struct mlx5_fc_bulk *fc_bulk;
>   	int bulk_len;
>   	u32 base_id;
>   	int i;
> @@ -478,71 +460,97 @@ static struct mlx5_fc_bulk *mlx5_fc_bulk_create(struct mlx5_core_dev *dev)
>   	alloc_bitmask = MLX5_CAP_GEN(dev, flow_counter_bulk_alloc);
>   	bulk_len = alloc_bitmask > 0 ? MLX5_FC_BULK_NUM_FCS(alloc_bitmask) : 1;
>   
> -	bulk = kvzalloc(struct_size(bulk, fcs, bulk_len), GFP_KERNEL);
> -	if (!bulk)
> -		goto err_alloc_bulk;
> +	fc_bulk = kvzalloc(struct_size(fc_bulk, fcs, bulk_len), GFP_KERNEL);
> +	if (!fc_bulk)
> +		return NULL;
>   
> -	bulk->bitmask = kvcalloc(BITS_TO_LONGS(bulk_len), sizeof(unsigned long),
> -				 GFP_KERNEL);
> -	if (!bulk->bitmask)
> -		goto err_alloc_bitmask;
> +	if (mlx5_fs_bulk_init(dev, &fc_bulk->fs_bulk, bulk_len))
> +		goto err_fs_bulk_init;

Locally (say two lines above) your label name is obvious.
But please imagine it in the context of whole function, it is much
better to name labels after what they jump to (instead of what they
jump from). It is not only easier to reason about, but also more
future proof. I think Simon would agree.
I'm fine with keeping existing code as-is, but for new code, it's
always better to write it up to the best practices known.

>   
> -	err = mlx5_cmd_fc_bulk_alloc(dev, alloc_bitmask, &base_id);
> -	if (err)
> -		goto err_mlx5_cmd_bulk_alloc;
> +	if (mlx5_cmd_fc_bulk_alloc(dev, alloc_bitmask, &base_id))
> +		goto err_cmd_bulk_alloc;
> +	fc_bulk->base_id = base_id;
> +	for (i = 0; i < bulk_len; i++)
> +		mlx5_fc_init(&fc_bulk->fcs[i], fc_bulk, base_id + i);
>   
> -	bulk->base_id = base_id;
> -	bulk->bulk_len = bulk_len;
> -	for (i = 0; i < bulk_len; i++) {
> -		mlx5_fc_init(&bulk->fcs[i], bulk, base_id + i);
> -		set_bit(i, bulk->bitmask);
> -	}
> +	return &fc_bulk->fs_bulk;
>   
> -	return bulk;
> -
> -err_mlx5_cmd_bulk_alloc:
> -	kvfree(bulk->bitmask);
> -err_alloc_bitmask:
> -	kvfree(bulk);
> -err_alloc_bulk:
> -	return ERR_PTR(err);
> +err_cmd_bulk_alloc:

fs_bulk_cleanup:

> +	mlx5_fs_bulk_cleanup(&fc_bulk->fs_bulk);
> +err_fs_bulk_init:

fs_bulk_free:

> +	kvfree(fc_bulk);
> +	return NULL;
>   }

[...]

> @@ -558,22 +566,22 @@ static int mlx5_fc_bulk_release_fc(struct mlx5_fc_bulk *bulk, struct mlx5_fc *fc
>   struct mlx5_fc *
>   mlx5_fc_local_create(u32 counter_id, u32 offset, u32 bulk_size)
>   {
> -	struct mlx5_fc_bulk *bulk;
> +	struct mlx5_fc_bulk *fc_bulk;

there is really no need to rename this variable in this patch
either drop the rename or name it like that in prev patch

#avoid-trashing

>   	struct mlx5_fc *counter;
>   
>   	counter = kzalloc(sizeof(*counter), GFP_KERNEL);
>   	if (!counter)
>   		return ERR_PTR(-ENOMEM);
> -	bulk = kzalloc(sizeof(*bulk), GFP_KERNEL);
> -	if (!bulk) {
> +	fc_bulk = kzalloc(sizeof(*fc_bulk), GFP_KERNEL);
> +	if (!fc_bulk) {
>   		kfree(counter);
>   		return ERR_PTR(-ENOMEM);
>   	}
>   
>   	counter->type = MLX5_FC_TYPE_LOCAL;
>   	counter->id = counter_id;
> -	bulk->base_id = counter_id - offset;
> -	bulk->bulk_len = bulk_size;
> +	fc_bulk->base_id = counter_id - offset;
> +	fc_bulk->fs_bulk.bulk_len = bulk_size;
>   	return counter;
>   }
>   EXPORT_SYMBOL(mlx5_fc_local_create);