On 2018/12/21 2:04, Yuval Shaia wrote:
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"?
Yeah. The patch "IB/rxe: Remove duplicate pool invalidation" is included
in this patch.
In fact, the V1 patch is "IB: rxe: replace read/write locks with atomic
bitops".
---
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.
Yeah. atomic_read only gets the value of pool->num_elem. And if
pool->num_elem > 0, a log will pop out.
It does not change anything.
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?
Sure. I agree with you. But I am not sure whether it is necessary to
change to check <=0 since the root cause is "rxe_pool_cleanup
incorrectly called".
Thanks for your review.
Zhu Yanjun
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
