| Message ID | 1546766583-27979-2-git-send-email-galpress@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IB device in-kernel API support indication | expand |
On Sun, Jan 06, 2019 at 11:23:02AM +0200, Gal Pressman wrote: > Drivers that do not provide kernel verbs support should not be used by > ib kernel clients and fail. > In case a device does not implement all mandatory verbs for kverbs usage > mark it as a non kverbs provider and prevent its usage for all clients > except for uverbs. > > The device is marked as a non kverbs provider using the > 'kverbs_provider' flag which should only be set by the core code. > The clients can choose whether kverbs are requested for it usage using > the 'no_kverbs_req' flag which is currently set for uverbs only. > > This patch allows drivers to remove mandatory verbs stubs and simply set > the callback to NULL. The IB device will be registered as a non-kverbs > provider. > > Signed-off-by: Gal Pressman <galpress@amazon.com> > --- > drivers/infiniband/core/device.c | 12 ++++++++---- > drivers/infiniband/core/uverbs_main.c | 1 + > include/rdma/ib_verbs.h | 5 +++++ > 3 files changed, 14 insertions(+), 4 deletions(-) > Thanks, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On Sun, Jan 06, 2019 at 11:23:02AM +0200, Gal Pressman wrote: > Drivers that do not provide kernel verbs support should not be used by > ib kernel clients and fail. > In case a device does not implement all mandatory verbs for kverbs usage > mark it as a non kverbs provider and prevent its usage for all clients > except for uverbs. > > The device is marked as a non kverbs provider using the > 'kverbs_provider' flag which should only be set by the core code. > The clients can choose whether kverbs are requested for it usage using > the 'no_kverbs_req' flag which is currently set for uverbs only. > > This patch allows drivers to remove mandatory verbs stubs and simply set > the callback to NULL. The IB device will be registered as a non-kverbs > provider. > > Signed-off-by: Gal Pressman <galpress@amazon.com> > drivers/infiniband/core/device.c | 12 ++++++++---- > drivers/infiniband/core/uverbs_main.c | 1 + > include/rdma/ib_verbs.h | 5 +++++ > 3 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > index 47ab34ee1a9d..6c1f71007ec4 100644 > +++ b/drivers/infiniband/core/device.c > @@ -121,13 +121,15 @@ static int ib_device_check_mandatory(struct ib_device *device) > }; > int i; > > + device->kverbs_provider = true; > for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) { > if (!*(void **) ((void *) &device->ops + > mandatory_table[i].offset)) { > dev_warn(&device->dev, > - "Device is missing mandatory function %s\n", > + "Device is missing mandatory function %s, disabling kverbs support\n", > mandatory_table[i].name); Why not get rid of the message? > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index a3ceed3a040a..6ac5b7c3dddc 100644 > +++ b/include/rdma/ib_verbs.h > @@ -2561,6 +2561,8 @@ struct ib_device { > __be64 node_guid; > u32 local_dma_lkey; > u16 is_switch:1; > + /* Indicates kernel verbs support, should not be used in drivers */ > + u8 kverbs_provider:1; > u8 node_type; > u8 phys_port_cnt; > struct ib_device_attr attrs; > @@ -2615,6 +2617,9 @@ struct ib_client { > const struct sockaddr *addr, > void *client_data); > struct list_head list; > + > + /* kverbs are not required by the device */ 'by the client' Jason
On 06-Jan-19 23:39, Jason Gunthorpe wrote: > On Sun, Jan 06, 2019 at 11:23:02AM +0200, Gal Pressman wrote: >> Drivers that do not provide kernel verbs support should not be used by >> ib kernel clients and fail. >> In case a device does not implement all mandatory verbs for kverbs usage >> mark it as a non kverbs provider and prevent its usage for all clients >> except for uverbs. >> >> The device is marked as a non kverbs provider using the >> 'kverbs_provider' flag which should only be set by the core code. >> The clients can choose whether kverbs are requested for it usage using >> the 'no_kverbs_req' flag which is currently set for uverbs only. >> >> This patch allows drivers to remove mandatory verbs stubs and simply set >> the callback to NULL. The IB device will be registered as a non-kverbs >> provider. >> >> Signed-off-by: Gal Pressman <galpress@amazon.com> >> drivers/infiniband/core/device.c | 12 ++++++++---- >> drivers/infiniband/core/uverbs_main.c | 1 + >> include/rdma/ib_verbs.h | 5 +++++ >> 3 files changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c >> index 47ab34ee1a9d..6c1f71007ec4 100644 >> +++ b/drivers/infiniband/core/device.c >> @@ -121,13 +121,15 @@ static int ib_device_check_mandatory(struct ib_device *device) >> }; >> int i; >> >> + device->kverbs_provider = true; >> for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) { >> if (!*(void **) ((void *) &device->ops + >> mandatory_table[i].offset)) { >> dev_warn(&device->dev, >> - "Device is missing mandatory function %s\n", >> + "Device is missing mandatory function %s, disabling kverbs support\n", >> mandatory_table[i].name); > > Why not get rid of the message? Will do. > >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h >> index a3ceed3a040a..6ac5b7c3dddc 100644 >> +++ b/include/rdma/ib_verbs.h >> @@ -2561,6 +2561,8 @@ struct ib_device { >> __be64 node_guid; >> u32 local_dma_lkey; >> u16 is_switch:1; >> + /* Indicates kernel verbs support, should not be used in drivers */ >> + u8 kverbs_provider:1; >> u8 node_type; >> u8 phys_port_cnt; >> struct ib_device_attr attrs; >> @@ -2615,6 +2617,9 @@ struct ib_client { >> const struct sockaddr *addr, >> void *client_data); >> struct list_head list; >> + >> + /* kverbs are not required by the device */ > > 'by the client' ACK.
On Mon, Jan 07, 2019 at 10:08:11AM +0200, Gal Pressman wrote: > On 06-Jan-19 23:39, Jason Gunthorpe wrote: > > On Sun, Jan 06, 2019 at 11:23:02AM +0200, Gal Pressman wrote: > >> Drivers that do not provide kernel verbs support should not be used by > >> ib kernel clients and fail. > >> In case a device does not implement all mandatory verbs for kverbs usage > >> mark it as a non kverbs provider and prevent its usage for all clients > >> except for uverbs. > >> > >> The device is marked as a non kverbs provider using the > >> 'kverbs_provider' flag which should only be set by the core code. > >> The clients can choose whether kverbs are requested for it usage using > >> the 'no_kverbs_req' flag which is currently set for uverbs only. > >> > >> This patch allows drivers to remove mandatory verbs stubs and simply set > >> the callback to NULL. The IB device will be registered as a non-kverbs > >> provider. > >> > >> Signed-off-by: Gal Pressman <galpress@amazon.com> > >> drivers/infiniband/core/device.c | 12 ++++++++---- > >> drivers/infiniband/core/uverbs_main.c | 1 + > >> include/rdma/ib_verbs.h | 5 +++++ > >> 3 files changed, 14 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > >> index 47ab34ee1a9d..6c1f71007ec4 100644 > >> +++ b/drivers/infiniband/core/device.c > >> @@ -121,13 +121,15 @@ static int ib_device_check_mandatory(struct ib_device *device) > >> }; > >> int i; > >> > >> + device->kverbs_provider = true; > >> for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) { > >> if (!*(void **) ((void *) &device->ops + > >> mandatory_table[i].offset)) { > >> dev_warn(&device->dev, > >> - "Device is missing mandatory function %s\n", > >> + "Device is missing mandatory function %s, disabling kverbs support\n", > >> mandatory_table[i].name); > > > > Why not get rid of the message? > > Will do. You need to be nice to users, and I see value in this warning. > > > > >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > >> index a3ceed3a040a..6ac5b7c3dddc 100644 > >> +++ b/include/rdma/ib_verbs.h > >> @@ -2561,6 +2561,8 @@ struct ib_device { > >> __be64 node_guid; > >> u32 local_dma_lkey; > >> u16 is_switch:1; > >> + /* Indicates kernel verbs support, should not be used in drivers */ > >> + u8 kverbs_provider:1; > >> u8 node_type; > >> u8 phys_port_cnt; > >> struct ib_device_attr attrs; > >> @@ -2615,6 +2617,9 @@ struct ib_client { > >> const struct sockaddr *addr, > >> void *client_data); > >> struct list_head list; > >> + > >> + /* kverbs are not required by the device */ > > > > 'by the client' > > ACK.
On Mon, Jan 07, 2019 at 10:15:20AM +0200, Leon Romanovsky wrote: > On Mon, Jan 07, 2019 at 10:08:11AM +0200, Gal Pressman wrote: > > On 06-Jan-19 23:39, Jason Gunthorpe wrote: > > > On Sun, Jan 06, 2019 at 11:23:02AM +0200, Gal Pressman wrote: > > >> Drivers that do not provide kernel verbs support should not be used by > > >> ib kernel clients and fail. > > >> In case a device does not implement all mandatory verbs for kverbs usage > > >> mark it as a non kverbs provider and prevent its usage for all clients > > >> except for uverbs. > > >> > > >> The device is marked as a non kverbs provider using the > > >> 'kverbs_provider' flag which should only be set by the core code. > > >> The clients can choose whether kverbs are requested for it usage using > > >> the 'no_kverbs_req' flag which is currently set for uverbs only. > > >> > > >> This patch allows drivers to remove mandatory verbs stubs and simply set > > >> the callback to NULL. The IB device will be registered as a non-kverbs > > >> provider. > > >> > > >> Signed-off-by: Gal Pressman <galpress@amazon.com> > > >> drivers/infiniband/core/device.c | 12 ++++++++---- > > >> drivers/infiniband/core/uverbs_main.c | 1 + > > >> include/rdma/ib_verbs.h | 5 +++++ > > >> 3 files changed, 14 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > > >> index 47ab34ee1a9d..6c1f71007ec4 100644 > > >> +++ b/drivers/infiniband/core/device.c > > >> @@ -121,13 +121,15 @@ static int ib_device_check_mandatory(struct ib_device *device) > > >> }; > > >> int i; > > >> > > >> + device->kverbs_provider = true; > > >> for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) { > > >> if (!*(void **) ((void *) &device->ops + > > >> mandatory_table[i].offset)) { > > >> dev_warn(&device->dev, > > >> - "Device is missing mandatory function %s\n", > > >> + "Device is missing mandatory function %s, disabling kverbs support\n", > > >> mandatory_table[i].name); > > > > > > Why not get rid of the message? > > > > Will do. > > You need to be nice to users, and I see value in this warning. If we are supporting this we should not have dev_warns for a normal occurance. Something like RDMA tool should report no-kverbs or somesuch instead Jason
On Mon, Jan 07, 2019 at 01:06:58PM -0700, Jason Gunthorpe wrote: > On Mon, Jan 07, 2019 at 10:15:20AM +0200, Leon Romanovsky wrote: > > On Mon, Jan 07, 2019 at 10:08:11AM +0200, Gal Pressman wrote: > > > On 06-Jan-19 23:39, Jason Gunthorpe wrote: > > > > On Sun, Jan 06, 2019 at 11:23:02AM +0200, Gal Pressman wrote: > > > >> Drivers that do not provide kernel verbs support should not be used by > > > >> ib kernel clients and fail. > > > >> In case a device does not implement all mandatory verbs for kverbs usage > > > >> mark it as a non kverbs provider and prevent its usage for all clients > > > >> except for uverbs. > > > >> > > > >> The device is marked as a non kverbs provider using the > > > >> 'kverbs_provider' flag which should only be set by the core code. > > > >> The clients can choose whether kverbs are requested for it usage using > > > >> the 'no_kverbs_req' flag which is currently set for uverbs only. > > > >> > > > >> This patch allows drivers to remove mandatory verbs stubs and simply set > > > >> the callback to NULL. The IB device will be registered as a non-kverbs > > > >> provider. > > > >> > > > >> Signed-off-by: Gal Pressman <galpress@amazon.com> > > > >> drivers/infiniband/core/device.c | 12 ++++++++---- > > > >> drivers/infiniband/core/uverbs_main.c | 1 + > > > >> include/rdma/ib_verbs.h | 5 +++++ > > > >> 3 files changed, 14 insertions(+), 4 deletions(-) > > > >> > > > >> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > > > >> index 47ab34ee1a9d..6c1f71007ec4 100644 > > > >> +++ b/drivers/infiniband/core/device.c > > > >> @@ -121,13 +121,15 @@ static int ib_device_check_mandatory(struct ib_device *device) > > > >> }; > > > >> int i; > > > >> > > > >> + device->kverbs_provider = true; > > > >> for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) { > > > >> if (!*(void **) ((void *) &device->ops + > > > >> mandatory_table[i].offset)) { > > > >> dev_warn(&device->dev, > > > >> - "Device is missing mandatory function %s\n", > > > >> + "Device is missing mandatory function %s, disabling kverbs support\n", > > > >> mandatory_table[i].name); > > > > > > > > Why not get rid of the message? > > > > > > Will do. > > > > You need to be nice to users, and I see value in this warning. > > If we are supporting this we should not have dev_warns for a normal > occurance. Something like RDMA tool should report no-kverbs or > somesuch instead I don't think that cleanup code do not use stubs immediately means that "we are supporting that mode". This warning is a reminder that such driver doesn't really belong to RDMA subsystem and many of built-in features are not working on this device. Thanks > > Jason
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 47ab34ee1a9d..6c1f71007ec4 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -121,13 +121,15 @@ static int ib_device_check_mandatory(struct ib_device *device) }; int i; + device->kverbs_provider = true; for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) { if (!*(void **) ((void *) &device->ops + mandatory_table[i].offset)) { dev_warn(&device->dev, - "Device is missing mandatory function %s\n", + "Device is missing mandatory function %s, disabling kverbs support\n", mandatory_table[i].name); - return -EINVAL; + device->kverbs_provider = false; + break; } } @@ -624,7 +626,8 @@ int ib_register_device(struct ib_device *device, const char *name, list_for_each_entry(client, &client_list, list) if (!add_client_context(device, client) && client->add) - client->add(device); + if (device->kverbs_provider || client->no_kverbs_req) + client->add(device); down_write(&lists_rwsem); list_add_tail(&device->core_list, &device_list); @@ -721,7 +724,8 @@ int ib_register_client(struct ib_client *client) list_for_each_entry(device, &device_list, core_list) if (!add_client_context(device, client) && client->add) - client->add(device); + if (device->kverbs_provider || client->no_kverbs_req) + client->add(device); down_write(&lists_rwsem); list_add_tail(&client->list, &client_list); diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index 9f9172eb1512..f0cef5693414 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -1128,6 +1128,7 @@ static const struct file_operations uverbs_mmap_fops = { static struct ib_client uverbs_client = { .name = "uverbs", + .no_kverbs_req = true, .add = ib_uverbs_add_one, .remove = ib_uverbs_remove_one }; diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index a3ceed3a040a..6ac5b7c3dddc 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -2561,6 +2561,8 @@ struct ib_device { __be64 node_guid; u32 local_dma_lkey; u16 is_switch:1; + /* Indicates kernel verbs support, should not be used in drivers */ + u8 kverbs_provider:1; u8 node_type; u8 phys_port_cnt; struct ib_device_attr attrs; @@ -2615,6 +2617,9 @@ struct ib_client { const struct sockaddr *addr, void *client_data); struct list_head list; + + /* kverbs are not required by the device */ + u8 no_kverbs_req:1; }; struct ib_device *ib_alloc_device(size_t size);
Drivers that do not provide kernel verbs support should not be used by ib kernel clients and fail. In case a device does not implement all mandatory verbs for kverbs usage mark it as a non kverbs provider and prevent its usage for all clients except for uverbs. The device is marked as a non kverbs provider using the 'kverbs_provider' flag which should only be set by the core code. The clients can choose whether kverbs are requested for it usage using the 'no_kverbs_req' flag which is currently set for uverbs only. This patch allows drivers to remove mandatory verbs stubs and simply set the callback to NULL. The IB device will be registered as a non-kverbs provider. Signed-off-by: Gal Pressman <galpress@amazon.com> --- drivers/infiniband/core/device.c | 12 ++++++++---- drivers/infiniband/core/uverbs_main.c | 1 + include/rdma/ib_verbs.h | 5 +++++ 3 files changed, 14 insertions(+), 4 deletions(-)