nvmet-rdma: use a private workqueue for delete

Queue deletion is done asynchronous when the last reference on the queue is dropped. Thus, in order to make sure we don't over allocate under a connect/disconnect storm, we let queue deletion complete before making forward progress.

However, given that we flush the system_wq from rdma_cm context which runs from a workqueue context, we can have a circular locking complaint [1]. Fix that by using a private workqueue for queue deletion.

Quoted 10 lines

Queue deletion is done asynchronous when the last reference on the queue is dropped. Thus, in order to make sure we don't over allocate under a connect/disconnect storm, we let queue deletion complete before making forward progress.

However, given that we flush the system_wq from rdma_cm context which runs from a workqueue context, we can have a circular locking complaint [1]. Fix that by using a private workqueue for queue deletion.

Hi Sagi,

Thanks for this patch. With this patch applied the warning I reported earlier disappears but a new warning appeared:

======================================================
WARNING: possible circular locking dependency detected
4.19.0-rc5-dbg+ #8 Not tainted
------------------------------------------------------
kworker/u24:7/977 is trying to acquire lock:
000000003dc8471f (&id_priv->handler_mutex){+.+.}, at: rdma_destroy_id+0x6f/0x440 [rdma_cm]

but task is already holding lock:
0000000086ca5cb1 ((work_completion)(&queue->release_work)){+.+.}, at: process_one_work+0x3ed/0xa20

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #3 ((work_completion)(&queue->release_work)){+.+.}:

        process_one_work+0x474/0xa20
        worker_thread+0x63/0x5a0
        kthread+0x1cf/0x1f0
        ret_from_fork+0x24/0x30

-> #2 ((wq_completion)"nvmet-rdma-delete-wq"){+.+.}:

        flush_workqueue+0xf3/0x970
        nvmet_rdma_cm_handler+0x1319/0x170f [nvmet_rdma]
        cma_ib_req_handler+0x72f/0xf90 [rdma_cm]
        cm_process_work+0x2e/0x110 [ib_cm]
        cm_req_handler+0x135b/0x1c30 [ib_cm]
        cm_work_handler+0x2b7/0x38cd [ib_cm]
        process_one_work+0x4ae/0xa20
        worker_thread+0x63/0x5a0
        kthread+0x1cf/0x1f0
        ret_from_fork+0x24/0x30

-> #1 (&id_priv->handler_mutex/1){+.+.}:

        __mutex_lock+0xfe/0xbe0
        mutex_lock_nested+0x1b/0x20
        cma_ib_req_handler+0x6aa/0xf90 [rdma_cm]
        cm_process_work+0x2e/0x110 [ib_cm]
        cm_req_handler+0x135b/0x1c30 [ib_cm]
        cm_work_handler+0x2b7/0x38cd [ib_cm]
        process_one_work+0x4ae/0xa20
        worker_thread+0x63/0x5a0
        kthread+0x1cf/0x1f0
        ret_from_fork+0x24/0x30

-> #0 (&id_priv->handler_mutex){+.+.}:

        lock_acquire+0xc5/0x200
        __mutex_lock+0xfe/0xbe0
        mutex_lock_nested+0x1b/0x20
        rdma_destroy_id+0x6f/0x440 [rdma_cm]
        nvmet_rdma_release_queue_work+0x8e/0x1b0 [nvmet_rdma]
        process_one_work+0x4ae/0xa20
        worker_thread+0x63/0x5a0
        kthread+0x1cf/0x1f0
        ret_from_fork+0x24/0x30

other info that might help us debug this:

Chain exists of:
   &id_priv->handler_mutex --> (wq_completion)"nvmet-rdma-delete-wq" --> (work_completion)(&queue->release_work)

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock((work_completion)(&queue->release_work));
                                lock((wq_completion)"nvmet-rdma-delete-wq");
                                lock((work_completion)(&queue->release_work));

lock(&id_priv->handler_mutex);

*** DEADLOCK ***

2 locks held by kworker/u24:7/977:
#0: 00000000148450ab ((wq_completion)"nvmet-rdma-delete-wq"){+.+.}, at: process_one_work+0x3ed/0xa20 #1: 0000000086ca5cb1 ((work_completion)(&queue->release_work)){+.+.}, at: process_one_work+0x3ed/0xa20

stack backtrace:
CPU: 1 PID: 977 Comm: kworker/u24:7 Not tainted 4.19.0-rc5-dbg+ #8
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Workqueue: nvmet-rdma-delete-wq nvmet_rdma_release_queue_work [nvmet_rdma]
Call Trace:
  dump_stack+0x86/0xc5
  print_circular_bug.isra.32+0x20a/0x218
  __lock_acquire+0x1a54/0x1b20
  lock_acquire+0xc5/0x200
  __mutex_lock+0xfe/0xbe0
  mutex_lock_nested+0x1b/0x20
  rdma_destroy_id+0x6f/0x440 [rdma_cm]
  nvmet_rdma_release_queue_work+0x8e/0x1b0 [nvmet_rdma]
  process_one_work+0x4ae/0xa20
  worker_thread+0x63/0x5a0
  kthread+0x1cf/0x1f0
  ret_from_fork+0x24/0x30

Quoted 10 lines

Quoted 9 lines

Queue deletion is done asynchronous when the last reference on the queue is dropped. Thus, in order to make sure we don't over allocate under a connect/disconnect storm, we let queue deletion complete before making forward progress.

However, given that we flush the system_wq from rdma_cm context which runs from a workqueue context, we can have a circular locking complaint [1]. Fix that by using a private workqueue for queue deletion.

Quoted 4 lines

Hi Sagi,

Thanks for this patch. With this patch applied the warning I reported earlier disappears but a new warning appeared:

Thanks for testing, this is a similar complaint though...

What I'm missing here is why flushing a work that runs on workqueue A can't be done from a work that runs on workqueue B. It is guaranteed that the id_priv that is used as a barrier from rdma_destroy_id is different from the id_priv that is handling the connect. So I'm not clear on what is the dependency yet.

Any insights?

It seems like this has not yet been fixed entirely. This is what appeared in the kernel log this morning on my test setup with Christoph's nvme-4.20 branch (commit cb4bfda62afa ("nvme-pci: fix hot removal during error handling")):

======================================================
WARNING: possible circular locking dependency detected 4.19.0-rc6-dbg+ #1 Not tainted
------------------------------------------------------
kworker/u16:7/169 is trying to acquire lock:
00000000578ccf82 (&id_priv->handler_mutex){+.+.}, at:
rdma_destroy_id+0x6f/0x440 [rdma_cm]

but task is already holding lock:
000000005d67271b ((work_completion)(&queue->release_work)){+.+.}, at:
process_one_work+0x3ed/0xa20

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #3 ((work_completion)(&queue->release_work)){+.+.}:
? ? ? ?process_one_work+0x474/0xa20
? ? ? ?worker_thread+0x63/0x5a0
? ? ? ?kthread+0x1cf/0x1f0
? ? ? ?ret_from_fork+0x24/0x30

-> #2 ((wq_completion)"nvmet-rdma-delete-wq"){+.+.}:
? ? ? ?flush_workqueue+0xf3/0x970
? ? ? ?nvmet_rdma_cm_handler+0x1319/0x170f [nvmet_rdma] ? ? ? ?cma_ib_req_handler+0x72f/0xf90 [rdma_cm] ? ? ? ?cm_process_work+0x2e/0x110 [ib_cm]
? ? ? ?cm_req_handler+0x135b/0x1c30 [ib_cm]
? ? ? ?cm_work_handler+0x2b7/0x38cd [ib_cm]
? ? ? ?process_one_work+0x4ae/0xa20
? ? ? ?worker_thread+0x63/0x5a0
? ? ? ?kthread+0x1cf/0x1f0
? ? ? ?ret_from_fork+0x24/0x30

-> #1 (&id_priv->handler_mutex/1){+.+.}:
? ? ? ?__mutex_lock+0xfe/0xbe0
? ? ? ?mutex_lock_nested+0x1b/0x20
? ? ? ?cma_ib_req_handler+0x6aa/0xf90 [rdma_cm]
? ? ? ?cm_process_work+0x2e/0x110 [ib_cm]
? ? ? ?cm_req_handler+0x135b/0x1c30 [ib_cm]
? ? ? ?cm_work_handler+0x2b7/0x38cd [ib_cm]
? ? ? ?process_one_work+0x4ae/0xa20
? ? ? ?worker_thread+0x63/0x5a0
? ? ? ?kthread+0x1cf/0x1f0
? ? ? ?ret_from_fork+0x24/0x30

-> #0 (&id_priv->handler_mutex){+.+.}:
? ? ? ?lock_acquire+0xd2/0x210
? ? ? ?__mutex_lock+0xfe/0xbe0
? ? ? ?mutex_lock_nested+0x1b/0x20
? ? ? ?rdma_destroy_id+0x6f/0x440 [rdma_cm]
? ? ? ?nvmet_rdma_release_queue_work+0x8e/0x1b0 [nvmet_rdma] ? ? ? ?process_one_work+0x4ae/0xa20
? ? ? ?worker_thread+0x63/0x5a0
? ? ? ?kthread+0x1cf/0x1f0
? ? ? ?ret_from_fork+0x24/0x30

other info that might help us debug this:

Chain exists of:
 ? &id_priv->handler_mutex --> (wq_completion)"nvmet-rdma-delete-wq" --> 

(work_completion)(&queue->release_work)

?Possible unsafe locking scenario:

? ? ? ?CPU0 ? ? ? ? ? ? ? ? ? ?CPU1
? ? ? ?---- ? ? ? ? ? ? ? ? ? ?----
? lock((work_completion)(&queue->release_work));
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?lock((wq_completion)"nvmet-rdma-delete-wq");

?lock((work_completion)(&queue->release_work));
? lock(&id_priv->handler_mutex);

?*** DEADLOCK ***

2 locks held by kworker/u16:7/169:
?#0: 00000000a32d4be9 ((wq_completion)"nvmet-rdma-delete-wq"){+.+.},
at: process_one_work+0x3ed/0xa20
?#1: 000000005d67271b ((work_completion)(&queue->release_work)){+.+.},
at: process_one_work+0x3ed/0xa20

stack backtrace:
CPU: 1 PID: 169 Comm: kworker/u16:7 Not tainted 4.19.0-rc6-dbg+ #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
Workqueue: nvmet-rdma-delete-wq nvmet_rdma_release_queue_work [nvmet_rdma]
Call Trace:
 ?dump_stack+0xa4/0xf5
 ?print_circular_bug.isra.32+0x20a/0x218
 ?__lock_acquire+0x1a5e/0x1b20
 ?lock_acquire+0xd2/0x210
 ?__mutex_lock+0xfe/0xbe0
 ?mutex_lock_nested+0x1b/0x20
 ?rdma_destroy_id+0x6f/0x440 [rdma_cm]
 ?nvmet_rdma_release_queue_work+0x8e/0x1b0 [nvmet_rdma]
 ?process_one_work+0x4ae/0xa20
 ?worker_thread+0x63/0x5a0
 ?kthread+0x1cf/0x1f0
 ?ret_from_fork+0x24/0x30

Quoted 4 lines · Sagi Grimberg

Quoted 4 lines

It seems like this has not yet been fixed entirely. This is what appeared in the kernel log this morning on my test setup with Christoph's nvme-4.20 branch (commit cb4bfda62afa ("nvme-pci: fix hot removal during error handling")):

Quoted 5 lines

There is something I'm missing here, the id_priv->handler_mutex that the connect context is running on is guaranteed to be different than the one being removed (a different cm_id) and also the workqueues are different.

Quoted 2 lines

Is it not allowed to flush workqueue A from a work that is hosted on workqueue B?

It seems like we ran into a lockdep complaint triggered by a recently queued patch (87915adc3f0a ("workqueue: re-add lockdep dependencies for flushing")). However, it's not clear to us whether anything is wrong with the code the complaint refers to. Can any of you have a look? I have attached the lockep complaint to this e-mail.

Quoted 5 lines · Bart Van Assche

Quoted 4 lines · Sagi Grimberg

Quoted 4 lines

It seems like this has not yet been fixed entirely. This is what appeared in the kernel log this morning on my test setup with Christoph's nvme-4.20 branch (commit cb4bfda62afa ("nvme-pci: fix hot removal during error handling")):

FWIW, I'm not sure where to find this tree (it's not on kernel.org, apparently, at least none of hch's?). As a result, I don't have the correct code here now.

Quoted 7 lines · Johannes Berg

Quoted 5 lines · Bart Van Assche

Quoted 4 lines · Sagi Grimberg

Quoted 4 lines

It seems like this has not yet been fixed entirely. This is what appeared in the kernel log this morning on my test setup with Christoph's nvme-4.20 branch (commit cb4bfda62afa ("nvme-pci: fix hot removal during error handling")):

Quoted 3 lines

FWIW, I'm not sure where to find this tree (it's not on kernel.org, apparently, at least none of hch's?). As a result, I don't have the correct code here now.

Christoph's NVMe tree is available at git://git.infradead.org/nvme.git.

Quoted 8 lines · Bart Van Assche

Quoted 7 lines · Johannes Berg

Quoted 5 lines · Bart Van Assche

Quoted 4 lines · Sagi Grimberg

Quoted 4 lines

It seems like this has not yet been fixed entirely. This is what appeared in the kernel log this morning on my test setup with Christoph's nvme-4.20 branch (commit cb4bfda62afa ("nvme-pci: fix hot removal during error handling")):

Quoted 5 lines

Quoted 3 lines

FWIW, I'm not sure where to find this tree (it's not on kernel.org, apparently, at least none of hch's?). As a result, I don't have the correct code here now.

Christoph's NVMe tree is available at git://git.infradead.org/nvme.git.

Ok, thanks. I think I'll go off Sagi Grimberg's explanation though rather than try to understand the code myself.

Quoted 9 lines · Johannes Berg

Quoted 8 lines · Bart Van Assche

Quoted 7 lines · Johannes Berg

Quoted 5 lines · Bart Van Assche

Quoted 4 lines · Sagi Grimberg

Quoted 4 lines

It seems like this has not yet been fixed entirely. This is what appeared in the kernel log this morning on my test setup with Christoph's nvme-4.20 branch (commit cb4bfda62afa ("nvme-pci: fix hot removal during error handling")):

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

FWIW, I'm not sure where to find this tree (it's not on kernel.org, apparently, at least none of hch's?). As a result, I don't have the correct code here now.

Christoph's NVMe tree is available at git://git.infradead.org/nvme.git.

Ok, thanks. I think I'll go off Sagi Grimberg's explanation though rather than try to understand the code myself.

Are the lockdep annotations in kernel/workqueue.c correct? My understanding is that lock_map_acquire() should only be used to annotate mutually exclusive locking (e.g. mutex, spinlock). However, multiple work items associated with the same workqueue can be executed concurrently. From lockdep.h:

#define lock_map_acquire(l) lock_acquire_exclusive(l, 0, 0, NULL, _THIS_IP_)

Quoted 24 lines · Bart Van Assche

Quoted 9 lines · Johannes Berg

Quoted 8 lines · Bart Van Assche

Quoted 7 lines · Johannes Berg

Quoted 5 lines · Bart Van Assche

Quoted 4 lines · Sagi Grimberg

Quoted 4 lines

It seems like this has not yet been fixed entirely. This is what appeared in the kernel log this morning on my test setup with Christoph's nvme-4.20 branch (commit cb4bfda62afa ("nvme-pci: fix hot removal during error handling")):

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

FWIW, I'm not sure where to find this tree (it's not on kernel.org, apparently, at least none of hch's?). As a result, I don't have the correct code here now.

Christoph's NVMe tree is available at git://git.infradead.org/nvme.git.

Ok, thanks. I think I'll go off Sagi Grimberg's explanation though rather than try to understand the code myself.

Are the lockdep annotations in kernel/workqueue.c correct? My understanding is that lock_map_acquire() should only be used to annotate mutually exclusive locking (e.g. mutex, spinlock). However, multiple work items associated with the same workqueue can be executed concurrently. From lockdep.h:

Quoted 1 line

#define lock_map_acquire(l) lock_acquire_exclusive(l, 0, 0, NULL, _THIS_IP_)

I've talked about this in the other thread, in the interest of keeping things together, can you ask that again over there?

Quoted 6 lines

Quoted 6 lines

There is something I'm missing here, the id_priv->handler_mutex that the connect context is running on is guaranteed to be different than the one being removed (a different cm_id) and also the workqueues are different.

Quoted 8 lines

Quoted 2 lines

Is it not allowed to flush workqueue A from a work that is hosted on workqueue B?

It seems like we ran into a lockdep complaint triggered by a recently queued patch (87915adc3f0a ("workqueue: re-add lockdep dependencies for flushing")). However, it's not clear to us whether anything is wrong with the code the complaint refers to. Can any of you have a look? I have attached the lockep complaint to this e-mail.

Is it possible that you're just running into the general lockdep limitation in that it doesn't know anything about *instances*, just *classes*?

Without looking at the correct code I don't want to try to diagnose this further, but that's what it seems like?

It's clearly telling you that you're acquiring "id_priv->handler_mutex" and lists a long dependency chain:

   &id_priv->handler_mutex --> (wq_completion)"nvmet-rdma-delete-wq" --> (work_completion)(&queue->release_work)

so that you shouldn't be trying to acquire this mutex again from running inside the release work:

kworker/u16:7/169 is trying to acquire lock:

   00000000578ccf82 (&id_priv->handler_mutex){+.+.}, at: rdma_destroy_id+0x6f/0x440 [rdma_cm]

   but task is already holding lock:
   000000005d67271b ((work_completion)(&queue->release_work)){+.+.}, at: process_one_work+0x3ed/0xa20

This looks like lockdep is working as it should.

If these are indeed (guaranteed to be) different, then I'd say you'd have to split them into different classes. However, I have no idea what the rules are for the recursion here, so I can't really say how you could do that.

Quoted 1 line

Quoted 1 line

Quoted 1 line

I'm not sure how I can divide them into classes. The priv is really an internal structure that surrounds a connection connection representation. The priv->handler_mutex wraps every event handling dispatched to the upper layer consumer.

The connection destruction barriers by acquiring and releasing this event_handler mutex such that no events are handled by the

See drivers/infiniband/core/cma.c rdma_destroy_id()

In our case, one of the event handlers flushes a workqueue that is hosting work items that essentially call rdma_destroy_id() on connections that are guaranteed not to be the one currently handling the event. So the priv is guaranteed to be a different instance.