[PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Hefty, Sean]

In order to support OFED or vendor specific calls, define a
generic extension mechanism.  This allows OFED, an RDMA vendor,
or another registered 3rd party (for example, the librdmacm)
to define RDMA extensions.

Users which make use extensions are aware that they are not
only using an extended call, but are given information regarding
how widely the extension by be supported.  Support for extended
functions, data structures, and enums are defined.

Extensions are referenced by name.  There is an assumption that
extension names are prefixed relative to the supporting party.
Until an extension has been incorporated into libibverbs, it
should be defined in an appropriate external header file.

For example, OFA could provide a header file with their definition
for XRC extensions.  A partial view of such a header file might
look something similar to:

#ifndef OFA_XRC_H
#define OFA_XRC_H
#include <infiniband/verbs.h>

#define OFA_XRC_OPS "ofa-xrc"

/* Extend IBV_QP_TYPE for XRC */
#define OFA_QPT_XRC ((enum ibv_qp_type) \
	(IBV_EXTENSION_OFA << IBV_EXTENSION_BASE_SHIFT) + 6)

struct ofa_xrcd {
	struct ibv_context *context;
};

struct ofa_xrc_ops {
	struct ofa_xrcd * (*open_xrcd)(struct ibv_context *context,
					inf fd, int oflags);
	int		* (*close_xrcd)(struct ofa_xrcd *xrcd);
	/* other functions left as exercise to the reader */
};

#endif /* OFA_XRC_H */

Driver libraries that support extensions are given a new
registration call, ibv_register_device_ext().  Use of this call
indicates to libibverbs that the library allocates extended
versions of struct ibv_device and struct ibv_context.

The following new APIs are added to libibverbs to applications
to use to determine if an extension is supported and to obtain the
extended function calls.

ibv_have_ext_ops - returns true if an extension is supported
ibv_get_device_ext_ops - return extended operations for a device
ibv_get_ext_ops - return extended operations for an open context

To maintain backwards compatibility with existing applications,
internally, the library uses the last byte of the device name
to record if the device was registered with extension support.

Signed-off-by: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 include/infiniband/driver.h |    1 +
 include/infiniband/verbs.h  |   40 +++++++++++++++++++++++++++++++++++++++-
 src/device.c                |   18 ++++++++++++++++++
 src/ibverbs.h               |   18 ++++++++++++++++++
 src/init.c                  |   17 ++++++++++++++++-
 src/libibverbs.map          |    5 +++++
 src/verbs.c                 |    9 +++++++++
 7 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/include/infiniband/driver.h b/include/infiniband/driver.h
index 9a81416..e48abfd 100644
--- a/include/infiniband/driver.h
+++ b/include/infiniband/driver.h
@@ -57,6 +57,7 @@ typedef struct ibv_device *(*ibv_driver_init_func)(const char *uverbs_sys_path,
 						   int abi_version);
 
 void ibv_register_driver(const char *name, ibv_driver_init_func init_func);
+void ibv_register_driver_ext(const char *name, ibv_driver_init_func init_func);
 int ibv_cmd_get_context(struct ibv_context *context, struct ibv_get_context *cmd,
 			size_t cmd_size, struct ibv_get_context_resp *resp,
 			size_t resp_size);
diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
index 0f1cb2e..b82cd3a 100644
--- a/include/infiniband/verbs.h
+++ b/include/infiniband/verbs.h
@@ -55,6 +55,15 @@
 
 BEGIN_C_DECLS
 
+enum ibv_extension_type {
+	IBV_EXTENSION_COMMON,
+	IBV_EXTENSION_VENDOR,
+	IBV_EXTENSION_OFA,
+	IBV_EXTENSION_RDMA_CM
+};
+#define IBV_EXTENSION_BASE_SHIFT 24
+#define IBV_EXTENSION_MASK 0xFF000000
+
 union ibv_gid {
 	uint8_t			raw[16];
 	struct {
@@ -92,7 +101,8 @@ enum ibv_device_cap_flags {
 	IBV_DEVICE_SYS_IMAGE_GUID	= 1 << 11,
 	IBV_DEVICE_RC_RNR_NAK_GEN	= 1 << 12,
 	IBV_DEVICE_SRQ_RESIZE		= 1 << 13,
-	IBV_DEVICE_N_NOTIFY_CQ		= 1 << 14
+	IBV_DEVICE_N_NOTIFY_CQ		= 1 << 14,
+	IBV_DEVICE_EXTENSIONS		= 1 << (IBV_EXTENSION_BASE_SHIFT - 1)
 };
 
 enum ibv_atomic_cap {
@@ -623,6 +633,13 @@ struct ibv_device {
 	char			dev_path[IBV_SYSFS_PATH_MAX];
 	/* Path to infiniband class device in sysfs */
 	char			ibdev_path[IBV_SYSFS_PATH_MAX];
+
+	/* Following fields only available if device supports extensions */
+	void		       *private;
+	int			(*have_ext_ops)(struct ibv_device *device,
+						const char *ext_name);
+	void *			(*get_device_ext_ops)(struct ibv_device *device,
+						      const char *ext_name);
 };
 
 struct ibv_context_ops {
@@ -691,6 +708,11 @@ struct ibv_context {
 	int			num_comp_vectors;
 	pthread_mutex_t		mutex;
 	void		       *abi_compat;
+
+	/* Following fields only available if device supports extensions */
+	void		       *private;
+	void *			(*get_ext_ops)(struct ibv_context *context,
+					       const char *ext_name);
 };
 
 /**
@@ -724,6 +746,17 @@ const char *ibv_get_device_name(struct ibv_device *device);
 uint64_t ibv_get_device_guid(struct ibv_device *device);
 
 /**
+ * ibv_have_ext_ops - Return true if device supports the requested
+ * extended operations.
+ */
+int ibv_have_ext_ops(struct ibv_device *device, const char *name);
+
+/**
+ * ibv_get_device_ext_ops - Return extended operations.
+ */
+void *ibv_get_device_ext_ops(struct ibv_device *device, const char *name);
+
+/**
  * ibv_open_device - Initialize device for use
  */
 struct ibv_context *ibv_open_device(struct ibv_device *device);
@@ -734,6 +767,11 @@ struct ibv_context *ibv_open_device(struct ibv_device *device);
 int ibv_close_device(struct ibv_context *context);
 
 /**
+ * ibv_get_ext_ops - Return extended operations.
+ */
+void *ibv_get_ext_ops(struct ibv_context *context, const char *name);
+
+/**
  * ibv_get_async_event - Get next async event
  * @event: Pointer to use to return async event
  *
diff --git a/src/device.c b/src/device.c
index 185f4a6..78d9d35 100644
--- a/src/device.c
+++ b/src/device.c
@@ -181,6 +181,24 @@ int __ibv_close_device(struct ibv_context *context)
 }
 default_symver(__ibv_close_device, ibv_close_device);
 
+int __ibv_have_ext_ops(struct ibv_device *device, const char *name)
+{
+	if (!ibv_get_ext_support(device))
+		return ENOSYS;
+
+	return device->have_ext_ops(device, name);
+}
+default_symver(__ibv_have_ext_ops, ibv_have_ext_ops);
+
+void *__ibv_get_device_ext_ops(struct ibv_device *device, const char *name)
+{
+	if (!ibv_get_ext_support(device) || !device->get_device_ext_ops)
+		return NULL;
+
+	return device->get_device_ext_ops(device, name);
+}
+default_symver(__ibv_get_device_ext_ops, ibv_get_device_ext_ops);
+
 int __ibv_get_async_event(struct ibv_context *context,
 			  struct ibv_async_event *event)
 {
diff --git a/src/ibverbs.h b/src/ibverbs.h
index 6a6e3c8..33bdee2 100644
--- a/src/ibverbs.h
+++ b/src/ibverbs.h
@@ -35,6 +35,7 @@
 #define IB_VERBS_H
 
 #include <pthread.h>
+#include <string.h>
 
 #include <infiniband/driver.h>
 
@@ -102,4 +103,21 @@ HIDDEN int ibverbs_init(struct ibv_device ***list);
 		(cmd)->response  = (uintptr_t) (out);			\
 	} while (0)
 
+/*
+ * Support for extended operations is recorded at the end of
+ * the name character array.  This way we don't need to query
+ * for the device capabilities with every call.
+ */
+static inline int ibv_get_ext_support(struct ibv_device *device)
+{
+	return device->name[IBV_SYSFS_NAME_MAX - 1];
+}
+
+static inline void ibv_set_ext_support(struct ibv_device *device,
+				       int ext_supported)
+{
+	if (strlen(device->name) < IBV_SYSFS_NAME_MAX - 1)
+		device->name[IBV_SYSFS_NAME_MAX - 1] = (char) ext_supported;
+}
+
 #endif /* IB_VERBS_H */
diff --git a/src/init.c b/src/init.c
index 4f0130e..419ab31 100644
--- a/src/init.c
+++ b/src/init.c
@@ -71,6 +71,7 @@ struct ibv_driver {
 	const char	       *name;
 	ibv_driver_init_func	init_func;
 	struct ibv_driver      *next;
+	int			ext_support;
 };
 
 static struct ibv_sysfs_dev *sysfs_dev_list;
@@ -153,7 +154,8 @@ static int find_sysfs_devs(void)
 	return ret;
 }
 
-void ibv_register_driver(const char *name, ibv_driver_init_func init_func)
+static void __ibv_register_driver(const char *name, ibv_driver_init_func init_func,
+				  int ext_support)
 {
 	struct ibv_driver *driver;
 
@@ -166,6 +168,7 @@ void ibv_register_driver(const char *name, ibv_driver_init_func init_func)
 	driver->name      = name;
 	driver->init_func = init_func;
 	driver->next      = NULL;
+	driver->ext_support = ext_support;
 
 	if (tail_driver)
 		tail_driver->next = driver;
@@ -174,6 +177,16 @@ void ibv_register_driver(const char *name, ibv_driver_init_func init_func)
 	tail_driver = driver;
 }
 
+void ibv_register_driver(const char *name, ibv_driver_init_func init_func)
+{
+	__ibv_register_driver(name, init_func, 0);
+}
+
+void ibv_register_driver_ext(const char *name, ibv_driver_init_func init_func)
+{
+	__ibv_register_driver(name, init_func, 1);
+}
+
 static void load_driver(const char *name)
 {
 	char *so_name;
@@ -368,6 +381,8 @@ static struct ibv_device *try_driver(struct ibv_driver *driver,
 	strcpy(dev->name,       sysfs_dev->ibdev_name);
 	strcpy(dev->ibdev_path, sysfs_dev->ibdev_path);
 
+	ibv_set_ext_support(dev, driver->ext_support);
+
 	return dev;
 }
 
diff --git a/src/libibverbs.map b/src/libibverbs.map
index 1827da0..422e07f 100644
--- a/src/libibverbs.map
+++ b/src/libibverbs.map
@@ -96,4 +96,9 @@ IBVERBS_1.1 {
 		ibv_port_state_str;
 		ibv_event_type_str;
 		ibv_wc_status_str;
+
+		ibv_register_driver_ext;
+		ibv_have_ext_ops;
+		ibv_get_device_ext_ops;
+		ibv_get_ext_ops;
 } IBVERBS_1.0;
diff --git a/src/verbs.c b/src/verbs.c
index ba3c0a4..a34a784 100644
--- a/src/verbs.c
+++ b/src/verbs.c
@@ -76,6 +76,15 @@ enum ibv_rate mult_to_ibv_rate(int mult)
 	}
 }
 
+void *__ibv_get_ext_ops(struct ibv_context *context, const char *name)
+{
+	if (!ibv_get_ext_support(context->device) || !context->get_ext_ops)
+		return NULL;
+
+	return context->get_ext_ops(context, name);
+}
+default_symver(__ibv_get_ext_ops, ibv_get_ext_ops);
+
 int __ibv_query_device(struct ibv_context *context,
 		       struct ibv_device_attr *device_attr)
 {


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Or Gerlitz]

Hefty, Sean wrote:
> In order to support OFED or vendor specific calls, define a
> generic extension mechanism.  This allows OFED, an RDMA vendor,
> or another registered 3rd party (for example, the librdmacm)
> to define RDMA extensions.

I'm trying to understand the way the user/kernel way of adding verbs
is implemented... I wasn't sure, if/which specific kernel patch out
of this series is matching this one?

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Hefty, Sean]

> I'm trying to understand the way the user/kernel way of adding verbs
> is implemented... I wasn't sure, if/which specific kernel patch out
> of this series is matching this one?

There are no matching kernel patches to this patch.  This does not try to provide any direct support for out of tree kernel patches.

The closest this comes is reserving the upper 8-bits of any enum or other value, which can be used to indicate vendor specific support.  For example, an out of tree kernel patch could define a new QP type and ABI command with one of the higher bits set (rather than the next in series).  An upstream patch would remove these bits.  This should make it possible for a vendor to continue to support their out of tree changes even after that feature was added to the mainline.  Hopefully this makes sense.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Or Gerlitz]

Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> I'm trying to understand the way the user/kernel way of adding verbs
>> is implemented... I wasn't sure, if/which specific kernel patch out
>> of this series is matching this one?

> There are no matching kernel patches to this patch.  This does not try to provide any
> direct support for out of tree kernel patches.

Sean, maybe I wasn't clear here, I was referring to the XRC kernel
patch series you were just sending yesterday for upstream
review/acceptance... isn't that series + libibverbs/libmlx4/librdmacm
compose a complete solution for XRC for (say) new applications (forget
about apps written to other XRC APIs)? I'm trying to understand
if/what is the framework you suggest to add new user space verbs.

Basically, I wasn't referring to such new verbs as vendor extensions,
but rather as new verbs we want to add at this and/or future points of
time which didn't exist at the time the IB stack and specifically its
kernel/user ABIs/APIs were written (couple of years ago...).

I hope to better spell out my question now, is this section below
still relevant as the answer?

Or.

> The closest this comes is reserving the upper 8-bits of any enum or other value, which can be used to indicate vendor specific support.  For example, an out of tree kernel patch could define a new QP type and ABI command with one of the higher bits set (rather than the next in series).  An upstream patch would remove these bits.  This should make it possible for a vendor to continue to support their out of tree changes even after that feature was added to the mainline.  Hopefully this makes sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Hefty, Sean]

> Basically, I wasn't referring to such new verbs as vendor extensions,
> but rather as new verbs we want to add at this and/or future points of
> time which didn't exist at the time the IB stack and specifically its
> kernel/user ABIs/APIs were written (couple of years ago...).

To be clear, there are 2 sides to ibverbs - the app side, and the provider library.

On the app side, new functionality would be added directly to libibverbs.  I would reuse what's there if possible, and provide direct API calls where needed.  For example, the xrc patch adds:

ibv_create_xsrq()
ibv_open_xrcd()
ibv_close_xrcd()

as new APIs.  On the provider side, the necessary calls are obtained by ibverbs calling get_ext_ops().

I haven't come up with another way of extended verbs that would be as easy for an application to use, given that most of the calls and data structures are reusable.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Jack Morgenstein]

On Thursday 16 June 2011 22:27, Hefty, Sean wrote:
> > Basically, I wasn't referring to such new verbs as vendor extensions,
> > but rather as new verbs we want to add at this and/or future points of
> > time which didn't exist at the time the IB stack and specifically its
> > kernel/user ABIs/APIs were written (couple of years ago...).
> 
> To be clear, there are 2 sides to ibverbs - the app side, and the provider library.
> 
> On the app side, new functionality would be added directly to libibverbs.  I would reuse what's there if possible, and provide direct API calls where needed.  For example, the xrc patch adds:
> 
> ibv_create_xsrq()
> ibv_open_xrcd()
> ibv_close_xrcd()
> 
> as new APIs.  On the provider side, the necessary calls are obtained by ibverbs calling get_ext_ops().
> 
> I haven't come up with another way of extended verbs that would be as easy for an application to use, given that most of the calls and data structures are reusable.
> 
> - Sean

Hi Sean,

I'm not sure about what this mechanism saves us over bumping the ABI numbers.
Actually, I think I do see a problem here, under the following situation:
	- All libraries (app, libibverbs, and libmlx4) support extensions.
	- A new verb (say extension "ib_new_verb" was added as an extension to the app,
          to libibverbs, and to libmlx4 and everything was compiled.
	- The APP is built on the full configuration, but is run on a configuration
	  which has the verb extension added to libmlx4, but NOT to libibverbs
	  (new libibverbs was installed originally, so that libmlx4 would succeed in the install,
	  then somehow libibverbs was rolled back to before "ib_new_verb" was added).

When the APP tries to run, it calls:
      ibv_get_device_ext_ops(struct ibv_device *device, "ib_new_verb");
This call will succeed (in the current implementation).
However, the verb helper function (ibv_cmd_<new_verb>) is not present
in libibverbs, so things will crash (if, indeed, libmlx4 can be loaded at all -

In fact, I'm not sure if it will fail loading because of unresolved references).
Indeed, I am not sure that the app can run at all due to unresolved references.

The problem here is that the new additions need support in libibverbs in order
to work (this is not simply a "pass-through" by libibverbs to the lower layer).

I may be wrong about all this --  userspace is not really my expertise.
If I am not wrong, what, then, is the advantage of this methodology over simply bumping the ABI numbers?

-Jack
 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Hefty, Sean]

> In fact, I'm not sure if it will fail loading because of unresolved
> references).
> Indeed, I am not sure that the app can run at all due to unresolved
> references.

I'm not sure what exactly we can do in the situation you described.  I would expect the app to see an unresolved reference.
 
> I may be wrong about all this --  userspace is not really my expertise.
> If I am not wrong, what, then, is the advantage of this methodology over
> simply bumping the ABI numbers?

I'm not sure I fully understand your question.  In an ideal case, any new feature would be added to the upstream ibverbs.  The extensions would be used to support some new feature that ibverbs is not aware of.  For example, a vendor could expose APIs that offload MPI collectives, which may still be under development.

- Sean

Re: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Jason Gunthorpe]

> In fact, I'm not sure if it will fail loading because of unresolved
> references).  Indeed, I am not sure that the app can run at all due
> to unresolved references.

The dlopen of mlx4 should fail due to unresolved references, so the
net effect will be that no apps cannot open the RDMA device in this
situation - so it is an invalid system configuration that is properly
detected by the runtime.

> The problem here is that the new additions need support in
> libibverbs in order to work (this is not simply a "pass-through" by
> libibverbs to the lower layer).

The hope is once the infrastructure is in libibverbs there will not
be as much need to change libibverbs, just the apps and the drivers.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Jack Morgenstein]

On Tuesday 02 August 2011 08:38, Jason Gunthorpe wrote:
> The hope is once the infrastructure is in libibverbs there will not
> be as much need to change libibverbs, just the apps and the drivers.
> 
If I understand correctly, the various additions which would normally be made to libibverbs
would then be made by third-party libraries which extend libibverbs to support their additions.

These additions would include the new ibv_cmd_xxx functions
(the core functions reside in src/cmd.c), and new, additional, enum values of
the form IBV_USER_VERBS_CMD_XXXX, of which the core enum is in file
include/infiniband/kern_abi.h.

The modified apps would then include the header files of the 3rd party additions
after the libibverbs headers when compiling.

Each new third-party package would need such a library.
While this will lead to a multiplicity of new libraries (one per addition),
the core libibverbs package would remain as is.

Am I correct?  If so, shouldn't the current XRC userspace implementation do the same (and
take the XRC-specific additions out of libibverbs and put them into a separate library)?

Note that coordination between third parties would still be required to insure that there is
no collision of enum values between the various packages.

-Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Hefty, Sean]

> If I understand correctly, the various additions which would normally be made
> to libibverbs
> would then be made by third-party libraries which extend libibverbs to support
> their additions.

It may help to read about extensions in opengl:

http://www.opengl.org/registry/doc/rules.html

Additions can be made to both.  Obviously Roland has the final say on any changes to ibverbs, but what I envision is:

If a feature is based on an industry standard and all necessary kernel changes are upstream, then the feature should be integrated into ibverbs.  An application would simply call ibv_new_feature() to make use of the feature (or call an existing function with some new enum value).  Internally, ibverbs may need to obtain a new interface from the provider library.

If there is no published specification for a feature (maybe it's still under development), kernel patches are needed, and there are customers who want to use the feature immediately, then a vendor can define an extension.  In this case, the application may call vendor_ops = ibv_get_ext_ops(), followed by vendor_ops->new_feature().  Or the app may call ibv_some_existing_function() using a vendor specific enum value.

> These additions would include the new ibv_cmd_xxx functions
> (the core functions reside in src/cmd.c), and new, additional, enum values of
> the form IBV_USER_VERBS_CMD_XXXX, of which the core enum is in file
> include/infiniband/kern_abi.h.

If an app uses an extension, then there are no changes to ibverbs.  The provider library either needs to use an existing ibv_cmd_* call or call into the kernel itself.  If the provider needs a new command, it could declare it as:

	enum {
		MLX4_USER_VERBS_CMD_BASE = IBV_EXTENSION_VENDOR <<
						IBV_EXTENSION_BASE_SHIFT,
		MLX4_USER_VERBS_CMD_NEW_FEATURE ...
	};

This requires a kernel patch to uverbs maintained by the vendor.  Note that this means that the vendor can continue to support their version of a feature (with continued kernel patches) even once the feature is merged into ibverbs.
 
> Each new third-party package would need such a library.
> While this will lead to a multiplicity of new libraries (one per addition),
> the core libibverbs package would remain as is.

I didn't follow this.  The new feature could be integrated directly into the provider library (e.g. mlx4).
 
> Am I correct?  If so, shouldn't the current XRC userspace implementation do
> the same (and
> take the XRC-specific additions out of libibverbs and put them into a separate
> library)?

XRC could be added as an extension, but since it's based on a published specification, IMO it makes more sense being integrated directly into ibverbs with the necessary kernel changes pushed upstream.  Extensions are more difficult for apps to use than an integrated feature.

> Note that coordination between third parties would still be required to insure
> that there is
> no collision of enum values between the various packages.

The use of enum ibv_extension_type should prevent collisions.  It may be that different vendors use the same value for different objects, but that doesn't result in a collision.  The scope of a vendor specific value is per device.

- Sean 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Or Gerlitz]

On 8/2/2011 7:28 PM, Hefty, Sean wrote:
>> If I understand correctly, the various additions which would normally be made
>> to libibverbs would then be made by third-party libraries which extend libibverbs to support their additions.
>
> It may help to read about extensions in opengl: http://www.opengl.org/registry/doc/rules.html
> Additions can be made to both.  Obviously Roland has the final say on any changes to ibverbs, but what I envision is:
>
> If a feature is based on an industry standard and all necessary kernel changes are upstream, then the feature should be integrated into ibverbs.  An application would simply call ibv_new_feature() to make use of the feature (or call an existing function with some new enum value).  Internally, ibverbs may need to obtain a new interface from the provider library.
>
> If there is no published specification for a feature (maybe it's still under development), kernel patches are needed, and there are customers who want to use the feature immediately, then a vendor can define an extension.  In this case, the application may call vendor_ops = ibv_get_ext_ops(), followed by vendor_ops->new_feature().  Or the app may call ibv_some_existing_function() using a vendor specific enum value.

Sean, All,

"Allow 3rd party extensions to verb routines" was posted couple of times 
over the last months, I'm replying here @ 
http://marc.info/?t=130815687800001&r=1&w=2 since there has been few 
emails exchanges with Q&A so maybe would be better to continue that, 
anyway,

I would suggest that we first and most see that the selected method to 
add extensions addresses the 1st use case, e.g a feature which is in the 
kernel, standard, etc and now needed to be added user space wise such 
that all parties involve in that game: libibverbs, provider libraries 
and 3rd party code which links against libibverbs (application and 
libraries) are okay:

So for new feature that uses new verb, new data structures, new kernel 
uverbs calls - the application would call ibv_new_feature() to make use 
of the feature - applications can require libibverbs version > X in 
their install RPM dependencies mechanism such that when the application 
is installed its ensured that it could load libibverbs they w.o hitting 
a failure. Provider libraries which issue ibv_cmd_new_feature() calls 
can (should?!) apply that mechanism, looking on libmlx4 rpm spec file 
I'm not sure this is done there - still, ibv_cmd_new_feature() could 
fail if the kernel doesn't support that feature yet, but assuming uverbs 
return a known/documented value e.g -ENOSYS on that case, someone up 
there application/system would realize that the kernel has to be updated 
or they can tweak the app to give up using on ibv_new_feature() calls.

AND, we are remained with the libibverbs --- new_feature() ---> provider 
library trail. Here since
the provider library is plugin, we can't add/track strict dependencies 
to libibverbs for all provider libraries. Sean, my understanding is that 
for the 1st case (feature in kernel,etc) - the essence of your 
suggestion is to address that.

Looking on the suggestion, it states that for each new_feature, 
libibverbs defines "struct ibv_new_feature_ops" and issues "n_f_ops_p = 
ibv_get_ext_ops(context, IBV_NEW_FEATURE_OPS)" call each time it needs 
to call into the provider library for that new feature.

Looking closely on applying that method for XRC extensions as use case, 
namely commit 563fb8c3e "Using extensions to define XRC support" on your 
libibverbs/xrc git tree/branch and commit 5c01e9f40c "mlx4: Add support 
for XRC extension" on your libmlx4/xrc git - this gets a bit more 
complicated (git://git.openfabrics.org/~shefty/libibverbs.git xrc and 
libmlx4.git xrc) here's some feedback:

1. for libibverbs some structures extended field is added at their end 
saying "Following fields only available if device supports extensions"e.g

> @@ -590,6 +634,13 @@ struct ibv_qp {
>         pthread_mutex_t         mutex;
>         pthread_cond_t          cond;
>         uint32_t                events_completed;
> +
> +       /* Following fields only available if device supports 
> extensions */
> +       union {
> +               struct {
> +                       struct ibv_xrcd *xrcd;
> +               } xrc_recv;
> +       } ext;
>  };

but this field is a -- union -- how would that work if more than one 
extension is to be applied for a structure?

2. indeed, reality wise, new features, much of the time will also 
interact with existing data structures... so what happens if we extend a 
structure but the the extended strucutre is actually a field within 
another existing structure e.g suppose we want to extend ibv_ah_attr 
which is within ibv_qp_attr e.g to be used for the RAW Ethernet QPs. I 
don't see how we can be backward compatible with apps linked against 
libibverbs with the internal structure size being smaller, correct? so 
extended fields must be on the END always - in the actual structure they 
are added and if this structure is a field of another structure then we 
can't just extend it and need to come up with new structure which is in 
turn used as the field?

3. The usage of "#ifdef IBV_NEW_FEATURE_OPS" (IBV_XRC_OPS) in libmlx4 
will become tireding to follow maybe and could be replaced by placing 
dependency on a version of libibverbs that supports IBV_NEW_FEATURE_OPS?

4. can we somehow come up with a method to avoid IBV_NEW_FEATURE_OPS for 
--every-- new feature?

> or call an existing function with some new enum value

5. what happens if we just want to enhance an -- existing -- function - 
suppose we want to enhance ibv_post_send / ibv_poll_cq to support 
features like LSO, checksum offload, masked atomic operations, fast 
memory remote invalidate, etc so we add IBV_WR_NEW_FEATURE / 
IB_WC_NEW_FEATURE enum values, this step is simple, again if we go the 
way of the applicayion ensuring through dependencies that they are 
loaded against libibverbs that supports IB_{WR,WC}_NEW_FEATURE.

Now we come up with a need to extend struct ibv_send_wr and struct 
ibv_wc - where we should be very careful - walking on glasses for the 
backward compatibility thing - so only adding at the end on as union 
field which doesn't change the size of the union, correct? for 
ibv_post_send we can rely on the provider library to return error if 
they don't support NEW_FEATURE, but this would happen in run time... is 
there a way to avoid that, should we better go and add 
ib_post_send_new_feature? this would be very tedious doing for each 
new_feature and not applicable to ibv_poll_cq - any idea what should be 
done here?

This is for now...

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Bart Van Assche]

On Thu, Dec 29, 2011 at 3:43 PM, Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> 5. what happens if we just want to enhance an -- existing -- function -
> suppose we want to enhance ibv_post_send / ibv_poll_cq to support features
> like LSO, checksum offload, masked atomic operations, fast memory remote
> invalidate, etc so we add IBV_WR_NEW_FEATURE / IB_WC_NEW_FEATURE enum
> values, this step is simple, again if we go the way of the applicayion
> ensuring through dependencies that they are loaded against libibverbs that
> supports IB_{WR,WC}_NEW_FEATURE.

Not only for the ABI between the Linux kernel and Linux user space but
also for Linux shared libraries it is required that the ABI of a new
version is backwards compatible with previous versions. So any
functionality that can't be added in a backwards compatible way to an
existing function should be added as a new system call, sysfs file or
shared library function - whatever is appropriate.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Or Gerlitz]

>
>> or call an existing function with some new enum value
>
> 5. what happens if we just want to enhance an -- existing -- function 
> - suppose we want to enhance ibv_post_send / ibv_poll_cq to support 
> features like LSO, checksum offload, masked atomic operations, fast 
> memory remote invalidate, etc so we add IBV_WR_NEW_FEATURE / 
> IB_WC_NEW_FEATURE enum values, this step is simple, again if we go the 
> way of the applicayion ensuring through dependencies that they are 
> loaded against libibverbs that supports IB_{WR,WC}_NEW_FEATURE.

Another element to take into account / use here, is the device 
capabilities, in the kernel RDMA stack this is the way to go for 
supporting extended features - those who are not common to all devices, 
e.g the way ipoib queries the device and concludes if checksum/lso 
offloads can be exposed to the network stack, or the rpc/rdma layer 
decides what memory management model to use for rdma conditioned if 
memory extensions etc are supported. But the kernel is one piece, so if 
a cap is defined and there's a driver down there advertizing it, all is 
in place software wise, where in user space, there are some more layers 
to address, as this thread attempts to do.

Or.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Hefty, Sean]

> 1. for libibverbs some structures extended field is added at their end
> saying "Following fields only available if device supports extensions"e.g
> 
> > @@ -590,6 +634,13 @@ struct ibv_qp {
> >         pthread_mutex_t         mutex;
> >         pthread_cond_t          cond;
> >         uint32_t                events_completed;
> > +
> > +       /* Following fields only available if device supports
> > extensions */
> > +       union {
> > +               struct {
> > +                       struct ibv_xrcd *xrcd;
> > +               } xrc_recv;
> > +       } ext;
> >  };
> 
> but this field is a -- union -- how would that work if more than one
> extension is to be applied for a structure?

The fields at the end of the structure should only be accessed if the structure is of the correct type.  In this case, ext.xrc_recv is only available if the qp type is xrc recv.

> 2. indeed, reality wise, new features, much of the time will also
> interact with existing data structures... so what happens if we extend a
> structure but the the extended strucutre is actually a field within
> another existing structure e.g suppose we want to extend ibv_ah_attr
> which is within ibv_qp_attr e.g to be used for the RAW Ethernet QPs. I
> don't see how we can be backward compatible with apps linked against
> libibverbs with the internal structure size being smaller, correct? so
> extended fields must be on the END always - in the actual structure they
> are added and if this structure is a field of another structure then we
> can't just extend it and need to come up with new structure which is in
> turn used as the field?

New features want to interact with existing structures and functions, which is what makes providing a clean separation difficult.  We can extend the structures using the above method as long as we have some sort type field available.  Where one is not available, we need to add one.  See the proposed struct ibv_srq for an example.  The extended SRQ type is only available by calling ibv_create_xsrq(), since ibv_create_srq() cannot know whether the user supports the extended ibv_srq_init_attr or not.
 
> 3. The usage of "#ifdef IBV_NEW_FEATURE_OPS" (IBV_XRC_OPS) in libmlx4
> will become tireding to follow maybe and could be replaced by placing
> dependency on a version of libibverbs that supports IBV_NEW_FEATURE_OPS?

Yes, you could make that replacement, which will work in this case.

> 4. can we somehow come up with a method to avoid IBV_NEW_FEATURE_OPS for
> --every-- new feature?
> 
> > or call an existing function with some new enum value

This probably depends on the feature.

> 
> 5. what happens if we just want to enhance an -- existing -- function -
> suppose we want to enhance ibv_post_send / ibv_poll_cq to support
> features like LSO, checksum offload, masked atomic operations, fast
> memory remote invalidate, etc so we add IBV_WR_NEW_FEATURE /
> IB_WC_NEW_FEATURE enum values, this step is simple, again if we go the
> way of the applicayion ensuring through dependencies that they are
> loaded against libibverbs that supports IB_{WR,WC}_NEW_FEATURE.
> 
> Now we come up with a need to extend struct ibv_send_wr and struct
> ibv_wc - where we should be very careful - walking on glasses for the
> backward compatibility thing - so only adding at the end on as union
> field which doesn't change the size of the union, correct? for
> ibv_post_send we can rely on the provider library to return error if
> they don't support NEW_FEATURE, but this would happen in run time... is
> there a way to avoid that, should we better go and add
> ib_post_send_new_feature? this would be very tedious doing for each
> new_feature and not applicable to ibv_poll_cq - any idea what should be
> done here?

ibv_wc and ibv_send_wr are allocated by the caller, so those are more difficult to deal with.  I agree that the size of those structures cannot change.  It may be possible that some of the features you mentioned could be set as part of the qp attributes (ibv_modify_qp), and for the others, I'm not sure.  Run time checks shouldn't be a big deal, since we already have to check things like ibv_wr_opcode and ibv_send_flags anyway.  But it could be that we require a new function, similar to ibv_create_xsrq.

- Sean


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Or Gerlitz]

On 1/2/2012 7:34 PM, Hefty, Sean wrote:
>> this field is a -- union -- how would that work if more than one 
>> extension is to be applied for a structure? 
> The fields at the end of the structure should only be accessed if the structure is of the correct type.  In this case, ext.xrc_recv is only available if the qp type is xrc recv.


Maybe I wasn't clear enough, I was thinking on a case where one extends 
a structure and later
on a 2nd extension is applied to the same structure, but the 
functionality / list of use cased related to the 1st extension isn't 
disjoint with that of the 2nd, e.g the second extends the first... maybe 
generally we can have a union named ext and on rare cases added ext2, etc?


>
>> 2. indeed, reality wise, new features, much of the time will also interact with existing data structures... so what happens if we extend a structure but the the extended strucutre is actually a field within another existing structure e.g suppose we want to extend ibv_ah_attr which is within ibv_qp_attr e.g to be used for the RAW Ethernet QPs. I
>> don't see how we can be backward compatible with apps linked against libibverbs with the internal structure size being smaller, correct? so extended fields must be on the END always - in the actual structure they are added and if this structure is a field of another structure then we can't just extend it and need to come up with new structure which is in turn used as the field?
>
> New features want to interact with existing structures and functions, which is what makes providing a clean separation difficult.  We can extend the structures using the above method as long as we have some sort type field available.  Where one is not available, we need to add one. See the proposed struct ibv_srq for an example.  The extended SRQ type is only available by calling ibv_create_xsrq(), since ibv_create_srq() cannot know whether the user supports the extended ibv_srq_init_attr or not.

Yes, I understand that, in 2nd thought, for the case of extending a 
structure which sits within another structure, e.g ibv_ah_attr within 
ibv_qp_attr, maybe we don't have much choice and rather add 
ib_ah_ext_attr and place it in the end of ibv_qp_attr ---> and here's a 
case for double extensions... as we need the "ext" union of ibv_qp_attr 
to contain struct  ibv_ah_ext_attr prim , struct  ibv_ah_ext_attr alt 
and possibly also struct qp_attr_ext qp_ext...


> ibv_wc and ibv_send_wr are allocated by the caller, so those are more difficult to deal with.  I agree that the size of those structures cannot change.  It may be possible that some of the features you mentioned could be set as part of the qp attributes (ibv_modify_qp), and for the others, I'm not sure.  Run time checks shouldn't be a big deal, since we already have to check things like ibv_wr_opcode and ibv_send_flags anyway.  But it could be that we require a new function, similar to ibv_create_xsrq.

I'd like to better understand the "allocated by the caller ... are more 
difficult to deal with" part of your response -  for ibv_send_wr - if 
the caller have set a new IBV_WR_NEW_FEATURE value for the wr type, they 
surely aware to the new fields and actually the size of the structure 
can change as of structs allocated by the library. As for ibv_wc, yep, 
looks like we can't change the size unless we want to write a copatility 
layer that also comes into play in fast path calls, specifically 
ibv_poll_cq and translates from the new ibv_wc to the old ibv_wc structure.

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Hefty, Sean]

> I'd like to better understand the "allocated by the caller ... are more
> difficult to deal with" part of your response -  for ibv_send_wr - if
> the caller have set a new IBV_WR_NEW_FEATURE value for the wr type, they
> surely aware to the new fields and actually the size of the structure
> can change as of structs allocated by the library. As for ibv_wc, yep,
> looks like we can't change the size unless we want to write a copatility
> layer that also comes into play in fast path calls, specifically
> ibv_poll_cq and translates from the new ibv_wc to the old ibv_wc structure.

You're right.  I was thinking more of ibv_wc, which has issues, since it may be used as an array.  ibv_send_wr is probably okay, since we walk a list using pointers.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] libibverbs: Allow 3rd party extensions to verb routines

[Jason Gunthorpe]

On Tue, Aug 02, 2011 at 10:53:05AM +0300, Jack Morgenstein wrote:

> These additions would include the new ibv_cmd_xxx functions
> (the core functions reside in src/cmd.c), and new, additional, enum values of
> the form IBV_USER_VERBS_CMD_XXXX, of which the core enum is in file
> include/infiniband/kern_abi.h.

I believe the only case where using an extension would make sense if
it if can be implemented entirely within the low level driver. So you
can't add new ibv_cmd calls to ibverbs, must duplicate them in your
driver, etc.

I'm a little unclear on how the application is going to get the access
enums and other structure definitions, though.. I suppose the low
level driver can install a .h file as well.
 
> Each new third-party package would need such a library.  While this
> will lead to a multiplicity of new libraries (one per addition), the
> core libibverbs package would remain as is.

I definitely don't want to see this..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html