On Tue, 15 Dec 2009 19:03:17 +0200 Sasha Khapyorsky <sas...@voltaire.com> 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 <sas...@voltaire.com> > > > > 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 wei...@llnl.gov -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html