| Message ID | 1496838172-39671-4-git-send-email-matanb@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 14, 2017 at 1:48 AM, Hefty, Sean <sean.hefty@intel.com> 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@vger.kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > >> +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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 14, 2017 at 8:25 PM, Hefty, Sean <sean.hefty@intel.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 14, 2017 at 8:39 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 15, 2017 at 7:57 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 20, 2017 at 6:38 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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@mellanox.com> Reviewed-by: Yishai Hadas <yishaih@mellanox.com> --- 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