On 2018/12/21 5:09, Jason Gunthorpe wrote:
On Thu, Dec 20, 2018 at 08:41:47AM -0500, Zhu Yanjun 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.
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
+++ 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);
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))
goto out;
These kref_reads make no sense, the caller has to be holding a kref on
pool to call this API, otherwise it is already a free'd pointer. So
there is no reason to check the kref.
Did you audit that all callers hold the kref?
No. Take pg->pool as an example.
In drivers/infiniband/sw/rxe/rxe_verbs.c:
"
static struct ib_qp *rxe_create_qp(struct ib_pd *ibpd,
struct ib_qp_init_attr *init,
struct ib_udata *udata)
{
int err;
struct rxe_dev *rxe = to_rdev(ibpd->device);
struct rxe_pd *pd = to_rpd(ibpd);
struct rxe_qp *qp;
struct rxe_create_qp_resp __user *uresp = NULL;
if (udata) {
if (udata->outlen < sizeof(*uresp))
return ERR_PTR(-EINVAL);
uresp = udata->outbuf;
}
err = rxe_qp_chk_init(rxe, init);
if (err)
goto err1;
qp = rxe_alloc(&rxe->qp_pool); <---This will call rxe_alloc
function.
if (!qp) {
err = -ENOMEM;
goto err1;
}
if (udata) {
if (udata->inlen) {
err = -EINVAL;
goto err2;
}
qp->is_user = 1;
}
rxe_add_index(qp);
...
"
Before qp = rxe_alloc(&rxe->qp_pool);, there is no any holding a kref on
pool.
And qp_pool is not pointer variable. So it will not be freed.
drivers/infiniband/sw/rxe/rxe_verbs.h:
"
struct rxe_dev {
struct ib_device ib_dev;
struct ib_device_attr attr;
int max_ucontext;
int max_inline_data;
struct kref ref_cnt;
struct mutex usdev_lock;
struct net_device *ndev;
int xmit_errors;
struct rxe_pool uc_pool;
struct rxe_pool pd_pool;
struct rxe_pool ah_pool;
struct rxe_pool srq_pool;
struct rxe_pool qp_pool; <----This is not a pointer
variable.
struct rxe_pool cq_pool;
struct rxe_pool mr_pool;
struct rxe_pool mw_pool;
struct rxe_pool mc_grp_pool;
struct rxe_pool mc_elem_pool;
...
"
And in rxe_pool_put
static void rxe_pool_put(struct rxe_pool *pool)
{
kref_put(&pool->ref_cnt, rxe_pool_release);
}
The function will decrease pool->fef_cnt. It is possible that
pool->ref_cnt is decreased to zero.
So it is necessary to test kref_read(&pool->ref_cnt).
If I am wrong, please let me know.
Thanks a lot.
Zhu Yanjun
Jason
