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

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

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.

I believe the comment should actually read that the function should not
be called from atomic contexts.

That might be nice to document, but could it ever happen?

thanks,

greg k-h