| Message ID | 1659335010-2-1-git-send-email-lizhijian@fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/RXE: Add send_common_ack() helper | expand |
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 >
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 >>
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 > >>
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 >>>>
在 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 >>>>>
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
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,
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(-)