[PATCHv2 0/5] Generate UDP src port with flow label or lqpn/rqpn

[PATCHv2 0/5] Generate UDP src port with flow label or lqpn/rqpn

[yanjun.zhu]

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

Follow the advice from Leon Romanovsky, rdma_get_udp_sport is moved to
ib_verbs.h. several drivers generate udp source port with this function.

---
v1->v2:Remove the local variables in commits "RDMA/irdma: Make the source
       udp port vary" and "RDMA/rxe: Use the standard method to produce
       udp source port". A new commit is added to remove the redundant
       randomization for UDP source port in RXE.
---

Zhu Yanjun (5):
  RDMA/core: Calculate UDP source port based on flow label or lqpn/rqpn
  RDMA/hns: Replace get_udp_sport with rdma_get_udp_sport
  RDMA/irdma: Make the source udp port vary
  RDMA/rxe: Use the standard method to produce udp source port
  RDMA/rxe: Remove the redundant randomization for UDP source port

 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 12 ++----------
 drivers/infiniband/hw/irdma/verbs.c        |  4 ++++
 drivers/infiniband/sw/rxe/rxe_qp.c         | 10 ++--------
 drivers/infiniband/sw/rxe/rxe_verbs.c      |  6 ++++++
 include/rdma/ib_verbs.h                    | 17 +++++++++++++++++
 5 files changed, 31 insertions(+), 18 deletions(-)

-- 
2.27.0

[PATCH 1/5] RDMA/core: Calculate UDP source port based on flow label or lqpn/rqpn

[yanjun.zhu]

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

Calculate and set UDP source port based on the flow label. If flow label
is not defined in GRH then calculate it based on lqpn/rqpn.

Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
---
 include/rdma/ib_verbs.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 6e9ad656ecb7..69d883f7fb41 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -4749,6 +4749,23 @@ static inline u32 rdma_calc_flow_label(u32 lqpn, u32 rqpn)
 	return (u32)(v & IB_GRH_FLOWLABEL_MASK);
 }
 
+/**
+ * rdma_get_udp_sport - Calculate and set UDP source port based on the flow
+ *                      label. If flow label is not defined in GRH then
+ *                      calculate it based on lqpn/rqpn.
+ *
+ * @fl:                 flow label from GRH
+ * @lqpn:               local qp number
+ * @rqpn:               remote qp number
+ */
+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);
+}
+
 const struct ib_port_immutable*
 ib_port_immutable_read(struct ib_device *dev, unsigned int port);
 #endif /* IB_VERBS_H */
-- 
2.27.0

Re: [PATCH 1/5] RDMA/core: Calculate UDP source port based on flow label or lqpn/rqpn

[Leon Romanovsky]

On Wed, Jan 05, 2022 at 05:12:33PM -0500, yanjun.zhu@linux.dev wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Calculate and set UDP source port based on the flow label. If flow label
> is not defined in GRH then calculate it based on lqpn/rqpn.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  include/rdma/ib_verbs.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

[PATCH 2/5] RDMA/hns: Replace get_udp_sport with rdma_get_udp_sport

[yanjun.zhu]

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

Several drivers have the same function xxx_get_udp_sport. So this
function is moved to ib_verbs.h.

Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index eb0defa80d0d..cb795663b813 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -4488,14 +4488,6 @@ static int modify_qp_rtr_to_rts(struct ib_qp *ibqp,
 	return 0;
 }
 
-static inline u16 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);
-}
-
 static int get_dip_ctx_idx(struct ib_qp *ibqp, const struct ib_qp_attr *attr,
 			   u32 *dip_idx)
 {
@@ -4712,8 +4704,8 @@ static int hns_roce_v2_set_path(struct ib_qp *ibqp,
 	}
 
 	hr_reg_write(context, QPC_UDPSPN,
-		     is_udp ? get_udp_sport(grh->flow_label, ibqp->qp_num,
-					    attr->dest_qp_num) : 0);
+		     is_udp ? rdma_get_udp_sport(grh->flow_label, ibqp->qp_num,
+						 attr->dest_qp_num) : 0);
 
 	hr_reg_clear(qpc_mask, QPC_UDPSPN);
 
-- 
2.27.0

Re: [PATCH 2/5] RDMA/hns: Replace get_udp_sport with rdma_get_udp_sport

[Leon Romanovsky]

On Wed, Jan 05, 2022 at 05:12:34PM -0500, yanjun.zhu@linux.dev wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Several drivers have the same function xxx_get_udp_sport. So this
> function is moved to ib_verbs.h.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

[PATCH 3/5] RDMA/irdma: Make the source udp port vary

[yanjun.zhu]

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

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.

Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
---
 drivers/infiniband/hw/irdma/verbs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
index 8cd5f9261692..8234a101e752 100644
--- a/drivers/infiniband/hw/irdma/verbs.c
+++ b/drivers/infiniband/hw/irdma/verbs.c
@@ -1170,6 +1170,10 @@ int irdma_modify_qp_roce(struct ib_qp *ibqp, struct ib_qp_attr *attr,
 			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;
+			udp_info->src_port = rdma_get_udp_sport(
+						udp_info->flow_label,
+						ibqp->qp_num,
+						roce_info->dest_qp);
 			irdma_qp_rem_qos(&iwqp->sc_qp);
 			dev->ws_remove(iwqp->sc_qp.vsi, ctx_info->user_pri);
 			ctx_info->user_pri = rt_tos2priority(udp_info->tos);
-- 
2.27.0

Re: [PATCH 3/5] RDMA/irdma: Make the source udp port vary

[Leon Romanovsky]

On Wed, Jan 05, 2022 at 05:12:35PM -0500, yanjun.zhu@linux.dev wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> 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.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/infiniband/hw/irdma/verbs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

RE: [PATCH 3/5] RDMA/irdma: Make the source udp port vary

[Saleem, Shiraz]

> Subject: [PATCH 3/5] RDMA/irdma: Make the source udp port vary
> 
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> 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.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/infiniband/hw/irdma/verbs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Acked-by: Shiraz Saleem <shiraz.saleem@intel.com>

[PATCH 4/5] RDMA/rxe: Use the standard method to produce udp source port

[yanjun.zhu]

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

Use the standard method to produce udp source port.

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

diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 0aa0d7e52773..42fa81b455de 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -469,6 +469,12 @@ static int rxe_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
 	if (err)
 		goto err1;
 
+	if ((mask & IB_QP_AV) && (attr->ah_attr.ah_flags & IB_AH_GRH))
+		qp->src_port = rdma_get_udp_sport(attr->ah_attr.grh.flow_label,
+						  qp->ibqp.qp_num,
+						  qp->attr.dest_qp_num);
+
+
 	return 0;
 
 err1:
-- 
2.27.0

Re: [PATCH 4/5] RDMA/rxe: Use the standard method to produce udp source port

[Leon Romanovsky]

On Wed, Jan 05, 2022 at 05:12:36PM -0500, yanjun.zhu@linux.dev wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Use the standard method to produce udp source port.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/infiniband/sw/rxe/rxe_verbs.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> index 0aa0d7e52773..42fa81b455de 100644
> --- a/drivers/infiniband/sw/rxe/rxe_verbs.c
> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
> @@ -469,6 +469,12 @@ static int rxe_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
>  	if (err)
>  		goto err1;
>  
> +	if ((mask & IB_QP_AV) && (attr->ah_attr.ah_flags & IB_AH_GRH))

You are leaving src_port default and wired to same port as other QPs
without any randomization.

Thanks

> +		qp->src_port = rdma_get_udp_sport(attr->ah_attr.grh.flow_label,
> +						  qp->ibqp.qp_num,
> +						  qp->attr.dest_qp_num);
> +
> +
>  	return 0;
>  
>  err1:
> -- 
> 2.27.0
> 

Re: [PATCH 4/5] RDMA/rxe: Use the standard method to produce udp source port

[Zhu Yanjun]

On Wed, Jan 5, 2022 at 3:52 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Wed, Jan 05, 2022 at 05:12:36PM -0500, yanjun.zhu@linux.dev wrote:
> > From: Zhu Yanjun <yanjun.zhu@linux.dev>
> >
> > Use the standard method to produce udp source port.
> >
> > Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> > ---
> >  drivers/infiniband/sw/rxe/rxe_verbs.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> > index 0aa0d7e52773..42fa81b455de 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_verbs.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
> > @@ -469,6 +469,12 @@ static int rxe_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
> >       if (err)
> >               goto err1;
> >
> > +     if ((mask & IB_QP_AV) && (attr->ah_attr.ah_flags & IB_AH_GRH))
>
> You are leaving src_port default and wired to same port as other QPs
> without any randomization.

Hi,

I do not get you. Why do you think I am leaving src_pport default?
Thanks.

Zhu Yanjun

>
> Thanks
>
> > +             qp->src_port = rdma_get_udp_sport(attr->ah_attr.grh.flow_label,
> > +                                               qp->ibqp.qp_num,
> > +                                               qp->attr.dest_qp_num);
> > +
> > +
> >       return 0;
> >
> >  err1:
> > --
> > 2.27.0
> >

Re: [PATCH 4/5] RDMA/rxe: Use the standard method to produce udp source port

[Leon Romanovsky]

On Wed, Jan 05, 2022 at 04:27:38PM +0800, Zhu Yanjun wrote:
> On Wed, Jan 5, 2022 at 3:52 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Wed, Jan 05, 2022 at 05:12:36PM -0500, yanjun.zhu@linux.dev wrote:
> > > From: Zhu Yanjun <yanjun.zhu@linux.dev>
> > >
> > > Use the standard method to produce udp source port.
> > >
> > > Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> > > ---
> > >  drivers/infiniband/sw/rxe/rxe_verbs.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> > > index 0aa0d7e52773..42fa81b455de 100644
> > > --- a/drivers/infiniband/sw/rxe/rxe_verbs.c
> > > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
> > > @@ -469,6 +469,12 @@ static int rxe_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
> > >       if (err)
> > >               goto err1;
> > >
> > > +     if ((mask & IB_QP_AV) && (attr->ah_attr.ah_flags & IB_AH_GRH))
> >
> > You are leaving src_port default and wired to same port as other QPs
> > without any randomization.
> 
> Hi,
> 
> I do not get you. Why do you think I am leaving src_pport default?

Because in original code, you randomized src_port without any relation
to mask flags. 

       qp->src_port = RXE_ROCE_V2_SPORT +
               (hash_32_generic(qp_num(qp), 14) & 0x3fff);

After patch #5, if user doesn't pass "proper" mask, you will leave
qp->src_port to be equal to RXE_ROCE_V2_SPORT, which is different from
the current behaviour.

Thanks


> Thanks.
> 
> Zhu Yanjun
> 
> >
> > Thanks
> >
> > > +             qp->src_port = rdma_get_udp_sport(attr->ah_attr.grh.flow_label,
> > > +                                               qp->ibqp.qp_num,
> > > +                                               qp->attr.dest_qp_num);
> > > +
> > > +
> > >       return 0;
> > >
> > >  err1:
> > > --
> > > 2.27.0
> > >

Re: [PATCH 4/5] RDMA/rxe: Use the standard method to produce udp source port

[Zhu Yanjun]

On Wed, Jan 5, 2022 at 4:55 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Wed, Jan 05, 2022 at 04:27:38PM +0800, Zhu Yanjun wrote:
> > On Wed, Jan 5, 2022 at 3:52 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Wed, Jan 05, 2022 at 05:12:36PM -0500, yanjun.zhu@linux.dev wrote:
> > > > From: Zhu Yanjun <yanjun.zhu@linux.dev>
> > > >
> > > > Use the standard method to produce udp source port.
> > > >
> > > > Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> > > > ---
> > > >  drivers/infiniband/sw/rxe/rxe_verbs.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> > > > index 0aa0d7e52773..42fa81b455de 100644
> > > > --- a/drivers/infiniband/sw/rxe/rxe_verbs.c
> > > > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
> > > > @@ -469,6 +469,12 @@ static int rxe_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
> > > >       if (err)
> > > >               goto err1;
> > > >
> > > > +     if ((mask & IB_QP_AV) && (attr->ah_attr.ah_flags & IB_AH_GRH))
> > >
> > > You are leaving src_port default and wired to same port as other QPs
> > > without any randomization.
> >
> > Hi,
> >
> > I do not get you. Why do you think I am leaving src_pport default?
>
> Because in original code, you randomized src_port without any relation
> to mask flags.
>
>        qp->src_port = RXE_ROCE_V2_SPORT +
>                (hash_32_generic(qp_num(qp), 14) & 0x3fff);
>
> After patch #5, if user doesn't pass "proper" mask, you will leave
> qp->src_port to be equal to RXE_ROCE_V2_SPORT, which is different from
> the current behaviour.

Hi, Leon Romanovsky

I read your comments again and checked the source code.
And I found this https://lkml.org/lkml/2015/12/15/566, Jason commented:
"
...
The GRH is optional for in-subnet communications.
...
"
I agree with you. When in-subnet communications, GRH is optional.
It is possible that qp->src_port is set to RXE_ROCE_V2_SPORT.

So I will remove patch #5 and send the patch series again.
Thanks.
Zhu Yanjun

>
> Thanks
>
>
> > Thanks.
> >
> > Zhu Yanjun
> >
> > >
> > > Thanks
> > >
> > > > +             qp->src_port = rdma_get_udp_sport(attr->ah_attr.grh.flow_label,
> > > > +                                               qp->ibqp.qp_num,
> > > > +                                               qp->attr.dest_qp_num);
> > > > +
> > > > +
> > > >       return 0;
> > > >
> > > >  err1:
> > > > --
> > > > 2.27.0
> > > >

Re: [PATCH 4/5] RDMA/rxe: Use the standard method to produce udp source port

[yanjun.zhu]

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

> On Wed, Jan 05, 2022 at 04:27:38PM +0800, Zhu Yanjun wrote:
> 
>> On Wed, Jan 5, 2022 at 3:52 PM Leon Romanovsky <leon@kernel.org> wrote:
>> 
>> On Wed, Jan 05, 2022 at 05:12:36PM -0500, yanjun.zhu@linux.dev wrote:
>>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>>> 
>>> Use the standard method to produce udp source port.
>>> 
>>> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>>> ---
>>> drivers/infiniband/sw/rxe/rxe_verbs.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>> 
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
>>> index 0aa0d7e52773..42fa81b455de 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_verbs.c
>>> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
>>> @@ -469,6 +469,12 @@ static int rxe_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
>>> if (err)
>>> goto err1;
>>> 
>>> + if ((mask & IB_QP_AV) && (attr->ah_attr.ah_flags & IB_AH_GRH))
>> 
>> You are leaving src_port default and wired to same port as other QPs
>> without any randomization.
>> 
>> Hi,
>> 
>> I do not get you. Why do you think I am leaving src_pport default?
> 
> Because in original code, you randomized src_port without any relation
> to mask flags.
> 
> qp->src_port = RXE_ROCE_V2_SPORT +
> (hash_32_generic(qp_num(qp), 14) & 0x3fff);
> 
> After patch #5, if user doesn't pass "proper" mask, you will leave
> qp->src_port to be equal to RXE_ROCE_V2_SPORT, which is different from
> the current behaviour.

About the "proper" mask, please check this link https://patchwork.kernel.org/project/linux-rdma/patch/20211218204438.1345160-1-yanjun.zhu@linux.dev/

"the udp_sport is only set when address vector and dest qpn (IB_QP_AV and IB_QP_DEST_QPN) is provided."

Zhu Yanjun
> 
> Thanks
> 
>> Thanks.
>> 
>> Zhu Yanjun
>> 
>> Thanks
>> 
>>> + qp->src_port = rdma_get_udp_sport(attr->ah_attr.grh.flow_label,
>>> + qp->ibqp.qp_num,
>>> + qp->attr.dest_qp_num);
>>> +
>>> +
>>> return 0;
>>> 
>>> err1:
>>> --
>>> 2.27.0
>>>

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

[yanjun.zhu]

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.
 	 */
-	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

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

[Leon Romanovsky]

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
> 

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

[yanjun.zhu]

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

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

[Leon Romanovsky]

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

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

[yanjun.zhu]

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