> 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
��.n��������+%������w��{.n�����{���fk��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
