netdev - RE: [net-next 17/17] net/mlx5: Improve performance in SF allocation

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM6PR12MB4330F30E51E9D86F2AA212A7DC239@DM6PR12MB4330.namprd12.prod.outlook.com>
Date:   Thu, 27 May 2021 05:36:44 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Saeed Mahameed <saeed@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...dia.com>, Eli Cohen <elic@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>
Subject: RE: [net-next 17/17] net/mlx5: Improve performance in SF allocation


> From: Saeed Mahameed <saeed@...nel.org>
> Sent: Thursday, May 27, 2021 10:06 AM
> From: Eli Cohen <elic@...dia.com>
> 
> Avoid second traversal on the SF table by recording the first free entry and
> using it in case the looked up entry was not found in the table.
> 
> Signed-off-by: Eli Cohen <elic@...dia.com>
> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> ---
>  .../ethernet/mellanox/mlx5/core/sf/hw_table.c | 23 +++++++++++--------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
> b/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
> index ef5f892aafad..0c1fbf711fe6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
> @@ -74,26 +74,29 @@ static int mlx5_sf_hw_table_id_alloc(struct
> mlx5_sf_hw_table *table, u32 control
>  				     u32 usr_sfnum)
>  {
>  	struct mlx5_sf_hwc_table *hwc;
> +	int free_idx = -1;
>  	int i;
> 
>  	hwc = mlx5_sf_controller_to_hwc(table->dev, controller);
>  	if (!hwc->sfs)
>  		return -ENOSPC;
> 
> -	/* Check if sf with same sfnum already exists or not. */
> -	for (i = 0; i < hwc->max_fn; i++) {
> -		if (hwc->sfs[i].allocated && hwc->sfs[i].usr_sfnum ==
> usr_sfnum)
> -			return -EEXIST;
> -	}
> -	/* Find the free entry and allocate the entry from the array */
>  	for (i = 0; i < hwc->max_fn; i++) {
>  		if (!hwc->sfs[i].allocated) {
> -			hwc->sfs[i].usr_sfnum = usr_sfnum;
> -			hwc->sfs[i].allocated = true;
> -			return i;
It is supposed to return an allocated entry.
> +			free_idx = free_idx == -1 ? i : -1;
This is incorrect. On first attempt it allocates the free index.
On iterating second entry, free index is valid, so this condition is false and assigns -1 to free index, making it invalid again.
This leads to never able to allocate an SF situation.

> +			continue;
>  		}
> +
> +		if (hwc->sfs[i].usr_sfnum == usr_sfnum)
> +			return -EEXIST;
>  	}
> -	return -ENOSPC;
> +
> +	if (free_idx == -1)
> +		return -ENOSPC;
> +
> +	hwc->sfs[free_idx].usr_sfnum = usr_sfnum;
> +	hwc->sfs[free_idx].allocated = true;
> +	return 0;
This is incorrect. It must return the free_index and not zero.

>  }
> 
>  static void mlx5_sf_hw_table_id_free(struct mlx5_sf_hw_table *table, u32
> controller, int id)
> --
> 2.31.1

With this erroneous patch single SF allocation fails.
Our internal tests are failing too.
Please drop this patch from the series.

This patch needs below additional small changes, which I verified.

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
index 0c1fbf711fe6..9cc6ba7085a5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
@@ -82,12 +82,12 @@ static int mlx5_sf_hw_table_id_alloc(struct mlx5_sf_hw_table *table, u32 control
                return -ENOSPC;

        for (i = 0; i < hwc->max_fn; i++) {
-               if (!hwc->sfs[i].allocated) {
-                       free_idx = free_idx == -1 ? i : -1;
+               if (!hwc->sfs[i].allocated && free_idx == -1) {
+                       free_idx = i;
                        continue;
                }

-               if (hwc->sfs[i].usr_sfnum == usr_sfnum)
+               if (hwc->sfs[i].allocated && hwc->sfs[i].usr_sfnum == usr_sfnum)
                        return -EEXIST;
        }

@@ -96,7 +96,7 @@ static int mlx5_sf_hw_table_id_alloc(struct mlx5_sf_hw_table *table, u32 control

        hwc->sfs[free_idx].usr_sfnum = usr_sfnum;
        hwc->sfs[free_idx].allocated = true;
-       return 0;
+       return free_idx;
 }