Re: [Pv-drivers] [PATCH 01/12] VMCI: context implementation.

On Tuesday, October 30, 2012 08:46:52 AM Greg KH wrote:

On Mon, Oct 29, 2012 at 09:01:40PM -0700, Dmitry Torokhov wrote:

quoted

Hi Greg,

On Mon, Oct 29, 2012 at 07:10:58PM -0700, Greg KH wrote:

quoted

On Mon, Oct 29, 2012 at 06:03:42PM -0700, George Zhang wrote:

quoted

+/*
+ * Releases the VMCI context. If this is the last reference to
+ * the context it will be deallocated. A context is created with
+ * a reference count of one, and on destroy, it is removed from
+ * the context list before its reference count is
+ * decremented. Thus, if we reach zero, we are sure that nobody
+ * else are about to increment it (they need the entry in the
+ * context list for that). This function musn't be called with a
+ * lock held.
+ */
+void vmci_ctx_release(struct vmci_ctx *context)
+{
+	ASSERT(context);
+	kref_put(&context->kref, ctx_free_ctx);
+}
+

Hm, are you _sure_ you should be calling this without a lock held?
That's usually kref-101, you MUST hold a lock when calling put,
otherwise you can race a kref_get() call, and all hell can break loose.

Because of this, some saner people (like Al Viro), have suggested that I
force the kref_put() and kref_get() calls pass in a spinlock just to
enforce this.

So, tell me what I'm missing here, and why you put the comment here
saying that it really is supposed to be called without a lock held?  How
is that safe?

Contexts are created/registered in vmci_ctx_init_ctx() and unregistered in
vmci_ctx_release_ctx() and these operations are protected by
ctx_list.lock spinlock. Context lookup (vmci_ctx_get) also uses spinlock
to traverse list of registered contexts and then grabs reference to the
[valid] context. The use of kref_put() without additional locking in
vmci_ctx_release() is fine as there is no chance of another thread
bumping count from 0 to 1.

As I didn't see all callers of this holding that spinlock, it was
confusing.  You should put this type of description somewhere so that
other reviewers don't have the same questions.

Fair enough, we'll add better comments to this code.

Thanks,
Dmitry