On Thu, Dec 20, 2018 at 08:41:47AM -0500, Yanjun Zhu wrote:
> The pool state is the duplicate of pool ref_cnt. If ref_cnt > 0,
> it indicates that state is valid. If ref_cnt = 0, it indicates
> that state is invalid.
>
> Signed-off-by: Zhu Yanjun <yanjun.zhu@xxxxxxxxxx>
> ---
> V1->V2: Follow Jason's advice, the state is replaced with ref_cnt.
Is it v2 the patch "IB/rxe: Remove duplicate pool invalidation"?
> ---
> drivers/infiniband/sw/rxe/rxe_pool.c | 20 +++-----------------
> drivers/infiniband/sw/rxe/rxe_pool.h | 6 ------
> 2 files changed, 3 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
> index 36b53fb..d8f969d 100644
> --- a/drivers/infiniband/sw/rxe/rxe_pool.c
> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
> @@ -222,8 +222,6 @@ int rxe_pool_init(
> pool->key_size = rxe_type_info[type].key_size;
> }
>
> - pool->state = RXE_POOL_STATE_VALID;
> -
> out:
> return err;
> }
> @@ -232,7 +230,6 @@ 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;
> kfree(pool->table);
> }
>
> @@ -243,14 +240,9 @@ 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;
> 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);
I wonder why it was surrounding the atomic_read call in the first place.
>
> rxe_pool_put(pool);
>
> @@ -380,17 +372,11 @@ 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 (!kref_get_unless_zero(&pool->ref_cnt))
> return NULL;
> - }
> - kref_get(&pool->ref_cnt);
> - read_unlock_irqrestore(&pool->pool_lock, flags);
>
> kref_get(&pool->rxe->ref_cnt);
>
> @@ -438,7 +424,7 @@ void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
>
> read_lock_irqsave(&pool->pool_lock, flags);
>
> - if (pool->state != RXE_POOL_STATE_VALID)
> + if (!kref_read(&pool->ref_cnt))
So ref_cnt cannot go below 0,right?
Will it be safer to check <= 0, just to protect against incorrect calls to
rxe_pool_cleanup?
> goto out;
>
> node = pool->tree.rb_node;
> @@ -470,7 +456,7 @@ void *rxe_pool_get_key(struct rxe_pool *pool, void *key)
>
> read_lock_irqsave(&pool->pool_lock, flags);
>
> - if (pool->state != RXE_POOL_STATE_VALID)
> + if (!kref_read(&pool->ref_cnt))
Ditto.
> goto out;
>
> node = pool->tree.rb_node;
> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.h b/drivers/infiniband/sw/rxe/rxe_pool.h
> index aa4ba30..eeca733 100644
> --- a/drivers/infiniband/sw/rxe/rxe_pool.h
> +++ b/drivers/infiniband/sw/rxe/rxe_pool.h
> @@ -73,11 +73,6 @@ struct rxe_type_info {
>
> extern struct rxe_type_info rxe_type_info[];
>
> -enum rxe_pool_state {
> - RXE_POOL_STATE_INVALID,
> - RXE_POOL_STATE_VALID,
> -};
> -
> struct rxe_pool_entry {
> struct rxe_pool *pool;
> struct kref ref_cnt;
> @@ -94,7 +89,6 @@ struct rxe_pool {
> size_t elem_size;
> struct kref ref_cnt;
> void (*cleanup)(struct rxe_pool_entry *obj);
> - enum rxe_pool_state state;
> enum rxe_pool_flags flags;
> enum rxe_elem_type type;
>
> --
> 2.7.4
>
