RE: [PATCH rdma-next v2 1/1] RDMA/mana: Provide a modern CQ creation interface

[Konstantin Taranov]

> > > > The uverbs CQ creation UAPI allows users to supply their own umem
> > > > for a
> > > CQ.
> > > > Update mana to support this workflow while preserving support for
> > > > creating umem through the legacy interface.
> > > >
> > > > To support RDMA objects that own umem, extend
> > > mana_ib_create_queue()
> > > > to return the umem to the caller and do not allocate umem if it
> > > > was allocted by the caller.
> > > >
> > > > Signed-off-by: Konstantin Taranov <kotaranov@xxxxxxxxxxxxx>
> > > > ---
> > > > v2: It is a rework of the patch proposed by Leon
> > >
> > > I am curious to know what changes were introduced?
> >
> > It is like your patch, but I kept get_umem in mana_ib_create_queue and
> > introduced ownership.
> > It made the code simpler and extendable. In your proposal, it was hard
> > to track the changes and it led to double free of the umem. With new
> > mana_ib_create_queue() it is clear from the caller what happens and no
> > special changes in the caller required.
> >
> > >
> > > >  drivers/infiniband/hw/mana/cq.c      | 125 +++++++++++++++++----------
> > > >  drivers/infiniband/hw/mana/device.c  |   1 +
> > > >  drivers/infiniband/hw/mana/main.c    |  30 +++++--
> > > >  drivers/infiniband/hw/mana/mana_ib.h |   5 +-
> > > >  drivers/infiniband/hw/mana/qp.c      |   5 +-
> > > >  drivers/infiniband/hw/mana/wq.c      |   3 +-
> > > >  6 files changed, 111 insertions(+), 58 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/mana/cq.c
> > > > b/drivers/infiniband/hw/mana/cq.c index b2749f971..fa951732a
> > > > 100644
> > > > --- a/drivers/infiniband/hw/mana/cq.c
> > > > +++ b/drivers/infiniband/hw/mana/cq.c
> > > > @@ -8,12 +8,8 @@
> > > >  int mana_ib_create_cq(struct ib_cq *ibcq, const struct
> > > > ib_cq_init_attr
> > > *attr,
> > > >  		      struct uverbs_attr_bundle *attrs)  {
> > > > -	struct ib_udata *udata = &attrs->driver_udata;
> > > >  	struct mana_ib_cq *cq = container_of(ibcq, struct mana_ib_cq, ibcq);
> > > > -	struct mana_ib_create_cq_resp resp = {};
> > > > -	struct mana_ib_ucontext *mana_ucontext;
> > > >  	struct ib_device *ibdev = ibcq->device;
> > > > -	struct mana_ib_create_cq ucmd = {};
> > > >  	struct mana_ib_dev *mdev;
> > > >  	bool is_rnic_cq;
> > > >  	u32 doorbell;
> > > > @@ -26,48 +22,91 @@ int mana_ib_create_cq(struct ib_cq *ibcq,
> > > > const
> > > struct ib_cq_init_attr *attr,
> > > >  	cq->cq_handle = INVALID_MANA_HANDLE;
> > > >  	is_rnic_cq = mana_ib_is_rnic(mdev);
> > > >
> > > > -	if (udata) {
> > > > -		if (udata->inlen < offsetof(struct mana_ib_create_cq, flags))
> > > > -			return -EINVAL;
> > > > -
> > > > -		err = ib_copy_from_udata(&ucmd, udata, min(sizeof(ucmd),
> > > udata->inlen));
> > > > -		if (err) {
> > > > -			ibdev_dbg(ibdev, "Failed to copy from udata for
> > > create cq, %d\n", err);
> > > > -			return err;
> > > > -		}
> > > > +	if (attr->cqe > U32_MAX / COMP_ENTRY_SIZE / 2 + 1)
> > > > +		return -EINVAL;
> > >
> > > We are talking about kernel verbs. ULPs are not designed to provide
> > > attributes and recover from random driver limitations.
> >
> > I understand, but there was an ask before to add that check as some
> > automated code verifier detected overflow. So if we remote it, I guess
> > we get again an ask to fix the potential overflow.
> >
> > >
> > > >
> > > > -		if ((!is_rnic_cq && attr->cqe > mdev-
> > > >adapter_caps.max_qp_wr) ||
> > > > -		    attr->cqe > U32_MAX / COMP_ENTRY_SIZE) {
> > > > -			ibdev_dbg(ibdev, "CQE %d exceeding limit\n", attr-
> > > >cqe);
> > > > -			return -EINVAL;
> > > > -		}
> > > > +	buf_size = MANA_PAGE_ALIGN(roundup_pow_of_two(attr->cqe *
> > > COMP_ENTRY_SIZE));
> > > > +	cq->cqe = buf_size / COMP_ENTRY_SIZE;
> > > > +	err = mana_ib_create_kernel_queue(mdev, buf_size, GDMA_CQ,
> > > &cq->queue);
> > > > +	if (err) {
> > > > +		ibdev_dbg(ibdev, "Failed to create kernel queue for create cq,
> > > %d\n", err);
> > > > +		return err;
> > > > +	}
> > > > +	doorbell = mdev->gdma_dev->doorbell;
> > > >
> > > > -		cq->cqe = attr->cqe;
> > > > -		err = mana_ib_create_queue(mdev, ucmd.buf_addr, cq->cqe
> > > * COMP_ENTRY_SIZE,
> > > > -					   &cq->queue);
> > > > +	if (is_rnic_cq) {
> > > > +		err = mana_ib_gd_create_cq(mdev, cq, doorbell);
> > > >  		if (err) {
> > > > -			ibdev_dbg(ibdev, "Failed to create queue for create
> > > cq, %d\n", err);
> > > > -			return err;
> > > > +			ibdev_dbg(ibdev, "Failed to create RNIC cq, %d\n",
> > > err);
> > > > +			goto err_destroy_queue;
> > > >  		}
> > > >
> > > > -		mana_ucontext = rdma_udata_to_drv_context(udata, struct
> > > mana_ib_ucontext,
> > > > -							  ibucontext);
> > > > -		doorbell = mana_ucontext->doorbell;
> > > > -	} else {
> > > > -		if (attr->cqe > U32_MAX / COMP_ENTRY_SIZE / 2 + 1) {
> > > > -			ibdev_dbg(ibdev, "CQE %d exceeding limit\n", attr-
> > > >cqe);
> > > > -			return -EINVAL;
> > > > -		}
> > > > -		buf_size = MANA_PAGE_ALIGN(roundup_pow_of_two(attr-
> > > >cqe * COMP_ENTRY_SIZE));
> > > > -		cq->cqe = buf_size / COMP_ENTRY_SIZE;
> > > > -		err = mana_ib_create_kernel_queue(mdev, buf_size,
> > > GDMA_CQ, &cq->queue);
> > > > +		err = mana_ib_install_cq_cb(mdev, cq);
> > > >  		if (err) {
> > > > -			ibdev_dbg(ibdev, "Failed to create kernel queue for
> > > create cq, %d\n", err);
> > > > -			return err;
> > > > +			ibdev_dbg(ibdev, "Failed to install cq callback, %d\n",
> > > err);
> > > > +			goto err_destroy_rnic_cq;
> > > >  		}
> > > > -		doorbell = mdev->gdma_dev->doorbell;
> > > >  	}
> > > >
> > > > +	spin_lock_init(&cq->cq_lock);
> > > > +	INIT_LIST_HEAD(&cq->list_send_qp);
> > > > +	INIT_LIST_HEAD(&cq->list_recv_qp);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +err_destroy_rnic_cq:
> > > > +	mana_ib_gd_destroy_cq(mdev, cq);
> > > > +err_destroy_queue:
> > > > +	mana_ib_destroy_queue(mdev, &cq->queue);
> > > > +
> > > > +	return err;
> > > > +}
> > > > +
> > > > +int mana_ib_create_user_cq(struct ib_cq *ibcq, const struct
> > > ib_cq_init_attr *attr,
> > > > +			   struct uverbs_attr_bundle *attrs) {
> > > > +	struct mana_ib_cq *cq = container_of(ibcq, struct mana_ib_cq, ibcq);
> > > > +	struct ib_udata *udata = &attrs->driver_udata;
> > > > +	struct mana_ib_create_cq_resp resp = {};
> > > > +	struct mana_ib_ucontext *mana_ucontext;
> > > > +	struct ib_device *ibdev = ibcq->device;
> > > > +	struct mana_ib_create_cq ucmd = {};
> > > > +	struct mana_ib_dev *mdev;
> > > > +	bool is_rnic_cq;
> > > > +	u32 doorbell;
> > > > +	int err;
> > > > +
> > > > +	mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
> > > > +
> > > > +	cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
> > > > +	cq->cq_handle = INVALID_MANA_HANDLE;
> > > > +	is_rnic_cq = mana_ib_is_rnic(mdev);
> > > > +
> > > > +	if (udata->inlen < offsetof(struct mana_ib_create_cq, flags))
> > > > +		return -EINVAL;
> > > > +
> > > > +	err = ib_copy_from_udata(&ucmd, udata, min(sizeof(ucmd), udata-
> > > >inlen));
> > > > +	if (err) {
> > > > +		ibdev_dbg(ibdev, "Failed to copy from udata for create cq,
> > > %d\n", err);
> > > > +		return err;
> > > > +	}
> > > > +
> > > > +	if ((!is_rnic_cq && attr->cqe > mdev->adapter_caps.max_qp_wr) ||
> > > > +	    attr->cqe > U32_MAX / COMP_ENTRY_SIZE)
> > > > +		return -EINVAL;
> > > > +
> > > > +	cq->cqe = attr->cqe;
> > > > +	err = mana_ib_create_queue(mdev, ucmd.buf_addr, cq->cqe *
> > > COMP_ENTRY_SIZE,
> > > > +				   &cq->queue, &ibcq->umem);
> 
> I just realized that I forgot to handle the case when ibcq->umem == NULL and
> mana fails later after this call. I need to clean ibcq->umem in this case.
> I will address that in v3. I am sorry.
> 

Hi Leon,
After re-reading the code, I see that there is no bug in v2 as the umem gets deallocated
on failure inside the handler of UVERBS_METHOD_CQ_CREATE. I also see that you also had
the same logic in v1. So, what is your recommendation? Leave v2 logic as is, so mana would
immediately give ownership of umem to cq->umem, and if mana_ib_create_user_cq() fails at later stage
it should not clean cq->umem and leave it to the caller handle (i.e., UVERBS_METHOD_CQ_CREATE)
to clean cq->umem regardless of who created it.

Or should I make v3, where I will assign umem to cq->umem right before return 0, so that if
mana_ib_create_user_cq() fails it does not change cq->umem at all.

> - Konstantin
>