RDMA fixes for 5.3
cover RDMA fixes for 5.3
From: Leon Romanovsky <[email protected]>
Hi,
Two lock fixes in device<->client interaction, please see extensive commit messages for more details.
Thanks
Jason Gunthorpe (2):
RDMA/devices: Do not deadlock during client removal RDMA/devices: Remove the lock around remove_client_context
drivers/infiniband/core/device.c | 102 ++++++++++++++++++++----------- include/rdma/ib_verbs.h | 4 +- 2 files changed, 71 insertions(+), 35 deletions(-)
--
2.20.1
PATCH 1/2 RDMA/devices: Do not deadlock during client removal
From: Jason Gunthorpe <[email protected]>
lockdep reports:
WARNING: possible circular locking dependency detected
modprobe/302 is trying to acquire lock:
0000000007c8919c ((wq_completion)ib_cm){+.+.}, at: flush_workqueue+0xdf/0x990
but task is already holding lock:
000000002d3d2ca9 (&device->client_data_rwsem){++++}, at: remove_client_context+0x79/0xd0 [ib_core]
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (&device->client_data_rwsem){++++}:
down_read+0x3f/0x160
ib_get_net_dev_by_params+0xd5/0x200 [ib_core]
cma_ib_req_handler+0x5f6/0x2090 [rdma_cm]
cm_process_work+0x29/0x110 [ib_cm]
cm_req_handler+0x10f5/0x1c00 [ib_cm]
cm_work_handler+0x54c/0x311d [ib_cm]
process_one_work+0x4aa/0xa30
worker_thread+0x62/0x5b0
kthread+0x1ca/0x1f0
ret_from_fork+0x24/0x30-> #1 ((work_completion)(&(&work->work)->work)){+.+.}:
process_one_work+0x45f/0xa30
worker_thread+0x62/0x5b0
kthread+0x1ca/0x1f0
ret_from_fork+0x24/0x30-> #0 ((wq_completion)ib_cm){+.+.}:
lock_acquire+0xc8/0x1d0
flush_workqueue+0x102/0x990
cm_remove_one+0x30e/0x3c0 [ib_cm]
remove_client_context+0x94/0xd0 [ib_core]
disable_device+0x10a/0x1f0 [ib_core]
__ib_unregister_device+0x5a/0xe0 [ib_core]
ib_unregister_device+0x21/0x30 [ib_core]
mlx5_ib_stage_ib_reg_cleanup+0x9/0x10 [mlx5_ib]
__mlx5_ib_remove+0x3d/0x70 [mlx5_ib]
mlx5_ib_remove+0x12e/0x140 [mlx5_ib]
mlx5_remove_device+0x144/0x150 [mlx5_core]
mlx5_unregister_interface+0x3f/0xf0 [mlx5_core]
mlx5_ib_cleanup+0x10/0x3a [mlx5_ib]
__x64_sys_delete_module+0x227/0x350
do_syscall_64+0xc3/0x6a4
entry_SYSCALL_64_after_hwframe+0x49/0xbeWhich is due to the read side of the client_data_rwsem being obtained recursively through a work queue flush during cm client removal.
The lock is being held across the remove in remove_client_context() so that the function is a fence, once it returns the client is removed. This is required so that the two callers do not proceed with destruction until the client completes removal.
Instead of using client_data_rwsem use the existing device unregistration refcount and add a similar client unregistration (client->uses) refcount.
This will fence the two unregistration paths without holding any locks.
--- drivers/infiniband/core/device.c | 54 ++++++++++++++++++++++++-------- include/rdma/ib_verbs.h | 3 ++ 2 files changed, 44 insertions(+), 13 deletions(-)
drivers/infiniband/core/device.c +41 -13
@@ -99,6 +99,12 @@ static LIST_HEAD(client_list); static DEFINE_XARRAY_FLAGS(clients, XA_FLAGS_ALLOC);static DECLARE_RWSEM (clients_rwsem); +static void ib_client_put (struct ib_client *client) +{ +if (refcount_dec_and_test (&client->uses)) +complete (&client->uses_zero); +} +/* * If client_data is registered then the corresponding client must also still * be registered.
@@ -660,6 +666,14 @@ static int add_client_context(struct ib_device *device,return 0 ;down_write (&device->client_data_rwsem); +/* +* So long as the client is registered hold both the client and device +* unregistration locks. +*/ +if (!refcount_inc_not_zero (&client->uses)) + goto out_unlock; +refcount_inc (&device->refcount); +/* * Another caller to add_client_context got here first and has already * completely initialized context.
@@ -683,6 +697,9 @@ static int add_client_context(struct ib_device *device,return 0 ; out: +ib_device_put (device); +ib_client_put (client); +out_unlock:up_write (&device->client_data_rwsem);return ret; }
@@ -702,7 +719,7 @@ static void remove_client_context(struct ib_device *device, client_data =xa_load (&device->client_data, client_id);xa_clear_mark (&device->client_data, client_id,CLIENT_DATA_REGISTERED ); client =xa_load (&clients, client_id); -downgrade_write (&device->client_data_rwsem); +up_write (&device->client_data_rwsem);/* * Notice we cannot be holding any exclusive locks when calling the
@@ -712,17 +729,13 @@ static void remove_client_context(struct ib_device *device,* * For this reason clients and drivers should not call the * unregistration functions will holdling any locks. -* -* It tempting to drop the client_data_rwsem too, but this is required -* to ensure that unregister_client does not return until all clients -* are completely unregistered, which is required to avoid module -* unloading races. */ if (client->remove) client->remove (device, client_data);xa_erase (&device->client_data, client_id); -up_read (&device->client_data_rwsem); +ib_device_put (device); +ib_client_put (client); }static int alloc_port_data (struct ib_device *device)
@@ -1705,6 +1718,8 @@ int ib_register_client(struct ib_client *client)unsigned long index;int ret; +refcount_set (&client->uses,1 ); +init_completion (&client->uses_zero); ret =assign_client_id (client);if (ret)return ret;
@@ -1740,16 +1755,29 @@ void ib_unregister_client(struct ib_client *client)unsigned long index;down_write (&clients_rwsem); +ib_client_put (client);xa_clear_mark (&clients, client->client_id,CLIENT_REGISTERED );up_write (&clients_rwsem); + +/* We do not want to have locks while calling client->remove() */ +rcu_read_lock (); + xa_for_each (&devices, index, device) { +if (!ib_device_try_get (device)) +continue ; +rcu_read_unlock (); + +remove_client_context (device, client->client_id); + +ib_device_put (device); +rcu_read_lock (); + } +rcu_read_unlock (); +/* -* Every device still known must be serialized to make sure we are -* done with the client callbacks before we return. +* remove_client_context() is not a fence, it can return even though a +* removal is ongoing. Wait until all removals are completed. */ -down_read (&devices_rwsem); - xa_for_each (&devices, index, device) -remove_client_context (device, client->client_id); -up_read (&devices_rwsem); +wait_for_completion (&client->uses_zero);down_write (&clients_rwsem);list_del (&client->list);
include/rdma/ib_verbs.h +3 -0
@@ -2647,6 +2647,9 @@ struct ib_client {const union ib_gid *gid, const struct sockaddr *addr, void *client_data); + +refcount_t uses; +struct completion uses_zero;struct list_head list;u32 client_id;
Signature · 1 line
2.20.1
PATCH 2/2 RDMA/devices: Remove the lock around remove_client_context
In reply to Leon RomanovskyFrom: Jason Gunthorpe <[email protected]>
Due to the complexity of client->remove() callbacks it is desirable to not hold any locks while calling them. Remove the last one by tracking only the highest client ID and running backwards from there over the xarray.
Since the only purpose of that lock was to protect the linked list, we can drop the lock.
--- drivers/infiniband/core/device.c | 48 ++++++++++++++++++-------------- include/rdma/ib_verbs.h | 1 - 2 files changed, 27 insertions(+), 22 deletions(-)
drivers/infiniband/core/device.c +27 -21
@@ -94,7 +94,7 @@ static DEFINE_XARRAY_FLAGS(devices, XA_FLAGS_ALLOC);static DECLARE_RWSEM (devices_rwsem);#define DEVICE_REGISTERED XA_MARK_1 -static LIST_HEAD (client_list); +static u32 highest_client_id;#define CLIENT_REGISTERED XA_MARK_1 static DEFINE_XARRAY_FLAGS(clients, XA_FLAGS_ALLOC);static DECLARE_RWSEM (clients_rwsem);
@@ -1237,7 +1237,7 @@ static int setup_device(struct ib_device *device)static void disable_device (struct ib_device *device) { -struct ib_client *client; +u32 cid;WARN_ON (!refcount_read (&device->refcount));
@@ -1245,10 +1245,19 @@ static void disable_device(struct ib_device *device)xa_clear_mark (&devices, device->index,DEVICE_REGISTERED );up_write (&devices_rwsem); +/* +* Remove clients in LIFO order, see assign_client_id. This could be +* more efficient if xarray learns to reverse iterate. Since no new +* clients can be added to this ib_device past this point we only need +* the maximum possible client_id value here. +*/ down_read (&clients_rwsem); -list_for_each_entry_reverse (client, &client_list, list) -remove_client_context (device, client->client_id); + cid = highest_client_id;up_read (&clients_rwsem); +while (cid) { + cid--; +remove_client_context (device, cid); + }/* Pairs with refcount_set in enable_device */ ib_device_put (device);
@@ -1675,30 +1684,31 @@ static int assign_client_id(struct ib_client *client)/* * The add/remove callbacks must be called in FIFO/LIFO order. To * achieve this we assign client_ids so they are sorted in -* registration order, and retain a linked list we can reverse iterate -* to get the LIFO order. The extra linked list can go away if xarray -* learns to reverse iterate. +* registration order. */ -if (list_empty (&client_list)) { - client->client_id =0 ; - }else { -struct ib_client *last; - - last = list_last_entry(&client_list, struct ib_client, list); - client->client_id = last->client_id +1 ; - } + client->client_id = highest_client_id; ret =xa_insert (&clients, client->client_id, client,GFP_KERNEL );if (ret) goto out; + highest_client_id++;xa_set_mark (&clients, client->client_id,CLIENT_REGISTERED ); -list_add_tail (&client->list, &client_list); out:up_write (&clients_rwsem);return ret; } +static void remove_client_id (struct ib_client *client) +{ +down_write (&clients_rwsem); +xa_erase (&clients, client->client_id); +for (; highest_client_id; highest_client_id--) +if (xa_load (&clients, highest_client_id -1 )) +break ; +up_write (&clients_rwsem); +} +/** * ib_register_client - Register an IB client * @client:Client to register
@@ -1778,11 +1788,7 @@ void ib_unregister_client(struct ib_client *client)* removal is ongoing. Wait until all removals are completed. */ wait_for_completion (&client->uses_zero); - -down_write (&clients_rwsem); -list_del (&client->list); -xa_erase (&clients, client->client_id); -up_write (&clients_rwsem); +remove_client_id (client); }EXPORT_SYMBOL (ib_unregister_client);
include/rdma/ib_verbs.h +0 -1
@@ -2650,7 +2650,6 @@ struct ib_client {refcount_t uses;struct completion uses_zero; -struct list_head list;u32 client_id;/* kverbs are not required by the client */
Signature · 1 line
2.20.1
Quoted 4 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Hi,
Quoted 8 lines
Two lock fixes in device<->client interaction, please see extensive commit messages for more details.
Thanks
Jason Gunthorpe (2):
RDMA/devices: Do not deadlock during client removal RDMA/devices: Remove the lock aroundremove_client_context
I tried to figure out some way to trip either of these up, but they both look good to me. I'll let them sit on the list for feedback before I take them though.
Signature · 3 lines
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Doug Ledford
AppliedQuoted 10 lines · Doug Ledford
Quoted 9 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Hi,
Two lock fixes in device<->client interaction, please see extensive commit messages for more details.
Thanks
Quoted 9 lines
Quoted 3 lines
Jason Gunthorpe (2):
RDMA/devices: Do not deadlock during client removal RDMA/devices: Remove the lock aroundremove_client_contextI tried to figure out some way to trip either of these up, but they both
look good to me. I'll let them sit on the list for feedback before I take them though.
Applied to for-rc, thanks.
Signature · 3 lines
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD