On Thu, Jan 12, 2017 at 1:58 AM, Jason Gunthorpe
<jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Jan 11, 2017 at 12:53:49PM +0200, Matan Barak wrote:
>
>> This commit adds the necessary mechanism to support FD based objects
>> like their IDR counterparts. FD objects release need to be synchronized
>> with context release. Since FDs could outlives their context, we use
>> a kref on this lock. We initialize the lock and the kref in downstream
>> patches. This is acceptable, as we don't use this infrastructure until
>> a later point in this series.
>
> This seems unnecessarily complicated. Just hold the kref on the
> ucontext in the fd. There isn't a problem with a dummy version of that
> struct hanging around..
>
Ok, I'll split the current event file structure to a completion event
file which begins with and ib_uobject
and an asynchronous event_file.
>> +static struct ib_uobject *uverbs_get_uobject_from_fd(const struct uverbs_type_alloc_action *type_alloc,
>> + struct ib_ucontext *ucontext,
>> + enum uverbs_idr_access access,
>> + unsigned int fd)
>> +{
>> + if (access == UVERBS_ACCESS_NEW) {
>> + int _fd;
>
> Something has gone terribly wrong if you need _ to disambiguate with a
> function argument...
I'll rename it to new_fd.
>> + } else if (access == UVERBS_ACCESS_READ) {
>> + struct file *f = fget(fd);
>> + struct ib_uobject *uobject;
>> +
>> + if (!f)
>> + return ERR_PTR(-EBADF);
>> +
>> + uobject = uverbs_priv_fd_to_uobject(f->private_data);
>> + if (f->f_op != type_alloc->fd.fops ||
>> + !uobject->context) {
>
> I think the if should be split, if fops isn't correct then do not even
> call uverbs_priv_fd_to_uobject.
>
If we start file objects with ib_uobject, we no longer need this
conversion function.
>> +void uverbs_cleanup_fd(void *private_data)
>> +{
>> + struct ib_uobject *uobject = uverbs_priv_fd_to_uobject(private_data);
>> +
>> + kfree(uobject);
>> +}
>
> Woah, this is not OK at this point in the series. There is really too
> much stuff bundled into patch 7 to make proper sense of this.
>
This is a standard structure of a series - you first build up the
infrastructure and then use it
to change everything. I'm a bit worried that embedding the actual
changes with the infrastructural
changes will require massive amounts of testing to verify it's bisect-able.
> I don't like this design. Do not implicitly prepend ib_uobject to
> structures. Require the users to include the struct as is common in
> linux.
>
I'll change that. It means we'll have different structures for the
completion event and the
asynchronous event (there will be a shared part of course).
> Do not drop the kref out of ib_uobject, that should be the master
> kref, drop the kref out of ib_uverbs_event_file instead.
>
I didn't really follow, could you please clarify?
>> +static inline void *uverbs_fd_uobj_to_priv(struct ib_uobject *uobj)
>> +{
>> + return uobj + 1;
>> +}
>
> Which means stuff like this can go away..
>
Right
>> - int id; /* index into kernel idr */
>> + int id; /* index into kernel idr/fd */
>
> Why do we need to store the fd number at all? We can *never* use it in
> the kernel except as the argument to fdinstall. For that reason we
> should never store it.
>
It's only used for fdinstall.
> Can you move the fdallocate into finalize? At the very least 0 the
> value after fdinstall so people don't get the wrong idea down the
> road.
>
It can't be moved to finalize, as fdallocate could fail and we assume nothing
in the finalize stage could fail. However, I'll zero this value.
>> int order;
>> size_t obj_size;
>> free_type free_fn;
>> + struct {
>> + const struct file_operations *fops;
>> + const char *name;
>
> Maybe name should be common
>
This could be a nice enhancement when we add an ability to print the
parsing tree.
I prefer to postpone this till we actually have code that uses this name.
> This idea also seems reasonable to me in general, but again, I'd
> rather see it more completely implemented in this patch instead of
> deferring so much stuff in to the giant #7 patch.
>
I agree it could make the review easier, but the effort to make sure the code
is really bisectable will be pretty big.
> Jason
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
--
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
