[PATCH] nvmet-rdma: Avoid o(n^2) loop in delete_ctrl

[PATCH] nvmet-rdma: Avoid o(n^2) loop in delete_ctrl

[Sagi Grimberg]

From: Sagi Grimberg <sagi.grimberg@vastdata.com>

When deleting a nvmet-rdma ctrl, we essentially loop over all
queues that belong to the controller and schedule a removal of
each. Instead of restarting the loop every time a queue is found,
do a simple safe list traversal.

This addresses an unneeded time spent scheduling queue removal in
cases there a lot of queues.

Signed-off-by: Sagi Grimberg <sagi.grimberg@vastdata.com>
---
 drivers/nvme/target/rdma.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 5b8c63e74639..26db04360e8c 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1814,18 +1814,14 @@ static int nvmet_rdma_cm_handler(struct rdma_cm_id *cm_id,
 
 static void nvmet_rdma_delete_ctrl(struct nvmet_ctrl *ctrl)
 {
-	struct nvmet_rdma_queue *queue;
+	struct nvmet_rdma_queue *queue, *tmp;
 
-restart:
 	mutex_lock(&nvmet_rdma_queue_mutex);
-	list_for_each_entry(queue, &nvmet_rdma_queue_list, queue_list) {
-		if (queue->nvme_sq.ctrl == ctrl) {
-			list_del_init(&queue->queue_list);
-			mutex_unlock(&nvmet_rdma_queue_mutex);
-
-			__nvmet_rdma_queue_disconnect(queue);
-			goto restart;
-		}
+	list_for_each_entry_safe(queue, tmp, &nvmet_rdma_queue_list, queue_list) {
+		if (queue->nvme_sq.ctrl != ctrl)
+			continue;
+		list_del_init(&queue->queue_list);
+		__nvmet_rdma_queue_disconnect(queue);
 	}
 	mutex_unlock(&nvmet_rdma_queue_mutex);
 }
-- 
2.40.1

Re: [PATCH] nvmet-rdma: Avoid o(n^2) loop in delete_ctrl

[Christoph Hellwig]

On Sun, May 05, 2024 at 01:39:44PM +0300, Sagi Grimberg wrote:
> From: Sagi Grimberg <sagi.grimberg@vastdata.com>
> 
> When deleting a nvmet-rdma ctrl, we essentially loop over all
> queues that belong to the controller and schedule a removal of
> each. Instead of restarting the loop every time a queue is found,
> do a simple safe list traversal.
> 
> This addresses an unneeded time spent scheduling queue removal in
> cases there a lot of queues.

I think the original reason for this was to avoid lock order dependencies
and/or deadlocks, I wish I would have documented that better.  Looking at
the current version __nvmet_rdma_queue_disconnect I can't find any
obvious problem, but rdma_disconnect is a bit of a block box from
the driver POV.  Did you test this extensively with lockdep enabled?

> +	list_for_each_entry_safe(queue, tmp, &nvmet_rdma_queue_list, queue_list) {

Nit: overly long line here.  Maybe just rename tmp to n?

Re: [PATCH] nvmet-rdma: Avoid o(n^2) loop in delete_ctrl

[Sagi Grimberg]

On 06/05/2024 8:50, Christoph Hellwig wrote:
> On Sun, May 05, 2024 at 01:39:44PM +0300, Sagi Grimberg wrote:
>> From: Sagi Grimberg <sagi.grimberg@vastdata.com>
>>
>> When deleting a nvmet-rdma ctrl, we essentially loop over all
>> queues that belong to the controller and schedule a removal of
>> each. Instead of restarting the loop every time a queue is found,
>> do a simple safe list traversal.
>>
>> This addresses an unneeded time spent scheduling queue removal in
>> cases there a lot of queues.
> I think the original reason for this was to avoid lock order dependencies
> and/or deadlocks, I wish I would have documented that better.  Looking at
> the current version __nvmet_rdma_queue_disconnect I can't find any
> obvious problem, but rdma_disconnect is a bit of a block box from
> the driver POV.

Yes rdma_disconnect is a black box (essentially move the qp to err state 
and send
a cm disconnect request/response). But it is not dependent on
nvmet_rdma_queue_mutex. It is true that in the cm handler we may take this
lock, but that handler has its own context.

The same pattern is used in nvmet_rdma_destroy_port_queues() and
nvmet_rdma_remove_one()

>   Did you test this extensively with lockdep enabled?

I can't say this is extensively tested. I can run blktests and make sure 
lockdep
is enabled.

>
>> +	list_for_each_entry_safe(queue, tmp, &nvmet_rdma_queue_list, queue_list) {
> Nit: overly long line here.  Maybe just rename tmp to n?

Sure.