> -----Original Message----- > From: Jason Gunthorpe [mailto:jgg@xxxxxxxx] > Sent: Wednesday, May 16, 2018 4:10 PM > To: Leon Romanovsky <leon@xxxxxxxxxx> > Cc: Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky > <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>; > Parav Pandit <parav@xxxxxxxxxxxx> > Subject: Re: [PATCH rdma-next 07/12] RDMA: Use ib_gid_attr in query attributes > > On Mon, May 14, 2018 at 11:11:13AM +0300, Leon Romanovsky wrote: > > > > [1] https://www.spinics.net/lists/linux-rdma/msg58148.html > > > > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx> > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx> > > Extra space between signed offs > Fixing in v1. > > /** > > - * ib_find_cached_gid - Returns the port number and GID table index where > > - * a specified GID value occurs. > > + * rdma_find_gid - Returns SGID attributes if the matching GID is found. > > * @device: The device to query. > > * @gid: The GID value to search for. > > * @gid_type: The GID type to search for. > > * @ndev: In RoCE, the net device of the device. NULL means ignore. > > - * @port_num: The port number of the device where the GID value was > found. > > - * @index: The index into the cached GID table where the GID was found. > This > > - * parameter may be NULL. > > * > > - * ib_find_cached_gid() searches for the specified GID value in > > + * rdma_find_gid() searches for the specified GID value in > > * the local software cache. > > + * > > + * Returns sgid attributes if the GID is found with valid reference. > > + * Or returns ERR_PTR for the error. > > + * Caller must invoke ib_put_cached_gid_attr() to release the reference. > > + * > > */ > > -int ib_find_cached_gid(struct ib_device *device, > > - const union ib_gid *gid, > > - enum ib_gid_type gid_type, > > - struct net_device *ndev, > > - u8 *port_num, > > - u16 *index) > > + > > Extra space between kdoc and function > Fixing in v1. > > +const struct ib_gid_attr * > > +rdma_find_gid(struct ib_device *device, > > + const union ib_gid *gid, > > + enum ib_gid_type gid_type, > > + struct net_device *ndev) > > { > > - return ib_cache_gid_find(device, gid, gid_type, ndev, port_num, index); > > + return ib_cache_gid_find(device, gid, gid_type, ndev); > > } > > -EXPORT_SYMBOL(ib_find_cached_gid); > > +EXPORT_SYMBOL(rdma_find_gid); > > Afer all the patches are applied this is the only call site for ib_cache_gid_find, no > sense in having a wrapper like this. > Fixing in v1 by having squashing ib_cache_gid_find to rdma_find_gid(). > > -int ib_find_gid_by_filter(struct ib_device *device, > > - const union ib_gid *gid, > > - u8 port_num, > > - bool (*filter)(const union ib_gid *gid, > > - const struct ib_gid_attr *, > > - void *), > > - void *context, u16 *index) > > +const struct ib_gid_attr * > > +rdma_find_gid_by_filter(struct ib_device *device, > > + const union ib_gid *gid, > > + u8 port_num, > > + bool (*filter)(const union ib_gid *gid, > > + const struct ib_gid_attr *, > > + void *), > > + void *context) > > { > > /* Only RoCE GID table supports filter function */ > > if (!rdma_protocol_roce(device, port_num) && filter) > > - return -EPROTONOSUPPORT; > > + return ERR_PTR(-EPROTONOSUPPORT); > > > > return ib_cache_gid_find_by_filter(device, gid, > > port_num, filter, > > - context, index); > > + context); > > } > > > > int ib_get_cached_pkey(struct ib_device *device, diff --git > > a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index > > 7df4c7173607..0efad89cfe22 100644 > > +++ b/drivers/infiniband/core/cm.c > > @@ -474,6 +474,11 @@ static int cm_init_av_for_lap(struct cm_port *port, > struct ib_wc *wc, > > if (ret) > > return ret; > > > > + /* > > + * Now that ah attribute is initialized based new wc, > > + * old ah attribute can be discarded. > > + */ > > + rdma_cleanup_ah_attr_gid_attr(&av->ah_attr); > > memcpy(&av->ah_attr, &new_ah_attr, sizeof(new_ah_attr)); > > return 0; > > } > > @@ -508,31 +513,50 @@ static int add_cm_id_to_port_list(struct > cm_id_private *cm_id_priv, > > return ret; > > } > > > > -static struct cm_port *get_cm_port_from_path(struct sa_path_rec > > *path) > > +static struct cm_port * > > +get_cm_port_from_path(struct sa_path_rec *path, const struct > > +ib_gid_attr *attr) > > { > > struct cm_device *cm_dev; > > struct cm_port *port = NULL; > > unsigned long flags; > > - u8 p; > > - struct net_device *ndev = ib_get_ndev_from_path(path); > > - > > - read_lock_irqsave(&cm.device_lock, flags); > > - list_for_each_entry(cm_dev, &cm.device_list, list) { > > - if (!ib_find_cached_gid(cm_dev->ib_device, &path->sgid, > > - sa_conv_pathrec_to_gid_type(path), > > - ndev, &p, NULL)) { > > - port = cm_dev->port[p - 1]; > > - break; > > + > > + if (attr) { > > + read_lock_irqsave(&cm.device_lock, flags); > > + list_for_each_entry(cm_dev, &cm.device_list, list) { > > + if (cm_dev->ib_device == attr->device) { > > + port = cm_dev->port[attr->port_num - 1]; > > + break; > > + } > > } > > + read_unlock_irqrestore(&cm.device_lock, flags); > > > + } else { > > + /* SGID attribute can be NULL in following > > + * conditions. > > + * (a) Alternative path > > + * (b) IB link layer without GRH > > + * (c) LAP send messages > > + */ > > This doesn't seem right.. > > Why wouldn't alternate path have a gid_attr too? If we are proposing or loading Alternative path is not in use, so once ib_ucm goes away, it won't be useful. > an alt path then we need to hold the reference as well, so it certainly should > exist, and if it exists it should be used here. > > The IB case should also have a SGID attr, IMHO. Currently gid attribute is based on the wc.grh on the rx path. In future it can be extended to drop the sgid_attr of wc and acquire based out of path record by using > > On the REQ side the attr should arise when the cm_id is bound to an IP, > otherwise it comes from the path (eg AF_IB style) the GID is already mandatory > in the paths even if the hop limit is 0. On requester side it comes from rdmacm. > > On the RESP side it comes from the IP or some random appropriate GID for the > Rxing namespace. APM paths are similar. > > It just makes everything cleaner and far more symmetric.. > > So this function shouldn't even accept a path.. > It accepts only for those cases when sgid_attr doesn't exist, just like current code. > As is this function is confusing because it does require the caller to pass a path > that perfectly matches the attr if attr is non NULL. At the very least this should > be a WARN_ON that the path->sgid and git_attr->gid are the same until it can be > cleaned up as in the above. > > Overall things are very confusing if there is both a gid attr and a path involved, > more so because the attr is not stored as part of the path structure... > When I wrote this patch in Jan sgid_attr was in the path record. You and Leon reviewed this that time and asked to take it out of the path record, because the way path record is used between ib_cm and rdma_cm was not so good. So now it out of path record, likely for good reason. So it is not as confusing as it sounds. If attribute is provided, device and port are used from attribute. If attribute is not provided, get_cm_port_from_path() finds it from the path record. Logic to do so resides inside this function to not duplicate to its callers. -- 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
