diff mbox series

[5/5] RDMA/rxe: Remove the redundant randomization for UDP source port

Message ID 20220105221237.2659462-6-yanjun.zhu@linux.dev (mailing list archive)
State Superseded
Headers show
Series Generate UDP src port with flow label or lqpn/rqpn | expand

Commit Message

Zhu Yanjun Jan. 5, 2022, 10:12 p.m. UTC 
From: Zhu Yanjun <yanjun.zhu@linux.dev>

Since the UDP source port is modified in rxe_modify_qp, the randomization
for UDP source port is redundant in this function. So remove it.

Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
---
 drivers/infiniband/sw/rxe/rxe_qp.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Comments

Leon Romanovsky Jan. 5, 2022, 7:49 a.m. UTC | #1 
On Wed, Jan 05, 2022 at 05:12:37PM -0500, yanjun.zhu@linux.dev wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Since the UDP source port is modified in rxe_modify_qp, the randomization
> for UDP source port is redundant in this function. So remove it.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/infiniband/sw/rxe/rxe_qp.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
> index 54b8711321c1..84d6ffe7350a 100644
> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
> @@ -210,15 +210,9 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>  		return err;
>  	qp->sk->sk->sk_user_data = qp;
>  
> -	/* pick a source UDP port number for this QP based on
> -	 * the source QPN. this spreads traffic for different QPs
> -	 * across different NIC RX queues (while using a single
> -	 * flow for a given QP to maintain packet order).
> -	 * the port number must be in the Dynamic Ports range
> -	 * (0xc000 - 0xffff).
> +	/* Source UDP port number for this QP is modified in rxe_qp_modify.
>  	 */

This makes me wonder why do we set this src_port here?
Are we using this field before modify QP?

Thanks

> -	qp->src_port = RXE_ROCE_V2_SPORT +
> -		(hash_32_generic(qp_num(qp), 14) & 0x3fff);
> +	qp->src_port		= RXE_ROCE_V2_SPORT;
>  	qp->sq.max_wr		= init->cap.max_send_wr;
>  
>  	/* These caps are limited by rxe_qp_chk_cap() done by the caller */
> -- 
> 2.27.0
>
Zhu Yanjun Jan. 5, 2022, 8:42 a.m. UTC | #2 
January 5, 2022 3:49 PM, "Leon Romanovsky" <leon@kernel.org> wrote:

> On Wed, Jan 05, 2022 at 05:12:37PM -0500, yanjun.zhu@linux.dev wrote:
> 
>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>> 
>> Since the UDP source port is modified in rxe_modify_qp, the randomization
>> for UDP source port is redundant in this function. So remove it.
>> 
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>> ---
>> drivers/infiniband/sw/rxe/rxe_qp.c | 10 ++--------
>> 1 file changed, 2 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
>> index 54b8711321c1..84d6ffe7350a 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
>> @@ -210,15 +210,9 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>> return err;
>> qp->sk->sk->sk_user_data = qp;
>> 
>> - /* pick a source UDP port number for this QP based on
>> - * the source QPN. this spreads traffic for different QPs
>> - * across different NIC RX queues (while using a single
>> - * flow for a given QP to maintain packet order).
>> - * the port number must be in the Dynamic Ports range
>> - * (0xc000 - 0xffff).
>> + /* Source UDP port number for this QP is modified in rxe_qp_modify.
>> */
> 
> This makes me wonder why do we set this src_port here?
> Are we using this field before modify QP?

The commit d3c04a3a6870 ("IB/rxe: vary the source udp port for receive scaling") sets this src_port here.

The advantage of setting src_port here is: before rxe_modify_qp, the src port is randomized, not 0xc000.
So after/before rxe_modify_qp, the src port is the same value.

If the src port is changed in rxe_modify_qp, before rxe_modify_qp, the src port is 0xc000, after rxe_modify_qp,
the src port is randomized, for example, src port is 0xF043.

So when the new method is adopted, I removed this.

Zhu Yanjun

> 
> Thanks
> 
>> - qp->src_port = RXE_ROCE_V2_SPORT +
>> - (hash_32_generic(qp_num(qp), 14) & 0x3fff);
>> + qp->src_port = RXE_ROCE_V2_SPORT;
>> qp->sq.max_wr = init->cap.max_send_wr;
>> 
>> /* These caps are limited by rxe_qp_chk_cap() done by the caller */
>> --
>> 2.27.0
Leon Romanovsky Jan. 5, 2022, 8:56 a.m. UTC | #3 
On Wed, Jan 05, 2022 at 08:42:03AM +0000, yanjun.zhu@linux.dev wrote:
> January 5, 2022 3:49 PM, "Leon Romanovsky" <leon@kernel.org> wrote:
> 
> > On Wed, Jan 05, 2022 at 05:12:37PM -0500, yanjun.zhu@linux.dev wrote:
> > 
> >> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> >> 
> >> Since the UDP source port is modified in rxe_modify_qp, the randomization
> >> for UDP source port is redundant in this function. So remove it.
> >> 
> >> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> >> ---
> >> drivers/infiniband/sw/rxe/rxe_qp.c | 10 ++--------
> >> 1 file changed, 2 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
> >> index 54b8711321c1..84d6ffe7350a 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
> >> @@ -210,15 +210,9 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
> >> return err;
> >> qp->sk->sk->sk_user_data = qp;
> >> 
> >> - /* pick a source UDP port number for this QP based on
> >> - * the source QPN. this spreads traffic for different QPs
> >> - * across different NIC RX queues (while using a single
> >> - * flow for a given QP to maintain packet order).
> >> - * the port number must be in the Dynamic Ports range
> >> - * (0xc000 - 0xffff).
> >> + /* Source UDP port number for this QP is modified in rxe_qp_modify.
> >> */
> > 
> > This makes me wonder why do we set this src_port here?
> > Are we using this field before modify QP?
> 
> The commit d3c04a3a6870 ("IB/rxe: vary the source udp port for receive scaling") sets this src_port here.
> 
> The advantage of setting src_port here is: before rxe_modify_qp, the src port is randomized, not 0xc000.
> So after/before rxe_modify_qp, the src port is the same value.
> 
> If the src port is changed in rxe_modify_qp, before rxe_modify_qp, the src port is 0xc000, after rxe_modify_qp,
> the src port is randomized, for example, src port is 0xF043.

I'm asking if you use qp->src_port between this line and rxe_modify_qp?

Thanks

> 
> So when the new method is adopted, I removed this.
> 
> Zhu Yanjun
> 
> > 
> > Thanks
> > 
> >> - qp->src_port = RXE_ROCE_V2_SPORT +
> >> - (hash_32_generic(qp_num(qp), 14) & 0x3fff);
> >> + qp->src_port = RXE_ROCE_V2_SPORT;
> >> qp->sq.max_wr = init->cap.max_send_wr;
> >> 
> >> /* These caps are limited by rxe_qp_chk_cap() done by the caller */
> >> --
> >> 2.27.0
Zhu Yanjun Jan. 5, 2022, 9:03 a.m. UTC | #4 
January 5, 2022 4:56 PM, "Leon Romanovsky" <leon@kernel.org> wrote:

> On Wed, Jan 05, 2022 at 08:42:03AM +0000, yanjun.zhu@linux.dev wrote:
> 
>> January 5, 2022 3:49 PM, "Leon Romanovsky" <leon@kernel.org> wrote:
>> 
>> On Wed, Jan 05, 2022 at 05:12:37PM -0500, yanjun.zhu@linux.dev wrote:
>> 
>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>> 
>> Since the UDP source port is modified in rxe_modify_qp, the randomization
>> for UDP source port is redundant in this function. So remove it.
>> 
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>> ---
>> drivers/infiniband/sw/rxe/rxe_qp.c | 10 ++--------
>> 1 file changed, 2 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
>> index 54b8711321c1..84d6ffe7350a 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
>> @@ -210,15 +210,9 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>> return err;
>> qp->sk->sk->sk_user_data = qp;
>> 
>> - /* pick a source UDP port number for this QP based on
>> - * the source QPN. this spreads traffic for different QPs
>> - * across different NIC RX queues (while using a single
>> - * flow for a given QP to maintain packet order).
>> - * the port number must be in the Dynamic Ports range
>> - * (0xc000 - 0xffff).
>> + /* Source UDP port number for this QP is modified in rxe_qp_modify.
>> */
>> 
>> This makes me wonder why do we set this src_port here?
>> Are we using this field before modify QP?
>> 
>> The commit d3c04a3a6870 ("IB/rxe: vary the source udp port for receive scaling") sets this src_port
>> here.
>> 
>> The advantage of setting src_port here is: before rxe_modify_qp, the src port is randomized, not
>> 0xc000.
>> So after/before rxe_modify_qp, the src port is the same value.
>> 
>> If the src port is changed in rxe_modify_qp, before rxe_modify_qp, the src port is 0xc000, after
>> rxe_modify_qp,
>> the src port is randomized, for example, src port is 0xF043.
> 
> I'm asking if you use qp->src_port between this line and rxe_modify_qp?

There are only 2 udp packets between this line and rxe_modify_qp.

Zhu Yanjun

> 
> Thanks
> 
>> So when the new method is adopted, I removed this.
>> 
>> Zhu Yanjun
>> 
>> Thanks
>> 
>> - qp->src_port = RXE_ROCE_V2_SPORT +
>> - (hash_32_generic(qp_num(qp), 14) & 0x3fff);
>> + qp->src_port = RXE_ROCE_V2_SPORT;
>> qp->sq.max_wr = init->cap.max_send_wr;
>> 
>> /* These caps are limited by rxe_qp_chk_cap() done by the caller */
>> --
>> 2.27.0
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
index 54b8711321c1..84d6ffe7350a 100644
--- a/drivers/infiniband/sw/rxe/rxe_qp.c
+++ b/drivers/infiniband/sw/rxe/rxe_qp.c
@@ -210,15 +210,9 @@  static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
 		return err;
 	qp->sk->sk->sk_user_data = qp;
 
-	/* pick a source UDP port number for this QP based on
-	 * the source QPN. this spreads traffic for different QPs
-	 * across different NIC RX queues (while using a single
-	 * flow for a given QP to maintain packet order).
-	 * the port number must be in the Dynamic Ports range
-	 * (0xc000 - 0xffff).
+	/* Source UDP port number for this QP is modified in rxe_qp_modify.
 	 */
-	qp->src_port = RXE_ROCE_V2_SPORT +
-		(hash_32_generic(qp_num(qp), 14) & 0x3fff);
+	qp->src_port		= RXE_ROCE_V2_SPORT;
 	qp->sq.max_wr		= init->cap.max_send_wr;
 
 	/* These caps are limited by rxe_qp_chk_cap() done by the caller */