| Message ID | 1505855931-4956-4-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 9/20/2017 12:18 AM, Jason Gunthorpe wrote: > The match step determines if the provider should bind to the sysfs device, > and the allocation step creates the verbs_device and cleanly matches the > uninit_device step which frees it. > > This split makes it simpler to factor out all the duplicated code in the > match step. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > libibverbs/driver.h | 21 ++++++++++++++- > libibverbs/init.c | 74 ++++++++++++++++++++++++++++++++++------------------- > 2 files changed, 68 insertions(+), 27 deletions(-) > > diff --git a/libibverbs/driver.h b/libibverbs/driver.h > index f9f5cfa2308c2f..397441f49a0e5a 100644 > --- a/libibverbs/driver.h > +++ b/libibverbs/driver.h > @@ -40,6 +40,7 @@ > #include <infiniband/kern-abi.h> > #include <ccan/list.h> > #include <config.h> > +#include <stdbool.h> > > #ifdef __cplusplus > # define BEGIN_C_DECLS extern "C" { > @@ -95,10 +96,27 @@ struct verbs_qp { > struct verbs_xrcd *xrcd; > }; > > +/* A rdma device detected in sysfs */ > +struct verbs_sysfs_dev { > + struct list_node entry; > + void *provider_data; It looks as this field isn't used in the series, correct ? if this is the case better add when it'll become applicable. > + char sysfs_name[IBV_SYSFS_NAME_MAX]; > + char ibdev_name[IBV_SYSFS_NAME_MAX]; > + char sysfs_path[IBV_SYSFS_PATH_MAX]; > + char ibdev_path[IBV_SYSFS_PATH_MAX]; > + int abi_ver; > + struct timespec time_created; > +}; > + -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 26, 2017 at 07:00:16PM +0300, Yishai Hadas wrote: > On 9/20/2017 12:18 AM, Jason Gunthorpe wrote: > > The match step determines if the provider should bind to the sysfs device, > > and the allocation step creates the verbs_device and cleanly matches the > > uninit_device step which frees it. > > > > This split makes it simpler to factor out all the duplicated code in the > > match step. > > > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > --- > > libibverbs/driver.h | 21 ++++++++++++++- > > libibverbs/init.c | 74 ++++++++++++++++++++++++++++++++++------------------- > > 2 files changed, 68 insertions(+), 27 deletions(-) > > > > diff --git a/libibverbs/driver.h b/libibverbs/driver.h > > index f9f5cfa2308c2f..397441f49a0e5a 100644 > > --- a/libibverbs/driver.h > > +++ b/libibverbs/driver.h > > @@ -40,6 +40,7 @@ > > #include <infiniband/kern-abi.h> > > #include <ccan/list.h> > > #include <config.h> > > +#include <stdbool.h> > > #ifdef __cplusplus > > # define BEGIN_C_DECLS extern "C" { > > @@ -95,10 +96,27 @@ struct verbs_qp { > > struct verbs_xrcd *xrcd; > > }; > > +/* A rdma device detected in sysfs */ > > +struct verbs_sysfs_dev { > > + struct list_node entry; > > + void *provider_data; > > It looks as this field isn't used in the series, correct ? if this is the > case better add when it'll become applicable. Yishai, Jason is a little bit overwhelmed now and because this series was posted long time here and got a lot of Acked-by tags, I would be happy to take it and move forward. Do you see any issues which he can't fix after this series merged? Thanks
On 9/27/2017 10:55 AM, Leon Romanovsky wrote: > On Tue, Sep 26, 2017 at 07:00:16PM +0300, Yishai Hadas wrote: >> On 9/20/2017 12:18 AM, Jason Gunthorpe wrote: >>> The match step determines if the provider should bind to the sysfs device, >>> and the allocation step creates the verbs_device and cleanly matches the >>> uninit_device step which frees it. >>> >>> This split makes it simpler to factor out all the duplicated code in the >>> match step. >>> >>> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> >>> --- >>> libibverbs/driver.h | 21 ++++++++++++++- >>> libibverbs/init.c | 74 ++++++++++++++++++++++++++++++++++------------------- >>> 2 files changed, 68 insertions(+), 27 deletions(-) >>> >>> diff --git a/libibverbs/driver.h b/libibverbs/driver.h >>> index f9f5cfa2308c2f..397441f49a0e5a 100644 >>> --- a/libibverbs/driver.h >>> +++ b/libibverbs/driver.h >>> @@ -40,6 +40,7 @@ >>> #include <infiniband/kern-abi.h> >>> #include <ccan/list.h> >>> #include <config.h> >>> +#include <stdbool.h> >>> #ifdef __cplusplus >>> # define BEGIN_C_DECLS extern "C" { >>> @@ -95,10 +96,27 @@ struct verbs_qp { >>> struct verbs_xrcd *xrcd; >>> }; >>> +/* A rdma device detected in sysfs */ >>> +struct verbs_sysfs_dev { >>> + struct list_node entry; >>> + void *provider_data; >> >> It looks as this field isn't used in the series, correct ? if this is the >> case better add when it'll become applicable. > > Yishai, > > Jason is a little bit overwhelmed now and because this series was posted > long time here and got a lot of Acked-by tags, I would be happy to take it > and move forward. Do you see any issues which he can't fix after this > series merged? > The above is some cleanup note that can be done later if we want to merge the series soon. Other verbs changes in the series looks fine to me. Reviewed-by: Yishai Hadas <yishaih@mellanox.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 26, 2017 at 07:00:16PM +0300, Yishai Hadas wrote: > >+/* A rdma device detected in sysfs */ > >+struct verbs_sysfs_dev { > >+ struct list_node entry; > >+ void *provider_data; > > It looks as this field isn't used in the series, correct ? if this is the > case better add when it'll become applicable. Ah, interesting. It is used during the series, but during the series all the uses get removed and replaced with verbs_match_ent.driver_data instead. So it could be dropped at the patch that removes the last use.. I'll look at that today. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/libibverbs/driver.h b/libibverbs/driver.h index f9f5cfa2308c2f..397441f49a0e5a 100644 --- a/libibverbs/driver.h +++ b/libibverbs/driver.h @@ -40,6 +40,7 @@ #include <infiniband/kern-abi.h> #include <ccan/list.h> #include <config.h> +#include <stdbool.h> #ifdef __cplusplus # define BEGIN_C_DECLS extern "C" { @@ -95,10 +96,27 @@ struct verbs_qp { struct verbs_xrcd *xrcd; }; +/* A rdma device detected in sysfs */ +struct verbs_sysfs_dev { + struct list_node entry; + void *provider_data; + char sysfs_name[IBV_SYSFS_NAME_MAX]; + char ibdev_name[IBV_SYSFS_NAME_MAX]; + char sysfs_path[IBV_SYSFS_PATH_MAX]; + char ibdev_path[IBV_SYSFS_PATH_MAX]; + int abi_ver; + struct timespec time_created; +}; + /* Must change the PRIVATE IBVERBS_PRIVATE_ symbol if this is changed */ struct verbs_device_ops { const char *name; + int match_min_abi_version; + int match_max_abi_version; + + bool (*match_device)(struct verbs_sysfs_dev *sysfs_dev); + /* Old interface, do not use in new code. */ struct ibv_context *(*alloc_context)(struct ibv_device *device, int cmd_fd); @@ -110,6 +128,7 @@ struct verbs_device_ops { void (*uninit_context)(struct verbs_device *device, struct ibv_context *ctx); + struct verbs_device *(*alloc_device)(struct verbs_sysfs_dev *sysfs_dev); struct verbs_device *(*init_device)(const char *uverbs_sys_path, int abi_version); void (*uninit_device)(struct verbs_device *device); @@ -123,7 +142,7 @@ struct verbs_device { size_t size_of_context; atomic_int refcount; struct list_node entry; - struct ibv_sysfs_dev *sysfs; + struct verbs_sysfs_dev *sysfs; }; static inline struct verbs_device * diff --git a/libibverbs/init.c b/libibverbs/init.c index 996406e2efa258..2369a1df6e1fbd 100644 --- a/libibverbs/init.c +++ b/libibverbs/init.c @@ -47,22 +47,11 @@ #include <errno.h> #include <assert.h> -#include <ccan/list.h> #include <util/util.h> #include "ibverbs.h" int abi_ver; -struct ibv_sysfs_dev { - struct list_node entry; - char sysfs_name[IBV_SYSFS_NAME_MAX]; - char ibdev_name[IBV_SYSFS_NAME_MAX]; - char sysfs_path[IBV_SYSFS_PATH_MAX]; - char ibdev_path[IBV_SYSFS_PATH_MAX]; - int abi_ver; - struct timespec time_created; -}; - struct ibv_driver_name { struct list_node entry; char *name; @@ -81,7 +70,7 @@ static int find_sysfs_devs(struct list_head *tmp_sysfs_dev_list) char class_path[IBV_SYSFS_PATH_MAX]; DIR *class_dir; struct dirent *dent; - struct ibv_sysfs_dev *sysfs_dev = NULL; + struct verbs_sysfs_dev *sysfs_dev = NULL; char value[8]; int ret = 0; @@ -100,7 +89,7 @@ static int find_sysfs_devs(struct list_head *tmp_sysfs_dev_list) continue; if (!sysfs_dev) - sysfs_dev = malloc(sizeof *sysfs_dev); + sysfs_dev = calloc(1, sizeof(*sysfs_dev)); if (!sysfs_dev) { ret = ENOMEM; goto out; @@ -148,8 +137,6 @@ static int find_sysfs_devs(struct list_head *tmp_sysfs_dev_list) if (ibv_read_sysfs_file(sysfs_dev->sysfs_path, "abi_version", value, sizeof value) > 0) sysfs_dev->abi_ver = strtol(value, NULL, 10); - else - sysfs_dev->abi_ver = 0; list_add(tmp_sysfs_dev_list, &sysfs_dev->entry); sysfs_dev = NULL; @@ -353,16 +340,51 @@ out: closedir(conf_dir); } +/* True if the provider matches the selected rdma sysfs device */ +static bool match_device(const struct verbs_device_ops *ops, + struct verbs_sysfs_dev *sysfs_dev) +{ + if (!ops->match_device(sysfs_dev)) + return false; + + if (sysfs_dev->abi_ver < ops->match_min_abi_version || + sysfs_dev->abi_ver > ops->match_max_abi_version) { + fprintf(stderr, PFX + "Warning: Driver %s does not support the kernel ABI of %u (supports %u to %u) for device %s\n", + ops->name, sysfs_dev->abi_ver, + ops->match_min_abi_version, + ops->match_max_abi_version, + sysfs_dev->ibdev_path); + return false; + } + return true; +} + static struct verbs_device *try_driver(const struct verbs_device_ops *ops, - struct ibv_sysfs_dev *sysfs_dev) + struct verbs_sysfs_dev *sysfs_dev) { struct verbs_device *vdev; struct ibv_device *dev; char value[16]; - vdev = ops->init_device(sysfs_dev->sysfs_path, sysfs_dev->abi_ver); - if (!vdev) - return NULL; + if (ops->alloc_device) { + if (!match_device(ops, sysfs_dev)) + return NULL; + + vdev = ops->alloc_device(sysfs_dev); + if (!vdev) { + fprintf(stderr, PFX + "Fatal: couldn't allocate device for %s\n", + sysfs_dev->ibdev_path); + return NULL; + } + } else { + vdev = + ops->init_device(sysfs_dev->sysfs_path, sysfs_dev->abi_ver); + if (!vdev) + return NULL; + } + vdev->ops = ops; atomic_init(&vdev->refcount, 1); @@ -409,7 +431,7 @@ static struct verbs_device *try_driver(const struct verbs_device_ops *ops, return vdev; } -static struct verbs_device *try_drivers(struct ibv_sysfs_dev *sysfs_dev) +static struct verbs_device *try_drivers(struct verbs_sysfs_dev *sysfs_dev) { struct ibv_driver *driver; struct verbs_device *dev; @@ -463,8 +485,8 @@ static void check_memlock_limit(void) rlim.rlim_cur); } -static int same_sysfs_dev(struct ibv_sysfs_dev *sysfs1, - struct ibv_sysfs_dev *sysfs2) +static int same_sysfs_dev(struct verbs_sysfs_dev *sysfs1, + struct verbs_sysfs_dev *sysfs2) { if (!strcmp(sysfs1->sysfs_name, sysfs2->sysfs_name) && ts_cmp(&sysfs1->time_created, @@ -481,8 +503,8 @@ static void try_all_drivers(struct list_head *sysfs_list, struct list_head *device_list, unsigned int *num_devices) { - struct ibv_sysfs_dev *sysfs_dev; - struct ibv_sysfs_dev *tmp; + struct verbs_sysfs_dev *sysfs_dev; + struct verbs_sysfs_dev *tmp; struct verbs_device *vdev; list_for_each_safe(sysfs_list, sysfs_dev, tmp, entry) { @@ -499,7 +521,7 @@ static void try_all_drivers(struct list_head *sysfs_list, int ibverbs_get_device_list(struct list_head *device_list) { LIST_HEAD(sysfs_list); - struct ibv_sysfs_dev *sysfs_dev, *next_dev; + struct verbs_sysfs_dev *sysfs_dev, *next_dev; struct verbs_device *vdev, *tmp; static int drivers_loaded; unsigned int num_devices = 0; @@ -515,7 +537,7 @@ int ibverbs_get_device_list(struct list_head *device_list) * present in the sysfs_list. */ list_for_each_safe(device_list, vdev, tmp, entry) { - struct ibv_sysfs_dev *old_sysfs = NULL; + struct verbs_sysfs_dev *old_sysfs = NULL; list_for_each(&sysfs_list, sysfs_dev, entry) { if (same_sysfs_dev(vdev->sysfs, sysfs_dev)) {
The match step determines if the provider should bind to the sysfs device, and the allocation step creates the verbs_device and cleanly matches the uninit_device step which frees it. This split makes it simpler to factor out all the duplicated code in the match step. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- libibverbs/driver.h | 21 ++++++++++++++- libibverbs/init.c | 74 ++++++++++++++++++++++++++++++++++------------------- 2 files changed, 68 insertions(+), 27 deletions(-)