| 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: <31a55447-60ef-4cfb-bfd9-cc613619e29c@intel.com>
Date: Mon, 12 Aug 2024 13:23:20 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: <netdev@...r.kernel.org>, Ido Schimmel <idosch@...dia.com>, Petr Machata
<petrm@...dia.com>, Jakub Kicinski <kuba@...nel.org>, Andrew Lunn
<andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean
<olteanv@...il.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Saeed Mahameed
<saeedm@...dia.com>, Leon Romanovsky <leon@...nel.org>, Tariq Toukan
<tariqt@...dia.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
<nex.sw.ncis.osdt.itp.upstreaming@...el.com>, Wojciech Drewek
<wojciech.drewek@...el.com>
Subject: Re: [PATCH net-next 5/5] mlxsw: spectrum_kvdl: combine devlink
resource occupation getters
On 8/7/24 08:47, Jiri Pirko wrote:
> Tue, Aug 06, 2024 at 04:33:07PM CEST, przemyslaw.kitszel@...el.com wrote:
>> Combine spectrum1 kvdl devlink resource occupation getters into one.
>>
>> Thanks to previous commit of the series we could easily embed more than
>> just a single pointer into devlink resource.
>>
>> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>> ---
>> .../net/ethernet/mellanox/mlxsw/spectrum.h | 5 ++
>> .../net/ethernet/mellanox/mlxsw/spectrum.c | 3 +-
>> .../ethernet/mellanox/mlxsw/spectrum1_kvdl.c | 80 ++++++++-----------
>> 3 files changed, 41 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> index 8d3c61287696..91fe5fffa675 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> @@ -836,6 +836,11 @@ int mlxsw_sp_kvdl_alloc_count_query(struct mlxsw_sp *mlxsw_sp,
>> unsigned int *p_alloc_count);
>>
>> /* spectrum1_kvdl.c */
>> +struct mlxsw_sp1_kvdl_occ_ctx {
>> + struct mlxsw_sp1_kvdl *kvdl;
>> + int first_part_id;
>> + bool count_all_parts;
>> +};
>> extern const struct mlxsw_sp_kvdl_ops mlxsw_sp1_kvdl_ops;
>> int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core);
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> index 2730ae3d8fe6..3bda2b2d16f9 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> @@ -3669,7 +3669,8 @@ static int mlxsw_sp1_resources_kvd_register(struct mlxsw_core *mlxsw_core)
>> linear_size,
>> MLXSW_SP_RESOURCE_KVD_LINEAR,
>> MLXSW_SP_RESOURCE_KVD,
>> - &linear_size_params, sizeof(void *));
>> + &linear_size_params,
>> + sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> if (err)
>> return err;
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>> index ee5f12746371..a8bf052adf31 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>> @@ -292,68 +292,53 @@ static u64 mlxsw_sp1_kvdl_part_occ(struct mlxsw_sp1_kvdl_part *part)
>>
>> static u64 mlxsw_sp1_kvdl_occ_get(void *priv)
>> {
>> - const struct mlxsw_sp1_kvdl *kvdl = priv;
>> + struct mlxsw_sp1_kvdl_occ_ctx *ctx = priv;
>> + bool cnt_all = ctx->count_all_parts;
>> + int beg, end;
>> u64 occ = 0;
>> - int i;
>>
>> - for (i = 0; i < MLXSW_SP1_KVDL_PARTS_INFO_LEN; i++)
>> - occ += mlxsw_sp1_kvdl_part_occ(kvdl->parts[i]);
>> + beg = cnt_all ? 0 : ctx->first_part_id,
>> + end = cnt_all ? MLXSW_SP1_KVDL_PARTS_INFO_LEN : beg + 1;
>> + for (int i = beg; i < end; i++)
>> + occ += mlxsw_sp1_kvdl_part_occ(ctx->kvdl->parts[i]);
>>
>> return occ;
>
> I don't see the benefit, this just makes code harder to read.
You mean in general or this particular function?
Anyway, a part of motivation is to avoid dumb (even if easy to read in
isolation) getters and replace it with a one that exposes the logic.
(Now you have 2+ functions that reader needs to manually compare).
Re general oddities:
sizeof(void *) stuff just follows from the main idea, and is a temporary
solution (see this patch, it removes such).
>
>
>> }
>>
>> -static u64 mlxsw_sp1_kvdl_single_occ_get(void *priv)
>> -{
>> - const struct mlxsw_sp1_kvdl *kvdl = priv;
>> - struct mlxsw_sp1_kvdl_part *part;
>> -
>> - part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_SINGLE];
>> - return mlxsw_sp1_kvdl_part_occ(part);
>> -}
>> -
>> -static u64 mlxsw_sp1_kvdl_chunks_occ_get(void *priv)
>> -{
>> - const struct mlxsw_sp1_kvdl *kvdl = priv;
>> - struct mlxsw_sp1_kvdl_part *part;
>> -
>> - part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_CHUNKS];
>> - return mlxsw_sp1_kvdl_part_occ(part);
>> -}
>> -
>> -static u64 mlxsw_sp1_kvdl_large_chunks_occ_get(void *priv)
>> -{
>> - const struct mlxsw_sp1_kvdl *kvdl = priv;
>> - struct mlxsw_sp1_kvdl_part *part;
>> -
>> - part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_LARGE_CHUNKS];
>> - return mlxsw_sp1_kvdl_part_occ(part);
>> -}
>> -
>> static int mlxsw_sp1_kvdl_init(struct mlxsw_sp *mlxsw_sp, void *priv)
>> {
>> struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
>> - struct mlxsw_sp1_kvdl *kvdl = priv;
>> + struct mlxsw_sp1_kvdl_occ_ctx ctx = { priv };
>> int err;
>>
>> - err = mlxsw_sp1_kvdl_parts_init(mlxsw_sp, kvdl);
>> + err = mlxsw_sp1_kvdl_parts_init(mlxsw_sp, ctx.kvdl);
>> if (err)
>> return err;
>> - devl_resource_occ_get_register(devlink,
>> - MLXSW_SP_RESOURCE_KVD_LINEAR,
>> - mlxsw_sp1_kvdl_occ_get,
>> - &kvdl, sizeof(kvdl));
>> +
>> + ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_SINGLE;
>> devl_resource_occ_get_register(devlink,
>> MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
>> - mlxsw_sp1_kvdl_single_occ_get,
>> - &kvdl, sizeof(kvdl));
>> + mlxsw_sp1_kvdl_occ_get,
>> + &ctx, sizeof(ctx));
>> +
>> + ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_CHUNKS;
>> devl_resource_occ_get_register(devlink,
>> MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
>> - mlxsw_sp1_kvdl_chunks_occ_get,
>> - &kvdl, sizeof(kvdl));
>> + mlxsw_sp1_kvdl_occ_get,
>> + &ctx, sizeof(ctx));
>> +
>> + ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_LARGE_CHUNKS;
>> devl_resource_occ_get_register(devlink,
>> MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
>> - mlxsw_sp1_kvdl_large_chunks_occ_get,
>> - &kvdl, sizeof(kvdl));
>> + mlxsw_sp1_kvdl_occ_get,
>> + &ctx, sizeof(ctx));
>> +
>> + ctx.count_all_parts = true;
>> + devl_resource_occ_get_register(devlink,
>> + MLXSW_SP_RESOURCE_KVD_LINEAR,
>> + mlxsw_sp1_kvdl_occ_get,
>> + &ctx, sizeof(ctx));
>> +
>> return 0;
>> }
>>
>> @@ -400,7 +385,8 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
>> MLXSW_SP1_KVDL_SINGLE_SIZE,
>> MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
>> MLXSW_SP_RESOURCE_KVD_LINEAR,
>> - &size_params, sizeof(void *));
>> + &size_params,
>> + sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> if (err)
>> return err;
>>
>> @@ -411,7 +397,8 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
>> MLXSW_SP1_KVDL_CHUNKS_SIZE,
>> MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
>> MLXSW_SP_RESOURCE_KVD_LINEAR,
>> - &size_params, sizeof(void *));
>> + &size_params,
>> + sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> if (err)
>> return err;
>>
>> @@ -422,6 +409,7 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
>> MLXSW_SP1_KVDL_LARGE_CHUNKS_SIZE,
>> MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
>> MLXSW_SP_RESOURCE_KVD_LINEAR,
>> - &size_params, sizeof(void *));
>> + &size_params,
>> + sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> return err;
>> }
>> --
>> 2.39.3
>>
Powered by blists - more mailing lists