>-----Original Message----- >From: Jason Gunthorpe [mailto:jgg@xxxxxxxxxxxx] >Sent: Thursday, August 9, 2018 1:16 PM >To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx> >Cc: linux-rdma@xxxxxxxxxxxxxxx; Leon Romanovsky <leonro@xxxxxxxxxxxx>; >Guy Levi(SW) <guyle@xxxxxxxxxxxx>; Yishai Hadas <yishaih@xxxxxxxxxxxx> >Subject: Re: [PATCH 02/10] IB/uverbs: Build the specs into a radix tree at >runtime > >On Thu, Aug 09, 2018 at 03:57:16PM +0000, Ruhl, Michael J wrote: > >> >+struct uverbs_api_ioctl_method { >> >+ int (__rcu *handler)(struct ib_uverbs_file *ufile, >> >+ struct uverbs_attr_bundle *ctx); >> >+ DECLARE_BITMAP(attr_mandatory, UVERBS_API_ATTR_BKEY_LEN); >> >+ u8 driver_method:1; >> >> This is used as a bool in some places. Is there a reason for the 1 bit usage? > >Yes, it is a 'bitfield as flags' pattern - the next patch adds more >flags to this memory location. Ahh. For future use. :) >There isn't a particularly strong reason to over space optimize this, >but at the same time there isn't a strong reason not to.. > >> >@@ -1020,7 +1022,14 @@ static int ib_uverbs_create_uapi(struct >ib_device >> >*device, >> > if (IS_ERR(specs_root)) >> > return PTR_ERR(specs_root); >> > >> >+ uapi = uverbs_alloc_api(device->driver_specs, device->driver_id); >> >+ if (IS_ERR(uapi)) { >> >+ uverbs_free_spec_tree(specs_root); >> >+ return PTR_ERR(uapi); >> >+ } >> >+ >> >> So this will completely replace the specs_root? > >Yes. > >> So what was the point of patch 1/10? Is it really necessary? > >Yes, the point of patch 1/10 is to centralize the above in the core >code so it can be changed, and more critically, to allow the new api >code to tell the difference between driver provide specs and core >specs by splitting them up. > >Prior to 1/10 the core code had no good way to tell what was driver >and what was core. Now it is clear, things referenced from >driver_specs are driver, everything else is core. OK. got it. >This part of making disassociation work as we need to know exactly >what specs came from the driver so to compute driver_method. > >> >+static int uapi_merge_method(struct uverbs_api *uapi, >> >+ struct uverbs_api_object *obj_elm, u32 obj_key, >> >+ const struct uverbs_method_def *method, >> >+ bool is_driver) >> >+{ >> >+ u32 method_key = obj_key | uapi_key_ioctl_method(method->id); >> >+ struct uverbs_api_ioctl_method *method_elm; >> >+ unsigned int i; >> >+ >> >+ if (!method->attrs) >> >+ return 0; >> >+ >> >+ method_elm = uapi_add_elm(uapi, method_key, >> >sizeof(*method_elm)); >> >+ if (IS_ERR(method_elm)) { >> >+ if (method_elm != ERR_PTR(-EEXIST)) >> >+ return PTR_ERR(method_elm); >> >+ >> >+ /* >> >+ * This occurs when a driver uses >> >ADD_UVERBS_ATTRIBUTES_SIMPLE >> >+ */ >> >+ if (WARN_ON(method->handler)) >> >+ return -EINVAL; >> >+ method_elm = radix_tree_lookup(&uapi->radix, >> >method_key); >> >+ if (WARN_ON(!method_elm)) >> >+ return -EINVAL; >> >+ } else { >> >+ WARN_ON(!method->handler); >> >+ method_elm->handler = method->handler; >> >+ if (method->handler != uverbs_destroy_def_handler) >> >+ method_elm->driver_method = is_driver; >> >+ } >> >+ >> >+ for (i = 0; i != method->num_attrs; i++) { >> >+ const struct uverbs_attr_def *attr = (*method->attrs)[i]; >> >+ struct uverbs_api_attr *attr_slot; >> >+ >> >+ if (!attr) >> >+ continue; >> >+ >> >+ /* >> >+ * ENUM_IN contains the 'ids' pointer to the driver's .rodata, >> >+ * so if it is specified by a driver then it always makes this >> >+ * into a driver method. >> >+ */ >> >+ if (attr->attr.type == UVERBS_ATTR_TYPE_ENUM_IN) >> >+ method_elm->driver_method |= is_driver; >> >> Why do you | this? > >driver_method is logically the OR of the 'driverness' of everything >that contributes to the method - the handler, and all the attribute >specs. > >In this case we possibly could use assignment and still be correct, >but that would be a very obtuse way to write it.. Ok. I understand the |. >'driverness' is the answer to the question: Does this method require >any data that is located in the driver module. ie a enum ids pointer >or a handler in the driver. > >> >+static int uapi_merge_tree(struct uverbs_api *uapi, >> >+ const struct uverbs_object_tree_def *tree, >> >+ bool is_driver) >> >+{ >> >+ unsigned int i, j; >> >+ int rc; >> >+ >> >+ if (!tree->objects) >> >+ return 0; >> >+ >> >+ for (i = 0; i != tree->num_objects; i++) { >> >+ const struct uverbs_object_def *obj = (*tree->objects)[i]; >> >+ struct uverbs_api_object *obj_elm; >> >+ u32 obj_key; >> >+ >> >+ if (!obj) >> >+ continue; >> >+ >> >+ >> >> Extra line? > >Yep > >> >+ obj_key = uapi_key_obj(obj->id); >> >+ obj_elm = uapi_add_elm(uapi, obj_key, sizeof(*obj_elm)); >> >+ if (IS_ERR(obj_elm)) { >> >+ if (obj_elm != ERR_PTR(-EEXIST)) >> >+ return PTR_ERR(obj_elm); >> >+ >> >+ /* This occurs when a driver uses >> >ADD_UVERBS_METHODS */ >> >+ if (WARN_ON(obj->type_attrs)) >> >+ return -EINVAL; >> >+ obj_elm = radix_tree_lookup(&uapi->radix, obj_key); >> >+ if (WARN_ON(!obj_elm)) >> >+ return -EINVAL; >> >+ } else { >> >+ obj_elm->type_attrs = obj->type_attrs; >> >+ if (obj->type_attrs) { >> >+ obj_elm->type_class = >> >+ obj->type_attrs->type_class; >> >+ /* >> >+ * Today drivers are only permitted to use >> >+ * idr_class types. They cannot use FD types >> >+ * because we currently have no way to >> >revoke >> >+ * the fops pointer after device >> >+ * disassociation. >> >+ */ >> >+ if (WARN_ON(is_driver && >> >+ obj->type_attrs->type_class != >> >+ &uverbs_idr_class)) >> >+ return -EINVAL; >> >+ } >> >> So you are saying that drivers cannot use this: >> >> UVERBS_TYPE_ALLOC_FD() >> >> type in their objects? > >Yes. > >> This seems like a new limitation. Is this going to change? Any thoughts on >how >> to make it work? > >It is not a new limitation, it was always true, we just didn't check >for it before.. > >The problem is how to handle the fops during disassociation, as the >fops pointer points to the driver, and the driver module can become >unloaded. > >I guess it would be fine if the driver sets struct >file_operations->owner to THIS_MODULE - maybe we should just check for >that here. > >It means disassocation via module unload doesn't work if the driver >creates an FD, which is probably OK. > >This is a tricky area, I prefer to leave it blocked until we have an >example driver user to study to get the lifetime right. Ahh. Ok, all my experimentation has been with how to know when to close the FD when destroying the object. I didn't think about what happens during driver unload. If I have any thoughts I will forward them to you. >> >+/* >> >+ * Called when a driver disassociates from the ib_uverbs_device. The >> >+ * assumption is that the driver module will unload after. Replace >eveything ^^^^^^^^^ "everything" Thanks, Mike >> >+ * related to the driver with NULL as a safety measure. >> >+ */ >> >+void uverbs_disassociate_api(struct uverbs_api *uapi) >> >+{ >> >+ struct radix_tree_iter iter; >> >+ void __rcu **slot; >> >+ bool driver_method = false; >> >+ >> >+ radix_tree_for_each_slot (slot, &uapi->radix, &iter, 0) { >> >+ if (uapi_key_is_object(iter.index)) { >> >+ struct uverbs_api_object *object_elm = >> >+ rcu_dereference_protected(*slot, true); >> >+ >> >+ /* >> >+ * Some type_attrs are in the driver, module we don't >> >+ * bother to keep track of which since there should be >> >+ * no use of this. >> >+ */ >> >+ object_elm->type_attrs = NULL; >> >+ } else if (uapi_key_is_ioctl_method(iter.index)) { >> >+ struct uverbs_api_ioctl_method *method_elm = >> >+ rcu_dereference_protected(*slot, true); >> >+ >> >+ driver_method = method_elm->driver_method; >> >> So what is the purpose of setting this? >> I don't see it being used anywhere else. > >Indeed, an earlier revision was using this but I dropped that logic >off. > >Thanks, >Jason -- 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
