Re: [PATCH net-next V3 03/11] net/mlx5: fs, add counter object to flow destination

[Moshe Shemesh <moshe@nvidia.com>]

On 12/19/2024 11:00 AM, Przemek Kitszel wrote:
> 
> On 12/18/24 16:09, Tariq Toukan wrote:
>> From: Moshe Shemesh <moshe@nvidia.com>
>>
>> Currently mlx5_flow_destination includes counter_id which is assigned in
>> case we use flow counter on the flow steering rule. However, counter_id
>> is not enough data in case of using HW Steering. Thus, have mlx5_fc
>> object as part of mlx5_flow_destination instead of counter_id and assign
>> it where needed.
>>
>> In case counter_id is received from user space, create a local counter
>> object to represent it.
>>
>> Signed-off-by: Moshe Shemesh <moshe@nvidia.com>
>> Reviewed-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
>> Reviewed-by: Mark Bloch <mbloch@nvidia.com>
>> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
>> ---
>>   drivers/infiniband/hw/mlx5/fs.c               | 37 +++++++++----
>>   .../mellanox/mlx5/core/diag/fs_tracepoint.h   |  2 +-
>>   .../mellanox/mlx5/core/en_accel/ipsec_fs.c    | 20 +++----
>>   .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  2 +-
>>   .../mellanox/mlx5/core/esw/acl/egress_lgcy.c  |  2 +-
>>   .../mellanox/mlx5/core/esw/acl/ingress_lgcy.c |  2 +-
>>   .../ethernet/mellanox/mlx5/core/esw/bridge.c  | 20 +++----
>>   .../mellanox/mlx5/core/eswitch_offloads.c     |  2 +-
>>   .../net/ethernet/mellanox/mlx5/core/fs_cmd.c  |  2 +-
>>   .../net/ethernet/mellanox/mlx5/core/fs_core.c |  1 +
>>   .../ethernet/mellanox/mlx5/core/fs_counters.c | 52 +++++++++++++++++++
>>   .../mellanox/mlx5/core/lib/macsec_fs.c        |  8 +--
>>   .../mellanox/mlx5/core/steering/sws/fs_dr.c   |  2 +-
>>   drivers/vdpa/mlx5/net/mlx5_vnet.c             |  4 +-
>>   include/linux/mlx5/fs.h                       |  4 +-
>>   15 files changed, 116 insertions(+), 44 deletions(-)
> 
> 
> 
>> +/**
>> + * mlx5_fc_local_create - Allocate mlx5_fc struct for a counter which
>> + * was already acquired using its counter id and bulk data.
>> + *
>> + * @counter_id: counter acquired counter id
>> + * @offset: counter offset from bulk base
>> + * @bulk_size: counter's bulk size as was allocated
>> + *
>> + * Return: Pointer to mlx5_fc on success, ERR_PTR otherwise.
>> + */
>> +struct mlx5_fc *
>> +mlx5_fc_local_create(u32 counter_id, u32 offset, u32 bulk_size)
>> +{
>> +     struct mlx5_fc_bulk *bulk;
>> +     struct mlx5_fc *counter;
>> +
>> +     counter = kzalloc(sizeof(*counter), GFP_KERNEL);
>> +     if (!counter)
>> +             return ERR_PTR(-ENOMEM);
>> +     bulk = kzalloc(sizeof(*bulk), GFP_KERNEL);
>> +     if (!bulk) {
>> +             kfree(counter);
>> +             return ERR_PTR(-ENOMEM);
>> +     }
>> +
> 
> I would expect to have following line added here:
> 
>         counter->bulk = bulk;
> 
> otherwise that is memleak?

right, will fix.
> 
>> +     counter->type = MLX5_FC_TYPE_LOCAL;
>> +     counter->id = counter_id;
>> +     bulk->base_id = counter_id - offset;
>> +     bulk->bulk_len = bulk_size;
>> +     return counter;
>> +}
>> +EXPORT_SYMBOL(mlx5_fc_local_create);
>> +
>> +void mlx5_fc_local_destroy(struct mlx5_fc *counter)
>> +{
>> +     if (!counter || counter->type != MLX5_FC_TYPE_LOCAL)
>> +             return;
>> +
>> +     kfree(counter->bulk);
> 
> in the whole patch there is no "->bulk ="
> you didn't catched that as it's fine to kfree(NULL) of course

right, thanks!

> 
>> +     kfree(counter);
>> +}
>> +EXPORT_SYMBOL(mlx5_fc_local_destroy);