RE: [PATCH rdma-next 1/2] IB/cma: Resolve route only while receiving CM requests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux