Re: [PATCH for-next v5 06/10] RDMA/rxe: Move local ops to subroutine

Pearson, Robert B
2021-04-28T16:16:57Z

On 4/24/2021 11:34 PM, Zhu Yanjun wrote:
> On Fri, Apr 23, 2021 at 12:13 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>> Simplify rxe_requester() by moving the local operations
>> to a subroutine. Add an error return for illegal send WR opcode.
>> Moved next_index ahead of rxe_run_task which fixed a small bug where
>> work completions were delayed until after the next wqe which was not
>> the intended behavior.
>>
>> Signed-off-by: Bob Pearson <rpearson@hpe.com>
>> ---
>> drivers/infiniband/sw/rxe/rxe_req.c | 89 +++++++++++++++++------------
>> 1 file changed, 54 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
>> index 0d4dcd514c55..0cf97e3db29f 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_req.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
>> @@ -555,6 +555,56 @@ static void update_state(struct rxe_qp *qp, struct rxe_send_wqe *wqe,
>> jiffies + qp->qp_timeout_jiffies);
>> }
>>
>> +static int do_local_ops(struct rxe_qp *qp, struct rxe_send_wqe *wqe)
> rxe_do_local_ops if not used out of softroce.

Same issue. I am trying to be consistent with the original style (which
mostly I wrote). Static names

don't need to be elaborate.

>
>> +{
>> + u8 opcode = wqe->wr.opcode;
>> + struct rxe_dev *rxe;
>> + struct rxe_mr *mr;
>> + u32 rkey;
>> +
>> + switch (opcode) {
>> + case IB_WR_LOCAL_INV:
>> + rxe = to_rdev(qp->ibqp.device);
>> + rkey = wqe->wr.ex.invalidate_rkey;
>> + mr = rxe_pool_get_index(&rxe->mr_pool, rkey >> 8);
>> + if (!mr) {
>> + pr_err("No MR for rkey %#x\n", rkey);
>> + wqe->state = wqe_state_error;
>> + wqe->status = IB_WC_LOC_QP_OP_ERR;
>> + return -EINVAL;
>> + }
>> + mr->state = RXE_MR_STATE_FREE;
>> + rxe_drop_ref(mr);
>> + break;
>> + case IB_WR_REG_MR:
>> + mr = to_rmr(wqe->wr.wr.reg.mr);
>> +
>> + rxe_add_ref(mr);
>> + mr->state = RXE_MR_STATE_VALID;
>> + mr->access = wqe->wr.wr.reg.access;
>> + mr->ibmr.lkey = wqe->wr.wr.reg.key;
>> + mr->ibmr.rkey = wqe->wr.wr.reg.key;
>> + mr->iova = wqe->wr.wr.reg.mr->iova;
>> + rxe_drop_ref(mr);
>> + break;
>> + default:
>> + pr_err("Unexpected send wqe opcode %d\n", opcode);
>> + wqe->state = wqe_state_error;
>> + wqe->status = IB_WC_LOC_QP_OP_ERR;
>> + return -EINVAL;
>> + }
>> +
>> + wqe->state = wqe_state_done;
>> + wqe->status = IB_WC_SUCCESS;
>> + qp->req.wqe_index = next_index(qp->sq.queue, qp->req.wqe_index);
>> +
>> + if ((wqe->wr.send_flags & IB_SEND_SIGNALED) ||
>> + qp->sq_sig_type == IB_SIGNAL_ALL_WR)
>> + rxe_run_task(&qp->comp.task, 1);
>> +
>> + return 0;
>> +}
>> +
>> int rxe_requester(void *arg)
>> {
>> struct rxe_qp *qp = (struct rxe_qp *)arg;
>> @@ -594,42 +644,11 @@ int rxe_requester(void *arg)
>> goto exit;
>>
>> if (wqe->mask & WR_LOCAL_OP_MASK) {
>> - if (wqe->wr.opcode == IB_WR_LOCAL_INV) {
>> - struct rxe_dev *rxe = to_rdev(qp->ibqp.device);
>> - struct rxe_mr *rmr;
>> -
>> - rmr = rxe_pool_get_index(&rxe->mr_pool,
>> - wqe->wr.ex.invalidate_rkey >> 8);
>> - if (!rmr) {
>> - pr_err("No mr for key %#x\n",
>> - wqe->wr.ex.invalidate_rkey);
>> - wqe->state = wqe_state_error;
>> - wqe->status = IB_WC_MW_BIND_ERR;
>> - goto exit;
>> - }
>> - rmr->state = RXE_MR_STATE_FREE;
>> - rxe_drop_ref(rmr);
>> - wqe->state = wqe_state_done;
>> - wqe->status = IB_WC_SUCCESS;
>> - } else if (wqe->wr.opcode == IB_WR_REG_MR) {
>> - struct rxe_mr *rmr = to_rmr(wqe->wr.wr.reg.mr);
>> -
>> - rmr->state = RXE_MR_STATE_VALID;
>> - rmr->access = wqe->wr.wr.reg.access;
>> - rmr->ibmr.lkey = wqe->wr.wr.reg.key;
>> - rmr->ibmr.rkey = wqe->wr.wr.reg.key;
>> - rmr->iova = wqe->wr.wr.reg.mr->iova;
>> - wqe->state = wqe_state_done;
>> - wqe->status = IB_WC_SUCCESS;
>> - } else {
>> + ret = do_local_ops(qp, wqe);
>> + if (unlikely(ret))
>> goto exit;
>> - }
>> - if ((wqe->wr.send_flags & IB_SEND_SIGNALED) ||
>> - qp->sq_sig_type == IB_SIGNAL_ALL_WR)
>> - rxe_run_task(&qp->comp.task, 1);
>> - qp->req.wqe_index = next_index(qp->sq.queue,
>> - qp->req.wqe_index);
>> - goto next_wqe;
>> + else
>> + goto next_wqe;
>> }
>>
>> if (unlikely(qp_type(qp) == IB_QPT_RC &&
>> --
>> 2.27.0
>>