On Tue, 2018-11-06 at 16:24 -0500, Doug Ledford wrote:
> On Mon, 2018-10-29 at 10:16 +0200, Sasha Kotchubievsky wrote:
> > I think, wr_id and status should be valid in existing implementation.
> > struct rxe_cqe {
> > union {
> > struct ib_wc ibwc;
> > struct ib_uverbs_wc uibwc;
> > };
> > };
> > In both cases wr_id and status are in the same places
> > So lines
> > wc->wr_id = wqe->wr_id;
>
> The original bug report clearly called out wr_id as being invalid in
> their specific scenario.
I thought it was, but only qp_num was actually invalid. My interpretation of
wr_id was dependent on an earlier use of qp_num and I didn't realize that was
the fundamental problem.
>
> > wc->status = qp->resp.status
> > set the valid values for rxe too.
> >
> > But I agree, that at the first glance, the code looks invalid and some
> > refactoring can be done.
>
> Personally, I like Sagi's patch. It's clear and unambiguous and you
> don't need to understand that there is a union in play making things
> work. And the original code obviously got the qp setting for user space
> wrong, period. You can argue that it doesn't need to provide the qpnum
> to user space, but the original code set it to the internal qp address
> for both user and kernel space and needs to be fixed.
The man page for ibv_poll_cq says qp_num should be valid. I understand the verbs
specification may indicate otherwise, but given what the man page says and how
easy it is to make qp_num valid, I am very glad to see that this is the approach
being taken. That said, I've since reworked my code to avoid needing the qp_num
on the error path and everything is working both with and without this patch.
Thanks for the help everyone.
>
> I'm inclined to take the patch, especially since it has verified test
> results to go along with it. So, applied to for-next, please post any
> fixups as incremental patches.
>
> > Regards
> > Sasha
> >
> > On 25/10/2018 22:40, Sagi Grimberg wrote:
> > > Error completions must still contain a valid wr_id and
> > > qp_num such that the consumer can rely on. Correctly
> > > fill these fields in receive error completions.
> > >
> > > Reported-by: Walker Benjamin <benjamin.walker@xxxxxxxxx>
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Sagi Grimberg <sagi@xxxxxxxxxxx>
> > > ---
> > > drivers/infiniband/sw/rxe/rxe_resp.c | 13 +++++++++----
> > > 1 file changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c
> > > b/drivers/infiniband/sw/rxe/rxe_resp.c
> > > index aa5833318372..3438f0653df0 100644
> > > --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> > > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> > > @@ -841,11 +841,16 @@ static enum resp_states do_complete(struct rxe_qp
> > > *qp,
> > >
> > > memset(&cqe, 0, sizeof(cqe));
> > >
> > > - wc->wr_id = wqe->wr_id;
> > > - wc->status = qp->resp.status;
> > > - wc->qp = &qp->ibqp;
> > > + if (qp->rcq->is_user) {
> > > + uwc->status = qp->resp.status;
> > > + uwc->qp_num = qp->ibqp.qp_num;
> > > + uwc->wr_id = wqe->wr_id;
> > > + } else {
> > > + wc->status = qp->resp.status;
> > > + wc->qp = &qp->ibqp;
> > > + wc->wr_id = wqe->wr_id;
> > > + }
> > >
> > > - /* fields after status are not required for errors */
> > > if (wc->status == IB_WC_SUCCESS) {
> > > wc->opcode = (pkt->mask & RXE_IMMDT_MASK &&
> > > pkt->mask & RXE_WRITE_MASK) ?
> > >
>
>
