netdev - Re: [net-next 06/12] net/mlx5: Refcount mlx5

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: <YRTKBw/q57G3erd9@unreal>
Date:   Thu, 12 Aug 2021 10:13:11 +0300
From:   Leon Romanovsky <leonro@...dia.com>
To:     Saeed Mahameed <saeed@...nel.org>
CC:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...dia.com>,
        Shay Drory <shayd@...dia.com>, Parav Pandit <parav@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>
Subject: Re: [net-next 06/12] net/mlx5: Refcount mlx5_irq with integer

On Wed, Aug 11, 2021 at 11:16:52AM -0700, Saeed Mahameed wrote:
> From: Shay Drory <shayd@...dia.com>
> 
> Currently, all access to mlx5 IRQs are done undere a lock. Hance, there
> isn't a reason to have kref in struct mlx5_irq.
> Switch it to integer.

Please fix spelling errors.

> 
> Signed-off-by: Shay Drory <shayd@...dia.com>
> Reviewed-by: Parav Pandit <parav@...dia.com>
> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> ---
>  .../net/ethernet/mellanox/mlx5/core/pci_irq.c | 65 +++++++++++++------
>  1 file changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c b/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
> index 717b9f1850ac..60bfcad1873c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
> @@ -32,7 +32,7 @@ struct mlx5_irq {
>  	cpumask_var_t mask;
>  	char name[MLX5_MAX_IRQ_NAME];
>  	struct mlx5_irq_pool *pool;
> -	struct kref kref;
> +	int refcount;

refcount has special meaning and semantics in the kernel.

>  	u32 index;
>  	int irqn;
>  };
> @@ -138,9 +138,8 @@ int mlx5_set_msix_vec_count(struct mlx5_core_dev *dev, int function_id,
>  	return ret;
>  }
>  
> -static void irq_release(struct kref *kref)
> +static void irq_release(struct mlx5_irq *irq)
>  {
> -	struct mlx5_irq *irq = container_of(kref, struct mlx5_irq, kref);
>  	struct mlx5_irq_pool *pool = irq->pool;
>  
>  	xa_erase(&pool->irqs, irq->index);
> @@ -159,10 +158,31 @@ static void irq_put(struct mlx5_irq *irq)
>  	struct mlx5_irq_pool *pool = irq->pool;
>  
>  	mutex_lock(&pool->lock);
> -	kref_put(&irq->kref, irq_release);
> +	irq->refcount--;
> +	if (!irq->refcount)
> +		irq_release(irq);
>  	mutex_unlock(&pool->lock);
>  }
>  
> +static int irq_get_locked(struct mlx5_irq *irq)
> +{
> +	lockdep_assert_held(&irq->pool->lock);
> +	if (WARN_ON_ONCE(!irq->refcount))
> +		return 0;
> +	irq->refcount++;
> +	return 1;
> +}
> +
> +static int irq_get(struct mlx5_irq *irq)
> +{
> +	int err;
> +
> +	mutex_lock(&irq->pool->lock);
> +	err = irq_get_locked(irq);
> +	mutex_unlock(&irq->pool->lock);
> +	return err;
> +}

>From not deep-dive review, all this "irq->pool->lock" is wrong.
The idea that you lock pool to change one entry can't be right.

So, I would invest time to clean locking here instead of removing kref.

Thanks