Re: [PATCH 1/1] IB: rxe: replace read/write locks with atomic bitops

[Yanjun Zhu <yanjun.zhu@xxxxxxxxxx>]

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. 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