[PATCH 0/3] RDMA/rxe misc fixes

[PATCH 0/3] RDMA/rxe misc fixes

[zhenwei pi]

Hi,

Please review these minor fixes of RXE. Thanks!

zhenwei pi (3):
  RDMA/rxe: Use sizeof instead of hard code number
  RDMA/rxe: Typo fix
  RDMA/rxe: Fix __bth_set_resv6a

 drivers/infiniband/sw/rxe/rxe_hdr.h  | 2 +-
 drivers/infiniband/sw/rxe/rxe_resp.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/3] RDMA/rxe: Use sizeof instead of hard code number

[zhenwei pi]

Use 'sizeof(union rdma_network_hdr)' instead of hard code GRH length
for GSI and UD.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/infiniband/sw/rxe/rxe_resp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index 6596a85723c9..bf8f4bc8c5c8 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -351,7 +351,7 @@ static enum resp_states rxe_resp_check_length(struct rxe_qp *qp,
 
 		for (i = 0; i < qp->resp.wqe->dma.num_sge; i++)
 			recv_buffer_len += qp->resp.wqe->dma.sge[i].length;
-		if (payload + 40 > recv_buffer_len) {
+		if (payload + sizeof(union rdma_network_hdr) > recv_buffer_len) {
 			rxe_dbg_qp(qp, "The receive buffer is too small for this UD packet.\n");
 			return RESPST_ERR_LENGTH;
 		}
-- 
2.34.1

Re: [PATCH 1/3] RDMA/rxe: Use sizeof instead of hard code number

[Zhu Yanjun]

在 2024/8/22 14:52, zhenwei pi 写道:
> Use 'sizeof(union rdma_network_hdr)' instead of hard code GRH length
> for GSI and UD.
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>   drivers/infiniband/sw/rxe/rxe_resp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index 6596a85723c9..bf8f4bc8c5c8 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -351,7 +351,7 @@ static enum resp_states rxe_resp_check_length(struct rxe_qp *qp,
>   
>   		for (i = 0; i < qp->resp.wqe->dma.num_sge; i++)
>   			recv_buffer_len += qp->resp.wqe->dma.sge[i].length;
> -		if (payload + 40 > recv_buffer_len) {
> +		if (payload + sizeof(union rdma_network_hdr) > recv_buffer_len) {

The definition of union rdma_network_hdr is as below

  797 union rdma_network_hdr {
  798         struct ib_grh ibgrh;
  799         struct {
  800                 /* The IB spec states that if it's IPv4, the header
  801                  * is located in the last 20 bytes of the header.
  802                  */
  803                 u8              reserved[20];
  804                 struct iphdr    roce4grh;
  805         };
  806 };

The length is 40 byte.

But in this,

sizeof(struct iphdr) (20) + sizeof(struct udphdr) (8) + sizeof(struct 
rxe_bth) (12) = 40

Not sure if we should use sizeof(union rdma_network_hdr) or 
(sizeof(struct iphdr) + sizeof(struct udphdr) + sizeof(struct rxe_bth)) 
in this place.

Best Regards,
Zhu Yanjun


>   			rxe_dbg_qp(qp, "The receive buffer is too small for this UD packet.\n");
>   			return RESPST_ERR_LENGTH;
>   		}

Re: [PATCH 1/3] RDMA/rxe: Use sizeof instead of hard code number

[Jason Gunthorpe]

On Thu, Aug 22, 2024 at 07:59:32PM +0800, Zhu Yanjun wrote:
> 在 2024/8/22 14:52, zhenwei pi 写道:
> > Use 'sizeof(union rdma_network_hdr)' instead of hard code GRH length
> > for GSI and UD.
> > 
> > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > ---
> >   drivers/infiniband/sw/rxe/rxe_resp.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> > index 6596a85723c9..bf8f4bc8c5c8 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> > @@ -351,7 +351,7 @@ static enum resp_states rxe_resp_check_length(struct rxe_qp *qp,
> >   		for (i = 0; i < qp->resp.wqe->dma.num_sge; i++)
> >   			recv_buffer_len += qp->resp.wqe->dma.sge[i].length;
> > -		if (payload + 40 > recv_buffer_len) {
> > +		if (payload + sizeof(union rdma_network_hdr) > recv_buffer_len) {
> 
> The definition of union rdma_network_hdr is as below
> 
>  797 union rdma_network_hdr {
>  798         struct ib_grh ibgrh;
>  799         struct {
>  800                 /* The IB spec states that if it's IPv4, the header
>  801                  * is located in the last 20 bytes of the header.
>  802                  */
>  803                 u8              reserved[20];
>  804                 struct iphdr    roce4grh;
>  805         };
>  806 };
> 
> The length is 40 byte.

This looks like the right struct to me if this is talking about the
special 40 byte blob that is placed in front of UD verbs completions.

Jason

Re: [PATCH 1/3] RDMA/rxe: Use sizeof instead of hard code number

[Zhu Yanjun]

在 2024/8/23 10:30, zhenwei pi 写道:
>
> On 8/22/24 20:36, Jason Gunthorpe wrote:
> > On Thu, Aug 22, 2024 at 07:59:32PM +0800, Zhu Yanjun wrote:
> >> 在 2024/8/22 14:52, zhenwei pi 写道:
> >>> Use 'sizeof(union rdma_network_hdr)' instead of hard code GRH length
> >>> for GSI and UD.
> >>>
> >>> Signed-off-by: zhenwei pi
> >>> ---
> >>> drivers/infiniband/sw/rxe/rxe_resp.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c 
> b/drivers/infiniband/sw/rxe/rxe_resp.c
> >>> index 6596a85723c9..bf8f4bc8c5c8 100644
> >>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> >>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> >>> @@ -351,7 +351,7 @@ static enum resp_states 
> rxe_resp_check_length(struct rxe_qp *qp,
> >>> for (i = 0; i < qp->resp.wqe->dma.num_sge; i++)
> >>> recv_buffer_len += qp->resp.wqe->dma.sge[i].length;
> >>> - if (payload + 40 > recv_buffer_len) {
> >>> + if (payload + sizeof(union rdma_network_hdr) > recv_buffer_len) {
> >>
> >> The definition of union rdma_network_hdr is as below
> >>
> >> 797 union rdma_network_hdr {
> >> 798 struct ib_grh ibgrh;
> >> 799 struct {
> >> 800 /* The IB spec states that if it's IPv4, the header
> >> 801 * is located in the last 20 bytes of the header.
> >> 802 */
> >> 803 u8 reserved[20];
> >> 804 struct iphdr roce4grh;
> >> 805 };
> >> 806 };
> >>
> >> The length is 40 byte.
> >
> > This looks like the right struct to me if this is talking about the
> > special 40 byte blob that is placed in front of UD verbs completions.
> >
> > Jason
>
> Yes, this is the front part(40 bytes) of UD/GSI verbs completion.
>
When running, you can print the value of the front part (40 bytes) of 
UD/GSI to confirm that these 40 bytes are the union rdma_network_hdr.

If these 40 bytes are the union rdma_network_hdr,

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Best Regards,

Zhu Yanjun

>
> -- 
> zhenwei pi
>
-- 
Best Regards,
Yanjun.Zhu

Re: Re: [PATCH 1/3] RDMA/rxe: Use sizeof instead of hard code number

[zhenwei pi]

On 8/23/24 13:56, Zhu Yanjun wrote:
> 
> 在 2024/8/23 10:30, zhenwei pi 写道:
>>
>> On 8/22/24 20:36, Jason Gunthorpe wrote:
>> > On Thu, Aug 22, 2024 at 07:59:32PM +0800, Zhu Yanjun wrote:
>> >> 在 2024/8/22 14:52, zhenwei pi 写道:
>> >>> Use 'sizeof(union rdma_network_hdr)' instead of hard code GRH length
>> >>> for GSI and UD.
>> >>>
>> >>> Signed-off-by: zhenwei pi
>> >>> ---
>> >>> drivers/infiniband/sw/rxe/rxe_resp.c | 2 +-
>> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/ 
>> infiniband/sw/rxe/rxe_resp.c
>> >>> index 6596a85723c9..bf8f4bc8c5c8 100644
>> >>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>> >>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>> >>> @@ -351,7 +351,7 @@ static enum resp_states 
>> rxe_resp_check_length(struct rxe_qp *qp,
>> >>> for (i = 0; i < qp->resp.wqe->dma.num_sge; i++)
>> >>> recv_buffer_len += qp->resp.wqe->dma.sge[i].length;
>> >>> - if (payload + 40 > recv_buffer_len) {
>> >>> + if (payload + sizeof(union rdma_network_hdr) > recv_buffer_len) {
>> >>
>> >> The definition of union rdma_network_hdr is as below
>> >>
>> >> 797 union rdma_network_hdr {
>> >> 798 struct ib_grh ibgrh;
>> >> 799 struct {
>> >> 800 /* The IB spec states that if it's IPv4, the header
>> >> 801 * is located in the last 20 bytes of the header.
>> >> 802 */
>> >> 803 u8 reserved[20];
>> >> 804 struct iphdr roce4grh;
>> >> 805 };
>> >> 806 };
>> >>
>> >> The length is 40 byte.
>> >
>> > This looks like the right struct to me if this is talking about the
>> > special 40 byte blob that is placed in front of UD verbs completions.
>> >
>> > Jason
>>
>> Yes, this is the front part(40 bytes) of UD/GSI verbs completion.
>>
> When running, you can print the value of the front part (40 bytes) of 
> UD/GSI to confirm that these 40 bytes are the union rdma_network_hdr.
> 
> If these 40 bytes are the union rdma_network_hdr,
> 
> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Best Regards,
> 
> Zhu Yanjun
> 

Hi Yanjun,

I test on mlx-cx5 by command: ibv_ud_pingpong -d mlx5_2 -g 3 -r 1 -n 1

Dump the front 64 bytes:
00000000000000000000000000000000000000004502043436f340000111448c
0a0b73120a0b73107b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b

Byte[0, 20) (rdma_network_hdr::reserved[20]) are filled by zero, 
byte[20,40) (rdma_network_hdr::roce4grh) are filled by IPv4 header.

>>
>> -- 
>> zhenwei pi
>>

Re: [PATCH 1/3] RDMA/rxe: Use sizeof instead of hard code number

[Zhu Yanjun]

在 2024/8/23 16:23, zhenwei pi 写道:
>
>
> On 8/23/24 13:56, Zhu Yanjun wrote:
>>
>> 在 2024/8/23 10:30, zhenwei pi 写道:
>>>
>>> On 8/22/24 20:36, Jason Gunthorpe wrote:
>>> > On Thu, Aug 22, 2024 at 07:59:32PM +0800, Zhu Yanjun wrote:
>>> >> 在 2024/8/22 14:52, zhenwei pi 写道:
>>> >>> Use 'sizeof(union rdma_network_hdr)' instead of hard code GRH 
>>> length
>>> >>> for GSI and UD.
>>> >>>
>>> >>> Signed-off-by: zhenwei pi
>>> >>> ---
>>> >>> drivers/infiniband/sw/rxe/rxe_resp.c | 2 +-
>>> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >>>
>>> >>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/ 
>>> infiniband/sw/rxe/rxe_resp.c
>>> >>> index 6596a85723c9..bf8f4bc8c5c8 100644
>>> >>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>>> >>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>>> >>> @@ -351,7 +351,7 @@ static enum resp_states 
>>> rxe_resp_check_length(struct rxe_qp *qp,
>>> >>> for (i = 0; i < qp->resp.wqe->dma.num_sge; i++)
>>> >>> recv_buffer_len += qp->resp.wqe->dma.sge[i].length;
>>> >>> - if (payload + 40 > recv_buffer_len) {
>>> >>> + if (payload + sizeof(union rdma_network_hdr) > recv_buffer_len) {
>>> >>
>>> >> The definition of union rdma_network_hdr is as below
>>> >>
>>> >> 797 union rdma_network_hdr {
>>> >> 798 struct ib_grh ibgrh;
>>> >> 799 struct {
>>> >> 800 /* The IB spec states that if it's IPv4, the header
>>> >> 801 * is located in the last 20 bytes of the header.
>>> >> 802 */
>>> >> 803 u8 reserved[20];
>>> >> 804 struct iphdr roce4grh;
>>> >> 805 };
>>> >> 806 };
>>> >>
>>> >> The length is 40 byte.
>>> >
>>> > This looks like the right struct to me if this is talking about the
>>> > special 40 byte blob that is placed in front of UD verbs completions.
>>> >
>>> > Jason
>>>
>>> Yes, this is the front part(40 bytes) of UD/GSI verbs completion.
>>>
>> When running, you can print the value of the front part (40 bytes) of 
>> UD/GSI to confirm that these 40 bytes are the union rdma_network_hdr.
>>
>> If these 40 bytes are the union rdma_network_hdr,
>>
>> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
>> Best Regards,
>>
>> Zhu Yanjun
>>
>
> Hi Yanjun,
>
> I test on mlx-cx5 by command: ibv_ud_pingpong -d mlx5_2 -g 3 -r 1 -n 1
>
> Dump the front 64 bytes:
> 00000000000000000000000000000000000000004502043436f340000111448c
> 0a0b73120a0b73107b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b7b
>
> Byte[0, 20) (rdma_network_hdr::reserved[20]) are filled by zero, 
> byte[20,40) (rdma_network_hdr::roce4grh) are filled by IPv4 header.

Got it. Thanks a lot.

I am fine with your commit.

Best Regards,

Zhu Yanjun

>
>>>
>>> -- 
>>> zhenwei pi
>>>
>
-- 
Best Regards,
Yanjun.Zhu

[PATCH 2/3] RDMA/rxe: Typo fix

[zhenwei pi]

Fix 'rmda' into 'RDMA'.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/infiniband/sw/rxe/rxe_resp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index bf8f4bc8c5c8..c11ab280551a 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -341,7 +341,7 @@ static enum resp_states rxe_resp_check_length(struct rxe_qp *qp,
 	/*
 	 * See IBA C9-92
 	 * For UD QPs we only check if the packet will fit in the
-	 * receive buffer later. For rmda operations additional
+	 * receive buffer later. For RDMA operations additional
 	 * length checks are performed in check_rkey.
 	 */
 	if ((qp_type(qp) == IB_QPT_GSI) || (qp_type(qp) == IB_QPT_UD)) {
-- 
2.34.1

Re: [PATCH 2/3] RDMA/rxe: Typo fix

[Zhu Yanjun]

在 2024/8/22 14:52, zhenwei pi 写道:
> Fix 'rmda' into 'RDMA'.
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Thanks.
Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun

> ---
>   drivers/infiniband/sw/rxe/rxe_resp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index bf8f4bc8c5c8..c11ab280551a 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -341,7 +341,7 @@ static enum resp_states rxe_resp_check_length(struct rxe_qp *qp,
>   	/*
>   	 * See IBA C9-92
>   	 * For UD QPs we only check if the packet will fit in the
> -	 * receive buffer later. For rmda operations additional
> +	 * receive buffer later. For RDMA operations additional
>   	 * length checks are performed in check_rkey.
>   	 */
>   	if ((qp_type(qp) == IB_QPT_GSI) || (qp_type(qp) == IB_QPT_UD)) {

[PATCH 3/3] RDMA/rxe: Fix __bth_set_resv6a

[zhenwei pi]

__bth_set_resv6a is used to clear BIT [24, 29] of rxe_bth::qpn, the
wrong expression leads other BITs into 1.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/infiniband/sw/rxe/rxe_hdr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_hdr.h b/drivers/infiniband/sw/rxe/rxe_hdr.h
index 46f82b27fcd2..1f0322491d8c 100644
--- a/drivers/infiniband/sw/rxe/rxe_hdr.h
+++ b/drivers/infiniband/sw/rxe/rxe_hdr.h
@@ -234,7 +234,7 @@ static inline void __bth_set_resv6a(void *arg)
 {
 	struct rxe_bth *bth = arg;
 
-	bth->qpn = cpu_to_be32(~BTH_RESV6A_MASK);
+	bth->qpn &= cpu_to_be32(~BTH_RESV6A_MASK);
 }
 
 static inline int __bth_ack(void *arg)
-- 
2.34.1

Re: [PATCH 3/3] RDMA/rxe: Fix __bth_set_resv6a

[Zhu Yanjun]

在 2024/8/22 14:52, zhenwei pi 写道:
> __bth_set_resv6a is used to clear BIT [24, 29] of rxe_bth::qpn, the
> wrong expression leads other BITs into 1.

 From the spec
"
9.2.11 RESERVE 6(RESV6) - 6 BITS
Reserved (variant) - 6 bits. Transmitted as 0, ignored on receive. This 
field
is not included in the invariant CRC.
C9-6: When generating a packet, the sender shall set the Resv6, F/Res1
and B/Res1 fields to zero. In general, the receiver shall ignore the 
reserved
fields.
"
I agree with you.
Thanks a lot.

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>   drivers/infiniband/sw/rxe/rxe_hdr.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_hdr.h b/drivers/infiniband/sw/rxe/rxe_hdr.h
> index 46f82b27fcd2..1f0322491d8c 100644
> --- a/drivers/infiniband/sw/rxe/rxe_hdr.h
> +++ b/drivers/infiniband/sw/rxe/rxe_hdr.h
> @@ -234,7 +234,7 @@ static inline void __bth_set_resv6a(void *arg)
>   {
>   	struct rxe_bth *bth = arg;
>   
> -	bth->qpn = cpu_to_be32(~BTH_RESV6A_MASK);
> +	bth->qpn &= cpu_to_be32(~BTH_RESV6A_MASK);
>   }
>   
>   static inline int __bth_ack(void *arg)

Re: [PATCH 0/3] RDMA/rxe misc fixes

[Jason Gunthorpe]

On Thu, Aug 22, 2024 at 02:52:20PM +0800, zhenwei pi wrote:
> Hi,
> 
> Please review these minor fixes of RXE. Thanks!
> 
> zhenwei pi (3):
>   RDMA/rxe: Use sizeof instead of hard code number
>   RDMA/rxe: Typo fix
>   RDMA/rxe: Fix __bth_set_resv6a

Applied to for-next, thanks

Jason