| Message ID | 20190204211751.19001-7-jgg@ziepe.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rework device, client and client data locking | expand |
On Mon, Feb 04, 2019 at 02:17:49PM -0700, Jason Gunthorpe wrote: > + * 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. I've just been waiting for a customer to add an xa_for_each_rev() ;-) I'll put it on the todo list ... unless someone else wants to do it first.
On 04-Feb-19 23:17, Jason Gunthorpe wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > This gives each client a unique ID and will let us move client_data to use > xarray, and revise the locking scheme. > > clients have to be add/removed in strict FIFO/LIFO order as they > interdepend. To support this the client_ids are assigned to increase in > FIFO order. The existing linked list is kept to support reverse iteration > until xarray can get a reverse iteration API. > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > Reviewed-by: Parav Pandit <parav@mellanox.com> > --- > drivers/infiniband/core/device.c | 50 ++++++++++++++++++++++++++++---- > include/rdma/ib_verbs.h | 3 +- > 2 files changed, 47 insertions(+), 6 deletions(-) > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > index 300a36beaa300c..f485e77223d429 100644 > --- a/drivers/infiniband/core/device.c > +++ b/drivers/infiniband/core/device.c > @@ -65,15 +65,17 @@ struct workqueue_struct *ib_comp_unbound_wq; > struct workqueue_struct *ib_wq; > EXPORT_SYMBOL_GPL(ib_wq); > > -/* The device_list and client_list contain devices and clients after their > +/* The device_list and clients contain devices and clients after their > * registration has completed, and the devices and clients are removed > * during unregistration. */ > static LIST_HEAD(device_list); > static LIST_HEAD(client_list); > +#define CLIENT_REGISTERED XA_MARK_1 > +static DEFINE_XARRAY_FLAGS(clients, XA_FLAGS_ALLOC); > > /* > * device_mutex and lists_rwsem protect access to both device_list and > - * client_list. device_mutex protects writer access by device and client > + * clients. device_mutex protects writer access by device and client > * registration / de-registration. lists_rwsem protects reader access to > * these lists. Iterators of these lists must lock it for read, while updates > * to the lists must be done with a write lock. A special case is when the > @@ -563,6 +565,7 @@ int ib_register_device(struct ib_device *device, const char *name) > { > int ret; > struct ib_client *client; > + unsigned long index; > > setup_dma_device(device); > > @@ -607,7 +610,7 @@ int ib_register_device(struct ib_device *device, const char *name) > > refcount_set(&device->refcount, 1); > > - list_for_each_entry(client, &client_list, list) > + xa_for_each_marked (&clients, index, client, CLIENT_REGISTERED) nit: extra space after xa_for_each_marked.
On Tue, Feb 05, 2019 at 04:19:14PM +0200, Gal Pressman wrote: > On 04-Feb-19 23:17, Jason Gunthorpe wrote: > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > This gives each client a unique ID and will let us move client_data to use > > xarray, and revise the locking scheme. > > > > clients have to be add/removed in strict FIFO/LIFO order as they > > interdepend. To support this the client_ids are assigned to increase in > > FIFO order. The existing linked list is kept to support reverse iteration > > until xarray can get a reverse iteration API. > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > Reviewed-by: Parav Pandit <parav@mellanox.com> > > drivers/infiniband/core/device.c | 50 ++++++++++++++++++++++++++++---- > > include/rdma/ib_verbs.h | 3 +- > > 2 files changed, 47 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > > index 300a36beaa300c..f485e77223d429 100644 > > +++ b/drivers/infiniband/core/device.c > > @@ -65,15 +65,17 @@ struct workqueue_struct *ib_comp_unbound_wq; > > struct workqueue_struct *ib_wq; > > EXPORT_SYMBOL_GPL(ib_wq); > > > > -/* The device_list and client_list contain devices and clients after their > > +/* The device_list and clients contain devices and clients after their > > * registration has completed, and the devices and clients are removed > > * during unregistration. */ > > static LIST_HEAD(device_list); > > static LIST_HEAD(client_list); > > +#define CLIENT_REGISTERED XA_MARK_1 > > +static DEFINE_XARRAY_FLAGS(clients, XA_FLAGS_ALLOC); > > > > /* > > * device_mutex and lists_rwsem protect access to both device_list and > > - * client_list. device_mutex protects writer access by device and client > > + * clients. device_mutex protects writer access by device and client > > * registration / de-registration. lists_rwsem protects reader access to > > * these lists. Iterators of these lists must lock it for read, while updates > > * to the lists must be done with a write lock. A special case is when the > > @@ -563,6 +565,7 @@ int ib_register_device(struct ib_device *device, const char *name) > > { > > int ret; > > struct ib_client *client; > > + unsigned long index; > > > > setup_dma_device(device); > > > > @@ -607,7 +610,7 @@ int ib_register_device(struct ib_device *device, const char *name) > > > > refcount_set(&device->refcount, 1); > > > > - list_for_each_entry(client, &client_list, list) > > + xa_for_each_marked (&clients, index, client, CLIENT_REGISTERED) > > nit: extra space after xa_for_each_marked. meh. Some places in the kernel put the space, maby don't. In this case clang-format wants to put the space for all 'for' macros (and who can blame, it for should aways have a space). I hate to fight with clang-format over such trivial things. Especially when there isn't a specific requirement in coding style. Jason
On Tue, Feb 05, 2019 at 09:07:45AM -0700, Jason Gunthorpe wrote: > On Tue, Feb 05, 2019 at 04:19:14PM +0200, Gal Pressman wrote: > > > - list_for_each_entry(client, &client_list, list) > > > + xa_for_each_marked (&clients, index, client, CLIENT_REGISTERED) > > > > nit: extra space after xa_for_each_marked. > > meh. Some places in the kernel put the space, maby don't. In this case > clang-format wants to put the space for all 'for' macros (and who can > blame, it for should aways have a space). I hate to fight with > clang-format over such trivial things. Especially when there isn't a > specific requirement in coding style. Aha. Apparently I should add this: +++ b/.clang-format @@ -371,6 +371,12 @@ ForEachMacros: - 'v4l2_m2m_for_each_dst_buf_safe' - 'v4l2_m2m_for_each_src_buf' - 'v4l2_m2m_for_each_src_buf_safe' + - 'xa_for_each' + - 'xa_for_each_marked' + - 'xa_for_each_start' + - 'xas_for_each' + - 'xas_for_each_conflict' + - 'xas_for_each_marked' - 'zorro_for_each_dev' #IncludeBlocks: Preserve # Unknown to clang-format-5.0 As a non-clang user, I didn't ever bother with this file.
On Tue, Feb 05, 2019 at 08:46:56AM -0800, Matthew Wilcox wrote: > On Tue, Feb 05, 2019 at 09:07:45AM -0700, Jason Gunthorpe wrote: > > On Tue, Feb 05, 2019 at 04:19:14PM +0200, Gal Pressman wrote: > > > > - list_for_each_entry(client, &client_list, list) > > > > + xa_for_each_marked (&clients, index, client, CLIENT_REGISTERED) > > > > > > nit: extra space after xa_for_each_marked. > > > > meh. Some places in the kernel put the space, maby don't. In this case > > clang-format wants to put the space for all 'for' macros (and who can > > blame, it for should aways have a space). I hate to fight with > > clang-format over such trivial things. Especially when there isn't a > > specific requirement in coding style. > > Aha. Apparently I should add this: > > +++ b/.clang-format > @@ -371,6 +371,12 @@ ForEachMacros: > - 'v4l2_m2m_for_each_dst_buf_safe' > - 'v4l2_m2m_for_each_src_buf' > - 'v4l2_m2m_for_each_src_buf_safe' > + - 'xa_for_each' > + - 'xa_for_each_marked' > + - 'xa_for_each_start' > + - 'xas_for_each' > + - 'xas_for_each_conflict' > + - 'xas_for_each_marked' > - 'zorro_for_each_dev' > > #IncludeBlocks: Preserve # Unknown to clang-format-5.0 > > As a non-clang user, I didn't ever bother with this file. I already have those fields. 411 - 'xa_for_each' 412 - 'xas_for_each' 413 - 'xas_for_each_conflict' 414 - 'xas_for_each_marked' Updated by Jason 99e309b6ed75 ("clang-format: Update .clang-format with the latest for_each macro list") Thanks >
On Tue, Feb 05, 2019 at 08:46:56AM -0800, Matthew Wilcox wrote: > On Tue, Feb 05, 2019 at 09:07:45AM -0700, Jason Gunthorpe wrote: > > On Tue, Feb 05, 2019 at 04:19:14PM +0200, Gal Pressman wrote: > > > > - list_for_each_entry(client, &client_list, list) > > > > + xa_for_each_marked (&clients, index, client, CLIENT_REGISTERED) > > > > > > nit: extra space after xa_for_each_marked. > > > > meh. Some places in the kernel put the space, maby don't. In this case > > clang-format wants to put the space for all 'for' macros (and who can > > blame, it for should aways have a space). I hate to fight with > > clang-format over such trivial things. Especially when there isn't a > > specific requirement in coding style. > > Aha. Apparently I should add this: > > +++ b/.clang-format > @@ -371,6 +371,12 @@ ForEachMacros: > - 'v4l2_m2m_for_each_dst_buf_safe' > - 'v4l2_m2m_for_each_src_buf' > - 'v4l2_m2m_for_each_src_buf_safe' > + - 'xa_for_each' > + - 'xa_for_each_marked' > + - 'xa_for_each_start' > + - 'xas_for_each' > + - 'xas_for_each_conflict' > + - 'xas_for_each_marked' > - 'zorro_for_each_dev' That won't fix the ' ' - this actually causes the space, since clang says a for macro and a for statement should have the same layout. clang-format makes a mess of for macros if they are not in this list as it treats the expression like a function call, so the '{' placement is wrong for the for-loop case.. It would be friendly to keep this list updated, I already sent a patch that is in -rc5, but it doesn't have the latest xa changes. > As a non-clang user, I didn't ever bother with this file. Using clang-format via the editor as an 'interactive' single expression indentor is a big, big time saver, IMHO. Mainly because it does a quite good job, ie run in whole file mode: clang-format -i lib/xarray.c && git diff To see how it disagrees with the style you used in this file.. I'd say the minor changes around the start of wrapped lines are more in line with the general kernel consensus, otherwise it already has a very close agreement to your own style. Jason
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 300a36beaa300c..f485e77223d429 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -65,15 +65,17 @@ struct workqueue_struct *ib_comp_unbound_wq; struct workqueue_struct *ib_wq; EXPORT_SYMBOL_GPL(ib_wq); -/* The device_list and client_list contain devices and clients after their +/* The device_list and clients contain devices and clients after their * registration has completed, and the devices and clients are removed * during unregistration. */ static LIST_HEAD(device_list); static LIST_HEAD(client_list); +#define CLIENT_REGISTERED XA_MARK_1 +static DEFINE_XARRAY_FLAGS(clients, XA_FLAGS_ALLOC); /* * device_mutex and lists_rwsem protect access to both device_list and - * client_list. device_mutex protects writer access by device and client + * clients. device_mutex protects writer access by device and client * registration / de-registration. lists_rwsem protects reader access to * these lists. Iterators of these lists must lock it for read, while updates * to the lists must be done with a write lock. A special case is when the @@ -563,6 +565,7 @@ int ib_register_device(struct ib_device *device, const char *name) { int ret; struct ib_client *client; + unsigned long index; setup_dma_device(device); @@ -607,7 +610,7 @@ int ib_register_device(struct ib_device *device, const char *name) refcount_set(&device->refcount, 1); - list_for_each_entry(client, &client_list, list) + xa_for_each_marked (&clients, index, client, CLIENT_REGISTERED) if (!add_client_context(device, client) && client->add) client->add(device); @@ -679,6 +682,32 @@ void ib_unregister_device(struct ib_device *device) } EXPORT_SYMBOL(ib_unregister_device); +static int assign_client_id(struct ib_client *client) +{ + int ret; + + /* + * 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. + */ + if (list_empty(&client_list)) + client->client_id = 0; + else + client->client_id = + list_last_entry(&client_list, struct ib_client, list) + ->client_id; + ret = xa_alloc(&clients, &client->client_id, INT_MAX, client, + GFP_KERNEL); + if (ret) + goto out; + +out: + return ret; +} + /** * ib_register_client - Register an IB client * @client:Client to register @@ -695,15 +724,21 @@ EXPORT_SYMBOL(ib_unregister_device); int ib_register_client(struct ib_client *client) { struct ib_device *device; + int ret; mutex_lock(&device_mutex); + ret = assign_client_id(client); + if (ret) { + mutex_unlock(&device_mutex); + return ret; + } list_for_each_entry(device, &device_list, core_list) if (!add_client_context(device, client) && client->add) client->add(device); down_write(&lists_rwsem); - list_add_tail(&client->list, &client_list); + xa_set_mark(&clients, client->client_id, CLIENT_REGISTERED); up_write(&lists_rwsem); mutex_unlock(&device_mutex); @@ -728,7 +763,7 @@ void ib_unregister_client(struct ib_client *client) mutex_lock(&device_mutex); down_write(&lists_rwsem); - list_del(&client->list); + xa_clear_mark(&clients, client->client_id, CLIENT_REGISTERED); up_write(&lists_rwsem); list_for_each_entry(device, &device_list, core_list) { @@ -764,6 +799,10 @@ void ib_unregister_client(struct ib_client *client) kfree(found_context); } + down_write(&lists_rwsem); + list_del(&client->list); + xa_erase(&clients, client->client_id); + up_write(&lists_rwsem); mutex_unlock(&device_mutex); } EXPORT_SYMBOL(ib_unregister_client); @@ -1417,6 +1456,7 @@ static void __exit ib_core_cleanup(void) destroy_workqueue(ib_comp_wq); /* Make sure that any pending umem accounting work is done. */ destroy_workqueue(ib_wq); + WARN_ON(!xa_empty(&clients)); } MODULE_ALIAS_RDMA_NETLINK(RDMA_NL_LS, 4); diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 76138ef4f839ea..84d7523984ee77 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -2596,7 +2596,7 @@ struct ib_device { }; struct ib_client { - char *name; + const char *name; void (*add) (struct ib_device *); void (*remove)(struct ib_device *, void *client_data); @@ -2623,6 +2623,7 @@ struct ib_client { const struct sockaddr *addr, void *client_data); struct list_head list; + u32 client_id; /* kverbs are not required by the client */ u8 no_kverbs_req:1;