Re: [PATCH for-next v10 06/11] RDMA/rxe: Add wait_for_completion to pool objects

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Feb 25, 2022 at 01:57:46PM -0600, Bob Pearson wrote:
>  int __rxe_add_ref(struct rxe_pool_elem *elem)
> @@ -262,3 +258,36 @@ int __rxe_drop_ref(struct rxe_pool_elem *elem)
>  	return kref_put_lock_irqsave(&elem->ref_cnt, rxe_elem_release,
>  			&pool->xa.xa_lock);

Also can't touch the xa_lock to do stuff like this,

> +int __rxe_drop_wait(struct rxe_pool_elem *elem)

I think I would call this something else since it it basically unconditionally
frees the memory.

> +{
> +	struct rxe_pool *pool = elem->pool;
> +	static int timeout = RXE_POOL_TIMEOUT;
> +	int ret;
> +
> +	__rxe_drop_ref(elem);

> +	if (timeout) {
> +		ret = wait_for_completion_timeout(&elem->complete, timeout);
> +		if (!ret) {
> +			pr_warn("Timed out waiting for %s#%d\n",
> +				pool->name + 4, elem->index);

This is a WARN_ON event, kernel is broken, and you should leak the
memory rather than cause memory corruption.

> +			if (++pool->timeouts == RXE_MAX_POOL_TIMEOUTS) {
> +				timeout = 0;
> +				pr_warn("Reached max %s timeouts.\n",
> +					pool->name + 4);
> +			}

Why?

> +		}
> +	}
> +
> +	if (pool->cleanup)
> +		pool->cleanup(elem);
> +
> +	if (pool->flags & RXE_POOL_ALLOC)
> +		kfree(elem->obj);
> +
> +	atomic_dec(&pool->num_elem);
> +
> +	return ret;

And we return a failure code but freed the memory? This shouldn't fail

But the idea is right..

Jason