Re: [PATCH 3/3] kref: Remove the memory barriers

[Peter Zijlstra <peterz@infradead.org>]

On Sun, 2011-12-11 at 16:35 +0100, Peter Zijlstra wrote:
> On Sun, 2011-12-11 at 20:59 +0800, Ming Lei wrote:
> > 
> > The implicit rule about kref is that use should make sure
> > that kref can not be touched once it is released. 
> 
> The only transition for with order makes any difference what so ever is
> 1 -> 0, you just said that should be done by external means (I agree and
> have argued thusly), therefore any memory barriers outside of these
> external means are superfluous.
> 
> Thus the proposed patch is correct.

Also, this was an entirely different issue than was raised in the
original changelog. Memory barriers in general are about ordering
visibility between multiple operations done on one cpu vs them being
visible on another.

The important point being that there need to be multiple operations on
one cpu in order for them to be able to make a difference. And the whole
issue in the past few emails only had a single operation on each cpu.
Therefore memory barriers are completely irrelevant.

Ever so more because atomic_t is atomic on the variable regardless of
visibility by a second party.

Anyway, one more way to illustrate my point; there's three distinct
phases in the life cycle of a kref managed object: insert, lookup and
removal. They are as follows:

INSERT:
	obj = alloc_obj();
	init_obj(obj);
	  kref_set(&obj->ref, 1);

	LOCK
	insert(obj);
	UNLOCK

LOOKUP:
	LOCK
	obj = lookup(key);
	if (obj)
	  kref_get();
	UNLOCK

	...

	kref_put(); /* assuming obj != NULL */

REMOVAL: /* assumes obj was acquired using a preceding LOOKUP */
	LOCK
	remove(obj);
	UNLOCK

	kref_sub(&obj->ref, 2); /* lookup + insert */


Before the insert() our obj isn't visible to other CPUs doing lookup(),
therefore there can be no concurrency on the kref, after insert the
insert's UNLOCK + lookup's LOCK provide the full memory barrier
separating the last write to the object and the first read of it.

Multiple lookup()s can be concurrent with each other and the last
remove(). Concurrent lookups are completely non-interesting since they
can't ever trigger the ->0 transition.

A lookup interleaved with a removal is serialized on the lock around our
data-structure, after the removal no new lookups will succeed, and thus
no new kref_get()s will be issued and all that happens is decrements
until we hit 0.

The issue from the original changelog was that within a lookup, reads
and writes to the obj might be re-ordered with the acquisition of the
refcount. IOW. something like:

  LOCK
  obj = lookup(); /* lets assume obj != NULL */
  kref_get(&obj->ref);
  UNLOCK

  value = obj->member;

  kref_put(&obj->ref);

Now, under our memory model, the read from obj->member can both happen,
or be observed to happen before the increment from kref_get() is
processed.

I completely fail to see how that is an issue, nor when it is, why kref
should care. If you rely on that, you're doing something weird and
exotic and had better place the appropriate memory barriers yourself.