kref: inline and barriers

From time to time people on #kernelnewbies ask what the difference is between

These are tiny functions, there's no point in having them out-of-line.

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Peter Zijlstra <[email protected]>

---
 include/linux/kref.h |   79 ++++++++++++++++++++++++++++++++++++++--- lib/Makefile         |    2 -
 lib/kref.c           |   97 ---------------------------------------------------

 3 files changed, 75 insertions(+), 103 deletions(-)

Index: linux-2.6/include/linux/kref.h
===================================================================

include/linux/kref.h +28 -5

@@ -21,10 +21,79 @@ struct kref {
        atomic_t refcount;
  };
 
-void kref_init(struct kref *kref);
-void kref_get(struct kref *kref);
-int kref_put(struct kref *kref, void (*release) (struct kref *kref));
-int kref_sub(struct kref *kref, unsigned int count,
-            void (*release) (struct kref *kref));
+/**
+ * kref_init - initialize object.
+ * @kref: object in question.
+ */
+static inline void kref_init(struct kref *kref)
+{
+       atomic_set(&kref->refcount, 1);
+       smp_mb();
+}
 
+/**
+ * kref_get - increment refcount for object.
+ * @kref: object.
+ */
+static inline void kref_get(struct kref *kref)
+{
+       WARN_ON(!atomic_read(&kref->refcount));
+       atomic_inc(&kref->refcount);
+       smp_mb__after_atomic_inc();
+}
+
+/**
+ * kref_put - decrement refcount for object.
+ * @kref: object.
+ * @release: pointer to the function that will clean up the object when the
+ *          last reference to the object is released.
+ *          This pointer is required, and it is not acceptable to pass kfree
+ *          in as this function.
+ *

include/linux/kref.h +36 -0

+
+       if (atomic_dec_and_test(&kref->refcount)) {
+               release(kref);
+               return 1;
+       }
+       return 0;
+}
+
+
+/**
+ * kref_sub - subtract a number of refcounts for object.
+ * @kref: object.
+ * @count: Number of recounts to subtract.
+ * @release: pointer to the function that will clean up the object when the
+ *          last reference to the object is released.
+ *          This pointer is required, and it is not acceptable to pass kfree
+ *          in as this function.
+ *
+ * Subtract @count from the refcount, and if 0, call release().
+ * Return 1 if the object was removed, otherwise return 0.  Beware, if this
+ * function returns 0, you still can not count on the kref from remaining in
+ * memory.  Only use the return value if you want to see if the kref is now
+ * gone, not present.
+ */
+static inline int kref_sub(struct kref *kref, unsigned int count,
+            void (*release)(struct kref *kref))
+{
+       WARN_ON(release == NULL);
+       WARN_ON(release == (void (*)(struct kref *))kfree);
+
+       if (atomic_sub_and_test((int) count, &kref->refcount)) {
+               release(kref);
+               return 1;
+       }
+       return 0;
+}
  #endif /* _KREF_H_ */

Index: linux-2.6/lib/kref.c
===================================================================

lib/kref.c deleted +0 -97

@@ -1,97 +0,0 @@
-/*
- * kref.c - library routines for handling generic reference counted objects
- *
- * Copyright (C) 2004 Greg Kroah-Hartman <[email protected]>
- * Copyright (C) 2004 IBM Corp.
- *
- * based on lib/kobject.c which was:
- * Copyright (C) 2002-2003 Patrick Mochel <[email protected]>
- *
- * This file is released under the GPLv2.
- *
- */
-
-#include <linux/kref.h>
-#include <linux/module.h>
-#include <linux/slab.h>
-
-/**
- * kref_init - initialize object.
- * @kref: object in question.
- */
-void kref_init(struct kref *kref)
-{
-       atomic_set(&kref->refcount, 1);
-       smp_mb();
-}
-
-/**
- * kref_get - increment refcount for object.
- * @kref: object.
- */
-void kref_get(struct kref *kref)
-{
-       WARN_ON(!atomic_read(&kref->refcount));
-       atomic_inc(&kref->refcount);
-       smp_mb__after_atomic_inc();
-}
-
-/**
- * kref_put - decrement refcount for object.
- * @kref: object.
- * @release: pointer to the function that will clean up the object when the
- *          last reference to the object is released.
- *          This pointer is required, and it is not acceptable to pass kfree
- *          in as this function.
- *
- * Decrement the refcount, and if 0, call release().
- * Return 1 if the object was removed, otherwise return 0.  Beware, if this
- * function returns 0, you still can not count on the kref from remaining in
- * memory.  Only use the return value if you want to see if the kref is now
- * gone, not present.
- */
-int kref_put(struct kref *kref, void (*release)(struct kref *kref))
-{
-       WARN_ON(release == NULL);
-       WARN_ON(release == (void (*)(struct kref *))kfree);
-
-       if (atomic_dec_and_test(&kref->refcount)) {
-               release(kref);
-               return 1;
-       }
-       return 0;
-}
-
-
-/**
- * kref_sub - subtract a number of refcounts for object.
- * @kref: object.
- * @count: Number of recounts to subtract.
- * @release: pointer to the function that will clean up the object when the
- *          last reference to the object is released.
- *          This pointer is required, and it is not acceptable to pass kfree
- *          in as this function.
- *
- * Subtract @count from the refcount, and if 0, call release().
- * Return 1 if the object was removed, otherwise return 0.  Beware, if this
- * function returns 0, you still can not count on the kref from remaining in
- * memory.  Only use the return value if you want to see if the kref is now
- * gone, not present.
- */
-int kref_sub(struct kref *kref, unsigned int count,
-            void (*release)(struct kref *kref))
-{
-       WARN_ON(release == NULL);
-       WARN_ON(release == (void (*)(struct kref *))kfree);
-
-       if (atomic_sub_and_test((int) count, &kref->refcount)) {
-               release(kref);
-               return 1;
-       }
-       return 0;
-}
-
-EXPORT_SYMBOL(kref_init);
-EXPORT_SYMBOL(kref_get);
-EXPORT_SYMBOL(kref_put);
-EXPORT_SYMBOL(kref_sub);

Index: linux-2.6/lib/Makefile
===================================================================

lib/Makefile +0 -0

@@ -17,7 +17,7 @@ lib-y := ctype.o string.o vsprintf.o cmd

 lib-$(CONFIG_MMU) += ioremap.o
 lib-$(CONFIG_SMP) += cpumask.o

lib/Makefile +1 -1

-lib-y  += kobject.o kref.o klist.o
+lib-y  += kobject.o klist.o

 obj-y += bcd.o div64.o sort.o parser.o halfmd4.o debug_locks.o random32.o \
        bust_spinlocks.o hexdump.o kasprintf.o bitmap.o scatterlist.o \

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Looks like no much text size increase in built vmlinux for ARM.

[[email protected]]$size vmlinux
text data bss dec hex filename
6030247 2083292 7912356 16025895 f48927 vmlinux

[[email protected]]$size vmlinux-before-kref-inline
text data bss dec hex filename
6023264 2083300 7912356 16018920 f46de8 vmlinux-before-kref-inline

These are tiny functions, there's no point in having them out-of-line.

Nice, but this breaks the build:

Quoted 1 line · Greg KH

Nice, but this breaks the build:

Sorry about that, this one includes the missing include and just booted on my test-box.

---
Subject: kref: Inline all functions
From: Peter Zijlstra <[email protected]>
Date: Sat Dec 10 11:29:57 CET 2011

These are tiny functions, there's no point in having them out-of-line.

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

---
 include/linux/kref.h |   80 +++++++++++++++++++++++++++++++++++++++---
 lib/Makefile         |    2 -
 lib/kref.c           |   97 ---------------------------------------------------
 3 files changed, 76 insertions(+), 103 deletions(-)

Index: linux-2.6/include/linux/kref.h
===================================================================

Quoted 7 lines

---
 include/linux/kref.h |   80 +++++++++++++++++++++++++++++++++++++++---
 lib/Makefile         |    2 -
 lib/kref.c           |   97 ---------------------------------------------------
 3 files changed, 76 insertions(+), 103 deletions(-)

Index: linux-2.6/include/linux/kref.h
===================================================================

What's this stuff for, we aren't using cvs anymore :)

Quoted 8 lines · Greg KH

Quoted 7 lines

---
 include/linux/kref.h |   80 +++++++++++++++++++++++++++++++++++++++---
 lib/Makefile         |    2 -
 lib/kref.c           |   97 ---------------------------------------------------
 3 files changed, 76 insertions(+), 103 deletions(-)

Index: linux-2.6/include/linux/kref.h
===================================================================

Quoted 1 line

What's this stuff for, we aren't using cvs anymore :)

Dunno, it looks like quilt generates that for me.

QUILT_REFRESH_ARGS="--diffstat --strip-trailing-whitespace --no-timestamps --no-index --sort -p1 -p ab" QUILT_DIFF_ARGS="--no-timestamps --no-index --sort --color=auto -p ab"

Commit 1b0b3b9980e ("kref: fix CPU ordering with respect to krefs") wrongly adds memory barriers to kref.

It states:

 some atomic operations are only atomic, not ordered. Thus a CPU is allowed to reorder memory references to an object to before the reference is obtained. This fixes it.

While true, it fails to show why this is a problem. I say it is not a

problem because if there is a race with kref_put() such that we could end up referencing a free'd object without this memory barrier, we would still have that race with the memory barrier.

The kref_put() in question could complete (and free the object) before the atomic_inc() and we'd still be up shit creek.

The kref_init() case is even worse, if your object is published at this time you're so wrong the memory barrier won't make a difference what so ever. If its not published, the act of publishing should include the needed barriers/locks to make sure all writes prior to the act of publishing are complete such that others will only observe a complete object.

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc Oliver Neukum <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---
 include/linux/kref.h |    2 --

 1 file changed, 2 deletions(-)

Index: linux-2.6/include/linux/kref.h
===================================================================

include/linux/kref.h +0 -3

@@ -28,7 +28,6 @@ struct kref {
  static inline void kref_init(struct kref *kref)
  {
        atomic_set(&kref->refcount, 1);
-       smp_mb();
  }
 
  /**

@@ -39,7 +38,6 @@ static inline void kref_get(struct kref
  {
        WARN_ON(!atomic_read(&kref->refcount));
        atomic_inc(&kref->refcount);
-       smp_mb__after_atomic_inc();
  }
 
  /**
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 1 line

While true, it fails to show why this is a problem. I say it is not a

IMO, the added two barriers are pairs of the implicit barrier in kref_put, so that we can order between kref_init/kref_get and kref_put.

Quoted 4 lines · Ming Lei

Quoted 1 line

While true, it fails to show why this is a problem. I say it is not a

IMO, the added two barriers are pairs of the implicit barrier in kref_put, so that we can order between kref_init/kref_get and kref_put.

Yeah so?

If there's a destruction race with kref_put() the barrier won't

solve it. Other than that the actual order of get/put is irrelevant.

CPU0 CPU1

atomic_set(v)
smp_mb()

				smp_mb()
				atomic_dec_and_test(v)

Without the barrier after atomic_set, CPU1 may see a stale value of v first, then decrease it, so may miss a release operation.

The pair of smp_mb can make order between atomic_set and atomic_dec_and_test, can't it?

Quoted 1 line

If there's a destruction race with kref_put() the barrier won't

Sorry, could you say what the destruction race is?

Quoted 7 lines · Ming Lei

CPU0                  CPU1

atomic_set(v)
smp_mb()
                              smp_mb() atomic_dec_and_test(v)

Quoted 2 lines

Without the barrier after atomic_set, CPU1 may see a stale value of v first, then decrease it, so may miss a release operation.

Your example is doubly broken. If there's concurrency possible with atomic_set() you've lost.

Lets change it to kref_get() aka atomic_inc():

       CPU0            CPU1

       atomic_inc()
                       atomic_dec_and_test()

and

                       atomic_dec_and_test()
       atomic_inc()

For if the first is possible, then so is the second.

This illustrates that no matter how many barriers you put in, you're still up shit creek without no paddle because the kref_put() can come in before you do the kref_get(), making the kref_get() the invalid operation.

Quoted 2 lines

The pair of smp_mb can make order between atomic_set and atomic_dec_and_test, can't it?

No. Because there's nothing stopping the dec from happening before the set/inc.

Quoted 3 lines

Quoted 1 line

If there's a destruction race with kref_put() the barrier won't

Sorry, could you say what the destruction race is?

The race to 0-refs, iow. the case above when you assume both cases start out with 1 ref.

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · Ming Lei

CPU0 CPU1

atomic_set(v)
smp_mb()

                              smp_mb()
                              atomic_dec_and_test(v)

Quoted 5 lines

Quoted 2 lines

Without the barrier after atomic_set, CPU1 may see a stale value of v first, then decrease it, so may miss a release operation.

Your example is doubly broken. If there's concurrency possible with atomic_set() you've lost.

kref_init is guaranteed to be run only one time __before__ executing kref_get/kref_put.

Quoted 7 lines

Lets change it to kref_get() aka atomic_inc():

CPU0 CPU1

atomic_inc()
atomic_dec_and_test()

Quoted 6 lines

and

atomic_dec_and_test()
atomic_inc()

For if the first is possible, then so is the second.

Yes, both are reasonable.

Quoted 1 line

Quoted 4 lines

This illustrates that no matter how many barriers you put in, you're still up shit creek without no paddle because the kref_put() can come in before you do the kref_get(), making the kref_get() the invalid operation.

So one smp_mb__before_atomic_inc should be added before atomic_inc to make sure that CPU0 can see the uptodate ref, right?

But the initial value of kref is 1, so seems we don't need to consider the 0-refs.

Quoted 10 lines · Ming Lei

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · Ming Lei

CPU0                  CPU1

atomic_set(v)
smp_mb()
                              smp_mb() atomic_dec_and_test(v)

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Without the barrier after atomic_set, CPU1 may see a stale value of v first, then decrease it, so may miss a release operation.

Your example is doubly broken. If there's concurrency possible with atomic_set() you've lost.

kref_init is guaranteed to be run only one time __before__ executing kref_get/kref_put.

If used properly, yes. But in that case you still don't need the barrier. Whatever means you use to make the object visible to other CPUs will include a barrier.

Quoted 17 lines

Quoted 7 lines

Lets change it to kref_get() aka atomic_inc():

       CPU0            CPU1

       atomic_inc()
                       atomic_dec_and_test()

Quoted 6 lines

and

                       atomic_dec_and_test()
       atomic_inc()

For if the first is possible, then so is the second.

Yes, both are reasonable.

Quoted 1 line

Quoted 7 lines

Quoted 4 lines

This illustrates that no matter how many barriers you put in, you're still up shit creek without no paddle because the kref_put() can come in before you do the kref_get(), making the kref_get() the invalid operation.

So one smp_mb__before_atomic_inc should be added before atomic_inc to make sure that CPU0 can see the uptodate ref, right?

No.

Assume v == 1:

       CPU0            CPU1

                       atomic_dec_and_test(); /* --v == 0 */ kfree()

       smp_mb__before_atomic_inc()
       atomic_inc(); <-- OOPS!

You still got an access to already freed memory. There is no amount of memory barriers that will solve this problem.

The implicit rule about kref is that use should make sure that kref can not be touched once it is released.

Quoted 3 lines · Ming Lei

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.

Quoted 10 lines · Peter Zijlstra

Quoted 3 lines · Ming Lei

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.

Quoted 1 line

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.

For kref, maybe it is still multiple operations done on one cpu vs them being visible on another, but seems a bit implicit, see the common kref usage below:

CPU0 CPU1

                                        A:kref_init(&obj->ref)

B:kref_get(&obj->ref)
C:access obj E:access obj

                                        F:kref_put(&ojb->ref)

D:kref_put(obj->ref)

Quoted 6 lines · Ming Lei

For kref, maybe it is still multiple operations done on one cpu vs them being visible on another, but seems a bit implicit, see the common kref usage below:

CPU0                            CPU1 A:kref_init(&obj->ref)

how does CPU0 get a ref to obj?

Quoted 5 lines

B:kref_get(&obj->ref)
C:access obj                    E:access obj F:kref_put(&ojb->ref) D:kref_put(obj->ref)

Quoted 5 lines · Peter Zijlstra

Quoted 4 lines · Ming Lei

For kref, maybe it is still multiple operations done on one cpu vs them being visible on another, but seems a bit implicit, see the common kref usage below:

Quoted 4 lines

Quoted 2 lines

CPU0 CPU1

                                        A:kref_init(&obj->ref)

how does CPU0 get a ref to obj?

Suppose open/close/read/.. context is run on CPU0, and driver .probe/.release context(hotplug context) is run on CPU1. There are a few examples on usb driver(eg. usb-skeleton.c, ...)

I don't know the driver model, and I don't plan to start learning it now. But if what you said is possible its broken and no memory barriers will fix it.

Quoted 3 lines · Peter Zijlstra

I don't know the driver model, and I don't plan to start learning it now. But if what you said is possible its broken and no memory barriers will fix it.

IMO, you don't need to learn it, and my example is very simple and common kref usage in device drivers, :-)

One needs to know the callchains and device driver core code leading up to those .open, .close, .read, .probe and .release callbacks. That's a lot of code.

Also, non of that is relevant. The kref interface is very simple. kref_set cannot and should not have any concurrency.

It it not for kref_set() to finalize the object (there is no requirement kref_set is the last operation before you publish the object).

As per Oliver's reply, your USB example uses usb_register_dev() to pushlish the object and that includes the LOCK and UNLOCK from my example; it also illustrates you do need to know the entire device model in order to talk sensibly about these callbacks you mentioned.

You already said that external means should be used to ensure no kref_get() is issued after the final kref_put(). This too is only

possible if you use locks one way or another and I bet that if you go look at the device model you'll find plenty around device_unregister().

These two constraints alone are enough to remove both memory barriers from the kref code.

Quoted 3 lines · Peter Zijlstra

I don't know the driver model, and I don't plan to start learning it now. But if what you said is possible its broken and no memory barriers will fix it.

IMO, you don't need to learn it, and my example is very simple and common kref usage in device drivers, :-)

Could we only focus on it and see what is problem? and why won't memory barrier fix it?

Anyway, device drivers are the most consumers of kref...

Quoted 4 lines

I don't know the driver model, and I don't plan to start learning it now. But if what you said is possible its broken and no memory barriers will fix it.

IMO, you don't need to learn it, and my example is very simple and common kref usage in device drivers, :-)

Could we only focus on it and see what is problem? and why won't memory barrier fix it?

Quoted 5 lines · Ming Lei

Quoted 4 lines

For kref, maybe it is still multiple operations done on one cpu vs them being visible on another, but seems a bit implicit, see the common kref usage below:

Quoted 4 lines

Quoted 2 lines

CPU0 CPU1

                                        A:kref_init(&obj->ref)

how does CPU0 get a ref to obj?

Suppose open/close/read/.. context is run on CPU0, and driver .probe/.release context(hotplug context) is run on CPU1. There are a few examples on usb driver(eg. usb-skeleton.c, ...)

Am Montag, 12. Dezember 2011, 10:57:31 schrieb Ming Lei:

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Ming Lei

Quoted 4 lines

For kref, maybe it is still multiple operations done on one cpu vs them being visible on another, but seems a bit implicit, see the common kref usage below:

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

CPU0 CPU1

                                        A:kref_init(&obj->ref)

how does CPU0 get a ref to obj?

Suppose open/close/read/.. context is run on CPU0, and driver .probe/.release context(hotplug context) is run on CPU1. There are a few examples on usb driver(eg. usb-skeleton.c, ...)

USB generally relies on an implied barrier just as:

        /* we can register the device now, as it is ready */
        retval = usb_register_dev(interface, &skel_class);

Generally reference counting cannot help you if kfree() is involved

Quoted 18 lines

Am Montag, 12. Dezember 2011, 10:57:31 schrieb Ming Lei:

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Ming Lei

Quoted 4 lines

For kref, maybe it is still multiple operations done on one cpu vs them being visible on another, but seems a bit implicit, see the common kref usage below:

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

CPU0 CPU1

                                        A:kref_init(&obj->ref)

how does CPU0 get a ref to obj?

Suppose open/close/read/.. context is run on CPU0, and driver .probe/.release context(hotplug context) is run on CPU1. There are a few examples on usb driver(eg. usb-skeleton.c, ...)

USB generally relies on an implied barrier just as:

Quoted 4 lines

        /* we can register the device now, as it is ready */
        retval = usb_register_dev(interface, &skel_class);

Generally reference counting cannot help you if kfree() is involved

So, Oliver, you don't have any objection to this patch removing the barriers in kref, right? Originally you added them, I just wanted to make sure before I applied this.

Am Montag, 12. Dezember 2011, 20:30:40 schrieb Greg KH:

Quoted 19 lines · Oliver Neukum

Quoted 18 lines

Am Montag, 12. Dezember 2011, 10:57:31 schrieb Ming Lei:

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Ming Lei

Quoted 4 lines

For kref, maybe it is still multiple operations done on one cpu vs them being visible on another, but seems a bit implicit, see the common kref usage below:

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

CPU0 CPU1

                                        A:kref_init(&obj->ref)

how does CPU0 get a ref to obj?

Suppose open/close/read/.. context is run on CPU0, and driver .probe/.release context(hotplug context) is run on CPU1. There are a few examples on usb driver(eg. usb-skeleton.c, ...)

USB generally relies on an implied barrier just as:

Quoted 8 lines

Quoted 4 lines

        /* we can register the device now, as it is ready */
        retval = usb_register_dev(interface, &skel_class);

Generally reference counting cannot help you if kfree() is involved

So, Oliver, you don't have any objection to this patch removing the barriers in kref, right? Originally you added them, I just wanted to make sure before I applied this.

I do not remember any more why I introduced this.

I guess I worried not about the increment, but the decrement. Which makes me wonder what happens if you don't intend to get the kref again, but need to make sure it is usually freed, like:

CPU A CPU B

kref_get(p)
start_io(p)

									[interrupt from IO]
									kref_put(p)

You need an ordering primitive between start_io() and kref_get() or the counter could go negative.

I think I was worried about it missing.

Quoted 12 lines

I guess I worried not about the increment, but the decrement. Which makes me wonder what happens if you don't intend to get the kref again, but need to make sure it is usually freed, like:

CPU A CPU B

kref_get(p)
start_io(p)
[interrupt from IO] kref_put(p)

Quoted 2 lines

You need an ordering primitive between start_io() and kref_get() or the counter could go negative.

Really? On an atomic variable? I didn't think this was needed for atomics to ensure this type of thing couldn't happen.

I guess I worried not about the increment, but the decrement. Which makes me wonder what happens if you don't intend to get the kref again, but need to make sure it is usually freed, like:

CPU A CPU B

kref_get(p)
start_io(p)
[interrupt from IO] kref_put(p)

Quoted 11 lines

I guess I worried not about the increment, but the decrement. Which makes me wonder what happens if you don't intend to get the kref again, but need to make sure it is usually freed, like:

CPU A CPU B

kref_get(p)
start_io(p)
[interrupt from IO] kref_put(p)

I would expect that if something was needed here, the io stack would provide the barriers since the io completion will probably want to change state set by the start_io thing.

Anyway, I would put this squarely outside the responsibility of kref.

We can't assume any lock about kref usage, because kref is often used in lockless scenario.

Quoted 2 lines · Ming Lei

We can't assume any lock about kref usage, because kref is often used in lockless scenario.

Then you're doing it wrong, seriously.

kref in lockless scenarios simply doesn't work, you need rcu (or another means to stabilize storage) and atomic_inc_not_zero() as well as means to validate your object after the lookup.

Quoted 7 lines

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

value = obj->member;

Quoted 5 lines

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.

It should be the problem if another CPU observed that write/read obj is done before kref_get.

Quoted 9 lines · Ming Lei

Quoted 9 lines

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

 value = obj->member;

 kref_put(&obj->ref);

Quoted 6 lines

Quoted 3 lines

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.

It should be the problem if another CPU observed that write/read obj is done before kref_get.

WHY?!?!?!