Re: [PATCH] RDMA/RXE: Add send_common_ack() helper

[Zhu Yanjun <zyjzyj2000@gmail.com>]

On Mon, Aug 1, 2022 at 3:28 PM lizhijian@fujitsu.com
<lizhijian@fujitsu.com> wrote:
>
>
>
> On 01/08/2022 15:11, Zhu Yanjun wrote:
> > On Mon, Aug 1, 2022 at 2:16 PM Li Zhijian <lizhijian@fujitsu.com> wrote:
> >> Most code in send_ack() and send_atomic_ack() are duplicate, move them
> >> to a new helper send_common_ack().
> >>
> >> In newer IBA SPEC, some opcodes require acknowledge with a zero-length read
> >> response, with this new helper, we can easily implement it later.
> >>
> >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> >> ---
> >>   drivers/infiniband/sw/rxe/rxe_resp.c | 43 ++++++++++++++----------------------
> >>   1 file changed, 17 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> >> index b36ec5c4d5e0..4c398fa220fa 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> >> @@ -1028,50 +1028,41 @@ static enum resp_states do_complete(struct rxe_qp *qp,
> >>                  return RESPST_CLEANUP;
> >>   }
> >>
> >> -static int send_ack(struct rxe_qp *qp, u8 syndrome, u32 psn)
> >> +
> >> +static int send_common_ack(struct rxe_qp *qp, u8 syndrome, u32 psn,
> > The function is better with rxe_send_common_ack?
> I'm not clear the exact rule about the naming prefix. if it has, please let me know :)
>
> IMHO, if a function is either a public API(export function) or a callback to a upper layer,  it's a good idea to a fixed prefix.
> Instead, if they are just static, no prefix is not too bad.

When debug, a rxe_ prefix can help us filter the functions whatever
the function static or public.

>
> BTW, current RXE are mixing the two rules, it should be another standalone patch to do the cleanup if needed.

Yes. Please make this standalone patch to complete this.

Thanks and Regards,
Zhu Yanjun

>
> Thanks
> Zhijian
>
>
> > So when debug, rxe_ prefix can help us.
> >
> >> +                                 int opcode, const char *msg)
> >>   {
> >> -       int err = 0;
> >> +       int err;
> >>          struct rxe_pkt_info ack_pkt;
> >>          struct sk_buff *skb;
> >>
> >> -       skb = prepare_ack_packet(qp, &ack_pkt, IB_OPCODE_RC_ACKNOWLEDGE,
> >> -                                0, psn, syndrome);
> >> -       if (!skb) {
> >> -               err = -ENOMEM;
> >> -               goto err1;
> >> -       }
> >> +       skb = prepare_ack_packet(qp, &ack_pkt, opcode, 0, psn, syndrome);
> >> +       if (!skb)
> >> +               return -ENOMEM;
> >>
> >>          err = rxe_xmit_packet(qp, &ack_pkt, skb);
> >>          if (err)
> >> -               pr_err_ratelimited("Failed sending ack\n");
> >> +               pr_err_ratelimited("Failed sending %s\n", msg);
> >>
> >> -err1:
> >>          return err;
> >>   }
> >>
> >> -static int send_atomic_ack(struct rxe_qp *qp, u8 syndrome, u32 psn)
> >> +static int send_ack(struct rxe_qp *qp, u8 syndrome, u32 psn)
> > rxe_send_ack
> >
> >>   {
> >> -       int err = 0;
> >> -       struct rxe_pkt_info ack_pkt;
> >> -       struct sk_buff *skb;
> >> -
> >> -       skb = prepare_ack_packet(qp, &ack_pkt, IB_OPCODE_RC_ATOMIC_ACKNOWLEDGE,
> >> -                                0, psn, syndrome);
> >> -       if (!skb) {
> >> -               err = -ENOMEM;
> >> -               goto out;
> >> -       }
> >> +       return send_common_ack(qp, syndrome, psn,
> >> +                       IB_OPCODE_RC_ACKNOWLEDGE, "ACK");
> >> +}
> >>
> >> -       err = rxe_xmit_packet(qp, &ack_pkt, skb);
> >> -       if (err)
> >> -               pr_err_ratelimited("Failed sending atomic ack\n");
> >> +static int send_atomic_ack(struct rxe_qp *qp, u8 syndrome, u32 psn)
> > rxe_send_atomic_ack
> >
> > Thanks and Regards,
> > Zhu Yanjun
> >> +{
> >> +       int ret = send_common_ack(qp, syndrome, psn,
> >> +                       IB_OPCODE_RC_ATOMIC_ACKNOWLEDGE, "ATOMIC ACK");
> >>
> >>          /* have to clear this since it is used to trigger
> >>           * long read replies
> >>           */
> >>          qp->resp.res = NULL;
> >> -out:
> >> -       return err;
> >> +       return ret;
> >>   }
> >>
> >>   static enum resp_states acknowledge(struct rxe_qp *qp,
> >> --
> >> 1.8.3.1
> >>