| Message ID | 5523CF0A.5090206@profitbricks.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Apr 07, 2015 at 02:35:22PM +0200, Michael Wang wrote: > index f704254..4e61104 100644 > +++ b/drivers/infiniband/core/sa_query.c > @@ -540,7 +540,7 @@ int ib_init_ah_from_path(struct ib_device *device, u8 port_num, > ah_attr->port_num = port_num; > ah_attr->static_rate = rec->rate; > > - force_grh = rdma_port_get_link_layer(device, port_num) == IB_LINK_LAYER_ETHERNET; > + force_grh = !rdma_transport_ib(device, port_num); Maybe these tests should be called cap_mandatory_grh - but I'm not really sure how iWarp uses the GRH fields in the AH... Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] > Sent: Tuesday, April 07, 2015 3:13 PM > To: Michael Wang > Cc: Roland Dreier; Sean Hefty; linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org; linux-nfs@vger.kernel.org; > netdev@vger.kernel.org; Hal Rosenstock; Tom Tucker; Steve Wise; Hoang-Nam Nguyen; Christoph Raisch; Mike Marciniszyn; Eli Cohen; > Faisal Latif; Upinder Malhi; Trond Myklebust; J. Bruce Fields; David S. Miller; Ira Weiny; PJ Waskiewicz; Tatyana Nikolova; Or Gerlitz; Jack > Morgenstein; Haggai Eran; Ilya Nelkenbaum; Yann Droneaud; Bart Van Assche; Shachar Raindel; Sagi Grimberg; Devesh Sharma; Matan > Barak; Moni Shoua; Jiri Kosina; Selvin Xavier; Mitesh Ahuja; Li RongQing; Rasmus Villemoes; Alex Estrin; Doug Ledford; Eric Dumazet; Erez > Shitrit; Tom Gundersen; Chuck Lever > Subject: Re: [PATCH v2 10/17] IB/Verbs: Adopt management helpers for IB helpers > > On Tue, Apr 07, 2015 at 02:35:22PM +0200, Michael Wang wrote: > > index f704254..4e61104 100644 > > +++ b/drivers/infiniband/core/sa_query.c > > @@ -540,7 +540,7 @@ int ib_init_ah_from_path(struct ib_device *device, u8 port_num, > > ah_attr->port_num = port_num; > > ah_attr->static_rate = rec->rate; > > > > - force_grh = rdma_port_get_link_layer(device, port_num) == IB_LINK_LAYER_ETHERNET; > > + force_grh = !rdma_transport_ib(device, port_num); > > Maybe these tests should be called cap_mandatory_grh - but I'm not > really sure how iWarp uses the GRH fields in the AH... > iWARP runs on top of TCP...this SA code is all IB-specific. The reason it was checking for ETHERNET, I think, is for RoCE. So this change is totally incorrect, I think, because RoCE is an IB transport, but it runs on ETHERNET. Steve. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > index f704254..4e61104 100644 > > +++ b/drivers/infiniband/core/sa_query.c > > @@ -540,7 +540,7 @@ int ib_init_ah_from_path(struct ib_device *device, > u8 port_num, > > ah_attr->port_num = port_num; > > ah_attr->static_rate = rec->rate; > > > > - force_grh = rdma_port_get_link_layer(device, port_num) == > IB_LINK_LAYER_ETHERNET; > > + force_grh = !rdma_transport_ib(device, port_num); > > Maybe these tests should be called cap_mandatory_grh - but I'm not > really sure how iWarp uses the GRH fields in the AH... AH are used with unconnected endpoints, which iWarp doesn't currently support. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/07/2015 08:40 PM, Hefty, Sean wrote: [snip] >> @@ -200,11 +200,9 @@ int ib_init_ah_from_wc(struct ib_device *device, u8 >> port_num, struct ib_wc *wc, >> u32 flow_class; >> u16 gid_index; >> int ret; >> - int is_eth = (rdma_port_get_link_layer(device, port_num) == >> - IB_LINK_LAYER_ETHERNET); >> >> memset(ah_attr, 0, sizeof *ah_attr); >> - if (is_eth) { >> + if (!rdma_transport_ib(device, port_num)) { >> if (!(wc->wc_flags & IB_WC_GRH)) >> return -EPROTOTYPE; >> >> @@ -873,7 +871,7 @@ int ib_resolve_eth_l2_attrs(struct ib_qp *qp, >> union ib_gid sgid; >> >> if ((*qp_attr_mask & IB_QP_AV) && >> - (rdma_port_get_link_layer(qp->device, qp_attr->ah_attr.port_num) >> == IB_LINK_LAYER_ETHERNET)) { >> + (!rdma_transport_ib(qp->device, qp_attr->ah_attr.port_num))) { >> ret = ib_query_gid(qp->device, qp_attr->ah_attr.port_num, >> qp_attr->ah_attr.grh.sgid_index, &sgid); >> if (ret) > > The above checks would be better as: > > force_grh = rdma_transport_iboe(...) > > They are RoCE/IBoE specific checks. Got it, will be in next version :-) Regards, Michael Wang > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Steve Thanks for the comment :-) On 04/07/2015 10:16 PM, Steve Wise wrote: [snip] >>> >>> - force_grh = rdma_port_get_link_layer(device, port_num) == IB_LINK_LAYER_ETHERNET; >>> + force_grh = !rdma_transport_ib(device, port_num); >> >> Maybe these tests should be called cap_mandatory_grh - but I'm not >> really sure how iWarp uses the GRH fields in the AH... >> > > iWARP runs on top of TCP...this SA code is all IB-specific. The reason it was checking for ETHERNET, I think, is for RoCE. So > this change is totally incorrect, I think, because RoCE is an IB transport, but it runs on ETHERNET. I guess it's the name 'transport' which confusing folks... actually (!rdma_transport_ib) including RoCE/IBoE, but yes, here it's for IBoE only, so let's change it to rdma_transport_iboe ;-) Regards, Michael Wang > > Steve. > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 07, 2015 at 03:16:30PM -0500, Steve Wise wrote: > > > > -----Original Message----- > > From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] > > Sent: Tuesday, April 07, 2015 3:13 PM > > To: Michael Wang > > Cc: Roland Dreier; Sean Hefty; linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org; linux-nfs@vger.kernel.org; > > netdev@vger.kernel.org; Hal Rosenstock; Tom Tucker; Steve Wise; Hoang-Nam Nguyen; Christoph Raisch; Mike Marciniszyn; Eli Cohen; > > Faisal Latif; Upinder Malhi; Trond Myklebust; J. Bruce Fields; David S. Miller; Ira Weiny; PJ Waskiewicz; Tatyana Nikolova; Or > Gerlitz; Jack > > Morgenstein; Haggai Eran; Ilya Nelkenbaum; Yann Droneaud; Bart Van Assche; Shachar Raindel; Sagi Grimberg; Devesh Sharma; Matan > > Barak; Moni Shoua; Jiri Kosina; Selvin Xavier; Mitesh Ahuja; Li RongQing; Rasmus Villemoes; Alex Estrin; Doug Ledford; Eric > Dumazet; Erez > > Shitrit; Tom Gundersen; Chuck Lever > > Subject: Re: [PATCH v2 10/17] IB/Verbs: Adopt management helpers for IB helpers > > > > On Tue, Apr 07, 2015 at 02:35:22PM +0200, Michael Wang wrote: > > > index f704254..4e61104 100644 > > > +++ b/drivers/infiniband/core/sa_query.c > > > @@ -540,7 +540,7 @@ int ib_init_ah_from_path(struct ib_device *device, u8 port_num, > > > ah_attr->port_num = port_num; > > > ah_attr->static_rate = rec->rate; > > > > > > - force_grh = rdma_port_get_link_layer(device, port_num) == IB_LINK_LAYER_ETHERNET; > > > + force_grh = !rdma_transport_ib(device, port_num); > > > > Maybe these tests should be called cap_mandatory_grh - but I'm not > > really sure how iWarp uses the GRH fields in the AH... > > > > iWARP runs on top of TCP...this SA code is all IB-specific. The reason it was checking for ETHERNET, I think, is for RoCE. So > this change is totally incorrect, I think, because RoCE is an IB transport, but it runs on ETHERNET. But RoCE does not have an SA? Looks like ib_init_ah_from_path was overloaded to handle non-standard "path records". It seems like the correct functionality would be to use ib_init_ah_from_path() for true SA PathRecords and have another call iboe_init_ah() wrap ib_init_ah_from_path() when RoCE address information is needed in the AH. For Michaels patches I think force_grh = rdma_device_is_iboe(...) is the logic we need here. Ira > > Steve. > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c index f704254..4e61104 100644 --- a/drivers/infiniband/core/sa_query.c +++ b/drivers/infiniband/core/sa_query.c @@ -540,7 +540,7 @@ int ib_init_ah_from_path(struct ib_device *device, u8 port_num, ah_attr->port_num = port_num; ah_attr->static_rate = rec->rate; - force_grh = rdma_port_get_link_layer(device, port_num) == IB_LINK_LAYER_ETHERNET; + force_grh = !rdma_transport_ib(device, port_num); if (rec->hop_limit > 1 || force_grh) { ah_attr->ah_flags = IB_AH_GRH; diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c index 83370de..ca06f76 100644 --- a/drivers/infiniband/core/verbs.c +++ b/drivers/infiniband/core/verbs.c @@ -200,11 +200,9 @@ int ib_init_ah_from_wc(struct ib_device *device, u8 port_num, struct ib_wc *wc, u32 flow_class; u16 gid_index; int ret; - int is_eth = (rdma_port_get_link_layer(device, port_num) == - IB_LINK_LAYER_ETHERNET); memset(ah_attr, 0, sizeof *ah_attr); - if (is_eth) { + if (!rdma_transport_ib(device, port_num)) { if (!(wc->wc_flags & IB_WC_GRH)) return -EPROTOTYPE; @@ -873,7 +871,7 @@ int ib_resolve_eth_l2_attrs(struct ib_qp *qp, union ib_gid sgid; if ((*qp_attr_mask & IB_QP_AV) && - (rdma_port_get_link_layer(qp->device, qp_attr->ah_attr.port_num) == IB_LINK_LAYER_ETHERNET)) { + (!rdma_transport_ib(qp->device, qp_attr->ah_attr.port_num))) { ret = ib_query_gid(qp->device, qp_attr->ah_attr.port_num, qp_attr->ah_attr.grh.sgid_index, &sgid); if (ret)
Adopt management helpers for: ib_init_ah_from_path() ib_init_ah_from_wc() ib_resolve_eth_l2_attrs() Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Cc: Doug Ledford <dledford@redhat.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Sean Hefty <sean.hefty@intel.com> Signed-off-by: Michael Wang <yun.wang@profitbricks.com> --- drivers/infiniband/core/sa_query.c | 2 +- drivers/infiniband/core/verbs.c | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-)