| Message ID | 20240822065223.1117056-2-pizhenwei@bytedance.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rxe misc fixes | expand |
在 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; > }
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
在 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 >
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 >>
在 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 >>> >
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; }
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(-)