[PATCHv3 1/1] RDMA/irdma: Make the source udp port vary

[PATCHv3 1/1] RDMA/irdma: Make the source udp port vary

[yanjun.zhu]

From: Zhu Yanjun <yanjun.zhu@linux.dev>

Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html,
get the source udp port number for a QP based on the grh.flow_label or
lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues.
The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp
source port when set path") is a standard way. So it is also adopted in
this commit.

Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
---
V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in ah_flags
V1->V2: Adopt a standard method to get udp source port.
---
 drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
index 8cd5f9261692..31039b295206 100644
--- a/drivers/infiniband/hw/irdma/verbs.c
+++ b/drivers/infiniband/hw/irdma/verbs.c
@@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev, u32 port, u16 index,
 	return 0;
 }
 
+
+static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn)
+{
+	if (!fl)
+		fl = rdma_calc_flow_label(lqpn, rqpn);
+
+	return rdma_flow_label_to_udp_sport(fl);
+}
+
 /**
  * irdma_modify_qp_roce - modify qp request
  * @ibqp: qp's pointer for modify
@@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct ib_qp_attr *attr,
 
 		memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah));
 		if (attr->ah_attr.ah_flags & IB_AH_GRH) {
+			u32 fl = udp_info->flow_label;
+			u32 lqp = ibqp->qp_num;
+			u32 rqp = roce_info->dest_qp;
+
+			udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp);
 			udp_info->ttl = attr->ah_attr.grh.hop_limit;
 			udp_info->flow_label = attr->ah_attr.grh.flow_label;
 			udp_info->tos = attr->ah_attr.grh.traffic_class;
-- 
2.27.0

RE: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary

[Ismail, Mustafa]

> Subject: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary
> 
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html,
> get the source udp port number for a QP based on the grh.flow_label or
> lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues.
> The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp
> source port when set path") is a standard way. So it is also adopted in this
> commit.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
> V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in
> V2->ah_flags
> V1->V2: Adopt a standard method to get udp source port.
> ---
>  drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 

Reviewed-by: Mustafa Ismail <mustafa.ismail@intel.com>

Re: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary

[Leon Romanovsky]

On Tue, Dec 21, 2021 at 12:39:13PM -0500, yanjun.zhu@linux.dev wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html,

Please use lore.kernel.org links. They have all chances to outlive spinics.

> get the source udp port number for a QP based on the grh.flow_label or
> lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues.
> The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp
> source port when set path") is a standard way. So it is also adopted in
> this commit.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
> V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in ah_flags
> V1->V2: Adopt a standard method to get udp source port.
> ---
>  drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
> index 8cd5f9261692..31039b295206 100644
> --- a/drivers/infiniband/hw/irdma/verbs.c
> +++ b/drivers/infiniband/hw/irdma/verbs.c
> @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev, u32 port, u16 index,
>  	return 0;
>  }
>  
> +
> +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn)
> +{
> +	if (!fl)
> +		fl = rdma_calc_flow_label(lqpn, rqpn);
> +
> +	return rdma_flow_label_to_udp_sport(fl);
> +}
> +
>  /**
>   * irdma_modify_qp_roce - modify qp request
>   * @ibqp: qp's pointer for modify
> @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct ib_qp_attr *attr,
>  
>  		memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah));
>  		if (attr->ah_attr.ah_flags & IB_AH_GRH) {
> +			u32 fl = udp_info->flow_label;
> +			u32 lqp = ibqp->qp_num;
> +			u32 rqp = roce_info->dest_qp;
> +

I don't see too much value in these extra variables and extra function
that is the same as get_udp_sport() from hns.

It is worth to add new function to ib_verbs.h and reuse in both drivers.

Thanks

> +			udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp);
>  			udp_info->ttl = attr->ah_attr.grh.hop_limit;
>  			udp_info->flow_label = attr->ah_attr.grh.flow_label;
>  			udp_info->tos = attr->ah_attr.grh.traffic_class;
> -- 
> 2.27.0
> 

Re: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary

[yanjun.zhu]

January 3, 2022 5:47 PM, "Leon Romanovsky" <leon@kernel.org> wrote:

> On Tue, Dec 21, 2021 at 12:39:13PM -0500, yanjun.zhu@linux.dev wrote:
> 
>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>> 
>> Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html,
> 
> Please use lore.kernel.org links. They have all chances to outlive spinics.
> 
>> get the source udp port number for a QP based on the grh.flow_label or
>> lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues.
>> The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp
>> source port when set path") is a standard way. So it is also adopted in
>> this commit.
>> 
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>> ---
>> V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in ah_flags
>> V1->V2: Adopt a standard method to get udp source port.
>> ---
>> drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>> 
>> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
>> index 8cd5f9261692..31039b295206 100644
>> --- a/drivers/infiniband/hw/irdma/verbs.c
>> +++ b/drivers/infiniband/hw/irdma/verbs.c
>> @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev, u32 port, u16 index,
>> return 0;
>> }
>> 
>> +
>> +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn)
>> +{
>> + if (!fl)
>> + fl = rdma_calc_flow_label(lqpn, rqpn);
>> +
>> + return rdma_flow_label_to_udp_sport(fl);
>> +}
>> +
>> /**
>> * irdma_modify_qp_roce - modify qp request
>> * @ibqp: qp's pointer for modify
>> @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct ib_qp_attr *attr,
>> 
>> memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah));
>> if (attr->ah_attr.ah_flags & IB_AH_GRH) {
>> + u32 fl = udp_info->flow_label;
>> + u32 lqp = ibqp->qp_num;
>> + u32 rqp = roce_info->dest_qp;
>> +
> 
> I don't see too much value in these extra variables and extra function
> that is the same as get_udp_sport() from hns.
> 
> It is worth to add new function to ib_verbs.h and reuse in both drivers.

Do you mean the following function should be added into ib_verbs.h?

"
static inline u16 rdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn)
{
        if (!fl)
                fl = rdma_calc_flow_label(lqpn, rqpn);

        return rdma_flow_label_to_udp_sport(fl);
}
"
Then in hns, rxe and irdma, this function is called to get udp source port?
If so, I will send new patches.

Thanks.
Zhu Yanjun

> 
> Thanks
> 
>> + udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp);
>> udp_info->ttl = attr->ah_attr.grh.hop_limit;
>> udp_info->flow_label = attr->ah_attr.grh.flow_label;
>> udp_info->tos = attr->ah_attr.grh.traffic_class;
>> --
>> 2.27.0

Re: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary

[Leon Romanovsky]

On Tue, Jan 04, 2022 at 07:51:54AM +0000, yanjun.zhu@linux.dev wrote:
> January 3, 2022 5:47 PM, "Leon Romanovsky" <leon@kernel.org> wrote:
> 
> > On Tue, Dec 21, 2021 at 12:39:13PM -0500, yanjun.zhu@linux.dev wrote:
> > 
> >> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> >> 
> >> Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html,
> > 
> > Please use lore.kernel.org links. They have all chances to outlive spinics.
> > 
> >> get the source udp port number for a QP based on the grh.flow_label or
> >> lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues.
> >> The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp
> >> source port when set path") is a standard way. So it is also adopted in
> >> this commit.
> >> 
> >> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> >> ---
> >> V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in ah_flags
> >> V1->V2: Adopt a standard method to get udp source port.
> >> ---
> >> drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++
> >> 1 file changed, 14 insertions(+)
> >> 
> >> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
> >> index 8cd5f9261692..31039b295206 100644
> >> --- a/drivers/infiniband/hw/irdma/verbs.c
> >> +++ b/drivers/infiniband/hw/irdma/verbs.c
> >> @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev, u32 port, u16 index,
> >> return 0;
> >> }
> >> 
> >> +
> >> +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn)
> >> +{
> >> + if (!fl)
> >> + fl = rdma_calc_flow_label(lqpn, rqpn);
> >> +
> >> + return rdma_flow_label_to_udp_sport(fl);
> >> +}
> >> +
> >> /**
> >> * irdma_modify_qp_roce - modify qp request
> >> * @ibqp: qp's pointer for modify
> >> @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct ib_qp_attr *attr,
> >> 
> >> memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah));
> >> if (attr->ah_attr.ah_flags & IB_AH_GRH) {
> >> + u32 fl = udp_info->flow_label;
> >> + u32 lqp = ibqp->qp_num;
> >> + u32 rqp = roce_info->dest_qp;
> >> +
> > 
> > I don't see too much value in these extra variables and extra function
> > that is the same as get_udp_sport() from hns.
> > 
> > It is worth to add new function to ib_verbs.h and reuse in both drivers.
> 
> Do you mean the following function should be added into ib_verbs.h?
> 
> "
> static inline u16 rdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn)
> {
>         if (!fl)
>                 fl = rdma_calc_flow_label(lqpn, rqpn);
> 
>         return rdma_flow_label_to_udp_sport(fl);
> }
> "
> Then in hns, rxe and irdma, this function is called to get udp source port?
> If so, I will send new patches.

Yes and group all these patches in one series, please.

Thanks

> 
> Thanks.
> Zhu Yanjun
> 
> > 
> > Thanks
> > 
> >> + udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp);
> >> udp_info->ttl = attr->ah_attr.grh.hop_limit;
> >> udp_info->flow_label = attr->ah_attr.grh.flow_label;
> >> udp_info->tos = attr->ah_attr.grh.traffic_class;
> >> --
> >> 2.27.0

RE: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary

[Saleem, Shiraz]

> Subject: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary
> 
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html,
> get the source udp port number for a QP based on the grh.flow_label or
> lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues.
> The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp
> source port when set path") is a standard way. So it is also adopted in this
> commit.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
> V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in
> V2->ah_flags
> V1->V2: Adopt a standard method to get udp source port.
> ---
>  drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
> index 8cd5f9261692..31039b295206 100644
> --- a/drivers/infiniband/hw/irdma/verbs.c
> +++ b/drivers/infiniband/hw/irdma/verbs.c
> @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev,
> u32 port, u16 index,
>  	return 0;
>  }
> 
> +
> +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn) {
> +	if (!fl)
> +		fl = rdma_calc_flow_label(lqpn, rqpn);
> +
> +	return rdma_flow_label_to_udp_sport(fl); }
> +
>  /**
>   * irdma_modify_qp_roce - modify qp request
>   * @ibqp: qp's pointer for modify
> @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct
> ib_qp_attr *attr,
> 
>  		memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah));
>  		if (attr->ah_attr.ah_flags & IB_AH_GRH) {
> +			u32 fl = udp_info->flow_label;
> +			u32 lqp = ibqp->qp_num;
> +			u32 rqp = roce_info->dest_qp;
> +
> +			udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp);
The flow label used in function is not right as udp_info->flow_label is only set a few lines below. Kindly fold this fix into your next revision.

>  			udp_info->ttl = attr->ah_attr.grh.hop_limit;
>  			udp_info->flow_label = attr->ah_attr.grh.flow_label;
>  			udp_info->tos = attr->ah_attr.grh.traffic_class;
> --
> 2.27.0

Re: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary

[Yanjun Zhu]

在 2022/1/4 23:07, Saleem, Shiraz 写道:
>> Subject: [PATCHv3 1/1] RDMA/irdma: Make the source udp port vary
>>
>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
>> Based on the link https://www.spinics.net/lists/linux-rdma/msg73735.html,
>> get the source udp port number for a QP based on the grh.flow_label or
>> lqpn/rqrpn. This provides a better spread of traffic across NIC RX queues.
>> The method in the commit 2b880b2e5e03 ("RDMA/mlx5: Define RoCEv2 udp
>> source port when set path") is a standard way. So it is also adopted in this
>> commit.
>>
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>> ---
>> V2->V3: Move to the block of IB_QP_AV in the mask and IB_AH_GRH in
>> V2->ah_flags
>> V1->V2: Adopt a standard method to get udp source port.
>> ---
>>   drivers/infiniband/hw/irdma/verbs.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
>> index 8cd5f9261692..31039b295206 100644
>> --- a/drivers/infiniband/hw/irdma/verbs.c
>> +++ b/drivers/infiniband/hw/irdma/verbs.c
>> @@ -1094,6 +1094,15 @@ static int irdma_query_pkey(struct ib_device *ibdev,
>> u32 port, u16 index,
>>   	return 0;
>>   }
>>
>> +
>> +static u16 irdma_get_udp_sport(u32 fl, u32 lqpn, u32 rqpn) {
>> +	if (!fl)
>> +		fl = rdma_calc_flow_label(lqpn, rqpn);
>> +
>> +	return rdma_flow_label_to_udp_sport(fl); }
>> +
>>   /**
>>    * irdma_modify_qp_roce - modify qp request
>>    * @ibqp: qp's pointer for modify
>> @@ -1167,6 +1176,11 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct
>> ib_qp_attr *attr,
>>
>>   		memset(&iwqp->roce_ah, 0, sizeof(iwqp->roce_ah));
>>   		if (attr->ah_attr.ah_flags & IB_AH_GRH) {
>> +			u32 fl = udp_info->flow_label;
>> +			u32 lqp = ibqp->qp_num;
>> +			u32 rqp = roce_info->dest_qp;
>> +
>> +			udp_info->src_port = irdma_get_udp_sport(fl, lqp, rqp);
> The flow label used in function is not right as udp_info->flow_label is only set a few lines below. Kindly fold this fix into your next revision.

Thanks.  " attr->ah_attr.grh.flow_label " should be OK.

Zhu Yanjun

>
>>   			udp_info->ttl = attr->ah_attr.grh.hop_limit;
>>   			udp_info->flow_label = attr->ah_attr.grh.flow_label;
>>   			udp_info->tos = attr->ah_attr.grh.traffic_class;
>> --
>> 2.27.0