Hi Parav, On Fri, Feb 23, 2018 at 12:29 PM, Parav Pandit <parav@xxxxxxxxxxxx> wrote: > Hi Selvin, > >> -----Original Message----- >> From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- >> owner@xxxxxxxxxxxxxxx] On Behalf Of Selvin Xavier >> Sent: Friday, February 23, 2018 12:13 AM >> To: dledford@xxxxxxxxxx; jgg@xxxxxxxx >> Cc: linux-rdma@xxxxxxxxxxxxxxx; Selvin Xavier <selvin.xavier@xxxxxxxxxxxx> >> Subject: [PATCH for-rc 1/2] RDMA/core: Add helper function to retrieve driver >> gid context from gid index >> >> Adding a helper function to retrieve the driver gid context from the gid index. >> >> Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx> >> Signed-off-by: Selvin Xavier <selvin.xavier@xxxxxxxxxxxx> >> --- >> drivers/infiniband/core/cache.c | 18 ++++++++++++++++++ >> include/rdma/ib_cache.h | 13 +++++++++++++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c >> index e9a409d..f336467 100644 >> --- a/drivers/infiniband/core/cache.c >> +++ b/drivers/infiniband/core/cache.c >> @@ -859,6 +859,24 @@ int ib_find_cached_gid(struct ib_device *device, } >> EXPORT_SYMBOL(ib_find_cached_gid); >> >> +void *ib_get_gid_context_by_index(struct ib_device *device, >> + u8 port_num, >> + u16 index) >> +{ >> + struct ib_gid_table *table; >> + >> + if (!rdma_is_port_valid(device, port_num)) >> + return NULL; >> + >> + table = device->cache.ports[port_num - rdma_start_port(device)].gid; >> + >> + if (index < 0 || index >= table->sz) >> + return NULL; >> + >> + return table->data_vec[index].context; } >> +EXPORT_SYMBOL(ib_get_gid_context_by_index); > This is potentially unsafe API in present form being called from create_ah side which races with GID addition/deletion callback where context being updated which can be stale value. > At minimum reading of [index].context requires current table->rwlock. > However that is not going to be sufficient, because it can be NULL right after GID deletion and what provider driver reads is stale value using this API. > I have 10+ patches in two different patchsets under review for last few days in Leon and Mark's queue internally that overcomes such stale GID table entry access which is mainly cm, cma facing APIs but are usable in provider layer too. > However my API refactor doesn't expose the context pointer since its ULP/core layer facing API. > > Bodong in my team also working on provider driver changes which requires access to context pointer; this API is useful now in multiple provider drivers. > > I am adding following APIs. > struct ib_gid_attr* ib_get_cached_gid_ref(device, port, index); > ib_put_cached_gid_ref(); > > So ib_get_gid_context_by_index() API makes sense to call after calling ib_get_cached_gid_ref(). > This ensure that GID won't get deleted while ib_get_gid_context_by_index() is invoked. > Agree to you points above. It makes sense to post the re-worked patch after your series is out and accepted. I will wait for your series. >> + >> int ib_find_gid_by_filter(struct ib_device *device, >> const union ib_gid *gid, >> u8 port_num, >> diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h index >> 385ec88..e5727e9 100644 >> --- a/include/rdma/ib_cache.h >> +++ b/include/rdma/ib_cache.h >> @@ -105,6 +105,19 @@ int ib_find_gid_by_filter(struct ib_device *device, >> const struct ib_gid_attr *, >> void *), >> void *context, u16 *index); >> + >> +/** >> + * ib_get_gid_context_by_index - Returns HW driver's GID context for >> +the >> + * specified gid index > > I prefer to have this API named as ib_read_gid_context_by_index. Sure.. will make the change. > This gives better readability because usually get/put prefix APIs tend to take reference and in this API we don't want to do that. > There are few APIs which are coded with get_ prefix but they don't actually take the references, but let's have correct names for any new APIs. > (Some GID APIs are already getting fixed in two patchsets pending to post on ML, waiting for Leon, Mark to review). > >> + * @device: The device to query >> + * @port_num: The port number of the device where the GID value belongs >> + * @index: The index into the cached GID table */ >> + > Please add comment about Returns valid gid_context or NULL on error in comment section here. > will take care of it in the next revision >> +void *ib_get_gid_context_by_index(struct ib_device *device, >> + u8 port_num, >> + u16 index); >> + >> /** >> * ib_get_cached_pkey - Returns a cached PKey table entry >> * @device: The device to query. >> -- >> 2.5.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body >> of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at >> http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
