On Mon, Jun 24, 2019 at 07:13:14PM +0300, Yishai Hadas wrote: > > > + u32 xa_key_level1; > > > + u32 xa_key_level2; > > > + struct rcu_head rcu; > > > + u64 cookie; > > > + bool is_obj_related; > > > + struct ib_uobject *fd_uobj; > > > + void *object; /* May need direct access upon hot unplug */ > > > > This should be a 'struct file *' and have a better name. > > > > OK, will change. > > > And I'm unclear why we need to store both the ib_uobject and the > > struct file for the same thing? > > Post hot unplug/unbind the uobj can't be accessed any more to reach the > object as it will be set to NULL by ib_core layer [1]. struct file users need to get the uobject from the file->private_data under a fget. There is only place place that needed fd_uobj, and it was under the fget section, so it should simply use private_data. This is why you should only store the struct file and not the uobject. > This was the comment that I have just put above in the code, I may improve > it with more details as pointed here. > > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/core/rdma_core.c#L149 I'm wondering if this is a bug to do this for fds? > > Since uobj->object == flip && filp->private_data == uobj, I have a > > hard time to understand why we need both things, it seems to me that > > if we get the fget on the filp then we can rely on the > > filp->private_data to get back to the devx_async_event_file. > > > > The idea was to not take an extra ref count on the file (i.e. fget) per > subscription, this will let the release option to be called once the file > will be closed by the application. No extra ref is needed, the fget is already obtained in the only place that needs fd_uobj. > > > + obj_event = xa_load(&event->object_ids, key_level2); > > > + if (!obj_event) { > > > + err = xa_reserve(&event->object_ids, key_level2, GFP_KERNEL); > > > + if (err) > > > + goto err_level1; > > > + > > > + obj_event = kzalloc(sizeof(*obj_event), GFP_KERNEL); > > > + if (!obj_event) { > > > + err = -ENOMEM; > > > + goto err_level2; > > > + } > > > + > > > + INIT_LIST_HEAD(&obj_event->obj_sub_list); > > > + *alloc_obj_event = obj_event; > > > > This is goofy, just store the empty obj_event in the xa instead of > > using xa_reserve, and when you go to do the error unwind just delete > > any level2' devx_obj_event' that has a list_empty(obj_sub_list), get > > rid of the wonky alloc_obj_event stuff. > > > > Please see my answer above about how level2 is managed by this > alloc_obj_event, is that really worth a change ? I found current logic to be > clear. I may put some note here if we can stay with that. I think it is alot cleaner/simpler than using this extra memory > > The best configuration would be to use devx_cleanup_subscription to > > undo the partially ready subscription. > > This partially ready subscription might not match the > devx_cleanup_subscription(), e.g. it wasn't added to xa_list and can't be > deleted without any specific flag to ignore .. Maybe, but I suspect it can work out > > > + event_sub_arr = uverbs_zalloc(attrs, > > > + MAX_NUM_EVENTS * sizeof(struct devx_event_subscription *)); > > > + event_obj_array_alloc = uverbs_zalloc(attrs, > > > + MAX_NUM_EVENTS * sizeof(struct devx_obj_event *)); > > > > There are so many list_heads in the devx_event_subscription, why not > > use just one of them to store the allocated events instead of this > > temp array? ie event_list looks good for this purpose. > > > > I'm using the array later on with direct access to the index that should be > de-allocated. I would prefer staying with this array rather than using the > 'event_list' which has other purpose down the road, it's used per > subscription and doesn't look match to hold the devx_obj_event which has no > list entry for this purpose.. Replace the event_obj_array_alloc by storing that data directly in the xarray Replace the event_sub_arr by building them into a linked list - it always need to iterate over the whole list anyhow. > > > + > > > + if (!event_sub_arr || !event_obj_array_alloc) > > > + return -ENOMEM; > > > + > > > + /* Protect from concurrent subscriptions to same XA entries to allow > > > + * both to succeed > > > + */ > > > + mutex_lock(&devx_event_table->event_xa_lock); > > > + for (i = 0; i < num_events; i++) { > > > + u32 key_level1; > > > + > > > + if (obj) > > > + obj_type = get_dec_obj_type(obj, > > > + event_type_num_list[i]); > > > + key_level1 = event_type_num_list[i] | obj_type << 16; > > > + > > > + err = subscribe_event_xa_alloc(devx_event_table, > > > + key_level1, > > > + obj ? true : false, > > > + obj_id, > > > + &event_obj_array_alloc[i]); > > > > Usless ?: > > What do you suggest instead ? Nothing is needed, cast to implicit bool is always forced to true/false Jason
