IB/core: Avoid deadlock during netlink message handling

PATCH In review Leon Romanovsky 3 participants 12 messages Oct 15, 2019 – Oct 24, 2019 InfiniBand

lore

Share & tools

lore

Leon Romanovsky

author

2019-10-15 08:07 UTC #0 lore raw

PATCH 1/1 IB/core: Avoid deadlock during netlink message handling

From: Parav Pandit <[email protected]>

When rdmacm module is not loaded, and when netlink message is received to get char device info, it results into a deadlock due to recursive locking rdma_nl_mutex in below call sequence.

[..]
rdma_nl_rcv()
mutex_lock()
rdma_nl_rcv_msg()

      ib_get_client_nl_info()
         request_module()
           iw_cm_init()
             rdma_nl_register()
               mutex_lock(); <- Deadlock, acquiring mutex again

Due to above call sequence, following call trace and deadlock is observed.

kernel: __mutex_lock+0x35e/0x860
kernel: ? __mutex_lock+0x129/0x860
kernel: ? rdma_nl_register+0x1a/0x90 [ib_core]
kernel: rdma_nl_register+0x1a/0x90 [ib_core]
kernel: ? 0xffffffffc029b000
kernel: iw_cm_init+0x34/0x1000 [iw_cm]
kernel: do_one_initcall+0x67/0x2d4
kernel: ? kmem_cache_alloc_trace+0x1ec/0x2a0
kernel: do_init_module+0x5a/0x223
kernel: load_module+0x1998/0x1e10
kernel: ? __symbol_put+0x60/0x60
kernel: __do_sys_finit_module+0x94/0xe0
kernel: do_syscall_64+0x5a/0x270
kernel: entry_SYSCALL_64_after_hwframe+0x49/0xbe

process stack trace:

[<0>] __request_module+0x1c9/0x460
[<0>] ib_get_client_nl_info+0x5e/0xb0 [ib_core]
[<0>] nldev_get_chardev+0x1ac/0x320 [ib_core]
[<0>] rdma_nl_rcv_msg+0xeb/0x1d0 [ib_core]
[<0>] rdma_nl_rcv+0xcd/0x120 [ib_core]
[<0>] netlink_unicast+0x179/0x220
[<0>] netlink_sendmsg+0x2f6/0x3f0
[<0>] sock_sendmsg+0x30/0x40
[<0>] ___sys_sendmsg+0x27a/0x290
[<0>] __sys_sendmsg+0x58/0xa0
[<0>] do_syscall_64+0x5a/0x270
[<0>] entry_SYSCALL_64_after_hwframe+0x49/0xbe

To overcome this deadlock and to allow multiple netlink messages to progress in parallel, following scheme is implemented.

1. Block netlink table unregistration of a client until all the callers finish executing callback for a given client.

2. Netlink clients shouldn't register table multiple times for a given index. Such basic requirement from two non IB core module eliminates mutex usage for table registratio,

Fixes 0e2d00eb6fd45 ("RDMA: Add NLDEV_GET_CHARDEV to allow char dev discovery and autoload")

Signed-off-by Parav Pandit <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/core_priv.h |  1 +
 drivers/infiniband/core/device.c    |  2 +
 drivers/infiniband/core/netlink.c   | 93 ++++++++++++++++-------------
 3 files changed, 54 insertions(+), 42 deletions(-)

drivers/infiniband/core/core_priv.h +1 -0

index 3a8b0911c3bc..9d07378b5b42 100644
@@ -199,6 +199,7 @@ void ib_mad_cleanup(void);
 int ib_sa_init(void);
 void ib_sa_cleanup(void);
 
+void rdma_nl_init(void);
 void rdma_nl_exit(void);
 
 int ib_nl_handle_resolve_resp(struct sk_buff *skb,

drivers/infiniband/core/device.c +2 -0

index 2e53aa25f0c7..2f89c4d64b73 100644
@@ -2716,6 +2716,8 @@ static int __init ib_core_init(void)
 		goto err_comp_unbound;
 	}
 
+	rdma_nl_init();
+
 	ret = addr_init();
 	if (ret) {
 		pr_warn("Could't init IB address resolution\n");

drivers/infiniband/core/netlink.c +51 ~~-42~~

182 lines not on this page · raw message

Signature · 1 line

2.20.1

Jason Gunthorpe

2019-10-24 13:17 UTC #1 lore raw

Quoted 5 lines · Leon Romanovsky

drivers/infiniband/core/netlink.c +1 -0

index 81dbd5f41bed..a3507b8be569 100644
+++ b/drivers/infiniband/core/netlink.c

@@ -42,9 +42,12 @@
 #include <linux/module.h>

drivers/infiniband/core/netlink.c line 50

Quoted 10 lines

drivers/infiniband/core/netlink.c +5 -2

 #include "core_priv.h"
 
-static DEFINE_MUTEX(rdma_nl_mutex);
 static struct {
-	const struct rdma_nl_cbs   *cb_table;
+	const struct rdma_nl_cbs __rcu *cb_table;
+	/* Synchronizes between ongoing netlink commands and netlink client
+	 * unregistration.
+	 */
+	struct srcu_struct unreg_srcu;

A srcu in every index is serious overkill for this. Lets just us a rwsem:

From 3ab38b889ab6b85b8689a8cd9bccbf2b28711677 Mon Sep 17 00:00:00 2001
From: Parav Pandit <[email protected]>
Date: Tue, 15 Oct 2019 11:07:33 +0300
Subject: [PATCH] IB/core: Avoid deadlock during netlink message handling

When rdmacm module is not loaded, and when netlink message is received to get char device info, it results into a deadlock due to recursive locking of rdma_nl_mutex with the below call sequence.

[..]
rdma_nl_rcv()
mutex_lock()
[..]
rdma_nl_rcv_msg()

      ib_get_client_nl_info()
         request_module()
           iw_cm_init()
             rdma_nl_register()
               mutex_lock(); <- Deadlock, acquiring mutex again

Due to above call sequence, following call trace and deadlock is observed.

process stack trace:

  [<0>] __request_module+0x1c9/0x460
  [<0>] ib_get_client_nl_info+0x5e/0xb0 [ib_core]
  [<0>] nldev_get_chardev+0x1ac/0x320 [ib_core]
  [<0>] rdma_nl_rcv_msg+0xeb/0x1d0 [ib_core]
  [<0>] rdma_nl_rcv+0xcd/0x120 [ib_core]
  [<0>] netlink_unicast+0x179/0x220
  [<0>] netlink_sendmsg+0x2f6/0x3f0
  [<0>] sock_sendmsg+0x30/0x40
  [<0>] ___sys_sendmsg+0x27a/0x290
  [<0>] __sys_sendmsg+0x58/0xa0
  [<0>] do_syscall_64+0x5a/0x270
  [<0>] entry_SYSCALL_64_after_hwframe+0x49/0xbe

To overcome this deadlock and to allow multiple netlink messages to progress in parallel, following scheme is implemented.

1. Split the lock protecting the cb_table into a per-index lock, and make it a rwlock. This lock is used to ensure no callbacks are running after unregistration returns. Since a module will not be registered once it is already running callbacks, this avoids the deadlock.

2. Use smp_store_release() to update the cb_table during registration so that no lock is required. This avoids lockdep problems with thinking all the rwsems are the same lock class.

Fixes 0e2d00eb6fd45 ("RDMA: Add NLDEV_GET_CHARDEV to allow char dev discovery and autoload")

Link https://lore.kernel.org/r/[email protected] open here

Signed-off-by Parav Pandit <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

Signed-off-by Jason Gunthorpe <[email protected]>

---
 drivers/infiniband/core/core_priv.h |   1 +
 drivers/infiniband/core/device.c    |   2 +
 drivers/infiniband/core/netlink.c   | 107 ++++++++++++++--------------
 3 files changed, 56 insertions(+), 54 deletions(-)

drivers/infiniband/core/core_priv.h +1 -0

index 3a8b0911c3bc16..9d07378b5b4230 100644
@@ -199,6 +199,7 @@ void ib_mad_cleanup(void);
 int ib_sa_init(void);
 void ib_sa_cleanup(void);
 
+void rdma_nl_init(void);
 void rdma_nl_exit(void);
 
 int ib_nl_handle_resolve_resp(struct sk_buff *skb,

drivers/infiniband/core/device.c +2 -0

index 2dd2cfe9b56136..50a92442c4f7c1 100644
@@ -2716,6 +2716,8 @@ static int __init ib_core_init(void)
 		goto err_comp_unbound;
 	}
 
+	rdma_nl_init();
+
 	ret = addr_init();
 	if (ret) {
 		pr_warn("Could't init IB address resolution\n");

drivers/infiniband/core/netlink.c +53 ~~-54~~

184 lines not on this page · raw message

Signature · 1 line

2.23.0

Leon Romanovsky

author

2019-10-24 13:26 UTC #2 lore raw

Quoted 9 lines · Jason Gunthorpe

Quoted 7 lines · Leon Romanovsky

drivers/infiniband/core/netlink.c +1 -0

index 81dbd5f41bed..a3507b8be569 100644
+++ b/drivers/infiniband/core/netlink.c

@@ -42,9 +42,12 @@
 #include <linux/module.h>
 #include "core_priv.h"

Quoted 11 lines

drivers/infiniband/core/netlink.c
Quoted 8 lines
drivers/infiniband/core/netlink.c +5 -2
-static DEFINE_MUTEX(rdma_nl_mutex);
 static struct {
-	const struct rdma_nl_cbs   *cb_table;
+	const struct rdma_nl_cbs __rcu *cb_table;
+	/* Synchronizes between ongoing netlink commands and netlink client
+	 * unregistration.
+	 */
+	struct srcu_struct unreg_srcu;
A srcu in every index is serious overkill for this. Lets just us a rwsem:

I liked previous variant more than rwsem, but it is Parav's patch.

Thanks

Jason Gunthorpe

2019-10-24 13:50 UTC #3 lore raw

Quoted 22 lines · Leon Romanovsky

Quoted 9 lines · Jason Gunthorpe
Quoted 7 lines · Leon Romanovsky
drivers/infiniband/core/netlink.c +1 -0
index 81dbd5f41bed..a3507b8be569 100644
+++ b/drivers/infiniband/core/netlink.c
@@ -42,9 +42,12 @@
 #include <linux/module.h>
 #include "core_priv.h"
 
Quoted 11 lines
drivers/infiniband/core/netlink.c
Quoted 8 lines
drivers/infiniband/core/netlink.c +5 -2
-static DEFINE_MUTEX(rdma_nl_mutex);
 static struct {
-	const struct rdma_nl_cbs   *cb_table;
+	const struct rdma_nl_cbs __rcu *cb_table;
+	/* Synchronizes between ongoing netlink commands and netlink client
+	 * unregistration.
+	 */
+	struct srcu_struct unreg_srcu;
A srcu in every index is serious overkill for this. Lets just us a rwsem:

Quoted 1 line

I liked previous variant more than rwsem, but it is Parav's patch.

Why? srcu is a huge data structure and slow on unregister

Jason

Leon Romanovsky

author

2019-10-24 16:02 UTC #4 lore raw

Quoted 23 lines · Jason Gunthorpe

Quoted 22 lines · Leon Romanovsky
Quoted 20 lines · Jason Gunthorpe
Quoted 5 lines · Leon Romanovsky
drivers/infiniband/core/netlink.c +1 -0
index 81dbd5f41bed..a3507b8be569 100644
+++ b/drivers/infiniband/core/netlink.c
@@ -42,9 +42,12 @@
 #include <linux/module.h>
drivers/infiniband/core/netlink.c line 50
Quoted 10 lines
drivers/infiniband/core/netlink.c +5 -2
 #include "core_priv.h"
 
-static DEFINE_MUTEX(rdma_nl_mutex);
 static struct {
-	const struct rdma_nl_cbs   *cb_table;
+	const struct rdma_nl_cbs __rcu *cb_table;
+	/* Synchronizes between ongoing netlink commands and netlink client
+	 * unregistration.
+	 */
+	struct srcu_struct unreg_srcu;
A srcu in every index is serious overkill for this. Lets just us a rwsem:

Quoted 3 lines

Quoted 1 line
I liked previous variant more than rwsem, but it is Parav's patch.
Why? srcu is a huge data structure and slow on unregister

The unregister time is not so important for those IB/core modules. I liked SRCU because it doesn't have *_ONCE() macros and smb_* calls.

Maybe wrong here, but the extra advantage of SRCU is that we are already using that mechanism in uverbs and my assumption that SRCU will greatly enjoy shared grace period.

Thanks

Quoted 2 lines

Jason

Jason Gunthorpe

2019-10-24 16:08 UTC #5 lore raw

Quoted 24 lines · Leon Romanovsky

Quoted 23 lines · Jason Gunthorpe
Quoted 22 lines · Leon Romanovsky
Quoted 20 lines · Jason Gunthorpe
Quoted 5 lines · Leon Romanovsky
drivers/infiniband/core/netlink.c +1 -0
index 81dbd5f41bed..a3507b8be569 100644
+++ b/drivers/infiniband/core/netlink.c
@@ -42,9 +42,12 @@
 #include <linux/module.h>
drivers/infiniband/core/netlink.c line 50
Quoted 10 lines
drivers/infiniband/core/netlink.c +5 -2
 #include "core_priv.h"
 
-static DEFINE_MUTEX(rdma_nl_mutex);
 static struct {
-	const struct rdma_nl_cbs   *cb_table;
+	const struct rdma_nl_cbs __rcu *cb_table;
+	/* Synchronizes between ongoing netlink commands and netlink client
+	 * unregistration.
+	 */
+	struct srcu_struct unreg_srcu;
A srcu in every index is serious overkill for this. Lets just us a rwsem:

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
I liked previous variant more than rwsem, but it is Parav's patch.
Why? srcu is a huge data structure and slow on unregister
The unregister time is not so important for those IB/core modules. I liked SRCU because it doesn't have *_ONCE() macros and smb_* calls.

It does, they are just hidden under other macros..

Quoted 3 lines

Maybe wrong here, but the extra advantage of SRCU is that we are already using that mechanism in uverbs and my assumption that SRCU will greatly enjoy shared grace period.

Hm, I'm not sure it works like that, the grace periods would be consecutive

Jason

Leon Romanovsky

author

2019-10-24 16:13 UTC #6 lore raw

Quoted 32 lines · Jason Gunthorpe

Quoted 24 lines · Leon Romanovsky
Quoted 23 lines · Jason Gunthorpe
Quoted 22 lines · Leon Romanovsky
Quoted 20 lines · Jason Gunthorpe
Quoted 5 lines · Leon Romanovsky
drivers/infiniband/core/netlink.c +1 -0
index 81dbd5f41bed..a3507b8be569 100644
+++ b/drivers/infiniband/core/netlink.c
@@ -42,9 +42,12 @@
 #include <linux/module.h>
drivers/infiniband/core/netlink.c line 50
Quoted 10 lines
drivers/infiniband/core/netlink.c +5 -2
 #include "core_priv.h"
 
-static DEFINE_MUTEX(rdma_nl_mutex);
 static struct {
-	const struct rdma_nl_cbs   *cb_table;
+	const struct rdma_nl_cbs __rcu *cb_table;
+	/* Synchronizes between ongoing netlink commands and netlink client
+	 * unregistration.
+	 */
+	struct srcu_struct unreg_srcu;
A srcu in every index is serious overkill for this. Lets just us a rwsem:
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
I liked previous variant more than rwsem, but it is Parav's patch.
Why? srcu is a huge data structure and slow on unregister
The unregister time is not so important for those IB/core modules. I liked SRCU because it doesn't have *_ONCE() macros and smb_* calls.

Quoted 7 lines

It does, they are just hidden under other macros..
Quoted 3 lines
Maybe wrong here, but the extra advantage of SRCU is that we are already using that mechanism in uverbs and my assumption that SRCU will greatly enjoy shared grace period.
Hm, I'm not sure it works like that, the grace periods would be consecutive

Whatever, the most important part of Parav's patch that we removed global lock from RDMA netlink interface.

Thanks

Quoted 2 lines

Jason

Parav Pandit

2019-10-24 18:28 UTC #7 lore raw

Quoted 26 lines

-----Original Message-----
From: Leon Romanovsky <[email protected]>
Sent: Thursday, October 24, 2019 11:13 AM
To: Jason Gunthorpe <[email protected]>
Cc: Doug Ledford <[email protected]>; Parav Pandit <[email protected]>; RDMA mailing list <[email protected]>
Subject: Re: [PATCH rdma-next] IB/core: Avoid deadlock during netlink message handling
Quoted 14 lines · Jason Gunthorpe
Quoted 13 lines · Leon Romanovsky
Quoted 12 lines · Jason Gunthorpe
Quoted 11 lines · Leon Romanovsky
Quoted 10 lines · Jason Gunthorpe
Quoted 8 lines · Leon Romanovsky
drivers/infiniband/core/netlink.c +1 -0
index 81dbd5f41bed..a3507b8be569 100644
+++ b/drivers/infiniband/core/netlink.c
@@ -42,9 +42,12 @@
 #include <linux/module.h>
 #include "core_priv.h"
 

Quoted 22 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/infiniband/core/netlink.c line 46
Quoted 4 lines
drivers/infiniband/core/netlink.c +2 -2
-static DEFINE_MUTEX(rdma_nl_mutex);  static struct {
-	const struct rdma_nl_cbs   *cb_table;
+	const struct rdma_nl_cbs __rcu *cb_table;
+	/* Synchronizes between ongoing netlink commands and
netlink client
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
drivers/infiniband/core/netlink.c
Quoted 3 lines
drivers/infiniband/core/netlink.c +3 -0
+	 * unregistration.
+	 */
+	struct srcu_struct unreg_srcu;
A srcu in every index is serious overkill for this. Lets just us a
rwsem:
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
I liked previous variant more than rwsem, but it is Parav's patch.
Why? srcu is a huge data structure and slow on unregister
The unregister time is not so important for those IB/core modules. I liked SRCU because it doesn't have *_ONCE() macros and smb_* calls.
It does, they are just hidden under other macros..

Its better that they are hidden. So that we don't need open code them.
Also with srcu, we don't need lock annotations in get_cb_table() which releases and acquires semaphore.
Additionally lock nesting makes overall more complex.
Given that there are only 3 indices, out of which only 2 are outside of the ib_core module and unlikely to be unloaded, I also prefer srcu version.

Quoted 14 lines

Quoted 6 lines
Quoted 3 lines
Maybe wrong here, but the extra advantage of SRCU is that we are already using that mechanism in uverbs and my assumption that SRCU will greatly enjoy shared grace period.
Hm, I'm not sure it works like that, the grace periods would be consecutive
Whatever, the most important part of Parav's patch that we removed global lock from RDMA netlink interface.
Thanks
Quoted 2 lines
Jason

Jason Gunthorpe

2019-10-24 18:36 UTC #8 lore raw

Quoted 42 lines · Parav Pandit

Quoted 42 lines
From: Leon Romanovsky <[email protected]>
Sent: Thursday, October 24, 2019 11:13 AM
To: Jason Gunthorpe <[email protected]>
Cc: Doug Ledford <[email protected]>; Parav Pandit <[email protected]>; RDMA mailing list <[email protected]>
Subject: Re: [PATCH rdma-next] IB/core: Avoid deadlock during netlink message handling
Quoted 14 lines · Jason Gunthorpe
Quoted 13 lines · Leon Romanovsky
Quoted 12 lines · Jason Gunthorpe
Quoted 11 lines · Leon Romanovsky
Quoted 10 lines · Jason Gunthorpe
Quoted 8 lines · Leon Romanovsky
drivers/infiniband/core/netlink.c +1 -0
index 81dbd5f41bed..a3507b8be569 100644
+++ b/drivers/infiniband/core/netlink.c
@@ -42,9 +42,12 @@
 #include <linux/module.h>
 #include "core_priv.h"
 
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/infiniband/core/netlink.c line 46
Quoted 4 lines
drivers/infiniband/core/netlink.c +2 -2
-static DEFINE_MUTEX(rdma_nl_mutex);  static struct {
-	const struct rdma_nl_cbs   *cb_table;
+	const struct rdma_nl_cbs __rcu *cb_table;
+	/* Synchronizes between ongoing netlink commands and
netlink client
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Quoted 9 lines
Quoted 7 lines
drivers/infiniband/core/netlink.c line 49
Quoted 3 lines
drivers/infiniband/core/netlink.c +3 -0
+	 * unregistration.
+	 */
+	struct srcu_struct unreg_srcu;
A srcu in every index is serious overkill for this. Lets just us a
rwsem:
I liked previous variant more than rwsem, but it is Parav's patch.
Why? srcu is a huge data structure and slow on unregister

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
The unregister time is not so important for those IB/core modules. I liked SRCU because it doesn't have *_ONCE() macros and smb_* calls.
It does, they are just hidden under other macros..
Its better that they are hidden. So that we don't need open code them.

I wouldn't call swapping one function call for another 'open coding'

Quoted 2 lines

Also with srcu, we don't need lock annotations in get_cb_table() which releases and acquires semaphore.

You don't need lock annoations for that.

Quoted 1 line

Additionally lock nesting makes overall more complex.

SRCU nesting is just as complicated! Don't think SRCU magically hides that issue, it is still proposing to nest SRCU read side sections.

Quoted 3 lines

Given that there are only 3 indices, out of which only 2 are outside of the ib_core module and unlikely to be unloaded, I also prefer srcu version.

Why? It isn't faster, it uses more memory, it still has the same complex concurrency arrangement..

Jason

Leon Romanovsky

author

2019-10-24 19:19 UTC #9 lore raw

Quoted 63 lines · Jason Gunthorpe

Quoted 42 lines · Parav Pandit
Quoted 42 lines
From: Leon Romanovsky <[email protected]>
Sent: Thursday, October 24, 2019 11:13 AM
To: Jason Gunthorpe <[email protected]>
Cc: Doug Ledford <[email protected]>; Parav Pandit <[email protected]>; RDMA mailing list <[email protected]>
Subject: Re: [PATCH rdma-next] IB/core: Avoid deadlock during netlink message handling
Quoted 14 lines · Jason Gunthorpe
Quoted 13 lines · Leon Romanovsky
Quoted 12 lines · Jason Gunthorpe
Quoted 11 lines · Leon Romanovsky
Quoted 10 lines · Jason Gunthorpe
Quoted 8 lines · Leon Romanovsky
drivers/infiniband/core/netlink.c +1 -0
index 81dbd5f41bed..a3507b8be569 100644
+++ b/drivers/infiniband/core/netlink.c
@@ -42,9 +42,12 @@
 #include <linux/module.h>
 #include "core_priv.h"
 
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/infiniband/core/netlink.c line 46
Quoted 4 lines
drivers/infiniband/core/netlink.c +2 -2
-static DEFINE_MUTEX(rdma_nl_mutex);  static struct {
-	const struct rdma_nl_cbs   *cb_table;
+	const struct rdma_nl_cbs __rcu *cb_table;
+	/* Synchronizes between ongoing netlink commands and
netlink client
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Quoted 9 lines
Quoted 7 lines
drivers/infiniband/core/netlink.c line 49
Quoted 3 lines
drivers/infiniband/core/netlink.c +3 -0
+	 * unregistration.
+	 */
+	struct srcu_struct unreg_srcu;
A srcu in every index is serious overkill for this. Lets just us a
rwsem:
I liked previous variant more than rwsem, but it is Parav's patch.
Why? srcu is a huge data structure and slow on unregister
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
The unregister time is not so important for those IB/core modules. I liked SRCU because it doesn't have *_ONCE() macros and smb_* calls.
It does, they are just hidden under other macros..
Its better that they are hidden. So that we don't need open code them.
I wouldn't call swapping one function call for another 'open coding'
Quoted 2 lines
Also with srcu, we don't need lock annotations in get_cb_table() which releases and acquires semaphore.
You don't need lock annoations for that.
Quoted 1 line
Additionally lock nesting makes overall more complex.
SRCU nesting is just as complicated! Don't think SRCU magically hides that issue, it is still proposing to nest SRCU read side sections.

Quoted 6 lines

Quoted 3 lines
Given that there are only 3 indices, out of which only 2 are outside of the ib_core module and unlikely to be unloaded, I also prefer srcu version.
Why? It isn't faster, it uses more memory, it still has the same complex concurrency arrangement..

Jason,

It doesn't worth arguing, both Parav and I prefer SRCU variant, you prefer rwsem, so go for it, take rwsem, it is not important.

Thanks

Quoted 2 lines

Jason

Parav Pandit

2019-10-24 19:54 UTC #10 lore raw

Quoted 84 lines

-----Original Message-----
From: Leon Romanovsky <[email protected]>
Sent: Thursday, October 24, 2019 2:20 PM
To: Jason Gunthorpe <[email protected]>
Cc: Parav Pandit <[email protected]>; Doug Ledford <[email protected]>; RDMA mailing list <[email protected]>
Subject: Re: [PATCH rdma-next] IB/core: Avoid deadlock during netlink message handling
Quoted 16 lines · Jason Gunthorpe
Quoted 15 lines · Parav Pandit
Quoted 15 lines
From: Leon Romanovsky <[email protected]>
Sent: Thursday, October 24, 2019 11:13 AM
To: Jason Gunthorpe <[email protected]>
Cc: Doug Ledford <[email protected]>; Parav Pandit <[email protected]>; RDMA mailing list <[email protected]>
Subject: Re: [PATCH rdma-next] IB/core: Avoid deadlock during netlink message handling
Quoted 3 lines · Jason Gunthorpe
Quoted 2 lines · Leon Romanovsky
Quoted 1 line · Jason Gunthorpe
On Thu, Oct 24, 2019 at 04:26:07PM +0300, Leon Romanovsky
Quoted 1 line · wrote:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
On Thu, Oct 24, 2019 at 10:17:43AM -0300, Jason Gunthorpe
Quoted 1 line · wrote:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
On Tue, Oct 15, 2019 at 11:07:33AM +0300, Leon Romanovsky
Quoted 8 lines · wrote:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/infiniband/core/netlink.c +1 -0
index 81dbd5f41bed..a3507b8be569 100644
+++ b/drivers/infiniband/core/netlink.c
@@ -42,9 +42,12 @@
 #include <linux/module.h>  #include "core_priv.h"
 
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/infiniband/core/netlink.c line 45
Quoted 4 lines
drivers/infiniband/core/netlink.c +2 -2
-static DEFINE_MUTEX(rdma_nl_mutex);  static struct {
-	const struct rdma_nl_cbs   *cb_table;
+	const struct rdma_nl_cbs __rcu *cb_table;
+	/* Synchronizes between ongoing netlink commands
and
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
netlink client
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
drivers/infiniband/core/netlink.c line 48
Quoted 3 lines
drivers/infiniband/core/netlink.c +3 -0
+	 * unregistration.
+	 */
+	struct srcu_struct unreg_srcu;
A srcu in every index is serious overkill for this. Lets just us a
rwsem:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
I liked previous variant more than rwsem, but it is Parav's patch.
Why? srcu is a huge data structure and slow on unregister
The unregister time is not so important for those IB/core modules. I liked SRCU because it doesn't have *_ONCE() macros and smb_*
calls.
Quoted 22 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
It does, they are just hidden under other macros..
Its better that they are hidden. So that we don't need open code them.
I wouldn't call swapping one function call for another 'open coding'
Quoted 2 lines
Also with srcu, we don't need lock annotations in get_cb_table() which releases and acquires semaphore.
You don't need lock annoations for that.
Quoted 1 line
Additionally lock nesting makes overall more complex.
SRCU nesting is just as complicated! Don't think SRCU magically hides that issue, it is still proposing to nest SRCU read side sections.
Quoted 3 lines
Given that there are only 3 indices, out of which only 2 are outside of the ib_core module and unlikely to be unloaded, I also prefer srcu version.

Quoted 8 lines

Quoted 2 lines
Why? It isn't faster, it uses more memory, it still has the same complex concurrency arrangement..
Jason,
It doesn't worth arguing, both Parav and I prefer SRCU variant, you prefer rwsem, so go for it, take rwsem, it is not important.

Jason's memory size point made be curious about the srcu_struct size.
On my x86 5.x kernel I see srcu_struct costs 70+Kbytes! Likely due to some debug info in my kernel. Which is probably a good reason in this case to shift to rwsem. (rwsem is 80 bytes).

One small comment correction needed is,

- rdma_nl_types[index].cb_table = cb_table;
- mutex_unlock(&rdma_nl_mutex);
+ /* Pairs with the READ_ONCE in is_nl_valid() */
+ smp_store_release(&rdma_nl_types[index].cb_table, cb_table);

It should be "Pairs with the READ_ONE in get_cb_table() */

Quoted 4 lines

Thanks
Quoted 2 lines
Jason

Jason Gunthorpe

2019-10-24 23:53 UTC #11 lore raw

Quoted 4 lines · Parav Pandit

Jason's memory size point made be curious about the srcu_struct size. On my x86 5.x kernel I see srcu_struct costs 70+Kbytes! Likely due to some debug info in my kernel. Which is probably a good reason in this case to shift to rwsem. (rwsem is 80 bytes).

Yikes, I knew it was bad, but I didn't think that bad.. So unbelievable I checked here and I got +4752 bytes using SRCU on my much less debug kernel. Crazy.

Quoted 8 lines

One small comment correction needed is,
(quoted diff) +2 -2
-	rdma_nl_types[index].cb_table = cb_table;
-	mutex_unlock(&rdma_nl_mutex);
+	/* Pairs with the READ_ONCE in is_nl_valid() */
+	smp_store_release(&rdma_nl_types[index].cb_table, cb_table);
It should be "Pairs with the READ_ONE in get_cb_table() */

Done, applied to for-rc, thanks

Jason