diff mbox series

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

Message ID 20220105221237.2659462-5-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>

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(+)

Comments

Leon Romanovsky Jan. 5, 2022, 7:52 a.m. UTC | #1 
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
>
Zhu Logan Jan. 5, 2022, 8:27 a.m. UTC | #2 
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
> >
Leon Romanovsky Jan. 5, 2022, 8:55 a.m. UTC | #3 
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
> > >
Zhu Yanjun Jan. 5, 2022, 9:13 a.m. UTC | #4 
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
>>>
Zhu Logan Jan. 5, 2022, 12:42 p.m. UTC | #5 
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
> > > >
diff mbox series

Patch

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: