| 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: <Zroj0cPAoH3vQf6k@nanopsycho.orion>
Date: Mon, 12 Aug 2024 17:01:37 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
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
Mon, Aug 12, 2024 at 01:23:20PM CEST, przemyslaw.kitszel@...el.com wrote:
>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?
In general.
>
>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).
I don't follow. Are you not able to implement what you need using the
existing api, or you just don't like it?
>
>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