[PATCH for-next] RDMA/efa: Limit EQs to available MSI-X vectors

[PATCH for-next] RDMA/efa: Limit EQs to available MSI-X vectors

[ynachum]

From: Yonatan Nachum <ynachum@amazon.com>

When creating EQs we take into consideration the max number of EQs the
device reported it can support and the number of available CPUs. There
are situations where the number of EQs the device reported it can
support and the PCI configuration of MSI-X is different, take it in
account as well when creating EQs.
Also request at least 1 MSI-X vector for the management queue and allow
the kernel to return a number of vectors in a range between 1 and the
max supported MSI-X vectors according to the PCI config.

Reviewed-by: Michael Margolin <mrgolin@amazon.com>
Reviewed-by: Yonatan Goldhirsh <ygold@amazon.com>
Signed-off-by: Yonatan Nachum <ynachum@amazon.com>
---
 drivers/infiniband/hw/efa/efa.h      |  3 ++-
 drivers/infiniband/hw/efa/efa_main.c | 32 +++++++++++++---------------
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h
index 7352a1f5d811..a17045e100cd 100644
--- a/drivers/infiniband/hw/efa/efa.h
+++ b/drivers/infiniband/hw/efa/efa.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
 /*
- * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
+ * Copyright 2018-2023 Amazon.com, Inc. or its affiliates. All rights reserved.
  */
 
 #ifndef _EFA_H_
@@ -57,6 +57,7 @@ struct efa_dev {
 	u64 db_bar_addr;
 	u64 db_bar_len;
 
+	unsigned int num_irq_vectors;
 	int admin_msix_vector_idx;
 	struct efa_irq admin_irq;
 
diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 15ee92081118..1aade398c723 100644
--- a/drivers/infiniband/hw/efa/efa_main.c
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -319,7 +319,9 @@ static int efa_create_eqs(struct efa_dev *dev)
 	int err;
 	int i;
 
-	neqs = min_t(unsigned int, neqs, num_online_cpus());
+	neqs = min_t(unsigned int, neqs,
+		     dev->num_irq_vectors - EFA_COMP_EQS_VEC_BASE);
+
 	dev->neqs = neqs;
 	dev->eqs = kcalloc(neqs, sizeof(*dev->eqs), GFP_KERNEL);
 	if (!dev->eqs)
@@ -463,34 +465,30 @@ static void efa_disable_msix(struct efa_dev *dev)
 
 static int efa_enable_msix(struct efa_dev *dev)
 {
-	int msix_vecs, irq_num;
+	int max_vecs, num_vecs;
 
 	/*
 	 * Reserve the max msix vectors we might need, one vector is reserved
 	 * for admin.
 	 */
-	msix_vecs = min_t(int, pci_msix_vec_count(dev->pdev),
-			  num_online_cpus() + 1);
+	max_vecs = min_t(int, pci_msix_vec_count(dev->pdev),
+			 num_online_cpus() + 1);
 	dev_dbg(&dev->pdev->dev, "Trying to enable MSI-X, vectors %d\n",
-		msix_vecs);
+		max_vecs);
 
 	dev->admin_msix_vector_idx = EFA_MGMNT_MSIX_VEC_IDX;
-	irq_num = pci_alloc_irq_vectors(dev->pdev, msix_vecs,
-					msix_vecs, PCI_IRQ_MSIX);
+	num_vecs = pci_alloc_irq_vectors(dev->pdev, 1,
+					 max_vecs, PCI_IRQ_MSIX);
 
-	if (irq_num < 0) {
-		dev_err(&dev->pdev->dev, "Failed to enable MSI-X. irq_num %d\n",
-			irq_num);
+	if (num_vecs < 0) {
+		dev_err(&dev->pdev->dev, "Failed to enable MSI-X. error %d\n",
+			num_vecs);
 		return -ENOSPC;
 	}
 
-	if (irq_num != msix_vecs) {
-		efa_disable_msix(dev);
-		dev_err(&dev->pdev->dev,
-			"Allocated %d MSI-X (out of %d requested)\n",
-			irq_num, msix_vecs);
-		return -ENOSPC;
-	}
+	dev_dbg(&dev->pdev->dev, "Allocated %d MSI-X vectors\n", num_vecs);
+
+	dev->num_irq_vectors = num_vecs;
 
 	return 0;
 }
-- 
2.40.1

Re: [PATCH for-next] RDMA/efa: Limit EQs to available MSI-X vectors

[Gal Pressman]

Hi Yonatan,

On 03/01/2024 16:21, ynachum@amazon.com wrote:
> From: Yonatan Nachum <ynachum@amazon.com>
> 
> When creating EQs we take into consideration the max number of EQs the
> device reported it can support and the number of available CPUs. There
> are situations where the number of EQs the device reported it can
> support and the PCI configuration of MSI-X is different, take it in
> account as well when creating EQs.
> Also request at least 1 MSI-X vector for the management queue and allow
> the kernel to return a number of vectors in a range between 1 and the
> max supported MSI-X vectors according to the PCI config.
> 
> Reviewed-by: Michael Margolin <mrgolin@amazon.com>
> Reviewed-by: Yonatan Goldhirsh <ygold@amazon.com>
> Signed-off-by: Yonatan Nachum <ynachum@amazon.com>
> ---
>  drivers/infiniband/hw/efa/efa.h      |  3 ++-
>  drivers/infiniband/hw/efa/efa_main.c | 32 +++++++++++++---------------
>  2 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h
> index 7352a1f5d811..a17045e100cd 100644
> --- a/drivers/infiniband/hw/efa/efa.h
> +++ b/drivers/infiniband/hw/efa/efa.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
>  /*
> - * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
> + * Copyright 2018-2023 Amazon.com, Inc. or its affiliates. All rights reserved.
>   */
>  
>  #ifndef _EFA_H_
> @@ -57,6 +57,7 @@ struct efa_dev {
>  	u64 db_bar_addr;
>  	u64 db_bar_len;
>  
> +	unsigned int num_irq_vectors;
>  	int admin_msix_vector_idx;
>  	struct efa_irq admin_irq;
>  
> diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
> index 15ee92081118..1aade398c723 100644
> --- a/drivers/infiniband/hw/efa/efa_main.c
> +++ b/drivers/infiniband/hw/efa/efa_main.c
> @@ -319,7 +319,9 @@ static int efa_create_eqs(struct efa_dev *dev)
>  	int err;
>  	int i;
>  
> -	neqs = min_t(unsigned int, neqs, num_online_cpus());
> +	neqs = min_t(unsigned int, neqs,
> +		     dev->num_irq_vectors - EFA_COMP_EQS_VEC_BASE);
> +

If the device supports one msix (which is reserved for commands) you'll
end up with zero neqs, and allocate a zero-sized dev->eqs array.

Won't that break when efa_create_cq() is called and try to access this
array?
Especially since efa_ib_device_add() sets num_comp_vectors to 1 in such
case..

>  	dev->neqs = neqs;
>  	dev->eqs = kcalloc(neqs, sizeof(*dev->eqs), GFP_KERNEL);
>  	if (!dev->eqs)

Re: [PATCH for-next] RDMA/efa: Limit EQs to available MSI-X vectors

[Nachum, Yonatan]

>> diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
>> index 15ee92081118..1aade398c723 100644
>> --- a/drivers/infiniband/hw/efa/efa_main.c
>> +++ b/drivers/infiniband/hw/efa/efa_main.c
>> @@ -319,7 +319,9 @@ static int efa_create_eqs(struct efa_dev *dev)
>>       int err;
>>       int i;
>>
>> -     neqs = min_t(unsigned int, neqs, num_online_cpus());
>> +     neqs = min_t(unsigned int, neqs,
>> +                  dev->num_irq_vectors - EFA_COMP_EQS_VEC_BASE);
>> +
> If the device supports one msix (which is reserved for commands) you'll
> end up with zero neqs, and allocate a zero-sized dev->eqs array.
>
> Won't that break when efa_create_cq() is called and try to access this
> array?
> Especially since efa_ib_device_add() sets num_comp_vectors to 1 in such
> case..
>
>>       dev->neqs = neqs;
>>       dev->eqs = kcalloc(neqs, sizeof(*dev->eqs), GFP_KERNEL);
>>       if (!dev->eqs)

When the number of EQs is 0 we don't report EFA_QUERY_DEVICE_CAPS_CQ_NOTIFICATIONS
to the upper layer (rdma-core for example) so it won't be able to request the driver for
CQs with interrupts enabled. So in terms of behavior we keep the same behavior as older driver
versions.

I will need create a separate patch for num_comp_vectors to represent the number of EQs even if its 0 and add driver protection for out of bounds reach to the EQ array. Thanks.