EFA CQ notifications

PATCH Applied Gal Pressman 3 participants 27 messages Aug 11, 2021 – Sep 9, 2021

Subsystems (2)

InfiniBand Amazon RDMA efa driver

lore

Share & tools

lore

Gal Pressman

author

2021-08-11 15:12 UTC #0 lore raw

cover EFA CQ notifications

This series adds support for CQ notifications through the verbs api (ibv_req_notify_cq/ibv_get_cq_event).

In order to achieve that, a new event queue (EQ) object is introduced, which is in charge of reporting completion events to the driver.

In addition, a new CQ doorbell is introduced that is mmapped to the userspace provider in order to arm the CQ when requested by the user.

PR was sent:
https://github.com/linux-rdma/rdma-core/pull/1044

Thanks,
Gal

Gal Pressman (4):
RDMA/efa: Free IRQ vectors on error flow RDMA/efa: Remove unused cpu field from irq struct RDMA/efa: Rename vector field in efa_irq struct to irqn RDMA/efa: CQ notifications

 drivers/infiniband/hw/efa/efa.h               |  19 +-
 .../infiniband/hw/efa/efa_admin_cmds_defs.h   | 100 ++++++++-
 drivers/infiniband/hw/efa/efa_admin_defs.h    |  41 ++++
 drivers/infiniband/hw/efa/efa_com.c           | 171 +++++++++++++++
 drivers/infiniband/hw/efa/efa_com.h           |  38 +++-
 drivers/infiniband/hw/efa/efa_com_cmd.c       |  35 +++-
 drivers/infiniband/hw/efa/efa_com_cmd.h       |  10 +-
 drivers/infiniband/hw/efa/efa_main.c          | 196 +++++++++++++++---
 drivers/infiniband/hw/efa/efa_regs_defs.h     |   7 +-
 drivers/infiniband/hw/efa/efa_verbs.c         |  60 +++++-
 include/uapi/rdma/efa-abi.h                   |  18 +-
 11 files changed, 631 insertions(+), 64 deletions(-)

Signature · 1 line

2.32.0

Gal Pressman

author

2021-08-11 15:12 UTC #1 lore raw

PATCH 1/4 RDMA/efa: Free IRQ vectors on error flow

Make sure to free the IRQ vectors in case the allocation doesn't return the expected number of IRQs.

Fixes b7f5e880f377 ("RDMA/efa: Add the efa module")

Reviewed-by Firas JahJah <[email protected]>

Reviewed-by Yossi Leybovich <[email protected]>

Signed-off-by Gal Pressman <[email protected]>

---
 drivers/infiniband/hw/efa/efa_main.c | 1 +
 1 file changed, 1 insertion(+)

drivers/infiniband/hw/efa/efa_main.c +1 -0

index 997947d77de6..42f9ac3f586e 100644
@@ -358,6 +358,7 @@ static int efa_enable_msix(struct efa_dev *dev)
 	}
 
 	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);

Signature · 1 line

2.32.0

Jason Gunthorpe

maintainer Applied

2021-08-20 18:34 UTC #2 lore raw

Re: [PATCH for-next 1/4] RDMA/efa: Free IRQ vectors on error flow

Quoted 10 lines · Gal Pressman

Make sure to free the IRQ vectors in case the allocation doesn't return the expected number of IRQs.
Fixes b7f5e880f377 ("RDMA/efa: Add the efa module")
Reviewed-by Firas JahJah <[email protected]>
Reviewed-by Yossi Leybovich <[email protected]>
Signed-off-by Gal Pressman <[email protected]>
---
 drivers/infiniband/hw/efa/efa_main.c | 1 +
 1 file changed, 1 insertion(+)

Applied to for-rc, thanks

Jason

Gal Pressman

author

2021-08-11 15:12 UTC #3 lore raw

PATCH 2/4 RDMA/efa: Remove unused cpu field from irq struct

In reply to Gal Pressman

The cpu field in efa_irq struct is unused, remove it.

Reviewed-by Firas JahJah <[email protected]>

Reviewed-by Yossi Leybovich <[email protected]>

Signed-off-by Gal Pressman <[email protected]>

---
 drivers/infiniband/hw/efa/efa.h      | 3 +--
 drivers/infiniband/hw/efa/efa_main.c | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

drivers/infiniband/hw/efa/efa.h +1 -2

index 1a1e60eee1dc..1148b308d3f6 100644
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
 /*
- * Copyright 2018-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
+ * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
  */
 
 #ifndef _EFA_H_

@@ -27,7 +27,6 @@
 struct efa_irq {
 	irq_handler_t handler;
 	void *data;
-	int cpu;
 	u32 vector;
 	cpumask_t affinity_hint_mask;
 	char name[EFA_IRQNAME_SIZE];

drivers/infiniband/hw/efa/efa_main.c +0 -1

index 42f9ac3f586e..5b2fcfe9be20 100644
@@ -109,7 +109,6 @@ static void efa_setup_mgmnt_irq(struct efa_dev *dev)
 	dev->admin_irq.vector =
 		pci_irq_vector(dev->pdev, dev->admin_msix_vector_idx);
 	cpu = cpumask_first(cpu_online_mask);
-	dev->admin_irq.cpu = cpu;
 	cpumask_set_cpu(cpu,
 			&dev->admin_irq.affinity_hint_mask);
 	dev_info(&dev->pdev->dev, "Setup irq:0x%p vector:%d name:%s\n",

Signature · 1 line

2.32.0

Gal Pressman

author

2021-08-11 15:12 UTC #4 lore raw

PATCH 3/4 RDMA/efa: Rename vector field in efa_irq struct to irqn

In reply to Gal Pressman

The vector field naming is quite confusing, it is better referred to as irqn.

Reviewed-by Firas JahJah <[email protected]>

Reviewed-by Yossi Leybovich <[email protected]>

Signed-off-by Gal Pressman <[email protected]>

---
 drivers/infiniband/hw/efa/efa.h      |  2 +-
 drivers/infiniband/hw/efa/efa_main.c | 18 ++++++++----------
 2 files changed, 9 insertions(+), 11 deletions(-)

drivers/infiniband/hw/efa/efa.h +1 -1

index 1148b308d3f6..87b1dadeb7fe 100644
@@ -27,7 +27,7 @@
 struct efa_irq {
 	irq_handler_t handler;
 	void *data;
-	u32 vector;
+	u32 irqn;
 	cpumask_t affinity_hint_mask;
 	char name[EFA_IRQNAME_SIZE];
 };

drivers/infiniband/hw/efa/efa_main.c +8 ~~-10~~

index 5b2fcfe9be20..417dea5f90cf 100644
@@ -83,8 +83,7 @@ static int efa_request_mgmnt_irq(struct efa_dev *dev)
 	int err;
 
 	irq = &dev->admin_irq;
-	err = request_irq(irq->vector, irq->handler, 0, irq->name,
-			  irq->data);
+	err = request_irq(irq->irqn, irq->handler, 0, irq->name, irq->data);
 	if (err) {
 		dev_err(&dev->pdev->dev, "Failed to request admin irq (%d)\n",
 			err);

@@ -92,8 +91,8 @@ static int efa_request_mgmnt_irq(struct efa_dev *dev)
 	}
 
 	dev_dbg(&dev->pdev->dev, "Set affinity hint of mgmnt irq to %*pbl (irq vector: %d)\n",
-		nr_cpumask_bits, &irq->affinity_hint_mask, irq->vector);
-	irq_set_affinity_hint(irq->vector, &irq->affinity_hint_mask);
+		nr_cpumask_bits, &irq->affinity_hint_mask, irq->irqn);
+	irq_set_affinity_hint(irq->irqn, &irq->affinity_hint_mask);
 
 	return 0;
 }

@@ -106,14 +105,13 @@ static void efa_setup_mgmnt_irq(struct efa_dev *dev)
 		 "efa-mgmnt@pci:%s", pci_name(dev->pdev));
 	dev->admin_irq.handler = efa_intr_msix_mgmnt;
 	dev->admin_irq.data = dev;
-	dev->admin_irq.vector =
+	dev->admin_irq.irqn =
 		pci_irq_vector(dev->pdev, dev->admin_msix_vector_idx);
 	cpu = cpumask_first(cpu_online_mask);
 	cpumask_set_cpu(cpu,
 			&dev->admin_irq.affinity_hint_mask);
-	dev_info(&dev->pdev->dev, "Setup irq:0x%p vector:%d name:%s\n",
-		 &dev->admin_irq,
-		 dev->admin_irq.vector,
+	dev_info(&dev->pdev->dev, "Setup irq:%d name:%s\n",
+		 dev->admin_irq.irqn,
 		 dev->admin_irq.name);
 }

@@ -122,8 +120,8 @@ static void efa_free_mgmnt_irq(struct efa_dev *dev)
 	struct efa_irq *irq;
 
 	irq = &dev->admin_irq;
-	irq_set_affinity_hint(irq->vector, NULL);
-	free_irq(irq->vector, irq->data);
+	irq_set_affinity_hint(irq->irqn, NULL);
+	free_irq(irq->irqn, irq->data);
 }
 
 static int efa_set_mgmnt_irq(struct efa_dev *dev)

Signature · 1 line

2.32.0

Gal Pressman

author

2021-08-11 15:13 UTC #5 lore raw

PATCH 4/4 RDMA/efa: CQ notifications

In reply to Gal Pressman

This patch adds support for CQ notifications through the standard verbs api.

In order to achieve that, a new event queue (EQ) object is introduced, which is in charge of reporting completion events to the driver. On driver load, EQs are allocated and their affinity is set to a single cpu. When a user app creates a CQ with a completion channel, the completion vector number is converted to a EQ number, which is in charge of reporting the CQ events.

In addition, the CQ creation admin command now returns an offset for the CQ doorbell, which is mapped to the userspace provider and is used to arm the CQ when requested by the user.

The EQs use a single doorbell (located on the registers BAR), which encodes the EQ number and arm as part of the doorbell value. The EQs are polled by the driver on each new EQE, and arm it when the poll is completed.

Reviewed-by Firas JahJah <[email protected]>

Reviewed-by Yossi Leybovich <[email protected]>

Signed-off-by Gal Pressman <[email protected]>

---
 drivers/infiniband/hw/efa/efa.h               |  16 +-
 .../infiniband/hw/efa/efa_admin_cmds_defs.h   | 100 +++++++++-
 drivers/infiniband/hw/efa/efa_admin_defs.h    |  41 ++++
 drivers/infiniband/hw/efa/efa_com.c           | 171 ++++++++++++++++
 drivers/infiniband/hw/efa/efa_com.h           |  38 +++-
 drivers/infiniband/hw/efa/efa_com_cmd.c       |  35 +++-
 drivers/infiniband/hw/efa/efa_com_cmd.h       |  10 +-
 drivers/infiniband/hw/efa/efa_main.c          | 184 +++++++++++++++---
 drivers/infiniband/hw/efa/efa_regs_defs.h     |   7 +-
 drivers/infiniband/hw/efa/efa_verbs.c         |  60 +++++-
 include/uapi/rdma/efa-abi.h                   |  18 +-
 11 files changed, 625 insertions(+), 55 deletions(-)

drivers/infiniband/hw/efa/efa.h +14 -2

index 87b1dadeb7fe..e72bc4741ca1 100644
@@ -20,14 +20,14 @@
 
 #define EFA_IRQNAME_SIZE        40
 
-/* 1 for AENQ + ADMIN */
-#define EFA_NUM_MSIX_VEC                  1
 #define EFA_MGMNT_MSIX_VEC_IDX            0
+#define EFA_COMP_EQS_VEC_BASE             1
 
 struct efa_irq {
 	irq_handler_t handler;
 	void *data;
 	u32 irqn;
+	u32 vector;
 	cpumask_t affinity_hint_mask;
 	char name[EFA_IRQNAME_SIZE];
 };

@@ -61,6 +61,12 @@ struct efa_dev {
 	struct efa_irq admin_irq;
 
 	struct efa_stats stats;
+
+	/* Array of completion EQs */
+	struct efa_eq *eqs;
+	unsigned int neqs;
+
+	struct xarray cqs_xa;
 };
 
 struct efa_ucontext {

@@ -84,6 +90,7 @@ struct efa_cq {
 	dma_addr_t dma_addr;
 	void *cpu_addr;
 	struct rdma_user_mmap_entry *mmap_entry;
+	struct rdma_user_mmap_entry *db_mmap_entry;
 	size_t size;
 	u16 cq_idx;
 };

@@ -116,6 +123,11 @@ struct efa_ah {
 	u8 id[EFA_GID_SIZE];
 };
 
+struct efa_eq {
+	struct efa_com_eq eeq;
+	struct efa_irq irq;
+};
+
 int efa_query_device(struct ib_device *ibdev,
 		     struct ib_device_attr *props,
 		     struct ib_udata *udata);

drivers/infiniband/hw/efa/efa_admin_cmds_defs.h +95 -5

158 lines not on this page · raw message

drivers/infiniband/hw/efa/efa_admin_defs.h +41 -0

51 lines not on this page · raw message

drivers/infiniband/hw/efa/efa_com.c +171 -0

185 lines not on this page · raw message

drivers/infiniband/hw/efa/efa_com.h +37 -1

index 5e4c88877ddb..0fe241cd48e7 100644
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
 /*
- * Copyright 2018-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
+ * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
  */
 
 #ifndef _EFA_COM_H_

@@ -80,6 +80,9 @@ struct efa_com_admin_queue {
 };
 
 struct efa_aenq_handlers;
+struct efa_com_eq;
+typedef void (*efa_eqe_handler)(struct efa_com_eq *eeq,
+				struct efa_admin_eqe *eqe);
 
 struct efa_com_aenq {
 	struct efa_admin_aenq_entry *entries;

@@ -112,6 +115,33 @@ struct efa_com_dev {
 	struct efa_com_mmio_read mmio_read;
 };
 
+struct efa_com_eq {
+	struct efa_com_dev *edev;
+	struct efa_admin_eqe *eqes;
+	dma_addr_t dma_addr;
+	u32 cc; /* Consumer counter */
+	u16 eqn;
+	u16 depth;
+	u8 phase;
+	efa_eqe_handler cb;
+};
+
+struct efa_com_create_eq_params {
+	dma_addr_t dma_addr;
+	u32 event_bitmask;
+	u16 depth;
+	u8 entry_size_in_bytes;
+	u8 msix_vec;
+};
+
+struct efa_com_create_eq_result {
+	u16 eqn;
+};
+
+struct efa_com_destroy_eq_params {
+	u16 eqn;
+};
+
 typedef void (*efa_aenq_handler)(void *data,
 	      struct efa_admin_aenq_entry *aenq_e);

@@ -121,9 +151,13 @@ struct efa_aenq_handlers {
 	efa_aenq_handler unimplemented_handler;
 };
 
+void efa_com_set_dma_addr(dma_addr_t addr, u32 *addr_high, u32 *addr_low);
 int efa_com_admin_init(struct efa_com_dev *edev,
 		       struct efa_aenq_handlers *aenq_handlers);
 void efa_com_admin_destroy(struct efa_com_dev *edev);
+int efa_com_eq_init(struct efa_com_dev *edev, struct efa_com_eq *eeq,
+		    efa_eqe_handler cb, u16 depth, u8 msix_vec);
+int efa_com_eq_destroy(struct efa_com_dev *edev, struct efa_com_eq *eeq);
 int efa_com_dev_reset(struct efa_com_dev *edev,
 		      enum efa_regs_reset_reason_types reset_reason);
 void efa_com_set_admin_polling_mode(struct efa_com_dev *edev, bool polling);

@@ -140,5 +174,7 @@ int efa_com_cmd_exec(struct efa_com_admin_queue *aq,
 		     struct efa_admin_acq_entry *comp,
 		     size_t comp_size);
 void efa_com_aenq_intr_handler(struct efa_com_dev *edev, void *data);
+void efa_com_eq_comp_intr_handler(struct efa_com_dev *edev,
+				  struct efa_com_eq *eeq);
 
 #endif /* _EFA_COM_H_ */

drivers/infiniband/hw/efa/efa_com_cmd.c +27 -8

index f752ef64159c..fb405da4e1db 100644
@@ -1,17 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
 /*
- * Copyright 2018-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
+ * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
  */
 
 #include "efa_com.h"
 #include "efa_com_cmd.h"
 
-void efa_com_set_dma_addr(dma_addr_t addr, u32 *addr_high, u32 *addr_low)
-{
-	*addr_low = lower_32_bits(addr);
-	*addr_high = upper_32_bits(addr);
-}
-
 int efa_com_create_qp(struct efa_com_dev *edev,
 		      struct efa_com_create_qp_params *params,
 		      struct efa_com_create_qp_result *res)

@@ -157,7 +151,7 @@ int efa_com_create_cq(struct efa_com_dev *edev,
 		      struct efa_com_create_cq_params *params,
 		      struct efa_com_create_cq_result *result)
 {
-	struct efa_admin_create_cq_resp cmd_completion;
+	struct efa_admin_create_cq_resp cmd_completion = {};
 	struct efa_admin_create_cq_cmd create_cmd = {};
 	struct efa_com_admin_queue *aq = &edev->aq;
 	int err;

@@ -169,6 +163,11 @@ int efa_com_create_cq(struct efa_com_dev *edev,
 	create_cmd.cq_depth = params->cq_depth;
 	create_cmd.num_sub_cqs = params->num_sub_cqs;
 	create_cmd.uar = params->uarn;
+	if (params->interrupt_mode_enabled) {
+		EFA_SET(&create_cmd.cq_caps_1,
+			EFA_ADMIN_CREATE_CQ_CMD_INTERRUPT_MODE_ENABLED, 1);
+		create_cmd.eqn = params->eqn;
+	}
 
 	efa_com_set_dma_addr(params->dma_addr,
 			     &create_cmd.cq_ba.mem_addr_high,

@@ -187,6 +186,9 @@ int efa_com_create_cq(struct efa_com_dev *edev,
 
 	result->cq_idx = cmd_completion.cq_idx;
 	result->actual_depth = params->cq_depth;
+	result->db_off = cmd_completion.db_offset;
+	result->db_valid = EFA_GET(&cmd_completion.flags,
+				   EFA_ADMIN_CREATE_CQ_RESP_DB_VALID);
 
 	return 0;
 }

@@ -497,6 +499,23 @@ int efa_com_get_device_attr(struct efa_com_dev *edev,
 	       sizeof(resp.u.network_attr.addr));
 	result->mtu = resp.u.network_attr.mtu;
 
+	if (efa_com_check_supported_feature_id(edev,
+					       EFA_ADMIN_EVENT_QUEUE_ATTR)) {
+		err = efa_com_get_feature(edev, &resp,
+					  EFA_ADMIN_EVENT_QUEUE_ATTR);
+		if (err) {
+			ibdev_err_ratelimited(
+				edev->efa_dev,
+				"Failed to get event queue attributes %d\n",
+				err);
+			return err;
+		}
+
+		result->max_eq = resp.u.event_queue_attr.max_eq;
+		result->max_eq_depth = resp.u.event_queue_attr.max_eq_depth;
+		result->event_bitmask = resp.u.event_queue_attr.event_bitmask;
+	}
+
 	return 0;
 }

drivers/infiniband/hw/efa/efa_com_cmd.h +8 -2

index eea4ebfbe6ec..c33010bbf9e8 100644
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
 /*
- * Copyright 2018-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
+ * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
  */
 
 #ifndef _EFA_COM_CMD_H_

@@ -73,7 +73,9 @@ struct efa_com_create_cq_params {
 	u16 cq_depth;
 	u16 num_sub_cqs;
 	u16 uarn;
+	u16 eqn;
 	u8 entry_size_in_bytes;
+	bool interrupt_mode_enabled;
 };
 
 struct efa_com_create_cq_result {

@@ -81,6 +83,8 @@ struct efa_com_create_cq_result {
 	u16 cq_idx;
 	/* actual cq depth in # of entries */
 	u16 actual_depth;
+	u32 db_off;
+	bool db_valid;
 };
 
 struct efa_com_destroy_cq_params {

@@ -125,6 +129,9 @@ struct efa_com_get_device_attr_result {
 	u32 max_llq_size;
 	u32 max_rdma_size;
 	u32 device_caps;
+	u32 max_eq;
+	u32 max_eq_depth;
+	u32 event_bitmask; /* EQ events bitmask */
 	u16 sub_cqs_per_cq;
 	u16 max_sq_sge;
 	u16 max_rq_sge;

@@ -260,7 +267,6 @@ union efa_com_get_stats_result {
 	struct efa_com_rdma_read_stats rdma_read_stats;
 };
 
-void efa_com_set_dma_addr(dma_addr_t addr, u32 *addr_high, u32 *addr_low);
 int efa_com_create_qp(struct efa_com_dev *edev,
 		      struct efa_com_create_qp_params *params,
 		      struct efa_com_create_qp_result *res);

drivers/infiniband/hw/efa/efa_main.c +158 ~~-26~~

288 lines not on this page · raw message

drivers/infiniband/hw/efa/efa_regs_defs.h +6 -1

index 4017982fe13b..714ae6258800 100644
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
 /*
- * Copyright 2018-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
+ * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
  */
 
 #ifndef _EFA_REGS_H_

@@ -42,6 +42,7 @@ enum efa_regs_reset_reason_types {
 #define EFA_REGS_MMIO_REG_READ_OFF                          0x5c
 #define EFA_REGS_MMIO_RESP_LO_OFF                           0x60
 #define EFA_REGS_MMIO_RESP_HI_OFF                           0x64
+#define EFA_REGS_EQ_DB_OFF                                  0x68
 
 /* version register */
 #define EFA_REGS_VERSION_MINOR_VERSION_MASK                 0xff

@@ -93,4 +94,8 @@ enum efa_regs_reset_reason_types {
 #define EFA_REGS_MMIO_REG_READ_REQ_ID_MASK                  0xffff
 #define EFA_REGS_MMIO_REG_READ_REG_OFF_MASK                 0xffff0000
 
+/* eq_db register */
+#define EFA_REGS_EQ_DB_EQN_MASK                             0xffff
+#define EFA_REGS_EQ_DB_ARM_MASK                             0x80000000
+
 #endif /* _EFA_REGS_H_ */

drivers/infiniband/hw/efa/efa_verbs.c +53 -7

138 lines not on this page · raw message

include/uapi/rdma/efa-abi.h +15 -3

index f89fbb5b1e8d..08035ccf1fff 100644
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */
 /*
- * Copyright 2018-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
+ * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
  */
 
 #ifndef EFA_ABI_USER_H

@@ -52,11 +52,20 @@ struct efa_ibv_alloc_pd_resp {
 	__u8 reserved_30[2];
 };
 
+enum {
+	EFA_CREATE_CQ_WITH_COMPLETION_CHANNEL = 1 << 0,
+};
+
 struct efa_ibv_create_cq {
 	__u32 comp_mask;
 	__u32 cq_entry_size;
 	__u16 num_sub_cqs;
-	__u8 reserved_50[6];
+	__u8 flags;
+	__u8 reserved_58[5];
+};
+
+enum {
+	EFA_CREATE_CQ_RESP_DB_OFF = 1 << 0,
 };
 
 struct efa_ibv_create_cq_resp {

@@ -65,7 +74,9 @@ struct efa_ibv_create_cq_resp {
 	__aligned_u64 q_mmap_key;
 	__aligned_u64 q_mmap_size;
 	__u16 cq_idx;
-	__u8 reserved_d0[6];
+	__u8 reserved_d0[2];
+	__u32 db_off;
+	__aligned_u64 db_mmap_key;
 };
 
 enum {

@@ -106,6 +117,7 @@ struct efa_ibv_create_ah_resp {
 enum {
 	EFA_QUERY_DEVICE_CAPS_RDMA_READ = 1 << 0,
 	EFA_QUERY_DEVICE_CAPS_RNR_RETRY = 1 << 1,
+	EFA_QUERY_DEVICE_CAPS_CQ_NOTIFICATIONS = 1 << 2,
 };
 
 struct efa_ibv_ex_query_device_resp {

Signature · 1 line

2.32.0

Jason Gunthorpe

maintainer

2021-08-20 18:27 UTC #6 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 4 lines · Gal Pressman

drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c

drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76

Quoted 10 lines

drivers/infiniband/hw/efa/efa_main.c +7 -0

@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
 	pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+	u16 cqn = eqe->u.comp_event.cqn;
+	struct efa_cq *cq;
+
+	cq = xa_load(&dev->cqs_xa, cqn);
+	if (unlikely(!cq)) {

This seems unlikely to be correct, what prevents cq from being destroyed concurrently?

A comp_handler cannot be running after cq destroy completes.

drivers/infiniband/hw/efa/efa_main.c static struct efa_dev *efa_probe_device(struct pci_dev *pdev) · line 554

Quoted 5 lines

drivers/infiniband/hw/efa/efa_main.c +1 -0

@@ -421,6 +551,7 @@ static struct efa_dev *efa_probe_device(struct pci_dev *pdev)
 	edev->efa_dev = dev;
 	edev->dmadev = &pdev->dev;
 	dev->pdev = pdev;
+	xa_init_flags(&dev->cqs_xa, XA_FLAGS_LOCK_IRQ);

And this is confusing too, since the above is the only reference and doesn't take the xa_lock why does the xa need to use LOCK_IRQ?

Jason

Gal Pressman

author

2021-09-01 11:51 UTC #7 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 19 lines · Jason Gunthorpe

Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
 	pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+	u16 cqn = eqe->u.comp_event.cqn;
+	struct efa_cq *cq;
+
+	cq = xa_load(&dev->cqs_xa, cqn);
+	if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?

Quoted 1 line

A comp_handler cannot be running after cq destroy completes.

Sorry for the long turnaround, was OOO.

The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208

Quoted 8 lines

drivers/infiniband/hw/efa/efa_main.c static struct efa_dev *efa_probe_device(struct pci_dev *pdev) · line 554
Quoted 5 lines
drivers/infiniband/hw/efa/efa_main.c +1 -0
@@ -421,6 +551,7 @@ static struct efa_dev *efa_probe_device(struct pci_dev *pdev)
 	edev->efa_dev = dev;
 	edev->dmadev = &pdev->dev;
 	dev->pdev = pdev;
+	xa_init_flags(&dev->cqs_xa, XA_FLAGS_LOCK_IRQ);
And this is confusing too, since the above is the only reference and doesn't take the xa_lock why does the xa need to use LOCK_IRQ?

Oh, I'm now noticing that the load operation uses an rcu lock, not the xa_lock.. This can be replaced with xa_init().

Thanks for the review.

Jason Gunthorpe

maintainer

2021-09-01 11:57 UTC #8 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 20 lines · Gal Pressman

Quoted 19 lines · Jason Gunthorpe
Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
 	pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+	u16 cqn = eqe->u.comp_event.cqn;
+	struct efa_cq *cq;
+
+	cq = xa_load(&dev->cqs_xa, cqn);
+	if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?

Quoted 7 lines

Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208

That is something quite different, and in userspace.

What in the kernel prevents tha xa_load and the xa_erase from racing together?

Jason

Gal Pressman

author

2021-09-01 14:25 UTC #9 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 25 lines · Jason Gunthorpe

Quoted 24 lines · Gal Pressman
Quoted 19 lines · Jason Gunthorpe
Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
 	pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+	u16 cqn = eqe->u.comp_event.cqn;
+	struct efa_cq *cq;
+
+	cq = xa_load(&dev->cqs_xa, cqn);
+	if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?
Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.

Quoted 7 lines

Quoted 3 lines
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?

Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq. Though I'd like to avoid copy-pasting xa_load() in order to use the advanced xas_load() function.

Do you have any better ideas?

Jason Gunthorpe

maintainer

2021-09-01 15:37 UTC #10 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 30 lines · Gal Pressman

Quoted 29 lines · Jason Gunthorpe
Quoted 27 lines · Gal Pressman
Quoted 19 lines · Jason Gunthorpe
Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
 	pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+	u16 cqn = eqe->u.comp_event.cqn;
+	struct efa_cq *cq;
+
+	cq = xa_load(&dev->cqs_xa, cqn);
+	if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?
Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208

Quoted 8 lines

Quoted 3 lines
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.

Try to avoid synchronize_rcu()

Quoted 2 lines

Though I'd like to avoid copy-pasting xa_load() in order to use the advanced xas_load() function.

Why would you need that? Just call xa_load() while holding the rcu_read_lock() if that is what you want. Can you put the whole function under the rcu_read_lock()?

Quoted 1 line

Do you have any better ideas?

Other common alternative is to use the xa_lock(), but that doesn't seem suitable for an EQ

Jason

Gal Pressman

author

2021-09-02 07:03 UTC #11 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 33 lines · Jason Gunthorpe

Quoted 32 lines · Gal Pressman
Quoted 31 lines · Jason Gunthorpe
Quoted 20 lines · Gal Pressman
Quoted 19 lines · Jason Gunthorpe
Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
 	pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+	u16 cqn = eqe->u.comp_event.cqn;
+	struct efa_cq *cq;
+
+	cq = xa_load(&dev->cqs_xa, cqn);
+	if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?
Quoted 7 lines
Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.
Try to avoid synchronize_rcu()

I don't see how that's possible?

Quoted 6 lines

Quoted 2 lines
Though I'd like to avoid copy-pasting xa_load() in order to use the advanced xas_load() function.
Why would you need that? Just call xa_load() while holding the rcu_read_lock() if that is what you want. Can you put the whole function under the rcu_read_lock()?

Sure, I wasn't sure if it's OK to nest rcu_read_lock() calls.

Quoted 4 lines

Quoted 1 line
Do you have any better ideas?
Other common alternative is to use the xa_lock(), but that doesn't seem suitable for an EQ

Right, I think I'll go with the rcu approach.

Jason Gunthorpe

maintainer

2021-09-02 13:02 UTC #12 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 36 lines · Gal Pressman

Quoted 35 lines · Jason Gunthorpe
Quoted 34 lines · Gal Pressman
Quoted 32 lines · Jason Gunthorpe
Quoted 20 lines · Gal Pressman
Quoted 19 lines · Jason Gunthorpe
Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
 	pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+	u16 cqn = eqe->u.comp_event.cqn;
+	struct efa_cq *cq;
+
+	cq = xa_load(&dev->cqs_xa, cqn);
+	if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?
Quoted 7 lines
Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.
Try to avoid synchronize_rcu()
I don't see how that's possible?

Usually people use call_rcu() instead

Quoted 1 line

Sure, I wasn't sure if it's OK to nest rcu_read_lock() calls.

It is OK

Jason

Gal Pressman

author

2021-09-02 15:09 UTC #13 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 42 lines · Jason Gunthorpe

Quoted 41 lines · Gal Pressman
Quoted 40 lines · Jason Gunthorpe
Quoted 38 lines · Gal Pressman
Quoted 25 lines · Jason Gunthorpe
Quoted 24 lines · Gal Pressman
Quoted 19 lines · Jason Gunthorpe
Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
 	pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+	u16 cqn = eqe->u.comp_event.cqn;
+	struct efa_cq *cq;
+
+	cq = xa_load(&dev->cqs_xa, cqn);
+	if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?
Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
Quoted 7 lines
Quoted 3 lines
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Try to avoid synchronize_rcu()
I don't see how that's possible?
Usually people use call_rcu() instead

Oh nice, thanks.

I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.

Jason Gunthorpe

maintainer

2021-09-02 15:10 UTC #14 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 47 lines · Gal Pressman

Quoted 46 lines · Jason Gunthorpe
Quoted 44 lines · Gal Pressman
Quoted 33 lines · Jason Gunthorpe
Quoted 32 lines · Gal Pressman
Quoted 31 lines · Jason Gunthorpe
Quoted 20 lines · Gal Pressman
Quoted 19 lines · Jason Gunthorpe
Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
 	pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+	u16 cqn = eqe->u.comp_event.cqn;
+	struct efa_cq *cq;
+
+	cq = xa_load(&dev->cqs_xa, cqn);
+	if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?
Quoted 7 lines
Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.
Try to avoid synchronize_rcu()
I don't see how that's possible?

Quoted 8 lines

Quoted 1 line
Usually people use call_rcu() instead
Oh nice, thanks.
I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.

I would not be happy to see synchronize_rcu on uverbs destroy functions, it is too easy to DOS the kernel with that.

Jason

Gal Pressman

author

2021-09-02 15:18 UTC #15 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 52 lines · Jason Gunthorpe

Quoted 51 lines · Gal Pressman
Quoted 42 lines · Jason Gunthorpe
Quoted 41 lines · Gal Pressman
Quoted 40 lines · Jason Gunthorpe
Quoted 38 lines · Gal Pressman
Quoted 25 lines · Jason Gunthorpe
Quoted 24 lines · Gal Pressman
Quoted 19 lines · Jason Gunthorpe
Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
     pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+    u16 cqn = eqe->u.comp_event.cqn;
+    struct efa_cq *cq;
+
+    cq = xa_load(&dev->cqs_xa, cqn);
+    if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?
Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
Quoted 7 lines
Quoted 3 lines
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Try to avoid synchronize_rcu()
I don't see how that's possible?
Usually people use call_rcu() instead
Oh nice, thanks.

Quoted 7 lines

Quoted 4 lines
I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.
I would not be happy to see synchronize_rcu on uverbs destroy functions, it is too easy to DOS the kernel with that.

OK, but isn't the fact that the uverb can return before the CQ is actually destroyed problematic?

Maybe it's an extreme corner case, but if I created max_cq CQs, destroyed one, and try to create another one, it is not guaranteed that the create operation would succeed - even though the destroy has finished.

Jason Gunthorpe

maintainer

2021-09-02 15:41 UTC #16 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 58 lines · Gal Pressman

Quoted 57 lines · Jason Gunthorpe
Quoted 55 lines · Gal Pressman
Quoted 42 lines · Jason Gunthorpe
Quoted 41 lines · Gal Pressman
Quoted 40 lines · Jason Gunthorpe
Quoted 38 lines · Gal Pressman
Quoted 25 lines · Jason Gunthorpe
Quoted 24 lines · Gal Pressman
Quoted 19 lines · Jason Gunthorpe
Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
     pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+    u16 cqn = eqe->u.comp_event.cqn;
+    struct efa_cq *cq;
+
+    cq = xa_load(&dev->cqs_xa, cqn);
+    if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?
Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
Quoted 7 lines
Quoted 3 lines
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Try to avoid synchronize_rcu()
I don't see how that's possible?
Usually people use call_rcu() instead
Oh nice, thanks.
I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.

Quoted 5 lines

Quoted 2 lines
I would not be happy to see synchronize_rcu on uverbs destroy functions, it is too easy to DOS the kernel with that.
OK, but isn't the fact that the uverb can return before the CQ is actually destroyed problematic?

Yes, you can't allow that, something other than RCU needs to prevent that

Quoted 3 lines

Maybe it's an extreme corner case, but if I created max_cq CQs, destroyed one, and try to create another one, it is not guaranteed that the create operation would succeed - even though the destroy has finished.

More importantly a driver cannot call completion callbacks once destroy cq has returned.

Jason

Gal Pressman

author

2021-09-05 07:25 UTC #17 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 68 lines · Jason Gunthorpe

Quoted 58 lines · Gal Pressman
Quoted 57 lines · Jason Gunthorpe
Quoted 55 lines · Gal Pressman
Quoted 42 lines · Jason Gunthorpe
Quoted 41 lines · Gal Pressman
Quoted 40 lines · Jason Gunthorpe
Quoted 38 lines · Gal Pressman
Quoted 25 lines · Jason Gunthorpe
Quoted 24 lines · Gal Pressman
Quoted 19 lines · Jason Gunthorpe
Quoted 4 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_main.c +1 -0
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
drivers/infiniband/hw/efa/efa_main.c static void efa_release_bars(struct efa_dev *dev, int bars_mask) · line 76
Quoted 10 lines
drivers/infiniband/hw/efa/efa_main.c +7 -0
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
     pci_release_selected_regions(pdev, release_bars);
 }
 
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+    u16 cqn = eqe->u.comp_event.cqn;
+    struct efa_cq *cq;
+
+    cq = xa_load(&dev->cqs_xa, cqn);
+    if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?
Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
Quoted 7 lines
Quoted 3 lines
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Try to avoid synchronize_rcu()
I don't see how that's possible?
Usually people use call_rcu() instead
Oh nice, thanks.
I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.
Quoted 5 lines
Quoted 2 lines
I would not be happy to see synchronize_rcu on uverbs destroy functions, it is too easy to DOS the kernel with that.
OK, but isn't the fact that the uverb can return before the CQ is actually destroyed problematic?
Yes, you can't allow that, something other than RCU needs to prevent that

Quoted 6 lines

Quoted 3 lines
Maybe it's an extreme corner case, but if I created max_cq CQs, destroyed one, and try to create another one, it is not guaranteed that the create operation would succeed - even though the destroy has finished.
More importantly a driver cannot call completion callbacks once destroy cq has returned.

So how is having some kind of synchronization to wait for the call_rcu() callback to finish different than using synchronize_rcu()? We'll have to wait for the readers to finish before returning.

Leon Romanovsky

2021-09-05 07:59 UTC #18 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 73 lines · Gal Pressman

Quoted 72 lines · Jason Gunthorpe
Quoted 58 lines · Gal Pressman
Quoted 57 lines · Jason Gunthorpe
Quoted 55 lines · Gal Pressman
Quoted 42 lines · Jason Gunthorpe
Quoted 41 lines · Gal Pressman
Quoted 40 lines · Jason Gunthorpe
Quoted 38 lines · Gal Pressman
Quoted 25 lines · Jason Gunthorpe
Quoted 24 lines · Gal Pressman
Quoted 19 lines · Jason Gunthorpe
Quoted 14 lines · Gal Pressman
diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
pci_release_selected_regions(pdev, release_bars);
}
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+ u16 cqn = eqe->u.comp_event.cqn;
+ struct efa_cq *cq;
+
+ cq = xa_load(&dev->cqs_xa, cqn);
+ if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being destroyed concurrently?
Quoted 1 line
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
Quoted 7 lines
Quoted 3 lines
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Try to avoid synchronize_rcu()
I don't see how that's possible?
Usually people use call_rcu() instead
Oh nice, thanks.
I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.
Quoted 5 lines
Quoted 2 lines
I would not be happy to see synchronize_rcu on uverbs destroy functions, it is too easy to DOS the kernel with that.
OK, but isn't the fact that the uverb can return before the CQ is actually destroyed problematic?
Yes, you can't allow that, something other than RCU needs to prevent that
Quoted 3 lines
Maybe it's an extreme corner case, but if I created max_cq CQs, destroyed one, and try to create another one, it is not guaranteed that the create operation would succeed - even though the destroy has finished.

Quoted 6 lines

Quoted 2 lines
More importantly a driver cannot call completion callbacks once destroy cq has returned.
So how is having some kind of synchronization to wait for the call_rcu() callback to finish different than using synchronize_rcu()? We'll have to wait for the readers to finish before returning.

Why do you need to do anything special in addition to nullify completion callback which will ensure that no new readers are coming and call_rcu to make sure that existing readers finished?

Thanks

Gal Pressman

author

2021-09-05 10:46 UTC #19 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 77 lines · Leon Romanovsky

Quoted 76 lines · Gal Pressman
Quoted 74 lines · Jason Gunthorpe
Quoted 58 lines · Gal Pressman
Quoted 57 lines · Jason Gunthorpe
Quoted 55 lines · Gal Pressman
Quoted 42 lines · Jason Gunthorpe
Quoted 41 lines · Gal Pressman
Quoted 40 lines · Jason Gunthorpe
Quoted 38 lines · Gal Pressman
Quoted 25 lines · Jason Gunthorpe
Quoted 24 lines · Gal Pressman
Quoted 20 lines · Jason Gunthorpe
On Wed, Aug 11, 2021 at 06:11:31PM +0300, Gal Pressman wrote:
diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
pci_release_selected_regions(pdev, release_bars);
}
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+ u16 cqn = eqe->u.comp_event.cqn;
+ struct efa_cq *cq;
+
+ cq = xa_load(&dev->cqs_xa, cqn);
+ if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being
destroyed concurrently?
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
Quoted 7 lines
Quoted 3 lines
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Try to avoid synchronize_rcu()
I don't see how that's possible?
Usually people use call_rcu() instead
Oh nice, thanks.
I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.
Quoted 5 lines
Quoted 2 lines
I would not be happy to see synchronize_rcu on uverbs destroy functions, it is too easy to DOS the kernel with that.
OK, but isn't the fact that the uverb can return before the CQ is actually destroyed problematic?
Yes, you can't allow that, something other than RCU needs to prevent that
Quoted 3 lines
Maybe it's an extreme corner case, but if I created max_cq CQs, destroyed one, and try to create another one, it is not guaranteed that the create operation would succeed - even though the destroy has finished.
More importantly a driver cannot call completion callbacks once destroy cq has returned.

Quoted 7 lines

Quoted 3 lines
So how is having some kind of synchronization to wait for the call_rcu() callback to finish different than using synchronize_rcu()? We'll have to wait for the readers to finish before returning.
Why do you need to do anything special in addition to nullify completion callback which will ensure that no new readers are coming and call_rcu to make sure that existing readers finished?

I ensure there are no new readers by removing the CQ from the xarray. Then I must wait for all existing readers before returning from efa_destroy_cq and freeing the cq struct (which is done by ib_core).

call_rcu() don't really fit this use case.

Leon Romanovsky

2021-09-05 10:54 UTC #20 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 82 lines · Gal Pressman

Quoted 81 lines · Leon Romanovsky
Quoted 79 lines · Gal Pressman
Quoted 68 lines · Jason Gunthorpe
Quoted 58 lines · Gal Pressman
Quoted 57 lines · Jason Gunthorpe
Quoted 55 lines · Gal Pressman
Quoted 42 lines · Jason Gunthorpe
Quoted 41 lines · Gal Pressman
Quoted 40 lines · Jason Gunthorpe
Quoted 38 lines · Gal Pressman
Quoted 25 lines · Jason Gunthorpe
Quoted 24 lines · Gal Pressman
On 20/08/2021 21:27, Jason Gunthorpe wrote:
On Wed, Aug 11, 2021 at 06:11:31PM +0300, Gal Pressman wrote:
diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
pci_release_selected_regions(pdev, release_bars);
}
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+ u16 cqn = eqe->u.comp_event.cqn;
+ struct efa_cq *cq;
+
+ cq = xa_load(&dev->cqs_xa, cqn);
+ if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being
destroyed concurrently?
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
Quoted 7 lines
Quoted 3 lines
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Try to avoid synchronize_rcu()
I don't see how that's possible?
Usually people use call_rcu() instead
Oh nice, thanks.
I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.
Quoted 5 lines
Quoted 2 lines
I would not be happy to see synchronize_rcu on uverbs destroy functions, it is too easy to DOS the kernel with that.
OK, but isn't the fact that the uverb can return before the CQ is actually destroyed problematic?
Yes, you can't allow that, something other than RCU needs to prevent that
Quoted 6 lines
Quoted 3 lines
Maybe it's an extreme corner case, but if I created max_cq CQs, destroyed one, and try to create another one, it is not guaranteed that the create operation would succeed - even though the destroy has finished.
More importantly a driver cannot call completion callbacks once destroy cq has returned.
So how is having some kind of synchronization to wait for the call_rcu() callback to finish different than using synchronize_rcu()? We'll have to wait for the readers to finish before returning.

Quoted 7 lines

Quoted 3 lines
Why do you need to do anything special in addition to nullify completion callback which will ensure that no new readers are coming and call_rcu to make sure that existing readers finished?
I ensure there are no new readers by removing the CQ from the xarray. Then I must wait for all existing readers before returning from efa_destroy_cq and freeing the cq struct (which is done by ib_core).

IB/core calls to rdma_restrack_del() which wait_for_completion() before freeing CQ and returning to the users. You don't need to wait in efa_destroy_cq().

Thanks

Quoted 2 lines

call_rcu() don't really fit this use case.

Gal Pressman

author

2021-09-05 11:05 UTC #21 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 87 lines · Leon Romanovsky

Quoted 86 lines · Gal Pressman
Quoted 84 lines · Leon Romanovsky
Quoted 73 lines · Gal Pressman
Quoted 72 lines · Jason Gunthorpe
Quoted 58 lines · Gal Pressman
Quoted 57 lines · Jason Gunthorpe
Quoted 55 lines · Gal Pressman
Quoted 42 lines · Jason Gunthorpe
Quoted 41 lines · Gal Pressman
Quoted 40 lines · Jason Gunthorpe
Quoted 38 lines · Gal Pressman
Quoted 32 lines · Jason Gunthorpe
On Wed, Sep 01, 2021 at 02:50:42PM +0300, Gal Pressman wrote:
On 20/08/2021 21:27, Jason Gunthorpe wrote:
On Wed, Aug 11, 2021 at 06:11:31PM +0300, Gal Pressman wrote:
diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
pci_release_selected_regions(pdev, release_bars);
}
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+ u16 cqn = eqe->u.comp_event.cqn;
+ struct efa_cq *cq;
+
+ cq = xa_load(&dev->cqs_xa, cqn);
+ if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being
destroyed concurrently?
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and have a synchronize_rcu() after removing it from the xarray in destroy_cq.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Try to avoid synchronize_rcu()
I don't see how that's possible?
Usually people use call_rcu() instead
Oh nice, thanks.
I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.
Quoted 5 lines
Quoted 2 lines
I would not be happy to see synchronize_rcu on uverbs destroy functions, it is too easy to DOS the kernel with that.
OK, but isn't the fact that the uverb can return before the CQ is actually destroyed problematic?
Yes, you can't allow that, something other than RCU needs to prevent that
Quoted 3 lines
Maybe it's an extreme corner case, but if I created max_cq CQs, destroyed one, and try to create another one, it is not guaranteed that the create operation would succeed - even though the destroy has finished.
Quoted 6 lines
Quoted 2 lines
More importantly a driver cannot call completion callbacks once destroy cq has returned.
So how is having some kind of synchronization to wait for the call_rcu() callback to finish different than using synchronize_rcu()? We'll have to wait for the readers to finish before returning.
Why do you need to do anything special in addition to nullify completion callback which will ensure that no new readers are coming and call_rcu to make sure that existing readers finished?

Quoted 7 lines

Quoted 3 lines
I ensure there are no new readers by removing the CQ from the xarray. Then I must wait for all existing readers before returning from efa_destroy_cq and freeing the cq struct (which is done by ib_core).
IB/core calls to rdma_restrack_del() which wait_for_completion() before freeing CQ and returning to the users. You don't need to wait in efa_destroy_cq().

The irq flow doesn't call rdma_restrack_get() so I'm not sure how the wait_for_completion() makes a difference here.
And if it does then the code is fine as is? There's nothing the call_rcu() needs to do.

Leon Romanovsky

2021-09-05 13:14 UTC #22 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 92 lines · Gal Pressman

Quoted 91 lines · Leon Romanovsky
Quoted 89 lines · Gal Pressman
Quoted 77 lines · Leon Romanovsky
Quoted 76 lines · Gal Pressman
Quoted 74 lines · Jason Gunthorpe
Quoted 58 lines · Gal Pressman
Quoted 57 lines · Jason Gunthorpe
Quoted 55 lines · Gal Pressman
Quoted 42 lines · Jason Gunthorpe
Quoted 41 lines · Gal Pressman
Quoted 40 lines · Jason Gunthorpe
Quoted 38 lines · Gal Pressman
On 01/09/2021 14:57, Jason Gunthorpe wrote:
On Wed, Sep 01, 2021 at 02:50:42PM +0300, Gal Pressman wrote:
On 20/08/2021 21:27, Jason Gunthorpe wrote:
On Wed, Aug 11, 2021 at 06:11:31PM +0300, Gal Pressman wrote:
diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
pci_release_selected_regions(pdev, release_bars);
}
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+ u16 cqn = eqe->u.comp_event.cqn;
+ struct efa_cq *cq;
+
+ cq = xa_load(&dev->cqs_xa, cqn);
+ if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being
destroyed concurrently?
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and
have a synchronize_rcu() after removing it from the xarray in
destroy_cq.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Try to avoid synchronize_rcu()
I don't see how that's possible?
Usually people use call_rcu() instead
Oh nice, thanks.
I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.
Quoted 5 lines
Quoted 2 lines
I would not be happy to see synchronize_rcu on uverbs destroy functions, it is too easy to DOS the kernel with that.
OK, but isn't the fact that the uverb can return before the CQ is actually destroyed problematic?
Yes, you can't allow that, something other than RCU needs to prevent that
Quoted 3 lines
Maybe it's an extreme corner case, but if I created max_cq CQs, destroyed one, and try to create another one, it is not guaranteed that the create operation would succeed - even though the destroy has finished.
More importantly a driver cannot call completion callbacks once destroy cq has returned.
Quoted 7 lines
Quoted 3 lines
So how is having some kind of synchronization to wait for the call_rcu() callback to finish different than using synchronize_rcu()? We'll have to wait for the readers to finish before returning.
Why do you need to do anything special in addition to nullify completion callback which will ensure that no new readers are coming and call_rcu to make sure that existing readers finished?
I ensure there are no new readers by removing the CQ from the xarray. Then I must wait for all existing readers before returning from efa_destroy_cq and freeing the cq struct (which is done by ib_core).

Quoted 8 lines

Quoted 3 lines
IB/core calls to rdma_restrack_del() which wait_for_completion() before freeing CQ and returning to the users. You don't need to wait in efa_destroy_cq().
The irq flow doesn't call rdma_restrack_get() so I'm not sure how the wait_for_completion() makes a difference here.
And if it does then the code is fine as is? There's nothing the call_rcu() needs to do.

I can't say if it is needed or not, just wanted to understand why you need complexity in destroy_cq path.

Thanks

Gal Pressman

author

2021-09-05 14:36 UTC #23 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 97 lines · Leon Romanovsky

Quoted 96 lines · Gal Pressman
Quoted 94 lines · Leon Romanovsky
Quoted 82 lines · Gal Pressman
Quoted 81 lines · Leon Romanovsky
Quoted 79 lines · Gal Pressman
Quoted 68 lines · Jason Gunthorpe
Quoted 58 lines · Gal Pressman
Quoted 57 lines · Jason Gunthorpe
Quoted 55 lines · Gal Pressman
Quoted 42 lines · Jason Gunthorpe
Quoted 41 lines · Gal Pressman
Quoted 40 lines · Jason Gunthorpe
On Wed, Sep 01, 2021 at 05:24:43PM +0300, Gal Pressman wrote:
On 01/09/2021 14:57, Jason Gunthorpe wrote:
On Wed, Sep 01, 2021 at 02:50:42PM +0300, Gal Pressman wrote:
On 20/08/2021 21:27, Jason Gunthorpe wrote:
On Wed, Aug 11, 2021 at 06:11:31PM +0300, Gal Pressman wrote:
diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 417dea5f90cf..29db4dec02f0 100644
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -67,6 +67,46 @@ static void efa_release_bars(struct efa_dev *dev, int bars_mask)
pci_release_selected_regions(pdev, release_bars);
}
+static void efa_process_comp_eqe(struct efa_dev *dev, struct efa_admin_eqe *eqe)
+{
+ u16 cqn = eqe->u.comp_event.cqn;
+ struct efa_cq *cq;
+
+ cq = xa_load(&dev->cqs_xa, cqn);
+ if (unlikely(!cq)) {
This seems unlikely to be correct, what prevents cq from being
destroyed concurrently?
A comp_handler cannot be running after cq destroy completes.
Sorry for the long turnaround, was OOO.
The CQ cannot be destroyed until all completion events are acked.
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/man/ibv_get_cq_event.3#L45
https://github.com/linux-rdma/rdma-core/blob/7fd01f0c6799f0ecb99cae03c22cf7ff61ffbf5a/libibverbs/cmd_cq.c#L208
That is something quite different, and in userspace.
What in the kernel prevents tha xa_load and the xa_erase from racing together?
Good point.
I think we need to surround efa_process_comp_eqe() with an rcu_read_lock() and
have a synchronize_rcu() after removing it from the xarray in
destroy_cq.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Try to avoid synchronize_rcu()
I don't see how that's possible?
Usually people use call_rcu() instead
Oh nice, thanks.
I think the code would be much simpler using synchronize_rcu(), and the destroy_cq flow is usually on the cold path anyway. I also prefer to be certain that the CQ is freed once the destroy verb returns and not rely on the callback scheduling.
Quoted 5 lines
Quoted 2 lines
I would not be happy to see synchronize_rcu on uverbs destroy functions, it is too easy to DOS the kernel with that.
OK, but isn't the fact that the uverb can return before the CQ is actually destroyed problematic?
Yes, you can't allow that, something other than RCU needs to prevent that
Quoted 6 lines
Quoted 3 lines
Maybe it's an extreme corner case, but if I created max_cq CQs, destroyed one, and try to create another one, it is not guaranteed that the create operation would succeed - even though the destroy has finished.
More importantly a driver cannot call completion callbacks once destroy cq has returned.
So how is having some kind of synchronization to wait for the call_rcu() callback to finish different than using synchronize_rcu()? We'll have to wait for the readers to finish before returning.
Quoted 7 lines
Quoted 3 lines
Why do you need to do anything special in addition to nullify completion callback which will ensure that no new readers are coming and call_rcu to make sure that existing readers finished?
I ensure there are no new readers by removing the CQ from the xarray. Then I must wait for all existing readers before returning from efa_destroy_cq and freeing the cq struct (which is done by ib_core).
IB/core calls to rdma_restrack_del() which wait_for_completion() before freeing CQ and returning to the users. You don't need to wait in efa_destroy_cq().

Quoted 7 lines

Quoted 4 lines
The irq flow doesn't call rdma_restrack_get() so I'm not sure how the wait_for_completion() makes a difference here.
And if it does then the code is fine as is? There's nothing the call_rcu() needs to do.
I can't say if it is needed or not, just wanted to understand why you need complexity in destroy_cq path.

Well, as I said, I don't think the restrack protection is enough in this case as it isn't aware of the concurrent eq flow.

I guess I can put a synchronize_irq() on destroy_cq flow to get rid of the race.

Jason Gunthorpe

maintainer

2021-09-07 11:31 UTC #24 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 7 lines · Gal Pressman

Quoted 2 lines
I can't say if it is needed or not, just wanted to understand why you need complexity in destroy_cq path.
Well, as I said, I don't think the restrack protection is enough in this case as it isn't aware of the concurrent eq flow.
I guess I can put a synchronize_irq() on destroy_cq flow to get rid of the race.

That is a better choice that synchronize_rcu(), IIRC

synchronize_rcu should be avoided compared to all other forms of synchronization because it can take seconds per call to complete, and in a reasonable verbs app this means potentially minutes to close the verbs FD and destroy many CQs.

Jason

Gal Pressman

author

2021-09-09 11:00 UTC #25 lore raw

Re: [PATCH for-next 4/4] RDMA/efa: CQ notifications

Quoted 8 lines · Jason Gunthorpe

Quoted 6 lines · Gal Pressman
Quoted 2 lines
I can't say if it is needed or not, just wanted to understand why you need complexity in destroy_cq path.
Well, as I said, I don't think the restrack protection is enough in this case as it isn't aware of the concurrent eq flow.

Quoted 8 lines

Quoted 1 line
I guess I can put a synchronize_irq() on destroy_cq flow to get rid of the race.
That is a better choice that synchronize_rcu(), IIRC
synchronize_rcu should be avoided compared to all other forms of synchronization because it can take seconds per call to complete, and in a reasonable verbs app this means potentially minutes to close the verbs FD and destroy many CQs.

Got it, will use synchronize_irq, thanks.

Jason Gunthorpe

maintainer Applied

2021-08-20 18:36 UTC #26 lore raw

In reply to Gal Pressman

Quoted 12 lines · Gal Pressman

This series adds support for CQ notifications through the verbs api (ibv_req_notify_cq/ibv_get_cq_event).
In order to achieve that, a new event queue (EQ) object is introduced, which is in charge of reporting completion events to the driver.
In addition, a new CQ doorbell is introduced that is mmapped to the userspace provider in order to arm the CQ when requested by the user.
PR was sent:
https://github.com/linux-rdma/rdma-core/pull/1044

Quoted 7 lines

Thanks,
Gal
Gal Pressman (4):
RDMA/efa: Free IRQ vectors on error flow RDMA/efa: Remove unused cpu field from irq struct RDMA/efa: Rename vector field in efa_irq struct to irqn

I applied these three, thanks

Jason