> -----Original Message----- > From: Jason Gunthorpe [mailto:jgg@xxxxxxxx] > Sent: Wednesday, March 21, 2018 5:45 PM > To: Parav Pandit <parav@xxxxxxxxxxxx> > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford > <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA > mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Mark Bloch <markb@xxxxxxxxxxxx> > Subject: Re: [PATCH rdma-next 1/2] IB/cma: Resolve route only while receiving > CM requests > > On Wed, Mar 21, 2018 at 10:27:02PM +0000, Parav Pandit wrote: > > > 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. > > I'm not sure it is clear.. I read the code and wonder why it is there, and cannot > figure it out. > > I expect route_resolved = false to be placed at any spot that invalidates the > route lookup cached in the struct. > > So lets put it in cm_format_paths_from_req which clearly does invalidate any > cached route? It is still unnecessary, but at least it is in a place that makes some > sense.. Yes. Since both primary and alternate path record entry have rec_type set before calling cm_format_paths_from_req(). It is fine to set in there. > > > > > @@ -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. > > Okay, lets do that, if we have a function called from only one place it may as > well do everything. Then it has a nice internal symmetry. > Yes. > So like this? > All the 3 changes in below patch looks good to me. > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index > 4cc0fe6a29ff08..38d79bc1bf78f0 100644 > --- a/drivers/infiniband/core/cm.c > +++ b/drivers/infiniband/core/cm.c > @@ -1543,6 +1543,8 @@ static void cm_format_paths_from_req(struct > cm_req_msg *req_msg, > cm_req_get_primary_local_ack_timeout(req_msg); > primary_path->packet_life_time -= (primary_path->packet_life_time > > 0); > primary_path->service_id = req_msg->service_id; > + if (sa_path_is_roce(primary_path)) > + primary_path->roce.route_resolved = false; > > if (cm_req_has_alt_path(req_msg)) { > alt_path->dgid = req_msg->alt_local_gid; @@ -1562,6 +1564,9 > @@ static void cm_format_paths_from_req(struct cm_req_msg *req_msg, > cm_req_get_alt_local_ack_timeout(req_msg); > alt_path->packet_life_time -= (alt_path->packet_life_time > 0); > alt_path->service_id = req_msg->service_id; > + > + if (sa_path_is_roce(alt_path)) > + alt_path->roce.route_resolved = false; > } > cm_format_path_lid_from_req(req_msg, primary_path, alt_path); } diff > --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index > 34fa0507ed4ff1..8512f633efd642 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -2506,6 +2506,7 @@ cma_iboe_set_path_rec_l2_fields(struct > rdma_id_private *id_priv) > gid_type = ib_network_to_gid_type(addr->dev_addr.network); > route->path_rec->rec_type = sa_conv_gid_to_pathrec_type(gid_type); > > + route->path_rec->roce.route_resolved = true; > sa_path_set_ndev(route->path_rec, addr->dev_addr.net); > sa_path_set_ifindex(route->path_rec, ndev->ifindex); > sa_path_set_dmac(route->path_rec, addr->dev_addr.dst_dev_addr); > diff --git a/drivers/infiniband/core/sa_query.c > b/drivers/infiniband/core/sa_query.c > index 1cfec68c79115a..a61ec7e33613e9 100644 > --- a/drivers/infiniband/core/sa_query.c > +++ b/drivers/infiniband/core/sa_query.c > @@ -1248,6 +1248,9 @@ roce_resolve_route_from_path(struct ib_device > *device, u8 port_num, > } sgid_addr, dgid_addr; > int ret; > > + if (rec->roce.route_resolved) > + return 0; > + > if (!device->get_netdev) > return -EOPNOTSUPP; > > @@ -1287,6 +1290,8 @@ roce_resolve_route_from_path(struct ib_device > *device, u8 port_num, > dev_put(ndev); > done: > dev_put(idev); > + if (!ret) > + rec->roce.route_resolved = true; > return ret; > } > > diff --git a/include/rdma/ib_sa.h b/include/rdma/ib_sa.h index > 82b8e59af14ab7..bacb144f778024 100644 > --- a/include/rdma/ib_sa.h > +++ b/include/rdma/ib_sa.h > @@ -163,7 +163,15 @@ struct sa_path_rec_ib { > u8 raw_traffic; > }; > > +/** > + * struct sa_path_rec_roce - RoCE specific portion of the path record entry > + * @route_resolved: When set, it indicates that this route is already > + * resolved for this path record entry. > + * @dmac: Destination mac address for the given DGID entry > + * of the path record entry. > + */ > struct sa_path_rec_roce { > + bool route_resolved; > u8 dmac[ETH_ALEN]; > /* ignored in IB */ > int ifindex; -- 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
