On Fri, Dec 21, 2018 at 04:00:23PM +0800, Yanjun Zhu wrote:
>
> 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:
>
> 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;
Oh. Use only one kref per struct.
Delete the sub-kref and move the table freeing to the release
function of rxe_dev's kref..
The pool should be functional as long as the rxe_dev exists, no need
for the invalid thing.
Jason
