[PATCH] IB/ehca: Construct MAD redirect replies from request MAD

[PATCH] IB/ehca: Construct MAD redirect replies from request MAD

[Joachim Fenkes]

The old code used a lot of hardcoded values, which might not be valid in all
environments (especially routed fabrics or partitioned subnets). Copy as
much information as possible from the incoming request to prevent that.

Signed-off-by: Joachim Fenkes <fenkes@de.ibm.com>
---

Hal, Jason -- here's the change I promised. Looks okay to you?
Roland -- if Hal and Jason don't object, please queue this up for the next
kernel. Thanks!

Regards,
  Joachim

 drivers/infiniband/hw/ehca/ehca_sqp.c |   47 ++++++++++++++++++++++++++++----
 1 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/hw/ehca/ehca_sqp.c b/drivers/infiniband/hw/ehca/ehca_sqp.c
index c568b28..8c1213f 100644
--- a/drivers/infiniband/hw/ehca/ehca_sqp.c
+++ b/drivers/infiniband/hw/ehca/ehca_sqp.c
@@ -125,14 +125,30 @@ struct ib_perf {
 	u8 data[192];
 } __attribute__ ((packed));
 
+/* TC/SL/FL packed into 32 bits, as in ClassPortInfo */
+struct tcslfl {
+	u32 tc:8;
+	u32 sl:4;
+	u32 fl:20;
+} __attribute__ ((packed));
+
+/* IP Version/TC/FL packed into 32 bits, as in GRH */
+struct vertcfl {
+	u32 ver:4;
+	u32 tc:8;
+	u32 fl:20;
+} __attribute__ ((packed));
 
 static int ehca_process_perf(struct ib_device *ibdev, u8 port_num,
+			     struct ib_wc *in_wc, struct ib_grh *in_grh,
 			     struct ib_mad *in_mad, struct ib_mad *out_mad)
 {
 	struct ib_perf *in_perf = (struct ib_perf *)in_mad;
 	struct ib_perf *out_perf = (struct ib_perf *)out_mad;
 	struct ib_class_port_info *poi =
 		(struct ib_class_port_info *)out_perf->data;
+	struct tcslfl *tcslfl =
+		(struct tcslfl *)&poi->redirect_tcslfl;
 	struct ehca_shca *shca =
 		container_of(ibdev, struct ehca_shca, ib_device);
 	struct ehca_sport *sport = &shca->sport[port_num - 1];
@@ -158,10 +174,29 @@ static int ehca_process_perf(struct ib_device *ibdev, u8 port_num,
 		poi->base_version = 1;
 		poi->class_version = 1;
 		poi->resp_time_value = 18;
-		poi->redirect_lid = sport->saved_attr.lid;
-		poi->redirect_qp = sport->pma_qp_nr;
+
+		/* copy local routing information from WC where applicable */
+		tcslfl->sl         = in_wc->sl;
+		poi->redirect_lid  =
+			sport->saved_attr.lid | in_wc->dlid_path_bits;
+		poi->redirect_qp   = sport->pma_qp_nr;
 		poi->redirect_qkey = IB_QP1_QKEY;
-		poi->redirect_pkey = IB_DEFAULT_PKEY_FULL;
+
+		ehca_query_pkey(ibdev, port_num, in_wc->pkey_index,
+				&poi->redirect_pkey);
+
+		/* if request was globally routed, copy route info */
+		if (in_grh) {
+			struct vertcfl *vertcfl =
+				(struct vertcfl *)&in_grh->version_tclass_flow;
+			memcpy(poi->redirect_gid, in_grh->dgid.raw,
+			       sizeof(poi->redirect_gid));
+			tcslfl->tc        = vertcfl->tc;
+			tcslfl->fl        = vertcfl->fl;
+		} else
+			/* else only fill in default GID */
+			ehca_query_gid(ibdev, port_num, 0,
+				       (union ib_gid *)&poi->redirect_gid);
 
 		ehca_dbg(ibdev, "ehca_pma_lid=%x ehca_pma_qp=%x",
 			 sport->saved_attr.lid, sport->pma_qp_nr);
@@ -183,8 +218,7 @@ perf_reply:
 
 int ehca_process_mad(struct ib_device *ibdev, int mad_flags, u8 port_num,
 		     struct ib_wc *in_wc, struct ib_grh *in_grh,
-		     struct ib_mad *in_mad,
-		     struct ib_mad *out_mad)
+		     struct ib_mad *in_mad, struct ib_mad *out_mad)
 {
 	int ret;
 
@@ -196,7 +230,8 @@ int ehca_process_mad(struct ib_device *ibdev, int mad_flags, u8 port_num,
 		return IB_MAD_RESULT_SUCCESS;
 
 	ehca_dbg(ibdev, "port_num=%x src_qp=%x", port_num, in_wc->src_qp);
-	ret = ehca_process_perf(ibdev, port_num, in_mad, out_mad);
+	ret = ehca_process_perf(ibdev, port_num, in_wc, in_grh,
+				in_mad, out_mad);
 
 	return ret;
 }
-- 
1.6.0.4

Re: [ewg] [PATCH] IB/ehca: Construct MAD redirect replies from request MAD

[Hal Rosenstock]

[-- Attachment #1: Type: text/plain, Size: 5297 bytes --]

On 8/26/09, Joachim Fenkes <fenkes@de.ibm.com> wrote:
>
> The old code used a lot of hardcoded values, which might not be valid in
> all
> environments (especially routed fabrics or partitioned subnets). Copy as
> much information as possible from the incoming request to prevent that.
>
> Signed-off-by: Joachim Fenkes <fenkes@de.ibm.com>
> ---
>
> Hal, Jason -- here's the change I promised. Looks okay to you?
> Roland -- if Hal and Jason don't object, please queue this up for the next
> kernel. Thanks!


Thanks for doing this. It looks sane to me. The only issue I recall that
appears to be remaining is a better setting of ClassPortInfo:RespTimeValue
rather than hardcoding. Perhaps using the value from PortInfo is the way to
go (ideally it would be that value from the port to which the the requester
is being redirected to but that might not be so easy to get from this port
(I guess that could be SA Get PortInfoRecord for that port but that is a
larger change and it likely to be same as local port issuing the redirect
response).

-- Hal

Regards,
> Joachim
>
> drivers/infiniband/hw/ehca/ehca_sqp.c |   47
> ++++++++++++++++++++++++++++----
> 1 files changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/infiniband/hw/ehca/ehca_sqp.c
> b/drivers/infiniband/hw/ehca/ehca_sqp.c
> index c568b28..8c1213f 100644
> --- a/drivers/infiniband/hw/ehca/ehca_sqp.c
> +++ b/drivers/infiniband/hw/ehca/ehca_sqp.c
> @@ -125,14 +125,30 @@ struct ib_perf {
>        u8 data[192];
> } __attribute__ ((packed));
>
> +/* TC/SL/FL packed into 32 bits, as in ClassPortInfo */
> +struct tcslfl {
> +       u32 tc:8;
> +       u32 sl:4;
> +       u32 fl:20;
> +} __attribute__ ((packed));
> +
> +/* IP Version/TC/FL packed into 32 bits, as in GRH */
> +struct vertcfl {
> +       u32 ver:4;
> +       u32 tc:8;
> +       u32 fl:20;
> +} __attribute__ ((packed));
>
> static int ehca_process_perf(struct ib_device *ibdev, u8 port_num,
> +                            struct ib_wc *in_wc, struct ib_grh *in_grh,
>                             struct ib_mad *in_mad, struct ib_mad *out_mad)
> {
>        struct ib_perf *in_perf = (struct ib_perf *)in_mad;
>        struct ib_perf *out_perf = (struct ib_perf *)out_mad;
>        struct ib_class_port_info *poi =
>                (struct ib_class_port_info *)out_perf->data;
> +       struct tcslfl *tcslfl =
> +               (struct tcslfl *)&poi->redirect_tcslfl;
>        struct ehca_shca *shca =
>                container_of(ibdev, struct ehca_shca, ib_device);
>        struct ehca_sport *sport = &shca->sport[port_num - 1];
> @@ -158,10 +174,29 @@ static int ehca_process_perf(struct ib_device *ibdev,
> u8 port_num,
>                poi->base_version = 1;
>                poi->class_version = 1;
>                poi->resp_time_value = 18;
> -               poi->redirect_lid = sport->saved_attr.lid;
> -               poi->redirect_qp = sport->pma_qp_nr;
> +
> +               /* copy local routing information from WC where applicable
> */
> +               tcslfl->sl         = in_wc->sl;
> +               poi->redirect_lid  =
> +                       sport->saved_attr.lid | in_wc->dlid_path_bits;
> +               poi->redirect_qp   = sport->pma_qp_nr;
>                poi->redirect_qkey = IB_QP1_QKEY;
> -               poi->redirect_pkey = IB_DEFAULT_PKEY_FULL;
> +
> +               ehca_query_pkey(ibdev, port_num, in_wc->pkey_index,
> +                               &poi->redirect_pkey);
> +
> +               /* if request was globally routed, copy route info */
> +               if (in_grh) {
> +                       struct vertcfl *vertcfl =
> +                               (struct vertcfl
> *)&in_grh->version_tclass_flow;
> +                       memcpy(poi->redirect_gid, in_grh->dgid.raw,
> +                              sizeof(poi->redirect_gid));
> +                       tcslfl->tc        = vertcfl->tc;
> +                       tcslfl->fl        = vertcfl->fl;
> +               } else
> +                       /* else only fill in default GID */
> +                       ehca_query_gid(ibdev, port_num, 0,
> +                                      (union ib_gid *)&poi->redirect_gid);
>
>                ehca_dbg(ibdev, "ehca_pma_lid=%x ehca_pma_qp=%x",
>                         sport->saved_attr.lid, sport->pma_qp_nr);
> @@ -183,8 +218,7 @@ perf_reply:
>
> int ehca_process_mad(struct ib_device *ibdev, int mad_flags, u8 port_num,
>                     struct ib_wc *in_wc, struct ib_grh *in_grh,
> -                    struct ib_mad *in_mad,
> -                    struct ib_mad *out_mad)
> +                    struct ib_mad *in_mad, struct ib_mad *out_mad)
> {
>        int ret;
>
> @@ -196,7 +230,8 @@ int ehca_process_mad(struct ib_device *ibdev, int
> mad_flags, u8 port_num,
>                return IB_MAD_RESULT_SUCCESS;
>
>        ehca_dbg(ibdev, "port_num=%x src_qp=%x", port_num, in_wc->src_qp);
> -       ret = ehca_process_perf(ibdev, port_num, in_mad, out_mad);
> +       ret = ehca_process_perf(ibdev, port_num, in_wc, in_grh,
> +                               in_mad, out_mad);
>
>        return ret;
> }
> --
> 1.6.0.4
>
>
> _______________________________________________
> ewg mailing list
> ewg@lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg
>

[-- Attachment #2: Type: text/html, Size: 7065 bytes --]

Re: [ewg] [PATCH] IB/ehca: Construct MAD redirect replies from request MAD

[Joachim Fenkes]

Hal Rosenstock <hal.rosenstock@gmail.com> wrote on 26.08.2009 17:15:03:

> Thanks for doing this. It looks sane to me. The only issue I recall that 

> appears to be remaining is a better setting of 
ClassPortInfo:RespTimeValue 
> rather than hardcoding. Perhaps using the value from PortInfo is the way 
to go
> (ideally it would be that value from the port to which the the requester 
is 
> being redirected to but that might not be so easy to get from this port.

I don't think that effort will be necessary or even legal. The requestor 
will react to the redirection with another Get(ClassPortInfo) to the 
redirection target, which will reply with its own RespTimeValue, so our 
driver should speak for itself. Since we don't know when our MAD 
processing and sending of the response is going to be scheduled (we're not 
running on real-time constraints here), we play it safe and return 18, 
which amounts to roughly a second. 

Make sense?

Regards
  Joachim

Re: [ewg] [PATCH] IB/ehca: Construct MAD redirect replies from request MAD

[Hal Rosenstock]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On 8/27/09, Joachim Fenkes <FENKES@de.ibm.com> wrote:
>
> Hal Rosenstock <hal.rosenstock@gmail.com> wrote on 26.08.2009 17:15:03:
>
> > Thanks for doing this. It looks sane to me. The only issue I recall that
>
> > appears to be remaining is a better setting of
> ClassPortInfo:RespTimeValue
> > rather than hardcoding. Perhaps using the value from PortInfo is the way
> to go
> > (ideally it would be that value from the port to which the the requester
> is
> > being redirected to but that might not be so easy to get from this port.
>
> I don't think that effort will be necessary or even legal. The requestor
> will react to the redirection with another Get(ClassPortInfo) to the
> redirection target, which will reply with its own RespTimeValue, so our
> driver should speak for itself.


I overreached with my comment on how this works.

 Since we don't know when our MAD
> processing and sending of the response is going to be scheduled (we're not
> running on real-time constraints here), we play it safe and return 18,
> which amounts to roughly a second.
>
> Make sense?


I don't think it should be hard coded. IMO it would be better to default to
18 and somehow able to be adjusted (via a (dynamic) module parameter ?).

-- Hal


> Regards
> Joachim
>

[-- Attachment #2: Type: text/html, Size: 2122 bytes --]

Re: [ewg] [PATCH] IB/ehca: Construct MAD redirect replies from request MAD

[Joachim Fenkes]

Hal Rosenstock <hal.rosenstock@gmail.com> wrote on 27.08.2009 15:31:40:

> I don't think it should be hard coded. IMO it would be better to default 
to 18
> and somehow able to be adjusted (via a (dynamic) module parameter ?).

I don't see how making this a parameter would benefit any end user, while 
on the other hand it clutters up our parameter list.

Changing RespTimeValue won't influence the IB performance or user-visible 
behavior of our driver in any way, and in fact, all RespTimeValue says is 
"Please use a timeout of one second for all future MADs you send me", only 
there won't be any more MADs in the future because we just redirected the 
client to someone else. So, the RespTimeValue field is a don't care in the 
redirection scenario. Setting it to an arbitrary, but legal value isn't 
much more than a concession towards any broken clients that may be out 
there.

Given that you seem to like the rest of the code and Jason hasn't spoken 
up yet, I think we can have Roland merge this patch. Roland, what do you 
think?

Regards,
  Joachim

Re: [ewg] [PATCH] IB/ehca: Construct MAD redirect replies from request MAD

[Roland Dreier]

 > Given that you seem to like the rest of the code and Jason hasn't spoken 
 > up yet, I think we can have Roland merge this patch. Roland, what do you 
 > think?

I don't see any problem with the idea and this does sound like a step
forward, so I am planning on merging this (pending review).

Re: [PATCH] IB/ehca: Construct MAD redirect replies from request MAD

[Roland Dreier]

this seems reasonable to me, applied, thanks.