* [PATCH] RDMA/RXE: Add send_common_ack() helper @ 2022-08-01 6:23 Li Zhijian 2022-08-01 7:11 ` Zhu Yanjun 2022-09-26 17:28 ` Jason Gunthorpe 0 siblings, 2 replies; 7+ messages in thread From: Li Zhijian @ 2022-08-01 6:23 UTC (permalink / raw) To: Zhu Yanjun, Jason Gunthorpe, Leon Romanovsky, linux-rdma Cc: yangx.jy, Bob Pearson, linux-kernel, Li Zhijian 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, + 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) { - 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) +{ + 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/RXE: Add send_common_ack() helper 2022-08-01 6:23 [PATCH] RDMA/RXE: Add send_common_ack() helper Li Zhijian @ 2022-08-01 7:11 ` Zhu Yanjun 2022-08-01 7:28 ` lizhijian 2022-09-26 17:28 ` Jason Gunthorpe 1 sibling, 1 reply; 7+ messages in thread From: Zhu Yanjun @ 2022-08-01 7:11 UTC (permalink / raw) To: Li Zhijian Cc: Jason Gunthorpe, Leon Romanovsky, RDMA mailing list, Xiao Yang, Bob Pearson, LKML 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? 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/RXE: Add send_common_ack() helper 2022-08-01 7:11 ` Zhu Yanjun @ 2022-08-01 7:28 ` lizhijian 2022-08-01 7:47 ` Zhu Yanjun 0 siblings, 1 reply; 7+ messages in thread From: lizhijian @ 2022-08-01 7:28 UTC (permalink / raw) To: Zhu Yanjun Cc: Jason Gunthorpe, Leon Romanovsky, RDMA mailing list, yangx.jy, Bob Pearson, LKML 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. BTW, current RXE are mixing the two rules, it should be another standalone patch to do the cleanup if needed. 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 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/RXE: Add send_common_ack() helper 2022-08-01 7:28 ` lizhijian @ 2022-08-01 7:47 ` Zhu Yanjun 2022-08-02 5:52 ` lizhijian 0 siblings, 1 reply; 7+ messages in thread From: Zhu Yanjun @ 2022-08-01 7:47 UTC (permalink / raw) To: lizhijian Cc: Jason Gunthorpe, Leon Romanovsky, RDMA mailing list, yangx.jy, Bob Pearson, LKML 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 > >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/RXE: Add send_common_ack() helper 2022-08-01 7:47 ` Zhu Yanjun @ 2022-08-02 5:52 ` lizhijian 2022-08-02 13:17 ` Yanjun Zhu 0 siblings, 1 reply; 7+ messages in thread From: lizhijian @ 2022-08-02 5:52 UTC (permalink / raw) To: Zhu Yanjun Cc: Jason Gunthorpe, Leon Romanovsky, RDMA mailing list, yangx.jy, Bob Pearson, LKML On 01/08/2022 15:47, Zhu Yanjun wrote: > 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. 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 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 >>>> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/RXE: Add send_common_ack() helper 2022-08-02 5:52 ` lizhijian @ 2022-08-02 13:17 ` Yanjun Zhu 0 siblings, 0 replies; 7+ messages in thread From: Yanjun Zhu @ 2022-08-02 13:17 UTC (permalink / raw) To: lizhijian, Zhu Yanjun Cc: Jason Gunthorpe, Leon Romanovsky, RDMA mailing list, yangx.jy, Bob Pearson, LKML 在 2022/8/2 13:52, lizhijian@fujitsu.com 写道: > > > On 01/08/2022 15:47, Zhu Yanjun wrote: >> 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. > > 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 >>>>> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/RXE: Add send_common_ack() helper 2022-08-01 6:23 [PATCH] RDMA/RXE: Add send_common_ack() helper Li Zhijian 2022-08-01 7:11 ` Zhu Yanjun @ 2022-09-26 17:28 ` Jason Gunthorpe 1 sibling, 0 replies; 7+ messages in thread From: Jason Gunthorpe @ 2022-09-26 17:28 UTC (permalink / raw) To: Li Zhijian Cc: Zhu Yanjun, Leon Romanovsky, linux-rdma, yangx.jy, Bob Pearson, linux-kernel On Mon, Aug 01, 2022 at 06:23:30AM +0000, Li Zhijian 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(-) Applied to for-next, thanks If someone wants to standardize on rxe_ prefixes in the driver please send that as a series Jason ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-09-26 17:53 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-01 6:23 [PATCH] RDMA/RXE: Add send_common_ack() helper Li Zhijian 2022-08-01 7:11 ` Zhu Yanjun 2022-08-01 7:28 ` lizhijian 2022-08-01 7:47 ` Zhu Yanjun 2022-08-02 5:52 ` lizhijian 2022-08-02 13:17 ` Yanjun Zhu 2022-09-26 17:28 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).