diff mbox series

[rdma-next,v1,6/7] RDMA/cm: Set flow label of recv_wc based on primary flow label

Message ID 20200322093031.918447-7-leon@kernel.org (mailing list archive)
State Superseded
Headers show
Series Set flow_label and RoCEv2 UDP source port for datagram QP | expand

Commit Message

Leon Romanovsky March 22, 2020, 9:30 a.m. UTC 
From: Mark Zhang <markz@mellanox.com>

In the request handler of the response side, Set flow label of the
recv_wc if it is not net. It will be used for all messages sent
by the responder.

Signed-off-by: Mark Zhang <markz@mellanox.com>
Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/cm.c | 7 +++++++
 1 file changed, 7 insertions(+)

--
2.24.1

Comments

Jason Gunthorpe March 27, 2020, 12:37 p.m. UTC | #1 
On Sun, Mar 22, 2020 at 11:30:30AM +0200, Leon Romanovsky wrote:
> From: Mark Zhang <markz@mellanox.com>
> 
> In the request handler of the response side, Set flow label of the
> recv_wc if it is not net. It will be used for all messages sent
> by the responder.
> 
> Signed-off-by: Mark Zhang <markz@mellanox.com>
> Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>  drivers/infiniband/core/cm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index bbbfa77dbce7..4ab2f71da522 100644
> +++ b/drivers/infiniband/core/cm.c
> @@ -2039,6 +2039,7 @@ static int cm_req_handler(struct cm_work *work)
>  	struct cm_req_msg *req_msg;
>  	const struct ib_global_route *grh;
>  	const struct ib_gid_attr *gid_attr;
> +	struct ib_grh *ibgrh;
>  	int ret;
> 
>  	req_msg = (struct cm_req_msg *)work->mad_recv_wc->recv_buf.mad;
> @@ -2048,6 +2049,12 @@ static int cm_req_handler(struct cm_work *work)
>  	if (IS_ERR(cm_id_priv))
>  		return PTR_ERR(cm_id_priv);
> 
> +	ibgrh = work->mad_recv_wc->recv_buf.grh;
> +	if (!(be32_to_cpu(ibgrh->version_tclass_flow) & IB_GRH_FLOWLABEL_MASK))
> +		ibgrh->version_tclass_flow |=
> +			cpu_to_be32(IBA_GET(CM_REQ_PRIMARY_FLOW_LABEL,
> +					    req_msg));

This doesn't seem right.

Up until the path is established the response should follow the
reversible GMP rules and the flow_label should come out of the
request's GRH.

Once we established the return data path and the GMP's switch to using
the datapath, the flowlabel should be set in something like
cm_format_paths_from_req()

If you want to switch to using the return data path for REP replies
earlier then it should be done completely and not only the flow
label. But somehow I suspect we cannot as this could fail too.

Jason
Leon Romanovsky March 29, 2020, 8 a.m. UTC | #2 
On Fri, Mar 27, 2020 at 09:37:33AM -0300, Jason Gunthorpe wrote:
> On Sun, Mar 22, 2020 at 11:30:30AM +0200, Leon Romanovsky wrote:
> > From: Mark Zhang <markz@mellanox.com>
> >
> > In the request handler of the response side, Set flow label of the
> > recv_wc if it is not net. It will be used for all messages sent
> > by the responder.
> >
> > Signed-off-by: Mark Zhang <markz@mellanox.com>
> > Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >  drivers/infiniband/core/cm.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> > index bbbfa77dbce7..4ab2f71da522 100644
> > +++ b/drivers/infiniband/core/cm.c
> > @@ -2039,6 +2039,7 @@ static int cm_req_handler(struct cm_work *work)
> >  	struct cm_req_msg *req_msg;
> >  	const struct ib_global_route *grh;
> >  	const struct ib_gid_attr *gid_attr;
> > +	struct ib_grh *ibgrh;
> >  	int ret;
> >
> >  	req_msg = (struct cm_req_msg *)work->mad_recv_wc->recv_buf.mad;
> > @@ -2048,6 +2049,12 @@ static int cm_req_handler(struct cm_work *work)
> >  	if (IS_ERR(cm_id_priv))
> >  		return PTR_ERR(cm_id_priv);
> >
> > +	ibgrh = work->mad_recv_wc->recv_buf.grh;
> > +	if (!(be32_to_cpu(ibgrh->version_tclass_flow) & IB_GRH_FLOWLABEL_MASK))
> > +		ibgrh->version_tclass_flow |=
> > +			cpu_to_be32(IBA_GET(CM_REQ_PRIMARY_FLOW_LABEL,
> > +					    req_msg));
>
> This doesn't seem right.

I will check, this part looks strange while reading IBTA sections
"13.5.4.3 CONSTRUCTING A RESPONSE WITHOUT A GRH" and
"13.5.4.4 CONSTRUCTING A RESPONSE WITH A GRH"

Thanks
Leon Romanovsky March 30, 2020, 6:27 a.m. UTC | #3 
On Fri, Mar 27, 2020 at 09:37:33AM -0300, Jason Gunthorpe wrote:
> On Sun, Mar 22, 2020 at 11:30:30AM +0200, Leon Romanovsky wrote:
> > From: Mark Zhang <markz@mellanox.com>
> >
> > In the request handler of the response side, Set flow label of the
> > recv_wc if it is not net. It will be used for all messages sent
> > by the responder.
> >
> > Signed-off-by: Mark Zhang <markz@mellanox.com>
> > Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >  drivers/infiniband/core/cm.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> > index bbbfa77dbce7..4ab2f71da522 100644
> > +++ b/drivers/infiniband/core/cm.c
> > @@ -2039,6 +2039,7 @@ static int cm_req_handler(struct cm_work *work)
> >  	struct cm_req_msg *req_msg;
> >  	const struct ib_global_route *grh;
> >  	const struct ib_gid_attr *gid_attr;
> > +	struct ib_grh *ibgrh;
> >  	int ret;
> >
> >  	req_msg = (struct cm_req_msg *)work->mad_recv_wc->recv_buf.mad;
> > @@ -2048,6 +2049,12 @@ static int cm_req_handler(struct cm_work *work)
> >  	if (IS_ERR(cm_id_priv))
> >  		return PTR_ERR(cm_id_priv);
> >
> > +	ibgrh = work->mad_recv_wc->recv_buf.grh;
> > +	if (!(be32_to_cpu(ibgrh->version_tclass_flow) & IB_GRH_FLOWLABEL_MASK))
> > +		ibgrh->version_tclass_flow |=
> > +			cpu_to_be32(IBA_GET(CM_REQ_PRIMARY_FLOW_LABEL,
> > +					    req_msg));
>
> This doesn't seem right.
>
> Up until the path is established the response should follow the
> reversible GMP rules and the flow_label should come out of the
> request's GRH.
>
> Once we established the return data path and the GMP's switch to using
> the datapath, the flowlabel should be set in something like
> cm_format_paths_from_req()
>
> If you want to switch to using the return data path for REP replies
> earlier then it should be done completely and not only the flow
> label. But somehow I suspect we cannot as this could fail too.

Jason,

We can drop this patch, it was added to provide same sport in REJ
messages, but it is not needed due to the IBTA section
"13.5.4.3 CONSTRUCTING A RESPONSE WITHOUT A GRH".

Rest of the series is fine.

Thanks

>
> Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index bbbfa77dbce7..4ab2f71da522 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -2039,6 +2039,7 @@  static int cm_req_handler(struct cm_work *work)
 	struct cm_req_msg *req_msg;
 	const struct ib_global_route *grh;
 	const struct ib_gid_attr *gid_attr;
+	struct ib_grh *ibgrh;
 	int ret;

 	req_msg = (struct cm_req_msg *)work->mad_recv_wc->recv_buf.mad;
@@ -2048,6 +2049,12 @@  static int cm_req_handler(struct cm_work *work)
 	if (IS_ERR(cm_id_priv))
 		return PTR_ERR(cm_id_priv);

+	ibgrh = work->mad_recv_wc->recv_buf.grh;
+	if (!(be32_to_cpu(ibgrh->version_tclass_flow) & IB_GRH_FLOWLABEL_MASK))
+		ibgrh->version_tclass_flow |=
+			cpu_to_be32(IBA_GET(CM_REQ_PRIMARY_FLOW_LABEL,
+					    req_msg));
+
 	cm_id_priv->id.remote_id =
 		cpu_to_be32(IBA_GET(CM_REQ_LOCAL_COMM_ID, req_msg));
 	cm_id_priv->id.service_id =