From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dennis Dalessandro Subject: Re: [PATCH for-next 02/13] IB/core: Add support to finalize objects in one transaction Date: Wed, 21 Jun 2017 11:33:32 -0400 Message-ID: <37c8c95b-eec5-af5c-8d72-eea429d84818@intel.com> References: <1496838172-39671-1-git-send-email-matanb@mellanox.com> <1496838172-39671-3-git-send-email-matanb@mellanox.com> <1828884A29C6694DAF28B7E6B8A82373AB141A91@ORSMSX109.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Matan Barak , "Hefty, Sean" Cc: Matan Barak , Doug Ledford , "linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Jason Gunthorpe , Liran Liss , Yishai Hadas , Leon Romanovsky , Tal Alon , Christoph Lameter , "Weiny, Ira" , Majd Dibbiny List-Id: linux-rdma@vger.kernel.org On 6/11/2017 4:16 AM, Matan Barak wrote: > On Fri, Jun 9, 2017 at 2:51 AM, Hefty, Sean 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 >>> Reviewed-by: Yishai Hadas >>> --- >>> 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