* [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).