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

[Yanjun Zhu]

在 2022/8/2 13:52, lizhijian@xxxxxxxxxxx 写道:
> On 01/08/2022 15:47, Zhu Yanjun wrote:
> > On Mon, Aug 1, 2022 at 3:28 PM lizhijian@xxxxxxxxxxx
> > <lizhijian@xxxxxxxxxxx> wrote:
> > > On 01/08/2022 15:11, Zhu Yanjun wrote:
> > > > On Mon, Aug 1, 2022 at 2:16 PM Li Zhijian <lizhijian@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > > ---
> > > > >     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.
>
> i tried to do a rough statistic.
>
> all functions:
> $ git grep -E '^[a-z].*\(' drivers/infiniband/sw/rxe | awk -F'(' '{print $1}' | awk '{print $NF}' | tr -d '\*' | grep -E ^[a-z]+ | wc -l
> 474
>
> without rxe_ prefix:
> git grep -E '^[a-z].*\(' drivers/infiniband/sw/rxe | awk -F'(' '{print $1}' | awk '{print $NF}' | tr -d '\*' | grep -E ^[a-z]+ | grep -v -e ^rxe | wc -l
> 199
The followings are the no rxe_ prefix functions. About 22 functions.

do_complete
retransmit_timer
next_opcode
rnr_nak_timer
find_resource
send_data_in
prepare_ack_packet
send_ack
check_keys
check_type_state
resize_finish
do_mmap_info
parent_show
post_one_recv
free_rd_atomic_resources
free_rd_atomic_resource
lookup_iova
mr_check_range
iova_to_vaddr
advance_dma_data
lookup_mr
copy_data

Zhu Yanjun

> Similarly, the mlx5 have the same situations.
> $ git grep -h -E '^[a-z].*\(' drivers/infiniband/hw/mlx5 | awk -F'(' '{print $1}' | awk '{print $NF}' | tr -d '\*' | grep -E ^[a-z]+ | wc -l
> 1083
> $ git grep -h -E '^[a-z].*\(' drivers/infiniband/hw/mlx5 | awk -F'(' '{print $1}' | awk '{print $NF}' | tr -d '\*' | grep -E ^[a-z]+ | grep -v -e ^mlx5 | wc -l
> 476
>
> TBH, i have no strong stomach to do such cleanup so far :)
>
> Thanks
> Zhijian
>
>
> > 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