On Mon, 2015-05-25 at 10:54 -0600, Jason Gunthorpe wrote: > On Wed, May 13, 2015 at 02:10:36PM +0300, Yishai Hadas wrote: > > > + struct srcu_struct disassociate_srcu; > > There is no need for rcu for this, use a rw sem.
The rcu was used becuase it's on the hot path I assume. Do we have
numbers on whether a rwsem vs. an rcu matters performance wise? If the
rcu actually helps performance, then I'm inclined to leave it, but if it
doesn't, then I'd agree that a rwsem is simpler and easier to deal with.
> > @@ -1326,6 +1327,13 @@ ssize_t ib_uverbs_create_comp_channel(struct
> > ib_uverbs_file *file,
> > return -EFAULT;
> > }
> >
> > + /* Taking ref count on uverbs_file to make sure that file won't be
> > + * freed till that event file is closed. It will enable accessing the
> > + * uverbs_device fields as part of closing the events file and making
> > + * sure that uverbs device is available by that time as well.
> > + * Note: similar is already done for the async event file.
> > + */
> > + kref_get(&file->ref);
>
> Is this a bug today? It doesn't look like it, but this stuff does look wrong.
>
> Woulnd't this would make more sense for ib_uverbs_alloc_event_file to
> unconditionally grab the kref and unconditionally release it on
> release?
>
> The existing code for this looks broken, in ib_uverbs_get_context all
> the error paths between ib_uverbs_alloc_event_file and the
> kref_get(file->ref) are wrong - the will result in fput() which will
> call ib_uverbs_event_close, which will try to do kref_put and
> ib_unregister_event_handler - which are no longer paired.
>
> [I recommend moving the kref_get and ib_register_event_handler into
> ib_uverbs_alloc_event_file, so the 'create' and 'destroy' code paths
> are clearly paired instead of being partially open coded in call
> sites]
>
> Fix all this in a seperate patch to add the needed change in kref
> semantics please.
Seconded.
> > - if (!try_module_get(dev->ib_dev->owner)) {
> > - ret = -ENODEV;
> > + mutex_lock(&dev->disassociate_mutex);
> > + if (dev->disassociated) {
> > + ret = -EIO;
> > goto err;
> > }
> >
> > - file = kmalloc(sizeof *file, GFP_KERNEL);
> > + /* In case IB device supports disassociate ucontext, there is no hard
> > + * dependency between uverbs device and its low level device.
> > + */
> > + module_dependent = !(dev->flags & UVERBS_FLAG_DISASSOCIATE);
> > +
> > + if (module_dependent) {
> > + if (!try_module_get(dev->ib_dev->owner)) {
> > + ret = -ENODEV;
> > + goto err;
>
> Again? Why I do I keep pointing this same basic thing to Mellanox
> people:
>
> If you hold a X then you hold the ref to X as well.
>
> So, if the core code is holding function pointers to module code, then
> the core code holds a module ref. When the core code null's those
> function pointers, then it can release the module ref.
Seconded.
> This might work today like this (I'm not entirely sure), but it makes
> no sense at all.
>
> I'll look more closely in a few weeks once the rwsem change is done.
--
Doug Ledford <[email protected]>
GPG KeyID: 0E572FDD
signature.asc
Description: This is a digitally signed message part
