diff mbox series

[net-next,V3,04/11] net/mlx5: fs, add mlx5_fs_pool API

Message ID 20241218150949.1037752-5-tariqt@nvidia.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series mlx5 misc changes 2024-12-18 | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 4 maintainers not CCed: linux-rdma@vger.kernel.org gustavoars@kernel.org kees@kernel.org linux-hardening@vger.kernel.org
netdev/build_clang fail Errors and warnings before: 32 this patch: 33
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Tariq Toukan Dec. 18, 2024, 3:09 p.m. UTC 
From: Moshe Shemesh <moshe@nvidia.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@nvidia.com>
Reviewed-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.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

Comments

Przemek Kitszel Dec. 19, 2024, 9:17 a.m. UTC | #1 
On 12/18/24 16:09, Tariq Toukan wrote:
> From: Moshe Shemesh <moshe@nvidia.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@nvidia.com>
> Reviewed-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
> Reviewed-by: Mark Bloch <mbloch@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.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);
Moshe Shemesh Dec. 19, 2024, 12:30 p.m. UTC | #2 
On 12/19/2024 11:17 AM, Przemek Kitszel wrote:
> 
> On 12/18/24 16:09, Tariq Toukan wrote:
>> From: Moshe Shemesh <moshe@nvidia.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@nvidia.com>
>> Reviewed-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
>> Reviewed-by: Mark Bloch <mbloch@nvidia.com>
>> Signed-off-by: Tariq Toukan <tariqt@nvidia.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);
>
Jakub Kicinski Dec. 19, 2024, 3:12 p.m. UTC | #3 
On Thu, 19 Dec 2024 14:30:41 +0200 Moshe Shemesh wrote:
> > 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 ?

Yes, there's a consensus now. But I think since all mlx* code uses
the "jump source" naming mixing the two could lead to confusion for
your internal developers, and bugs. So I'd say up to you :(
Przemek Kitszel Dec. 19, 2024, 3:36 p.m. UTC | #4 
On 12/19/24 13:30, Moshe Shemesh wrote:
> 
> 
> On 12/19/2024 11:17 AM, Przemek Kitszel wrote:
>>
>> On 12/18/24 16:09, Tariq Toukan wrote:
>>> From: Moshe Shemesh <moshe@nvidia.com>


>>> +     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 ?

Would be great, but do we really need to open naming things for dispute/
call for moderation? (To give even more trival example: I always push
people to not name variable "status" when "err" is better, should that
be in the official doc? xD)

(to be clear - I'm fine with setting "the rule" for new code, to don't
mandate change for things already in-flight)

--
Re convincing: I would say that "came from" labels have almost no
benefits, and according to wikipedia, people make fun of them since '70
https://en.wikipedia.org/wiki/COMEFROM

It's hard to make a good came-from name for a label that needs to be
jumped-to multiple times.
Such labels are also disconnected from the code under them.

On the plus side I see only "a bit smaller diff" in some cases. But that
hurts when you just move a call with it's error handling, because, due
to name, it looks fine in the new place, but it obviously isn't.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index be3d0876c521..79fe09de0a9f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -17,7 +17,7 @@  mlx5_core-y :=	main.o cmd.o debugfs.o fw.o eq.o uar.o pagealloc.o \
 		fs_counters.o fs_ft_pool.o rl.o lag/debugfs.o lag/lag.o dev.o events.o wq.o lib/gid.o \
 		lib/devcom.o lib/pci_vsc.o lib/dm.o lib/fs_ttc.o diag/fs_tracepoint.o \
 		diag/fw_tracer.o diag/crdump.o devlink.o diag/rsc_dump.o diag/reporter_vnic.o \
-		fw_reset.o qos.o lib/tout.o lib/aso.o wc.o
+		fw_reset.o qos.o lib/tout.o lib/aso.o wc.o fs_pool.o
 
 #
 # Netdev basic
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
index e95488ed1547..a693c5c792b3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
@@ -34,6 +34,7 @@ 
 #include <linux/mlx5/fs.h>
 #include "mlx5_core.h"
 #include "fs_core.h"
+#include "fs_pool.h"
 #include "fs_cmd.h"
 
 #define MLX5_FC_STATS_PERIOD msecs_to_jiffies(1000)
@@ -65,17 +66,6 @@  struct mlx5_fc {
 	u64 lastbytes;
 };
 
-struct mlx5_fc_pool {
-	struct mlx5_core_dev *dev;
-	struct mutex pool_lock; /* protects pool lists */
-	struct list_head fully_used;
-	struct list_head partially_used;
-	struct list_head unused;
-	int available_fcs;
-	int used_fcs;
-	int threshold;
-};
-
 struct mlx5_fc_stats {
 	struct xarray counters;
 
@@ -86,13 +76,13 @@  struct mlx5_fc_stats {
 	int bulk_query_len;
 	bool bulk_query_alloc_failed;
 	unsigned long next_bulk_query_alloc;
-	struct mlx5_fc_pool fc_pool;
+	struct mlx5_fs_pool fc_pool;
 };
 
-static void mlx5_fc_pool_init(struct mlx5_fc_pool *fc_pool, struct mlx5_core_dev *dev);
-static void mlx5_fc_pool_cleanup(struct mlx5_fc_pool *fc_pool);
-static struct mlx5_fc *mlx5_fc_pool_acquire_counter(struct mlx5_fc_pool *fc_pool);
-static void mlx5_fc_pool_release_counter(struct mlx5_fc_pool *fc_pool, struct mlx5_fc *fc);
+static void mlx5_fc_pool_init(struct mlx5_fs_pool *fc_pool, struct mlx5_core_dev *dev);
+static void mlx5_fc_pool_cleanup(struct mlx5_fs_pool *fc_pool);
+static struct mlx5_fc *mlx5_fc_pool_acquire_counter(struct mlx5_fs_pool *fc_pool);
+static void mlx5_fc_pool_release_counter(struct mlx5_fs_pool *fc_pool, struct mlx5_fc *fc);
 
 static int get_init_bulk_query_len(struct mlx5_core_dev *dev)
 {
@@ -447,11 +437,9 @@  void mlx5_fc_update_sampling_interval(struct mlx5_core_dev *dev,
 /* Flow counter bluks */
 
 struct mlx5_fc_bulk {
-	struct list_head pool_list;
+	struct mlx5_fs_bulk fs_bulk;
 	u32 base_id;
-	int bulk_len;
-	unsigned long *bitmask;
-	struct mlx5_fc fcs[] __counted_by(bulk_len);
+	struct mlx5_fc fcs[];
 };
 
 static void mlx5_fc_init(struct mlx5_fc *counter, struct mlx5_fc_bulk *bulk,
@@ -461,16 +449,10 @@  static void mlx5_fc_init(struct mlx5_fc *counter, struct mlx5_fc_bulk *bulk,
 	counter->id = id;
 }
 
-static int mlx5_fc_bulk_get_free_fcs_amount(struct mlx5_fc_bulk *bulk)
-{
-	return bitmap_weight(bulk->bitmask, bulk->bulk_len);
-}
-
-static struct mlx5_fc_bulk *mlx5_fc_bulk_create(struct mlx5_core_dev *dev)
+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;
 
-	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:
+	mlx5_fs_bulk_cleanup(&fc_bulk->fs_bulk);
+err_fs_bulk_init:
+	kvfree(fc_bulk);
+	return NULL;
 }
 
 static int
-mlx5_fc_bulk_destroy(struct mlx5_core_dev *dev, struct mlx5_fc_bulk *bulk)
+mlx5_fc_bulk_destroy(struct mlx5_core_dev *dev, struct mlx5_fs_bulk *fs_bulk)
 {
-	if (mlx5_fc_bulk_get_free_fcs_amount(bulk) < bulk->bulk_len) {
+	struct mlx5_fc_bulk *fc_bulk = container_of(fs_bulk,
+						    struct mlx5_fc_bulk,
+						    fs_bulk);
+
+	if (mlx5_fs_bulk_get_free_amount(fs_bulk) < fs_bulk->bulk_len) {
 		mlx5_core_err(dev, "Freeing bulk before all counters were released\n");
 		return -EBUSY;
 	}
 
-	mlx5_cmd_fc_free(dev, bulk->base_id);
-	kvfree(bulk->bitmask);
-	kvfree(bulk);
+	mlx5_cmd_fc_free(dev, fc_bulk->base_id);
+	mlx5_fs_bulk_cleanup(fs_bulk);
+	kvfree(fc_bulk);
 
 	return 0;
 }
 
-static struct mlx5_fc *mlx5_fc_bulk_acquire_fc(struct mlx5_fc_bulk *bulk)
+static void mlx5_fc_pool_update_threshold(struct mlx5_fs_pool *fc_pool)
 {
-	int free_fc_index = find_first_bit(bulk->bitmask, bulk->bulk_len);
+	fc_pool->threshold = min_t(int, MLX5_FC_POOL_MAX_THRESHOLD,
+				   fc_pool->used_units / MLX5_FC_POOL_USED_BUFF_RATIO);
+}
 
-	if (free_fc_index >= bulk->bulk_len)
-		return ERR_PTR(-ENOSPC);
+/* Flow counters pool API */
+
+static const struct mlx5_fs_pool_ops mlx5_fc_pool_ops = {
+	.bulk_destroy = mlx5_fc_bulk_destroy,
+	.bulk_create = mlx5_fc_bulk_create,
+	.update_threshold = mlx5_fc_pool_update_threshold,
+};
 
-	clear_bit(free_fc_index, bulk->bitmask);
-	return &bulk->fcs[free_fc_index];
+static void
+mlx5_fc_pool_init(struct mlx5_fs_pool *fc_pool, struct mlx5_core_dev *dev)
+{
+	mlx5_fs_pool_init(fc_pool, dev, &mlx5_fc_pool_ops);
 }
 
-static int mlx5_fc_bulk_release_fc(struct mlx5_fc_bulk *bulk, struct mlx5_fc *fc)
+static void mlx5_fc_pool_cleanup(struct mlx5_fs_pool *fc_pool)
 {
-	int fc_index = fc->id - bulk->base_id;
+	mlx5_fs_pool_cleanup(fc_pool);
+}
 
-	if (test_bit(fc_index, bulk->bitmask))
-		return -EINVAL;
+static struct mlx5_fc *
+mlx5_fc_pool_acquire_counter(struct mlx5_fs_pool *fc_pool)
+{
+	struct mlx5_fs_pool_index pool_index = {};
+	struct mlx5_fc_bulk *fc_bulk;
+	int err;
 
-	set_bit(fc_index, bulk->bitmask);
-	return 0;
+	err = mlx5_fs_pool_acquire_index(fc_pool, &pool_index);
+	if (err)
+		return ERR_PTR(err);
+	fc_bulk = container_of(pool_index.fs_bulk, struct mlx5_fc_bulk, fs_bulk);
+	return &fc_bulk->fcs[pool_index.index];
+}
+
+static void
+mlx5_fc_pool_release_counter(struct mlx5_fs_pool *fc_pool, struct mlx5_fc *fc)
+{
+	struct mlx5_fs_bulk *fs_bulk = &fc->bulk->fs_bulk;
+	struct mlx5_fs_pool_index pool_index = {};
+	struct mlx5_core_dev *dev = fc_pool->dev;
+
+	pool_index.fs_bulk = fs_bulk;
+	pool_index.index = fc->id - fc->bulk->base_id;
+	if (mlx5_fs_pool_release_index(fc_pool, &pool_index))
+		mlx5_core_warn(dev, "Attempted to release a counter which is not acquired\n");
 }
 
 /**
@@ -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;
 	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);
@@ -587,141 +595,3 @@  void mlx5_fc_local_destroy(struct mlx5_fc *counter)
 	kfree(counter);
 }
 EXPORT_SYMBOL(mlx5_fc_local_destroy);
-
-/* Flow counters pool API */
-
-static void mlx5_fc_pool_init(struct mlx5_fc_pool *fc_pool, struct mlx5_core_dev *dev)
-{
-	fc_pool->dev = dev;
-	mutex_init(&fc_pool->pool_lock);
-	INIT_LIST_HEAD(&fc_pool->fully_used);
-	INIT_LIST_HEAD(&fc_pool->partially_used);
-	INIT_LIST_HEAD(&fc_pool->unused);
-	fc_pool->available_fcs = 0;
-	fc_pool->used_fcs = 0;
-	fc_pool->threshold = 0;
-}
-
-static void mlx5_fc_pool_cleanup(struct mlx5_fc_pool *fc_pool)
-{
-	struct mlx5_core_dev *dev = fc_pool->dev;
-	struct mlx5_fc_bulk *bulk;
-	struct mlx5_fc_bulk *tmp;
-
-	list_for_each_entry_safe(bulk, tmp, &fc_pool->fully_used, pool_list)
-		mlx5_fc_bulk_destroy(dev, bulk);
-	list_for_each_entry_safe(bulk, tmp, &fc_pool->partially_used, pool_list)
-		mlx5_fc_bulk_destroy(dev, bulk);
-	list_for_each_entry_safe(bulk, tmp, &fc_pool->unused, pool_list)
-		mlx5_fc_bulk_destroy(dev, bulk);
-}
-
-static void mlx5_fc_pool_update_threshold(struct mlx5_fc_pool *fc_pool)
-{
-	fc_pool->threshold = min_t(int, MLX5_FC_POOL_MAX_THRESHOLD,
-				   fc_pool->used_fcs / MLX5_FC_POOL_USED_BUFF_RATIO);
-}
-
-static struct mlx5_fc_bulk *
-mlx5_fc_pool_alloc_new_bulk(struct mlx5_fc_pool *fc_pool)
-{
-	struct mlx5_core_dev *dev = fc_pool->dev;
-	struct mlx5_fc_bulk *new_bulk;
-
-	new_bulk = mlx5_fc_bulk_create(dev);
-	if (!IS_ERR(new_bulk))
-		fc_pool->available_fcs += new_bulk->bulk_len;
-	mlx5_fc_pool_update_threshold(fc_pool);
-	return new_bulk;
-}
-
-static void
-mlx5_fc_pool_free_bulk(struct mlx5_fc_pool *fc_pool, struct mlx5_fc_bulk *bulk)
-{
-	struct mlx5_core_dev *dev = fc_pool->dev;
-
-	fc_pool->available_fcs -= bulk->bulk_len;
-	mlx5_fc_bulk_destroy(dev, bulk);
-	mlx5_fc_pool_update_threshold(fc_pool);
-}
-
-static struct mlx5_fc *
-mlx5_fc_pool_acquire_from_list(struct list_head *src_list,
-			       struct list_head *next_list,
-			       bool move_non_full_bulk)
-{
-	struct mlx5_fc_bulk *bulk;
-	struct mlx5_fc *fc;
-
-	if (list_empty(src_list))
-		return ERR_PTR(-ENODATA);
-
-	bulk = list_first_entry(src_list, struct mlx5_fc_bulk, pool_list);
-	fc = mlx5_fc_bulk_acquire_fc(bulk);
-	if (move_non_full_bulk || mlx5_fc_bulk_get_free_fcs_amount(bulk) == 0)
-		list_move(&bulk->pool_list, next_list);
-	return fc;
-}
-
-static struct mlx5_fc *
-mlx5_fc_pool_acquire_counter(struct mlx5_fc_pool *fc_pool)
-{
-	struct mlx5_fc_bulk *new_bulk;
-	struct mlx5_fc *fc;
-
-	mutex_lock(&fc_pool->pool_lock);
-
-	fc = mlx5_fc_pool_acquire_from_list(&fc_pool->partially_used,
-					    &fc_pool->fully_used, false);
-	if (IS_ERR(fc))
-		fc = mlx5_fc_pool_acquire_from_list(&fc_pool->unused,
-						    &fc_pool->partially_used,
-						    true);
-	if (IS_ERR(fc)) {
-		new_bulk = mlx5_fc_pool_alloc_new_bulk(fc_pool);
-		if (IS_ERR(new_bulk)) {
-			fc = ERR_CAST(new_bulk);
-			goto out;
-		}
-		fc = mlx5_fc_bulk_acquire_fc(new_bulk);
-		list_add(&new_bulk->pool_list, &fc_pool->partially_used);
-	}
-	fc_pool->available_fcs--;
-	fc_pool->used_fcs++;
-
-out:
-	mutex_unlock(&fc_pool->pool_lock);
-	return fc;
-}
-
-static void
-mlx5_fc_pool_release_counter(struct mlx5_fc_pool *fc_pool, struct mlx5_fc *fc)
-{
-	struct mlx5_core_dev *dev = fc_pool->dev;
-	struct mlx5_fc_bulk *bulk = fc->bulk;
-	int bulk_free_fcs_amount;
-
-	mutex_lock(&fc_pool->pool_lock);
-
-	if (mlx5_fc_bulk_release_fc(bulk, fc)) {
-		mlx5_core_warn(dev, "Attempted to release a counter which is not acquired\n");
-		goto unlock;
-	}
-
-	fc_pool->available_fcs++;
-	fc_pool->used_fcs--;
-
-	bulk_free_fcs_amount = mlx5_fc_bulk_get_free_fcs_amount(bulk);
-	if (bulk_free_fcs_amount == 1)
-		list_move_tail(&bulk->pool_list, &fc_pool->partially_used);
-	if (bulk_free_fcs_amount == bulk->bulk_len) {
-		list_del(&bulk->pool_list);
-		if (fc_pool->available_fcs > fc_pool->threshold)
-			mlx5_fc_pool_free_bulk(fc_pool, bulk);
-		else
-			list_add(&bulk->pool_list, &fc_pool->unused);
-	}
-
-unlock:
-	mutex_unlock(&fc_pool->pool_lock);
-}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_pool.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_pool.c
new file mode 100644
index 000000000000..b891d7b9e3e0
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_pool.c
@@ -0,0 +1,194 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+/* Copyright (c) 2024 NVIDIA Corporation & Affiliates */
+
+#include <mlx5_core.h>
+#include "fs_pool.h"
+
+int mlx5_fs_bulk_init(struct mlx5_core_dev *dev, struct mlx5_fs_bulk *fs_bulk,
+		      int bulk_len)
+{
+	int i;
+
+	fs_bulk->bitmask = kvcalloc(BITS_TO_LONGS(bulk_len), sizeof(unsigned long),
+				    GFP_KERNEL);
+	if (!fs_bulk->bitmask)
+		return -ENOMEM;
+
+	fs_bulk->bulk_len = bulk_len;
+	for (i = 0; i < bulk_len; i++)
+		set_bit(i, fs_bulk->bitmask);
+
+	return 0;
+}
+
+void mlx5_fs_bulk_cleanup(struct mlx5_fs_bulk *fs_bulk)
+{
+	kvfree(fs_bulk->bitmask);
+}
+
+int mlx5_fs_bulk_get_free_amount(struct mlx5_fs_bulk *bulk)
+{
+	return bitmap_weight(bulk->bitmask, bulk->bulk_len);
+}
+
+static int mlx5_fs_bulk_acquire_index(struct mlx5_fs_bulk *fs_bulk,
+				      struct mlx5_fs_pool_index *pool_index)
+{
+	int free_index = find_first_bit(fs_bulk->bitmask, fs_bulk->bulk_len);
+
+	WARN_ON_ONCE(!pool_index || !fs_bulk);
+	if (free_index >= fs_bulk->bulk_len)
+		return -ENOSPC;
+
+	clear_bit(free_index, fs_bulk->bitmask);
+	pool_index->fs_bulk = fs_bulk;
+	pool_index->index = free_index;
+	return 0;
+}
+
+static int mlx5_fs_bulk_release_index(struct mlx5_fs_bulk *fs_bulk, int index)
+{
+	if (test_bit(index, fs_bulk->bitmask))
+		return -EINVAL;
+
+	set_bit(index, fs_bulk->bitmask);
+	return 0;
+}
+
+void mlx5_fs_pool_init(struct mlx5_fs_pool *pool, struct mlx5_core_dev *dev,
+		       const struct mlx5_fs_pool_ops *ops)
+{
+	WARN_ON_ONCE(!ops || !ops->bulk_destroy || !ops->bulk_create ||
+		     !ops->update_threshold);
+	pool->dev = dev;
+	mutex_init(&pool->pool_lock);
+	INIT_LIST_HEAD(&pool->fully_used);
+	INIT_LIST_HEAD(&pool->partially_used);
+	INIT_LIST_HEAD(&pool->unused);
+	pool->available_units = 0;
+	pool->used_units = 0;
+	pool->threshold = 0;
+	pool->ops = ops;
+}
+
+void mlx5_fs_pool_cleanup(struct mlx5_fs_pool *pool)
+{
+	struct mlx5_core_dev *dev = pool->dev;
+	struct mlx5_fs_bulk *bulk;
+	struct mlx5_fs_bulk *tmp;
+
+	list_for_each_entry_safe(bulk, tmp, &pool->fully_used, pool_list)
+		pool->ops->bulk_destroy(dev, bulk);
+	list_for_each_entry_safe(bulk, tmp, &pool->partially_used, pool_list)
+		pool->ops->bulk_destroy(dev, bulk);
+	list_for_each_entry_safe(bulk, tmp, &pool->unused, pool_list)
+		pool->ops->bulk_destroy(dev, bulk);
+}
+
+static struct mlx5_fs_bulk *
+mlx5_fs_pool_alloc_new_bulk(struct mlx5_fs_pool *fs_pool)
+{
+	struct mlx5_core_dev *dev = fs_pool->dev;
+	struct mlx5_fs_bulk *new_bulk;
+
+	new_bulk = fs_pool->ops->bulk_create(dev);
+	if (new_bulk)
+		fs_pool->available_units += new_bulk->bulk_len;
+	fs_pool->ops->update_threshold(fs_pool);
+	return new_bulk;
+}
+
+static void
+mlx5_fs_pool_free_bulk(struct mlx5_fs_pool *fs_pool, struct mlx5_fs_bulk *bulk)
+{
+	struct mlx5_core_dev *dev = fs_pool->dev;
+
+	fs_pool->available_units -= bulk->bulk_len;
+	fs_pool->ops->bulk_destroy(dev, bulk);
+	fs_pool->ops->update_threshold(fs_pool);
+}
+
+static int
+mlx5_fs_pool_acquire_from_list(struct list_head *src_list,
+			       struct list_head *next_list,
+			       bool move_non_full_bulk,
+			       struct mlx5_fs_pool_index *pool_index)
+{
+	struct mlx5_fs_bulk *fs_bulk;
+	int err;
+
+	if (list_empty(src_list))
+		return -ENODATA;
+
+	fs_bulk = list_first_entry(src_list, struct mlx5_fs_bulk, pool_list);
+	err = mlx5_fs_bulk_acquire_index(fs_bulk, pool_index);
+	if (move_non_full_bulk || mlx5_fs_bulk_get_free_amount(fs_bulk) == 0)
+		list_move(&fs_bulk->pool_list, next_list);
+	return err;
+}
+
+int mlx5_fs_pool_acquire_index(struct mlx5_fs_pool *fs_pool,
+			       struct mlx5_fs_pool_index *pool_index)
+{
+	struct mlx5_fs_bulk *new_bulk;
+	int err;
+
+	mutex_lock(&fs_pool->pool_lock);
+
+	err = mlx5_fs_pool_acquire_from_list(&fs_pool->partially_used,
+					     &fs_pool->fully_used, false,
+					     pool_index);
+	if (err)
+		err = mlx5_fs_pool_acquire_from_list(&fs_pool->unused,
+						     &fs_pool->partially_used,
+						     true, pool_index);
+	if (err) {
+		new_bulk = mlx5_fs_pool_alloc_new_bulk(fs_pool);
+		if (!new_bulk) {
+			err = -ENOENT;
+			goto out;
+		}
+		err = mlx5_fs_bulk_acquire_index(new_bulk, pool_index);
+		WARN_ON_ONCE(err);
+		list_add(&new_bulk->pool_list, &fs_pool->partially_used);
+	}
+	fs_pool->available_units--;
+	fs_pool->used_units++;
+
+out:
+	mutex_unlock(&fs_pool->pool_lock);
+	return err;
+}
+
+int mlx5_fs_pool_release_index(struct mlx5_fs_pool *fs_pool,
+			       struct mlx5_fs_pool_index *pool_index)
+{
+	struct mlx5_fs_bulk *bulk = pool_index->fs_bulk;
+	int bulk_free_amount;
+	int err;
+
+	mutex_lock(&fs_pool->pool_lock);
+
+	/* TBD would rather return void if there was no warn here in original code */
+	err = mlx5_fs_bulk_release_index(bulk, pool_index->index);
+	if (err)
+		goto unlock;
+
+	fs_pool->available_units++;
+	fs_pool->used_units--;
+
+	bulk_free_amount = mlx5_fs_bulk_get_free_amount(bulk);
+	if (bulk_free_amount == 1)
+		list_move_tail(&bulk->pool_list, &fs_pool->partially_used);
+	if (bulk_free_amount == bulk->bulk_len) {
+		list_del(&bulk->pool_list);
+		if (fs_pool->available_units > fs_pool->threshold)
+			mlx5_fs_pool_free_bulk(fs_pool, bulk);
+		else
+			list_add(&bulk->pool_list, &fs_pool->unused);
+	}
+
+unlock:
+	mutex_unlock(&fs_pool->pool_lock);
+	return err;
+}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_pool.h b/drivers/net/ethernet/mellanox/mlx5/core/fs_pool.h
new file mode 100644
index 000000000000..3b149863260c
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_pool.h
@@ -0,0 +1,54 @@ 
+/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
+/* Copyright (c) 2024 NVIDIA Corporation & Affiliates */
+
+#ifndef __MLX5_FS_POOL_H__
+#define __MLX5_FS_POOL_H__
+
+#include <linux/mlx5/driver.h>
+
+struct mlx5_fs_bulk {
+	struct list_head pool_list;
+	int bulk_len;
+	unsigned long *bitmask;
+};
+
+struct mlx5_fs_pool_index {
+	struct mlx5_fs_bulk *fs_bulk;
+	int index;
+};
+
+struct mlx5_fs_pool;
+
+struct mlx5_fs_pool_ops {
+	int (*bulk_destroy)(struct mlx5_core_dev *dev, struct mlx5_fs_bulk *bulk);
+	struct mlx5_fs_bulk * (*bulk_create)(struct mlx5_core_dev *dev);
+	void (*update_threshold)(struct mlx5_fs_pool *pool);
+};
+
+struct mlx5_fs_pool {
+	struct mlx5_core_dev *dev;
+	void *pool_ctx;
+	const struct mlx5_fs_pool_ops *ops;
+	struct mutex pool_lock; /* protects pool lists */
+	struct list_head fully_used;
+	struct list_head partially_used;
+	struct list_head unused;
+	int available_units;
+	int used_units;
+	int threshold;
+};
+
+int mlx5_fs_bulk_init(struct mlx5_core_dev *dev, struct mlx5_fs_bulk *fs_bulk,
+		      int bulk_len);
+void mlx5_fs_bulk_cleanup(struct mlx5_fs_bulk *fs_bulk);
+int mlx5_fs_bulk_get_free_amount(struct mlx5_fs_bulk *bulk);
+
+void mlx5_fs_pool_init(struct mlx5_fs_pool *pool, struct mlx5_core_dev *dev,
+		       const struct mlx5_fs_pool_ops *ops);
+void mlx5_fs_pool_cleanup(struct mlx5_fs_pool *pool);
+int mlx5_fs_pool_acquire_index(struct mlx5_fs_pool *fs_pool,
+			       struct mlx5_fs_pool_index *pool_index);
+int mlx5_fs_pool_release_index(struct mlx5_fs_pool *fs_pool,
+			       struct mlx5_fs_pool_index *pool_index);
+
+#endif /* __MLX5_FS_POOL_H__ */