| Message ID | 1582006810-32174-2-git-send-email-selvin.xavier@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Retrieve HW GID context from ib_gid_attr | expand |
On Mon, Feb 17, 2020 at 10:20:09PM -0800, Selvin Xavier wrote: > Adding a helper function to retrieve the driver gid context > from the gid attr. > > Suggested-by: Jason Gunthorpe <jgg@mellanox.com> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++ > include/rdma/ib_cache.h | 1 + > 2 files changed, 42 insertions(+) > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > index 17bfedd..1b73a71 100644 > +++ b/drivers/infiniband/core/cache.c > @@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num, > EXPORT_SYMBOL(rdma_query_gid); > > /** > + * rdma_read_gid_hw_context - Read the HW GID context from GID attribute > + * @attr: Potinter to the GID attribute > + * > + * rdma_read_gid_hw_context() reads the vendor drivers GID HW > + * context corresponding to SGID attr. It takes reference to the GID > + * attribute and this need to be released by the caller using > + * rdma_put_gid_attr > + * > + * Returns HW context on success or NULL on error > + * > + */ > +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr) > +{ > + struct ib_gid_table_entry *entry = > + container_of(attr, struct ib_gid_table_entry, attr); > + struct ib_device *device = entry->attr.device; > + u8 port_num = entry->attr.port_num; > + struct ib_gid_table *table; > + unsigned long flags; > + void *context = NULL; > + > + if (!rdma_is_port_valid(device, port_num)) > + return NULL; > + > + table = rdma_gid_table(device, port_num); > + read_lock_irqsave(&table->rwlock, flags); > + > + if (attr->index < 0 || attr->index >= table->sz || > + !is_gid_entry_valid(table->data_vec[attr->index])) > + goto done; Why all this validation and locking? ib_gid_attrs are only created by the core code.. > + get_gid_entry(entry); And why a get? Surely it is invalid to call this function without a get already held? Jason
On Tue, Feb 18, 2020 at 9:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Mon, Feb 17, 2020 at 10:20:09PM -0800, Selvin Xavier wrote: > > Adding a helper function to retrieve the driver gid context > > from the gid attr. > > > > Suggested-by: Jason Gunthorpe <jgg@mellanox.com> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > > drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > include/rdma/ib_cache.h | 1 + > > 2 files changed, 42 insertions(+) > > > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > > index 17bfedd..1b73a71 100644 > > +++ b/drivers/infiniband/core/cache.c > > @@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num, > > EXPORT_SYMBOL(rdma_query_gid); > > > > /** > > + * rdma_read_gid_hw_context - Read the HW GID context from GID attribute > > + * @attr: Potinter to the GID attribute > > + * > > + * rdma_read_gid_hw_context() reads the vendor drivers GID HW > > + * context corresponding to SGID attr. It takes reference to the GID > > + * attribute and this need to be released by the caller using > > + * rdma_put_gid_attr > > + * > > + * Returns HW context on success or NULL on error > > + * > > + */ > > +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr) > > +{ > > + struct ib_gid_table_entry *entry = > > + container_of(attr, struct ib_gid_table_entry, attr); > > + struct ib_device *device = entry->attr.device; > > + u8 port_num = entry->attr.port_num; > > + struct ib_gid_table *table; > > + unsigned long flags; > > + void *context = NULL; > > + > > + if (!rdma_is_port_valid(device, port_num)) > > + return NULL; > > + > > + table = rdma_gid_table(device, port_num); > > + read_lock_irqsave(&table->rwlock, flags); > > + > > + if (attr->index < 0 || attr->index >= table->sz || > > + !is_gid_entry_valid(table->data_vec[attr->index])) > > + goto done; > > Why all this validation and locking? ib_gid_attrs are only created by > the core code.. Locking and validation was added to avoid any scenario where the gid entry is deleted while we are executing this API. I saw similar implementation for rdma_read_gid_attr_ndev_rcu symbol. > > > + get_gid_entry(entry); > > And why a get? Surely it is invalid to call this function without a > get already held? Getting the reference to the entry only if the entry is valid after all the checks. This is the reason for invoking this inside the function, rather than in the caller. Added a note in the symbol description that the caller needs to release reference using rdma_put_gid_attr, once the caller finished using the void * pointer returned. > > Jason
On Tue, Feb 18, 2020 at 10:12:17PM +0530, Selvin Xavier wrote: > On Tue, Feb 18, 2020 at 9:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Mon, Feb 17, 2020 at 10:20:09PM -0800, Selvin Xavier wrote: > > > Adding a helper function to retrieve the driver gid context > > > from the gid attr. > > > > > > Suggested-by: Jason Gunthorpe <jgg@mellanox.com> > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > > > drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > > include/rdma/ib_cache.h | 1 + > > > 2 files changed, 42 insertions(+) > > > > > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > > > index 17bfedd..1b73a71 100644 > > > +++ b/drivers/infiniband/core/cache.c > > > @@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num, > > > EXPORT_SYMBOL(rdma_query_gid); > > > > > > /** > > > + * rdma_read_gid_hw_context - Read the HW GID context from GID attribute > > > + * @attr: Potinter to the GID attribute > > > + * > > > + * rdma_read_gid_hw_context() reads the vendor drivers GID HW > > > + * context corresponding to SGID attr. It takes reference to the GID > > > + * attribute and this need to be released by the caller using > > > + * rdma_put_gid_attr > > > + * > > > + * Returns HW context on success or NULL on error > > > + * > > > + */ > > > +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr) > > > +{ > > > + struct ib_gid_table_entry *entry = > > > + container_of(attr, struct ib_gid_table_entry, attr); > > > + struct ib_device *device = entry->attr.device; > > > + u8 port_num = entry->attr.port_num; > > > + struct ib_gid_table *table; > > > + unsigned long flags; > > > + void *context = NULL; > > > + > > > + if (!rdma_is_port_valid(device, port_num)) > > > + return NULL; > > > + > > > + table = rdma_gid_table(device, port_num); > > > + read_lock_irqsave(&table->rwlock, flags); > > > + > > > + if (attr->index < 0 || attr->index >= table->sz || > > > + !is_gid_entry_valid(table->data_vec[attr->index])) > > > + goto done; > > > > Why all this validation and locking? ib_gid_attrs are only created by > > the core code.. > Locking and validation was added to avoid any scenario where the gid > entry is deleted while we are > executing this API. I saw similar implementation for > rdma_read_gid_attr_ndev_rcu symbol. This is required to deref the ndev as GID_TABLE_ENTRY_PENDING_DEL no longer has the ndev memory. However here things are not derefing the ndev, there is no reason to check this. The driver state attached to a gid entry should always be valid so long as the pointer is valid. This is the entire point of the refcounting scheme. > > > + get_gid_entry(entry); > > > > And why a get? Surely it is invalid to call this function without a > > get already held? > Getting the reference to the entry only if the entry is valid after > all the checks. This is the > reason for invoking this inside the function, rather than in the > caller. Added a note in the symbol description > that the caller needs to release reference using rdma_put_gid_attr, > once the caller finished using > the void * pointer returned. That makes no sense, we already *must* have a get at this point or the whole thing is really buggy Jason
On Wed, Feb 19, 2020 at 1:33 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Tue, Feb 18, 2020 at 10:12:17PM +0530, Selvin Xavier wrote: > > On Tue, Feb 18, 2020 at 9:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > On Mon, Feb 17, 2020 at 10:20:09PM -0800, Selvin Xavier wrote: > > > > Adding a helper function to retrieve the driver gid context > > > > from the gid attr. > > > > > > > > Suggested-by: Jason Gunthorpe <jgg@mellanox.com> > > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > > > > drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > > > include/rdma/ib_cache.h | 1 + > > > > 2 files changed, 42 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > > > > index 17bfedd..1b73a71 100644 > > > > +++ b/drivers/infiniband/core/cache.c > > > > @@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num, > > > > EXPORT_SYMBOL(rdma_query_gid); > > > > > > > > /** > > > > + * rdma_read_gid_hw_context - Read the HW GID context from GID attribute > > > > + * @attr: Potinter to the GID attribute > > > > + * > > > > + * rdma_read_gid_hw_context() reads the vendor drivers GID HW > > > > + * context corresponding to SGID attr. It takes reference to the GID > > > > + * attribute and this need to be released by the caller using > > > > + * rdma_put_gid_attr > > > > + * > > > > + * Returns HW context on success or NULL on error > > > > + * > > > > + */ > > > > +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr) > > > > +{ > > > > + struct ib_gid_table_entry *entry = > > > > + container_of(attr, struct ib_gid_table_entry, attr); > > > > + struct ib_device *device = entry->attr.device; > > > > + u8 port_num = entry->attr.port_num; > > > > + struct ib_gid_table *table; > > > > + unsigned long flags; > > > > + void *context = NULL; > > > > + > > > > + if (!rdma_is_port_valid(device, port_num)) > > > > + return NULL; > > > > + > > > > + table = rdma_gid_table(device, port_num); > > > > + read_lock_irqsave(&table->rwlock, flags); > > > > + > > > > + if (attr->index < 0 || attr->index >= table->sz || > > > > + !is_gid_entry_valid(table->data_vec[attr->index])) > > > > + goto done; > > > > > > Why all this validation and locking? ib_gid_attrs are only created by > > > the core code.. > > Locking and validation was added to avoid any scenario where the gid > > entry is deleted while we are > > executing this API. I saw similar implementation for > > rdma_read_gid_attr_ndev_rcu symbol. > > This is required to deref the ndev as GID_TABLE_ENTRY_PENDING_DEL no > longer has the ndev memory. > > However here things are not derefing the ndev, there is no reason to > check this. The driver state attached to a gid entry should always be > valid so long as the pointer is valid. This is the entire point of the > refcounting scheme. > > > > > + get_gid_entry(entry); > > > > > > And why a get? Surely it is invalid to call this function without a > > > get already held? > > Getting the reference to the entry only if the entry is valid after > > all the checks. This is the > > reason for invoking this inside the function, rather than in the > > caller. Added a note in the symbol description > > that the caller needs to release reference using rdma_put_gid_attr, > > once the caller finished using > > the void * pointer returned. > > That makes no sense, we already *must* have a get at this point or the > whole thing is really buggy > Got it now. I missed the fact that the ref is taken in rdma_fill_sgid_attr for both create_ah and modify_qp contexts. Thanks for your explanation. I will respin the patch. Thanks, Selvin > Jason
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c index 17bfedd..1b73a71 100644 --- a/drivers/infiniband/core/cache.c +++ b/drivers/infiniband/core/cache.c @@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num, EXPORT_SYMBOL(rdma_query_gid); /** + * rdma_read_gid_hw_context - Read the HW GID context from GID attribute + * @attr: Potinter to the GID attribute + * + * rdma_read_gid_hw_context() reads the vendor drivers GID HW + * context corresponding to SGID attr. It takes reference to the GID + * attribute and this need to be released by the caller using + * rdma_put_gid_attr + * + * Returns HW context on success or NULL on error + * + */ +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr) +{ + struct ib_gid_table_entry *entry = + container_of(attr, struct ib_gid_table_entry, attr); + struct ib_device *device = entry->attr.device; + u8 port_num = entry->attr.port_num; + struct ib_gid_table *table; + unsigned long flags; + void *context = NULL; + + if (!rdma_is_port_valid(device, port_num)) + return NULL; + + table = rdma_gid_table(device, port_num); + read_lock_irqsave(&table->rwlock, flags); + + if (attr->index < 0 || attr->index >= table->sz || + !is_gid_entry_valid(table->data_vec[attr->index])) + goto done; + + get_gid_entry(entry); + context = entry->context; + +done: + read_unlock_irqrestore(&table->rwlock, flags); + return context; +} +EXPORT_SYMBOL(rdma_read_gid_hw_context); + +/** * rdma_find_gid - Returns SGID attributes if the matching GID is found. * @device: The device to query. * @gid: The GID value to search for. diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h index 870b5e6..e06d133 100644 --- a/include/rdma/ib_cache.h +++ b/include/rdma/ib_cache.h @@ -39,6 +39,7 @@ int rdma_query_gid(struct ib_device *device, u8 port_num, int index, union ib_gid *gid); +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr); const struct ib_gid_attr *rdma_find_gid(struct ib_device *device, const union ib_gid *gid, enum ib_gid_type gid_type,
Adding a helper function to retrieve the driver gid context from the gid attr. Suggested-by: Jason Gunthorpe <jgg@mellanox.com> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> --- drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++ include/rdma/ib_cache.h | 1 + 2 files changed, 42 insertions(+)