* [PATCH for-next 00/13] IB/core: SG IOCTL based RDMA ABI
@ 2017-06-07 12:22 Matan Barak
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
Hi Doug,
This patch series adds ioctl() interface to the existing write() interface and
provide an easy route to backport this change to legacy supported systems.
Analyzing the current uverbs role in dispatching and parsing commands, we find
that:
(a) uverbs validates the basic properties of the command.
(b) uverbs is responsible of doing all the IDR and uobject management and
locking. It's also responsible for handling completion FDs.
(c) uverbs transforms the user<-->kernel ABI to kernel API.
(a) and (b) are valid for every kABI. Although the nature of commands could
change, they still have to be validated and transform to kernel pointers.
In order to avoid duplications between the various drivers, we would like to
keep (a) and (b) as shared code.
In addition, this is a good time to expand the ABI to be more scalable, so we
added a few goals:
(1) Command's attributes shall be extensible in an easy way. Either by allowing
drivers to have their own extensible set of attributes or core code
extensible attributes.
(2) Each driver may have specific type system (i.e QP, CQ, ....). It could extend
this type system in the future. Try to avoid duplicating existing types or
actions.
Thus, in order to allow this flexibility, we decide giving (a) and (b) as a
common infrastructure, but use per-driver guidelines in order to do that
parsing and uobject management. Handlers are also set by the drivers
themselves (though they can point to either shared common code) or
driver specific code.
We introduce a hierarchal object-method-attributes structure. Adding an
entity to this hierarchy doesn't affect the rest of the interface.
Such a hierarchy could be rooted in a specific device and describes both the
common features and features which are unique to this specific device.
This hierarchy is actually a per-device parsing tree, composed of three
layers - objects, actions and attributes. Each such layer contains two
groups - common entities and hardware specific entities. This way, a
device could add hardware specific actions to a common object, it could
add hardware specific objects, etc. Abstractions which really make sense,
should go to the common section. This means that we still need to be able to
pass optional parameters. In order to enable optional parameters, each command
is composed of a header and a bunch of TLVs to pass the attributes of this
command. The supported attribute classes are:
* PTR_IN (command) [in case of a small buffer, we could pass the data inlined]
* PTR_OUT (response)
* IDR_OBJECT
* FD_OBJECT
We differentiate between blobs and objects in order to allow a generic piece of
code in the kernel to do some syntactic validations and translate the given
user object id to a kernel structure. This could really help in sharing code
between different handlers.
Scatter gather was chosen in order to allow us not to recompile user space
drivers. By using pointers to driver specific data, we could just use it
without introduce copying data and without changing the user-space driver at
all.
We elevate the locking and IDR changes accepted to linux-rdma in this series.
Since types are no longer enforced by the common infrastructure, there is no
point of pre-allocating common IDR types in the common code. Instead, we
provide an API for driver to add new types. We use one IDR per context
for all its IDR types. The driver declared all its supported types, their
free function and release order. After that, all uboject, exclusive access
and types are handled automatically for the driver by the infrastructure.
When putting the pieces together, we have per-device parsing tree, that actually
describes all the objects, actions and attributes a device supports by using a
descriptive language. A command is given by the user-space, as a header plus an
array of Type-Length-Pointer/Object attributes. The ioctl callback executes a
generic code that shares as much logic between the various verbs handlers as
possible. This generic code gets the command input from the user-space and by
reading the device's parsing tree, it could syntactically validate it, grab all
required objects, lock them, call the right handler and then
commit/unlock/rollback the result, depending on the handler's result. Having
such a flexible extensible mechanism, that allows introducing new common and
hardware-specific to existing common attributes, but also allows adding new
hardware-specific entities, enhances the support for device diversity quite
vastly.
This series lays the foundations of such an infrastructure. It demonstrate the
CREATE_CQ and DESTROY_CQ handlers that use this new infrastructure with most of
its features. We still use a common parsing tree for all devices, but this will
be changed in the future, when we introduce a parse tree that is tailored for
devices. We still don't share code between the regular write() interface handlers
and the new ioctl(), but it should be done in the future too. An enhanced query
mechanism, based on the parse tree, is planned in the future as well.
After this infrastructure is merged, we could continue enhancing and build the
require functionality on top of it.
This is rebased over Doug's latest k.o/for-4.13 branch.
The series (with some extra untested/un-reviewed extra handlers) is available
here:
https://github.com/matanb10/linux branch: kabi_ioctl_v0
rdma-core code for testing:
https://github.com/matanb10/rdma-core branch: kabi_ioctl_testing
Regards,
Matan
Matan Barak (13):
IB/core: Add a generic way to execute an operation on a uobject
IB/core: Add support to finalize objects in one transaction
IB/core: Add new ioctl interface
IB/core: Declare a type instead of declaring only type attributes
IB/core: Add DEVICE type and root types structure
IB/core: Initialize uverbs types specification
IB/core: Add macros for declaring actions and attributes
IB/core: Explicitly destroy an object while keeping uobject
IB/core: Export ioctl enum types to user-space
IB/core: Add legacy driver's user-data
IB/core: Add completion queue (cq) object actions
IB/core: Assign root to all drivers
IB/core: Expose ioctl interface through experimental Kconfig
drivers/infiniband/Kconfig | 7 +
drivers/infiniband/core/Makefile | 2 +-
drivers/infiniband/core/rdma_core.c | 178 +++++++++++
drivers/infiniband/core/rdma_core.h | 42 +++
drivers/infiniband/core/uverbs.h | 5 +
drivers/infiniband/core/uverbs_ioctl.c | 418 +++++++++++++++++++++++++
drivers/infiniband/core/uverbs_main.c | 9 +
drivers/infiniband/core/uverbs_std_types.c | 288 ++++++++++++++---
drivers/infiniband/hw/bnxt_re/main.c | 5 +
drivers/infiniband/hw/cxgb3/iwch_provider.c | 5 +
drivers/infiniband/hw/cxgb4/provider.c | 5 +
drivers/infiniband/hw/hns/hns_roce_main.c | 5 +
drivers/infiniband/hw/i40iw/i40iw_verbs.c | 5 +
drivers/infiniband/hw/mlx4/main.c | 5 +
drivers/infiniband/hw/mlx5/main.c | 5 +
drivers/infiniband/hw/mthca/mthca_provider.c | 5 +
drivers/infiniband/hw/nes/nes_verbs.c | 5 +
drivers/infiniband/hw/ocrdma/ocrdma_main.c | 5 +
drivers/infiniband/hw/qedr/main.c | 5 +
drivers/infiniband/hw/usnic/usnic_ib_main.c | 5 +
drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 5 +
drivers/infiniband/sw/rdmavt/vt.c | 5 +
drivers/infiniband/sw/rxe/rxe_verbs.c | 5 +
include/rdma/ib_verbs.h | 2 +
include/rdma/uverbs_ioctl.h | 305 ++++++++++++++++++
include/rdma/uverbs_std_types.h | 45 +--
include/rdma/uverbs_types.h | 39 ++-
include/uapi/rdma/ib_user_ioctl_verbs.h | 84 +++++
include/uapi/rdma/rdma_user_ioctl.h | 24 ++
29 files changed, 1435 insertions(+), 88 deletions(-)
create mode 100644 drivers/infiniband/core/uverbs_ioctl.c
create mode 100644 include/rdma/uverbs_ioctl.h
create mode 100644 include/uapi/rdma/ib_user_ioctl_verbs.h
--
1.8.3.1
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH for-next 01/13] IB/core: Add a generic way to execute an operation on a uobject
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 02/13] IB/core: Add support to finalize objects in one transaction Matan Barak
` (12 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
The ioctl infrastructure treats all user-objects in the same manner.
It gets objects ids from the user-space and by using the object type
and operation mentioned in the action specification, it executes this
operation. An operation is split to two stages. The first one is
carried out before executing the handler and the second one is
executed afterwards.
The different supported operations are read, write, destroy and create.
In the first stage, the former three actions just fetches the object
from the repository (by using its id) and locks it. The last action
allocates a new uobject. The second stage is carried out after the
handler finishes and commits the result. The former two stages just
unlocks the object. Destroy calls the "free object" operation, taking
into account the object's type and releases the uobject as well.
Creation just adds the new uobject to the repository, making the
object visible to the application.
In order to abstract these details from the ioctl infrastructure
layer, we add uverbs_get_uobject_from_context and
uverbs_finalize_object functions which corresponds to the first
and second stages respectively.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/core/rdma_core.c | 58 +++++++++++++++++++++++++++++++++++++
drivers/infiniband/core/rdma_core.h | 17 +++++++++++
include/rdma/uverbs_ioctl.h | 52 +++++++++++++++++++++++++++++++++
3 files changed, 127 insertions(+)
create mode 100644 include/rdma/uverbs_ioctl.h
diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index 41c31a2..2bd58ff 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -35,6 +35,7 @@
#include <rdma/ib_verbs.h>
#include <rdma/uverbs_types.h>
#include <linux/rcupdate.h>
+#include <rdma/uverbs_ioctl.h>
#include "uverbs.h"
#include "core_priv.h"
#include "rdma_core.h"
@@ -625,3 +626,60 @@ void uverbs_initialize_ucontext(struct ib_ucontext *ucontext)
.needs_kfree_rcu = false,
};
+struct ib_uobject *uverbs_get_uobject_from_context(const struct uverbs_obj_type *type_attrs,
+ struct ib_ucontext *ucontext,
+ enum uverbs_obj_access access,
+ int id)
+{
+ switch (access) {
+ case UVERBS_ACCESS_READ:
+ return rdma_lookup_get_uobject(type_attrs, ucontext, id, false);
+ case UVERBS_ACCESS_DESTROY:
+ case UVERBS_ACCESS_WRITE:
+ return rdma_lookup_get_uobject(type_attrs, ucontext, id, true);
+ case UVERBS_ACCESS_NEW:
+ return rdma_alloc_begin_uobject(type_attrs, ucontext);
+ default:
+ WARN_ON(true);
+ return ERR_PTR(-EOPNOTSUPP);
+ }
+}
+
+int uverbs_finalize_object(struct ib_uobject *uobj,
+ enum uverbs_obj_access access,
+ bool commit)
+{
+ int ret = 0;
+
+ /*
+ * refcounts should be handled at the object level and not at the
+ * uobject level. Refcounts of the objects themselves are done in
+ * handlers.
+ */
+
+ switch (access) {
+ case UVERBS_ACCESS_READ:
+ rdma_lookup_put_uobject(uobj, false);
+ break;
+ case UVERBS_ACCESS_WRITE:
+ rdma_lookup_put_uobject(uobj, true);
+ break;
+ case UVERBS_ACCESS_DESTROY:
+ if (commit)
+ ret = rdma_remove_commit_uobject(uobj);
+ else
+ rdma_lookup_put_uobject(uobj, true);
+ break;
+ case UVERBS_ACCESS_NEW:
+ if (commit)
+ ret = rdma_alloc_commit_uobject(uobj);
+ else
+ rdma_alloc_abort_uobject(uobj);
+ break;
+ default:
+ WARN_ON(true);
+ ret = -EOPNOTSUPP;
+ }
+
+ return ret;
+}
diff --git a/drivers/infiniband/core/rdma_core.h b/drivers/infiniband/core/rdma_core.h
index 1b82e7f..97483d1 100644
--- a/drivers/infiniband/core/rdma_core.h
+++ b/drivers/infiniband/core/rdma_core.h
@@ -39,6 +39,7 @@
#include <linux/idr.h>
#include <rdma/uverbs_types.h>
+#include <rdma/uverbs_ioctl.h>
#include <rdma/ib_verbs.h>
#include <linux/mutex.h>
@@ -75,4 +76,20 @@
*/
void uverbs_close_fd(struct file *f);
+/*
+ * Get an ib_uobject that corresponds to the given id from ucontext, assuming
+ * the object is from the given type. Lock it to the required access when
+ * applicable.
+ * This function could create (access == NEW), destroy (access == DESTROY)
+ * or unlock (access == READ || access == WRITE) objects if required.
+ * The action will be finalized only when uverbs_finalize_object is called.
+ */
+struct ib_uobject *uverbs_get_uobject_from_context(const struct uverbs_obj_type *type_attrs,
+ struct ib_ucontext *ucontext,
+ enum uverbs_obj_access access,
+ int id);
+int uverbs_finalize_object(struct ib_uobject *uobj,
+ enum uverbs_obj_access access,
+ bool commit);
+
#endif /* RDMA_CORE_H */
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
new file mode 100644
index 0000000..4ff87ee
--- /dev/null
+++ b/include/rdma/uverbs_ioctl.h
@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2016, Mellanox Technologies inc. All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses. You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef _UVERBS_IOCTL_
+#define _UVERBS_IOCTL_
+
+#include <rdma/uverbs_types.h>
+
+/*
+ * =======================================
+ * Verbs action specifications
+ * =======================================
+ */
+
+enum uverbs_obj_access {
+ UVERBS_ACCESS_READ,
+ UVERBS_ACCESS_WRITE,
+ UVERBS_ACCESS_NEW,
+ UVERBS_ACCESS_DESTROY
+};
+
+#endif
+
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 02/13] IB/core: Add support to finalize objects in one transaction
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2017-06-07 12:22 ` [PATCH for-next 01/13] IB/core: Add a generic way to execute an operation on a uobject Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
[not found] ` <1496838172-39671-3-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2017-06-07 12:22 ` [PATCH for-next 03/13] IB/core: Add new ioctl interface Matan Barak
` (11 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
The new ioctl based infrastructure either commits or rollbacks
all objects of the command as one transaction. In order to do
that, we introduce a notion of dealing with a collection of
objects that are related to a specific action.
This also requires adding a notion of an action and attribute.
An action contains a groups of attributes, where each group
contains several attributes.
For example, an object could be a CQ, which has an action of CREATE_CQ.
This action has multiple attributes. For example, the CQ's new handle
and the comp_channel. Each layer in this hierarchy - objects, actions
and attributes is split into groups. The common example for that is
one group representing the common entities and another one
representing the driver specific entities.
When declaring these actions and attributes, we actually declare
their specifications. When a command is executed, we actually
allocates some space to hold auxiliary information. This auxiliary
information contains meta-data about the required objects, such
as pointers to their type information, pointers to the uobjects
themselves (if exist), etc.
The specification, along with the auxiliary information we allocated
and filled is given to the finalize_objects function.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/core/rdma_core.c | 39 ++++++++++++++++++++++++++++++
drivers/infiniband/core/rdma_core.h | 22 ++++++++++++++++-
include/rdma/uverbs_ioctl.h | 48 +++++++++++++++++++++++++++++++++++++
3 files changed, 108 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index 2bd58ff..3d3cf07 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -683,3 +683,42 @@ int uverbs_finalize_object(struct ib_uobject *uobj,
return ret;
}
+
+int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
+ struct uverbs_attr_spec_group ** const spec_groups,
+ size_t num,
+ bool commit)
+{
+ unsigned int i;
+ int ret = 0;
+
+ for (i = 0; i < num; i++) {
+ struct uverbs_attr_array *attr_group_array = &attr_array[i];
+ const struct uverbs_attr_spec_group *attr_spec_group =
+ spec_groups[i];
+ unsigned int j;
+
+ for (j = 0; j < attr_group_array->num_attrs; j++) {
+ struct uverbs_attr *attr;
+ struct uverbs_attr_spec *spec;
+
+ if (!uverbs_is_valid(attr_group_array, j))
+ continue;
+
+ attr = &attr_group_array->attrs[j];
+ spec = &attr_spec_group->attrs[j];
+
+ if (spec->type == UVERBS_ATTR_TYPE_IDR ||
+ spec->type == UVERBS_ATTR_TYPE_FD) {
+ int current_ret;
+
+ current_ret = uverbs_finalize_object(attr->obj_attr.uobject,
+ spec->obj.access,
+ commit);
+ if (!ret)
+ ret = current_ret;
+ }
+ }
+ }
+ return ret;
+}
diff --git a/drivers/infiniband/core/rdma_core.h b/drivers/infiniband/core/rdma_core.h
index 97483d1..5cc00eb 100644
--- a/drivers/infiniband/core/rdma_core.h
+++ b/drivers/infiniband/core/rdma_core.h
@@ -82,7 +82,8 @@
* applicable.
* This function could create (access == NEW), destroy (access == DESTROY)
* or unlock (access == READ || access == WRITE) objects if required.
- * The action will be finalized only when uverbs_finalize_object is called.
+ * The action will be finalized only when uverbs_finalize_object or
+ * uverbs_finalize_objects are called.
*/
struct ib_uobject *uverbs_get_uobject_from_context(const struct uverbs_obj_type *type_attrs,
struct ib_ucontext *ucontext,
@@ -91,5 +92,24 @@ struct ib_uobject *uverbs_get_uobject_from_context(const struct uverbs_obj_type
int uverbs_finalize_object(struct ib_uobject *uobj,
enum uverbs_obj_access access,
bool commit);
+/*
+ * Note that certain finalize stages could return a status:
+ * (a) alloc_commit could return a failure if the object is committed at the
+ * same time when the context is destroyed.
+ * (b) remove_commit could fail if the object wasn't destroyed successfully.
+ * Since multiple objects could be finalized in one transaction, it is very NOT
+ * recommended to have several finalize actions which have side effects.
+ * For example, it's NOT recommended to have a certain action which has both
+ * a commit action and a destroy action or two destroy objects in the same
+ * action. The rule of thumb is to have one destroy or commit action with
+ * multiple lookups.
+ * The first non zero return value of finalize_object is returned from this
+ * function. For example, this could happen when we couldn't destroy an
+ * object.
+ */
+int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
+ struct uverbs_attr_spec_group ** const spec_groups,
+ size_t num,
+ bool commit);
#endif /* RDMA_CORE_H */
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index 4ff87ee..e06f4cf 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -41,6 +41,12 @@
* =======================================
*/
+enum uverbs_attr_type {
+ UVERBS_ATTR_TYPE_NA,
+ UVERBS_ATTR_TYPE_IDR,
+ UVERBS_ATTR_TYPE_FD,
+};
+
enum uverbs_obj_access {
UVERBS_ACCESS_READ,
UVERBS_ACCESS_WRITE,
@@ -48,5 +54,47 @@ enum uverbs_obj_access {
UVERBS_ACCESS_DESTROY
};
+struct uverbs_attr_spec {
+ enum uverbs_attr_type type;
+ struct {
+ /*
+ * higher bits mean the group and lower bits mean
+ * the type id within the group.
+ */
+ u16 obj_type;
+ u8 access;
+ } obj;
+};
+
+struct uverbs_attr_spec_group {
+ struct uverbs_attr_spec *attrs;
+ size_t num_attrs;
+};
+
+struct uverbs_obj_attr {
+ struct ib_uobject *uobject;
+};
+
+struct uverbs_attr {
+ struct uverbs_obj_attr obj_attr;
+};
+
+struct uverbs_attr_array {
+ /* if bit i is set, it means attrs[i] contains valid information */
+ unsigned long *valid_bitmap;
+ size_t num_attrs;
+ /*
+ * arrays of attributes, each element corresponds to the specification
+ * of the attribute in the same index.
+ */
+ struct uverbs_attr *attrs;
+};
+
+static inline bool uverbs_is_valid(const struct uverbs_attr_array *attr_array,
+ unsigned int idx)
+{
+ return test_bit(idx, attr_array->valid_bitmap);
+}
+
#endif
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2017-06-07 12:22 ` [PATCH for-next 01/13] IB/core: Add a generic way to execute an operation on a uobject Matan Barak
2017-06-07 12:22 ` [PATCH for-next 02/13] IB/core: Add support to finalize objects in one transaction Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
[not found] ` <1496838172-39671-4-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2017-06-07 12:22 ` [PATCH for-next 04/13] IB/core: Declare a type instead of declaring only type attributes Matan Barak
` (10 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
In this ioctl interface, processing the command starts from
properties of the command and fetching the appropriate user objects
before calling the handler.
Parsing and validation is done according to a specifier declared by
the driver's code. In the driver, all supported types are declared.
These types are separated to different type groups, each could be
declared in a different place (for example, common types and driver
specific types).
For each type we list all supported actions. Similarly to types,
actions are separated to actions groups too. Each group is declared
separately. This could be used in order to add actions to an existing
type.
Each action has a specific handler, which could be either a common
handler or a driver specific handler.
Along with the handler, a group of attributes is specified as well.
This group lists all supported attributes and is used for automatic
fetching and validation of the command, response and its related
objects.
When a group of elements is used, the high bits of the elements ids
are used in order to calculate the group index. Then, these high bits
are masked out in order to have a zero based namespace for every
group. This is mandatory for compact representation and O(1) array
access.
A group of attributes is actually an array of attributes. Each
attribute has a type (PTR_IN, PTR_OUT, IDR and FD) and a length.
Attributes could be validated through some attributes, like:
(*) Minimum size / Exact size
(*) Fops for FD
(*) Object type for IDR
If an IDR/fd attribute is specified, the kernel also states the object
type and the required access (NEW, WRITE, READ or DESTROY).
All uobject/fd management is done automatically by the infrastructure,
meaning - the infrastructure will fail concurrent commands that at
least one of them requires concurrent access (WRITE/DESTROY),
synchronize actions with device removals (dissociate context events)
and take care of reference counting (increase/decrease) for concurrent
actions invocation. The reference counts on the actual kernel objects
shall be handled by the handlers.
types
+--------+
| |
| | actions +--------+
| | group action action_spec +-----+ |len |
+--------+ +------+[d]+-------+ +----------------+[d]+------------+ |attr1+-> |type |
| type +> |action+-> | spec +-> + attr_groups +-> |common_chain+--> +-----+ |idr_type|
+--------+ +------+ |handler| | | +------------+ |attr2| |access |
| | | | +-------+ +----------------+ |vendor chain| +-----+ +--------+
| | | | +------------+
| | +------+
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
+--------+
[d] = distribute ids to groups using the high order bits
The right types table is also chosen by using the high bits from
the ids. Currently we have either common or driver specific groups.
Once validation and object fetching (or creation) completed, we call
the handler:
int (*handler)(struct ib_device *ib_dev, struct ib_uverbs_file *ufile,
struct uverbs_attr_array *ctx, size_t num);
Where ctx is an array of uverbs_attr_array. Each element in this array
is an array of attributes which corresponds to one group of attributes.
For example, in the usually used case:
ctx core
+----------------------------+ +------------+
| core: uverbs_attr_array +---> | valid |
+----------------------------+ | cmd_attr |
| driver: uverbs_attr_array | +------------+
|----------------------------+--+ | valid |
| | cmd_attr |
| +------------+
| | valid |
| | obj_attr |
| +------------+
|
| vendor
| +------------+
+> | valid |
| cmd_attr |
+------------+
| valid |
| cmd_attr |
+------------+
| valid |
| obj_attr |
+------------+
Ctx array's indices corresponds to the attributes groups order. The indices
of core and driver corresponds to the attributes name spaces of each
group. Thus, we could think of the following as one object:
1. Set of attribute specification (with their attribute IDs)
2. Attribute group which owns (1) specifications
3. A function which could handle this attributes which the handler
could call
4. The allocation descriptor of this type uverbs_obj_type.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/core/Makefile | 2 +-
drivers/infiniband/core/rdma_core.c | 45 +++++
drivers/infiniband/core/rdma_core.h | 5 +
drivers/infiniband/core/uverbs_ioctl.c | 360 +++++++++++++++++++++++++++++++++
include/rdma/ib_verbs.h | 2 +
include/rdma/uverbs_ioctl.h | 100 ++++++++-
include/uapi/rdma/rdma_user_ioctl.h | 24 +++
7 files changed, 528 insertions(+), 10 deletions(-)
create mode 100644 drivers/infiniband/core/uverbs_ioctl.c
diff --git a/drivers/infiniband/core/Makefile b/drivers/infiniband/core/Makefile
index 6ebd9ad..e18f2f8 100644
--- a/drivers/infiniband/core/Makefile
+++ b/drivers/infiniband/core/Makefile
@@ -30,4 +30,4 @@ ib_umad-y := user_mad.o
ib_ucm-y := ucm.o
ib_uverbs-y := uverbs_main.o uverbs_cmd.o uverbs_marshall.o \
- rdma_core.o uverbs_std_types.o
+ rdma_core.o uverbs_std_types.o uverbs_ioctl.o
diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index 3d3cf07..d16a519 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -40,6 +40,51 @@
#include "core_priv.h"
#include "rdma_core.h"
+int uverbs_group_idx(u16 *id, unsigned int ngroups)
+{
+ int ret = (*id & UVERBS_ID_GROUP_MASK) >> UVERBS_ID_GROUP_SHIFT;
+
+ if (ret >= ngroups)
+ return -EINVAL;
+
+ *id &= ~UVERBS_ID_GROUP_MASK;
+ return ret;
+}
+
+const struct uverbs_type *uverbs_get_type(const struct ib_device *ibdev,
+ uint16_t type)
+{
+ const struct uverbs_spec_root *groups = ibdev->specs_root;
+ const struct uverbs_type_group *types;
+ int ret = uverbs_group_idx(&type, groups->num_groups);
+
+ if (ret < 0)
+ return NULL;
+
+ types = groups->type_groups[ret];
+
+ if (type >= types->num_types)
+ return NULL;
+
+ return types->types[type];
+}
+
+const struct uverbs_action *uverbs_get_action(const struct uverbs_type *type,
+ uint16_t action)
+{
+ const struct uverbs_action_group *action_group;
+ int ret = uverbs_group_idx(&action, type->num_groups);
+
+ if (ret < 0)
+ return NULL;
+
+ action_group = type->action_groups[ret];
+ if (action >= action_group->num_actions)
+ return NULL;
+
+ return action_group->actions[action];
+}
+
void uverbs_uobject_get(struct ib_uobject *uobject)
{
kref_get(&uobject->ref);
diff --git a/drivers/infiniband/core/rdma_core.h b/drivers/infiniband/core/rdma_core.h
index 5cc00eb..1014a8a 100644
--- a/drivers/infiniband/core/rdma_core.h
+++ b/drivers/infiniband/core/rdma_core.h
@@ -43,6 +43,11 @@
#include <rdma/ib_verbs.h>
#include <linux/mutex.h>
+int uverbs_group_idx(u16 *id, unsigned int ngroups);
+const struct uverbs_type *uverbs_get_type(const struct ib_device *ibdev,
+ uint16_t type);
+const struct uverbs_action *uverbs_get_action(const struct uverbs_type *type,
+ uint16_t action);
/*
* These functions initialize the context and cleanups its uobjects.
* The context has a list of objects which is protected by a mutex
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
new file mode 100644
index 0000000..a375a7b
--- /dev/null
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -0,0 +1,360 @@
+/*
+ * Copyright (c) 2017, Mellanox Technologies inc. All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses. You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <rdma/rdma_user_ioctl.h>
+#include <rdma/uverbs_ioctl.h>
+#include "rdma_core.h"
+#include "uverbs.h"
+
+static int uverbs_process_attr(struct ib_device *ibdev,
+ struct ib_ucontext *ucontext,
+ const struct ib_uverbs_attr *uattr,
+ u16 attr_id,
+ const struct uverbs_attr_spec_group *attr_spec_group,
+ struct uverbs_attr_array *attr_array,
+ struct ib_uverbs_attr __user *uattr_ptr)
+{
+ const struct uverbs_attr_spec *spec;
+ struct uverbs_attr *e;
+ const struct uverbs_type *type;
+ struct uverbs_obj_attr *o_attr;
+ struct uverbs_attr *elements = attr_array->attrs;
+
+ if (uattr->reserved)
+ return -EINVAL;
+
+ if (attr_id >= attr_spec_group->num_attrs) {
+ if (uattr->flags & UVERBS_ATTR_F_MANDATORY)
+ return -EINVAL;
+ else
+ return 0;
+ }
+
+ spec = &attr_spec_group->attrs[attr_id];
+ e = &elements[attr_id];
+
+ switch (spec->type) {
+ case UVERBS_ATTR_TYPE_PTR_IN:
+ case UVERBS_ATTR_TYPE_PTR_OUT:
+ if (uattr->len < spec->len ||
+ (!(spec->flags & UVERBS_ATTR_SPEC_F_MIN_SZ) &&
+ uattr->len > spec->len))
+ return -EINVAL;
+
+ e->ptr_attr.ptr = (__force void __user *)uattr->data;
+ e->ptr_attr.len = uattr->len;
+ break;
+
+ case UVERBS_ATTR_TYPE_IDR:
+ if (uattr->data >> 32)
+ return -EINVAL;
+ /* fall through */
+ case UVERBS_ATTR_TYPE_FD:
+ if (uattr->len != 0 || !ucontext || uattr->data > INT_MAX)
+ return -EINVAL;
+
+ o_attr = &e->obj_attr;
+ type = uverbs_get_type(ibdev, spec->obj.obj_type);
+ if (!type)
+ return -EINVAL;
+ o_attr->type = type->type_attrs;
+ o_attr->uattr = uattr_ptr;
+
+ o_attr->id = (int)uattr->data;
+ o_attr->uobject = uverbs_get_uobject_from_context(
+ o_attr->type,
+ ucontext,
+ spec->obj.access,
+ o_attr->id);
+
+ if (IS_ERR(o_attr->uobject))
+ return PTR_ERR(o_attr->uobject);
+
+ if (spec->obj.access == UVERBS_ACCESS_NEW) {
+ u64 id = o_attr->uobject->id;
+
+ /* Copy the allocated id to the user-space */
+ if (put_user(id, &o_attr->uattr->data)) {
+ uverbs_finalize_object(o_attr->uobject,
+ UVERBS_ACCESS_NEW,
+ false);
+ return -EFAULT;
+ }
+ }
+
+ break;
+ default:
+ return -EOPNOTSUPP;
+ };
+
+ set_bit(attr_id, attr_array->valid_bitmap);
+ return 0;
+}
+
+static int uverbs_uattrs_process(struct ib_device *ibdev,
+ struct ib_ucontext *ucontext,
+ const struct ib_uverbs_attr *uattrs,
+ size_t num_uattrs,
+ const struct uverbs_action *action,
+ struct uverbs_attr_array *attr_array,
+ struct ib_uverbs_attr __user *uattr_ptr)
+{
+ size_t i;
+ int ret = 0;
+ int num_given_groups = 0;
+
+ for (i = 0; i < num_uattrs; i++) {
+ const struct ib_uverbs_attr *uattr = &uattrs[i];
+ u16 attr_id = uattr->attr_id;
+ const struct uverbs_attr_spec_group *attr_spec_group;
+
+ ret = uverbs_group_idx(&attr_id, action->num_groups);
+ if (ret < 0) {
+ if (uattr->flags & UVERBS_ATTR_F_MANDATORY) {
+ uverbs_finalize_objects(attr_array,
+ action->attr_groups,
+ num_given_groups,
+ false);
+ return ret;
+ }
+ continue;
+ }
+
+ /*
+ * ret is the found group, so increase num_give_groups if
+ * necessary.
+ */
+ if (ret >= num_given_groups)
+ num_given_groups = ret + 1;
+
+ attr_spec_group = action->attr_groups[ret];
+ ret = uverbs_process_attr(ibdev, ucontext, uattr, attr_id,
+ attr_spec_group, &attr_array[ret],
+ uattr_ptr++);
+ if (ret) {
+ uverbs_finalize_objects(attr_array,
+ action->attr_groups,
+ num_given_groups,
+ false);
+ return ret;
+ }
+ }
+
+ return num_given_groups;
+}
+
+static int uverbs_validate_kernel_mandatory(const struct uverbs_action *action,
+ struct uverbs_attr_array *attr_array,
+ unsigned int num_given_groups)
+{
+ unsigned int i;
+
+ for (i = 0; i < num_given_groups; i++) {
+ const struct uverbs_attr_spec_group *attr_spec_group =
+ action->attr_groups[i];
+
+ if (!bitmap_subset(attr_spec_group->mandatory_attrs_bitmask,
+ attr_array[i].valid_bitmap,
+ attr_spec_group->num_attrs))
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int uverbs_handle_action(struct ib_uverbs_attr __user *uattr_ptr,
+ const struct ib_uverbs_attr *uattrs,
+ size_t num_uattrs,
+ struct ib_device *ibdev,
+ struct ib_uverbs_file *ufile,
+ const struct uverbs_action *action,
+ struct uverbs_attr_array *attr_array)
+{
+ int ret;
+ int finalize_ret;
+ int num_given_groups;
+
+ num_given_groups = uverbs_uattrs_process(ibdev, ufile->ucontext, uattrs,
+ num_uattrs, action, attr_array,
+ uattr_ptr);
+ if (num_given_groups <= 0)
+ return -EINVAL;
+
+ ret = uverbs_validate_kernel_mandatory(action, attr_array,
+ num_given_groups);
+ if (ret)
+ goto cleanup;
+
+ ret = action->handler(ibdev, ufile, attr_array, num_given_groups);
+cleanup:
+ finalize_ret = uverbs_finalize_objects(attr_array, action->attr_groups,
+ num_given_groups, !ret);
+
+ return ret ? ret : finalize_ret;
+}
+
+#define UVERBS_OPTIMIZE_USING_STACK_SZ 256
+static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev,
+ struct ib_uverbs_file *file,
+ struct ib_uverbs_ioctl_hdr *hdr,
+ void __user *buf)
+{
+ const struct uverbs_type *type;
+ const struct uverbs_action *action;
+ long err = 0;
+ unsigned int i;
+ struct {
+ struct ib_uverbs_attr *uattrs;
+ struct uverbs_attr_array *uverbs_attr_array;
+ } *ctx = NULL;
+ struct uverbs_attr *curr_attr;
+ unsigned long *curr_bitmap;
+ size_t ctx_size;
+#ifdef UVERBS_OPTIMIZE_USING_STACK_SZ
+ uintptr_t data[UVERBS_OPTIMIZE_USING_STACK_SZ / sizeof(uintptr_t)];
+#endif
+
+ if (hdr->reserved)
+ return -EINVAL;
+
+ type = uverbs_get_type(ib_dev, hdr->object_type);
+ if (!type)
+ return -EOPNOTSUPP;
+
+ action = uverbs_get_action(type, hdr->action);
+ if (!action)
+ return -EOPNOTSUPP;
+
+ if ((action->flags & UVERBS_ACTION_FLAG_CREATE_ROOT) ^ !file->ucontext)
+ return -EINVAL;
+
+ ctx_size = sizeof(*ctx) +
+ sizeof(struct uverbs_attr_array) * action->num_groups +
+ sizeof(*ctx->uattrs) * hdr->num_attrs +
+ sizeof(*ctx->uverbs_attr_array->attrs) *
+ action->num_child_attrs +
+ sizeof(*ctx->uverbs_attr_array->valid_bitmap) *
+ (action->num_child_attrs / BITS_PER_LONG +
+ action->num_groups);
+
+#ifdef UVERBS_OPTIMIZE_USING_STACK_SZ
+ if (ctx_size <= UVERBS_OPTIMIZE_USING_STACK_SZ)
+ ctx = (void *)data;
+
+ if (!ctx)
+#endif
+ ctx = kmalloc(ctx_size, GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->uverbs_attr_array = (void *)ctx + sizeof(*ctx);
+ ctx->uattrs = (void *)(ctx->uverbs_attr_array +
+ action->num_groups);
+ curr_attr = (void *)(ctx->uattrs + hdr->num_attrs);
+ curr_bitmap = (void *)(curr_attr + action->num_child_attrs);
+
+ /*
+ * We just fill the pointers and num_attrs here. The data itself will be
+ * filled at a later stage (uverbs_process_attr)
+ */
+ for (i = 0; i < action->num_groups; i++) {
+ unsigned int curr_num_attrs = action->attr_groups[i]->num_attrs;
+
+ ctx->uverbs_attr_array[i].attrs = curr_attr;
+ curr_attr += curr_num_attrs;
+ ctx->uverbs_attr_array[i].num_attrs = curr_num_attrs;
+ ctx->uverbs_attr_array[i].valid_bitmap = curr_bitmap;
+ bitmap_zero(curr_bitmap, curr_num_attrs);
+ curr_bitmap += BITS_TO_LONGS(curr_num_attrs);
+ }
+
+ err = copy_from_user(ctx->uattrs, buf,
+ sizeof(*ctx->uattrs) * hdr->num_attrs);
+ if (err) {
+ err = -EFAULT;
+ goto out;
+ }
+
+ err = uverbs_handle_action(buf, ctx->uattrs, hdr->num_attrs, ib_dev,
+ file, action, ctx->uverbs_attr_array);
+out:
+#ifdef UVERBS_OPTIMIZE_USING_STACK_SZ
+ if (ctx_size > UVERBS_OPTIMIZE_USING_STACK_SZ)
+#endif
+ kfree(ctx);
+ return err;
+}
+
+#define IB_UVERBS_MAX_CMD_SZ 4096
+
+long ib_uverbs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+ struct ib_uverbs_file *file = filp->private_data;
+ struct ib_uverbs_ioctl_hdr __user *user_hdr =
+ (struct ib_uverbs_ioctl_hdr __user *)arg;
+ struct ib_uverbs_ioctl_hdr hdr;
+ struct ib_device *ib_dev;
+ int srcu_key;
+ long err;
+
+ srcu_key = srcu_read_lock(&file->device->disassociate_srcu);
+ ib_dev = srcu_dereference(file->device->ib_dev,
+ &file->device->disassociate_srcu);
+ if (!ib_dev) {
+ err = -EIO;
+ goto out;
+ }
+
+ if (cmd == RDMA_VERBS_IOCTL) {
+ err = copy_from_user(&hdr, user_hdr, sizeof(hdr));
+
+ if (err || hdr.length > IB_UVERBS_MAX_CMD_SZ ||
+ hdr.length != sizeof(hdr) + hdr.num_attrs * sizeof(struct ib_uverbs_attr)) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ if (hdr.reserved) {
+ err = -EOPNOTSUPP;
+ goto out;
+ }
+
+ err = ib_uverbs_cmd_verbs(ib_dev, file, &hdr,
+ (__user void *)arg + sizeof(hdr));
+ } else {
+ err = -ENOIOCTLCMD;
+ }
+out:
+ srcu_read_unlock(&file->device->disassociate_srcu, srcu_key);
+
+ return err;
+}
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index ba8314e..36c5cc7 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2245,6 +2245,8 @@ struct ib_device {
*/
int (*get_port_immutable)(struct ib_device *, u8, struct ib_port_immutable *);
void (*get_dev_fw_str)(struct ib_device *, char *str, size_t str_len);
+
+ const struct uverbs_spec_root *specs_root;
};
struct ib_client {
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index e06f4cf..7fed6d9 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -41,8 +41,13 @@
* =======================================
*/
+#define UVERBS_ID_GROUP_MASK 0xF000
+#define UVERBS_ID_GROUP_SHIFT 12
+
enum uverbs_attr_type {
UVERBS_ATTR_TYPE_NA,
+ UVERBS_ATTR_TYPE_PTR_IN,
+ UVERBS_ATTR_TYPE_PTR_OUT,
UVERBS_ATTR_TYPE_IDR,
UVERBS_ATTR_TYPE_FD,
};
@@ -54,29 +59,106 @@ enum uverbs_obj_access {
UVERBS_ACCESS_DESTROY
};
+enum uverbs_attr_spec_flags {
+ UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
+ /* Support extending attributes by length */
+ UVERBS_ATTR_SPEC_F_MIN_SZ = 1U << 1,
+};
+
struct uverbs_attr_spec {
enum uverbs_attr_type type;
- struct {
- /*
- * higher bits mean the group and lower bits mean
- * the type id within the group.
- */
- u16 obj_type;
- u8 access;
- } obj;
+ /* a combination of enum uverbs_attr_spec_flags */
+ u8 flags;
+ union {
+ u16 len;
+ struct {
+ /*
+ * higher bits mean the group and lower bits mean
+ * the type id within the group.
+ */
+ u16 obj_type;
+ u8 access;
+ } obj;
+ };
};
struct uverbs_attr_spec_group {
struct uverbs_attr_spec *attrs;
size_t num_attrs;
+ /* populate at runtime */
+ unsigned long *mandatory_attrs_bitmask;
+};
+
+struct uverbs_attr_array;
+struct ib_uverbs_file;
+
+enum uverbs_action_flags {
+ /*
+ * Action marked with this flag creates a context (or root for all
+ * objects).
+ */
+ UVERBS_ACTION_FLAG_CREATE_ROOT = 1U << 0,
+};
+
+struct uverbs_action {
+ struct uverbs_attr_spec_group **attr_groups;
+ size_t num_groups;
+ size_t num_child_attrs;
+ /* Combination of bits from enum uverbs_action_flags */
+ u32 flags;
+ int (*handler)(struct ib_device *ib_dev, struct ib_uverbs_file *ufile,
+ struct uverbs_attr_array *ctx, size_t num);
+};
+
+struct uverbs_action_group {
+ size_t num_actions;
+ struct uverbs_action **actions;
+};
+
+struct uverbs_type {
+ size_t num_groups;
+ const struct uverbs_action_group **action_groups;
+ const struct uverbs_obj_type *type_attrs;
+};
+
+struct uverbs_type_group {
+ size_t num_types;
+ const struct uverbs_type **types;
+};
+
+struct uverbs_spec_root {
+ const struct uverbs_type_group **type_groups;
+ size_t num_groups;
+};
+
+/* =================================================
+ * Parsing infrastructure
+ * =================================================
+ */
+
+struct uverbs_ptr_attr {
+ void __user *ptr;
+ u16 len;
};
struct uverbs_obj_attr {
+ /*
+ * pointer to the user-space given attribute, in order to write the
+ * new uobject's id.
+ */
+ struct ib_uverbs_attr __user *uattr;
+ /* pointer to the kernel descriptor -> type, access, etc */
+ const struct uverbs_obj_type *type;
struct ib_uobject *uobject;
+ /* fd or id in idr of this object */
+ int id;
};
struct uverbs_attr {
- struct uverbs_obj_attr obj_attr;
+ union {
+ struct uverbs_ptr_attr ptr_attr;
+ struct uverbs_obj_attr obj_attr;
+ };
};
struct uverbs_attr_array {
diff --git a/include/uapi/rdma/rdma_user_ioctl.h b/include/uapi/rdma/rdma_user_ioctl.h
index 9388125..3a40b9d 100644
--- a/include/uapi/rdma/rdma_user_ioctl.h
+++ b/include/uapi/rdma/rdma_user_ioctl.h
@@ -43,6 +43,30 @@
/* Legacy name, for user space application which already use it */
#define IB_IOCTL_MAGIC RDMA_IOCTL_MAGIC
+#define RDMA_VERBS_IOCTL \
+ _IOWR(RDMA_IOCTL_MAGIC, 1, struct ib_uverbs_ioctl_hdr)
+
+enum ib_uverbs_attr_flags {
+ UVERBS_ATTR_F_MANDATORY = 1U << 0,
+};
+
+struct ib_uverbs_attr {
+ __u16 attr_id; /* command specific type attribute */
+ __u16 len; /* only for pointers */
+ __u16 flags; /* combination of ib_uverbs_attr_flags */
+ __u16 reserved;
+ __u64 data; /* ptr to command, inline data or idr/fd */
+};
+
+struct ib_uverbs_ioctl_hdr {
+ __u16 length;
+ __u16 object_type;
+ __u16 action;
+ __u16 num_attrs;
+ __u64 reserved;
+ struct ib_uverbs_attr attrs[0];
+};
+
/*
* General blocks assignments
* It is closed on purpose do not expose it it user space
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 04/13] IB/core: Declare a type instead of declaring only type attributes
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (2 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 03/13] IB/core: Add new ioctl interface Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 05/13] IB/core: Add DEVICE type and root types structure Matan Barak
` (9 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
Switch all uverbs_type_attrs_xxxx with DECLARE_UVERBS_TYPE
macros. This will be later used in order to embed the types
specific actions in the types as well.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/core/uverbs_std_types.c | 89 ++++++++++++------------------
include/rdma/uverbs_ioctl.h | 4 ++
include/rdma/uverbs_std_types.h | 38 ++++++-------
include/rdma/uverbs_types.h | 38 ++++++++-----
4 files changed, 82 insertions(+), 87 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index ef29337..a204975 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -209,67 +209,50 @@ static int uverbs_hot_unplug_completion_event_file(struct ib_uobject_file *uobj_
return 0;
};
-const struct uverbs_obj_fd_type uverbs_type_attrs_comp_channel = {
- .type = UVERBS_TYPE_ALLOC_FD(sizeof(struct ib_uverbs_completion_event_file), 0),
- .context_closed = uverbs_hot_unplug_completion_event_file,
- .fops = &uverbs_event_fops,
- .name = "[infinibandevent]",
- .flags = O_RDONLY,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_comp_channel,
+ &UVERBS_TYPE_ALLOC_FD(0,
+ sizeof(struct ib_uverbs_completion_event_file),
+ uverbs_hot_unplug_completion_event_file,
+ &uverbs_event_fops,
+ "[infinibandevent]", O_RDONLY));
-const struct uverbs_obj_idr_type uverbs_type_attrs_cq = {
- .type = UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0),
- .destroy_object = uverbs_free_cq,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_cq,
+ &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0,
+ uverbs_free_cq));
-const struct uverbs_obj_idr_type uverbs_type_attrs_qp = {
- .type = UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uqp_object), 0),
- .destroy_object = uverbs_free_qp,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_qp,
+ &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uqp_object), 0,
+ uverbs_free_qp));
-const struct uverbs_obj_idr_type uverbs_type_attrs_mw = {
- .type = UVERBS_TYPE_ALLOC_IDR(0),
- .destroy_object = uverbs_free_mw,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_mw,
+ &UVERBS_TYPE_ALLOC_IDR(0, uverbs_free_mw));
-const struct uverbs_obj_idr_type uverbs_type_attrs_mr = {
- /* 1 is used in order to free the MR after all the MWs */
- .type = UVERBS_TYPE_ALLOC_IDR(1),
- .destroy_object = uverbs_free_mr,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_mr,
+ /* 1 is used in order to free the MR after all the MWs */
+ &UVERBS_TYPE_ALLOC_IDR(1, uverbs_free_mr));
-const struct uverbs_obj_idr_type uverbs_type_attrs_srq = {
- .type = UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_usrq_object), 0),
- .destroy_object = uverbs_free_srq,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_srq,
+ &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_usrq_object), 0,
+ uverbs_free_srq));
-const struct uverbs_obj_idr_type uverbs_type_attrs_ah = {
- .type = UVERBS_TYPE_ALLOC_IDR(0),
- .destroy_object = uverbs_free_ah,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_ah,
+ &UVERBS_TYPE_ALLOC_IDR(0, uverbs_free_ah));
-const struct uverbs_obj_idr_type uverbs_type_attrs_flow = {
- .type = UVERBS_TYPE_ALLOC_IDR(0),
- .destroy_object = uverbs_free_flow,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_flow,
+ &UVERBS_TYPE_ALLOC_IDR(0, uverbs_free_flow));
-const struct uverbs_obj_idr_type uverbs_type_attrs_wq = {
- .type = UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uwq_object), 0),
- .destroy_object = uverbs_free_wq,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_wq,
+ &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uwq_object), 0,
+ uverbs_free_wq));
-const struct uverbs_obj_idr_type uverbs_type_attrs_rwq_ind_table = {
- .type = UVERBS_TYPE_ALLOC_IDR(0),
- .destroy_object = uverbs_free_rwq_ind_tbl,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_rwq_ind_table,
+ &UVERBS_TYPE_ALLOC_IDR(0, uverbs_free_rwq_ind_tbl));
-const struct uverbs_obj_idr_type uverbs_type_attrs_xrcd = {
- .type = UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uxrcd_object), 0),
- .destroy_object = uverbs_free_xrcd,
-};
+DECLARE_UVERBS_TYPE(uverbs_type_xrcd,
+ &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uxrcd_object), 0,
+ uverbs_free_xrcd));
+
+DECLARE_UVERBS_TYPE(uverbs_type_pd,
+ /* 2 is used in order to free the PD after MRs */
+ &UVERBS_TYPE_ALLOC_IDR(2, uverbs_free_pd));
-const struct uverbs_obj_idr_type uverbs_type_attrs_pd = {
- /* 2 is used in order to free the PD after MRs */
- .type = UVERBS_TYPE_ALLOC_IDR(2),
- .destroy_object = uverbs_free_pd,
-};
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index 7fed6d9..ee89cce 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -131,6 +131,10 @@ struct uverbs_spec_root {
size_t num_groups;
};
+#define DECLARE_UVERBS_TYPE(name, _type_attrs) \
+ const struct uverbs_type name = { \
+ .type_attrs = _type_attrs, \
+ }
/* =================================================
* Parsing infrastructure
* =================================================
diff --git a/include/rdma/uverbs_std_types.h b/include/rdma/uverbs_std_types.h
index 7771ce9..0810faf 100644
--- a/include/rdma/uverbs_std_types.h
+++ b/include/rdma/uverbs_std_types.h
@@ -35,18 +35,18 @@
#include <rdma/uverbs_types.h>
-extern const struct uverbs_obj_fd_type uverbs_type_attrs_comp_channel;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_cq;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_qp;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_rwq_ind_table;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_wq;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_srq;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_ah;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_flow;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_mr;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_mw;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_pd;
-extern const struct uverbs_obj_idr_type uverbs_type_attrs_xrcd;
+extern const struct uverbs_type uverbs_type_comp_channel;
+extern const struct uverbs_type uverbs_type_cq;
+extern const struct uverbs_type uverbs_type_qp;
+extern const struct uverbs_type uverbs_type_rwq_ind_table;
+extern const struct uverbs_type uverbs_type_wq;
+extern const struct uverbs_type uverbs_type_srq;
+extern const struct uverbs_type uverbs_type_ah;
+extern const struct uverbs_type uverbs_type_flow;
+extern const struct uverbs_type uverbs_type_mr;
+extern const struct uverbs_type uverbs_type_mw;
+extern const struct uverbs_type uverbs_type_pd;
+extern const struct uverbs_type uverbs_type_xrcd;
static inline struct ib_uobject *__uobj_get(const struct uverbs_obj_type *type,
bool write,
@@ -56,22 +56,22 @@ static inline struct ib_uobject *__uobj_get(const struct uverbs_obj_type *type,
return rdma_lookup_get_uobject(type, ucontext, id, write);
}
-#define uobj_get_type(_type) uverbs_type_attrs_##_type.type
+#define uobj_get_type(_type) uverbs_type_##_type.type_attrs
#define uobj_get_read(_type, _id, _ucontext) \
- __uobj_get(&(_type), false, _ucontext, _id)
+ __uobj_get(_type, false, _ucontext, _id)
#define uobj_get_obj_read(_type, _id, _ucontext) \
({ \
- struct ib_uobject *uobj = \
- __uobj_get(&uobj_get_type(_type), \
+ struct ib_uobject *__uobj = \
+ __uobj_get(uverbs_type_##_type.type_attrs, \
false, _ucontext, _id); \
\
- (struct ib_##_type *)(IS_ERR(uobj) ? NULL : uobj->object); \
+ (struct ib_##_type *)(IS_ERR(__uobj) ? NULL : __uobj->object); \
})
#define uobj_get_write(_type, _id, _ucontext) \
- __uobj_get(&(_type), true, _ucontext, _id)
+ __uobj_get(_type, true, _ucontext, _id)
static inline void uobj_put_read(struct ib_uobject *uobj)
{
@@ -108,7 +108,7 @@ static inline struct ib_uobject *__uobj_alloc(const struct uverbs_obj_type *type
}
#define uobj_alloc(_type, ucontext) \
- __uobj_alloc(&(_type), ucontext)
+ __uobj_alloc(_type, ucontext)
#endif
diff --git a/include/rdma/uverbs_types.h b/include/rdma/uverbs_types.h
index 351ea18..9760b6d 100644
--- a/include/rdma/uverbs_types.h
+++ b/include/rdma/uverbs_types.h
@@ -151,22 +151,30 @@ struct uverbs_obj_fd_type {
#define UVERBS_BUILD_BUG_ON(cond) (sizeof(char[1 - 2 * !!(cond)]) - \
sizeof(char))
-#define UVERBS_TYPE_ALLOC_FD(_size, _order) \
- { \
- .destroy_order = _order, \
- .type_class = &uverbs_fd_class, \
- .obj_size = (_size) + \
- UVERBS_BUILD_BUG_ON((_size) < \
- sizeof(struct ib_uobject_file)),\
- }
-#define UVERBS_TYPE_ALLOC_IDR_SZ(_size, _order) \
- { \
+#define UVERBS_TYPE_ALLOC_FD(_order, _obj_size, _context_closed, _fops, _name, _flags)\
+ ((&((const struct uverbs_obj_fd_type) \
+ {.type = { \
+ .destroy_order = _order, \
+ .type_class = &uverbs_fd_class, \
+ .obj_size = (_obj_size) + \
+ UVERBS_BUILD_BUG_ON((_obj_size) < sizeof(struct ib_uobject_file)), \
+ }, \
+ .context_closed = _context_closed, \
+ .fops = _fops, \
+ .name = _name, \
+ .flags = _flags}))->type)
+#define UVERBS_TYPE_ALLOC_IDR_SZ(_size, _order, _destroy_object) \
+ ((&((const struct uverbs_obj_idr_type) \
+ {.type = { \
.destroy_order = _order, \
.type_class = &uverbs_idr_class, \
.obj_size = (_size) + \
- UVERBS_BUILD_BUG_ON((_size) < \
- sizeof(struct ib_uobject)), \
- }
-#define UVERBS_TYPE_ALLOC_IDR(_order) \
- UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uobject), _order)
+ UVERBS_BUILD_BUG_ON((_size) < \
+ sizeof(struct ib_uobject)) \
+ }, \
+ .destroy_object = _destroy_object,}))->type)
+#define UVERBS_TYPE_ALLOC_IDR(_order, _destroy_object) \
+ UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uobject), _order, \
+ _destroy_object)
+
#endif
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 05/13] IB/core: Add DEVICE type and root types structure
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (3 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 04/13] IB/core: Declare a type instead of declaring only type attributes Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 06/13] IB/core: Initialize uverbs types specification Matan Barak
` (8 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
This adds the DEVICE type. This type supports creating the context that
all objects are created from. Moreover, it supports executing actions
which are related to the device itself, such as QUERY_DEVICE.
The only related object to this type is a singleton (per file
instance) context.
All standard types are put in the root structure. This root will later
on be used in drivers as their parsing tree root. Later on, when new
features are added, these drivers could define their own root.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/core/uverbs_std_types.c | 19 +++++++++++++++++++
include/rdma/uverbs_ioctl.h | 11 +++++++++++
include/rdma/uverbs_std_types.h | 19 +++++++++++++++++++
3 files changed, 49 insertions(+)
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index a204975..4c97be8 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -256,3 +256,22 @@ static int uverbs_hot_unplug_completion_event_file(struct ib_uobject_file *uobj_
/* 2 is used in order to free the PD after MRs */
&UVERBS_TYPE_ALLOC_IDR(2, uverbs_free_pd));
+DECLARE_UVERBS_TYPE(uverbs_type_device, NULL);
+
+DECLARE_UVERBS_TYPES(uverbs_common_types,
+ ADD_UVERBS_TYPE(UVERBS_TYPE_DEVICE, uverbs_type_device),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_PD, uverbs_type_pd),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_MR, uverbs_type_mr),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_COMP_CHANNEL, uverbs_type_comp_channel),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_CQ, uverbs_type_cq),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_QP, uverbs_type_qp),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_AH, uverbs_type_ah),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_MW, uverbs_type_mw),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_SRQ, uverbs_type_srq),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_FLOW, uverbs_type_flow),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_WQ, uverbs_type_wq),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_RWQ_IND_TBL,
+ uverbs_type_rwq_ind_table),
+ ADD_UVERBS_TYPE(UVERBS_TYPE_XRCD, uverbs_type_xrcd),
+);
+EXPORT_SYMBOL(uverbs_common_types);
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index ee89cce..ca636a2 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -135,6 +135,17 @@ struct uverbs_spec_root {
const struct uverbs_type name = { \
.type_attrs = _type_attrs, \
}
+#define _UVERBS_TYPE_SZ(...) \
+ (sizeof((const struct uverbs_type *[]){__VA_ARGS__}) / \
+ sizeof(const struct uverbs_type *))
+#define ADD_UVERBS_TYPE(type_idx, type_ptr) \
+ [type_idx] = ((const struct uverbs_type * const)&(type_ptr))
+#define UVERBS_TYPES(...) ((const struct uverbs_type_group) \
+ {.num_types = _UVERBS_TYPE_SZ(__VA_ARGS__), \
+ .types = (const struct uverbs_type *[]){__VA_ARGS__} })
+#define DECLARE_UVERBS_TYPES(name, ...) \
+ const struct uverbs_type_group name = UVERBS_TYPES(__VA_ARGS__)
+
/* =================================================
* Parsing infrastructure
* =================================================
diff --git a/include/rdma/uverbs_std_types.h b/include/rdma/uverbs_std_types.h
index 0810faf..3f582ad 100644
--- a/include/rdma/uverbs_std_types.h
+++ b/include/rdma/uverbs_std_types.h
@@ -35,6 +35,23 @@
#include <rdma/uverbs_types.h>
+enum uverbs_common_types {
+ UVERBS_TYPE_DEVICE, /* No instances of DEVICE are allowed */
+ UVERBS_TYPE_PD,
+ UVERBS_TYPE_COMP_CHANNEL,
+ UVERBS_TYPE_CQ,
+ UVERBS_TYPE_QP,
+ UVERBS_TYPE_SRQ,
+ UVERBS_TYPE_AH,
+ UVERBS_TYPE_MR,
+ UVERBS_TYPE_MW,
+ UVERBS_TYPE_FLOW,
+ UVERBS_TYPE_XRCD,
+ UVERBS_TYPE_RWQ_IND_TBL,
+ UVERBS_TYPE_WQ,
+ UVERBS_TYPE_LAST,
+};
+
extern const struct uverbs_type uverbs_type_comp_channel;
extern const struct uverbs_type uverbs_type_cq;
extern const struct uverbs_type uverbs_type_qp;
@@ -47,6 +64,8 @@
extern const struct uverbs_type uverbs_type_mw;
extern const struct uverbs_type uverbs_type_pd;
extern const struct uverbs_type uverbs_type_xrcd;
+extern const struct uverbs_type uverbs_type_device;
+extern const struct uverbs_type_group uverbs_common_types;
static inline struct ib_uobject *__uobj_get(const struct uverbs_obj_type *type,
bool write,
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 06/13] IB/core: Initialize uverbs types specification
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (4 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 05/13] IB/core: Add DEVICE type and root types structure Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 07/13] IB/core: Add macros for declaring actions and attributes Matan Barak
` (7 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
In order to accelerate the validation and parsing process, we
calculate the number of attributes of all groups in an action
and the mandatory attributes bitmask in advance.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/core/uverbs.h | 3 ++
drivers/infiniband/core/uverbs_ioctl.c | 58 ++++++++++++++++++++++++++++++++++
drivers/infiniband/core/uverbs_main.c | 3 ++
3 files changed, 64 insertions(+)
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 64d494a..6bb1302 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -191,7 +191,10 @@ struct ib_ucq_object {
u32 async_events_reported;
};
+struct uverbs_type_group;
+
extern const struct file_operations uverbs_event_fops;
+void uverbs_initialize_type_group(const struct uverbs_type_group *type_group);
void ib_uverbs_init_event_queue(struct ib_uverbs_event_queue *ev_queue);
struct file *ib_uverbs_alloc_async_event_file(struct ib_uverbs_file *uverbs_file,
struct ib_device *ib_dev);
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index a375a7b..4f7491e 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -358,3 +358,61 @@ long ib_uverbs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
return err;
}
+
+static void uverbs_initialize_action(struct uverbs_action *action)
+{
+ size_t attr_group_idx;
+
+ for (attr_group_idx = 0; attr_group_idx < action->num_groups;
+ attr_group_idx++) {
+ struct uverbs_attr_spec_group *attr_group =
+ action->attr_groups[attr_group_idx];
+ size_t attr_idx;
+
+ if (!attr_group)
+ continue;
+ action->num_child_attrs += attr_group->num_attrs;
+ for (attr_idx = 0; attr_idx < attr_group->num_attrs;
+ attr_idx++) {
+ struct uverbs_attr_spec *attr =
+ &attr_group->attrs[attr_idx];
+
+ if (attr->flags & UVERBS_ATTR_SPEC_F_MANDATORY)
+ set_bit(attr_idx,
+ attr_group->mandatory_attrs_bitmask);
+ }
+ }
+}
+
+void uverbs_initialize_type_group(const struct uverbs_type_group *type_group)
+{
+ size_t type_idx;
+
+ for (type_idx = 0; type_idx < type_group->num_types; type_idx++) {
+ const struct uverbs_type *type = type_group->types[type_idx];
+ size_t action_group_idx;
+
+ if (!type)
+ continue;
+ for (action_group_idx = 0;
+ action_group_idx < type->num_groups;
+ action_group_idx++) {
+ const struct uverbs_action_group *action_group =
+ type->action_groups[action_group_idx];
+ size_t action_idx;
+
+ if (!action_group)
+ continue;
+ for (action_idx = 0;
+ action_idx < action_group->num_actions;
+ action_idx++) {
+ struct uverbs_action *action =
+ action_group->actions[action_idx];
+
+ if (!action)
+ continue;
+ uverbs_initialize_action(action);
+ }
+ }
+ }
+}
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 3d26096..1836a36 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -45,6 +45,7 @@
#include <linux/cdev.h>
#include <linux/anon_inodes.h>
#include <linux/slab.h>
+#include <rdma/uverbs_std_types.h>
#include <linux/uaccess.h>
@@ -1254,6 +1255,8 @@ static int __init ib_uverbs_init(void)
{
int ret;
+ uverbs_initialize_type_group(&uverbs_common_types);
+
ret = register_chrdev_region(IB_UVERBS_BASE_DEV, IB_UVERBS_MAX_DEVICES,
"infiniband_verbs");
if (ret) {
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 07/13] IB/core: Add macros for declaring actions and attributes
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (5 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 06/13] IB/core: Initialize uverbs types specification Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 08/13] IB/core: Explicitly destroy an object while keeping uobject Matan Barak
` (6 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
This patch adds macros for declaring action groups, actions,
attribute groups and attributes. These definitions are later
used by downstream patches to declare some of the common types.
In addition, we add some helper inline functions to copy_{from,to}
user-space buffers and check if an attribute is valid.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
include/rdma/uverbs_ioctl.h | 113 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 112 insertions(+), 1 deletion(-)
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index ca636a2..4a03659 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -34,6 +34,11 @@
#define _UVERBS_IOCTL_
#include <rdma/uverbs_types.h>
+#include <linux/uaccess.h>
+#include <rdma/rdma_user_ioctl.h>
+
+struct uverbs_object_type;
+struct uverbs_uobject_type;
/*
* =======================================
@@ -131,9 +136,75 @@ struct uverbs_spec_root {
size_t num_groups;
};
-#define DECLARE_UVERBS_TYPE(name, _type_attrs) \
+#define UA_FLAGS(_flags) .flags = _flags
+#define UVERBS_ATTR(_id, _len, _type, ...) \
+ [_id] = {.len = _len, .type = _type, ##__VA_ARGS__}
+#define UVERBS_ATTR_PTR_IN_SZ(_id, _len, ...) \
+ UVERBS_ATTR(_id, _len, UVERBS_ATTR_TYPE_PTR_IN, ##__VA_ARGS__)
+/* If sizeof(_type) <= sizeof(u64), this will be inlined rather than a pointer */
+#define UVERBS_ATTR_PTR_IN(_id, _type, ...) \
+ UVERBS_ATTR_PTR_IN_SZ(_id, sizeof(_type), ##__VA_ARGS__)
+#define UVERBS_ATTR_PTR_OUT_SZ(_id, _len, ...) \
+ UVERBS_ATTR(_id, _len, UVERBS_ATTR_TYPE_PTR_OUT, ##__VA_ARGS__)
+#define UVERBS_ATTR_PTR_OUT(_id, _type, ...) \
+ UVERBS_ATTR_PTR_OUT_SZ(_id, sizeof(_type), ##__VA_ARGS__)
+#define UVERBS_ATTR_IDR(_id, _idr_type, _access, ...) \
+ [_id] = {.type = UVERBS_ATTR_TYPE_IDR, \
+ .obj = {.obj_type = _idr_type, \
+ .access = _access \
+ }, ##__VA_ARGS__ }
+#define UVERBS_ATTR_FD(_id, _fd_type, _access, ...) \
+ [_id] = {.type = UVERBS_ATTR_TYPE_FD, \
+ .obj = {.obj_type = _fd_type, \
+ .access = (_access) + BUILD_BUG_ON_ZERO( \
+ (_access) != UVERBS_ACCESS_NEW && \
+ (_access) != UVERBS_ACCESS_READ) \
+ }, ##__VA_ARGS__ }
+#define _UVERBS_ATTR_SPEC_SZ(...) \
+ (sizeof((struct uverbs_attr_spec[]){__VA_ARGS__}) / \
+ sizeof(struct uverbs_attr_spec))
+#define UVERBS_ATTR_SPEC(...) \
+ ((struct uverbs_attr_spec_group) \
+ {.attrs = (struct uverbs_attr_spec[]){__VA_ARGS__}, \
+ .num_attrs = _UVERBS_ATTR_SPEC_SZ(__VA_ARGS__), \
+ .mandatory_attrs_bitmask = \
+ ((unsigned long[BITS_TO_LONGS(_UVERBS_ATTR_SPEC_SZ(__VA_ARGS__))]) {})})
+#define DECLARE_UVERBS_ATTR_SPEC(name, ...) \
+ struct uverbs_attr_spec_group name = \
+ UVERBS_ATTR_SPEC(__VA_ARGS__)
+#define _UVERBS_ATTR_ACTION_SPEC_SZ(...) \
+ (sizeof((struct uverbs_attr_spec_group *[]){__VA_ARGS__}) / \
+ sizeof(struct uverbs_attr_spec_group *))
+#define _UVERBS_ACTION(_handler, _flags, ...) \
+ ((struct uverbs_action) { \
+ .flags = _flags, \
+ .handler = _handler, \
+ .num_groups = _UVERBS_ATTR_ACTION_SPEC_SZ(__VA_ARGS__), \
+ .attr_groups = (struct uverbs_attr_spec_group *[]){__VA_ARGS__} })
+#define UVERBS_ACTION(_handler, ...) \
+ _UVERBS_ACTION(_handler, 0, __VA_ARGS__)
+#define UVERBS_CTX_ACTION(_handler, ...) \
+ _UVERBS_ACTION(_handler, UVERBS_ACTION_FLAG_CREATE_ROOT, __VA_ARGS__)
+#define _UVERBS_ACTIONS_SZ(...) \
+ (sizeof((struct uverbs_action *[]){__VA_ARGS__}) / \
+ sizeof(struct uverbs_action *))
+#define ADD_UVERBS_ACTION(action_idx, _handler, ...) \
+ [action_idx] = &UVERBS_ACTION(_handler, __VA_ARGS__)
+#define ADD_UVERBS_CTX_ACTION(action_idx, _handler, ...) \
+ [action_idx] = &UVERBS_CTX_ACTION(_handler, __VA_ARGS__)
+#define UVERBS_ACTIONS(...) \
+ ((const struct uverbs_action_group) \
+ {.num_actions = _UVERBS_ACTIONS_SZ(__VA_ARGS__), \
+ .actions = (struct uverbs_action *[]){__VA_ARGS__} })
+#define _UVERBS_ACTIONS_GROUP_SZ(...) \
+ (sizeof((const struct uverbs_action_group*[]){__VA_ARGS__}) / \
+ sizeof(const struct uverbs_action_group *))
+
+#define DECLARE_UVERBS_TYPE(name, _type_attrs, ...) \
const struct uverbs_type name = { \
.type_attrs = _type_attrs, \
+ .num_groups = _UVERBS_ACTIONS_GROUP_SZ(__VA_ARGS__), \
+ .action_groups = (const struct uverbs_action_group *[]){__VA_ARGS__} \
}
#define _UVERBS_TYPE_SZ(...) \
(sizeof((const struct uverbs_type *[]){__VA_ARGS__}) / \
@@ -146,6 +217,17 @@ struct uverbs_spec_root {
#define DECLARE_UVERBS_TYPES(name, ...) \
const struct uverbs_type_group name = UVERBS_TYPES(__VA_ARGS__)
+#define _UVERBS_TYPES_SZ(...) \
+ (sizeof((const struct uverbs_type_group *[]){__VA_ARGS__}) / \
+ sizeof(const struct uverbs_type_group *))
+
+#define UVERBS_TYPES_GROUP(...) \
+ ((const struct uverbs_spec_root){ \
+ .type_groups = (const struct uverbs_type_group *[]){__VA_ARGS__},\
+ .num_groups = _UVERBS_TYPES_SZ(__VA_ARGS__)})
+#define DECLARE_UVERBS_TYPES_GROUP(name, ...) \
+ const struct uverbs_spec_root name = UVERBS_TYPES_GROUP(__VA_ARGS__)
+
/* =================================================
* Parsing infrastructure
* =================================================
@@ -193,5 +275,34 @@ static inline bool uverbs_is_valid(const struct uverbs_attr_array *attr_array,
return test_bit(idx, attr_array->valid_bitmap);
}
+static inline int uverbs_copy_to(struct uverbs_attr_array *attr_array,
+ size_t idx, const void *from)
+{
+ if (!uverbs_is_valid(attr_array, idx))
+ return -ENOENT;
+
+ return copy_to_user(attr_array->attrs[idx].ptr_attr.ptr, from,
+ attr_array->attrs[idx].ptr_attr.len) ? -EFAULT : 0;
+}
+
+#define uverbs_copy_from(to, attr_array, idx) \
+ (uverbs_is_valid((attr_array), idx) ? \
+ (sizeof(*(to)) <= sizeof(((struct ib_uverbs_attr *)0)->data) ? \
+ (memcpy((to), &(attr_array)->attrs[idx].ptr_attr.ptr, \
+ (attr_array)->attrs[idx].ptr_attr.len), 0) : \
+ (copy_from_user((to), (attr_array)->attrs[idx].ptr_attr.ptr, \
+ (attr_array)->attrs[idx].ptr_attr.len) ? \
+ -EFAULT : 0)) : -ENOENT)
+#define uverbs_get_attr(to, attr_array, idx) \
+ (uverbs_is_valid((attr_array), idx) ? \
+ (sizeof(to) <= sizeof(((struct ib_uverbs_attr *)0)->data) ? \
+ (sizeof(to) == sizeof((&(to))[0]) ? \
+ ((to) = *(typeof(to) *)&(attr_array)->attrs[idx].ptr_attr.ptr, 0) :\
+ (memcpy(&(to), &(attr_array)->attrs[idx].ptr_attr.ptr, \
+ (attr_array)->attrs[idx].ptr_attr.len), 0)) : \
+ (copy_from_user(&(to), (attr_array)->attrs[idx].ptr_attr.ptr, \
+ (attr_array)->attrs[idx].ptr_attr.len) ? \
+ -EFAULT : 0)) : -ENOENT)
+
#endif
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 08/13] IB/core: Explicitly destroy an object while keeping uobject
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (6 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 07/13] IB/core: Add macros for declaring actions and attributes Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 09/13] IB/core: Export ioctl enum types to user-space Matan Barak
` (5 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
When some objects are destroyed, we need to extract their status at
destruction. After object's destruction, this status
(e.g. events_reported) relies in the uobject. In order to have the
latest and correct status, the underlying object should be destroyed,
but we should keep the uobject alive and read this information off the
uobject. We introduce a rdma_explicit_destroy function. This function
destroys the class type object (for example, the IDR class type which
destroys the underlying object as well) and then convert the uobject
to be of a null class type. This uobject will then be destroyed as any
other uobject once uverbs_finalize_object[s] is called.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/core/rdma_core.c | 35 +++++++++++++++++++++++++++++++++++
include/rdma/uverbs_types.h | 1 +
2 files changed, 36 insertions(+)
diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index d16a519..de30016 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -450,6 +450,41 @@ int __must_check rdma_remove_commit_uobject(struct ib_uobject *uobj)
return ret;
}
+static int null_obj_type_class_remove_commit(struct ib_uobject *uobj,
+ enum rdma_remove_reason why)
+{
+ return 0;
+}
+
+static const struct uverbs_obj_type null_obj_type = {
+ .type_class = &((const struct uverbs_obj_type_class){
+ .remove_commit = null_obj_type_class_remove_commit,
+ /* be cautious */
+ .needs_kfree_rcu = true}),
+};
+
+int rdma_explicit_destroy(struct ib_uobject *uobject)
+{
+ int ret;
+ struct ib_ucontext *ucontext = uobject->context;
+
+ /* Cleanup is running. Calling this should have been impossible */
+ if (!down_read_trylock(&ucontext->cleanup_rwsem)) {
+ WARN(true, "ib_uverbs: Cleanup is running while removing an uobject\n");
+ return 0;
+ }
+ lockdep_check(uobject, true);
+ ret = uobject->type->type_class->remove_commit(uobject,
+ RDMA_REMOVE_DESTROY);
+ if (ret)
+ return ret;
+
+ uobject->type = &null_obj_type;
+
+ up_read(&ucontext->cleanup_rwsem);
+ return 0;
+}
+
static void alloc_commit_idr_uobject(struct ib_uobject *uobj)
{
uverbs_uobject_add(uobj);
diff --git a/include/rdma/uverbs_types.h b/include/rdma/uverbs_types.h
index 9760b6d..cc04ec6 100644
--- a/include/rdma/uverbs_types.h
+++ b/include/rdma/uverbs_types.h
@@ -129,6 +129,7 @@ struct ib_uobject *rdma_alloc_begin_uobject(const struct uverbs_obj_type *type,
void rdma_alloc_abort_uobject(struct ib_uobject *uobj);
int __must_check rdma_remove_commit_uobject(struct ib_uobject *uobj);
int rdma_alloc_commit_uobject(struct ib_uobject *uobj);
+int rdma_explicit_destroy(struct ib_uobject *uobject);
struct uverbs_obj_fd_type {
/*
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 09/13] IB/core: Export ioctl enum types to user-space
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (7 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 08/13] IB/core: Explicitly destroy an object while keeping uobject Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 10/13] IB/core: Add legacy driver's user-data Matan Barak
` (4 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
Add a new ib_user_ioctl_verbs.h which exports all required ABI
enums and structs to the user-space.
Export the common types to user-space through this file.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
include/rdma/uverbs_std_types.h | 18 +----------
include/uapi/rdma/ib_user_ioctl_verbs.h | 54 +++++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+), 17 deletions(-)
create mode 100644 include/uapi/rdma/ib_user_ioctl_verbs.h
diff --git a/include/rdma/uverbs_std_types.h b/include/rdma/uverbs_std_types.h
index 3f582ad..c7ec8be 100644
--- a/include/rdma/uverbs_std_types.h
+++ b/include/rdma/uverbs_std_types.h
@@ -34,23 +34,7 @@
#define _UVERBS_STD_TYPES__
#include <rdma/uverbs_types.h>
-
-enum uverbs_common_types {
- UVERBS_TYPE_DEVICE, /* No instances of DEVICE are allowed */
- UVERBS_TYPE_PD,
- UVERBS_TYPE_COMP_CHANNEL,
- UVERBS_TYPE_CQ,
- UVERBS_TYPE_QP,
- UVERBS_TYPE_SRQ,
- UVERBS_TYPE_AH,
- UVERBS_TYPE_MR,
- UVERBS_TYPE_MW,
- UVERBS_TYPE_FLOW,
- UVERBS_TYPE_XRCD,
- UVERBS_TYPE_RWQ_IND_TBL,
- UVERBS_TYPE_WQ,
- UVERBS_TYPE_LAST,
-};
+#include <rdma/ib_user_ioctl_verbs.h>
extern const struct uverbs_type uverbs_type_comp_channel;
extern const struct uverbs_type uverbs_type_cq;
diff --git a/include/uapi/rdma/ib_user_ioctl_verbs.h b/include/uapi/rdma/ib_user_ioctl_verbs.h
new file mode 100644
index 0000000..9db94d0
--- /dev/null
+++ b/include/uapi/rdma/ib_user_ioctl_verbs.h
@@ -0,0 +1,54 @@
+/*
+ * Copyright (c) 2017, Mellanox Technologies inc. All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses. You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef IB_USER_IOCTL_VERBS_H
+#define IB_USER_IOCTL_VERBS_H
+
+enum uverbs_common_types {
+ UVERBS_TYPE_DEVICE, /* No instances of DEVICE are allowed */
+ UVERBS_TYPE_PD,
+ UVERBS_TYPE_COMP_CHANNEL,
+ UVERBS_TYPE_CQ,
+ UVERBS_TYPE_QP,
+ UVERBS_TYPE_SRQ,
+ UVERBS_TYPE_AH,
+ UVERBS_TYPE_MR,
+ UVERBS_TYPE_MW,
+ UVERBS_TYPE_FLOW,
+ UVERBS_TYPE_XRCD,
+ UVERBS_TYPE_RWQ_IND_TBL,
+ UVERBS_TYPE_WQ,
+ UVERBS_TYPE_LAST,
+};
+
+#endif
+
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 10/13] IB/core: Add legacy driver's user-data
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (8 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 09/13] IB/core: Export ioctl enum types to user-space Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 11/13] IB/core: Add completion queue (cq) object actions Matan Barak
` (3 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
In this phase, we don't want to change all the drivers to use
flexible driver's specific attributes. Therefore, we add two default
attributes: UHW_IN and UHW_OUT. These attributes are optional in some
commands and they encode the driver specific command data. We add
a function that extract this data and creates the legacy udata over
it.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/core/rdma_core.c | 1 +
drivers/infiniband/core/uverbs_std_types.c | 39 ++++++++++++++++++++++++++++++
include/rdma/uverbs_ioctl.h | 3 ---
include/uapi/rdma/ib_user_ioctl_verbs.h | 10 ++++++++
4 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index de30016..f684f2f 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -36,6 +36,7 @@
#include <rdma/uverbs_types.h>
#include <linux/rcupdate.h>
#include <rdma/uverbs_ioctl.h>
+#include <rdma/ib_user_ioctl_verbs.h>
#include "uverbs.h"
#include "core_priv.h"
#include "rdma_core.h"
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index 4c97be8..caf82a0 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -209,6 +209,45 @@ static int uverbs_hot_unplug_completion_event_file(struct ib_uobject_file *uobj_
return 0;
};
+/*
+ * This spec is used in order to pass information to the hardware driver in a
+ * legacy way. Every verb that could get driver specific data should get this
+ * spec.
+ */
+static DECLARE_UVERBS_ATTR_SPEC(
+ uverbs_uhw_compat_spec,
+ UVERBS_ATTR_PTR_IN_SZ(UVERBS_UHW_IN, 0, UA_FLAGS(UVERBS_ATTR_SPEC_F_MIN_SZ)),
+ UVERBS_ATTR_PTR_OUT_SZ(UVERBS_UHW_OUT, 0, UA_FLAGS(UVERBS_ATTR_SPEC_F_MIN_SZ)));
+
+static void create_udata(struct uverbs_attr_array *ctx, size_t num,
+ struct ib_udata *udata)
+{
+ /*
+ * This is for ease of conversion. The purpose is to convert all drivers
+ * to use uverbs_attr_array instead of ib_udata.
+ * Assume attr == 0 is input and attr == 1 is output.
+ */
+ void __user *inbuf;
+ size_t inbuf_len = 0;
+ void __user *outbuf;
+ size_t outbuf_len = 0;
+
+ if (num > UVERBS_UDATA_DRIVER_DATA_GROUP) {
+ struct uverbs_attr_array *driver = &ctx[UVERBS_UDATA_DRIVER_DATA_GROUP];
+
+ if (uverbs_is_valid(driver, UVERBS_UHW_IN)) {
+ inbuf = driver->attrs[UVERBS_UHW_IN].ptr_attr.ptr;
+ inbuf_len = driver->attrs[UVERBS_UHW_IN].ptr_attr.len;
+ }
+
+ if (driver->num_attrs >= UVERBS_UHW_OUT &&
+ uverbs_is_valid(driver, UVERBS_UHW_OUT)) {
+ outbuf = driver->attrs[UVERBS_UHW_OUT].ptr_attr.ptr;
+ outbuf_len = driver->attrs[UVERBS_UHW_OUT].ptr_attr.len;
+ }
+ }
+ INIT_UDATA_BUF_OR_NULL(udata, inbuf, outbuf, inbuf_len, outbuf_len);
+}
DECLARE_UVERBS_TYPE(uverbs_type_comp_channel,
&UVERBS_TYPE_ALLOC_FD(0,
sizeof(struct ib_uverbs_completion_event_file),
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index 4a03659..c7b3439 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -46,9 +46,6 @@
* =======================================
*/
-#define UVERBS_ID_GROUP_MASK 0xF000
-#define UVERBS_ID_GROUP_SHIFT 12
-
enum uverbs_attr_type {
UVERBS_ATTR_TYPE_NA,
UVERBS_ATTR_TYPE_PTR_IN,
diff --git a/include/uapi/rdma/ib_user_ioctl_verbs.h b/include/uapi/rdma/ib_user_ioctl_verbs.h
index 9db94d0..4165738 100644
--- a/include/uapi/rdma/ib_user_ioctl_verbs.h
+++ b/include/uapi/rdma/ib_user_ioctl_verbs.h
@@ -33,6 +33,11 @@
#ifndef IB_USER_IOCTL_VERBS_H
#define IB_USER_IOCTL_VERBS_H
+#define UVERBS_ID_GROUP_MASK 0xF000
+#define UVERBS_ID_GROUP_SHIFT 12
+#define UVERBS_UDATA_DRIVER_DATA_GROUP 1
+#define UVERBS_UDATA_DRIVER_DATA_FLAG (1UL << UVERBS_ID_GROUP_SHIFT)
+
enum uverbs_common_types {
UVERBS_TYPE_DEVICE, /* No instances of DEVICE are allowed */
UVERBS_TYPE_PD,
@@ -50,5 +55,10 @@ enum uverbs_common_types {
UVERBS_TYPE_LAST,
};
+enum {
+ UVERBS_UHW_IN,
+ UVERBS_UHW_OUT,
+};
+
#endif
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 11/13] IB/core: Add completion queue (cq) object actions
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (9 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 10/13] IB/core: Add legacy driver's user-data Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 12/13] IB/core: Assign root to all drivers Matan Barak
` (2 subsequent siblings)
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
Adding CQ ioctl actions:
1. create_cq
2. destroy_cq
This requires adding the following:
1. A specification describing the action
a. Handler
b. Common attributes specification group
c. (Optional) Driver specific specification group
2. A specification describing the attribute group.
Each attribute is one of the following:
a. PTR_IN - input data
Note: This could be encoded inlined for data < 64bit
b. PTR_OUT - response data
c. IDR - idr based object
d. FD - fd based object
Blobs attributes (clauses a and b) contain their type, while objects
specifications (clauses c and d) contains the expected object type
(for example, the given id should be UVERBS_TYPE_PD) and the
required access (READ, WRITE, NEW or DESTROY). If a NEW is required,
the new object's id will be assigned to this attribute.
All attributes could get UA_FLAGS attribute. Currently we support
stating that an attribute is mandatory or that the specification size
corresponds to a lower bound (and that this attribute could be
extended).
3. Handler
A handler gets an array of uverbs_attr_array (meaning, array of
arrays). Each element in uverbs_attr_array corresponds to one
attribute group indexed by the group number (currently,
uverbs_attr_array[0] -> common group and
uverbs_attr_array[1] -> driver-specific group).
Each of these attribute groups correspond to the specification
group defined in the action (clauses 1.b and 1.c respectively).
The indices of these arrays corresponds to the attribute ids
declared in the specifications (clause 2).
The handler is quite simple. It assumes the infrastructure fetched
all objects and locked, created or destroyed them as required by
the specification. Pointer (or blob) attributes were validated to
match their required sizes. After the handler finished, the
infrastructure commits or rollbacks the objects.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/core/uverbs_std_types.c | 145 ++++++++++++++++++++++++++++-
include/rdma/uverbs_std_types.h | 2 +
include/uapi/rdma/ib_user_ioctl_verbs.h | 20 ++++
3 files changed, 166 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index caf82a0..cef13b43 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -248,6 +248,141 @@ static void create_udata(struct uverbs_attr_array *ctx, size_t num,
}
INIT_UDATA_BUF_OR_NULL(udata, inbuf, outbuf, inbuf_len, outbuf_len);
}
+
+static DECLARE_UVERBS_ATTR_SPEC(
+ uverbs_create_cq_spec,
+ UVERBS_ATTR_IDR(CREATE_CQ_HANDLE, UVERBS_TYPE_CQ, UVERBS_ACCESS_NEW,
+ UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)),
+ UVERBS_ATTR_PTR_IN(CREATE_CQ_CQE, u32,
+ UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)),
+ UVERBS_ATTR_PTR_IN(CREATE_CQ_USER_HANDLE, u64),
+ UVERBS_ATTR_FD(CREATE_CQ_COMP_CHANNEL, UVERBS_TYPE_COMP_CHANNEL, UVERBS_ACCESS_READ),
+ /*
+ * Currently, COMP_VECTOR is mandatory, but that could be lifted in the
+ * future.
+ */
+ UVERBS_ATTR_PTR_IN(CREATE_CQ_COMP_VECTOR, u32,
+ UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)),
+ UVERBS_ATTR_PTR_IN(CREATE_CQ_FLAGS, u32),
+ UVERBS_ATTR_PTR_OUT(CREATE_CQ_RESP_CQE, u32,
+ UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)));
+
+static int uverbs_create_cq_handler(struct ib_device *ib_dev,
+ struct ib_uverbs_file *file,
+ struct uverbs_attr_array *ctx, size_t num)
+{
+ struct uverbs_attr_array *common = &ctx[0];
+ struct ib_ucontext *ucontext = file->ucontext;
+ struct ib_ucq_object *obj;
+ struct ib_udata uhw;
+ int ret;
+ u64 user_handle = 0;
+ struct ib_cq_init_attr attr = {};
+ struct ib_cq *cq;
+ struct ib_uverbs_completion_event_file *ev_file = NULL;
+
+ if (!(ib_dev->uverbs_cmd_mask & 1ULL << IB_USER_VERBS_CMD_CREATE_CQ))
+ return -EOPNOTSUPP;
+
+ ret = uverbs_copy_from(&attr.comp_vector, common, CREATE_CQ_COMP_VECTOR);
+ if (!ret)
+ ret = uverbs_copy_from(&attr.cqe, common, CREATE_CQ_CQE);
+ if (ret)
+ return ret;
+
+ /* Optional params, if they don't exist, we get -ENOENT and skip them */
+ if (uverbs_copy_from(&attr.flags, common, CREATE_CQ_FLAGS) == -EFAULT ||
+ uverbs_copy_from(&user_handle, common, CREATE_CQ_USER_HANDLE) == -EFAULT)
+ return -EFAULT;
+
+ if (uverbs_is_valid(common, CREATE_CQ_COMP_CHANNEL)) {
+ struct ib_uobject *ev_file_uobj =
+ common->attrs[CREATE_CQ_COMP_CHANNEL].obj_attr.uobject;
+
+ ev_file = container_of(ev_file_uobj,
+ struct ib_uverbs_completion_event_file,
+ uobj_file.uobj);
+ uverbs_uobject_get(ev_file_uobj);
+ }
+
+ if (attr.comp_vector >= ucontext->ufile->device->num_comp_vectors) {
+ ret = -EINVAL;
+ goto err_event_file;
+ }
+
+ obj = container_of(common->attrs[CREATE_CQ_HANDLE].obj_attr.uobject,
+ typeof(*obj), uobject);
+ obj->uverbs_file = ucontext->ufile;
+ obj->comp_events_reported = 0;
+ obj->async_events_reported = 0;
+ INIT_LIST_HEAD(&obj->comp_list);
+ INIT_LIST_HEAD(&obj->async_list);
+
+ /* Temporary, only until drivers get the new uverbs_attr_array */
+ create_udata(ctx, num, &uhw);
+
+ cq = ib_dev->create_cq(ib_dev, &attr, ucontext, &uhw);
+ if (IS_ERR(cq)) {
+ ret = PTR_ERR(cq);
+ goto err_event_file;
+ }
+
+ cq->device = ib_dev;
+ cq->uobject = &obj->uobject;
+ cq->comp_handler = ib_uverbs_comp_handler;
+ cq->event_handler = ib_uverbs_cq_event_handler;
+ cq->cq_context = &ev_file->ev_queue;
+ obj->uobject.object = cq;
+ obj->uobject.user_handle = user_handle;
+ atomic_set(&cq->usecnt, 0);
+
+ ret = uverbs_copy_to(common, CREATE_CQ_RESP_CQE, &cq->cqe);
+ if (ret)
+ goto err_cq;
+
+ return 0;
+err_cq:
+ ib_destroy_cq(cq);
+
+err_event_file:
+ if (ev_file)
+ uverbs_uobject_put(&ev_file->uobj_file.uobj);
+ return ret;
+};
+
+static DECLARE_UVERBS_ATTR_SPEC(
+ uverbs_destroy_cq_spec,
+ UVERBS_ATTR_IDR(DESTROY_CQ_HANDLE, UVERBS_TYPE_CQ,
+ UVERBS_ACCESS_DESTROY,
+ UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)),
+ UVERBS_ATTR_PTR_OUT(DESTROY_CQ_RESP, struct ib_uverbs_destroy_cq_resp,
+ UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY)));
+
+static int uverbs_destroy_cq_handler(struct ib_device *ib_dev,
+ struct ib_uverbs_file *file,
+ struct uverbs_attr_array *ctx, size_t num)
+{
+ struct uverbs_attr_array *common = &ctx[0];
+ struct ib_uverbs_destroy_cq_resp resp;
+ struct ib_uobject *uobj =
+ common->attrs[DESTROY_CQ_HANDLE].obj_attr.uobject;
+ struct ib_ucq_object *obj = container_of(uobj, struct ib_ucq_object,
+ uobject);
+ int ret;
+
+ if (!(ib_dev->uverbs_cmd_mask & 1ULL << IB_USER_VERBS_CMD_DESTROY_CQ))
+ return -EOPNOTSUPP;
+
+ ret = rdma_explicit_destroy(uobj);
+ if (ret)
+ return ret;
+
+ resp.comp_events_reported = obj->comp_events_reported;
+ resp.async_events_reported = obj->async_events_reported;
+
+ return uverbs_copy_to(common, DESTROY_CQ_RESP, &resp);
+}
+
DECLARE_UVERBS_TYPE(uverbs_type_comp_channel,
&UVERBS_TYPE_ALLOC_FD(0,
sizeof(struct ib_uverbs_completion_event_file),
@@ -257,7 +392,15 @@ static void create_udata(struct uverbs_attr_array *ctx, size_t num,
DECLARE_UVERBS_TYPE(uverbs_type_cq,
&UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0,
- uverbs_free_cq));
+ uverbs_free_cq),
+ &UVERBS_ACTIONS(
+ ADD_UVERBS_ACTION(UVERBS_CQ_CREATE,
+ uverbs_create_cq_handler,
+ &uverbs_create_cq_spec,
+ &uverbs_uhw_compat_spec),
+ ADD_UVERBS_ACTION(UVERBS_CQ_DESTROY,
+ uverbs_destroy_cq_handler,
+ &uverbs_destroy_cq_spec)));
DECLARE_UVERBS_TYPE(uverbs_type_qp,
&UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uqp_object), 0,
diff --git a/include/rdma/uverbs_std_types.h b/include/rdma/uverbs_std_types.h
index c7ec8be..1f22bb2 100644
--- a/include/rdma/uverbs_std_types.h
+++ b/include/rdma/uverbs_std_types.h
@@ -36,6 +36,8 @@
#include <rdma/uverbs_types.h>
#include <rdma/ib_user_ioctl_verbs.h>
+extern const struct uverbs_action_group uverbs_actions_cq;
+
extern const struct uverbs_type uverbs_type_comp_channel;
extern const struct uverbs_type uverbs_type_cq;
extern const struct uverbs_type uverbs_type_qp;
diff --git a/include/uapi/rdma/ib_user_ioctl_verbs.h b/include/uapi/rdma/ib_user_ioctl_verbs.h
index 4165738..1f00020 100644
--- a/include/uapi/rdma/ib_user_ioctl_verbs.h
+++ b/include/uapi/rdma/ib_user_ioctl_verbs.h
@@ -60,5 +60,25 @@ enum {
UVERBS_UHW_OUT,
};
+enum uverbs_create_cq_cmd_attr_ids {
+ CREATE_CQ_HANDLE,
+ CREATE_CQ_CQE,
+ CREATE_CQ_USER_HANDLE,
+ CREATE_CQ_COMP_CHANNEL,
+ CREATE_CQ_COMP_VECTOR,
+ CREATE_CQ_FLAGS,
+ CREATE_CQ_RESP_CQE,
+};
+
+enum uverbs_destroy_cq_cmd_attr_ids {
+ DESTROY_CQ_HANDLE,
+ DESTROY_CQ_RESP,
+};
+
+enum uverbs_actions_cq_ops {
+ UVERBS_CQ_CREATE,
+ UVERBS_CQ_DESTROY,
+};
+
#endif
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 12/13] IB/core: Assign root to all drivers
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (10 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 11/13] IB/core: Add completion queue (cq) object actions Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 13/13] IB/core: Expose ioctl interface through experimental Kconfig Matan Barak
2017-06-07 15:53 ` [PATCH for-next 00/13] IB/core: SG IOCTL based RDMA ABI Jason Gunthorpe
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
In order to use the parsing tree, we need to assign the root
to all drivers. Currently, we just assign the default parsing
tree.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/hw/bnxt_re/main.c | 5 +++++
drivers/infiniband/hw/cxgb3/iwch_provider.c | 5 +++++
drivers/infiniband/hw/cxgb4/provider.c | 5 +++++
drivers/infiniband/hw/hns/hns_roce_main.c | 5 +++++
drivers/infiniband/hw/i40iw/i40iw_verbs.c | 5 +++++
drivers/infiniband/hw/mlx4/main.c | 5 +++++
drivers/infiniband/hw/mlx5/main.c | 5 +++++
drivers/infiniband/hw/mthca/mthca_provider.c | 5 +++++
drivers/infiniband/hw/nes/nes_verbs.c | 5 +++++
drivers/infiniband/hw/ocrdma/ocrdma_main.c | 5 +++++
drivers/infiniband/hw/qedr/main.c | 5 +++++
drivers/infiniband/hw/usnic/usnic_ib_main.c | 5 +++++
drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 5 +++++
drivers/infiniband/sw/rdmavt/vt.c | 5 +++++
drivers/infiniband/sw/rxe/rxe_verbs.c | 5 +++++
15 files changed, 75 insertions(+)
diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index 5d35540..23acca2 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -53,6 +53,8 @@
#include <rdma/ib_user_verbs.h>
#include <rdma/ib_umem.h>
#include <rdma/ib_addr.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include "bnxt_ulp.h"
#include "roce_hsi.h"
@@ -421,6 +423,8 @@ static void bnxt_re_unregister_ib(struct bnxt_re_dev *rdev)
ib_unregister_device(&rdev->ibdev);
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
static int bnxt_re_register_ib(struct bnxt_re_dev *rdev)
{
struct ib_device *ibdev = &rdev->ibdev;
@@ -517,6 +521,7 @@ static int bnxt_re_register_ib(struct bnxt_re_dev *rdev)
ibdev->dealloc_ucontext = bnxt_re_dealloc_ucontext;
ibdev->mmap = bnxt_re_mmap;
+ ibdev->specs_root = &root;
return ib_register_device(ibdev, NULL);
}
diff --git a/drivers/infiniband/hw/cxgb3/iwch_provider.c b/drivers/infiniband/hw/cxgb3/iwch_provider.c
index 29d3074..a5e4c6b 100644
--- a/drivers/infiniband/hw/cxgb3/iwch_provider.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_provider.c
@@ -53,6 +53,8 @@
#include <rdma/ib_smi.h>
#include <rdma/ib_umem.h>
#include <rdma/ib_user_verbs.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include "cxio_hal.h"
#include "iwch.h"
@@ -1349,6 +1351,8 @@ static void get_dev_fw_ver_str(struct ib_device *ibdev, char *str,
snprintf(str, str_len, "%s", info.fw_version);
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
int iwch_register_device(struct iwch_dev *dev)
{
int ret;
@@ -1442,6 +1446,7 @@ int iwch_register_device(struct iwch_dev *dev)
memcpy(dev->ibdev.iwcm->ifname, dev->rdev.t3cdev_p->lldev->name,
sizeof(dev->ibdev.iwcm->ifname));
+ dev->ibdev.specs_root = &root;
ret = ib_register_device(&dev->ibdev, NULL);
if (ret)
goto bail1;
diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c
index 0771e9a..4fcc567 100644
--- a/drivers/infiniband/hw/cxgb4/provider.c
+++ b/drivers/infiniband/hw/cxgb4/provider.c
@@ -51,6 +51,8 @@
#include <rdma/ib_smi.h>
#include <rdma/ib_umem.h>
#include <rdma/ib_user_verbs.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include "iw_cxgb4.h"
@@ -531,6 +533,8 @@ static void get_dev_fw_str(struct ib_device *dev, char *str,
FW_HDR_FW_VER_BUILD_G(c4iw_dev->rdev.lldi.fw_vers));
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
int c4iw_register_device(struct c4iw_dev *dev)
{
int ret;
@@ -624,6 +628,7 @@ int c4iw_register_device(struct c4iw_dev *dev)
memcpy(dev->ibdev.iwcm->ifname, dev->rdev.lldi.ports[0]->name,
sizeof(dev->ibdev.iwcm->ifname));
+ dev->ibdev.specs_root = &root;
ret = ib_register_device(&dev->ibdev, NULL);
if (ret)
goto bail1;
diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c b/drivers/infiniband/hw/hns/hns_roce_main.c
index c3b41f9..302e5f9 100644
--- a/drivers/infiniband/hw/hns/hns_roce_main.c
+++ b/drivers/infiniband/hw/hns/hns_roce_main.c
@@ -37,6 +37,8 @@
#include <rdma/ib_smi.h>
#include <rdma/ib_user_verbs.h>
#include <rdma/ib_cache.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include "hns_roce_common.h"
#include "hns_roce_device.h"
#include <rdma/hns-abi.h>
@@ -424,6 +426,8 @@ static void hns_roce_unregister_device(struct hns_roce_dev *hr_dev)
ib_unregister_device(&hr_dev->ib_dev);
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
static int hns_roce_register_device(struct hns_roce_dev *hr_dev)
{
int ret;
@@ -507,6 +511,7 @@ static int hns_roce_register_device(struct hns_roce_dev *hr_dev)
/* OTHERS */
ib_dev->get_port_immutable = hns_roce_port_immutable;
+ dev->ib_dev.specs_root = &root;
ret = ib_register_device(ib_dev, NULL);
if (ret) {
dev_err(dev, "ib_register_device failed!\n");
diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.c b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
index 4dbe61e..f2e89d3 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_verbs.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
@@ -43,6 +43,8 @@
#include <rdma/ib_verbs.h>
#include <rdma/iw_cm.h>
#include <rdma/ib_user_verbs.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include <rdma/ib_umem.h>
#include "i40iw.h"
@@ -2859,6 +2861,8 @@ void i40iw_destroy_rdma_device(struct i40iw_ib_device *iwibdev)
ib_dealloc_device(&iwibdev->ibdev);
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
/**
* i40iw_register_rdma_device - register iwarp device to IB
* @iwdev: iwarp device
@@ -2873,6 +2877,7 @@ int i40iw_register_rdma_device(struct i40iw_device *iwdev)
return -ENOMEM;
iwibdev = iwdev->iwibdev;
+ iwibdev->ibdev.specs_root = &root;
ret = ib_register_device(&iwibdev->ibdev, NULL);
if (ret)
goto error;
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 521d0de..75bbd4b 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -48,6 +48,8 @@
#include <rdma/ib_smi.h>
#include <rdma/ib_user_verbs.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include <rdma/ib_addr.h>
#include <rdma/ib_cache.h>
@@ -2578,6 +2580,8 @@ static void get_fw_ver_str(struct ib_device *device, char *str,
(int) dev->dev->caps.fw_ver & 0xffff);
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
static void *mlx4_ib_add(struct mlx4_dev *dev)
{
struct mlx4_ib_dev *ibdev;
@@ -2860,6 +2864,7 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
if (mlx4_ib_alloc_diag_counters(ibdev))
goto err_steer_free_bitmap;
+ ibdev->ib_dev.specs_root = &root;
if (ib_register_device(&ibdev->ib_dev, NULL))
goto err_diag_counters;
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index d45772d..7541296 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -58,6 +58,8 @@
#include <linux/mlx5/vport.h>
#include "mlx5_ib.h"
#include "cmd.h"
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#define DRIVER_NAME "mlx5_ib"
#define DRIVER_VERSION "2.2-1"
@@ -3550,6 +3552,8 @@ static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
return mlx5_rdma_netdev_free(netdev);
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
static void *mlx5_ib_add(struct mlx5_core_dev *mdev)
{
struct mlx5_ib_dev *dev;
@@ -3773,6 +3777,7 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev)
if (err)
goto err_bfreg;
+ dev->ib_dev.specs_root = &root;
err = ib_register_device(&dev->ib_dev, NULL);
if (err)
goto err_fp_bfreg;
diff --git a/drivers/infiniband/hw/mthca/mthca_provider.c b/drivers/infiniband/hw/mthca/mthca_provider.c
index c197cd9..95303ad 100644
--- a/drivers/infiniband/hw/mthca/mthca_provider.c
+++ b/drivers/infiniband/hw/mthca/mthca_provider.c
@@ -37,6 +37,8 @@
#include <rdma/ib_smi.h>
#include <rdma/ib_umem.h>
#include <rdma/ib_user_verbs.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include <linux/sched.h>
#include <linux/slab.h>
@@ -1189,6 +1191,8 @@ static void get_dev_fw_str(struct ib_device *device, char *str,
(int) dev->fw_ver & 0xffff);
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
int mthca_register_device(struct mthca_dev *dev)
{
int ret;
@@ -1296,6 +1300,7 @@ int mthca_register_device(struct mthca_dev *dev)
mutex_init(&dev->cap_mask_mutex);
+ dev->ib_dev.specs_root = &root;
ret = ib_register_device(&dev->ib_dev, NULL);
if (ret)
return ret;
diff --git a/drivers/infiniband/hw/nes/nes_verbs.c b/drivers/infiniband/hw/nes/nes_verbs.c
index 25dcd75..01a4631 100644
--- a/drivers/infiniband/hw/nes/nes_verbs.c
+++ b/drivers/infiniband/hw/nes/nes_verbs.c
@@ -41,6 +41,8 @@
#include <rdma/ib_verbs.h>
#include <rdma/iw_cm.h>
#include <rdma/ib_user_verbs.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include "nes.h"
@@ -3852,6 +3854,8 @@ void nes_destroy_ofa_device(struct nes_ib_device *nesibdev)
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
/**
* nes_register_ofa_device
*/
@@ -3862,6 +3866,7 @@ int nes_register_ofa_device(struct nes_ib_device *nesibdev)
struct nes_adapter *nesadapter = nesdev->nesadapter;
int i, ret;
+ nesvnic->nesibdev->ibdev.specs_root = &root;
ret = ib_register_device(&nesvnic->nesibdev->ibdev, NULL);
if (ret) {
return ret;
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_main.c b/drivers/infiniband/hw/ocrdma/ocrdma_main.c
index 57c9a2a..2ad90cc 100644
--- a/drivers/infiniband/hw/ocrdma/ocrdma_main.c
+++ b/drivers/infiniband/hw/ocrdma/ocrdma_main.c
@@ -44,6 +44,8 @@
#include <linux/idr.h>
#include <rdma/ib_verbs.h>
#include <rdma/ib_user_verbs.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include <rdma/ib_addr.h>
#include <rdma/ib_mad.h>
@@ -116,6 +118,8 @@ static void get_dev_fw_str(struct ib_device *device, char *str,
snprintf(str, str_len, "%s", &dev->attr.fw_ver[0]);
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
static int ocrdma_register_device(struct ocrdma_dev *dev)
{
strlcpy(dev->ibdev.name, "ocrdma%d", IB_DEVICE_NAME_MAX);
@@ -219,6 +223,7 @@ static int ocrdma_register_device(struct ocrdma_dev *dev)
dev->ibdev.destroy_srq = ocrdma_destroy_srq;
dev->ibdev.post_srq_recv = ocrdma_post_srq_recv;
}
+ dev->ibdev.specs_root = &root;
return ib_register_device(&dev->ibdev, NULL);
}
diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c
index 6a72095..e5a7c26 100644
--- a/drivers/infiniband/hw/qedr/main.c
+++ b/drivers/infiniband/hw/qedr/main.c
@@ -33,6 +33,8 @@
#include <rdma/ib_verbs.h>
#include <rdma/ib_addr.h>
#include <rdma/ib_user_verbs.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include <linux/netdevice.h>
#include <linux/iommu.h>
#include <linux/pci.h>
@@ -94,6 +96,8 @@ static struct net_device *qedr_get_netdev(struct ib_device *dev, u8 port_num)
return qdev->ndev;
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
static int qedr_register_device(struct qedr_dev *dev)
{
strlcpy(dev->ibdev.name, "qedr%d", IB_DEVICE_NAME_MAX);
@@ -176,6 +180,7 @@ static int qedr_register_device(struct qedr_dev *dev)
dev->ibdev.get_link_layer = qedr_link_layer;
dev->ibdev.get_dev_fw_str = qedr_get_dev_fw_str;
+ dev->ibdev.specs_root = &root;
return ib_register_device(&dev->ibdev, NULL);
}
diff --git a/drivers/infiniband/hw/usnic/usnic_ib_main.c b/drivers/infiniband/hw/usnic/usnic_ib_main.c
index c0c1e8b..2d7deb6 100644
--- a/drivers/infiniband/hw/usnic/usnic_ib_main.c
+++ b/drivers/infiniband/hw/usnic/usnic_ib_main.c
@@ -48,6 +48,8 @@
#include <linux/netdevice.h>
#include <rdma/ib_user_verbs.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include <rdma/ib_addr.h>
#include "usnic_abi.h"
@@ -348,6 +350,8 @@ static void usnic_get_dev_fw_str(struct ib_device *device,
snprintf(str, str_len, "%s", info.fw_version);
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
/* Start of PF discovery section */
static void *usnic_ib_device_add(struct pci_dev *dev)
{
@@ -434,6 +438,7 @@ static void *usnic_ib_device_add(struct pci_dev *dev)
us_ibdev->ib_dev.get_dev_fw_str = usnic_get_dev_fw_str;
+ us_ibdev->ib_dev.specs_root = &root;
if (ib_register_device(&us_ibdev->ib_dev, NULL))
goto err_fwd_dealloc;
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
index 34ebc76..a8b033d 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
@@ -51,6 +51,8 @@
#include <rdma/ib_addr.h>
#include <rdma/ib_smi.h>
#include <rdma/ib_user_verbs.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include <net/addrconf.h>
#include "pvrdma.h"
@@ -162,6 +164,8 @@ static struct net_device *pvrdma_get_netdev(struct ib_device *ibdev,
return netdev;
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
static int pvrdma_register_device(struct pvrdma_dev *dev)
{
int ret = -1;
@@ -251,6 +255,7 @@ static int pvrdma_register_device(struct pvrdma_dev *dev)
goto err_cq_free;
spin_lock_init(&dev->qp_tbl_lock);
+ dev->ib_dev.specs_root = &root;
ret = ib_register_device(&dev->ib_dev, NULL);
if (ret)
goto err_qp_free;
diff --git a/drivers/infiniband/sw/rdmavt/vt.c b/drivers/infiniband/sw/rdmavt/vt.c
index 0d7c6bb..9c99d45 100644
--- a/drivers/infiniband/sw/rdmavt/vt.c
+++ b/drivers/infiniband/sw/rdmavt/vt.c
@@ -48,6 +48,8 @@
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/dma-mapping.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include "vt.h"
#include "trace.h"
@@ -716,6 +718,8 @@ static noinline int check_support(struct rvt_dev_info *rdi, int verb)
return 0;
}
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
/**
* rvt_register_device - register a driver
* @rdi: main dev structure for all of rdmavt operations
@@ -826,6 +830,7 @@ int rvt_register_device(struct rvt_dev_info *rdi)
rdi->ibdev.node_type = RDMA_NODE_IB_CA;
rdi->ibdev.num_comp_vectors = 1;
+ rdi->ibdev.specs_root = &root;
/* We are now good to announce we exist */
ret = ib_register_device(&rdi->ibdev, rdi->driver_f.port_callback);
if (ret) {
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 83d709e..cf4eef8 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -32,6 +32,8 @@
*/
#include <linux/dma-mapping.h>
+#include <rdma/uverbs_ioctl.h>
+#include <rdma/uverbs_std_types.h>
#include <net/addrconf.h>
#include "rxe.h"
#include "rxe_loc.h"
@@ -1227,6 +1229,8 @@ static ssize_t rxe_show_parent(struct device *device,
&dev_attr_parent,
};
+static DECLARE_UVERBS_TYPES_GROUP(root, &uverbs_common_types);
+
int rxe_register_device(struct rxe_dev *rxe)
{
int err;
@@ -1334,6 +1338,7 @@ int rxe_register_device(struct rxe_dev *rxe)
return PTR_ERR(rxe->tfm);
}
+ dev->specs_root = &root;
err = ib_register_device(dev, NULL);
if (err) {
pr_warn("rxe_register_device failed, err = %d\n", err);
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH for-next 13/13] IB/core: Expose ioctl interface through experimental Kconfig
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (11 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 12/13] IB/core: Assign root to all drivers Matan Barak
@ 2017-06-07 12:22 ` Matan Barak
2017-06-07 15:53 ` [PATCH for-next 00/13] IB/core: SG IOCTL based RDMA ABI Jason Gunthorpe
13 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-07 12:22 UTC (permalink / raw)
To: Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny, Matan Barak
Add CONFIG_INFINIBAND_EXP_USER_ACCESS that enables the ioctl
interface. This interface is experimental and is subject to change.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/Kconfig | 7 +++++++
drivers/infiniband/core/uverbs.h | 2 ++
drivers/infiniband/core/uverbs_main.c | 6 ++++++
3 files changed, 15 insertions(+)
diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index 234fe01..26b2d21 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -34,6 +34,13 @@ config INFINIBAND_USER_ACCESS
libibverbs, libibcm and a hardware driver library from
<http://www.openfabrics.org/git/>.
+config INFINIBAND_EXP_USER_ACCESS
+ bool "Allow experimental support for Infiniband ABI"
+ depends on INFINIBAND_USER_ACCESS
+ ---help---
+ IOCTL based ABI support for Infiniband. This allows userspace
+ to invoke the experimental IOCTL based ABI.
+
config INFINIBAND_USER_MEM
bool
depends on INFINIBAND_USER_ACCESS != n
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 6bb1302..cda0764 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -221,6 +221,8 @@ int ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev, struct ib_xrcd *xrcd,
void ib_uverbs_detach_umcast(struct ib_qp *qp,
struct ib_uqp_object *uobj);
+long ib_uverbs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+
struct ib_uverbs_flow_spec {
union {
union {
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 1836a36..c968b95 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -951,6 +951,9 @@ static int ib_uverbs_close(struct inode *inode, struct file *filp)
.open = ib_uverbs_open,
.release = ib_uverbs_close,
.llseek = no_llseek,
+#if IS_ENABLED(CONFIG_INFINIBAND_EXP_USER_ACCESS)
+ .unlocked_ioctl = ib_uverbs_ioctl,
+#endif
};
static const struct file_operations uverbs_mmap_fops = {
@@ -960,6 +963,9 @@ static int ib_uverbs_close(struct inode *inode, struct file *filp)
.open = ib_uverbs_open,
.release = ib_uverbs_close,
.llseek = no_llseek,
+#if IS_ENABLED(CONFIG_INFINIBAND_EXP_USER_ACCESS)
+ .unlocked_ioctl = ib_uverbs_ioctl,
+#endif
};
static struct ib_client uverbs_client = {
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 00/13] IB/core: SG IOCTL based RDMA ABI
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
` (12 preceding siblings ...)
2017-06-07 12:22 ` [PATCH for-next 13/13] IB/core: Expose ioctl interface through experimental Kconfig Matan Barak
@ 2017-06-07 15:53 ` Jason Gunthorpe
[not found] ` <20170607155323.GB30124-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
13 siblings, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2017-06-07 15:53 UTC (permalink / raw)
To: Matan Barak
Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA, Sean Hefty,
Liran Liss, Yishai Hadas, Leon Romanovsky, Tal Alon,
Christoph Lameter, Ira Weiny, Majd Dibbiny
On Wed, Jun 07, 2017 at 03:22:39PM +0300, Matan Barak wrote:
> Hi Doug,
>
> This patch series adds ioctl() interface to the existing write() interface and
> provide an easy route to backport this change to legacy supported systems.
> Analyzing the current uverbs role in dispatching and parsing commands, we find
> that:
You need to send this series to linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 00/13] IB/core: SG IOCTL based RDMA ABI
[not found] ` <20170607155323.GB30124-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
@ 2017-06-08 14:09 ` Matan Barak
0 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-08 14:09 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Matan Barak, Doug Ledford, linux-rdma, Sean Hefty, Liran Liss,
Yishai Hadas, Leon Romanovsky, Tal Alon, Christoph Lameter,
Ira Weiny, Majd Dibbiny
On Wed, Jun 7, 2017 at 6:53 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Wed, Jun 07, 2017 at 03:22:39PM +0300, Matan Barak wrote:
>> Hi Doug,
>>
>> This patch series adds ioctl() interface to the existing write() interface and
>> provide an easy route to backport this change to legacy supported systems.
>> Analyzing the current uverbs role in dispatching and parsing commands, we find
>> that:
>
> You need to send this series to linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>
Yeah, let's start review this V0 internally in linux-rdma.
I'll send V1 to both lists.
> Jason
Matan
> --
> 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
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH for-next 02/13] IB/core: Add support to finalize objects in one transaction
[not found] ` <1496838172-39671-3-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2017-06-08 23:51 ` Hefty, Sean
[not found] ` <1828884A29C6694DAF28B7E6B8A82373AB141A91-P5GAC/sN6hkd3b2yrw5b5LfspsVTdybXVpNB7YpNyf8@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Hefty, Sean @ 2017-06-08 23:51 UTC (permalink / raw)
To: Matan Barak, Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Liran Liss,
Yishai Hadas, Leon Romanovsky, Tal Alon, Christoph Lameter,
Weiny, Ira, Majd Dibbiny
> The new ioctl based infrastructure either commits or rollbacks
> all objects of the command as one transaction. In order to do
> that, we introduce a notion of dealing with a collection of
> objects that are related to a specific action.
>
> This also requires adding a notion of an action and attribute.
> An action contains a groups of attributes, where each group
> contains several attributes.
>
> For example, an object could be a CQ, which has an action of
> CREATE_CQ.
> This action has multiple attributes. For example, the CQ's new handle
> and the comp_channel. Each layer in this hierarchy - objects, actions
> and attributes is split into groups. The common example for that is
> one group representing the common entities and another one
> representing the driver specific entities.
>
> When declaring these actions and attributes, we actually declare
> their specifications. When a command is executed, we actually
> allocates some space to hold auxiliary information. This auxiliary
> information contains meta-data about the required objects, such
> as pointers to their type information, pointers to the uobjects
> themselves (if exist), etc.
> The specification, along with the auxiliary information we allocated
> and filled is given to the finalize_objects function.
>
> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
> drivers/infiniband/core/rdma_core.c | 39
> ++++++++++++++++++++++++++++++
> drivers/infiniband/core/rdma_core.h | 22 ++++++++++++++++-
> include/rdma/uverbs_ioctl.h | 48
> +++++++++++++++++++++++++++++++++++++
> 3 files changed, 108 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/rdma_core.c
> b/drivers/infiniband/core/rdma_core.c
> index 2bd58ff..3d3cf07 100644
> --- a/drivers/infiniband/core/rdma_core.c
> +++ b/drivers/infiniband/core/rdma_core.c
> @@ -683,3 +683,42 @@ int uverbs_finalize_object(struct ib_uobject
> *uobj,
>
> return ret;
> }
> +
> +int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
> + struct uverbs_attr_spec_group ** const
> spec_groups,
> + size_t num,
> + bool commit)
> +{
> + unsigned int i;
> + int ret = 0;
> +
> + for (i = 0; i < num; i++) {
> + struct uverbs_attr_array *attr_group_array =
> &attr_array[i];
> + const struct uverbs_attr_spec_group *attr_spec_group =
> + spec_groups[i];
> + unsigned int j;
> +
> + for (j = 0; j < attr_group_array->num_attrs; j++) {
> + struct uverbs_attr *attr;
> + struct uverbs_attr_spec *spec;
> +
> + if (!uverbs_is_valid(attr_group_array, j))
> + continue;
> +
> + attr = &attr_group_array->attrs[j];
> + spec = &attr_spec_group->attrs[j];
> +
> + if (spec->type == UVERBS_ATTR_TYPE_IDR ||
> + spec->type == UVERBS_ATTR_TYPE_FD) {
> + int current_ret;
> +
> + current_ret = uverbs_finalize_object(attr-
> >obj_attr.uobject,
> + spec->obj.access,
> + commit);
> + if (!ret)
> + ret = current_ret;
> + }
> + }
> + }
> + return ret;
> +}
> diff --git a/drivers/infiniband/core/rdma_core.h
> b/drivers/infiniband/core/rdma_core.h
> index 97483d1..5cc00eb 100644
> --- a/drivers/infiniband/core/rdma_core.h
> +++ b/drivers/infiniband/core/rdma_core.h
> @@ -82,7 +82,8 @@
> * applicable.
> * This function could create (access == NEW), destroy (access ==
> DESTROY)
> * or unlock (access == READ || access == WRITE) objects if required.
> - * The action will be finalized only when uverbs_finalize_object is
> called.
> + * The action will be finalized only when uverbs_finalize_object or
> + * uverbs_finalize_objects are called.
> */
> struct ib_uobject *uverbs_get_uobject_from_context(const struct
> uverbs_obj_type *type_attrs,
> struct ib_ucontext *ucontext,
> @@ -91,5 +92,24 @@ struct ib_uobject
> *uverbs_get_uobject_from_context(const struct uverbs_obj_type
> int uverbs_finalize_object(struct ib_uobject *uobj,
> enum uverbs_obj_access access,
> bool commit);
> +/*
> + * Note that certain finalize stages could return a status:
> + * (a) alloc_commit could return a failure if the object is
> committed at the
> + * same time when the context is destroyed.
> + * (b) remove_commit could fail if the object wasn't destroyed
> successfully.
> + * Since multiple objects could be finalized in one transaction, it
> is very NOT
> + * recommended to have several finalize actions which have side
> effects.
> + * For example, it's NOT recommended to have a certain action which
> has both
> + * a commit action and a destroy action or two destroy objects in the
> same
> + * action. The rule of thumb is to have one destroy or commit action
> with
> + * multiple lookups.
> + * The first non zero return value of finalize_object is returned
> from this
> + * function. For example, this could happen when we couldn't destroy
> an
> + * object.
> + */
> +int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
> + struct uverbs_attr_spec_group ** const
> spec_groups,
> + size_t num,
> + bool commit);
>
> #endif /* RDMA_CORE_H */
> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
> index 4ff87ee..e06f4cf 100644
> --- a/include/rdma/uverbs_ioctl.h
> +++ b/include/rdma/uverbs_ioctl.h
> @@ -41,6 +41,12 @@
> * =======================================
> */
>
> +enum uverbs_attr_type {
> + UVERBS_ATTR_TYPE_NA,
> + UVERBS_ATTR_TYPE_IDR,
> + UVERBS_ATTR_TYPE_FD,
> +};
> +
> enum uverbs_obj_access {
> UVERBS_ACCESS_READ,
> UVERBS_ACCESS_WRITE,
> @@ -48,5 +54,47 @@ enum uverbs_obj_access {
> UVERBS_ACCESS_DESTROY
> };
>
> +struct uverbs_attr_spec {
> + enum uverbs_attr_type type;
> + struct {
> + /*
> + * higher bits mean the group and lower bits mean
> + * the type id within the group.
> + */
> + u16 obj_type;
Why aren't separate fields used instead of an unspecified bit separation?
> + u8 access;
> + } obj;
> +};
> +
> +struct uverbs_attr_spec_group {
> + struct uverbs_attr_spec *attrs;
> + size_t num_attrs;
> +};
> +
> +struct uverbs_obj_attr {
> + struct ib_uobject *uobject;
> +};
> +
> +struct uverbs_attr {
> + struct uverbs_obj_attr obj_attr;
> +};
I'm assuming the above two structures are expanded in subsequent patches.
> +
> +struct uverbs_attr_array {
> + /* if bit i is set, it means attrs[i] contains valid information
> */
> + unsigned long *valid_bitmap;
> + size_t num_attrs;
> + /*
> + * arrays of attributes, each element corresponds to the
> specification
> + * of the attribute in the same index.
> + */
> + struct uverbs_attr *attrs;
> +};
> +
> +static inline bool uverbs_is_valid(const struct uverbs_attr_array
Call uverbs_attr_is_valid() instead?
> *attr_array,
> + unsigned int idx)
> +{
> + return test_bit(idx, attr_array->valid_bitmap);
> +}
> +
> #endif
>
> --
> 1.8.3.1
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 02/13] IB/core: Add support to finalize objects in one transaction
[not found] ` <1828884A29C6694DAF28B7E6B8A82373AB141A91-P5GAC/sN6hkd3b2yrw5b5LfspsVTdybXVpNB7YpNyf8@public.gmane.org>
@ 2017-06-11 8:16 ` Matan Barak
[not found] ` <CAAKD3BCpRwiCejxuKiP07q-jGoRF1W8Ovs5aMt07uO2gAc1Qug-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Matan Barak @ 2017-06-11 8:16 UTC (permalink / raw)
To: Hefty, Sean
Cc: Matan Barak, Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
Jason Gunthorpe, Liran Liss, Yishai Hadas, Leon Romanovsky,
Tal Alon, Christoph Lameter, Weiny, Ira, Majd Dibbiny
On Fri, Jun 9, 2017 at 2:51 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> The new ioctl based infrastructure either commits or rollbacks
>> all objects of the command as one transaction. In order to do
>> that, we introduce a notion of dealing with a collection of
>> objects that are related to a specific action.
>>
>> This also requires adding a notion of an action and attribute.
>> An action contains a groups of attributes, where each group
>> contains several attributes.
>>
>> For example, an object could be a CQ, which has an action of
>> CREATE_CQ.
>> This action has multiple attributes. For example, the CQ's new handle
>> and the comp_channel. Each layer in this hierarchy - objects, actions
>> and attributes is split into groups. The common example for that is
>> one group representing the common entities and another one
>> representing the driver specific entities.
>>
>> When declaring these actions and attributes, we actually declare
>> their specifications. When a command is executed, we actually
>> allocates some space to hold auxiliary information. This auxiliary
>> information contains meta-data about the required objects, such
>> as pointers to their type information, pointers to the uobjects
>> themselves (if exist), etc.
>> The specification, along with the auxiliary information we allocated
>> and filled is given to the finalize_objects function.
>>
>> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> ---
>> drivers/infiniband/core/rdma_core.c | 39
>> ++++++++++++++++++++++++++++++
>> drivers/infiniband/core/rdma_core.h | 22 ++++++++++++++++-
>> include/rdma/uverbs_ioctl.h | 48
>> +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 108 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/core/rdma_core.c
>> b/drivers/infiniband/core/rdma_core.c
>> index 2bd58ff..3d3cf07 100644
>> --- a/drivers/infiniband/core/rdma_core.c
>> +++ b/drivers/infiniband/core/rdma_core.c
>> @@ -683,3 +683,42 @@ int uverbs_finalize_object(struct ib_uobject
>> *uobj,
>>
>> return ret;
>> }
>> +
>> +int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
>> + struct uverbs_attr_spec_group ** const
>> spec_groups,
>> + size_t num,
>> + bool commit)
>> +{
>> + unsigned int i;
>> + int ret = 0;
>> +
>> + for (i = 0; i < num; i++) {
>> + struct uverbs_attr_array *attr_group_array =
>> &attr_array[i];
>> + const struct uverbs_attr_spec_group *attr_spec_group =
>> + spec_groups[i];
>> + unsigned int j;
>> +
>> + for (j = 0; j < attr_group_array->num_attrs; j++) {
>> + struct uverbs_attr *attr;
>> + struct uverbs_attr_spec *spec;
>> +
>> + if (!uverbs_is_valid(attr_group_array, j))
>> + continue;
>> +
>> + attr = &attr_group_array->attrs[j];
>> + spec = &attr_spec_group->attrs[j];
>> +
>> + if (spec->type == UVERBS_ATTR_TYPE_IDR ||
>> + spec->type == UVERBS_ATTR_TYPE_FD) {
>> + int current_ret;
>> +
>> + current_ret = uverbs_finalize_object(attr-
>> >obj_attr.uobject,
>> + spec->obj.access,
>> + commit);
>> + if (!ret)
>> + ret = current_ret;
>> + }
>> + }
>> + }
>> + return ret;
>> +}
>> diff --git a/drivers/infiniband/core/rdma_core.h
>> b/drivers/infiniband/core/rdma_core.h
>> index 97483d1..5cc00eb 100644
>> --- a/drivers/infiniband/core/rdma_core.h
>> +++ b/drivers/infiniband/core/rdma_core.h
>> @@ -82,7 +82,8 @@
>> * applicable.
>> * This function could create (access == NEW), destroy (access ==
>> DESTROY)
>> * or unlock (access == READ || access == WRITE) objects if required.
>> - * The action will be finalized only when uverbs_finalize_object is
>> called.
>> + * The action will be finalized only when uverbs_finalize_object or
>> + * uverbs_finalize_objects are called.
>> */
>> struct ib_uobject *uverbs_get_uobject_from_context(const struct
>> uverbs_obj_type *type_attrs,
>> struct ib_ucontext *ucontext,
>> @@ -91,5 +92,24 @@ struct ib_uobject
>> *uverbs_get_uobject_from_context(const struct uverbs_obj_type
>> int uverbs_finalize_object(struct ib_uobject *uobj,
>> enum uverbs_obj_access access,
>> bool commit);
>> +/*
>> + * Note that certain finalize stages could return a status:
>> + * (a) alloc_commit could return a failure if the object is
>> committed at the
>> + * same time when the context is destroyed.
>> + * (b) remove_commit could fail if the object wasn't destroyed
>> successfully.
>> + * Since multiple objects could be finalized in one transaction, it
>> is very NOT
>> + * recommended to have several finalize actions which have side
>> effects.
>> + * For example, it's NOT recommended to have a certain action which
>> has both
>> + * a commit action and a destroy action or two destroy objects in the
>> same
>> + * action. The rule of thumb is to have one destroy or commit action
>> with
>> + * multiple lookups.
>> + * The first non zero return value of finalize_object is returned
>> from this
>> + * function. For example, this could happen when we couldn't destroy
>> an
>> + * object.
>> + */
>> +int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
>> + struct uverbs_attr_spec_group ** const
>> spec_groups,
>> + size_t num,
>> + bool commit);
>>
>> #endif /* RDMA_CORE_H */
>> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
>> index 4ff87ee..e06f4cf 100644
>> --- a/include/rdma/uverbs_ioctl.h
>> +++ b/include/rdma/uverbs_ioctl.h
>> @@ -41,6 +41,12 @@
>> * =======================================
>> */
>>
>> +enum uverbs_attr_type {
>> + UVERBS_ATTR_TYPE_NA,
>> + UVERBS_ATTR_TYPE_IDR,
>> + UVERBS_ATTR_TYPE_FD,
>> +};
>> +
>> enum uverbs_obj_access {
>> UVERBS_ACCESS_READ,
>> UVERBS_ACCESS_WRITE,
>> @@ -48,5 +54,47 @@ enum uverbs_obj_access {
>> UVERBS_ACCESS_DESTROY
>> };
>>
>> +struct uverbs_attr_spec {
>> + enum uverbs_attr_type type;
>> + struct {
>> + /*
>> + * higher bits mean the group and lower bits mean
>> + * the type id within the group.
>> + */
>> + u16 obj_type;
>
> Why aren't separate fields used instead of an unspecified bit separation?
>
>
The "spec" part isn't passed from user-space to kernel. It's intended
to instruct the kernel parser how to parse the command.
Since these structs are parsed automatically and built using macros, I
was in favor of reducing the .text section size here.
>> + u8 access;
>> + } obj;
>> +};
>> +
>> +struct uverbs_attr_spec_group {
>> + struct uverbs_attr_spec *attrs;
>> + size_t num_attrs;
>> +};
>> +
>> +struct uverbs_obj_attr {
>> + struct ib_uobject *uobject;
>> +};
>> +
>> +struct uverbs_attr {
>> + struct uverbs_obj_attr obj_attr;
>> +};
>
> I'm assuming the above two structures are expanded in subsequent patches.
>
>
Yeah, pointer based attributes, which simply carry blobs are added in the
"Add new ioctl interface" patch.
>> +
>> +struct uverbs_attr_array {
>> + /* if bit i is set, it means attrs[i] contains valid information
>> */
>> + unsigned long *valid_bitmap;
>> + size_t num_attrs;
>> + /*
>> + * arrays of attributes, each element corresponds to the
>> specification
>> + * of the attribute in the same index.
>> + */
>> + struct uverbs_attr *attrs;
>> +};
>> +
>> +static inline bool uverbs_is_valid(const struct uverbs_attr_array
>
> Call uverbs_attr_is_valid() instead?
>
>
Ok
>> *attr_array,
>> + unsigned int idx)
>> +{
>> + return test_bit(idx, attr_array->valid_bitmap);
>> +}
>> +
>> #endif
>>
>> --
>> 1.8.3.1
>
Thanks for taking a look.
Matan
> --
> 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
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <1496838172-39671-4-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2017-06-13 22:48 ` Hefty, Sean
[not found] ` <1828884A29C6694DAF28B7E6B8A82373AB143340-P5GAC/sN6hkd3b2yrw5b5LfspsVTdybXVpNB7YpNyf8@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Hefty, Sean @ 2017-06-13 22:48 UTC (permalink / raw)
To: Matan Barak, Doug Ledford
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Liran Liss,
Yishai Hadas, Leon Romanovsky, Tal Alon, Christoph Lameter,
Weiny, Ira, Majd Dibbiny
> In this ioctl interface, processing the command starts from
> properties of the command and fetching the appropriate user objects
> before calling the handler.
>
> Parsing and validation is done according to a specifier declared by
> the driver's code. In the driver, all supported types are declared.
> These types are separated to different type groups, each could be
> declared in a different place (for example, common types and driver
> specific types).
>
> For each type we list all supported actions. Similarly to types,
> actions are separated to actions groups too. Each group is declared
> separately. This could be used in order to add actions to an existing
> type.
>
> Each action has a specific handler, which could be either a common
> handler or a driver specific handler.
> Along with the handler, a group of attributes is specified as well.
> This group lists all supported attributes and is used for automatic
> fetching and validation of the command, response and its related
> objects.
>
> When a group of elements is used, the high bits of the elements ids
> are used in order to calculate the group index. Then, these high bits
> are masked out in order to have a zero based namespace for every
> group. This is mandatory for compact representation and O(1) array
> access.
>
> A group of attributes is actually an array of attributes. Each
> attribute has a type (PTR_IN, PTR_OUT, IDR and FD) and a length.
> Attributes could be validated through some attributes, like:
> (*) Minimum size / Exact size
> (*) Fops for FD
> (*) Object type for IDR
>
> If an IDR/fd attribute is specified, the kernel also states the object
> type and the required access (NEW, WRITE, READ or DESTROY).
> All uobject/fd management is done automatically by the infrastructure,
> meaning - the infrastructure will fail concurrent commands that at
> least one of them requires concurrent access (WRITE/DESTROY),
> synchronize actions with device removals (dissociate context events)
> and take care of reference counting (increase/decrease) for concurrent
> actions invocation. The reference counts on the actual kernel objects
> shall be handled by the handlers.
>
> types
> +--------+
> | |
> | | actions
> +--------+
> | | group action action_spec
> +-----+ |len |
> +--------+ +------+[d]+-------+ +----------------+[d]+------------+
> |attr1+-> |type |
> | type +> |action+-> | spec +-> + attr_groups +->
> |common_chain+--> +-----+ |idr_type|
> +--------+ +------+ |handler| | | +------------+
> |attr2| |access |
> | | | | +-------+ +----------------+ |vendor chain|
> +-----+ +--------+
> | | | | +------------+
> | | +------+
> | |
> | |
> | |
> | |
> | |
> | |
> | |
> | |
> | |
> | |
> +--------+
Architecturally, this looks decent.
It took me a few times reading through this before I could start to understand what this is describing though. (And I'm still not sure I'm there). I think part of the problem is that this patch introduces terms for abstract concepts that don't easily convey the intent. (At least they didn't to me.) Simply changing some of the terms may help people figure out how to use this.
At the top-level ("type"), I think you're describing the concept of an object-oriented class. Each class contains a set ("action group") of methods or operations ("action"). An operation takes a set of parameters ("action_spec"). Some parameters are common ("common_chain") and some vendor specific ("vendor_chain"). Each parameter is a some data type ("attr"). Please confirm if this is correct.
Assuming my understanding is correct (and I haven't read through the rest of the patches yet), then I would suggest using these terms instead (based on standard OO design terminology):
type_group -> namespace (?)
type -> class or obj_class
action_group -> operations or methods or functions
action -> op_def or method_def or func_def
attr_spec_group -> param_list or op_param_list or func_param_list ...
common_chain -> common_params
vendor_chain -> vendor_params
nit: attr_spec -> attr_def
TYPE_IDR -> TYPE_KOBJ (kernel object)
See other comments below.
> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
> index e06f4cf..7fed6d9 100644
> --- a/include/rdma/uverbs_ioctl.h
> +++ b/include/rdma/uverbs_ioctl.h
> @@ -41,8 +41,13 @@
> * =======================================
> */
>
> +#define UVERBS_ID_GROUP_MASK 0xF000
> +#define UVERBS_ID_GROUP_SHIFT 12
> +
> enum uverbs_attr_type {
> UVERBS_ATTR_TYPE_NA,
> + UVERBS_ATTR_TYPE_PTR_IN,
> + UVERBS_ATTR_TYPE_PTR_OUT,
> UVERBS_ATTR_TYPE_IDR,
This is really indicating that we're dealing with a kernel object of some sort, not the actual indexer.
> UVERBS_ATTR_TYPE_FD,
> };
> @@ -54,29 +59,106 @@ enum uverbs_obj_access {
> UVERBS_ACCESS_DESTROY
> };
>
> +enum uverbs_attr_spec_flags {
> + UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
> + /* Support extending attributes by length */
> + UVERBS_ATTR_SPEC_F_MIN_SZ = 1U << 1,
> +};
Don't use named enums for bit flags. The result of ORing flags ends up as a non-enum value.
> +
> struct uverbs_attr_spec {
> enum uverbs_attr_type type;
> - struct {
> - /*
> - * higher bits mean the group and lower bits mean
> - * the type id within the group.
> - */
> - u16 obj_type;
> - u8 access;
> - } obj;
> + /* a combination of enum uverbs_attr_spec_flags */
> + u8 flags;
> + union {
> + u16 len;
> + struct {
> + /*
> + * higher bits mean the group and lower bits mean
> + * the type id within the group.
> + */
> + u16 obj_type;
> + u8 access;
> + } obj;
> + };
You mentioned trying to conserve space, but this layout will have 1 byte padding after flags and 3 bytes at the end. Swapping flags to the end should eliminate the padding
> };
>
> struct uverbs_attr_spec_group {
> struct uverbs_attr_spec *attrs;
> size_t num_attrs;
> + /* populate at runtime */
> + unsigned long *mandatory_attrs_bitmask;
> +};
I'm assuming that this references all the parameters for a given method, so suggesting renaming to param_list.
> +
> +struct uverbs_attr_array;
> +struct ib_uverbs_file;
> +
> +enum uverbs_action_flags {
> + /*
> + * Action marked with this flag creates a context (or root for
> all
> + * objects).
> + */
> + UVERBS_ACTION_FLAG_CREATE_ROOT = 1U << 0,
> +};
Same comment as above regarding named enums.
> +
> +struct uverbs_action {
> + struct uverbs_attr_spec_group **attr_groups;
> + size_t num_groups;
> + size_t num_child_attrs;
> + /* Combination of bits from enum uverbs_action_flags */
> + u32 flags;
> + int (*handler)(struct ib_device *ib_dev, struct ib_uverbs_file
> *ufile,
> + struct uverbs_attr_array *ctx, size_t num);
> +};
I think this is defining the function handler and parameters of a single method. Why is attr_groups **, rather than just *?
> +
> +struct uverbs_action_group {
> + size_t num_actions;
> + struct uverbs_action **actions;
> +};
> +
> +struct uverbs_type {
> + size_t num_groups;
> + const struct uverbs_action_group **action_groups;
> + const struct uverbs_obj_type *type_attrs;
> +};
Conceptually, I think this is representing an object-oriented class; consider renaming type to class. Similar to my question above, why is action_groups **, rather than *? Actually, why doesn't this just reference an array of uverbs_action? The extra layers of indirection to **actions_groups -> ** actions seems overly complex.
> +
> +struct uverbs_type_group {
> + size_t num_types;
> + const struct uverbs_type **types;
> +};
> +
> +struct uverbs_spec_root {
> + const struct uverbs_type_group **type_groups;
> + size_t num_groups;
> +};
Similar comments as above regarding **type_groups and **types. Can we avoid the extra indirection, and maybe drop the action_group and type_group structures completely? (If this is purely a space issue, we're only saving 1-2 pointers worth of space).
> +
> +/* =================================================
> + * Parsing infrastructure
> + * =================================================
> + */
> +
> +struct uverbs_ptr_attr {
> + void __user *ptr;
> + u16 len;
> };
>
> struct uverbs_obj_attr {
> + /*
> + * pointer to the user-space given attribute, in order to write
> the
> + * new uobject's id.
> + */
> + struct ib_uverbs_attr __user *uattr;
> + /* pointer to the kernel descriptor -> type, access, etc */
> + const struct uverbs_obj_type *type;
> struct ib_uobject *uobject;
> + /* fd or id in idr of this object */
> + int id;
> };
>
> struct uverbs_attr {
> - struct uverbs_obj_attr obj_attr;
> + union {
> + struct uverbs_ptr_attr ptr_attr;
> + struct uverbs_obj_attr obj_attr;
> + };
> };
>
> struct uverbs_attr_array {
> diff --git a/include/uapi/rdma/rdma_user_ioctl.h
> b/include/uapi/rdma/rdma_user_ioctl.h
> index 9388125..3a40b9d 100644
> --- a/include/uapi/rdma/rdma_user_ioctl.h
> +++ b/include/uapi/rdma/rdma_user_ioctl.h
> @@ -43,6 +43,30 @@
> /* Legacy name, for user space application which already use it */
> #define IB_IOCTL_MAGIC RDMA_IOCTL_MAGIC
>
> +#define RDMA_VERBS_IOCTL \
> + _IOWR(RDMA_IOCTL_MAGIC, 1, struct ib_uverbs_ioctl_hdr)
> +
> +enum ib_uverbs_attr_flags {
> + UVERBS_ATTR_F_MANDATORY = 1U << 0,
> +};
Named enum for flags
> +
> +struct ib_uverbs_attr {
> + __u16 attr_id; /* command specific type attribute */
> + __u16 len; /* only for pointers */
> + __u16 flags; /* combination of ib_uverbs_attr_flags
> */
> + __u16 reserved;
> + __u64 data; /* ptr to command, inline data or idr/fd */
> +};
> +
> +struct ib_uverbs_ioctl_hdr {
> + __u16 length;
> + __u16 object_type;
> + __u16 action;
> + __u16 num_attrs;
> + __u64 reserved;
> + struct ib_uverbs_attr attrs[0];
> +};
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <1828884A29C6694DAF28B7E6B8A82373AB143340-P5GAC/sN6hkd3b2yrw5b5LfspsVTdybXVpNB7YpNyf8@public.gmane.org>
@ 2017-06-14 8:39 ` Matan Barak
[not found] ` <CAAKD3BArF5U49WQ2PcYjpSbPpQmyubJQ6vKKUer_L05QBQpT0w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Matan Barak @ 2017-06-14 8:39 UTC (permalink / raw)
To: Hefty, Sean
Cc: Matan Barak, Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
Jason Gunthorpe, Liran Liss, Yishai Hadas, Leon Romanovsky,
Tal Alon, Christoph Lameter, Weiny, Ira, Majd Dibbiny
On Wed, Jun 14, 2017 at 1:48 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> In this ioctl interface, processing the command starts from
>> properties of the command and fetching the appropriate user objects
>> before calling the handler.
>>
>> Parsing and validation is done according to a specifier declared by
>> the driver's code. In the driver, all supported types are declared.
>> These types are separated to different type groups, each could be
>> declared in a different place (for example, common types and driver
>> specific types).
>>
>> For each type we list all supported actions. Similarly to types,
>> actions are separated to actions groups too. Each group is declared
>> separately. This could be used in order to add actions to an existing
>> type.
>>
>> Each action has a specific handler, which could be either a common
>> handler or a driver specific handler.
>> Along with the handler, a group of attributes is specified as well.
>> This group lists all supported attributes and is used for automatic
>> fetching and validation of the command, response and its related
>> objects.
>>
>> When a group of elements is used, the high bits of the elements ids
>> are used in order to calculate the group index. Then, these high bits
>> are masked out in order to have a zero based namespace for every
>> group. This is mandatory for compact representation and O(1) array
>> access.
>>
>> A group of attributes is actually an array of attributes. Each
>> attribute has a type (PTR_IN, PTR_OUT, IDR and FD) and a length.
>> Attributes could be validated through some attributes, like:
>> (*) Minimum size / Exact size
>> (*) Fops for FD
>> (*) Object type for IDR
>>
>> If an IDR/fd attribute is specified, the kernel also states the object
>> type and the required access (NEW, WRITE, READ or DESTROY).
>> All uobject/fd management is done automatically by the infrastructure,
>> meaning - the infrastructure will fail concurrent commands that at
>> least one of them requires concurrent access (WRITE/DESTROY),
>> synchronize actions with device removals (dissociate context events)
>> and take care of reference counting (increase/decrease) for concurrent
>> actions invocation. The reference counts on the actual kernel objects
>> shall be handled by the handlers.
>>
>> types
>> +--------+
>> | |
>> | | actions
>> +--------+
>> | | group action action_spec
>> +-----+ |len |
>> +--------+ +------+[d]+-------+ +----------------+[d]+------------+
>> |attr1+-> |type |
>> | type +> |action+-> | spec +-> + attr_groups +->
>> |common_chain+--> +-----+ |idr_type|
>> +--------+ +------+ |handler| | | +------------+
>> |attr2| |access |
>> | | | | +-------+ +----------------+ |vendor chain|
>> +-----+ +--------+
>> | | | | +------------+
>> | | +------+
>> | |
>> | |
>> | |
>> | |
>> | |
>> | |
>> | |
>> | |
>> | |
>> | |
>> +--------+
>
> Architecturally, this looks decent.
>
> It took me a few times reading through this before I could start to understand what this is describing though. (And I'm still not sure I'm there). I think part of the problem is that this patch introduces terms for abstract concepts that don't easily convey the intent. (At least they didn't to me.) Simply changing some of the terms may help people figure out how to use this.
>
> At the top-level ("type"), I think you're describing the concept of an object-oriented class. Each class contains a set ("action group") of methods or operations ("action"). An operation takes a set of parameters ("action_spec"). Some parameters are common ("common_chain") and some vendor specific ("vendor_chain"). Each parameter is a some data type ("attr"). Please confirm if this is correct.
>
Let's start from the root of the parsing tree. at the top level we
have the root of the parsing tree ("uverbs_spec_root"). The root has
several "type_groups", one for each "namespace".
Currently, two "namespace"s are declared - common and driver
("vendor_chain" here, should be changed to "driver").
Each "type_group" contains a few "type"s. "type" has several
"action_group"s, one for each "namespace". Each "action_group"
contains several "action"s.
Each "action" contains groups of "attr_spec", one for each namespace.
Finally, "attr_spec" contains attributes ("attrs").
Each attribute is of one of the following classes: ptr_in, ptr_out,
idr or fd (in the future, we might add a few more like "flags").
> Assuming my understanding is correct (and I haven't read through the rest of the patches yet), then I would suggest using these terms instead (based on standard OO design terminology):
>
> type_group -> namespace (?)
I currently use "namespace" term to differentiate between the groups
of the entities. For example, in each layer of the hierarchy we
(possibly) have a namespace of common entities and driver-specific
entities.
> type -> class or obj_class
I use the term "class" for the nature of the actual attributes
("attrs"). Each attribute is one of the following classes: idr, fd,
ptr_in or ptr_out.
> action_group -> operations or methods or functions
> action -> op_def or method_def or func_def
> attr_spec_group -> param_list or op_param_list or func_param_list ...
I used the term "group" to emphasize that these aren't just a random
collection of params in a lit.
Grouping this entities together has a specific meaning.
> common_chain -> common_params
> vendor_chain -> vendor_params
I agree the term "chain" is problematic. Regarding the drawing above -
"common_params" and "driver_params" seems fine.
> nit: attr_spec -> attr_def
Actually, the term "spec" is used all over the code in order to
emphasize this is a specification the kernel is using for parsing.
> TYPE_IDR -> TYPE_KOBJ (kernel object)
>
What about fds (like completion_channel)? They're KOBJs too.
I don't have an objection coming up with a better name-schema that
captures the subtle details mentioned above :)
> See other comments below.
>
>> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
>> index e06f4cf..7fed6d9 100644
>> --- a/include/rdma/uverbs_ioctl.h
>> +++ b/include/rdma/uverbs_ioctl.h
>> @@ -41,8 +41,13 @@
>> * =======================================
>> */
>>
>> +#define UVERBS_ID_GROUP_MASK 0xF000
>> +#define UVERBS_ID_GROUP_SHIFT 12
>> +
>> enum uverbs_attr_type {
>> UVERBS_ATTR_TYPE_NA,
>> + UVERBS_ATTR_TYPE_PTR_IN,
>> + UVERBS_ATTR_TYPE_PTR_OUT,
>> UVERBS_ATTR_TYPE_IDR,
>
> This is really indicating that we're dealing with a kernel object of some sort, not the actual indexer.
>
Yeah, but FDs could represent kernel objects too. The meaning here are
kernel objects that resides in the idr.
What do you propose?
>> UVERBS_ATTR_TYPE_FD,
>> };
>> @@ -54,29 +59,106 @@ enum uverbs_obj_access {
>> UVERBS_ACCESS_DESTROY
>> };
>>
>> +enum uverbs_attr_spec_flags {
>> + UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
>> + /* Support extending attributes by length */
>> + UVERBS_ATTR_SPEC_F_MIN_SZ = 1U << 1,
>> +};
>
> Don't use named enums for bit flags. The result of ORing flags ends up as a non-enum value.
>
Sure
>> +
>> struct uverbs_attr_spec {
>> enum uverbs_attr_type type;
>> - struct {
>> - /*
>> - * higher bits mean the group and lower bits mean
>> - * the type id within the group.
>> - */
>> - u16 obj_type;
>> - u8 access;
>> - } obj;
>> + /* a combination of enum uverbs_attr_spec_flags */
>> + u8 flags;
>> + union {
>> + u16 len;
>> + struct {
>> + /*
>> + * higher bits mean the group and lower bits mean
>> + * the type id within the group.
>> + */
>> + u16 obj_type;
>> + u8 access;
>> + } obj;
>> + };
>
> You mentioned trying to conserve space, but this layout will have 1 byte padding after flags and 3 bytes at the end. Swapping flags to the end should eliminate the padding
>
Yep, thanks
>> };
>>
>> struct uverbs_attr_spec_group {
>> struct uverbs_attr_spec *attrs;
>> size_t num_attrs;
>> + /* populate at runtime */
>> + unsigned long *mandatory_attrs_bitmask;
>> +};
>
> I'm assuming that this references all the parameters for a given method, so suggesting renaming to param_list.
>
This implies changing all attrs->params (to keep the terms in sync).
Let's come with a clear schema for all entities.
>> +
>> +struct uverbs_attr_array;
>> +struct ib_uverbs_file;
>> +
>> +enum uverbs_action_flags {
>> + /*
>> + * Action marked with this flag creates a context (or root for
>> all
>> + * objects).
>> + */
>> + UVERBS_ACTION_FLAG_CREATE_ROOT = 1U << 0,
>> +};
>
> Same comment as above regarding named enums.
>
Sure, thanks.
>> +
>> +struct uverbs_action {
>> + struct uverbs_attr_spec_group **attr_groups;
>> + size_t num_groups;
>> + size_t num_child_attrs;
>> + /* Combination of bits from enum uverbs_action_flags */
>> + u32 flags;
>> + int (*handler)(struct ib_device *ib_dev, struct ib_uverbs_file
>> *ufile,
>> + struct uverbs_attr_array *ctx, size_t num);
>> +};
>
> I think this is defining the function handler and parameters of a single method. Why is attr_groups **, rather than just *?
>
It points to an array (defined somewhere). Each entry in this array is
a pointer to a uverbs_attr_spec_group (defined somewhere).
For example, in the driver's code we define a modified copy of the
create_cq operation, i.e. mlx5_create_cq_action.
mlx5_create_cq_action points to an array (usually unnamed, but let's
name it for the ease of discussion): mlx5_create_cq_attr_groups.
mlx5_create_cq_attr_groups is an array of 2 - the first is a pointer
to uverbs_common_create_cq_attrs and the second one is
mlx5_create_cq_attrs.
>> +
>> +struct uverbs_action_group {
>> + size_t num_actions;
>> + struct uverbs_action **actions;
>> +};
>> +
>> +struct uverbs_type {
>> + size_t num_groups;
>> + const struct uverbs_action_group **action_groups;
>> + const struct uverbs_obj_type *type_attrs;
>> +};
>
> Conceptually, I think this is representing an object-oriented class; consider renaming type to class. Similar to my question above, why is action_groups **, rather than *? Actually, why doesn't this just reference an array of uverbs_action? The extra layers of indirection to **actions_groups -> ** actions seems overly complex.
>
The problem with class is that it's already used for attribute classes
(idr, fd, ptr_in, ptr_out and in the future maybe "flags").
The pointer-to-pointer schema is used for the exact same reason as
before. We have an array of pointers. For the sake of completion,
let's say the mlx5 driver adds a new action for type cq, so we'll
have:
mlx5_cq.action_groups -> point to an array of pointers of
action_group(s), let's say mlx5_cq_action_groups.
mlx5_cq_action_groups is an array of two pointers, the first is to
uverbs_common_cq_actions and the second is to mlx5_cq_actions.
>> +
>> +struct uverbs_type_group {
>> + size_t num_types;
>> + const struct uverbs_type **types;
>> +};
>> +
>> +struct uverbs_spec_root {
>> + const struct uverbs_type_group **type_groups;
>> + size_t num_groups;
>> +};
>
> Similar comments as above regarding **type_groups and **types. Can we avoid the extra indirection, and maybe drop the action_group and type_group structures completely? (If this is purely a space issue, we're only saving 1-2 pointers worth of space).
>
>
We support 4 bits of namespaces (16 namespaces). Currently, we use
only 2 (common and driver), but obviously, this could let you have
some limited support of nested attributes (could be nice for example
for the flow-steering case).
The real reason for the indirection is to let you re-use entities that
were already defined in the common code. In order to do that, that
array of groups has to define "pointers to entities" and not the
entities themselves.
That currently mean that adding features would make drivers duplicate
this branch in their tree. However, for unchanged entities (types,
actions, attributes), they could still use the common pointers.
Once this goes in, we can introduce some function which merges special
features for drivers seamlessly - without the need to replicate that
structure (again - only the tree structure is currently replicated,
the data is stored as pointers which could be just used as
&uverbs_common_xxxx).
>> +
>> +/* =================================================
>> + * Parsing infrastructure
>> + * =================================================
>> + */
>> +
>> +struct uverbs_ptr_attr {
>> + void __user *ptr;
>> + u16 len;
>> };
>>
>> struct uverbs_obj_attr {
>> + /*
>> + * pointer to the user-space given attribute, in order to write
>> the
>> + * new uobject's id.
>> + */
>> + struct ib_uverbs_attr __user *uattr;
>> + /* pointer to the kernel descriptor -> type, access, etc */
>> + const struct uverbs_obj_type *type;
>> struct ib_uobject *uobject;
>> + /* fd or id in idr of this object */
>> + int id;
>> };
>>
>> struct uverbs_attr {
>> - struct uverbs_obj_attr obj_attr;
>> + union {
>> + struct uverbs_ptr_attr ptr_attr;
>> + struct uverbs_obj_attr obj_attr;
>> + };
>> };
>>
>> struct uverbs_attr_array {
>> diff --git a/include/uapi/rdma/rdma_user_ioctl.h
>> b/include/uapi/rdma/rdma_user_ioctl.h
>> index 9388125..3a40b9d 100644
>> --- a/include/uapi/rdma/rdma_user_ioctl.h
>> +++ b/include/uapi/rdma/rdma_user_ioctl.h
>> @@ -43,6 +43,30 @@
>> /* Legacy name, for user space application which already use it */
>> #define IB_IOCTL_MAGIC RDMA_IOCTL_MAGIC
>>
>> +#define RDMA_VERBS_IOCTL \
>> + _IOWR(RDMA_IOCTL_MAGIC, 1, struct ib_uverbs_ioctl_hdr)
>> +
>> +enum ib_uverbs_attr_flags {
>> + UVERBS_ATTR_F_MANDATORY = 1U << 0,
>> +};
>
> Named enum for flags
>
Yeah
>> +
>> +struct ib_uverbs_attr {
>> + __u16 attr_id; /* command specific type attribute */
>> + __u16 len; /* only for pointers */
>> + __u16 flags; /* combination of ib_uverbs_attr_flags
>> */
>> + __u16 reserved;
>> + __u64 data; /* ptr to command, inline data or idr/fd */
>> +};
>> +
>> +struct ib_uverbs_ioctl_hdr {
>> + __u16 length;
>> + __u16 object_type;
>> + __u16 action;
>> + __u16 num_attrs;
>> + __u64 reserved;
>> + struct ib_uverbs_attr attrs[0];
>> +};
>
> --
> 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
Thanks for the review.
I hope this is clearer now. If you still think some of the terminology
should be changed, let's propose and discuss.
Regards,
Matan
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <CAAKD3BArF5U49WQ2PcYjpSbPpQmyubJQ6vKKUer_L05QBQpT0w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-06-14 17:25 ` Hefty, Sean
[not found] ` <1828884A29C6694DAF28B7E6B8A82373AB1436F6-P5GAC/sN6hkd3b2yrw5b5LfspsVTdybXVpNB7YpNyf8@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Hefty, Sean @ 2017-06-14 17:25 UTC (permalink / raw)
To: Matan Barak
Cc: Matan Barak, Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
Jason Gunthorpe, Liran Liss, Yishai Hadas, Leon Romanovsky,
Tal Alon, Christoph Lameter, Weiny, Ira, Majd Dibbiny
> Let's start from the root of the parsing tree. at the top level we
> have the root of the parsing tree ("uverbs_spec_root"). The root has
> several "type_groups", one for each "namespace".
> Currently, two "namespace"s are declared - common and driver
> ("vendor_chain" here, should be changed to "driver").
> Each "type_group" contains a few "type"s. "type" has several
> "action_group"s, one for each "namespace". Each "action_group"
> contains several "action"s.
> Each "action" contains groups of "attr_spec", one for each namespace.
> Finally, "attr_spec" contains attributes ("attrs").
> Each attribute is of one of the following classes: ptr_in, ptr_out,
> idr or fd (in the future, we might add a few more like "flags").
Again, I think the approach looks good. I can see *how* the relationships between structures are defined by looking at the struct definitions. What's not clear is *why* those relationships exist. I really do think trying to use standard object-oriented terminology helps convey the latter.
Using a commonly known programming term (e.g. class, namespace) with a different meaning makes it difficult to understand what's going on. I don't want to get too buried in terminology here, but I do think that coming up with the best terms will simplify maintenance and make it easier for people to use. I'm even fine with name changes coming as follow on patches, so the entire series doesn't need to be reworked.
If no one else cares what things are called, we can just move on. But I wonder if the lack of review of this series isn't a reflection on how much effort it takes to understand what's happening.
> > Assuming my understanding is correct (and I haven't read through the
> rest of the patches yet), then I would suggest using these terms
> instead (based on standard OO design terminology):
> >
> > type_group -> namespace (?)
>
> I currently use "namespace" term to differentiate between the groups
> of the entities. For example, in each layer of the hierarchy we
> (possibly) have a namespace of common entities and driver-specific
> entities.
>
> > type -> class or obj_class
>
> I use the term "class" for the nature of the actual attributes
> ("attrs"). Each attribute is one of the following classes: idr, fd,
> ptr_in or ptr_out.
>
> > action_group -> operations or methods or functions
> > action -> op_def or method_def or func_def
> > attr_spec_group -> param_list or op_param_list or func_param_list
> ...
>
> I used the term "group" to emphasize that these aren't just a random
> collection of params in a lit.
> Grouping this entities together has a specific meaning.
But *what* is that meaning? That's what I'd like the name to convey. I thought the grouping was because the attributes were a set of parameters being passed into a single function/operation/action/method.
> > common_chain -> common_params
> > vendor_chain -> vendor_params
>
> I agree the term "chain" is problematic. Regarding the drawing above -
> "common_params" and "driver_params" seems fine.
>
> > nit: attr_spec -> attr_def
>
> Actually, the term "spec" is used all over the code in order to
> emphasize this is a specification the kernel is using for parsing.
I'm far more indifferent to spec vs def.
> > TYPE_IDR -> TYPE_KOBJ (kernel object)
> >
>
> What about fds (like completion_channel)? They're KOBJs too.
How about KSTRUCT or IDR_OBJ? The fact that we're using an IDR seems like an internal implementation detail of the framework.
> I don't have an objection coming up with a better name-schema that
> captures the subtle details mentioned above :)
>
> > See other comments below.
> >
> >> diff --git a/include/rdma/uverbs_ioctl.h
> b/include/rdma/uverbs_ioctl.h
> >> index e06f4cf..7fed6d9 100644
> >> --- a/include/rdma/uverbs_ioctl.h
> >> +++ b/include/rdma/uverbs_ioctl.h
> >> @@ -41,8 +41,13 @@
> >> * =======================================
> >> */
> >>
> >> +#define UVERBS_ID_GROUP_MASK 0xF000
> >> +#define UVERBS_ID_GROUP_SHIFT 12
> >> +
> >> enum uverbs_attr_type {
> >> UVERBS_ATTR_TYPE_NA,
> >> + UVERBS_ATTR_TYPE_PTR_IN,
> >> + UVERBS_ATTR_TYPE_PTR_OUT,
> >> UVERBS_ATTR_TYPE_IDR,
> >
> > This is really indicating that we're dealing with a kernel object of
> some sort, not the actual indexer.
> >
>
> Yeah, but FDs could represent kernel objects too. The meaning here are
> kernel objects that resides in the idr.
> What do you propose?
>
> >> UVERBS_ATTR_TYPE_FD,
> >> };
> >> @@ -54,29 +59,106 @@ enum uverbs_obj_access {
> >> UVERBS_ACCESS_DESTROY
> >> };
> >>
> >> +enum uverbs_attr_spec_flags {
> >> + UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
> >> + /* Support extending attributes by length */
> >> + UVERBS_ATTR_SPEC_F_MIN_SZ = 1U << 1,
> >> +};
> >
> > Don't use named enums for bit flags. The result of ORing flags ends
> up as a non-enum value.
> >
>
> Sure
>
> >> +
> >> struct uverbs_attr_spec {
> >> enum uverbs_attr_type type;
> >> - struct {
> >> - /*
> >> - * higher bits mean the group and lower bits mean
> >> - * the type id within the group.
> >> - */
> >> - u16 obj_type;
> >> - u8 access;
> >> - } obj;
> >> + /* a combination of enum uverbs_attr_spec_flags */
> >> + u8 flags;
> >> + union {
> >> + u16 len;
> >> + struct {
> >> + /*
> >> + * higher bits mean the group and lower bits
> mean
> >> + * the type id within the group.
> >> + */
> >> + u16 obj_type;
> >> + u8 access;
> >> + } obj;
> >> + };
> >
> > You mentioned trying to conserve space, but this layout will have 1
> byte padding after flags and 3 bytes at the end. Swapping flags to
> the end should eliminate the padding
> >
>
> Yep, thanks
>
> >> };
> >>
> >> struct uverbs_attr_spec_group {
> >> struct uverbs_attr_spec *attrs;
> >> size_t num_attrs;
> >> + /* populate at runtime */
> >> + unsigned long *mandatory_attrs_bitmask;
> >> +};
> >
> > I'm assuming that this references all the parameters for a given
> method, so suggesting renaming to param_list.
> >
>
> This implies changing all attrs->params (to keep the terms in sync).
> Let's come with a clear schema for all entities.
>
> >> +
> >> +struct uverbs_attr_array;
> >> +struct ib_uverbs_file;
> >> +
> >> +enum uverbs_action_flags {
> >> + /*
> >> + * Action marked with this flag creates a context (or root
> for
> >> all
> >> + * objects).
> >> + */
> >> + UVERBS_ACTION_FLAG_CREATE_ROOT = 1U << 0,
> >> +};
> >
> > Same comment as above regarding named enums.
> >
>
> Sure, thanks.
>
> >> +
> >> +struct uverbs_action {
> >> + struct uverbs_attr_spec_group
> **attr_groups;
> >> + size_t num_groups;
> >> + size_t
> num_child_attrs;
> >> + /* Combination of bits from enum uverbs_action_flags */
> >> + u32 flags;
> >> + int (*handler)(struct ib_device *ib_dev, struct
> ib_uverbs_file
> >> *ufile,
> >> + struct uverbs_attr_array *ctx, size_t num);
> >> +};
> >
> > I think this is defining the function handler and parameters of a
> single method. Why is attr_groups **, rather than just *?
> >
>
> It points to an array (defined somewhere). Each entry in this array is
> a pointer to a uverbs_attr_spec_group (defined somewhere).
> For example, in the driver's code we define a modified copy of the
> create_cq operation, i.e. mlx5_create_cq_action.
> mlx5_create_cq_action points to an array (usually unnamed, but let's
> name it for the ease of discussion): mlx5_create_cq_attr_groups.
> mlx5_create_cq_attr_groups is an array of 2 - the first is a pointer
> to uverbs_common_create_cq_attrs and the second one is
> mlx5_create_cq_attrs.
>
> >> +
> >> +struct uverbs_action_group {
> >> + size_t num_actions;
> >> + struct uverbs_action **actions;
> >> +};
> >> +
> >> +struct uverbs_type {
> >> + size_t num_groups;
> >> + const struct uverbs_action_group **action_groups;
> >> + const struct uverbs_obj_type *type_attrs;
> >> +};
> >
> > Conceptually, I think this is representing an object-oriented class;
> consider renaming type to class. Similar to my question above, why is
> action_groups **, rather than *? Actually, why doesn't this just
> reference an array of uverbs_action? The extra layers of indirection
> to **actions_groups -> ** actions seems overly complex.
Thanks -- I see the reason for the extra indirection. I'll see if I can think of a way to flatten things, but I'm doubtful.
- Sean
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <1828884A29C6694DAF28B7E6B8A82373AB1436F6-P5GAC/sN6hkd3b2yrw5b5LfspsVTdybXVpNB7YpNyf8@public.gmane.org>
@ 2017-06-14 17:39 ` Jason Gunthorpe
[not found] ` <20170614173940.GA15278-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2017-06-15 11:50 ` Matan Barak
1 sibling, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2017-06-14 17:39 UTC (permalink / raw)
To: Hefty, Sean
Cc: Matan Barak, Matan Barak, Doug Ledford,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Liran Liss, Yishai Hadas,
Leon Romanovsky, Tal Alon, Christoph Lameter, Weiny, Ira,
Majd Dibbiny
On Wed, Jun 14, 2017 at 05:25:42PM +0000, Hefty, Sean wrote:
> > Let's start from the root of the parsing tree. at the top level we
> > have the root of the parsing tree ("uverbs_spec_root"). The root has
> > several "type_groups", one for each "namespace".
> > Currently, two "namespace"s are declared - common and driver
> > ("vendor_chain" here, should be changed to "driver").
> > Each "type_group" contains a few "type"s. "type" has several
> > "action_group"s, one for each "namespace". Each "action_group"
> > contains several "action"s.
> > Each "action" contains groups of "attr_spec", one for each namespace.
> > Finally, "attr_spec" contains attributes ("attrs").
> > Each attribute is of one of the following classes: ptr_in, ptr_out,
> > idr or fd (in the future, we might add a few more like "flags").
>
> Again, I think the approach looks good. I can see *how* the
> relationships between structures are defined by looking at the
> struct definitions. What's not clear is *why* those relationships
> exist. I really do think trying to use standard object-oriented
> terminology helps convey the latter.
I think it will help a lot as well.
> > > type_group -> namespace (?)
> >
> > I currently use "namespace" term to differentiate between the groups
> > of the entities. For example, in each layer of the hierarchy we
> > (possibly) have a namespace of common entities and driver-specific
> > entities.
Why do we even need this concept to have a name? At the end of the day
I thought it was just restricting what ID ranges are usable in
different parts of the code?
Just calling the numbers assigned to the driver 'driver specific ids's
woudl seem to be an improvement..
> > > type -> class or obj_class
> >
> > I use the term "class" for the nature of the actual attributes
> > ("attrs"). Each attribute is one of the following classes: idr, fd,
> > ptr_in or ptr_out.
Those would be commonly called types..
> > > action_group -> operations or methods or functions
> > > action -> op_def or method_def or func_def
> > > attr_spec_group -> param_list or op_param_list or func_param_list
> > ...
> >
> > I used the term "group" to emphasize that these aren't just a random
> > collection of params in a lit.
> > Grouping this entities together has a specific meaning.
>
> But *what* is that meaning? That's what I'd like the name to
> convey. I thought the grouping was because the attributes were a
> set of parameters being passed into a single
> function/operation/action/method.
Me too..
> > > common_chain -> common_params
> > > vendor_chain -> vendor_params
> >
> > I agree the term "chain" is problematic. Regarding the drawing above -
> > "common_params" and "driver_params" seems fine.
.. again, remove vendor from everything, these are driver specific
things..
> > > TYPE_IDR -> TYPE_KOBJ (kernel object)
> > >
> > What about fds (like completion_channel)? They're KOBJs too.
>
> How about KSTRUCT or IDR_OBJ? The fact that we're using an IDR
> seems like an internal implementation detail of the framework.
I think it is trying to say TYPE_OBJECT_ID, TYPE_FD_NUM, etc ?
Eg it informs the user what the attribute is. In our system an object
id is a 32 bit unsigned value with ~0 as the invalid, fd is a signed
integer open fd with -1 as invalid, etc
> > >> enum uverbs_attr_type {
> > >> UVERBS_ATTR_TYPE_NA,
> > >> + UVERBS_ATTR_TYPE_PTR_IN,
> > >> + UVERBS_ATTR_TYPE_PTR_OUT,
> > >> UVERBS_ATTR_TYPE_IDR,
> > >
> > > This is really indicating that we're dealing with a kernel object of
> > some sort, not the actual indexer.
> > >
> >
> > Yeah, but FDs could represent kernel objects too. The meaning here are
> > kernel objects that resides in the idr.
> > What do you propose?
UVERBS_ATTR_TYPE_OBJECT_ID ?
> > >> +enum uverbs_attr_spec_flags {
> > >> + UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
> > >> + /* Support extending attributes by length */
> > >> + UVERBS_ATTR_SPEC_F_MIN_SZ = 1U << 1,
> > >> +};
> > >
> > > Don't use named enums for bit flags. The result of ORing flags ends
> > up as a non-enum value.
> > >
> >
> > Sure
Isn't this common place in the kernel now? The enum should be
anonymous for this usage though.
There are advantages to using enums vs #define for constants..
> > >> +struct uverbs_action {
> > >> + struct uverbs_attr_spec_group **attr_groups;
Please make sure you get your consts right so stuff can live in
rodata.. I would expect this to be const??
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <20170614173940.GA15278-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
@ 2017-06-14 17:46 ` Hefty, Sean
2017-06-15 11:51 ` Matan Barak
1 sibling, 0 replies; 31+ messages in thread
From: Hefty, Sean @ 2017-06-14 17:46 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Matan Barak, Matan Barak, Doug Ledford,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Liran Liss, Yishai Hadas,
Leon Romanovsky, Tal Alon, Christoph Lameter, Weiny, Ira,
Majd Dibbiny
> > > >> +enum uverbs_attr_spec_flags {
> > > >> + UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
> > > >> + /* Support extending attributes by length */
> > > >> + UVERBS_ATTR_SPEC_F_MIN_SZ = 1U << 1,
> > > >> +};
> > > >
> > > > Don't use named enums for bit flags. The result of ORing flags
> ends
> > > up as a non-enum value.
> > > >
> > >
> > > Sure
>
> Isn't this common place in the kernel now? The enum should be
> anonymous for this usage though.
I wasn't clear. Using an anonymous enum was what I was getting at.
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <1828884A29C6694DAF28B7E6B8A82373AB1436F6-P5GAC/sN6hkd3b2yrw5b5LfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2017-06-14 17:39 ` Jason Gunthorpe
@ 2017-06-15 11:50 ` Matan Barak
[not found] ` <CAAKD3BA+Bpwbx+pZivK_+jRLc9kCrvhQkPy4tKcaTqUZhfRiOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 1 reply; 31+ messages in thread
From: Matan Barak @ 2017-06-15 11:50 UTC (permalink / raw)
To: Hefty, Sean
Cc: Matan Barak, Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
Jason Gunthorpe, Liran Liss, Yishai Hadas, Leon Romanovsky,
Tal Alon, Christoph Lameter, Weiny, Ira, Majd Dibbiny
On Wed, Jun 14, 2017 at 8:25 PM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> Let's start from the root of the parsing tree. at the top level we
>> have the root of the parsing tree ("uverbs_spec_root"). The root has
>> several "type_groups", one for each "namespace".
>> Currently, two "namespace"s are declared - common and driver
>> ("vendor_chain" here, should be changed to "driver").
>> Each "type_group" contains a few "type"s. "type" has several
>> "action_group"s, one for each "namespace". Each "action_group"
>> contains several "action"s.
>> Each "action" contains groups of "attr_spec", one for each namespace.
>> Finally, "attr_spec" contains attributes ("attrs").
>> Each attribute is of one of the following classes: ptr_in, ptr_out,
>> idr or fd (in the future, we might add a few more like "flags").
>
> Again, I think the approach looks good. I can see *how* the relationships between structures are defined by looking at the struct definitions. What's not clear is *why* those relationships exist. I really do think trying to use standard object-oriented terminology helps convey the latter.
>
> Using a commonly known programming term (e.g. class, namespace) with a different meaning makes it difficult to understand what's going on. I don't want to get too buried in terminology here, but I do think that coming up with the best terms will simplify maintenance and make it easier for people to use. I'm even fine with name changes coming as follow on patches, so the entire series doesn't need to be reworked.
>
Following object oriented approach, I can think of the following
trivial changes:
type->class
action->method (actually, these are static methods, but nm)
Since I would like to differentiate between specifications and the
actual given data, I'll add the _spec suffix.
The "groups" concept is a bit harder to grasp in OO. In our case, an
entity is actually divided into a few "groups" and its actual content
is the unification of these groups.
You could think of it as a derived class with multiple inheritance
from bases classes, where you have a base class for each "group":
class cq: public cq_common, public cq_mlx5
{
}
In contrast of the OO multiple inheritance case, we have a specific
meaning to the order of these groups - the handler actually uses them.
When the method handler is called, it gets an array of "argument
groups". Each element in this array contains another array of all the
arguments in this specific group.
For example,
common_attrs_array = &uverbs_attr_array[UVERBS_NAMESPACE_COMMON];
first_attr = common_attrs_array[0];
Maybe we could use the term "namespace" here, as what we are actually
doing here is giving the common and driver different namespaces inside
the same entity (avoid collision between the derived classes in the
multiple inheritance).
This is not the "namespace" terminology in OO, but it fits what we're
actually doing here.
What do you think on the following?
/*
* =======================================
* Verbs action specifications
* =======================================
*/
#define UVERBS_ID_NS_MASK 0xF000
#define UVERBS_ID_NS_SHIFT 12
enum uverbs_attr_type {
UVERBS_ATTR_TYPE_NA,
UVERBS_ATTR_TYPE_PTR_IN,
UVERBS_ATTR_TYPE_PTR_OUT,
UVERBS_ATTR_TYPE_OBJECT_ID,
UVERBS_ATTR_TYPE_FD_NUM,
};
enum uverbs_obj_access {
UVERBS_ACCESS_READ,
UVERBS_ACCESS_WRITE,
UVERBS_ACCESS_NEW,
UVERBS_ACCESS_DESTROY
};
struct uverbs_attr_spec {
enum uverbs_attr_type type;
<...>
};
struct uverbs_attr_spec_ns {
struct uverbs_attr_spec *attrs;
<...>
};
struct uverbs_method_spec {
struct uverbs_attr_spec_ns **attr_ns;
size_t num_ns;
<...>
};
struct uverbs_method_ns_spec {
size_t num_methods;
struct uverbs_method_spec **methods;
};
struct uverbs_class_spec {
size_t num_ns;
const struct uverbs_method_ns_spec **method_ns;
/* Attributes of objects created of this class. Contains obj_size,
destroy_order and the type_class which are either idr/fd */
const struct uverbs_obj_type *type_attrs;
};
struct uverbs_class_ns_spec {
size_t num_ns;
const struct uverbs_class_spec **classes;
};
struct uverbs_root_spec {
const struct uverbs_class_ns_spec **class_ns;
size_t num_ns;
};
> If no one else cares what things are called, we can just move on. But I wonder if the lack of review of this series isn't a reflection on how much effort it takes to understand what's happening.
>
>
I'm sure we can agree on some acceptable terminology. If it helps
other to convey the reason and usage for this design, it's even
welcomed.
>> > Assuming my understanding is correct (and I haven't read through the
>> rest of the patches yet), then I would suggest using these terms
>> instead (based on standard OO design terminology):
>> >
>> > type_group -> namespace (?)
>>
>> I currently use "namespace" term to differentiate between the groups
>> of the entities. For example, in each layer of the hierarchy we
>> (possibly) have a namespace of common entities and driver-specific
>> entities.
>>
>> > type -> class or obj_class
>>
>> I use the term "class" for the nature of the actual attributes
>> ("attrs"). Each attribute is one of the following classes: idr, fd,
>> ptr_in or ptr_out.
>>
>> > action_group -> operations or methods or functions
>> > action -> op_def or method_def or func_def
>> > attr_spec_group -> param_list or op_param_list or func_param_list
>> ...
>>
>> I used the term "group" to emphasize that these aren't just a random
>> collection of params in a lit.
>> Grouping this entities together has a specific meaning.
>
> But *what* is that meaning? That's what I'd like the name to convey. I thought the grouping was because the attributes were a set of parameters being passed into a single function/operation/action/method.
>
>
Entities were grouped together because they belong to the same
namespace. If we take the attributes for an example, once we execute
the method's handler, we get one group of attributes for the common
namespace
and another one for the driver namespace. By using namespaces, we
could introduce new arguments for either common and driver specific
without stepping on other attributes in different namespaces.
When reaching the handler, each argument has a unique meaning. Please
see the uverbs_attr_array example above.
>> > common_chain -> common_params
>> > vendor_chain -> vendor_params
>>
>> I agree the term "chain" is problematic. Regarding the drawing above -
>> "common_params" and "driver_params" seems fine.
>>
>> > nit: attr_spec -> attr_def
>>
>> Actually, the term "spec" is used all over the code in order to
>> emphasize this is a specification the kernel is using for parsing.
>
> I'm far more indifferent to spec vs def.
>
>> > TYPE_IDR -> TYPE_KOBJ (kernel object)
>> >
>>
>> What about fds (like completion_channel)? They're KOBJs too.
>
> How about KSTRUCT or IDR_OBJ? The fact that we're using an IDR seems like an internal implementation detail of the framework.
>
>
Jason suggested UVERBS_ATTR_TYPE_OBJECT_ID and UVERBS_ATTR_TYPE_FD_NUM.
I think it make sense and it captures that a fd has some special linux
(posix) mechanics.
>> I don't have an objection coming up with a better name-schema that
>> captures the subtle details mentioned above :)
>>
>> > See other comments below.
>> >
>> >> diff --git a/include/rdma/uverbs_ioctl.h
>> b/include/rdma/uverbs_ioctl.h
>> >> index e06f4cf..7fed6d9 100644
>> >> --- a/include/rdma/uverbs_ioctl.h
>> >> +++ b/include/rdma/uverbs_ioctl.h
>> >> @@ -41,8 +41,13 @@
>> >> * =======================================
>> >> */
>> >>
>> >> +#define UVERBS_ID_GROUP_MASK 0xF000
>> >> +#define UVERBS_ID_GROUP_SHIFT 12
>> >> +
>> >> enum uverbs_attr_type {
>> >> UVERBS_ATTR_TYPE_NA,
>> >> + UVERBS_ATTR_TYPE_PTR_IN,
>> >> + UVERBS_ATTR_TYPE_PTR_OUT,
>> >> UVERBS_ATTR_TYPE_IDR,
>> >
>> > This is really indicating that we're dealing with a kernel object of
>> some sort, not the actual indexer.
>> >
>>
>> Yeah, but FDs could represent kernel objects too. The meaning here are
>> kernel objects that resides in the idr.
>> What do you propose?
>>
>> >> UVERBS_ATTR_TYPE_FD,
>> >> };
>> >> @@ -54,29 +59,106 @@ enum uverbs_obj_access {
>> >> UVERBS_ACCESS_DESTROY
>> >> };
>> >>
>> >> +enum uverbs_attr_spec_flags {
>> >> + UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
>> >> + /* Support extending attributes by length */
>> >> + UVERBS_ATTR_SPEC_F_MIN_SZ = 1U << 1,
>> >> +};
>> >
>> > Don't use named enums for bit flags. The result of ORing flags ends
>> up as a non-enum value.
>> >
>>
>> Sure
>>
>> >> +
>> >> struct uverbs_attr_spec {
>> >> enum uverbs_attr_type type;
>> >> - struct {
>> >> - /*
>> >> - * higher bits mean the group and lower bits mean
>> >> - * the type id within the group.
>> >> - */
>> >> - u16 obj_type;
>> >> - u8 access;
>> >> - } obj;
>> >> + /* a combination of enum uverbs_attr_spec_flags */
>> >> + u8 flags;
>> >> + union {
>> >> + u16 len;
>> >> + struct {
>> >> + /*
>> >> + * higher bits mean the group and lower bits
>> mean
>> >> + * the type id within the group.
>> >> + */
>> >> + u16 obj_type;
>> >> + u8 access;
>> >> + } obj;
>> >> + };
>> >
>> > You mentioned trying to conserve space, but this layout will have 1
>> byte padding after flags and 3 bytes at the end. Swapping flags to
>> the end should eliminate the padding
>> >
>>
>> Yep, thanks
>>
>> >> };
>> >>
>> >> struct uverbs_attr_spec_group {
>> >> struct uverbs_attr_spec *attrs;
>> >> size_t num_attrs;
>> >> + /* populate at runtime */
>> >> + unsigned long *mandatory_attrs_bitmask;
>> >> +};
>> >
>> > I'm assuming that this references all the parameters for a given
>> method, so suggesting renaming to param_list.
>> >
>>
>> This implies changing all attrs->params (to keep the terms in sync).
>> Let's come with a clear schema for all entities.
>>
>> >> +
>> >> +struct uverbs_attr_array;
>> >> +struct ib_uverbs_file;
>> >> +
>> >> +enum uverbs_action_flags {
>> >> + /*
>> >> + * Action marked with this flag creates a context (or root
>> for
>> >> all
>> >> + * objects).
>> >> + */
>> >> + UVERBS_ACTION_FLAG_CREATE_ROOT = 1U << 0,
>> >> +};
>> >
>> > Same comment as above regarding named enums.
>> >
>>
>> Sure, thanks.
>>
>> >> +
>> >> +struct uverbs_action {
>> >> + struct uverbs_attr_spec_group
>> **attr_groups;
>> >> + size_t num_groups;
>> >> + size_t
>> num_child_attrs;
>> >> + /* Combination of bits from enum uverbs_action_flags */
>> >> + u32 flags;
>> >> + int (*handler)(struct ib_device *ib_dev, struct
>> ib_uverbs_file
>> >> *ufile,
>> >> + struct uverbs_attr_array *ctx, size_t num);
>> >> +};
>> >
>> > I think this is defining the function handler and parameters of a
>> single method. Why is attr_groups **, rather than just *?
>> >
>>
>> It points to an array (defined somewhere). Each entry in this array is
>> a pointer to a uverbs_attr_spec_group (defined somewhere).
>> For example, in the driver's code we define a modified copy of the
>> create_cq operation, i.e. mlx5_create_cq_action.
>> mlx5_create_cq_action points to an array (usually unnamed, but let's
>> name it for the ease of discussion): mlx5_create_cq_attr_groups.
>> mlx5_create_cq_attr_groups is an array of 2 - the first is a pointer
>> to uverbs_common_create_cq_attrs and the second one is
>> mlx5_create_cq_attrs.
>>
>> >> +
>> >> +struct uverbs_action_group {
>> >> + size_t num_actions;
>> >> + struct uverbs_action **actions;
>> >> +};
>> >> +
>> >> +struct uverbs_type {
>> >> + size_t num_groups;
>> >> + const struct uverbs_action_group **action_groups;
>> >> + const struct uverbs_obj_type *type_attrs;
>> >> +};
>> >
>> > Conceptually, I think this is representing an object-oriented class;
>> consider renaming type to class. Similar to my question above, why is
>> action_groups **, rather than *? Actually, why doesn't this just
>> reference an array of uverbs_action? The extra layers of indirection
>> to **actions_groups -> ** actions seems overly complex.
>
> Thanks -- I see the reason for the extra indirection. I'll see if I can think of a way to flatten things, but I'm doubtful.
>
> - Sean
Thanks for reviewing.
- Matan
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <20170614173940.GA15278-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2017-06-14 17:46 ` Hefty, Sean
@ 2017-06-15 11:51 ` Matan Barak
1 sibling, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-15 11:51 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Hefty, Sean, Matan Barak, Doug Ledford,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Liran Liss, Yishai Hadas,
Leon Romanovsky, Tal Alon, Christoph Lameter, Weiny, Ira,
Majd Dibbiny
On Wed, Jun 14, 2017 at 8:39 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Wed, Jun 14, 2017 at 05:25:42PM +0000, Hefty, Sean wrote:
>> > Let's start from the root of the parsing tree. at the top level we
>> > have the root of the parsing tree ("uverbs_spec_root"). The root has
>> > several "type_groups", one for each "namespace".
>> > Currently, two "namespace"s are declared - common and driver
>> > ("vendor_chain" here, should be changed to "driver").
>> > Each "type_group" contains a few "type"s. "type" has several
>> > "action_group"s, one for each "namespace". Each "action_group"
>> > contains several "action"s.
>> > Each "action" contains groups of "attr_spec", one for each namespace.
>> > Finally, "attr_spec" contains attributes ("attrs").
>> > Each attribute is of one of the following classes: ptr_in, ptr_out,
>> > idr or fd (in the future, we might add a few more like "flags").
>>
>> Again, I think the approach looks good. I can see *how* the
>> relationships between structures are defined by looking at the
>> struct definitions. What's not clear is *why* those relationships
>> exist. I really do think trying to use standard object-oriented
>> terminology helps convey the latter.
>
> I think it will help a lot as well.
>
Please review the proposal I sent in response to Sean's mail.
>> > > type_group -> namespace (?)
>> >
>> > I currently use "namespace" term to differentiate between the groups
>> > of the entities. For example, in each layer of the hierarchy we
>> > (possibly) have a namespace of common entities and driver-specific
>> > entities.
>
> Why do we even need this concept to have a name? At the end of the day
> I thought it was just restricting what ID ranges are usable in
> different parts of the code?
>
> Just calling the numbers assigned to the driver 'driver specific ids's
> woudl seem to be an improvement..
>
They are also used to namespace the attributes given to a handler.
For example, when a handler is executed, it's given uverbs_attr_array.
This is an array of "attribute groups", where each group correspond to
a namespace.
common_attrs_array = &uverbs_attr_array[UVERBS_NAMESPACE_COMMON];
first_attr_in_common = common_attrs_array[0];
You could find an example of usage in the create_cq/destroy_cq
handlers in this patchset.
>> > > type -> class or obj_class
>> >
>> > I use the term "class" for the nature of the actual attributes
>> > ("attrs"). Each attribute is one of the following classes: idr, fd,
>> > ptr_in or ptr_out.
>
> Those would be commonly called types..
>
Using the proposed terminology, a class contains a pointer to obj_type
(types of objects instantiated from this class).
obj_type contains an obj_type_class which is idr/fd.
>> > > action_group -> operations or methods or functions
>> > > action -> op_def or method_def or func_def
>> > > attr_spec_group -> param_list or op_param_list or func_param_list
>> > ...
>> >
>> > I used the term "group" to emphasize that these aren't just a random
>> > collection of params in a lit.
>> > Grouping this entities together has a specific meaning.
>>
>> But *what* is that meaning? That's what I'd like the name to
>> convey. I thought the grouping was because the attributes were a
>> set of parameters being passed into a single
>> function/operation/action/method.
>
> Me too..
>
Please see the response to Sean's mail :)
>> > > common_chain -> common_params
>> > > vendor_chain -> vendor_params
>> >
>> > I agree the term "chain" is problematic. Regarding the drawing above -
>> > "common_params" and "driver_params" seems fine.
>
> .. again, remove vendor from everything, these are driver specific
> things..
>
Sure
>> > > TYPE_IDR -> TYPE_KOBJ (kernel object)
>> > >
>> > What about fds (like completion_channel)? They're KOBJs too.
>>
>> How about KSTRUCT or IDR_OBJ? The fact that we're using an IDR
>> seems like an internal implementation detail of the framework.
>
> I think it is trying to say TYPE_OBJECT_ID, TYPE_FD_NUM, etc ?
>
> Eg it informs the user what the attribute is. In our system an object
> id is a 32 bit unsigned value with ~0 as the invalid, fd is a signed
> integer open fd with -1 as invalid, etc
>
Ok
>> > >> enum uverbs_attr_type {
>> > >> UVERBS_ATTR_TYPE_NA,
>> > >> + UVERBS_ATTR_TYPE_PTR_IN,
>> > >> + UVERBS_ATTR_TYPE_PTR_OUT,
>> > >> UVERBS_ATTR_TYPE_IDR,
>> > >
>> > > This is really indicating that we're dealing with a kernel object of
>> > some sort, not the actual indexer.
>> > >
>> >
>> > Yeah, but FDs could represent kernel objects too. The meaning here are
>> > kernel objects that resides in the idr.
>> > What do you propose?
>
> UVERBS_ATTR_TYPE_OBJECT_ID ?
>
Ok
>> > >> +enum uverbs_attr_spec_flags {
>> > >> + UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
>> > >> + /* Support extending attributes by length */
>> > >> + UVERBS_ATTR_SPEC_F_MIN_SZ = 1U << 1,
>> > >> +};
>> > >
>> > > Don't use named enums for bit flags. The result of ORing flags ends
>> > up as a non-enum value.
>> > >
>> >
>> > Sure
>
> Isn't this common place in the kernel now? The enum should be
> anonymous for this usage though.
>
> There are advantages to using enums vs #define for constants..
>
>> > >> +struct uverbs_action {
>> > >> + struct uverbs_attr_spec_group **attr_groups;
>
> Please make sure you get your consts right so stuff can live in
> rodata.. I would expect this to be const??
>
Yeah, I'll enhance the const correctness here.
Actually, action is the only non-const spec, as we want to initialize
num_child_attrs to speed-up the parsing.
> Jason
Thanks for the review.
- Matan
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <CAAKD3BA+Bpwbx+pZivK_+jRLc9kCrvhQkPy4tKcaTqUZhfRiOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-06-15 16:57 ` Jason Gunthorpe
[not found] ` <20170615165751.GB23773-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2017-06-15 16:57 UTC (permalink / raw)
To: Matan Barak
Cc: Hefty, Sean, Matan Barak, Doug Ledford,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Liran Liss, Yishai Hadas,
Leon Romanovsky, Tal Alon, Christoph Lameter, Weiny, Ira,
Majd Dibbiny
On Thu, Jun 15, 2017 at 02:50:17PM +0300, Matan Barak wrote:
> Following object oriented approach, I can think of the following
> trivial changes:
> type->class
> action->method (actually, these are static methods, but nm)
I don't think they are static methods.. anything that takes an object
id or a fd num is a normal method acting on that object, anything that
returns an object id/fd num is a constructor, anything that destroys
an object id is a destructor.
> The "groups" concept is a bit harder to grasp in OO. In our case, an
> entity is actually divided into a few "groups" and its actual
> content
This is similar to standard OO concept of mixins/multiple inheritance,
but you are applying the idea to function parameter lists, not classes.
> In contrast of the OO multiple inheritance case, we have a specific
> meaning to the order of these groups - the handler actually uses
> them.
That distinction seems conceptually unnecessary.. The access of the
arguments should not depend on where the argument is defined, only on
the argument ID, which is back to what I said before, why do we need
to even have a word for namepace?
static inline void *uverbs_get_attr(const uvbers_attr_bundle *args, uint32_t attr_id)
{
if (((attr_id & UVERBS_ID_NS_MASK) >> UVERBS_ID_NS_SHIFT) == 0)
return args.common_attrs[attr_id & XX];
if (((attr_id & UVERBS_ID_NS_MASK) >> UVERBS_ID_NS_SHIFT) == 1)
return args.driver_attrs[attr_id & YY];
return NULL;
}
void some_method(const uvbers_attr_bundle *args)
{
uverbs_get_attr(args, ATTR_ID_COMMON_A);
uverbs_get_attr(args, ATTR_ID_MLX5_B);
}
> struct uverbs_attr_spec {
> enum uverbs_attr_type type;
> <...>
> };
>
> struct uverbs_attr_spec_ns {
> struct uverbs_attr_spec *attrs;
> <...>
> };
I don't think we need _ns versions of any of these at all. Stop
treating ns as special, there is only attribute IDs, and the only time
the specialness of the ID layout comes into play is when you hash the
ID for quick lookup, or 'hash' the ID for attribute storage (eg
uverbs_get_attr)
*NOTHING* else should care if an ID is within the driver, common or
other reserved space.
> >> I used the term "group" to emphasize that these aren't just a random
> >> collection of params in a lit.
> >> Grouping this entities together has a specific meaning.
> >
> > But *what* is that meaning? That's what I'd like the name to
> > convey. I thought the grouping was because the attributes were a
> > set of parameters being passed into a single
> > function/operation/action/method.
>
> Entities were grouped together because they belong to the same
> namespace. If we take the attributes for an example, once we execute
> the method's handler, we get one group of attributes for the common
> namespace and another one for the driver namespace. By using
> namespaces, we could introduce new arguments for either common and
> driver specific without stepping on other attributes in different
> namespaces. When reaching the handler, each argument has a unique
> meaning. Please see the uverbs_attr_array example above.
Again, don't see why we should care. All of this micro-optimizing
should be hidden from the method author.
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <20170615165751.GB23773-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
@ 2017-06-20 12:03 ` Matan Barak
2017-06-20 15:38 ` Jason Gunthorpe
0 siblings, 1 reply; 31+ messages in thread
From: Matan Barak @ 2017-06-20 12:03 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Hefty, Sean, Matan Barak, Doug Ledford,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Liran Liss, Yishai Hadas,
Leon Romanovsky, Tal Alon, Christoph Lameter, Weiny, Ira,
Majd Dibbiny
On Thu, Jun 15, 2017 at 7:57 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Thu, Jun 15, 2017 at 02:50:17PM +0300, Matan Barak wrote:
>
>> Following object oriented approach, I can think of the following
>> trivial changes:
>> type->class
>> action->method (actually, these are static methods, but nm)
>
> I don't think they are static methods.. anything that takes an object
> id or a fd num is a normal method acting on that object, anything that
> returns an object id/fd num is a constructor, anything that destroys
> an object id is a destructor.
>
They are "static" as they don't have to get the actual objects itself.
It's true that in most cases you'll pass the object_id itself (and by
that they become methods).
>> The "groups" concept is a bit harder to grasp in OO. In our case, an
>> entity is actually divided into a few "groups" and its actual
>> content
>
> This is similar to standard OO concept of mixins/multiple inheritance,
> but you are applying the idea to function parameter lists, not classes.
>
Yep, that what I had in mind too (please see the earlier messages in
this thread).
We apply this idea both to methods and to function arguments.
>> In contrast of the OO multiple inheritance case, we have a specific
>> meaning to the order of these groups - the handler actually uses
>> them.
>
> That distinction seems conceptually unnecessary.. The access of the
> arguments should not depend on where the argument is defined, only on
> the argument ID, which is back to what I said before, why do we need
> to even have a word for namepace?
>
> static inline void *uverbs_get_attr(const uvbers_attr_bundle *args, uint32_t attr_id)
> {
> if (((attr_id & UVERBS_ID_NS_MASK) >> UVERBS_ID_NS_SHIFT) == 0)
> return args.common_attrs[attr_id & XX];
> if (((attr_id & UVERBS_ID_NS_MASK) >> UVERBS_ID_NS_SHIFT) == 1)
> return args.driver_attrs[attr_id & YY];
> return NULL;
> }
>
> void some_method(const uvbers_attr_bundle *args)
> {
> uverbs_get_attr(args, ATTR_ID_COMMON_A);
> uverbs_get_attr(args, ATTR_ID_MLX5_B);
> }
>
You could (and maybe even should) abstract the namespace (or argument
id higher bits) from the method developer.
The only exception here is that the developer still has to group
arguments according to their higher bits together and
use this group in the right place in the specification:
ADD_UVERBS_ACTION(UVERBS_CQ_CREATE,
uverbs_create_cq_handler,
&uverbs_create_cq_spec,
/* args with higher bits set to 0 */
&uverbs_uhw_compat_spec /*
args with higher bits set to 1 */),
So it's not exactly transparent to the developer. Moreover, he should
be aware that adding entries to a common specification
affects other drivers.
>> struct uverbs_attr_spec {
>> enum uverbs_attr_type type;
>> <...>
>> };
>>
>> struct uverbs_attr_spec_ns {
>> struct uverbs_attr_spec *attrs;
>> <...>
>> };
>
> I don't think we need _ns versions of any of these at all. Stop
> treating ns as special, there is only attribute IDs, and the only time
> the specialness of the ID layout comes into play is when you hash the
> ID for quick lookup, or 'hash' the ID for attribute storage (eg
> uverbs_get_attr)
>
Even if we ignore the "ns" naming, we still have to group them
together and name that somehow.
What about replacing the _ns suffix with _hash?
> *NOTHING* else should care if an ID is within the driver, common or
> other reserved space.
>
What about the specification deceleration?
Since this patch-set builds everything statically, we need somehow to
hash the entities correctly
according to their higher ids.
>> >> I used the term "group" to emphasize that these aren't just a random
>> >> collection of params in a lit.
>> >> Grouping this entities together has a specific meaning.
>> >
>> > But *what* is that meaning? That's what I'd like the name to
>> > convey. I thought the grouping was because the attributes were a
>> > set of parameters being passed into a single
>> > function/operation/action/method.
>>
>> Entities were grouped together because they belong to the same
>> namespace. If we take the attributes for an example, once we execute
>> the method's handler, we get one group of attributes for the common
>> namespace and another one for the driver namespace. By using
>> namespaces, we could introduce new arguments for either common and
>> driver specific without stepping on other attributes in different
>> namespaces. When reaching the handler, each argument has a unique
>> meaning. Please see the uverbs_attr_array example above.
>
> Again, don't see why we should care. All of this micro-optimizing
> should be hidden from the method author.
>
When a developer wants to add additional attributes he can't just add
them wherever he wants.
He needs to understand the implications. Attributes which are added as
common attribute ids are
expected to be generic enough and fit everyone. They can't collide
with attributes defined by other
drivers in this common space. As a community, we want to make sure the
common space stays
clean and generic enough.
> Jason
Thanks for reviewing.
I would really like to establish an agreeable terminology that covers
all the required cases here.
Matan
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 03/13] IB/core: Add new ioctl interface
2017-06-20 12:03 ` Matan Barak
@ 2017-06-20 15:38 ` Jason Gunthorpe
[not found] ` <20170620153855.GA29283-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2017-06-20 15:38 UTC (permalink / raw)
To: Matan Barak
Cc: Hefty, Sean, Matan Barak, Doug Ledford,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Liran Liss, Yishai Hadas,
Leon Romanovsky, Tal Alon, Christoph Lameter, Weiny, Ira,
Majd Dibbiny
On Tue, Jun 20, 2017 at 03:03:36PM +0300, Matan Barak wrote:
> What about the specification deceleration?
> Since this patch-set builds everything statically, we need somehow to
> hash the entities correctly
> according to their higher ids.
Don't try and build everything statically.
Do what I suggested in Santa Fe - have a .rodata description that is
easy for the programmer to build and understand, 'compile' that into a
fast-access version (eg a hash table, etc) that is suitable for runtime
use.
> When a developer wants to add additional attributes he can't just
> add them wherever he wants. He needs to understand the
> implications. Attributes which are added as common attribute ids are
We can look at this stuff based on the attribute ID value and where in
the code it lives, not based on the type of a .rodata structure.
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 02/13] IB/core: Add support to finalize objects in one transaction
[not found] ` <CAAKD3BCpRwiCejxuKiP07q-jGoRF1W8Ovs5aMt07uO2gAc1Qug-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-06-21 15:33 ` Dennis Dalessandro
[not found] ` <37c8c95b-eec5-af5c-8d72-eea429d84818-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Dennis Dalessandro @ 2017-06-21 15:33 UTC (permalink / raw)
To: Matan Barak, Hefty, Sean
Cc: Matan Barak, Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
Jason Gunthorpe, Liran Liss, Yishai Hadas, Leon Romanovsky,
Tal Alon, Christoph Lameter, Weiny, Ira, Majd Dibbiny
On 6/11/2017 4:16 AM, Matan Barak wrote:
> On Fri, Jun 9, 2017 at 2:51 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>>> The new ioctl based infrastructure either commits or rollbacks
>>> all objects of the command as one transaction. In order to do
>>> that, we introduce a notion of dealing with a collection of
>>> objects that are related to a specific action.
>>>
>>> This also requires adding a notion of an action and attribute.
>>> An action contains a groups of attributes, where each group
>>> contains several attributes.
>>>
>>> For example, an object could be a CQ, which has an action of
>>> CREATE_CQ.
>>> This action has multiple attributes. For example, the CQ's new handle
>>> and the comp_channel. Each layer in this hierarchy - objects, actions
>>> and attributes is split into groups. The common example for that is
>>> one group representing the common entities and another one
>>> representing the driver specific entities.
>>>
>>> When declaring these actions and attributes, we actually declare
>>> their specifications. When a command is executed, we actually
>>> allocates some space to hold auxiliary information. This auxiliary
>>> information contains meta-data about the required objects, such
>>> as pointers to their type information, pointers to the uobjects
>>> themselves (if exist), etc.
>>> The specification, along with the auxiliary information we allocated
>>> and filled is given to the finalize_objects function.
>>>
>>> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>> Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>> ---
>>> drivers/infiniband/core/rdma_core.c | 39
>>> ++++++++++++++++++++++++++++++
>>> drivers/infiniband/core/rdma_core.h | 22 ++++++++++++++++-
>>> include/rdma/uverbs_ioctl.h | 48
>>> +++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 108 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/infiniband/core/rdma_core.c
>>> b/drivers/infiniband/core/rdma_core.c
>>> index 2bd58ff..3d3cf07 100644
>>> --- a/drivers/infiniband/core/rdma_core.c
>>> +++ b/drivers/infiniband/core/rdma_core.c
>>> @@ -683,3 +683,42 @@ int uverbs_finalize_object(struct ib_uobject
>>> *uobj,
>>>
>>> return ret;
>>> }
>>> +
>>> +int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
>>> + struct uverbs_attr_spec_group ** const
>>> spec_groups,
>>> + size_t num,
>>> + bool commit)
>>> +{
>>> + unsigned int i;
>>> + int ret = 0;
>>> +
>>> + for (i = 0; i < num; i++) {
>>> + struct uverbs_attr_array *attr_group_array =
>>> &attr_array[i];
>>> + const struct uverbs_attr_spec_group *attr_spec_group =
>>> + spec_groups[i];
>>> + unsigned int j;
>>> +
>>> + for (j = 0; j < attr_group_array->num_attrs; j++) {
>>> + struct uverbs_attr *attr;
>>> + struct uverbs_attr_spec *spec;
>>> +
>>> + if (!uverbs_is_valid(attr_group_array, j))
>>> + continue;
>>> +
>>> + attr = &attr_group_array->attrs[j];
>>> + spec = &attr_spec_group->attrs[j];
>>> +
>>> + if (spec->type == UVERBS_ATTR_TYPE_IDR ||
>>> + spec->type == UVERBS_ATTR_TYPE_FD) {
>>> + int current_ret;
>>> +
>>> + current_ret = uverbs_finalize_object(attr-
>>>> obj_attr.uobject,
>>> + spec->obj.access,
>>> + commit);
>>> + if (!ret)
>>> + ret = current_ret;
>>> + }
>>> + }
>>> + }
>>> + return ret;
>>> +}
>>> diff --git a/drivers/infiniband/core/rdma_core.h
>>> b/drivers/infiniband/core/rdma_core.h
>>> index 97483d1..5cc00eb 100644
>>> --- a/drivers/infiniband/core/rdma_core.h
>>> +++ b/drivers/infiniband/core/rdma_core.h
>>> @@ -82,7 +82,8 @@
>>> * applicable.
>>> * This function could create (access == NEW), destroy (access ==
>>> DESTROY)
>>> * or unlock (access == READ || access == WRITE) objects if required.
>>> - * The action will be finalized only when uverbs_finalize_object is
>>> called.
>>> + * The action will be finalized only when uverbs_finalize_object or
>>> + * uverbs_finalize_objects are called.
>>> */
>>> struct ib_uobject *uverbs_get_uobject_from_context(const struct
>>> uverbs_obj_type *type_attrs,
>>> struct ib_ucontext *ucontext,
>>> @@ -91,5 +92,24 @@ struct ib_uobject
>>> *uverbs_get_uobject_from_context(const struct uverbs_obj_type
>>> int uverbs_finalize_object(struct ib_uobject *uobj,
>>> enum uverbs_obj_access access,
>>> bool commit);
>>> +/*
>>> + * Note that certain finalize stages could return a status:
>>> + * (a) alloc_commit could return a failure if the object is
>>> committed at the
>>> + * same time when the context is destroyed.
>>> + * (b) remove_commit could fail if the object wasn't destroyed
>>> successfully.
>>> + * Since multiple objects could be finalized in one transaction, it
>>> is very NOT
>>> + * recommended to have several finalize actions which have side
>>> effects.
>>> + * For example, it's NOT recommended to have a certain action which
>>> has both
>>> + * a commit action and a destroy action or two destroy objects in the
>>> same
>>> + * action. The rule of thumb is to have one destroy or commit action
>>> with
>>> + * multiple lookups.
>>> + * The first non zero return value of finalize_object is returned
>>> from this
>>> + * function. For example, this could happen when we couldn't destroy
>>> an
>>> + * object.
>>> + */
>>> +int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
>>> + struct uverbs_attr_spec_group ** const
>>> spec_groups,
>>> + size_t num,
>>> + bool commit);
>>>
>>> #endif /* RDMA_CORE_H */
>>> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
>>> index 4ff87ee..e06f4cf 100644
>>> --- a/include/rdma/uverbs_ioctl.h
>>> +++ b/include/rdma/uverbs_ioctl.h
>>> @@ -41,6 +41,12 @@
>>> * =======================================
>>> */
>>>
>>> +enum uverbs_attr_type {
>>> + UVERBS_ATTR_TYPE_NA,
>>> + UVERBS_ATTR_TYPE_IDR,
>>> + UVERBS_ATTR_TYPE_FD,
>>> +};
>>> +
>>> enum uverbs_obj_access {
>>> UVERBS_ACCESS_READ,
>>> UVERBS_ACCESS_WRITE,
>>> @@ -48,5 +54,47 @@ enum uverbs_obj_access {
>>> UVERBS_ACCESS_DESTROY
>>> };
>>>
>>> +struct uverbs_attr_spec {
>>> + enum uverbs_attr_type type;
>>> + struct {
>>> + /*
>>> + * higher bits mean the group and lower bits mean
>>> + * the type id within the group.
>>> + */
>>> + u16 obj_type;
>>
>> Why aren't separate fields used instead of an unspecified bit separation?
>>
>>
>
> The "spec" part isn't passed from user-space to kernel. It's intended
> to instruct the kernel parser how to parse the command.
> Since these structs are parsed automatically and built using macros, I
> was in favor of reducing the .text section size here.
My opinion is splitting would make the code nicer to read, but if it's
really only ever touched using macros I guess that's not too big of a
deal. I hardly think the .text section size here is really that big of a
concern given all the other stuff we are going to be cramming in.
-Denny
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 03/13] IB/core: Add new ioctl interface
[not found] ` <20170620153855.GA29283-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
@ 2017-06-29 16:38 ` Matan Barak
0 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-29 16:38 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Hefty, Sean, Matan Barak, Doug Ledford,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Liran Liss, Yishai Hadas,
Leon Romanovsky, Tal Alon, Christoph Lameter, Weiny, Ira,
Majd Dibbiny
On Tue, Jun 20, 2017 at 6:38 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Tue, Jun 20, 2017 at 03:03:36PM +0300, Matan Barak wrote:
>
>> What about the specification deceleration?
>> Since this patch-set builds everything statically, we need somehow to
>> hash the entities correctly
>> according to their higher ids.
>
> Don't try and build everything statically.
>
> Do what I suggested in Santa Fe - have a .rodata description that is
> easy for the programmer to build and understand, 'compile' that into a
> fast-access version (eg a hash table, etc) that is suitable for runtime
> use.
>
I started coding this. Actually, method becomes just some info + an
array of attributes at some arbitrary order.
Every attribute carries its own id. At driver initialization, we build
the compact hash.
>> When a developer wants to add additional attributes he can't just
>> add them wherever he wants. He needs to understand the
>> implications. Attributes which are added as common attribute ids are
>
> We can look at this stuff based on the attribute ID value and where in
> the code it lives, not based on the type of a .rodata structure.
>
I guess the static->dynamic mechanism already solves that :)
> Jason
Matan
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH for-next 02/13] IB/core: Add support to finalize objects in one transaction
[not found] ` <37c8c95b-eec5-af5c-8d72-eea429d84818-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2017-06-29 16:41 ` Matan Barak
0 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2017-06-29 16:41 UTC (permalink / raw)
To: Dennis Dalessandro
Cc: Hefty, Sean, Matan Barak, Doug Ledford,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe, Liran Liss,
Yishai Hadas, Leon Romanovsky, Tal Alon, Christoph Lameter,
Weiny, Ira, Majd Dibbiny
On Wed, Jun 21, 2017 at 6:33 PM, Dennis Dalessandro
<dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> On 6/11/2017 4:16 AM, Matan Barak wrote:
>>
>> On Fri, Jun 9, 2017 at 2:51 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>>>>
>>>> The new ioctl based infrastructure either commits or rollbacks
>>>> all objects of the command as one transaction. In order to do
>>>> that, we introduce a notion of dealing with a collection of
>>>> objects that are related to a specific action.
>>>>
>>>> This also requires adding a notion of an action and attribute.
>>>> An action contains a groups of attributes, where each group
>>>> contains several attributes.
>>>>
>>>> For example, an object could be a CQ, which has an action of
>>>> CREATE_CQ.
>>>> This action has multiple attributes. For example, the CQ's new handle
>>>> and the comp_channel. Each layer in this hierarchy - objects, actions
>>>> and attributes is split into groups. The common example for that is
>>>> one group representing the common entities and another one
>>>> representing the driver specific entities.
>>>>
>>>> When declaring these actions and attributes, we actually declare
>>>> their specifications. When a command is executed, we actually
>>>> allocates some space to hold auxiliary information. This auxiliary
>>>> information contains meta-data about the required objects, such
>>>> as pointers to their type information, pointers to the uobjects
>>>> themselves (if exist), etc.
>>>> The specification, along with the auxiliary information we allocated
>>>> and filled is given to the finalize_objects function.
>>>>
>>>> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>> Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>> drivers/infiniband/core/rdma_core.c | 39
>>>> ++++++++++++++++++++++++++++++
>>>> drivers/infiniband/core/rdma_core.h | 22 ++++++++++++++++-
>>>> include/rdma/uverbs_ioctl.h | 48
>>>> +++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 108 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/infiniband/core/rdma_core.c
>>>> b/drivers/infiniband/core/rdma_core.c
>>>> index 2bd58ff..3d3cf07 100644
>>>> --- a/drivers/infiniband/core/rdma_core.c
>>>> +++ b/drivers/infiniband/core/rdma_core.c
>>>> @@ -683,3 +683,42 @@ int uverbs_finalize_object(struct ib_uobject
>>>> *uobj,
>>>>
>>>> return ret;
>>>> }
>>>> +
>>>> +int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
>>>> + struct uverbs_attr_spec_group ** const
>>>> spec_groups,
>>>> + size_t num,
>>>> + bool commit)
>>>> +{
>>>> + unsigned int i;
>>>> + int ret = 0;
>>>> +
>>>> + for (i = 0; i < num; i++) {
>>>> + struct uverbs_attr_array *attr_group_array =
>>>> &attr_array[i];
>>>> + const struct uverbs_attr_spec_group *attr_spec_group =
>>>> + spec_groups[i];
>>>> + unsigned int j;
>>>> +
>>>> + for (j = 0; j < attr_group_array->num_attrs; j++) {
>>>> + struct uverbs_attr *attr;
>>>> + struct uverbs_attr_spec *spec;
>>>> +
>>>> + if (!uverbs_is_valid(attr_group_array, j))
>>>> + continue;
>>>> +
>>>> + attr = &attr_group_array->attrs[j];
>>>> + spec = &attr_spec_group->attrs[j];
>>>> +
>>>> + if (spec->type == UVERBS_ATTR_TYPE_IDR ||
>>>> + spec->type == UVERBS_ATTR_TYPE_FD) {
>>>> + int current_ret;
>>>> +
>>>> + current_ret = uverbs_finalize_object(attr-
>>>>>
>>>>> obj_attr.uobject,
>>>>
>>>> +
>>>> spec->obj.access,
>>>> +
>>>> commit);
>>>> + if (!ret)
>>>> + ret = current_ret;
>>>> + }
>>>> + }
>>>> + }
>>>> + return ret;
>>>> +}
>>>> diff --git a/drivers/infiniband/core/rdma_core.h
>>>> b/drivers/infiniband/core/rdma_core.h
>>>> index 97483d1..5cc00eb 100644
>>>> --- a/drivers/infiniband/core/rdma_core.h
>>>> +++ b/drivers/infiniband/core/rdma_core.h
>>>> @@ -82,7 +82,8 @@
>>>> * applicable.
>>>> * This function could create (access == NEW), destroy (access ==
>>>> DESTROY)
>>>> * or unlock (access == READ || access == WRITE) objects if required.
>>>> - * The action will be finalized only when uverbs_finalize_object is
>>>> called.
>>>> + * The action will be finalized only when uverbs_finalize_object or
>>>> + * uverbs_finalize_objects are called.
>>>> */
>>>> struct ib_uobject *uverbs_get_uobject_from_context(const struct
>>>> uverbs_obj_type *type_attrs,
>>>> struct ib_ucontext
>>>> *ucontext,
>>>> @@ -91,5 +92,24 @@ struct ib_uobject
>>>> *uverbs_get_uobject_from_context(const struct uverbs_obj_type
>>>> int uverbs_finalize_object(struct ib_uobject *uobj,
>>>> enum uverbs_obj_access access,
>>>> bool commit);
>>>> +/*
>>>> + * Note that certain finalize stages could return a status:
>>>> + * (a) alloc_commit could return a failure if the object is
>>>> committed at the
>>>> + * same time when the context is destroyed.
>>>> + * (b) remove_commit could fail if the object wasn't destroyed
>>>> successfully.
>>>> + * Since multiple objects could be finalized in one transaction, it
>>>> is very NOT
>>>> + * recommended to have several finalize actions which have side
>>>> effects.
>>>> + * For example, it's NOT recommended to have a certain action which
>>>> has both
>>>> + * a commit action and a destroy action or two destroy objects in the
>>>> same
>>>> + * action. The rule of thumb is to have one destroy or commit action
>>>> with
>>>> + * multiple lookups.
>>>> + * The first non zero return value of finalize_object is returned
>>>> from this
>>>> + * function. For example, this could happen when we couldn't destroy
>>>> an
>>>> + * object.
>>>> + */
>>>> +int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
>>>> + struct uverbs_attr_spec_group ** const
>>>> spec_groups,
>>>> + size_t num,
>>>> + bool commit);
>>>>
>>>> #endif /* RDMA_CORE_H */
>>>> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
>>>> index 4ff87ee..e06f4cf 100644
>>>> --- a/include/rdma/uverbs_ioctl.h
>>>> +++ b/include/rdma/uverbs_ioctl.h
>>>> @@ -41,6 +41,12 @@
>>>> * =======================================
>>>> */
>>>>
>>>> +enum uverbs_attr_type {
>>>> + UVERBS_ATTR_TYPE_NA,
>>>> + UVERBS_ATTR_TYPE_IDR,
>>>> + UVERBS_ATTR_TYPE_FD,
>>>> +};
>>>> +
>>>> enum uverbs_obj_access {
>>>> UVERBS_ACCESS_READ,
>>>> UVERBS_ACCESS_WRITE,
>>>> @@ -48,5 +54,47 @@ enum uverbs_obj_access {
>>>> UVERBS_ACCESS_DESTROY
>>>> };
>>>>
>>>> +struct uverbs_attr_spec {
>>>> + enum uverbs_attr_type type;
>>>> + struct {
>>>> + /*
>>>> + * higher bits mean the group and lower bits mean
>>>> + * the type id within the group.
>>>> + */
>>>> + u16 obj_type;
>>>
>>>
>>> Why aren't separate fields used instead of an unspecified bit separation?
>>>
>>>
>>
>> The "spec" part isn't passed from user-space to kernel. It's intended
>> to instruct the kernel parser how to parse the command.
>> Since these structs are parsed automatically and built using macros, I
>> was in favor of reducing the .text section size here.
>
>
> My opinion is splitting would make the code nicer to read, but if it's
> really only ever touched using macros I guess that's not too big of a deal.
> I hardly think the .text section size here is really that big of a concern
> given all the other stuff we are going to be cramming in.
>
Currently bunch of macros just build these stuff for the developer, so
I think it's reasonable.
We could have a simple xxxx_to_string function to print it correctly.
> -Denny
Matan
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2017-06-29 16:41 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-07 12:22 [PATCH for-next 00/13] IB/core: SG IOCTL based RDMA ABI Matan Barak
[not found] ` <1496838172-39671-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2017-06-07 12:22 ` [PATCH for-next 01/13] IB/core: Add a generic way to execute an operation on a uobject Matan Barak
2017-06-07 12:22 ` [PATCH for-next 02/13] IB/core: Add support to finalize objects in one transaction Matan Barak
[not found] ` <1496838172-39671-3-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2017-06-08 23:51 ` Hefty, Sean
[not found] ` <1828884A29C6694DAF28B7E6B8A82373AB141A91-P5GAC/sN6hkd3b2yrw5b5LfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2017-06-11 8:16 ` Matan Barak
[not found] ` <CAAKD3BCpRwiCejxuKiP07q-jGoRF1W8Ovs5aMt07uO2gAc1Qug-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-21 15:33 ` Dennis Dalessandro
[not found] ` <37c8c95b-eec5-af5c-8d72-eea429d84818-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-06-29 16:41 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 03/13] IB/core: Add new ioctl interface Matan Barak
[not found] ` <1496838172-39671-4-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2017-06-13 22:48 ` Hefty, Sean
[not found] ` <1828884A29C6694DAF28B7E6B8A82373AB143340-P5GAC/sN6hkd3b2yrw5b5LfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2017-06-14 8:39 ` Matan Barak
[not found] ` <CAAKD3BArF5U49WQ2PcYjpSbPpQmyubJQ6vKKUer_L05QBQpT0w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-14 17:25 ` Hefty, Sean
[not found] ` <1828884A29C6694DAF28B7E6B8A82373AB1436F6-P5GAC/sN6hkd3b2yrw5b5LfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2017-06-14 17:39 ` Jason Gunthorpe
[not found] ` <20170614173940.GA15278-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2017-06-14 17:46 ` Hefty, Sean
2017-06-15 11:51 ` Matan Barak
2017-06-15 11:50 ` Matan Barak
[not found] ` <CAAKD3BA+Bpwbx+pZivK_+jRLc9kCrvhQkPy4tKcaTqUZhfRiOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-15 16:57 ` Jason Gunthorpe
[not found] ` <20170615165751.GB23773-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2017-06-20 12:03 ` Matan Barak
2017-06-20 15:38 ` Jason Gunthorpe
[not found] ` <20170620153855.GA29283-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2017-06-29 16:38 ` Matan Barak
2017-06-07 12:22 ` [PATCH for-next 04/13] IB/core: Declare a type instead of declaring only type attributes Matan Barak
2017-06-07 12:22 ` [PATCH for-next 05/13] IB/core: Add DEVICE type and root types structure Matan Barak
2017-06-07 12:22 ` [PATCH for-next 06/13] IB/core: Initialize uverbs types specification Matan Barak
2017-06-07 12:22 ` [PATCH for-next 07/13] IB/core: Add macros for declaring actions and attributes Matan Barak
2017-06-07 12:22 ` [PATCH for-next 08/13] IB/core: Explicitly destroy an object while keeping uobject Matan Barak
2017-06-07 12:22 ` [PATCH for-next 09/13] IB/core: Export ioctl enum types to user-space Matan Barak
2017-06-07 12:22 ` [PATCH for-next 10/13] IB/core: Add legacy driver's user-data Matan Barak
2017-06-07 12:22 ` [PATCH for-next 11/13] IB/core: Add completion queue (cq) object actions Matan Barak
2017-06-07 12:22 ` [PATCH for-next 12/13] IB/core: Assign root to all drivers Matan Barak
2017-06-07 12:22 ` [PATCH for-next 13/13] IB/core: Expose ioctl interface through experimental Kconfig Matan Barak
2017-06-07 15:53 ` [PATCH for-next 00/13] IB/core: SG IOCTL based RDMA ABI Jason Gunthorpe
[not found] ` <20170607155323.GB30124-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2017-06-08 14:09 ` Matan Barak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.