| 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
| ||
|
Message-ID: <d8788869-51d6-45c8-9009-e72453cc381c@nvidia.com>
Date: Thu, 19 Dec 2024 14:30:41 +0200
From: Moshe Shemesh <moshe@...dia.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>, Tariq Toukan
<tariqt@...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/19/2024 11:17 AM, Przemek Kitszel wrote:
>
> 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.
>
I tend to name labels according to what they jump from. Though if I see
on same function labels are used the other way I try to be consistent
with current code.
I think there are pros and cons for both ways and both ways are used.
I can change here, but is that kernel or netdev consensus ?
>>
>> - 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
Agree, will fix
>
> #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);
>
Powered by blists - more mailing lists