SRP issues with OpenSM 3.3.3

SRP issues with OpenSM 3.3.3

[Ira Weiny]

Sasha, Hal,

I have found that the following patch caused our SRP connected storage to
break.

patch: 3d20f82edd3246879063b77721d0bcef927bdc48

    opensm/osm_sa_path_record.c: separate router guid resolution code
    
    Move off subnet destination (router address) resolution code to separate
    function to improve readability.
    
    Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>

I further traced the problem to pr_rcv_build_pr and the patch below fixes the
bug.

16:03:34 > git diff 
diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
index be0cd71..32c889f 100644
--- a/opensm/opensm/osm_sa_path_record.c
+++ b/opensm/opensm/osm_sa_path_record.c
@@ -800,7 +800,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
 
        /* Only set HopLimit if going through a router */
        if (is_nonzero_gid)
-               p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
+               p_pr->hop_flow_raw |= IB_HOPLIMIT_MAX;
 
        p_pr->pkey = p_parms->pkey;
        ib_path_rec_set_sl(p_pr, p_parms->sl);


"hop_flow_raw" is not really a 32bit value but rather 4 fields:
Component     Length(bits)   Offset(bits)
RawTraffic    1              352
Reserved      3              353
FlowLabel     20             356
HopLimit      8              384


However, I don't understand the comment "Only set HopLimit if going through a
router"?

Was the intent to only set p_dgid in pr_rcv_get_end_points if we are heading
through a router?  I don't think it matters if we are going through a router
or not does it?  If not, I think the comment needs to be removed in the patch
above.

Thanks,
Ira

-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Hal Rosenstock]

Ira,

On Mon, Dec 14, 2009 at 7:43 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
> Sasha, Hal,
>
> I have found that the following patch caused our SRP connected storage to
> break.

What is causing the SRP target to fail ? Is it a non zero hop limit
returned in the SA PathRecord ?

> patch: 3d20f82edd3246879063b77721d0bcef927bdc48
>
>    opensm/osm_sa_path_record.c: separate router guid resolution code
>
>    Move off subnet destination (router address) resolution code to separate
>    function to improve readability.
>
>    Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>
> I further traced the problem to pr_rcv_build_pr and the patch below fixes the
> bug.

On quick glance, I'm missing the connection between Sasha's patch and
this routine setting something bad in the hop limit of the returned
path record.

> 16:03:34 > git diff
> diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> index be0cd71..32c889f 100644
> --- a/opensm/opensm/osm_sa_path_record.c
> +++ b/opensm/opensm/osm_sa_path_record.c
> @@ -800,7 +800,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
>
>        /* Only set HopLimit if going through a router */
>        if (is_nonzero_gid)
> -               p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
> +               p_pr->hop_flow_raw |= IB_HOPLIMIT_MAX;

This may fix this issue but it doesn't look right to me and I think
would break router scenarios and corrupts other field(s). Path record
fields are in network (not host) order.

>        p_pr->pkey = p_parms->pkey;
>        ib_path_rec_set_sl(p_pr, p_parms->sl);
>
>
> "hop_flow_raw" is not really a 32bit value but rather 4 fields:
> Component     Length(bits)   Offset(bits)
> RawTraffic    1              352
> Reserved      3              353
> FlowLabel     20             356
> HopLimit      8              384
>
>
> However, I don't understand the comment "Only set HopLimit if going through a
> router"?

Well, in a sense the HopLimit is always set. It's already set to 0
right above that code snippet in the patch where:
p_pr->hop_flow_raw &= cl_hton32(1 << 31);
This code is only setting the HopLimit to 255 (as this is very
primitive so far) when going through a router. Maybe it could be
stated better.

> Was the intent to only set p_dgid in pr_rcv_get_end_points if we are heading
> through a router? I don't think it matters if we are going through a router
> or not does it?

It's used when DGID is selected in the SA PR query via the comp mask
and in that instance is used for both the router and non router DGID
cases.

> If not, I think the comment needs to be removed in the patch above.

I'm not following why you say this.

-- Hal

> Thanks,
> Ira
>
> --
> Ira Weiny
> Math Programmer/Computer Scientist
> Lawrence Livermore National Lab
> 925-423-8008
> weiny2-i2BcT+NCU+M@public.gmane.org
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Sasha Khapyorsky]

Hi Ira,

On 16:43 Mon 14 Dec     , Ira Weiny wrote:
> 
> I have found that the following patch caused our SRP connected storage to
> break.
> 
> patch: 3d20f82edd3246879063b77721d0bcef927bdc48
> 
>     opensm/osm_sa_path_record.c: separate router guid resolution code
>     
>     Move off subnet destination (router address) resolution code to separate
>     function to improve readability.
>     
>     Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
> 
> I further traced the problem to pr_rcv_build_pr and the patch below fixes the
> bug.

Nice catch and great investigation!

> 
> 16:03:34 > git diff 
> diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> index be0cd71..32c889f 100644
> --- a/opensm/opensm/osm_sa_path_record.c
> +++ b/opensm/opensm/osm_sa_path_record.c
> @@ -800,7 +800,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
>  
>         /* Only set HopLimit if going through a router */
>         if (is_nonzero_gid)
> -               p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
> +               p_pr->hop_flow_raw |= IB_HOPLIMIT_MAX;
>  
>         p_pr->pkey = p_parms->pkey;
>         ib_path_rec_set_sl(p_pr, p_parms->sl);
> 
> 
> "hop_flow_raw" is not really a 32bit value but rather 4 fields:
> Component     Length(bits)   Offset(bits)
> RawTraffic    1              352
> Reserved      3              353
> FlowLabel     20             356
> HopLimit      8              384

This patch doesn't look correct for me. Instead of placing
IB_HOPLIMIT_MAX in proper place this will put it in RawTraffic and
Reserved fields on little-endian machines (such as x86). And this
changes nothing on a big-endian machine. Right?

Following your investigation it seems that SRP target breaks when
IB_HOPLIMIT_MAX is set in SA PR query responses for intra-subnet DGIDs.

> However, I don't understand the comment "Only set HopLimit if going through a
> router"?

This is from '#ifdef ROUTER_EXP' days - as far as I could understand
HopLimit should be set to IB_HOPLIMIT_MAX for inter-subnet MADs.

> 
> Was the intent to only set p_dgid in pr_rcv_get_end_points if we are heading
> through a router?

Seems that this was an original logic.

> I don't think it matters if we are going through a router
> or not does it?

I thought so too, but as we can see it triggers the bug you discovered.

So immediate fix for this is to move p_dgid setup back to router
section, like this:


diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
index 7855be5..fbeef03 100644
--- a/opensm/opensm/osm_sa_path_record.c
+++ b/opensm/opensm/osm_sa_path_record.c
@@ -1236,6 +1236,8 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
 				sa_status = IB_SA_MAD_STATUS_INVALID_GID;
 				goto Exit;
 			}
+			if (p_dgid)
+				*p_dgid = p_pr->dgid;
 		} else
 			dest_guid = p_pr->dgid.unicast.interface_id;
 
@@ -1253,9 +1255,6 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
 			sa_status = IB_SA_MAD_STATUS_INVALID_GID;
 			goto Exit;
 		}
-
-		if (p_dgid)
-			*p_dgid = p_pr->dgid;
 	} else {
 		*pp_dest_port = 0;
 		if (comp_mask & IB_PR_COMPMASK_DLID) {


BTW, could you test this? Unfortunately I don't have SRP target :(

And seems that as more solid solution we need to cleanup some logic in
osm_sa_path_record.c code.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Ira Weiny]

On Tue, 15 Dec 2009 10:16:42 -0500
Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Ira,
> 
> On Mon, Dec 14, 2009 at 7:43 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
> > Sasha, Hal,
> >
> > I have found that the following patch caused our SRP connected storage to
> > break.
> 
> What is causing the SRP target to fail ? Is it a non zero hop limit
> returned in the SA PathRecord ?

I am not sure exactly because I don't know what the SRP Target is seeing.
(pesky firmware...)  :-(

> 
> > patch: 3d20f82edd3246879063b77721d0bcef927bdc48
> >
> >    opensm/osm_sa_path_record.c: separate router guid resolution code
> >
> >    Move off subnet destination (router address) resolution code to separate
> >    function to improve readability.
> >
> >    Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
> >
> > I further traced the problem to pr_rcv_build_pr and the patch below fixes the
> > bug.
> 
> On quick glance, I'm missing the connection between Sasha's patch and
> this routine setting something bad in the hop limit of the returned
> path record.

It sets p_dgid even when we are not using a router which results in
is_nonzero_gid being true.

> 
> > 16:03:34 > git diff
> > diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> > index be0cd71..32c889f 100644
> > --- a/opensm/opensm/osm_sa_path_record.c
> > +++ b/opensm/opensm/osm_sa_path_record.c
> > @@ -800,7 +800,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
> >
> >        /* Only set HopLimit if going through a router */
> >        if (is_nonzero_gid)
> > -               p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
> > +               p_pr->hop_flow_raw |= IB_HOPLIMIT_MAX;
> 
> This may fix this issue but it doesn't look right to me and I think
> would break router scenarios and corrupts other field(s). Path record
> fields are in network (not host) order.

But HopLimit is an 8bit field?  I might be wrong on this but I was thinking
this set RawTraffic and parts of Flow label to 1's.  I should double check
that.  Also, I wonder if the FW on the other side is flipping this wrong?  :-/
I have not been able to determine this and I still have to test with our other
vendor's SRP target...

> 
> >        p_pr->pkey = p_parms->pkey;
> >        ib_path_rec_set_sl(p_pr, p_parms->sl);
> >
> >
> > "hop_flow_raw" is not really a 32bit value but rather 4 fields:
> > Component     Length(bits)   Offset(bits)
> > RawTraffic    1              352
> > Reserved      3              353
> > FlowLabel     20             356
> > HopLimit      8              384
> >
> >
> > However, I don't understand the comment "Only set HopLimit if going through a
> > router"?
> 
> Well, in a sense the HopLimit is always set. It's already set to 0
> right above that code snippet in the patch where:
> p_pr->hop_flow_raw &= cl_hton32(1 << 31);

True, however, I believe the behavior before was that the entire field is 0
so the above statement really does nothing.

> This code is only setting the HopLimit to 255 (as this is very
> primitive so far) when going through a router. Maybe it could be
> stated better.

That is exactly what Sasha's patch changes.  This is not only executed when
going through a router but at all times DGID is specified in the CompMask.  I
am willing to accept the fact that perhaps p_dgid should not be set unless we
find a router, however, I __don't__ think that is correct.  Searching the spec
I attempted to find where HopLimit was supposed to be 0xFF only when going
through a router and I could not find it; therefore my email, I'm confused
again...  :-(

> 
> > Was the intent to only set p_dgid in pr_rcv_get_end_points if we are heading
> > through a router? I don't think it matters if we are going through a router
> > or not does it?
> 
> It's used when DGID is selected in the SA PR query via the comp mask
> and in that instance is used for both the router and non router DGID
> cases.

So we agree that Sasha's patch is correct?

> 
> > If not, I think the comment needs to be removed in the patch above.
> 
> I'm not following why you say this.

Well, if HopLimit must only be set to 0xff when going through a router and
Sasha's patch is correct, then I think we need another way to determine if
this path is going through a router.  In that case the comment is correct but
the logic is wrong.

Ira

> 
> -- Hal
> 
> > Thanks,
> > Ira
> >
> > --
> > Ira Weiny
> > Math Programmer/Computer Scientist
> > Lawrence Livermore National Lab
> > 925-423-8008
> > weiny2-i2BcT+NCU+M@public.gmane.org
> >


-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Sasha Khapyorsky]

On 10:16 Tue 15 Dec     , Hal Rosenstock wrote:
> 
> > patch: 3d20f82edd3246879063b77721d0bcef927bdc48
> >
> >    opensm/osm_sa_path_record.c: separate router guid resolution code
> >
> >    Move off subnet destination (router address) resolution code to separate
> >    function to improve readability.
> >
> >    Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
> >
> > I further traced the problem to pr_rcv_build_pr and the patch below fixes the
> > bug.
> 
> On quick glance, I'm missing the connection between Sasha's patch and
> this routine setting something bad in the hop limit of the returned
> path record.

In this patch I moved *p_dgid initialization out from router only
section, as side effect it turns on this section:

	/* Only set HopLimit if going through a router */
	if (is_nonzero_gid)
		p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);

for all SA PR queries where DGID was selected (local subnet or not).

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Ira Weiny]

On Tue, 15 Dec 2009 19:03:17 +0200
Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:

> Hi Ira,
> 
> On 16:43 Mon 14 Dec     , Ira Weiny wrote:
> > 
> > I have found that the following patch caused our SRP connected storage to
> > break.
> > 
> > patch: 3d20f82edd3246879063b77721d0bcef927bdc48
> > 
> >     opensm/osm_sa_path_record.c: separate router guid resolution code
> >     
> >     Move off subnet destination (router address) resolution code to separate
> >     function to improve readability.
> >     
> >     Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
> > 
> > I further traced the problem to pr_rcv_build_pr and the patch below fixes the
> > bug.
> 
> Nice catch and great investigation!
> 
> > 
> > 16:03:34 > git diff 
> > diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> > index be0cd71..32c889f 100644
> > --- a/opensm/opensm/osm_sa_path_record.c
> > +++ b/opensm/opensm/osm_sa_path_record.c
> > @@ -800,7 +800,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
> >  
> >         /* Only set HopLimit if going through a router */
> >         if (is_nonzero_gid)
> > -               p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
> > +               p_pr->hop_flow_raw |= IB_HOPLIMIT_MAX;
> >  
> >         p_pr->pkey = p_parms->pkey;
> >         ib_path_rec_set_sl(p_pr, p_parms->sl);
> > 
> > 
> > "hop_flow_raw" is not really a 32bit value but rather 4 fields:
> > Component     Length(bits)   Offset(bits)
> > RawTraffic    1              352
> > Reserved      3              353
> > FlowLabel     20             356
> > HopLimit      8              384
> 
> This patch doesn't look correct for me. Instead of placing
> IB_HOPLIMIT_MAX in proper place this will put it in RawTraffic and
> Reserved fields on little-endian machines (such as x86). And this
> changes nothing on a big-endian machine. Right?

Yea I think this is wrong.  As I said perhaps the FW on the other side is
interpreting the fields wrong.  I will check with the vendor.

> 
> Following your investigation it seems that SRP target breaks when
> IB_HOPLIMIT_MAX is set in SA PR query responses for intra-subnet DGIDs.

Yes

> 
> > However, I don't understand the comment "Only set HopLimit if going through a
> > router"?
> 
> This is from '#ifdef ROUTER_EXP' days - as far as I could understand
> HopLimit should be set to IB_HOPLIMIT_MAX for inter-subnet MADs.

But I have not found where that is stated.

> 
> > 
> > Was the intent to only set p_dgid in pr_rcv_get_end_points if we are heading
> > through a router?
> 
> Seems that this was an original logic.
> 
> > I don't think it matters if we are going through a router
> > or not does it?
> 
> I thought so too, but as we can see it triggers the bug you discovered.
> 
> So immediate fix for this is to move p_dgid setup back to router
> section, like this:
> 
> 
> diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> index 7855be5..fbeef03 100644
> --- a/opensm/opensm/osm_sa_path_record.c
> +++ b/opensm/opensm/osm_sa_path_record.c
> @@ -1236,6 +1236,8 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
>  				sa_status = IB_SA_MAD_STATUS_INVALID_GID;
>  				goto Exit;
>  			}
> +			if (p_dgid)
> +				*p_dgid = p_pr->dgid;
>  		} else
>  			dest_guid = p_pr->dgid.unicast.interface_id;
>  
> @@ -1253,9 +1255,6 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
>  			sa_status = IB_SA_MAD_STATUS_INVALID_GID;
>  			goto Exit;
>  		}
> -
> -		if (p_dgid)
> -			*p_dgid = p_pr->dgid;
>  	} else {
>  		*pp_dest_port = 0;
>  		if (comp_mask & IB_PR_COMPMASK_DLID) {
> 
> 
> BTW, could you test this? Unfortunately I don't have SRP target :(

Yes this works and was my original fix.  However, it did not seem right and I
think Hal agrees.

> 
> And seems that as more solid solution we need to cleanup some logic in
> osm_sa_path_record.c code.

I think so.

Ira

> 
> Sasha


-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Jason Gunthorpe]

> > However, I don't understand the comment "Only set HopLimit if going through a
> > router"?
> 
> This is from '#ifdef ROUTER_EXP' days - as far as I could understand
> HopLimit should be set to IB_HOPLIMIT_MAX for inter-subnet MADs.

If HopLimit is 0 then no GRH is required, if it is non 0 then the user
of that path needs to apply a GRH. IIRC there are already a few things
in the kernel that follow this?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Ira Weiny]

On Tue, 15 Dec 2009 10:15:32 -0700
Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:

> > > However, I don't understand the comment "Only set HopLimit if going through a
> > > router"?
> > 
> > This is from '#ifdef ROUTER_EXP' days - as far as I could understand
> > HopLimit should be set to IB_HOPLIMIT_MAX for inter-subnet MADs.
> 
> If HopLimit is 0 then no GRH is required, if it is non 0 then the user
> of that path needs to apply a GRH. IIRC there are already a few things
> in the kernel that follow this?

Is that in the spec somewhere?  The follow on would be that if they specify a
DGID in the PathRecord query and the result is subnet local they must still
use a GRH even if the packet is going to stay local?  I think this makes sense
but I wonder if __everyone__ is following that?

Ira

> 
> Jason


-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Jason Gunthorpe]

On Tue, Dec 15, 2009 at 09:18:19AM -0800, Ira Weiny wrote:
> On Tue, 15 Dec 2009 10:15:32 -0700
> Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> 
> > > > However, I don't understand the comment "Only set HopLimit if going through a
> > > > router"?
> > > 
> > > This is from '#ifdef ROUTER_EXP' days - as far as I could understand
> > > HopLimit should be set to IB_HOPLIMIT_MAX for inter-subnet MADs.
> > 
> > If HopLimit is 0 then no GRH is required, if it is non 0 then the user
> > of that path needs to apply a GRH. IIRC there are already a few things
> > in the kernel that follow this?
> 
> Is that in the spec somewhere?  The follow on would be that if they specify a
> DGID in the PathRecord query and the result is subnet local they must still
> use a GRH even if the packet is going to stay local?  I think this makes sense
> but I wonder if __everyone__ is following that?

The spec provides no guidance whatsoever on how to tell if a packets
generated from a PR need a GRH or not. The only other choice is to
compare the resulting DGID against all the GID prefixs on the port and
see if any match.

If a router spec is ever written I'm sure it will clarify this matter,
and everyone will follow.

The SM decides what hop limit should be so the SM decides if it needs
a GRH. I belive the original long ago version of this always set the
hop limit to 0 unless the DLID was a router port.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Ira Weiny]

On Tue, 15 Dec 2009 10:31:40 -0700
Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:

[snip]

> 
> The spec provides no guidance whatsoever on how to tell if a packets
> generated from a PR need a GRH or not. The only other choice is to
> compare the resulting DGID against all the GID prefixs on the port and
> see if any match.
> 
> If a router spec is ever written I'm sure it will clarify this matter,
> and everyone will follow.
> 
> The SM decides what hop limit should be so the SM decides if it needs
> a GRH. I belive the original long ago version of this always set the
> hop limit to 0 unless the DLID was a router port.

Ok, then I think we need a new way to tell if we are hitting a router port in
pr_rcv_build_pr.  I don't think having a non-zero GID is the definition of
going through a router port.

Ira

> 
> Jason


-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Hal Rosenstock]

On Tue, Dec 15, 2009 at 12:09 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
> On Tue, 15 Dec 2009 10:16:42 -0500
> Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Ira,
>>
>> On Mon, Dec 14, 2009 at 7:43 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
>> > Sasha, Hal,
>> >
>> > I have found that the following patch caused our SRP connected storage to
>> > break.
>>
>> What is causing the SRP target to fail ? Is it a non zero hop limit
>> returned in the SA PathRecord ?
>
> I am not sure exactly because I don't know what the SRP Target is seeing.
> (pesky firmware...)  :-(
>
>>
>> > patch: 3d20f82edd3246879063b77721d0bcef927bdc48
>> >
>> >    opensm/osm_sa_path_record.c: separate router guid resolution code
>> >
>> >    Move off subnet destination (router address) resolution code to separate
>> >    function to improve readability.
>> >
>> >    Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>> >
>> > I further traced the problem to pr_rcv_build_pr and the patch below fixes the
>> > bug.
>>
>> On quick glance, I'm missing the connection between Sasha's patch and
>> this routine setting something bad in the hop limit of the returned
>> path record.
>
> It sets p_dgid even when we are not using a router which results in
> is_nonzero_gid being true.

Yes, that's the cause of the issue.

>>
>> > 16:03:34 > git diff
>> > diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
>> > index be0cd71..32c889f 100644
>> > --- a/opensm/opensm/osm_sa_path_record.c
>> > +++ b/opensm/opensm/osm_sa_path_record.c
>> > @@ -800,7 +800,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
>> >
>> >        /* Only set HopLimit if going through a router */
>> >        if (is_nonzero_gid)
>> > -               p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
>> > +               p_pr->hop_flow_raw |= IB_HOPLIMIT_MAX;
>>
>> This may fix this issue but it doesn't look right to me and I think
>> would break router scenarios and corrupts other field(s). Path record
>> fields are in network (not host) order.
>
> But HopLimit is an 8bit field?

Yes but in terms of the ib_path_rec_t definition hop_flow_raw is a 32 bit field.

> I might be wrong on this but I was thinking
> this set RawTraffic and parts of Flow label to 1's.  I should double check
> that.  Also, I wonder if the FW on the other side is flipping this wrong?  :-/
> I have not been able to determine this and I still have to test with our other
> vendor's SRP target...
>
>>
>> >        p_pr->pkey = p_parms->pkey;
>> >        ib_path_rec_set_sl(p_pr, p_parms->sl);
>> >
>> >
>> > "hop_flow_raw" is not really a 32bit value but rather 4 fields:
>> > Component     Length(bits)   Offset(bits)
>> > RawTraffic    1              352
>> > Reserved      3              353
>> > FlowLabel     20             356
>> > HopLimit      8              384
>> >
>> >
>> > However, I don't understand the comment "Only set HopLimit if going through a
>> > router"?
>>
>> Well, in a sense the HopLimit is always set. It's already set to 0
>> right above that code snippet in the patch where:
>> p_pr->hop_flow_raw &= cl_hton32(1 << 31);
>
> True, however, I believe the behavior before was that the entire field is 0
> so the above statement really does nothing.

For all practical purposes that is true as raw traffic is deprecated.

>> This code is only setting the HopLimit to 255 (as this is very
>> primitive so far) when going through a router. Maybe it could be
>> stated better.
>
> That is exactly what Sasha's patch changes.  This is not only executed when
> going through a router but at all times DGID is specified in the CompMask.  I
> am willing to accept the fact that perhaps p_dgid should not be set unless we
> find a router, however, I __don't__ think that is correct.  Searching the spec
> I attempted to find where HopLimit was supposed to be 0xFF only when going
> through a router and I could not find it; therefore my email, I'm confused
> again...  :-(

When going through a router, it needs to be set to something other
than 0 or 1. 255 (max) was used as there's nothing currently to scope
it down to something more realistic. See IBA 1.2.1 vol 1 p.229 8.3.6
for one on settings for this field.

>>
>> > Was the intent to only set p_dgid in pr_rcv_get_end_points if we are heading
>> > through a router? I don't think it matters if we are going through a router
>> > or not does it?
>>
>> It's used when DGID is selected in the SA PR query via the comp mask
>> and in that instance is used for both the router and non router DGID
>> cases.
>
> So we agree that Sasha's patch is correct?

Yes; it restores this back the way it used to be when it worked.

-- Hal

>>
>> > If not, I think the comment needs to be removed in the patch above.
>>
>> I'm not following why you say this.
>
> Well, if HopLimit must only be set to 0xff when going through a router and
> Sasha's patch is correct, then I think we need another way to determine if
> this path is going through a router.  In that case the comment is correct but
> the logic is wrong.
>
> Ira
>
>>
>> -- Hal
>>
>> > Thanks,
>> > Ira
>> >
>> > --
>> > Ira Weiny
>> > Math Programmer/Computer Scientist
>> > Lawrence Livermore National Lab
>> > 925-423-8008
>> > weiny2-i2BcT+NCU+M@public.gmane.org
>> >
>
>
> --
> Ira Weiny
> Math Programmer/Computer Scientist
> Lawrence Livermore National Lab
> 925-423-8008
> weiny2-i2BcT+NCU+M@public.gmane.org
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Hal Rosenstock]

On Tue, Dec 15, 2009 at 12:15 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
>> > However, I don't understand the comment "Only set HopLimit if going through a
>> > router"?
>>
>> This is from '#ifdef ROUTER_EXP' days - as far as I could understand
>> HopLimit should be set to IB_HOPLIMIT_MAX for inter-subnet MADs.
>
> If HopLimit is 0 then no GRH is required, if it is non 0 then the user

I think it is:
HopLimit 0 or 1 requires no GRH (GRH is optional in these cases)
and
other than 0 or 1 requires GRH

> of that path needs to apply a GRH. IIRC there are already a few things
> in the kernel that follow this?

Yes, we started down this path. This was all discussed on the list
back prior to SC07 days AFAIR.

-- Hal

> Jason
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SRP issues with OpenSM 3.3.3

[Hal Rosenstock]

On Tue, Dec 15, 2009 at 12:18 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
> On Tue, 15 Dec 2009 10:15:32 -0700
> Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
>
>> > > However, I don't understand the comment "Only set HopLimit if going through a
>> > > router"?
>> >
>> > This is from '#ifdef ROUTER_EXP' days - as far as I could understand
>> > HopLimit should be set to IB_HOPLIMIT_MAX for inter-subnet MADs.
>>
>> If HopLimit is 0 then no GRH is required, if it is non 0 then the user
>> of that path needs to apply a GRH. IIRC there are already a few things
>> in the kernel that follow this?
>
> Is that in the spec somewhere?

IBA 1.2.1 vol 1 p.229 8.3.6 for one.

> The follow on would be that if they specify a
> DGID in the PathRecord query and the result is subnet local they must still
> use a GRH even if the packet is going to stay local?

No; with subnet local traffic, GRH is optional.

-- Hal

>  I think this makes sense
> but I wonder if __everyone__ is following that?
>
> Ira
>
>>
>> Jason
>
>
> --
> Ira Weiny
> Math Programmer/Computer Scientist
> Lawrence Livermore National Lab
> 925-423-8008
> weiny2-i2BcT+NCU+M@public.gmane.org
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH] Set HopLimit based on off subnet Re: SRP issues with OpenSM 3.3.3

[Ira Weiny]

On Tue, 15 Dec 2009 12:59:11 -0500
Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Dec 15, 2009 at 12:18 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:

[snip]

> 
> > The follow on would be that if they specify a
> > DGID in the PathRecord query and the result is subnet local they must still
> > use a GRH even if the packet is going to stay local?
> 
> No; with subnet local traffic, GRH is optional.
> 

Ok, if the GRH is optional with local traffic then I think the appropriate fix
is:

diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
index be0cd71..1fa83a1 100644
--- a/opensm/opensm/osm_sa_path_record.c
+++ b/opensm/opensm/osm_sa_path_record.c
@@ -757,6 +757,14 @@ Exit:
        return (status);
 }
 
+static int gid_is_off_subnet(IN osm_sa_t * sa, IN const ib_gid_t * p_dgid)
+{
+       return (!ib_gid_is_link_local(p_dgid) &&
+               !ib_gid_is_multicast(p_dgid) &&
+               ib_gid_get_subnet_prefix(p_dgid) !=
+               sa->p_subn->opt.subnet_prefix);
+}
+
 /**********************************************************************
  **********************************************************************/
 static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
@@ -770,16 +778,12 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
 {
        const osm_physp_t *p_src_physp;
        const osm_physp_t *p_dest_physp;
-       boolean_t is_nonzero_gid = 0;
 
        OSM_LOG_ENTER(sa->p_log);
 
        p_src_physp = p_src_port->p_physp;
 
        if (p_dgid)
-               is_nonzero_gid = ib_gid_is_notzero(p_dgid);
-
-       if (is_nonzero_gid)
                p_pr->dgid = *p_dgid;
        else {
                p_dest_physp = p_dest_port->p_physp;
@@ -799,7 +803,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
        p_pr->hop_flow_raw &= cl_hton32(1 << 31);
 
        /* Only set HopLimit if going through a router */
-       if (is_nonzero_gid)
+       if (gid_is_off_subnet(sa, &p_pr->dgid))
                p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
 
        p_pr->pkey = p_parms->pkey;
@@ -1248,10 +1252,7 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
                memset(p_dgid, 0, sizeof(*p_dgid));
 
        if (comp_mask & IB_PR_COMPMASK_DGID) {
-               if (!ib_gid_is_link_local(&p_pr->dgid) &&
-                   !ib_gid_is_multicast(&p_pr->dgid) &&
-                   ib_gid_get_subnet_prefix(&p_pr->dgid) !=
-                   sa->p_subn->opt.subnet_prefix) {
+               if (gid_is_off_subnet(sa, &p_pr->dgid)) {
                        dest_guid = find_router(sa, p_pr->dgid.unicast.prefix);
                        if (!dest_guid) {
                                char gid_str[INET6_ADDRSTRLEN];


This maintains the p_dgid found in pr_rcv_get_end_points but appropriately
set's the HopLimit when the DGID is off subnet.

Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] Set HopLimit based on off subnet Re: SRP issues with OpenSM 3.3.3

[Hal Rosenstock]

On Tue, Dec 15, 2009 at 9:55 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
> On Tue, 15 Dec 2009 12:59:11 -0500
> Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Tue, Dec 15, 2009 at 12:18 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
>
> [snip]
>
>>
>> > The follow on would be that if they specify a
>> > DGID in the PathRecord query and the result is subnet local they must still
>> > use a GRH even if the packet is going to stay local?
>>
>> No; with subnet local traffic, GRH is optional.
>>
>
> Ok, if the GRH is optional with local traffic then I think the appropriate fix
> is:
>
> diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> index be0cd71..1fa83a1 100644
> --- a/opensm/opensm/osm_sa_path_record.c
> +++ b/opensm/opensm/osm_sa_path_record.c
> @@ -757,6 +757,14 @@ Exit:
>        return (status);
>  }
>
> +static int gid_is_off_subnet(IN osm_sa_t * sa, IN const ib_gid_t * p_dgid)
> +{
> +       return (!ib_gid_is_link_local(p_dgid) &&
> +               !ib_gid_is_multicast(p_dgid) &&
> +               ib_gid_get_subnet_prefix(p_dgid) !=
> +               sa->p_subn->opt.subnet_prefix);
> +}
> +
>  /**********************************************************************
>  **********************************************************************/
>  static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
> @@ -770,16 +778,12 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
>  {
>        const osm_physp_t *p_src_physp;
>        const osm_physp_t *p_dest_physp;
> -       boolean_t is_nonzero_gid = 0;
>
>        OSM_LOG_ENTER(sa->p_log);
>
>        p_src_physp = p_src_port->p_physp;
>
>        if (p_dgid)
> -               is_nonzero_gid = ib_gid_is_notzero(p_dgid);
> -
> -       if (is_nonzero_gid)
>                p_pr->dgid = *p_dgid;
>        else {
>                p_dest_physp = p_dest_port->p_physp;
> @@ -799,7 +803,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
>        p_pr->hop_flow_raw &= cl_hton32(1 << 31);
>
>        /* Only set HopLimit if going through a router */
> -       if (is_nonzero_gid)
> +       if (gid_is_off_subnet(sa, &p_pr->dgid))
>                p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
>
>        p_pr->pkey = p_parms->pkey;
> @@ -1248,10 +1252,7 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
>                memset(p_dgid, 0, sizeof(*p_dgid));
>
>        if (comp_mask & IB_PR_COMPMASK_DGID) {
> -               if (!ib_gid_is_link_local(&p_pr->dgid) &&
> -                   !ib_gid_is_multicast(&p_pr->dgid) &&
> -                   ib_gid_get_subnet_prefix(&p_pr->dgid) !=
> -                   sa->p_subn->opt.subnet_prefix) {
> +               if (gid_is_off_subnet(sa, &p_pr->dgid)) {
>                        dest_guid = find_router(sa, p_pr->dgid.unicast.prefix);
>                        if (!dest_guid) {
>                                char gid_str[INET6_ADDRSTRLEN];
>
>
> This maintains the p_dgid found in pr_rcv_get_end_points but appropriately
> set's the HopLimit when the DGID is off subnet.

Yes, this looks right to me. Would it be better to only determine
gid_is_off_subnet once by saving this in a variable and passing it as
a parameter where needed ?

-- Hal

>
> Ira
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] Set HopLimit based on off subnet Re: SRP issues with OpenSM 3.3.3

[Ira Weiny]

On Wed, 16 Dec 2009 08:29:43 -0500
Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Dec 15, 2009 at 9:55 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:

[snip]

> >
> > diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> > index be0cd71..1fa83a1 100644
> > --- a/opensm/opensm/osm_sa_path_record.c
> > +++ b/opensm/opensm/osm_sa_path_record.c
> > @@ -757,6 +757,14 @@ Exit:
> >        return (status);
> >  }
> >
> > +static int gid_is_off_subnet(IN osm_sa_t * sa, IN const ib_gid_t * p_dgid)
> > +{
> > +       return (!ib_gid_is_link_local(p_dgid) &&
> > +               !ib_gid_is_multicast(p_dgid) &&
> > +               ib_gid_get_subnet_prefix(p_dgid) !=
> > +               sa->p_subn->opt.subnet_prefix);
> > +}
> > +
> >  /**********************************************************************
> >  **********************************************************************/
> >  static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
> > @@ -770,16 +778,12 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
> >  {
> >        const osm_physp_t *p_src_physp;
> >        const osm_physp_t *p_dest_physp;
> > -       boolean_t is_nonzero_gid = 0;
> >
> >        OSM_LOG_ENTER(sa->p_log);
> >
> >        p_src_physp = p_src_port->p_physp;
> >
> >        if (p_dgid)
> > -               is_nonzero_gid = ib_gid_is_notzero(p_dgid);
> > -
> > -       if (is_nonzero_gid)
> >                p_pr->dgid = *p_dgid;
> >        else {
> >                p_dest_physp = p_dest_port->p_physp;
> > @@ -799,7 +803,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
> >        p_pr->hop_flow_raw &= cl_hton32(1 << 31);
> >
> >        /* Only set HopLimit if going through a router */
> > -       if (is_nonzero_gid)
> > +       if (gid_is_off_subnet(sa, &p_pr->dgid))
> >                p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
> >
> >        p_pr->pkey = p_parms->pkey;
> > @@ -1248,10 +1252,7 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
> >                memset(p_dgid, 0, sizeof(*p_dgid));
> >
> >        if (comp_mask & IB_PR_COMPMASK_DGID) {
> > -               if (!ib_gid_is_link_local(&p_pr->dgid) &&
> > -                   !ib_gid_is_multicast(&p_pr->dgid) &&
> > -                   ib_gid_get_subnet_prefix(&p_pr->dgid) !=
> > -                   sa->p_subn->opt.subnet_prefix) {
> > +               if (gid_is_off_subnet(sa, &p_pr->dgid)) {
> >                        dest_guid = find_router(sa, p_pr->dgid.unicast.prefix);
> >                        if (!dest_guid) {
> >                                char gid_str[INET6_ADDRSTRLEN];
> >
> >
> > This maintains the p_dgid found in pr_rcv_get_end_points but appropriately
> > set's the HopLimit when the DGID is off subnet.
> 
> Yes, this looks right to me. Would it be better to only determine
> gid_is_off_subnet once by saving this in a variable and passing it as
> a parameter where needed ?
> 

It might be faster but it would mean a bigger change.  It looks like having
the dgid or p_pr structures in some internal opensm structure to carry that
information along would be best but that is a lot of work.

Do you think this will adversely affect performance on a big system?  I plan
to roll this out internally and we will have a chance to test on bigger
systems here.  But there are larger out there now.

Ira


-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] Set HopLimit based on off subnet Re: SRP issues with OpenSM 3.3.3

[Hal Rosenstock]

On Thu, Dec 17, 2009 at 9:18 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
> On Wed, 16 Dec 2009 08:29:43 -0500
> Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Tue, Dec 15, 2009 at 9:55 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
>
> [snip]
>
>> >
>> > diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
>> > index be0cd71..1fa83a1 100644
>> > --- a/opensm/opensm/osm_sa_path_record.c
>> > +++ b/opensm/opensm/osm_sa_path_record.c
>> > @@ -757,6 +757,14 @@ Exit:
>> >        return (status);
>> >  }
>> >
>> > +static int gid_is_off_subnet(IN osm_sa_t * sa, IN const ib_gid_t * p_dgid)
>> > +{
>> > +       return (!ib_gid_is_link_local(p_dgid) &&
>> > +               !ib_gid_is_multicast(p_dgid) &&
>> > +               ib_gid_get_subnet_prefix(p_dgid) !=
>> > +               sa->p_subn->opt.subnet_prefix);
>> > +}
>> > +
>> >  /**********************************************************************
>> >  **********************************************************************/
>> >  static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
>> > @@ -770,16 +778,12 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
>> >  {
>> >        const osm_physp_t *p_src_physp;
>> >        const osm_physp_t *p_dest_physp;
>> > -       boolean_t is_nonzero_gid = 0;
>> >
>> >        OSM_LOG_ENTER(sa->p_log);
>> >
>> >        p_src_physp = p_src_port->p_physp;
>> >
>> >        if (p_dgid)
>> > -               is_nonzero_gid = ib_gid_is_notzero(p_dgid);
>> > -
>> > -       if (is_nonzero_gid)
>> >                p_pr->dgid = *p_dgid;
>> >        else {
>> >                p_dest_physp = p_dest_port->p_physp;
>> > @@ -799,7 +803,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
>> >        p_pr->hop_flow_raw &= cl_hton32(1 << 31);
>> >
>> >        /* Only set HopLimit if going through a router */
>> > -       if (is_nonzero_gid)
>> > +       if (gid_is_off_subnet(sa, &p_pr->dgid))
>> >                p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
>> >
>> >        p_pr->pkey = p_parms->pkey;
>> > @@ -1248,10 +1252,7 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
>> >                memset(p_dgid, 0, sizeof(*p_dgid));
>> >
>> >        if (comp_mask & IB_PR_COMPMASK_DGID) {
>> > -               if (!ib_gid_is_link_local(&p_pr->dgid) &&
>> > -                   !ib_gid_is_multicast(&p_pr->dgid) &&
>> > -                   ib_gid_get_subnet_prefix(&p_pr->dgid) !=
>> > -                   sa->p_subn->opt.subnet_prefix) {
>> > +               if (gid_is_off_subnet(sa, &p_pr->dgid)) {
>> >                        dest_guid = find_router(sa, p_pr->dgid.unicast.prefix);
>> >                        if (!dest_guid) {
>> >                                char gid_str[INET6_ADDRSTRLEN];
>> >
>> >
>> > This maintains the p_dgid found in pr_rcv_get_end_points but appropriately
>> > set's the HopLimit when the DGID is off subnet.
>>
>> Yes, this looks right to me. Would it be better to only determine
>> gid_is_off_subnet once by saving this in a variable and passing it as
>> a parameter where needed ?
>>
>
> It might be faster but it would mean a bigger change.

Yes.

> It looks like having
> the dgid or p_pr structures in some internal opensm structure to carry that
> information along would be best but that is a lot of work.

It could also be done by passing this as an additional parameter
through the necessary routines.

> Do you think this will adversely affect performance on a big system?

I don't know as I haven't measured the difference in the two
approaches. Just trying to minimize additions to computing PRs.

-- Hal

> I plan
> to roll this out internally and we will have a chance to test on bigger
> systems here.  But there are larger out there now.
>
> Ira
>
>
> --
> Ira Weiny
> Math Programmer/Computer Scientist
> Lawrence Livermore National Lab
> 925-423-8008
> weiny2-i2BcT+NCU+M@public.gmane.org
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] Set HopLimit based on off subnet Re: SRP issues with OpenSM 3.3.3

[Sasha Khapyorsky]

On 18:18 Thu 17 Dec     , Ira Weiny wrote:
> 
> It might be faster but it would mean a bigger change.  It looks like having
> the dgid or p_pr structures in some internal opensm structure to carry that
> information along would be best but that is a lot of work.

Actually now dgid parameter is never used when intra-subnet PR is
requested - maybe those parameters (dgid, is_off_subnet, etc.) could be
merged into some context struct which is initialized once and passed down
to the functions.

Likely some general cleanup of osm_sa_path_record.c is needed to see a
most obvious coding solution.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] opensm/osm_sa_path_record.c: MGID must be specified explicitly

[Sasha Khapyorsky]

IBA 1.2.1 states (Vol.1, 15.2.5.16, p.918) that DGID shell be explicitly
specified when path destination of SA PathRecord query is multicast
group. So ignore all other cases when choosing multicast PR destination.
Other related parameters (mlid) is checked in pr_match_mgrp_attributes()
anyway. As result consolidate many of effectively duplicate code.

Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
---
 opensm/opensm/osm_sa_path_record.c |   88 +++++-------------------------------
 1 files changed, 11 insertions(+), 77 deletions(-)

diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
index fbeef03..082a41d 100644
--- a/opensm/opensm/osm_sa_path_record.c
+++ b/opensm/opensm/osm_sa_path_record.c
@@ -1401,34 +1401,6 @@ static void pr_rcv_process_pair(IN osm_sa_t * sa, IN const osm_madw_t * p_madw,
 	OSM_LOG_EXIT(sa->p_log);
 }
 
-static osm_mgrp_t *pr_get_mgrp(IN osm_sa_t * sa, IN const osm_madw_t * p_madw)
-{
-	ib_path_rec_t *p_pr;
-	const ib_sa_mad_t *p_sa_mad;
-	osm_mgrp_t *mgrp;
-
-	p_sa_mad = osm_madw_get_sa_mad_ptr(p_madw);
-	p_pr = ib_sa_mad_get_payload_ptr(p_sa_mad);
-
-	if (!(p_sa_mad->comp_mask & IB_PR_COMPMASK_DGID)) {
-		OSM_LOG(sa->p_log, OSM_LOG_VERBOSE,
-			"discard multicast target SA PR with wildcarded MGID");
-		return NULL;
-	}
-
-	mgrp = osm_get_mgrp_by_mgid(sa->p_subn, &p_pr->dgid);
-	if (!mgrp) {
-		char gid_str[INET6_ADDRSTRLEN];
-		OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F09: "
-			"No MC group found for PathRecord destination GID %s\n",
-			inet_ntop(AF_INET6, p_pr->dgid.raw, gid_str,
-				  sizeof gid_str));
-		return NULL;
-	}
-
-	return mgrp;
-}
-
 static ib_api_status_t pr_match_mgrp_attributes(IN osm_sa_t * sa,
 						IN const osm_madw_t * p_madw,
 						IN const osm_mgrp_t * p_mgrp)
@@ -1510,47 +1482,11 @@ Exit:
 	return status;
 }
 
-static int pr_rcv_check_mcast_dest(osm_sa_t * sa, IN const osm_madw_t * p_madw)
-{
-	const ib_path_rec_t *p_pr;
-	const ib_sa_mad_t *p_sa_mad;
-	ib_net64_t comp_mask;
-	int is_multicast = 0;
-
-	OSM_LOG_ENTER(sa->p_log);
-
-	p_sa_mad = osm_madw_get_sa_mad_ptr(p_madw);
-	p_pr = (ib_path_rec_t *) ib_sa_mad_get_payload_ptr(p_sa_mad);
-
-	comp_mask = p_sa_mad->comp_mask;
-
-	if (comp_mask & IB_PR_COMPMASK_DGID) {
-		is_multicast = ib_gid_is_multicast(&p_pr->dgid);
-		if (!is_multicast)
-			goto Exit;
-	}
-
-	if (comp_mask & IB_PR_COMPMASK_DLID) {
-		if (cl_ntoh16(p_pr->dlid) >= IB_LID_MCAST_START_HO &&
-		    cl_ntoh16(p_pr->dlid) <= IB_LID_MCAST_END_HO)
-			is_multicast = 1;
-		else if (is_multicast) {
-			OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F12: "
-				"PathRecord request indicates MGID but not MLID\n");
-			is_multicast = -1;
-		}
-	}
-
-Exit:
-	OSM_LOG_EXIT(sa->p_log);
-	return is_multicast;
-}
-
 void osm_pr_rcv_process(IN void *context, IN void *data)
 {
 	osm_sa_t *sa = context;
 	osm_madw_t *p_madw = data;
-	const ib_path_rec_t *p_pr;
+	ib_path_rec_t *p_pr;
 	const ib_sa_mad_t *p_sa_mad;
 	const osm_port_t *p_src_port;
 	const osm_port_t *p_dest_port;
@@ -1558,7 +1494,6 @@ void osm_pr_rcv_process(IN void *context, IN void *data)
 	ib_gid_t dgid;
 	ib_net16_t sa_status;
 	osm_port_t *requester_port;
-	int ret;
 
 	OSM_LOG_ENTER(sa->p_log);
 
@@ -1601,21 +1536,14 @@ void osm_pr_rcv_process(IN void *context, IN void *data)
 	cl_plock_acquire(sa->p_lock);
 
 	/* Handle multicast destinations separately */
-	if ((ret = pr_rcv_check_mcast_dest(sa, p_madw)) < 0) {
-		/* Multicast DGID with unicast DLID */
-		cl_plock_release(sa->p_lock);
-		osm_sa_send_error(sa, p_madw, IB_MAD_STATUS_INVALID_FIELD);
-		goto Exit;
-	}
-
-	if (ret > 0)
+	if ((p_sa_mad->comp_mask & IB_PR_COMPMASK_DGID) &&
+	    ib_gid_is_multicast(&p_pr->dgid));
 		goto McastDest;
 
 	OSM_LOG(sa->p_log, OSM_LOG_DEBUG, "Unicast destination requested\n");
 
 	sa_status = pr_rcv_get_end_points(sa, p_madw, &p_src_port, &p_dest_port,
 					  &dgid);
-
 	if (sa_status == IB_SA_MAD_STATUS_SUCCESS) {
 		/*
 		   What happens next depends on the type of endpoint information
@@ -1660,9 +1588,15 @@ McastDest:
 		uint8_t hop_limit;
 
 		/* First, get the MC info */
-		p_mgrp = pr_get_mgrp(sa, p_madw);
-		if (!p_mgrp)
+		p_mgrp = osm_get_mgrp_by_mgid(sa->p_subn, &p_pr->dgid);
+		if (!p_mgrp) {
+			char gid_str[INET6_ADDRSTRLEN];
+			OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F09: "
+				"No MC group found for PathRecord destination GID %s\n",
+				inet_ntop(AF_INET6, p_pr->dgid.raw, gid_str,
+					  sizeof gid_str));
 			goto Unlock;
+		}
 
 		/* Make sure the rest of the PathRecord matches the MC group attributes */
 		status = pr_match_mgrp_attributes(sa, p_madw, p_mgrp);
-- 
1.6.6.rc4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] osm_sa_path_record.c: separate mutlicast processing code

[Sasha Khapyorsky]

Move multicast destination PR processing into separate function
pr_process_multicast(). As result also merge some duplicated code.

Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
---
 opensm/opensm/osm_sa_path_record.c |  162 +++++++++++++++++------------------
 1 files changed, 79 insertions(+), 83 deletions(-)

diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
index 082a41d..b78225e 100644
--- a/opensm/opensm/osm_sa_path_record.c
+++ b/opensm/opensm/osm_sa_path_record.c
@@ -1402,11 +1402,10 @@ static void pr_rcv_process_pair(IN osm_sa_t * sa, IN const osm_madw_t * p_madw,
 }
 
 static ib_api_status_t pr_match_mgrp_attributes(IN osm_sa_t * sa,
-						IN const osm_madw_t * p_madw,
+						IN const ib_sa_mad_t * sa_mad,
 						IN const osm_mgrp_t * p_mgrp)
 {
 	const ib_path_rec_t *p_pr;
-	const ib_sa_mad_t *p_sa_mad;
 	ib_net64_t comp_mask;
 	const osm_port_t *port;
 	ib_api_status_t status = IB_ERROR;
@@ -1416,10 +1415,9 @@ static ib_api_status_t pr_match_mgrp_attributes(IN osm_sa_t * sa,
 
 	OSM_LOG_ENTER(sa->p_log);
 
-	p_sa_mad = osm_madw_get_sa_mad_ptr(p_madw);
-	p_pr = ib_sa_mad_get_payload_ptr(p_sa_mad);
+	p_pr = ib_sa_mad_get_payload_ptr(sa_mad);
 
-	comp_mask = p_sa_mad->comp_mask;
+	comp_mask = sa_mad->comp_mask;
 
 	/* check that MLID of the MC group matches the PathRecord DLID */
 	if ((comp_mask & IB_PR_COMPMASK_DLID) && p_mgrp->mlid != p_pr->dlid)
@@ -1455,7 +1453,7 @@ static ib_api_status_t pr_match_mgrp_attributes(IN osm_sa_t * sa,
 
 	/* If SubnAdmGet, assume NumbPaths of 1 (1.2 erratum) */
 	if ((comp_mask & IB_PR_COMPMASK_NUMBPATH) &&
-	    (p_sa_mad->method != IB_MAD_METHOD_GET)) {
+	    (sa_mad->method != IB_MAD_METHOD_GET)) {
 		if (ib_path_rec_num_path(p_pr) == 0)
 			goto Exit;
 	}
@@ -1482,6 +1480,77 @@ Exit:
 	return status;
 }
 
+static void pr_process_multicast(osm_sa_t * sa, const ib_sa_mad_t *sa_mad,
+				 cl_qlist_t *list)
+{
+	ib_path_rec_t *pr = ib_sa_mad_get_payload_ptr(sa_mad);
+	osm_mgrp_t *mgrp;
+	ib_api_status_t status;
+	osm_pr_item_t *pr_item;
+	uint32_t flow_label;
+	uint8_t sl, hop_limit;
+
+	OSM_LOG(sa->p_log, OSM_LOG_DEBUG, "Multicast destination requested\n");
+
+	mgrp = osm_get_mgrp_by_mgid(sa->p_subn, &pr->dgid);
+	if (!mgrp) {
+		char gid_str[INET6_ADDRSTRLEN];
+		OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F09: "
+			"No MC group found for PathRecord destination GID %s\n",
+			inet_ntop(AF_INET6, pr->dgid.raw, gid_str,
+				  sizeof gid_str));
+		return;
+	}
+
+	/* Make sure the rest of the PathRecord matches the MC group attributes */
+	status = pr_match_mgrp_attributes(sa, sa_mad, mgrp);
+	if (status != IB_SUCCESS) {
+		OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F19: "
+			"MC group attributes don't match PathRecord request\n");
+		return;
+	}
+
+	pr_item = malloc(sizeof(*pr_item));
+	if (pr_item == NULL) {
+		OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F18: "
+			"Unable to allocate path record for MC group\n");
+		return;
+	}
+	memset(pr_item, 0, sizeof(*pr_item));
+
+	/* Copy PathRecord request into response */
+	pr_item->path_rec = *pr;
+
+	/* Now, use the MC info to cruft up the PathRecord response */
+	pr_item->path_rec.dgid = mgrp->mcmember_rec.mgid;
+	pr_item->path_rec.dlid = mgrp->mcmember_rec.mlid;
+	pr_item->path_rec.tclass = mgrp->mcmember_rec.tclass;
+	pr_item->path_rec.num_path = 1;
+	pr_item->path_rec.pkey = mgrp->mcmember_rec.pkey;
+
+	/* MTU, rate, and packet lifetime should be exactly */
+	pr_item->path_rec.mtu = (2 << 6) | mgrp->mcmember_rec.mtu;
+	pr_item->path_rec.rate = (2 << 6) | mgrp->mcmember_rec.rate;
+	pr_item->path_rec.pkt_life = (2 << 6) | mgrp->mcmember_rec.pkt_life;
+
+	/* SL, Hop Limit, and Flow Label */
+	ib_member_get_sl_flow_hop(mgrp->mcmember_rec.sl_flow_hop,
+				  &sl, &flow_label, &hop_limit);
+	ib_path_rec_set_sl(&pr_item->path_rec, sl);
+	ib_path_rec_set_qos_class(&pr_item->path_rec, 0);
+
+	/* HopLimit is not yet set in non link local MC groups */
+	/* If it were, this would not be needed */
+	if (ib_mgid_get_scope(&mgrp->mcmember_rec.mgid) !=
+	    IB_MC_SCOPE_LINK_LOCAL)
+		hop_limit = IB_HOPLIMIT_MAX;
+
+	pr_item->path_rec.hop_flow_raw =
+	    cl_hton32(hop_limit) | (flow_label << 8);
+
+	cl_qlist_insert_tail(list, &pr_item->list_item);
+}
+
 void osm_pr_rcv_process(IN void *context, IN void *data)
 {
 	osm_sa_t *sa = context;
@@ -1537,8 +1606,10 @@ void osm_pr_rcv_process(IN void *context, IN void *data)
 
 	/* Handle multicast destinations separately */
 	if ((p_sa_mad->comp_mask & IB_PR_COMPMASK_DGID) &&
-	    ib_gid_is_multicast(&p_pr->dgid));
-		goto McastDest;
+	    ib_gid_is_multicast(&p_pr->dgid)) {
+		pr_process_multicast(sa, p_sa_mad, &pr_list);
+		goto Unlock;
+	}
 
 	OSM_LOG(sa->p_log, OSM_LOG_DEBUG, "Unicast destination requested\n");
 
@@ -1575,81 +1646,6 @@ void osm_pr_rcv_process(IN void *context, IN void *data)
 						     &pr_list);
 		}
 	}
-	goto Unlock;
-
-McastDest:
-	OSM_LOG(sa->p_log, OSM_LOG_DEBUG, "Multicast destination requested\n");
-	{
-		osm_mgrp_t *p_mgrp = NULL;
-		ib_api_status_t status;
-		osm_pr_item_t *p_pr_item;
-		uint32_t flow_label;
-		uint8_t sl;
-		uint8_t hop_limit;
-
-		/* First, get the MC info */
-		p_mgrp = osm_get_mgrp_by_mgid(sa->p_subn, &p_pr->dgid);
-		if (!p_mgrp) {
-			char gid_str[INET6_ADDRSTRLEN];
-			OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F09: "
-				"No MC group found for PathRecord destination GID %s\n",
-				inet_ntop(AF_INET6, p_pr->dgid.raw, gid_str,
-					  sizeof gid_str));
-			goto Unlock;
-		}
-
-		/* Make sure the rest of the PathRecord matches the MC group attributes */
-		status = pr_match_mgrp_attributes(sa, p_madw, p_mgrp);
-		if (status != IB_SUCCESS) {
-			OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F19: "
-				"MC group attributes don't match PathRecord request\n");
-			goto Unlock;
-		}
-
-		p_pr_item = malloc(sizeof(*p_pr_item));
-		if (p_pr_item == NULL) {
-			OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F18: "
-				"Unable to allocate path record for MC group\n");
-			goto Unlock;
-		}
-		memset(p_pr_item, 0, sizeof(*p_pr_item));
-
-		/* Copy PathRecord request into response */
-		p_sa_mad = osm_madw_get_sa_mad_ptr(p_madw);
-		p_pr = (ib_path_rec_t *)
-		    ib_sa_mad_get_payload_ptr(p_sa_mad);
-		p_pr_item->path_rec = *p_pr;
-
-		/* Now, use the MC info to cruft up the PathRecord response */
-		p_pr_item->path_rec.dgid = p_mgrp->mcmember_rec.mgid;
-		p_pr_item->path_rec.dlid = p_mgrp->mcmember_rec.mlid;
-		p_pr_item->path_rec.tclass = p_mgrp->mcmember_rec.tclass;
-		p_pr_item->path_rec.num_path = 1;
-		p_pr_item->path_rec.pkey = p_mgrp->mcmember_rec.pkey;
-
-		/* MTU, rate, and packet lifetime should be exactly */
-		p_pr_item->path_rec.mtu = (2 << 6) | p_mgrp->mcmember_rec.mtu;
-		p_pr_item->path_rec.rate = (2 << 6) | p_mgrp->mcmember_rec.rate;
-		p_pr_item->path_rec.pkt_life =
-		    (2 << 6) | p_mgrp->mcmember_rec.pkt_life;
-
-		/* SL, Hop Limit, and Flow Label */
-		ib_member_get_sl_flow_hop(p_mgrp->mcmember_rec.sl_flow_hop,
-					  &sl, &flow_label, &hop_limit);
-		ib_path_rec_set_sl(&p_pr_item->path_rec, sl);
-		ib_path_rec_set_qos_class(&p_pr_item->path_rec, 0);
-
-		/* HopLimit is not yet set in non link local MC groups */
-		/* If it were, this would not be needed */
-		if (ib_mgid_get_scope(&p_mgrp->mcmember_rec.mgid) !=
-		    IB_MC_SCOPE_LINK_LOCAL)
-			hop_limit = IB_HOPLIMIT_MAX;
-
-		p_pr_item->path_rec.hop_flow_raw =
-		    cl_hton32(hop_limit) | (flow_label << 8);
-
-		cl_qlist_insert_tail(&pr_list, &p_pr_item->list_item);
-	}
 
 Unlock:
 	cl_plock_release(sa->p_lock);
-- 
1.6.6.rc4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] osm_sa_path_record.c: use PR DGID by reference

[Sasha Khapyorsky]

When passing an original value of DGID requested (currently only for
inter-subnet PR requests) use it by reference instead of copied value.

Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
---
 opensm/opensm/osm_sa_path_record.c |   27 ++++++++++-----------------
 1 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
index b78225e..4614b6d 100644
--- a/opensm/opensm/osm_sa_path_record.c
+++ b/opensm/opensm/osm_sa_path_record.c
@@ -754,16 +754,12 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
 {
 	const osm_physp_t *p_src_physp;
 	const osm_physp_t *p_dest_physp;
-	boolean_t is_nonzero_gid = 0;
 
 	OSM_LOG_ENTER(sa->p_log);
 
 	p_src_physp = p_src_port->p_physp;
 
 	if (p_dgid)
-		is_nonzero_gid = ib_gid_is_notzero(p_dgid);
-
-	if (is_nonzero_gid)
 		p_pr->dgid = *p_dgid;
 	else {
 		p_dest_physp = p_dest_port->p_physp;
@@ -783,7 +779,7 @@ static void pr_rcv_build_pr(IN osm_sa_t * sa, IN const osm_port_t * p_src_port,
 	p_pr->hop_flow_raw &= cl_hton32(1 << 31);
 
 	/* Only set HopLimit if going through a router */
-	if (is_nonzero_gid)
+	if (p_dgid)
 		p_pr->hop_flow_raw |= cl_hton32(IB_HOPLIMIT_MAX);
 
 	p_pr->pkey = p_parms->pkey;
@@ -1133,7 +1129,7 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
 					IN const osm_madw_t * p_madw,
 					OUT const osm_port_t ** pp_src_port,
 					OUT const osm_port_t ** pp_dest_port,
-					OUT ib_gid_t * p_dgid)
+					OUT const ib_gid_t ** pp_dgid)
 {
 	const ib_path_rec_t *p_pr;
 	const ib_sa_mad_t *p_sa_mad;
@@ -1217,9 +1213,6 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
 		}
 	}
 
-	if (p_dgid)
-		memset(p_dgid, 0, sizeof(*p_dgid));
-
 	if (comp_mask & IB_PR_COMPMASK_DGID) {
 		if (!ib_gid_is_link_local(&p_pr->dgid) &&
 		    !ib_gid_is_multicast(&p_pr->dgid) &&
@@ -1236,8 +1229,8 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
 				sa_status = IB_SA_MAD_STATUS_INVALID_GID;
 				goto Exit;
 			}
-			if (p_dgid)
-				*p_dgid = p_pr->dgid;
+			if (pp_dgid)
+				*pp_dgid = &p_pr->dgid;
 		} else
 			dest_guid = p_pr->dgid.unicast.interface_id;
 
@@ -1560,7 +1553,7 @@ void osm_pr_rcv_process(IN void *context, IN void *data)
 	const osm_port_t *p_src_port;
 	const osm_port_t *p_dest_port;
 	cl_qlist_t pr_list;
-	ib_gid_t dgid;
+	const ib_gid_t *p_dgid = NULL;
 	ib_net16_t sa_status;
 	osm_port_t *requester_port;
 
@@ -1614,7 +1607,7 @@ void osm_pr_rcv_process(IN void *context, IN void *data)
 	OSM_LOG(sa->p_log, OSM_LOG_DEBUG, "Unicast destination requested\n");
 
 	sa_status = pr_rcv_get_end_points(sa, p_madw, &p_src_port, &p_dest_port,
-					  &dgid);
+					  &p_dgid);
 	if (sa_status == IB_SA_MAD_STATUS_SUCCESS) {
 		/*
 		   What happens next depends on the type of endpoint information
@@ -1624,17 +1617,17 @@ void osm_pr_rcv_process(IN void *context, IN void *data)
 			if (p_dest_port)
 				pr_rcv_process_pair(sa, p_madw, requester_port,
 						    p_src_port, p_dest_port,
-						    &dgid, p_sa_mad->comp_mask,
+						    p_dgid, p_sa_mad->comp_mask,
 						    &pr_list);
 			else
 				pr_rcv_process_half(sa, p_madw, requester_port,
-						    p_src_port, NULL, &dgid,
+						    p_src_port, NULL, p_dgid,
 						    p_sa_mad->comp_mask,
 						    &pr_list);
 		} else {
 			if (p_dest_port)
 				pr_rcv_process_half(sa, p_madw, requester_port,
-						    NULL, p_dest_port, &dgid,
+						    NULL, p_dest_port, p_dgid,
 						    p_sa_mad->comp_mask,
 						    &pr_list);
 			else
@@ -1642,7 +1635,7 @@ void osm_pr_rcv_process(IN void *context, IN void *data)
 				   Katie, bar the door!
 				 */
 				pr_rcv_process_world(sa, p_madw, requester_port,
-						     &dgid, p_sa_mad->comp_mask,
+						     p_dgid, p_sa_mad->comp_mask,
 						     &pr_list);
 		}
 	}
-- 
1.6.6.rc4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] opensm/osm_sa_path_record.c: MGID must be specified explicitly

[Hal Rosenstock]

On Tue, Dec 22, 2009 at 6:37 AM, Sasha Khapyorsky <sashak@voltaire.com> wrote:
>
> IBA 1.2.1 states (Vol.1, 15.2.5.16, p.918) that DGID shell be explicitly
> specified when path destination of SA PathRecord query is multicast
> group.

That's informative rather than compliance text. It appears to only
refer to GetTable (and not Get). Also, it appears overly restrictive
to me. For example, DGID is wildcarded but MLID is supplied as DLID
should be honored. The reworked code appears not to support that
AFAIT.

-- Hal

> So ignore all other cases when choosing multicast PR destination.
> Other related parameters (mlid) is checked in pr_match_mgrp_attributes()
> anyway. As result consolidate many of effectively duplicate code.
>
> Signed-off-by: Sasha Khapyorsky <sashak@voltaire.com>
> ---
>  opensm/opensm/osm_sa_path_record.c |   88 +++++-------------------------------
>  1 files changed, 11 insertions(+), 77 deletions(-)
>
> diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> index fbeef03..082a41d 100644
> --- a/opensm/opensm/osm_sa_path_record.c
> +++ b/opensm/opensm/osm_sa_path_record.c
> @@ -1401,34 +1401,6 @@ static void pr_rcv_process_pair(IN osm_sa_t * sa, IN const osm_madw_t * p_madw,
>        OSM_LOG_EXIT(sa->p_log);
>  }
>
> -static osm_mgrp_t *pr_get_mgrp(IN osm_sa_t * sa, IN const osm_madw_t * p_madw)
> -{
> -       ib_path_rec_t *p_pr;
> -       const ib_sa_mad_t *p_sa_mad;
> -       osm_mgrp_t *mgrp;
> -
> -       p_sa_mad = osm_madw_get_sa_mad_ptr(p_madw);
> -       p_pr = ib_sa_mad_get_payload_ptr(p_sa_mad);
> -
> -       if (!(p_sa_mad->comp_mask & IB_PR_COMPMASK_DGID)) {
> -               OSM_LOG(sa->p_log, OSM_LOG_VERBOSE,
> -                       "discard multicast target SA PR with wildcarded MGID");
> -               return NULL;
> -       }
> -
> -       mgrp = osm_get_mgrp_by_mgid(sa->p_subn, &p_pr->dgid);
> -       if (!mgrp) {
> -               char gid_str[INET6_ADDRSTRLEN];
> -               OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F09: "
> -                       "No MC group found for PathRecord destination GID %s\n",
> -                       inet_ntop(AF_INET6, p_pr->dgid.raw, gid_str,
> -                                 sizeof gid_str));
> -               return NULL;
> -       }
> -
> -       return mgrp;
> -}
> -
>  static ib_api_status_t pr_match_mgrp_attributes(IN osm_sa_t * sa,
>                                                IN const osm_madw_t * p_madw,
>                                                IN const osm_mgrp_t * p_mgrp)
> @@ -1510,47 +1482,11 @@ Exit:
>        return status;
>  }
>
> -static int pr_rcv_check_mcast_dest(osm_sa_t * sa, IN const osm_madw_t * p_madw)
> -{
> -       const ib_path_rec_t *p_pr;
> -       const ib_sa_mad_t *p_sa_mad;
> -       ib_net64_t comp_mask;
> -       int is_multicast = 0;
> -
> -       OSM_LOG_ENTER(sa->p_log);
> -
> -       p_sa_mad = osm_madw_get_sa_mad_ptr(p_madw);
> -       p_pr = (ib_path_rec_t *) ib_sa_mad_get_payload_ptr(p_sa_mad);
> -
> -       comp_mask = p_sa_mad->comp_mask;
> -
> -       if (comp_mask & IB_PR_COMPMASK_DGID) {
> -               is_multicast = ib_gid_is_multicast(&p_pr->dgid);
> -               if (!is_multicast)
> -                       goto Exit;
> -       }
> -
> -       if (comp_mask & IB_PR_COMPMASK_DLID) {
> -               if (cl_ntoh16(p_pr->dlid) >= IB_LID_MCAST_START_HO &&
> -                   cl_ntoh16(p_pr->dlid) <= IB_LID_MCAST_END_HO)
> -                       is_multicast = 1;
> -               else if (is_multicast) {
> -                       OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F12: "
> -                               "PathRecord request indicates MGID but not MLID\n");
> -                       is_multicast = -1;
> -               }
> -       }
> -
> -Exit:
> -       OSM_LOG_EXIT(sa->p_log);
> -       return is_multicast;
> -}
> -
>  void osm_pr_rcv_process(IN void *context, IN void *data)
>  {
>        osm_sa_t *sa = context;
>        osm_madw_t *p_madw = data;
> -       const ib_path_rec_t *p_pr;
> +       ib_path_rec_t *p_pr;
>        const ib_sa_mad_t *p_sa_mad;
>        const osm_port_t *p_src_port;
>        const osm_port_t *p_dest_port;
> @@ -1558,7 +1494,6 @@ void osm_pr_rcv_process(IN void *context, IN void *data)
>        ib_gid_t dgid;
>        ib_net16_t sa_status;
>        osm_port_t *requester_port;
> -       int ret;
>
>        OSM_LOG_ENTER(sa->p_log);
>
> @@ -1601,21 +1536,14 @@ void osm_pr_rcv_process(IN void *context, IN void *data)
>        cl_plock_acquire(sa->p_lock);
>
>        /* Handle multicast destinations separately */
> -       if ((ret = pr_rcv_check_mcast_dest(sa, p_madw)) < 0) {
> -               /* Multicast DGID with unicast DLID */
> -               cl_plock_release(sa->p_lock);
> -               osm_sa_send_error(sa, p_madw, IB_MAD_STATUS_INVALID_FIELD);
> -               goto Exit;
> -       }
> -
> -       if (ret > 0)
> +       if ((p_sa_mad->comp_mask & IB_PR_COMPMASK_DGID) &&
> +           ib_gid_is_multicast(&p_pr->dgid));
>                goto McastDest;
>
>        OSM_LOG(sa->p_log, OSM_LOG_DEBUG, "Unicast destination requested\n");
>
>        sa_status = pr_rcv_get_end_points(sa, p_madw, &p_src_port, &p_dest_port,
>                                          &dgid);
> -
>        if (sa_status == IB_SA_MAD_STATUS_SUCCESS) {
>                /*
>                   What happens next depends on the type of endpoint information
> @@ -1660,9 +1588,15 @@ McastDest:
>                uint8_t hop_limit;
>
>                /* First, get the MC info */
> -               p_mgrp = pr_get_mgrp(sa, p_madw);
> -               if (!p_mgrp)
> +               p_mgrp = osm_get_mgrp_by_mgid(sa->p_subn, &p_pr->dgid);
> +               if (!p_mgrp) {
> +                       char gid_str[INET6_ADDRSTRLEN];
> +                       OSM_LOG(sa->p_log, OSM_LOG_ERROR, "ERR 1F09: "
> +                               "No MC group found for PathRecord destination GID %s\n",
> +                               inet_ntop(AF_INET6, p_pr->dgid.raw, gid_str,
> +                                         sizeof gid_str));
>                        goto Unlock;
> +               }
>
>                /* Make sure the rest of the PathRecord matches the MC group attributes */
>                status = pr_match_mgrp_attributes(sa, p_madw, p_mgrp);
> --
> 1.6.6.rc4
>
>