On 5/27/2015 7:04 PM, Doug Ledford wrote:
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.
That's correct, it was chosen from performance reasons to enable
parallel commands as part of ib_uverbs_write with minimum
synchronization overhead comparing the rwsem.
Most of its usage is for read, upon low level device removal there is
only one point when synchronize_srcu is used.
@@ -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.
OK, will add a separate patch to handle that.
- 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.
The module get/put mechanism was previously used here and wasn't
introduced by that patch.
In case the low level driver can be unloaded (e.g. rmmod mlx4_ib)
unconditionally to active uverbs clients we should prevent the module
dependency by ignoring the get/put mechanism. Hope that it clarifies the
usage here.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
