> -----Original Message----- > From: Jason Gunthorpe [mailto:jgg@xxxxxxxx] > Sent: Wednesday, March 21, 2018 4:51 PM > To: Leon Romanovsky <leon@xxxxxxxxxx> > Cc: Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky > <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>; > Mark Bloch <markb@xxxxxxxxxxxx>; Parav Pandit <parav@xxxxxxxxxxxx> > Subject: Re: [PATCH rdma-next 1/2] IB/cma: Resolve route only while receiving > CM requests > > On Wed, Mar 21, 2018 at 05:16:35PM +0200, Leon Romanovsky wrote: > > From: Parav Pandit <parav@xxxxxxxxxxxx> > > > > Currently CM request for RoCE follows following flow. > > rdma_create_id() > > rdma_resolve_addr() > > rdma_resolve_route() > > For RC QPs: > > rdma_connect() > > ->cma_connect_ib() > > ->ib_send_cm_req() > > ->cm_init_av_by_path() > > ->ib_init_ah_attr_from_path() > > For UD QPs: > > rdma_connect() > > ->cma_resolve_ib_udp() > > ->ib_send_cm_sidr_req() > > ->cm_init_av_by_path() > > ->ib_init_ah_attr_from_path() > > > > In both the flows, route is already resolved before sending CM requests. > > Therefore, code is refactored to avoid resolving route second time in > > ib_cm layer. > > ib_init_ah_attr_from_path() is extended to resolve route when it is > > not yet resolved for RoCE link layer. This is achieved by caller > > setting route_resolved field in path record whenever it has route > > already resolved. > > > > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx> > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx> > > drivers/infiniband/core/cm.c | 7 ++++++- > > drivers/infiniband/core/cma.c | 1 + > > drivers/infiniband/core/sa_query.c | 11 ++++++++--- > > include/rdma/ib_sa.h | 8 ++++++++ > > 4 files changed, 23 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/infiniband/core/cm.c > > b/drivers/infiniband/core/cm.c index 4cc0fe6a29ff..71a26dc798b0 100644 > > +++ b/drivers/infiniband/core/cm.c > > @@ -1943,9 +1943,11 @@ static int cm_req_handler(struct cm_work *work) > > work->path[1].rec_type = work->path[0].rec_type; > > cm_format_paths_from_req(req_msg, &work->path[0], > > &work->path[1]); > > - if (cm_id_priv->av.ah_attr.type == RDMA_AH_ATTR_TYPE_ROCE) > > + if (cm_id_priv->av.ah_attr.type == RDMA_AH_ATTR_TYPE_ROCE) { > > sa_path_set_dmac(&work->path[0], > > cm_id_priv->av.ah_attr.roce.dmac); > > + work->path[0].roce.route_resolved = false; > > + } > > Why this hunk? path[0] was memset to 0 up above, don't see any obvious path > that sets route_resolved = true between here and the memset? > Right. It is not needed due to memset. Since CM path appears in general little complicated, setting it explicitly gives more clarity, but otherwise it can be omitted in both of these places. > > work->path[0].hop_limit = grh->hop_limit; > > ret = cm_init_av_by_path(&work->path[0], &cm_id_priv->av, > > cm_id_priv); > > @@ -1967,6 +1969,9 @@ static int cm_req_handler(struct cm_work *work) > > goto rejected; > > } > > if (cm_req_has_alt_path(req_msg)) { > > + if (sa_path_is_roce(&work->path[1])) > > + work->path[1].roce.route_resolved = false; > > + > > ret = cm_init_av_by_path(&work->path[1], &cm_id_priv- > >alt_av, > > cm_id_priv); > > ditto > > > @@ -1327,9 +1329,12 @@ int ib_init_ah_attr_from_path(struct ib_device > *device, u8 port_num, > > rdma_ah_set_static_rate(ah_attr, rec->rate); > > > > if (sa_path_is_roce(rec)) { > > - ret = roce_resolve_route_from_path(device, port_num, rec); > > - if (ret) > > - return ret; > > + if (!rec->roce.route_resolved) { > > + ret = roce_resolve_route_from_path(device, port_num, > > + rec); > > Any particular reason to just not exit from > roce_resolve_route_from_path() if route_resolved? > Do you mean, Can if (!rec->roce.route_resolved) reside inside roce_resolve_route_from_path()? If I understood you right, yes, it can be. But it is only checked once, so placed it outside. But it is ok to have it inside too. init_ah_attr_from_path does more of the ah_attr (after route is resolved), which continues to be needed. > Otherwise seems reasonable to me > > Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
