use ib_client API to wrap ib_device
Subsystems (3)
v2 cover use ib_client API to wrap ib_device
Hi all,
This is a second patchset which uses ib_client API in order to wrap ib_device with a specific ULP device, which is created on add() callback and destroyed on remove(). Patches change initiator and target sides for NVMEoRDMA and ISER. Changes do not bring anything in terms of LoC but extra mutexes and lists are vanished, since ib_client API does required locking internally.
Also would be nice to avoid refcounting and synchronization in ISER case, but ULP ib_clients remove() callback is called before cma_client, thus ULP device should be alive till last reference put (NVMe queues explicit delete works with workqueue flush, which guarantees us that nvme_dev_dev_put() puts last reference in remove() callback).
The following are the minor fixes since previous RFC:
NVMe:
o Keep IB_DEVICE_MEM_MGT_EXTENSIONS check in nvme_rdma_alloc_device().
o No WARN_ON if nvme[t]_rdma_alloc_device() failed, just a log output.
iSER[t]:
o Rename ib_client.name from "iser[t]_rdma_ib" to "iser[t]_ib_client".
o Move all refcount changes to separate patches.
Roman Pen (8):
nvme-rdma: use ib_client API to wrap ib_device
nvmet-rdma: wrap raw kref_get/put() with corresponding helpers
nvmet-rdma: use ib_client API to wrap ib_device IB/iser: use refcount_t instead of just a raw integer IB/iser: use ib_client API to wrap ib_device IB/isert: embed mutex inside isert_device structure IB/isert: use refcount_t instead of just a raw integer IB/isert: use ib_client API to wrap ib_device
drivers/infiniband/ulp/iser/iscsi_iser.c | 21 ++++-- drivers/infiniband/ulp/iser/iscsi_iser.h | 14 ++-- drivers/infiniband/ulp/iser/iser_verbs.c | 73 +++++++++++++-------- drivers/infiniband/ulp/isert/ib_isert.c | 109 ++++++++++++++++++++----------- drivers/infiniband/ulp/isert/ib_isert.h | 4 +- drivers/nvme/host/rdma.c | 82 +++++++++++------------ drivers/nvme/target/rdma.c | 87 ++++++++++++------------ 7 files changed, 227 insertions(+), 163 deletions(-)
Signature · 1 line
2.13.1
PATCH v2 1/8 nvme-rdma: use ib_client API to wrap ib_device
ib_client API provides a way to wrap an ib_device with a specific ULP structure. Using that API local lists and mutexes can be completely avoided and allocation/removal paths become a bit cleaner.
--- drivers/nvme/host/rdma.c | 82 ++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 44 deletions(-)
drivers/nvme/host/rdma.c +38 -44
@@ -46,7 +46,6 @@ struct nvme_rdma_device {struct ib_device *dev;struct ib_pd *pd;struct kref ref; -struct list_head entry; };struct nvme_rdma_qe {
@@ -124,9 +123,7 @@ static inline struct nvme_rdma_ctrl *to_rdma_ctrl(struct nvme_ctrl *ctrl) return container_of(ctrl, struct nvme_rdma_ctrl, ctrl); } -static LIST_HEAD (device_list); -static DEFINE_MUTEX (device_list_mutex); - +static struct ib_client nvme_rdma_ib_client;static LIST_HEAD (nvme_rdma_ctrl_list);static DEFINE_MUTEX (nvme_rdma_ctrl_mutex);
@@ -325,17 +322,14 @@ static void nvme_rdma_free_dev(struct kref *ref)struct nvme_rdma_device *ndev = container_of(ref, struct nvme_rdma_device, ref); -mutex_lock (&device_list_mutex); -list_del (&ndev->entry); -mutex_unlock (&device_list_mutex); - +ib_set_client_data (ndev->dev, &nvme_rdma_ib_client,NULL );ib_dealloc_pd (ndev->pd);kfree (ndev); } -static void nvme_rdma_dev_put (struct nvme_rdma_device *dev) +static int nvme_rdma_dev_put (struct nvme_rdma_device *dev) { -kref_put (&dev->ref, nvme_rdma_free_dev); +return kref_put (&dev->ref, nvme_rdma_free_dev); }static int nvme_rdma_dev_get (struct nvme_rdma_device *dev)
@@ -348,43 +342,42 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id) {struct nvme_rdma_device *ndev; -mutex_lock (&device_list_mutex); -list_for_each_entry (ndev, &device_list, entry) { -if (ndev->dev->node_guid == cm_id->device->node_guid && -nvme_rdma_dev_get (ndev)) - goto out_unlock; + ndev =ib_get_client_data (cm_id->device, &nvme_rdma_ib_client); +if (ndev &&WARN_ON (!nvme_rdma_dev_get (ndev))) + ndev =NULL ; + +return ndev; +} + +static struct nvme_rdma_device * +nvme_rdma_alloc_device (struct ib_device *device) +{ +struct nvme_rdma_device *ndev; + +if (!(device->attrs.device_cap_flags & +IB_DEVICE_MEM_MGT_EXTENSIONS )) { +pr_err ("Memory registrations not supported.\n" ); +return NULL ; } ndev =kzalloc (sizeof (*ndev),GFP_KERNEL ); -if (!ndev) - goto out_err; +if (unlikely (!ndev)) +return NULL ; - ndev->dev = cm_id->device; + ndev->dev = device;kref_init (&ndev->ref); ndev->pd =ib_alloc_pd (ndev->dev, register_always ?0 :IB_PD_UNSAFE_GLOBAL_RKEY ); -if (IS_ERR (ndev->pd)) +if (unlikely (IS_ERR (ndev->pd))) goto out_free_dev; -if (!(ndev->dev->attrs.device_cap_flags & -IB_DEVICE_MEM_MGT_EXTENSIONS )) { -dev_err (&ndev->dev->dev, -"Memory registrations not supported.\n" ); - goto out_free_pd; - } +ib_set_client_data (ndev->dev, &nvme_rdma_ib_client, ndev); -list_add (&ndev->entry, &device_list); -out_unlock: -mutex_unlock (&device_list_mutex);return ndev; -out_free_pd: -ib_dealloc_pd (ndev->pd); out_free_dev:kfree (ndev); -out_err: -mutex_unlock (&device_list_mutex);return NULL ; }
@@ -2017,22 +2010,21 @@ static struct nvmf_transport_ops nvme_rdma_transport = { .create_ctrl = nvme_rdma_create_ctrl, }; -static void nvme_rdma_remove_one (struct ib_device *ib_device,void *client_data) +static void nvme_rdma_add_one (struct ib_device *ib_device) { -struct nvme_rdma_ctrl *ctrl;struct nvme_rdma_device *ndev; -bool found = false; -mutex_lock (&device_list_mutex); -list_for_each_entry (ndev, &device_list, entry) { -if (ndev->dev == ib_device) { - found = true; -break ; - } - } -mutex_unlock (&device_list_mutex); + ndev =nvme_rdma_alloc_device (ib_device); +if (unlikely (!ndev)) +pr_info ("Allocation of a device failed.\n" ); +} + +static void nvme_rdma_remove_one (struct ib_device *ib_device,void *client_data) +{ +struct nvme_rdma_device *ndev = client_data; +struct nvme_rdma_ctrl *ctrl; -if (!found) +if (unlikely (!ndev))return ;/* Delete all controllers using this device */
@@ -2045,10 +2037,12 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)mutex_unlock (&nvme_rdma_ctrl_mutex);flush_workqueue (nvme_delete_wq); +WARN_ON (!nvme_rdma_dev_put (ndev)); }static struct ib_client nvme_rdma_ib_client = { .name ="nvme_rdma" , + .add = nvme_rdma_add_one, .remove = nvme_rdma_remove_one };
Signature · 1 line
2.13.1
PATCH v2 1/8 nvme-rdma: use ib_client API to wrap ib_device
Quoted 59 lines · Roman Pen
ib_clientAPI provides a way to wrap anib_devicewith a specific ULP structure. Using that API local lists and mutexes can be completely avoided and allocation/removal paths become a bit cleaner.Signed-off-by Roman Pen <roman.penyaev at profitbricks.de>Cc Christoph Hellwig <hch at lst.de>Cc Steve Wise <swise at opengridcomputing.com>Cc Bart Van Assche <bart.vanassche at sandisk.com>Cc Sagi Grimberg <sagi at grimberg.me>Cc Doug Ledford <dledford at redhat.com>Cc linux-nvme at lists.infradead.orgdrivers/nvme/host/rdma.c | 82 ++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 44 deletions(-)drivers/nvme/host/rdma.c +5
-10+++ b/drivers/nvme/host/rdma.c@@ -46,7 +46,6 @@ struct nvme_rdma_device {struct ib_device *dev;struct ib_pd *pd;struct kref ref; -struct list_head entry; };struct nvme_rdma_qe {@@ -124,9 +123,7 @@ static inline struct nvme_rdma_ctrl *to_rdma_ctrl(struct nvme_ctrl *ctrl) return container_of(ctrl, struct nvme_rdma_ctrl, ctrl); } -static LIST_HEAD (device_list); -static DEFINE_MUTEX (device_list_mutex); - +static struct ib_client nvme_rdma_ib_client;static LIST_HEAD (nvme_rdma_ctrl_list);static DEFINE_MUTEX (nvme_rdma_ctrl_mutex);@@ -325,17 +322,14 @@ static void nvme_rdma_free_dev(struct kref *ref)struct nvme_rdma_device *ndev = container_of(ref, struct nvme_rdma_device, ref); -mutex_lock (&device_list_mutex); -list_del (&ndev->entry); -mutex_unlock (&device_list_mutex); - +ib_set_client_data (ndev->dev, &nvme_rdma_ib_client,NULL );ib_dealloc_pd (ndev->pd);kfree (ndev); } -static void nvme_rdma_dev_put (struct nvme_rdma_device *dev) +static int nvme_rdma_dev_put (struct nvme_rdma_device *dev) { -kref_put (&dev->ref, nvme_rdma_free_dev); +return kref_put (&dev->ref, nvme_rdma_free_dev); }static int nvme_rdma_dev_get (struct nvme_rdma_device *dev)@@ -348,43 +342,42 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id) {
drivers/nvme/host/rdma.c nvme_rdma_find_get_device(struct rdma_cm_id *cm_id) · line 347
Quoted 10 lines
drivers/nvme/host/rdma.c +3
-5struct nvme_rdma_device *ndev; -mutex_lock (&device_list_mutex); -list_for_each_entry (ndev, &device_list, entry) { -if (ndev->dev->node_guid == cm_id->device->node_guid && -nvme_rdma_dev_get (ndev)) - goto out_unlock; + ndev =ib_get_client_data (cm_id->device, &nvme_rdma_ib_client); +if (ndev &&WARN_ON (!nvme_rdma_dev_get (ndev))) + ndev =NULL ;
I think this is a much better idea to use the client data than maintaining an internal list - this is what client data was for..
But I wonder if the allocation of the client data should be deferred until the ulp actually needs to use the device?
Jason
PATCH v2 1/8 nvme-rdma: use ib_client API to wrap ib_device
Quoted 74 lines · Jason Gunthorpe
Quoted 59 lines · Roman Pen
ib_clientAPI provides a way to wrap anib_devicewith a specific ULP structure. Using that API local lists and mutexes can be completely avoided and allocation/removal paths become a bit cleaner.Signed-off-by Roman Pen <roman.penyaev at profitbricks.de>Cc Christoph Hellwig <hch at lst.de>Cc Steve Wise <swise at opengridcomputing.com>Cc Bart Van Assche <bart.vanassche at sandisk.com>Cc Sagi Grimberg <sagi at grimberg.me>Cc Doug Ledford <dledford at redhat.com>Cc linux-nvme at lists.infradead.orgdrivers/nvme/host/rdma.c | 82 ++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 44 deletions(-)drivers/nvme/host/rdma.c +5
-10+++ b/drivers/nvme/host/rdma.c@@ -46,7 +46,6 @@ struct nvme_rdma_device {struct ib_device *dev;struct ib_pd *pd;struct kref ref; -struct list_head entry; };struct nvme_rdma_qe {@@ -124,9 +123,7 @@ static inline struct nvme_rdma_ctrl *to_rdma_ctrl(struct nvme_ctrl *ctrl) return container_of(ctrl, struct nvme_rdma_ctrl, ctrl); } -static LIST_HEAD (device_list); -static DEFINE_MUTEX (device_list_mutex); - +static struct ib_client nvme_rdma_ib_client;static LIST_HEAD (nvme_rdma_ctrl_list);static DEFINE_MUTEX (nvme_rdma_ctrl_mutex);@@ -325,17 +322,14 @@ static void nvme_rdma_free_dev(struct kref *ref)struct nvme_rdma_device *ndev = container_of(ref, struct nvme_rdma_device, ref); -mutex_lock (&device_list_mutex); -list_del (&ndev->entry); -mutex_unlock (&device_list_mutex); - +ib_set_client_data (ndev->dev, &nvme_rdma_ib_client,NULL );ib_dealloc_pd (ndev->pd);kfree (ndev); } -static void nvme_rdma_dev_put (struct nvme_rdma_device *dev) +static int nvme_rdma_dev_put (struct nvme_rdma_device *dev) { -kref_put (&dev->ref, nvme_rdma_free_dev); +return kref_put (&dev->ref, nvme_rdma_free_dev); }static int nvme_rdma_dev_get (struct nvme_rdma_device *dev)@@ -348,43 +342,42 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id) {drivers/nvme/host/rdma.c nvme_rdma_find_get_device(struct rdma_cm_id *cm_id) · line 347
Quoted 10 lines
drivers/nvme/host/rdma.c +3
-5struct nvme_rdma_device *ndev; -mutex_lock (&device_list_mutex); -list_for_each_entry (ndev, &device_list, entry) { -if (ndev->dev->node_guid == cm_id->device->node_guid && -nvme_rdma_dev_get (ndev)) - goto out_unlock; + ndev =ib_get_client_data (cm_id->device, &nvme_rdma_ib_client); +if (ndev &&WARN_ON (!nvme_rdma_dev_get (ndev))) + ndev =NULL ;I think this is a much better idea to use the client data than maintaining an internal list - this is what client data was for..
Quoted 2 lines
But I wonder if the allocation of the client data should be deferred until the ulp actually needs to use the device?
I agree with Jason here. You will create ndev for every ib_device and this is redundant.
Also, any reason you use likely/unlikely statements not in the fast path ?
Max
Quoted 7 lines
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/8 nvme-rdma: use ib_client API to wrap ib_device
Quoted 75 lines · Max Gurtovoy
Quoted 72 lines · Jason Gunthorpe
Quoted 55 lines · Roman Pen
ib_clientAPI provides a way to wrap anib_devicewith a specific ULP structure. Using that API local lists and mutexes can be completely avoided and allocation/removal paths become a bit cleaner.Signed-off-by Roman Pen <roman.penyaev at profitbricks.de>Cc Christoph Hellwig <hch at lst.de>Cc Steve Wise <swise at opengridcomputing.com>Cc Bart Van Assche <bart.vanassche at sandisk.com>Cc Sagi Grimberg <sagi at grimberg.me>Cc Doug Ledford <dledford at redhat.com>Cc linux-nvme at lists.infradead.orgdrivers/nvme/host/rdma.c | 82 ++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 44 deletions(-)drivers/nvme/host/rdma.c +5
-7+++ b/drivers/nvme/host/rdma.c@@ -46,7 +46,6 @@ struct nvme_rdma_device {struct ib_device *dev;struct ib_pd *pd;struct kref ref; -struct list_head entry; };struct nvme_rdma_qe {@@ -124,9 +123,7 @@ static inline struct nvme_rdma_ctrl *to_rdma_ctrl(struct nvme_ctrl *ctrl) return container_of(ctrl, struct nvme_rdma_ctrl, ctrl); } -static LIST_HEAD(device_list); -static DEFINE_MUTEX (device_list_mutex); - +static struct ib_client nvme_rdma_ib_client;static LIST_HEAD (nvme_rdma_ctrl_list);static DEFINE_MUTEX (nvme_rdma_ctrl_mutex); @@ -325,17 +322,14 @@ static void nvme_rdma_free_dev(struct kref *ref) struct nvme_rdma_device *ndev = container_of(ref, struct nvme_rdma_device, ref); -mutex_lock (&device_list_mutex); -list_del (&ndev->entry); -mutex_unlock (&device_list_mutex); - +ib_set_client_data (ndev->dev, &nvme_rdma_ib_client,NULL );ib_dealloc_pd (ndev->pd);kfree (ndev); } -static void nvme_rdma_dev_put(struct nvme_rdma_device *dev) +static int nvme_rdma_dev_put(struct nvme_rdma_device *dev) { -kref_put (&dev->ref, nvme_rdma_free_dev); +return kref_put (&dev->ref, nvme_rdma_free_dev); }static int nvme_rdma_dev_get (struct nvme_rdma_device *dev)drivers/nvme/host/rdma.c nvme_rdma_find_get_device(struct rdma_cm_id *cm_id) · line 347
Quoted 10 lines
drivers/nvme/host/rdma.c +3
-4@@ -348,43 +342,42 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id) {struct nvme_rdma_device *ndev; -mutex_lock (&device_list_mutex); -list_for_each_entry (ndev, &device_list, entry) { -if (ndev->dev->node_guid == cm_id->device->node_guid && -nvme_rdma_dev_get (ndev)) - goto out_unlock; + ndev =ib_get_client_data (cm_id->device, &nvme_rdma_ib_client); +if (ndev &&WARN_ON (!nvme_rdma_dev_get (ndev))) + ndev =NULL ;I think this is a much better idea to use the client data than maintaining an internal list - this is what client data was for..
Quoted 6 lines
Quoted 2 lines
But I wonder if the allocation of the client data should be deferred until the ulp actually needs to use the device?
I agree with Jason here. You will create ndev for every
ib_deviceand this is redundant.
It seems this is a common pattern: wrap all ib devices in *add_one() and use them calling ib_get_client_data(). But of course ulp dev can be created on demand, when it is not found. I sent a chunk of the code in reply to Jason's email, please take a look.
Quoted 2 lines
Also, any reason you use likely/unlikely statements not in the fast path ?
IMO sometimes it is easier to read the code, e.g. error branches better distinguishable.
--
Roman
PATCH v2 1/8 nvme-rdma: use ib_client API to wrap ib_device
In reply to Jason GunthorpeQuoted 74 lines · Jason Gunthorpe
Quoted 59 lines · Roman Pen
ib_clientAPI provides a way to wrap anib_devicewith a specific ULP structure. Using that API local lists and mutexes can be completely avoided and allocation/removal paths become a bit cleaner.Signed-off-by Roman Pen <roman.penyaev at profitbricks.de>Cc Christoph Hellwig <hch at lst.de>Cc Steve Wise <swise at opengridcomputing.com>Cc Bart Van Assche <bart.vanassche at sandisk.com>Cc Sagi Grimberg <sagi at grimberg.me>Cc Doug Ledford <dledford at redhat.com>Cc linux-nvme at lists.infradead.orgdrivers/nvme/host/rdma.c | 82 ++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 44 deletions(-)drivers/nvme/host/rdma.c +5
-10+++ b/drivers/nvme/host/rdma.c@@ -46,7 +46,6 @@ struct nvme_rdma_device {struct ib_device *dev;struct ib_pd *pd;struct kref ref; -struct list_head entry; };struct nvme_rdma_qe {@@ -124,9 +123,7 @@ static inline struct nvme_rdma_ctrl *to_rdma_ctrl(struct nvme_ctrl *ctrl) return container_of(ctrl, struct nvme_rdma_ctrl, ctrl); } -static LIST_HEAD (device_list); -static DEFINE_MUTEX (device_list_mutex); - +static struct ib_client nvme_rdma_ib_client;static LIST_HEAD (nvme_rdma_ctrl_list);static DEFINE_MUTEX (nvme_rdma_ctrl_mutex);@@ -325,17 +322,14 @@ static void nvme_rdma_free_dev(struct kref *ref)struct nvme_rdma_device *ndev = container_of(ref, struct nvme_rdma_device, ref); -mutex_lock (&device_list_mutex); -list_del (&ndev->entry); -mutex_unlock (&device_list_mutex); - +ib_set_client_data (ndev->dev, &nvme_rdma_ib_client,NULL );ib_dealloc_pd (ndev->pd);kfree (ndev); } -static void nvme_rdma_dev_put (struct nvme_rdma_device *dev) +static int nvme_rdma_dev_put (struct nvme_rdma_device *dev) { -kref_put (&dev->ref, nvme_rdma_free_dev); +return kref_put (&dev->ref, nvme_rdma_free_dev); }static int nvme_rdma_dev_get (struct nvme_rdma_device *dev)@@ -348,43 +342,42 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id) {drivers/nvme/host/rdma.c nvme_rdma_find_get_device(struct rdma_cm_id *cm_id) · line 347
Quoted 10 lines
drivers/nvme/host/rdma.c +3
-5struct nvme_rdma_device *ndev; -mutex_lock (&device_list_mutex); -list_for_each_entry (ndev, &device_list, entry) { -if (ndev->dev->node_guid == cm_id->device->node_guid && -nvme_rdma_dev_get (ndev)) - goto out_unlock; + ndev =ib_get_client_data (cm_id->device, &nvme_rdma_ib_client); +if (ndev &&WARN_ON (!nvme_rdma_dev_get (ndev))) + ndev =NULL ;I think this is a much better idea to use the client data than maintaining an internal list - this is what client data was for..
Quoted 2 lines
But I wonder if the allocation of the client data should be deferred until the ulp actually needs to use the device?
No, actually it should not, I've reused common pattern: wrap all ib devices in *add_one() and use them calling ib_get_client_data().
Code might be rewritten as following with the help of a global mutex:
static DEFINE_MUTEX(nvme_devices_mutex);
static struct nvme_rdma_device *
nvme_rdma_find_get_device(struct rdma_cm_id *cm_id)
{
struct nvme_rdma_device *ndev;
mutex_lock(&nvme_devices_mutex);
ndev = ib_get_client_data(cm_id->device, &nvme_rdma_ib_client);
if (ndev && WARN_ON(!nvme_rdma_dev_get(ndev)))
ndev = NULL;
else if (!ndev) {
ndev = nvme_rdma_alloc_device(cmd_id->device);
if (likely(ndev))
ib_set_client_data(cm_id->device,
&nvmet_rdma_ib_client,
ndev);
}
mutex_unlock(&nvme_devices_mutex);
return ndev;
}
static void nvme_rdma_free_dev(struct kref *ref)
{
...
/* Remove pointer from internal lists before actual free */
ib_set_client_data(ndev->dev, &nvme_rdma_ib_client, NULL);
kfree(ndev);
}
static void
nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
{
...
flush_workqueue(nvme_delete_wq);
/*
* Here at this point all refs on ndev should be put,
* ndev freed and client_data set to NULL.
*/
WARN_ON(ib_get_client_data(ib_device, &nvme_rdma_ib_client));
}--
Roman
PATCH v2 2/8 nvmet-rdma: wrap raw kref_get/put() with corresponding helpers
In reply to Roman PenThis is just a preparation patch: put kref_put() and kref_get_unless_zero() inside helpers with more suitable names.
--- drivers/nvme/target/rdma.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
drivers/nvme/target/rdma.c +14 -5
@@ -775,7 +775,7 @@ static int nvmet_rdma_init_srq(struct nvmet_rdma_device *ndev)return ret; } -static void nvmet_rdma_free_dev (struct kref *ref) +static void nvmet_rdma_free_device (struct kref *ref) {struct nvmet_rdma_device *ndev = container_of(ref, struct nvmet_rdma_device, ref);
@@ -790,6 +790,16 @@ static void nvmet_rdma_free_dev(struct kref *ref)kfree (ndev); } +static int nvmet_rdma_dev_put (struct nvmet_rdma_device *dev) +{ +return kref_put (&dev->ref, nvmet_rdma_free_device); +} + +static int nvmet_rdma_dev_get (struct nvmet_rdma_device *dev) +{ +return kref_get_unless_zero (&dev->ref); +} +static struct nvmet_rdma_device *nvmet_rdma_find_get_device (struct rdma_cm_id *cm_id) {
@@ -799,7 +809,7 @@ nvmet_rdma_find_get_device(struct rdma_cm_id *cm_id)mutex_lock (&device_list_mutex);list_for_each_entry (ndev, &device_list, entry) {if (ndev->device->node_guid == cm_id->device->node_guid && -kref_get_unless_zero (&ndev->ref)) +nvmet_rdma_dev_get (ndev)) goto out_unlock; }
@@ -945,8 +955,7 @@ static void nvmet_rdma_release_queue_work(struct work_struct *w)struct nvmet_rdma_device *dev = queue->dev;nvmet_rdma_free_queue (queue); - -kref_put (&dev->ref, nvmet_rdma_free_dev); +nvmet_rdma_dev_put (dev); } static int
@@ -1163,7 +1172,7 @@ static int nvmet_rdma_queue_connect(struct rdma_cm_id *cm_id,return 0 ; put_device: -kref_put (&ndev->ref, nvmet_rdma_free_dev); +nvmet_rdma_dev_put (ndev);return ret; }
Signature · 1 line
2.13.1
PATCH v2 2/8 nvmet-rdma: wrap raw kref_get/put() with corresponding helpers
Quoted 28 lines · Roman Pen
This is just a preparation patch: put
kref_put()andkref_get_unless_zero()inside helpers with more suitable names.Signed-off-by Roman Pen <roman.penyaev at profitbricks.de>Cc Christoph Hellwig <hch at lst.de>Cc Steve Wise <swise at opengridcomputing.com>Cc Bart Van Assche <bart.vanassche at sandisk.com>Cc Sagi Grimberg <sagi at grimberg.me>Cc Doug Ledford <dledford at redhat.com>Cc linux-nvme at lists.infradead.orgdrivers/nvme/target/rdma.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)drivers/nvme/target/rdma.c +2
-1+++ b/drivers/nvme/target/rdma.c@@ -775,7 +775,7 @@ static int nvmet_rdma_init_srq(struct nvmet_rdma_device *ndev)return ret; } -static void nvmet_rdma_free_dev (struct kref *ref) +static void nvmet_rdma_free_device (struct kref *ref) {struct nvmet_rdma_device *ndev = container_of(ref, struct nvmet_rdma_device, ref);@@ -790,6 +790,16 @@ static void nvmet_rdma_free_dev(struct kref *ref)kfree (ndev); }
drivers/nvme/target/rdma.c static void nvmet_rdma_free_dev(struct kref *ref) · line 801
Quoted 10 lines
drivers/nvme/target/rdma.c +9
-0+static int nvmet_rdma_dev_put (struct nvmet_rdma_device *dev) +{ +return kref_put (&dev->ref, nvmet_rdma_free_device); +} + +static int nvmet_rdma_dev_get (struct nvmet_rdma_device *dev) +{ +return kref_get_unless_zero (&dev->ref); +}
Don't wrap get_unless_zero with something called get..
unless_zero should only be used in special situations where something tricky is going on.. In this case it looks like it is because a nvmet_rdma_device can continue to reside in the device_list() even after being fully 'put'..
Which does pose a question to this whole series, what have you done to ensure the client data access isn't racing with the removal of the device/client?
We might want to adjust the core code to make this a little saner, eg maybe the client_data should have an option that manages a kref'd memory blob internally with proper locking or something..
Jason
Christoph Hellwig
maintainerPATCH v2 2/8 nvmet-rdma: wrap raw kref_get/put() with corresponding helpers
Quoted 10 lines · Jason Gunthorpe
drivers/nvme/target/rdma.c static void nvmet_rdma_free_dev(struct kref *ref) · line 801
Quoted 8 lines
drivers/nvme/target/rdma.c +8
-0+{ +return kref_put (&dev->ref, nvmet_rdma_free_device); +} + +static int nvmet_rdma_dev_get (struct nvmet_rdma_device *dev) +{ +return kref_get_unless_zero (&dev->ref); +}Don't wrap
get_unless_zerowith something called get..
Agreed. And I hate all these wrappers. Either we use kref_* or we use wrappers and plain refcount_* ops, not both.
PATCH v2 2/8 nvmet-rdma: wrap raw kref_get/put() with corresponding helpers
Quoted 12 lines · Christoph Hellwig
Quoted 10 lines · Jason Gunthorpe
drivers/nvme/target/rdma.c static void nvmet_rdma_free_dev(struct kref *ref) · line 801
Quoted 8 lines
drivers/nvme/target/rdma.c +8
-0+{ +return kref_put (&dev->ref, nvmet_rdma_free_device); +} + +static int nvmet_rdma_dev_get (struct nvmet_rdma_device *dev) +{ +return kref_get_unless_zero (&dev->ref); +}Don't wrap
get_unless_zerowith something called get..
Quoted 2 lines
Agreed. And I hate all these wrappers. Either we use kref_* or we use wrappers and plain refcount_* ops, not both.
What I did not mention in the commit message is that this is done only to unify the code with host/rdma.c, which has exactly the same wrappers. Since I am touching both sides (host and target) this code difference annoys.
You think better to remove wrappers from nvme host or just leave everything as is?
--
Roman
PATCH v2 2/8 nvmet-rdma: wrap raw kref_get/put() with corresponding helpers
In reply to Jason GunthorpeQuoted 42 lines · Jason Gunthorpe
Quoted 28 lines · Roman Pen
This is just a preparation patch: put
kref_put()andkref_get_unless_zero()inside helpers with more suitable names.Signed-off-by Roman Pen <roman.penyaev at profitbricks.de>Cc Christoph Hellwig <hch at lst.de>Cc Steve Wise <swise at opengridcomputing.com>Cc Bart Van Assche <bart.vanassche at sandisk.com>Cc Sagi Grimberg <sagi at grimberg.me>Cc Doug Ledford <dledford at redhat.com>Cc linux-nvme at lists.infradead.orgdrivers/nvme/target/rdma.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)drivers/nvme/target/rdma.c +2
-1+++ b/drivers/nvme/target/rdma.c@@ -775,7 +775,7 @@ static int nvmet_rdma_init_srq(struct nvmet_rdma_device *ndev)return ret; } -static void nvmet_rdma_free_dev (struct kref *ref) +static void nvmet_rdma_free_device (struct kref *ref) {struct nvmet_rdma_device *ndev = container_of(ref, struct nvmet_rdma_device, ref);@@ -790,6 +790,16 @@ static void nvmet_rdma_free_dev(struct kref *ref)kfree (ndev); }drivers/nvme/target/rdma.c static void nvmet_rdma_free_dev(struct kref *ref) · line 801
Quoted 10 lines
drivers/nvme/target/rdma.c +9
-0+static int nvmet_rdma_dev_put (struct nvmet_rdma_device *dev) +{ +return kref_put (&dev->ref, nvmet_rdma_free_device); +} + +static int nvmet_rdma_dev_get (struct nvmet_rdma_device *dev) +{ +return kref_get_unless_zero (&dev->ref); +}Don't wrap
get_unless_zerowith something called get..
Quoted 8 lines
unless_zeroshould only be used in special situations where something tricky is going on.. In this case it looks like it is because anvmet_rdma_devicecan continue to reside in thedevice_list()even after being fully 'put'..Which does pose a question to this whole series, what have you done to ensure the client data access isn't racing with the removal of the device/client?
It races, but nvmet_rdma_remove_one() has guarantees that all queued release works will finish, so no users of ndev are left at the point when flush_scheduled_work() returns control:
static void
nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
{
...
flush_workqueue(nvme_delete_wq);
/*
* Here at this point no users of ndev are left
*/
}--
Roman
PATCH v2 3/8 nvmet-rdma: use ib_client API to wrap ib_device
In reply to Roman Penib_client API provides a way to wrap an ib_device with a specific ULP structure. Using that API local lists and mutexes can be completely avoided and allocation/removal paths become a bit cleaner.
--- drivers/nvme/target/rdma.c | 70 ++++++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 37 deletions(-)
drivers/nvme/target/rdma.c +33 -37
@@ -125,9 +125,7 @@ MODULE_PARM_DESC(use_srq, "Use shared receive queue.");static DEFINE_IDA (nvmet_rdma_queue_ida);static LIST_HEAD (nvmet_rdma_queue_list);static DEFINE_MUTEX (nvmet_rdma_queue_mutex); - -static LIST_HEAD (device_list); -static DEFINE_MUTEX (device_list_mutex); +static struct ib_client nvmet_rdma_ib_client;static bool nvmet_rdma_execute_command (struct nvmet_rdma_rsp *rsp);static void nvmet_rdma_send_done (struct ib_cq *cq,struct ib_wc *wc);
@@ -780,13 +778,9 @@ static void nvmet_rdma_free_device(struct kref *ref)struct nvmet_rdma_device *ndev = container_of(ref, struct nvmet_rdma_device, ref); -mutex_lock (&device_list_mutex); -list_del (&ndev->entry); -mutex_unlock (&device_list_mutex); - +ib_set_client_data (ndev->device, &nvmet_rdma_ib_client,NULL );nvmet_rdma_destroy_srq (ndev);ib_dealloc_pd (ndev->pd); -kfree (ndev); }
@@ -804,24 +798,29 @@ static struct nvmet_rdma_device * nvmet_rdma_find_get_device(struct rdma_cm_id *cm_id) {struct nvmet_rdma_device *ndev; -int ret; -mutex_lock (&device_list_mutex); -list_for_each_entry (ndev, &device_list, entry) { -if (ndev->device->node_guid == cm_id->device->node_guid && -nvmet_rdma_dev_get (ndev)) - goto out_unlock; - } + ndev =ib_get_client_data (cm_id->device, &nvmet_rdma_ib_client); +if (ndev &&WARN_ON (!nvmet_rdma_dev_get (ndev))) + ndev =NULL ; + +return ndev; +} + +static struct nvmet_rdma_device * +nvmet_rdma_alloc_device (struct ib_device *device) +{ +struct nvmet_rdma_device *ndev; +int ret; ndev =kzalloc (sizeof (*ndev),GFP_KERNEL ); -if (!ndev) - goto out_err; +if (unlikely (!ndev)) +return NULL ; - ndev->device = cm_id->device; + ndev->device = device;kref_init (&ndev->ref); ndev->pd =ib_alloc_pd (ndev->device,0 ); -if (IS_ERR (ndev->pd)) +if (unlikely (IS_ERR (ndev->pd))) goto out_free_dev;if (nvmet_rdma_use_srq) {
@@ -829,19 +828,15 @@ nvmet_rdma_find_get_device(struct rdma_cm_id *cm_id)if (ret) goto out_free_pd; } - -list_add (&ndev->entry, &device_list); -out_unlock: -mutex_unlock (&device_list_mutex); +ib_set_client_data (ndev->device, &nvmet_rdma_ib_client, ndev);pr_debug ("added %s.\n" , ndev->device->name); +return ndev; out_free_pd:ib_dealloc_pd (ndev->pd); out_free_dev:kfree (ndev); -out_err: -mutex_unlock (&device_list_mutex);return NULL ; }
@@ -1475,22 +1470,21 @@ static const struct nvmet_fabrics_ops nvmet_rdma_ops = { .disc_traddr = nvmet_rdma_disc_port_addr, }; -static void nvmet_rdma_remove_one (struct ib_device *ib_device,void *client_data) +static void nvmet_rdma_add_one (struct ib_device *ib_device) { -struct nvmet_rdma_queue *queue, *tmp;struct nvmet_rdma_device *ndev; -bool found = false; -mutex_lock (&device_list_mutex); -list_for_each_entry (ndev, &device_list, entry) { -if (ndev->device == ib_device) { - found = true; -break ; - } - } -mutex_unlock (&device_list_mutex); + ndev =nvmet_rdma_alloc_device (ib_device); +if (unlikely (!ndev)) +pr_info ("Allocation of a device failed.\n" ); +} + +static void nvmet_rdma_remove_one (struct ib_device *ib_device,void *client_data) +{ +struct nvmet_rdma_device *ndev = client_data; +struct nvmet_rdma_queue *queue, *tmp; -if (!found) +if (unlikely (!ndev))return ;/*
@@ -1510,10 +1504,12 @@ static void nvmet_rdma_remove_one(struct ib_device *ib_device, void *client_datamutex_unlock (&nvmet_rdma_queue_mutex);flush_scheduled_work (); +WARN_ON (!nvmet_rdma_dev_put (ndev)); }static struct ib_client nvmet_rdma_ib_client = { .name ="nvmet_rdma" , + .add = nvmet_rdma_add_one, .remove = nvmet_rdma_remove_one };
Signature · 1 line
2.13.1