Re: [PATCH for-next 02/13] IB/core: Add support to finalize objects in one transaction

[Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On 6/11/2017 4:16 AM, Matan Barak wrote:
> On Fri, Jun 9, 2017 at 2:51 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>>> The new ioctl based infrastructure either commits or rollbacks
>>> all objects of the command as one transaction. In order to do
>>> that, we introduce a notion of dealing with a collection of
>>> objects that are related to a specific action.
>>>
>>> This also requires adding a notion of an action and attribute.
>>> An action contains a groups of attributes, where each group
>>> contains several attributes.
>>>
>>> For example, an object could be a CQ, which has an action of
>>> CREATE_CQ.
>>> This action has multiple attributes. For example, the CQ's new handle
>>> and the comp_channel. Each layer in this hierarchy - objects, actions
>>> and attributes is split into groups. The common example for that is
>>> one group representing the common entities and another one
>>> representing the driver specific entities.
>>>
>>> When declaring these actions and attributes, we actually declare
>>> their specifications. When a command is executed, we actually
>>> allocates some space to hold auxiliary information. This auxiliary
>>> information contains meta-data about the required objects, such
>>> as pointers to their type information, pointers to the uobjects
>>> themselves (if exist), etc.
>>> The specification, along with the auxiliary information we allocated
>>> and filled is given to the finalize_objects function.
>>>
>>> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>> Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>> ---
>>>   drivers/infiniband/core/rdma_core.c | 39
>>> ++++++++++++++++++++++++++++++
>>>   drivers/infiniband/core/rdma_core.h | 22 ++++++++++++++++-
>>>   include/rdma/uverbs_ioctl.h         | 48
>>> +++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 108 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/infiniband/core/rdma_core.c
>>> b/drivers/infiniband/core/rdma_core.c
>>> index 2bd58ff..3d3cf07 100644
>>> --- a/drivers/infiniband/core/rdma_core.c
>>> +++ b/drivers/infiniband/core/rdma_core.c
>>> @@ -683,3 +683,42 @@ int uverbs_finalize_object(struct ib_uobject
>>> *uobj,
>>>
>>>        return ret;
>>>   }
>>> +
>>> +int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
>>> +                         struct uverbs_attr_spec_group ** const
>>> spec_groups,
>>> +                         size_t num,
>>> +                         bool commit)
>>> +{
>>> +     unsigned int i;
>>> +     int ret = 0;
>>> +
>>> +     for (i = 0; i < num; i++) {
>>> +             struct uverbs_attr_array *attr_group_array =
>>> &attr_array[i];
>>> +             const struct uverbs_attr_spec_group *attr_spec_group =
>>> +                     spec_groups[i];
>>> +             unsigned int j;
>>> +
>>> +             for (j = 0; j < attr_group_array->num_attrs; j++) {
>>> +                     struct uverbs_attr *attr;
>>> +                     struct uverbs_attr_spec *spec;
>>> +
>>> +                     if (!uverbs_is_valid(attr_group_array, j))
>>> +                             continue;
>>> +
>>> +                     attr = &attr_group_array->attrs[j];
>>> +                     spec = &attr_spec_group->attrs[j];
>>> +
>>> +                     if (spec->type == UVERBS_ATTR_TYPE_IDR ||
>>> +                         spec->type == UVERBS_ATTR_TYPE_FD) {
>>> +                             int current_ret;
>>> +
>>> +                             current_ret = uverbs_finalize_object(attr-
>>>> obj_attr.uobject,
>>> +                                                                  spec->obj.access,
>>> +                                                                  commit);
>>> +                             if (!ret)
>>> +                                     ret = current_ret;
>>> +                     }
>>> +             }
>>> +     }
>>> +     return ret;
>>> +}
>>> diff --git a/drivers/infiniband/core/rdma_core.h
>>> b/drivers/infiniband/core/rdma_core.h
>>> index 97483d1..5cc00eb 100644
>>> --- a/drivers/infiniband/core/rdma_core.h
>>> +++ b/drivers/infiniband/core/rdma_core.h
>>> @@ -82,7 +82,8 @@
>>>    * applicable.
>>>    * This function could create (access == NEW), destroy (access ==
>>> DESTROY)
>>>    * or unlock (access == READ || access == WRITE) objects if required.
>>> - * The action will be finalized only when uverbs_finalize_object is
>>> called.
>>> + * The action will be finalized only when uverbs_finalize_object or
>>> + * uverbs_finalize_objects are called.
>>>    */
>>>   struct ib_uobject *uverbs_get_uobject_from_context(const struct
>>> uverbs_obj_type *type_attrs,
>>>                                                   struct ib_ucontext *ucontext,
>>> @@ -91,5 +92,24 @@ struct ib_uobject
>>> *uverbs_get_uobject_from_context(const struct uverbs_obj_type
>>>   int uverbs_finalize_object(struct ib_uobject *uobj,
>>>                           enum uverbs_obj_access access,
>>>                           bool commit);
>>> +/*
>>> + * Note that certain finalize stages could return a status:
>>> + *   (a) alloc_commit could return a failure if the object is
>>> committed at the
>>> + *       same time when the context is destroyed.
>>> + *   (b) remove_commit could fail if the object wasn't destroyed
>>> successfully.
>>> + * Since multiple objects could be finalized in one transaction, it
>>> is very NOT
>>> + * recommended to have several finalize actions which have side
>>> effects.
>>> + * For example, it's NOT recommended to have a certain action which
>>> has both
>>> + * a commit action and a destroy action or two destroy objects in the
>>> same
>>> + * action. The rule of thumb is to have one destroy or commit action
>>> with
>>> + * multiple lookups.
>>> + * The first non zero return value of finalize_object is returned
>>> from this
>>> + * function. For example, this could happen when we couldn't destroy
>>> an
>>> + * object.
>>> + */
>>> +int uverbs_finalize_objects(struct uverbs_attr_array *attr_array,
>>> +                         struct uverbs_attr_spec_group ** const
>>> spec_groups,
>>> +                         size_t num,
>>> +                         bool commit);
>>>
>>>   #endif /* RDMA_CORE_H */
>>> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
>>> index 4ff87ee..e06f4cf 100644
>>> --- a/include/rdma/uverbs_ioctl.h
>>> +++ b/include/rdma/uverbs_ioctl.h
>>> @@ -41,6 +41,12 @@
>>>    * =======================================
>>>    */
>>>
>>> +enum uverbs_attr_type {
>>> +     UVERBS_ATTR_TYPE_NA,
>>> +     UVERBS_ATTR_TYPE_IDR,
>>> +     UVERBS_ATTR_TYPE_FD,
>>> +};
>>> +
>>>   enum uverbs_obj_access {
>>>        UVERBS_ACCESS_READ,
>>>        UVERBS_ACCESS_WRITE,
>>> @@ -48,5 +54,47 @@ enum uverbs_obj_access {
>>>        UVERBS_ACCESS_DESTROY
>>>   };
>>>
>>> +struct uverbs_attr_spec {
>>> +     enum uverbs_attr_type           type;
>>> +     struct {
>>> +             /*
>>> +              * higher bits mean the group and lower bits mean
>>> +              * the type id within the group.
>>> +              */
>>> +             u16                     obj_type;
>>
>> Why aren't separate fields used instead of an unspecified bit separation?
>>
>>
> 
> The "spec" part isn't passed from user-space to kernel. It's intended
> to instruct the kernel parser how to parse the command.
> Since these structs are parsed automatically and built using macros, I
> was in favor of reducing the .text section size here.

My opinion is splitting would make the code nicer to read, but if it's 
really only ever touched using macros I guess that's not too big of a 
deal. I hardly think the .text section size here is really that big of a 
concern given all the other stuff we are going to be cramming in.

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