Allow ib_clients to nullify .add but still get .remove events reliably

PATCH New Sagi Grimberg 4 participants 9 messages Jul 2, 2017 – Aug 18, 2017

Subsystems (3)

InfiniBand NVMe NVMe target

lore

Share & tools

lore

Sagi Grimberg

author maintainer

2017-07-02 08:20 UTC #0 lore raw

v2 cover Allow ib_clients to nullify .add but still get .remove events reliably

Some ULPs are just interested in device removal notification with no specific association because they work over RDMA_CM so no need for detecting appearance of the devices. That wasn't the because the corresponding ib_client_data was never added to the device clients list when the ib_client registers, Patch 1 fixes it. Patches 2-3 just removes the empty .add callout nvme(t)-rdma set.

Changes from v1:
- reword patch 1 change log title
- collected review tags

Note that this will create a conflict with jens's for-4.13 which we added ib_client registration to nvmet-rdma.

Sagi Grimberg (3):
RDMA/core: make ib_device.add method optional
nvme-rdma: remove redundant empty device add callout
nvmet-rdma: remove redundant empty device add callout

 drivers/infiniband/core/device.c | 4 ++--
 drivers/nvme/host/rdma.c         | 5 -----
 drivers/nvme/target/rdma.c       | 5 -----
 3 files changed, 2 insertions(+), 12 deletions(-)

Signature · 1 line

2.7.4

Sagi Grimberg

author maintainer

2017-07-02 08:20 UTC #1 lore raw

PATCH v2 1/3 RDMA/core: make ib_device.add method optional

ib_clients can indeed fill .add to NULL, but then they will not see any device removal notifications. The reason is that that ib_register_client and ib_register_device checked existence of .add before adding the creating a corresponding client_data and adding it to the list. Simple condition reverse fixes the issue.

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Reviewed-by Christoph Hellwig <hch at lst.de>

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/infiniband/core/device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

index 81d447da0048..26eb428af473 100644
@@ -414,7 +414,7 @@ int ib_register_device(struct ib_device *device,
 	device->reg_state = IB_DEV_REGISTERED;
 
 	list_for_each_entry(client, &client_list, list)
-		if (client->add && !add_client_context(device, client))
+		if (!add_client_context(device, client) && client->add)
 			client->add(device);
 
 	down_write(&lists_rwsem);

@@ -499,7 +499,7 @@ int ib_register_client(struct ib_client *client)
 	mutex_lock(&device_mutex);
 
 	list_for_each_entry(device, &device_list, core_list)
-		if (client->add && !add_client_context(device, client))
+		if (!add_client_context(device, client) && client->add)
 			client->add(device);
 
 	down_write(&lists_rwsem);

Signature · 1 line

2.7.4

Sagi Grimberg

author maintainer

2017-07-06 07:51 UTC #2 lore raw

PATCH v2 1/3 RDMA/core: make ib_device.add method optional

Quoted 9 lines

ib_clients can indeed fill .add to NULL, but then they will not see any device removal notifications. The reason is that that ib_register_client and ib_register_device checked existence of .add before adding the creating a corresponding client_data and adding it to the list. Simple condition reverse fixes the issue.
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by Christoph Hellwig <hch at lst.de>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>

Any RDMA folks?

Leon Romanovsky

2017-07-06 08:33 UTC #3 lore raw

PATCH v2 1/3 RDMA/core: make ib_device.add method optional

Quoted 11 lines · Sagi Grimberg

Quoted 9 lines
ib_clients can indeed fill .add to NULL, but then they will not see any device removal notifications. The reason is that that ib_register_client and ib_register_device checked existence of .add before adding the creating a corresponding client_data and adding it to the list. Simple condition reverse fixes the issue.
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by Christoph Hellwig <hch at lst.de>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
Any RDMA folks?

It looks right and two gentlemen above already reviewed it.

Thanks,
Reviewed-by: Leon Romanovsky <leonro at mellanox.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20170706/eeb4cec0/attachment.sig>

Sagi Grimberg

author maintainer

2017-07-06 08:38 UTC #4 lore raw

PATCH v2 1/3 RDMA/core: make ib_device.add method optional

Quoted 3 lines

Quoted 1 line
Any RDMA folks?
It looks right and two gentlemen above already reviewed it.

None of them is focused on the RDMA core on a day-to-day basis.

Quoted 2 lines

Thanks,
Reviewed-by: Leon Romanovsky <leonro at mellanox.com>

Awesome.

Sagi Grimberg

author maintainer

2017-07-02 08:20 UTC #5 lore raw

PATCH v2 2/3 nvme-rdma: remove redundant empty device add callout

In reply to Sagi Grimberg

Now that its not needed, we can simply not assign it.

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Reviewed-by Christoph Hellwig <hch at lst.de>

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/nvme/host/rdma.c | 5 -----
 1 file changed, 5 deletions(-)

drivers/nvme/host/rdma.c +0 -5

index 6d4119dfbdaa..934183f80fff 100644
@@ -1956,10 +1956,6 @@ static struct nvmf_transport_ops nvme_rdma_transport = {
 	.create_ctrl	= nvme_rdma_create_ctrl,
 };
 
-static void nvme_rdma_add_one(struct ib_device *ib_device)
-{
-}
-
 static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 {
 	struct nvme_rdma_ctrl *ctrl;

@@ -1981,7 +1977,6 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 
 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.7.4

Sagi Grimberg

author maintainer

2017-07-02 08:20 UTC #6 lore raw

PATCH v2 3/3 nvmet-rdma: remove redundant empty device add callout

In reply to Sagi Grimberg

Now that its not needed, we can simply not assign it.

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Reviewed-by Christoph Hellwig <hch at lst.de>

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/nvme/target/rdma.c | 5 -----
 1 file changed, 5 deletions(-)

drivers/nvme/target/rdma.c +0 -5

index 56a4cba690b5..76d2bb793afe 100644
@@ -1510,10 +1510,6 @@ static struct nvmet_fabrics_ops nvmet_rdma_ops = {
 	.delete_ctrl		= nvmet_rdma_delete_ctrl,
 };
 
-static void nvmet_rdma_add_one(struct ib_device *ib_device)
-{
-}
-
 static void nvmet_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 {
 	struct nvmet_rdma_queue *queue;

@@ -1534,7 +1530,6 @@ static void nvmet_rdma_remove_one(struct ib_device *ib_device, void *client_data
 
 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.7.4

Max Gurtovoy

2017-07-02 09:14 UTC #7 lore raw

v2 cover Allow ib_clients to nullify .add but still get .remove events reliably

In reply to Sagi Grimberg

Quoted 10 lines · Sagi Grimberg

Some ULPs are just interested in device removal notification with no specific association because they work over RDMA_CM so no need for detecting appearance of the devices. That wasn't the because the corresponding ib_client_data was never added to the device clients list when the ib_client registers, Patch 1 fixes it. Patches 2-3 just removes the empty .add callout nvme(t)-rdma set.
Changes from v1:
- reword patch 1 change log title
- collected review tags

Quoted 7 lines

Note that this will create a conflict with jens's for-4.13 which we added ib_client registration to nvmet-rdma.
Sagi Grimberg (3):
RDMA/core: make ib_device.add method optional
nvme-rdma: remove redundant empty device add callout
nvmet-rdma: remove redundant empty device add callout

All these patches looks fine to me,

Reviewed-by Max Gurtovoy <maxg at mellanox.com>

Doug Ledford

2017-08-18 14:46 UTC #8 lore raw

v2 cover Allow ib_clients to nullify .add but still get .remove events reliably

Quoted 21 lines · Max Gurtovoy

Quoted 19 lines · Sagi Grimberg
Some ULPs are just interested in device removal notification with no specific
association because they work over RDMA_CM so no need for detecting appearance
of the devices. That wasn't the because the corresponding ib_client_data
was never added to the device clients list when the ib_client registers,
Patch 1 fixes it. Patches 2-3 just removes the empty .add callout nvme(t)-rdma set.
Changes from v1:
- reword patch 1 change log title
- collected review tags
Note that this will create a conflict with jens's for-4.13 which we added
ib_client registration to nvmet-rdma.

Quoted 8 lines

Quoted 4 lines
Sagi Grimberg (3):
RDMA/core: make ib_device.add method optional
nvme-rdma: remove redundant empty device add callout
nvmet-rdma: remove redundant empty device add callout
All these patches looks fine to me,
Reviewed-by Max Gurtovoy <maxg at mellanox.com>

Thanks, series applied.

Signature · 3 lines

Doug Ledford <dledford at redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD