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..
> > > @@ -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.
So like this?
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
