On Wed, Jun 14, 2017 at 8:39 PM, Jason Gunthorpe
<jgunthorpe@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
