| Message ID | 20211010235931.24042-4-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rxe: Fix potential races | expand |
On Sun, Oct 10, 2021 at 06:59:28PM -0500, Bob Pearson wrote: > In rxe_pool.c currently there are many cases where it is necessary to > compute the offset from a pool element struct to the object containing > the pool element in a type independent way. By saving a pointer to the > object when they are created extra work can be saved. > > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> > --- > drivers/infiniband/sw/rxe/rxe_pool.c | 16 +++++++++------- > drivers/infiniband/sw/rxe/rxe_pool.h | 1 + > 2 files changed, 10 insertions(+), 7 deletions(-) This would be better to just add a static_assert or build_bug_on that the offsetof() the rxe_pool_entry == 0. There is no reason for these to be sprinkled at every offset Then you don't need to do anything at all Jason
On 10/20/21 6:20 PM, Jason Gunthorpe wrote: > On Sun, Oct 10, 2021 at 06:59:28PM -0500, Bob Pearson wrote: >> In rxe_pool.c currently there are many cases where it is necessary to >> compute the offset from a pool element struct to the object containing >> the pool element in a type independent way. By saving a pointer to the >> object when they are created extra work can be saved. >> >> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> >> --- >> drivers/infiniband/sw/rxe/rxe_pool.c | 16 +++++++++------- >> drivers/infiniband/sw/rxe/rxe_pool.h | 1 + >> 2 files changed, 10 insertions(+), 7 deletions(-) > > This would be better to just add a static_assert or build_bug_on that > the offsetof() the rxe_pool_entry == 0. There is no reason for these > to be sprinkled at every offset > > Then you don't need to do anything at all > > Jason > I think you missed something. Once upon a time all the rxe objects had the local pool entry struct followed by the ib_xx struct and then anything else needed locally. As Leon has been moving the allocations to rdma-core he had to make the ib_xx struct first followed by the pool element. So there is a mish mash of offsets. Some are/were 0, and the rest are all different depending on the size of the struct from rdma-core. E.g. sizeof(struct ib_mr) != sizeof(struct ib_qp), etc. In order to make the API simple and consistent I use a macro to convert from object to pool elem. e.g. #define some_function(void *obj) __some_function(&obj->elem) but to convert back from elem to obj we need to subtract some random offset e.g. obj = elem->obj; without knowing the type of obj which is simpler than what we had before, roughly struct rxe_pool_info *info = &pool_info[elem->pool->type]; obj = (u8 *)(elem - info->elem_offset);
On Thu, Oct 21, 2021 at 12:21:15PM -0500, Bob Pearson wrote: > On 10/20/21 6:20 PM, Jason Gunthorpe wrote: > > On Sun, Oct 10, 2021 at 06:59:28PM -0500, Bob Pearson wrote: > >> In rxe_pool.c currently there are many cases where it is necessary to > >> compute the offset from a pool element struct to the object containing > >> the pool element in a type independent way. By saving a pointer to the > >> object when they are created extra work can be saved. > >> > >> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> > >> drivers/infiniband/sw/rxe/rxe_pool.c | 16 +++++++++------- > >> drivers/infiniband/sw/rxe/rxe_pool.h | 1 + > >> 2 files changed, 10 insertions(+), 7 deletions(-) > > > > This would be better to just add a static_assert or build_bug_on that > > the offsetof() the rxe_pool_entry == 0. There is no reason for these > > to be sprinkled at every offset > > > > Then you don't need to do anything at all > > > > Jason > > > I think you missed something. Once upon a time all the rxe objects had the local > pool entry struct followed by the ib_xx struct and then anything else needed locally. > As Leon has been moving the allocations to rdma-core he had to make the ib_xx struct > first followed by the pool element. Oh, I forgot that we were forcing the ib_xx to be at the start already Jason
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c index e9d74ad3f0b7..4caaecdb0d68 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.c +++ b/drivers/infiniband/sw/rxe/rxe_pool.c @@ -337,6 +337,7 @@ void *rxe_alloc_locked(struct rxe_pool *pool) elem = (struct rxe_pool_entry *)(obj + pool->elem_offset); elem->pool = pool; + elem->obj = obj; kref_init(&elem->ref_cnt); return obj; @@ -349,7 +350,7 @@ void *rxe_alloc_locked(struct rxe_pool *pool) void *rxe_alloc(struct rxe_pool *pool) { unsigned long flags; - u8 *obj; + void *obj; write_lock_irqsave(&pool->pool_lock, flags); obj = rxe_alloc_locked(pool); @@ -364,6 +365,7 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_entry *elem) goto out_cnt; elem->pool = pool; + elem->obj = (u8 *)elem - pool->elem_offset; kref_init(&elem->ref_cnt); return 0; @@ -378,13 +380,13 @@ void rxe_elem_release(struct kref *kref) struct rxe_pool_entry *elem = container_of(kref, struct rxe_pool_entry, ref_cnt); struct rxe_pool *pool = elem->pool; - u8 *obj; + void *obj; if (pool->cleanup) pool->cleanup(elem); if (!(pool->flags & RXE_POOL_NO_ALLOC)) { - obj = (u8 *)elem - pool->elem_offset; + obj = elem->obj; kfree(obj); } @@ -395,7 +397,7 @@ void *rxe_pool_get_index_locked(struct rxe_pool *pool, u32 index) { struct rb_node *node; struct rxe_pool_entry *elem; - u8 *obj; + void *obj; node = pool->index.tree.rb_node; @@ -412,7 +414,7 @@ void *rxe_pool_get_index_locked(struct rxe_pool *pool, u32 index) if (node) { kref_get(&elem->ref_cnt); - obj = (u8 *)elem - pool->elem_offset; + obj = elem->obj; } else { obj = NULL; } @@ -436,7 +438,7 @@ void *rxe_pool_get_key_locked(struct rxe_pool *pool, void *key) { struct rb_node *node; struct rxe_pool_entry *elem; - u8 *obj; + void *obj; int cmp; node = pool->key.tree.rb_node; @@ -457,7 +459,7 @@ void *rxe_pool_get_key_locked(struct rxe_pool *pool, void *key) if (node) { kref_get(&elem->ref_cnt); - obj = (u8 *)elem - pool->elem_offset; + obj = elem->obj; } else { obj = NULL; } diff --git a/drivers/infiniband/sw/rxe/rxe_pool.h b/drivers/infiniband/sw/rxe/rxe_pool.h index cd962dc5de9d..570bda77f4a6 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.h +++ b/drivers/infiniband/sw/rxe/rxe_pool.h @@ -46,6 +46,7 @@ struct rxe_type_info { struct rxe_pool_entry { struct rxe_pool *pool; + void *obj; struct kref ref_cnt; struct list_head list;
In rxe_pool.c currently there are many cases where it is necessary to compute the offset from a pool element struct to the object containing the pool element in a type independent way. By saving a pointer to the object when they are created extra work can be saved. Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> --- drivers/infiniband/sw/rxe/rxe_pool.c | 16 +++++++++------- drivers/infiniband/sw/rxe/rxe_pool.h | 1 + 2 files changed, 10 insertions(+), 7 deletions(-)