On 2018/12/19 13:22, Yanjun Zhu wrote:
On 2018/12/19 0:42, Jason Gunthorpe wrote:
On Tue, Dec 18, 2018 at 12:03:34AM -0500, Zhu Yanjun wrote:
In rxe_pool.c, pool state is protected by some read/write locks.
Now these read/write locks are replaced with atomic bitops. This can
make source code compact.
Signed-off-by: Zhu Yanjun <yanjun.zhu@xxxxxxxxxx>
drivers/infiniband/sw/rxe/rxe_pool.c | 22 +++++++---------------
drivers/infiniband/sw/rxe/rxe_pool.h | 2 +-
2 files changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c
b/drivers/infiniband/sw/rxe/rxe_pool.c
index 36b53fb94a49..1a7efe0e3bb1 100644
+++ b/drivers/infiniband/sw/rxe/rxe_pool.c
@@ -222,7 +222,7 @@ int rxe_pool_init(
pool->key_size = rxe_type_info[type].key_size;
}
- pool->state = RXE_POOL_STATE_VALID;
+ set_bit(RXE_POOL_STATE_VALID, &pool->state);
out:
return err;
@@ -232,7 +232,7 @@ static void rxe_pool_release(struct kref *kref)
{
struct rxe_pool *pool = container_of(kref, struct rxe_pool,
ref_cnt);
- pool->state = RXE_POOL_STATE_INVALID;
+ set_bit(RXE_POOL_STATE_INVALID, &pool->state);
kfree(pool->table);
}
@@ -243,14 +243,10 @@ static void rxe_pool_put(struct rxe_pool *pool)
int rxe_pool_cleanup(struct rxe_pool *pool)
{
- unsigned long flags;
-
- write_lock_irqsave(&pool->pool_lock, flags);
- pool->state = RXE_POOL_STATE_INVALID;
+ set_bit(RXE_POOL_STATE_INVALID, &pool->state);
if (atomic_read(&pool->num_elem) > 0)
pr_warn("%s pool destroyed with unfree'd elem\n",
pool_name(pool));
- write_unlock_irqrestore(&pool->pool_lock, flags);
rxe_pool_put(pool);
@@ -380,17 +376,13 @@ void rxe_drop_index(void *arg)
void *rxe_alloc(struct rxe_pool *pool)
{
struct rxe_pool_entry *elem;
- unsigned long flags;
might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC));
- read_lock_irqsave(&pool->pool_lock, flags);
- if (pool->state != RXE_POOL_STATE_VALID) {
- read_unlock_irqrestore(&pool->pool_lock, flags);
+ if (!test_bit(RXE_POOL_STATE_VALID, &pool->state))
return NULL;
- }
+
kref_get(&pool->ref_cnt);
- read_unlock_irqrestore(&pool->pool_lock, flags);
I doubt this locking can be removed without creating races between
pool_state and the kref_get.
I suspect the locking, refcounting, and lifetime in this pool code is
just all wrong. If you fix that then the pool->state probably just
goes away completely.
Thanks. Jason. Now V2 patch is sent. Please check it. I made simple
tests with Patch v2. And I can not find any bug.
If there is something wrong this Patch v2, please let me know.
Zhu Yanjun
Thanks. The pool->state is the duplicate of ref_cnt.
ref_cnt = 0 is the same with pool->state = RXE_POOL_STATE_INVALID
ref_cnt > 0 is the same with pool->state = RXE_POOL_STATE_VALID.
So we can use ref_cn to replace pool->state.
Thanks for your suggestions.
I will send V2 soon.
Zhu Yanjun
Good code usually does't have racy coding patterns like the above at
least..
Jason
