On Mon, Aug 29, 2016 at 07:44:24 -0700, Yuval Shaia wrote:
> On Wed, Aug 03, 2016 at 04:27:40PM -0700, Adit Ranadive wrote:
> > This patch adds the ability to create, modify, query and destroy QPs.
> > The PVRDMA device supports RC, UD and GSI QPs.
> >
> > Changes v2->v3:
> > - Removed boolean in pvrdma_cmd_post.
> >
> > Reviewed-by: Jorgen Hansen <jhansen@xxxxxxxxxx>
> > Reviewed-by: George Zhang <georgezhang@xxxxxxxxxx>
> > Reviewed-by: Aditya Sarwade <asarwade@xxxxxxxxxx>
> > Reviewed-by: Bryan Tan <bryantan@xxxxxxxxxx>
> > Signed-off-by: Adit Ranadive <aditr@xxxxxxxxxx>
> > ---
> > drivers/infiniband/hw/pvrdma/pvrdma_qp.c | 975
> > +++++++++++++++++++++++++++++++
> > 1 file changed, 975 insertions(+)
> > create mode 100644 drivers/infiniband/hw/pvrdma/pvrdma_qp.c
> >
...
> > +static int pvrdma_set_rq_size(struct pvrdma_dev *dev,
> > + struct ib_qp_cap *cap,
> > + struct pvrdma_qp *qp)
> > +{
> > + if (cap->max_recv_wr > dev->dsr->caps.max_qp_wr ||
> > + cap->max_recv_sge > dev->dsr->caps.max_sge) {
> > + dev_warn(&dev->pdev->dev, "recv queue size invalid\n");
> > + return -EINVAL;
> > + }
>
> (extremely) minor suggestion to rename cap to req_cap as w/o knowing the
> context (cap is caller's requested attr) one might be confused how come QP
> max can be more than device max.
> Again, minor.
> (applied also to pvrdma_set_sq_size).
Ok. Makes sense. Adding in v4.
> > +
> > + qp->rq.wqe_cnt = roundup_pow_of_two(max(1U, cap-
> >max_recv_wr));
> > + qp->rq.max_gs = roundup_pow_of_two(max(1U, cap-
> >max_recv_sge));
> > +
> > + /* Write back */
> > + cap->max_recv_wr = qp->rq.wqe_cnt;
> > + cap->max_recv_sge = qp->rq.max_gs;
> > +
> > + qp->rq.wqe_size = roundup_pow_of_two(sizeof(struct
> pvrdma_rq_wqe_hdr) +
> > + sizeof(struct ib_sge) *
> > + qp->rq.max_gs);
> > + qp->npages_recv = (qp->rq.wqe_cnt * qp->rq.wqe_size +
> PAGE_SIZE - 1) /
> > + PAGE_SIZE;
> > +
> > + return 0;
> > +}
> > +
> > +static int pvrdma_set_sq_size(struct pvrdma_dev *dev, struct ib_qp_cap
> *cap,
> > + enum ib_qp_type type, struct pvrdma_qp *qp) {
> > + if (cap->max_send_wr > dev->dsr->caps.max_qp_wr ||
> > + cap->max_send_sge > dev->dsr->caps.max_sge) {
> > + dev_warn(&dev->pdev->dev, "send queue size invalid\n");
> > + return -EINVAL;
> > + }
> > +
> > + qp->sq.wqe_cnt = roundup_pow_of_two(max(1U, cap-
> >max_send_wr));
> > + qp->sq.max_gs = roundup_pow_of_two(max(1U, cap-
> >max_send_sge));
> > +
> > + /* Write back */
> > + cap->max_send_wr = qp->sq.wqe_cnt;
> > + cap->max_send_sge = qp->sq.max_gs;
> > +
> > + qp->sq.wqe_size = roundup_pow_of_two(sizeof(struct
> pvrdma_sq_wqe_hdr) +
> > + sizeof(struct ib_sge) *
> > + qp->sq.max_gs);
> > + /* Note: one extra page for the header. */
> > + qp->npages_send = 1 + (qp->sq.wqe_cnt * qp->sq.wqe_size +
> > + PAGE_SIZE - 1) / PAGE_SIZE;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * pvrdma_create_qp - create queue pair
> > + * @pd: protection domain
> > + * @init_attr: queue pair attributes
> > + * @udata: user data
> > + *
> > + * @return: the ib_qp pointer on success, otherwise returns an errno.
> > + */
> > +struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
> > + struct ib_qp_init_attr *init_attr,
> > + struct ib_udata *udata)
> > +{
> > + struct pvrdma_qp *qp;
> > + struct pvrdma_dev *dev = to_vdev(pd->device);
> > + struct pvrdma_cq *send_cq, *recv_cq;
> > + union pvrdma_cmd_req req;
> > + union pvrdma_cmd_resp rsp;
> > + struct pvrdma_cmd_create_qp *cmd = &req.create_qp;
> > + struct pvrdma_cmd_create_qp_resp *resp = &rsp.create_qp_resp;
> > + struct pvrdma_create_qp ucmd;
> > + unsigned long flags;
> > + int ret;
> > +
> > + if (!atomic_add_unless(&dev->num_qps, 1, dev->dsr-
> >caps.max_qp))
> > + return ERR_PTR(-EINVAL);
>
> Suggesting EAGAIN, ENOMEM, ENOSPC but not "Invalid argument" as
> nothing is wrong with input args.
Changed this to -ENOMEM.
> > +
> > + if (init_attr->create_flags) {
> > + dev_warn(&dev->pdev->dev,
> > + "invalid create queuepair flags %#x\n",
> > + init_attr->create_flags);
> > + atomic_dec(&dev->num_qps);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + if (init_attr->qp_type != IB_QPT_RC &&
> > + init_attr->qp_type != IB_QPT_UD &&
> > + init_attr->qp_type != IB_QPT_GSI) {
> > + dev_warn(&dev->pdev->dev, "queuepair type %d not
> supported\n",
> > + init_attr->qp_type);
> > + atomic_dec(&dev->num_qps);
> > + return ERR_PTR(-EINVAL);
> > + }
>
> Can we move the two blocks above so we would not need to dec(num_qps)
> on validation failure.
> It also make sense to first validate input and just then to allocate resource.
Ok.
...
> > +static void pvrdma_free_qp(struct pvrdma_qp *qp) {
> > + struct pvrdma_dev *dev = to_vdev(qp->ibqp.device);
> > + struct pvrdma_cq *scq;
> > + struct pvrdma_cq *rcq;
> > + unsigned long flags, flags1, flags2;
> > +
> > + /* In case cq is polling */
> > + get_cqs(qp, &scq, &rcq);
> > + if ((uintptr_t)scq <= (uintptr_t)rcq) {
>
> ???
> Comment might help here :)
I updated this whole cleanup CQs path so this check will go away.
...
> > +
> > + index = pvrdma_idx(&qp->sq.ring->prod_tail, qp->sq.wqe_cnt);
> > + for (nreq = 0; wr; nreq++, wr = wr->next) {
> > + unsigned int tail;
> > +
> > + if (unlikely(!pvrdma_idx_ring_has_space(
> > + qp->sq.ring, qp->sq.wqe_cnt, &tail))) {
> > + dev_warn(&dev->pdev->dev, "send queue is
> full\n");
>
> Please consider replace it with dev_warn_ratelimited.
> (applied to all dev_warn in this function and in pvrdma_post_recv).
Ok. Will look at this.
Thanks,
Adit
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
