[PATCH 0/3] kref: inline and barriers

[PATCH 0/3] kref: inline and barriers

[Peter Zijlstra]

>From time to time people on #kernelnewbies ask what the difference is between
kref and using atomic_t yourself. And I keep having to tell them that kref is
the slow and weird option.

These patches try and cure some of that, it inlines all functions and removes
the superfluous memory barriers that are there for no good reason at all.

[PATCH 1/3] kref: Inline all functions

[Peter Zijlstra]

[-- Attachment #1: kref-simplify.patch --]
[-- Type: text/plain, Size: 6652 bytes --]

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

Cc: adobriyan@gmail.com
Cc: eric.dumazet@gmail.com
Cc: mingo@elte.hu
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 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
===================================================================
--- linux-2.6.orig/include/linux/kref.h
+++ linux-2.6/include/linux/kref.h
@@ -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.
+ *
+ * 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.
+ */
+static inline 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.
+ */
+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
===================================================================
--- linux-2.6.orig/lib/kref.c
+++ /dev/null
@@ -1,97 +0,0 @@
-/*
- * kref.c - library routines for handling generic reference counted objects
- *
- * Copyright (C) 2004 Greg Kroah-Hartman <greg@kroah.com>
- * Copyright (C) 2004 IBM Corp.
- *
- * based on lib/kobject.c which was:
- * Copyright (C) 2002-2003 Patrick Mochel <mochel@osdl.org>
- *
- * 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
===================================================================
--- linux-2.6.orig/lib/Makefile
+++ linux-2.6/lib/Makefile
@@ -17,7 +17,7 @@ lib-y := ctype.o string.o vsprintf.o cmd
 lib-$(CONFIG_MMU) += ioremap.o
 lib-$(CONFIG_SMP) += cpumask.o
 
-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 \

[PATCH 2/3] kref: Implement kref_put in terms of kref_sub

[Peter Zijlstra]

[-- Attachment #1: kref-simplify-1.patch --]
[-- Type: text/plain, Size: 2979 bytes --]

Less lines of code is better.

Cc: adobriyan@gmail.com
Cc: eric.dumazet@gmail.com
Cc: mingo@elte.hu
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 include/linux/kref.h |   28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

Index: linux-2.6/include/linux/kref.h
===================================================================
--- linux-2.6.orig/include/linux/kref.h
+++ linux-2.6/include/linux/kref.h
@@ -43,57 +43,49 @@ static inline void kref_get(struct kref
 }
 
 /**
- * kref_put - decrement refcount for object.
+ * 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.
  *
- * Decrement the refcount, and if 0, call release().
+ * 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_put(struct kref *kref, void (*release)(struct kref *kref))
+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_dec_and_test(&kref->refcount)) {
+	if (atomic_sub_and_test((int) count, &kref->refcount)) {
 		release(kref);
 		return 1;
 	}
 	return 0;
 }
 
-
 /**
- * kref_sub - subtract a number of refcounts for object.
+ * kref_put - decrement refcount 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().
+ * 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.
  */
-static inline int kref_sub(struct kref *kref, unsigned int count,
-	     void (*release)(struct kref *kref))
+static inline int kref_put(struct kref *kref, 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;
+	return kref_sub(kref, 1, release);
 }
 #endif /* _KREF_H_ */

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

[Peter Zijlstra]

[-- Attachment #1: kref-simplify-2.patch --]
[-- Type: text/plain, Size: 1761 bytes --]

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: adobriyan@gmail.com
Cc: eric.dumazet@gmail.com
Cc: mingo@elte.hu
Cc: Oliver Neukum <oneukum@suse.de>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 include/linux/kref.h |    2 --
 1 file changed, 2 deletions(-)

Index: linux-2.6/include/linux/kref.h
===================================================================
--- linux-2.6.orig/include/linux/kref.h
+++ linux-2.6/include/linux/kref.h
@@ -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();
 }
 
 /**

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

[Ming Lei]

Hi,

On Sat, Dec 10, 2011 at 6:43 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 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

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.

> 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: adobriyan@gmail.com
> Cc: eric.dumazet@gmail.com
> Cc: mingo@elte.hu
> Cc: Oliver Neukum <oneukum@suse.de>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  include/linux/kref.h |    2 --
>  1 file changed, 2 deletions(-)
>
> Index: linux-2.6/include/linux/kref.h
> ===================================================================
> --- linux-2.6.orig/include/linux/kref.h
> +++ linux-2.6/include/linux/kref.h
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


thanks,
-- 
Ming Lei

Re: [PATCH 1/3] kref: Inline all functions

[Ming Lei]

Hi,

On Sat, Dec 10, 2011 at 6:43 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> These are tiny functions, there's no point in having them out-of-line.

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

[tom@linux-2.6-omap]$size vmlinux
   text	   data	    bss	    dec	    hex	filename
6030247	2083292	7912356	16025895	 f48927	vmlinux

[tom@linux-2.6-omap]$size vmlinux-before-kref-inline
   text	   data	    bss	    dec	    hex	filename
6023264	2083300	7912356	16018920	 f46de8	vmlinux-before-kref-inline

>
> Cc: adobriyan@gmail.com
> Cc: eric.dumazet@gmail.com
> Cc: mingo@elte.hu
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  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
> ===================================================================
> --- linux-2.6.orig/include/linux/kref.h
> +++ linux-2.6/include/linux/kref.h
> @@ -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.
> + *
> + * 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.
> + */
> +static inline int kref_put(struct kref *kref, void (*release)(struct kref *kref))
> +{
> +       WARN_ON(release == NULL);
> +       WARN_ON(release == (void (*)(struct kref *))kfree);

<linux/slab.h> is needed to avoid compiling failure.

> +
> +       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
> ===================================================================
> --- linux-2.6.orig/lib/kref.c
> +++ /dev/null
> @@ -1,97 +0,0 @@
> -/*
> - * kref.c - library routines for handling generic reference counted objects
> - *
> - * Copyright (C) 2004 Greg Kroah-Hartman <greg@kroah.com>
> - * Copyright (C) 2004 IBM Corp.
> - *
> - * based on lib/kobject.c which was:
> - * Copyright (C) 2002-2003 Patrick Mochel <mochel@osdl.org>
> - *
> - * 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
> ===================================================================
> --- linux-2.6.orig/lib/Makefile
> +++ linux-2.6/lib/Makefile
> @@ -17,7 +17,7 @@ lib-y := ctype.o string.o vsprintf.o cmd
>  lib-$(CONFIG_MMU) += ioremap.o
>  lib-$(CONFIG_SMP) += cpumask.o
>
> -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


thanks,
-- 
Ming Lei

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

[Peter Zijlstra]

On Sat, 2011-12-10 at 22:07 +0800, Ming Lei wrote:
> > 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.

Re: [PATCH 1/3] kref: Inline all functions

[Peter Zijlstra]

On Sat, 2011-12-10 at 22:32 +0800, Ming Lei wrote:
> Looks like no much text size increase in built vmlinux for ARM.
> 
> [tom@linux-2.6-omap]$size vmlinux
>    text    data     bss     dec     hex filename
> 6030247 2083292 7912356 16025895         f48927 vmlinux
> 
> [tom@linux-2.6-omap]$size vmlinux-before-kref-inline
>    text    data     bss     dec     hex filename
> 6023264 2083300 7912356 16018920         f46de8 vmlinux-before-kref-inline 

Yeah, and I guess you can reduce it even further if you put the
WARN_ON() in some CONFIG_DEBUG_BLAH section, like the refcount stuff
from Alexey was doing.

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

[Ming Lei]

On Sat, Dec 10, 2011 at 10:58 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Sat, 2011-12-10 at 22:07 +0800, Ming Lei wrote:
>> > 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?

I think so, see below:


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?

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

Sorry, could you say what the destruction race is?

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


thanks,
-- 
Ming Lei

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

[Peter Zijlstra]

On Sat, 2011-12-10 at 23:57 +0800, Ming Lei wrote:

> 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.

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.

> 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.

> > 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.

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

[Ming Lei]

On Sun, Dec 11, 2011 at 3:49 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Sat, 2011-12-10 at 23:57 +0800, Ming Lei wrote:
>
>> 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.
>
> 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.

>
> 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.

Yes, both are reasonable.

>
> 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?

>> 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.

As stated above, one smp_mb__before_atomic_inc before atomic_inc
may avoid the race.

>> > 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.

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


thanks,
-- 
Ming Lei

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

[Peter Zijlstra]

On Sun, 2011-12-11 at 10:22 +0800, Ming Lei wrote:
> On Sun, Dec 11, 2011 at 3:49 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > On Sat, 2011-12-10 at 23:57 +0800, Ming Lei wrote:
> >
> >> 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.
> >
> > 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.

> > 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.
> 
> Yes, both are reasonable.
> 
> >
> > 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.

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

There's a dec in there, isn't it. How much is 1-1?

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

[Ming Lei]

On Sun, Dec 11, 2011 at 8:47 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Sun, 2011-12-11 at 10:22 +0800, Ming Lei wrote:
>> On Sun, Dec 11, 2011 at 3:49 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
>> > On Sat, 2011-12-10 at 23:57 +0800, Ming Lei wrote:
>> >
>> >> 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.
>> >
>> > 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.
>
>> > 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.
>>
>> Yes, both are reasonable.
>>
>> >
>> > 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.

Yes, kref is designed as so, and it is nothing to do with memory barrier,
and more like a logical issue.

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


thanks,
-- 
Ming Lei

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

[Peter Zijlstra]

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.

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

[Peter Zijlstra]

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.

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

[Ming Lei]

On Mon, Dec 12, 2011 at 4:42 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 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.

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)

There is one smp_mb between E and F on CPU1, and we still need
another one between B and C  on CPU 0 to keep the order. Otherwise,
CPU1 may observe out of order about B and C, then make obj released
(F)too early,  and cause OOPS on CPU 0.

>
> 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.

Maybe not, see above.

>
> 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 */

Looks like should be kref_sub(1) since LOOKUP may handle by itself.

>
> 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

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

> 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.

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

> 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.
>
>


thanks,
-- 
Ming Lei

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

[Peter Zijlstra]

On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
> 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?

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

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

[Peter Zijlstra]

On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
> 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.

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

[Peter Zijlstra]

On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
> >  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.
> 
> It should be the problem if another CPU observed that write/read obj
> is done before kref_get. 

WHY?!?!?!

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

[Ming Lei]

On Mon, Dec 12, 2011 at 4:54 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
>> 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?

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, ...)

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


thanks,
-- 
Ming Lei

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

[Ming Lei]

On Mon, Dec 12, 2011 at 4:56 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
>> >  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.
>>
>> It should be the problem if another CPU observed that write/read obj
>> is done before kref_get.
>
> WHY?!?!?!

I did't say the reason because it is same with the example I explained in
the same reply. So if you can make me enlightened on this example,
all will be OK, :-)

thanks,
-- 
Ming Lei

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

[Peter Zijlstra]

On Mon, 2011-12-12 at 17:57 +0800, Ming Lei wrote:
> On Mon, Dec 12, 2011 at 4:54 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
> >> 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?
> 
> 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.

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

[Oliver Neukum]

Am Montag, 12. Dezember 2011, 10:57:31 schrieb Ming Lei:
> On Mon, Dec 12, 2011 at 4:54 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
> >> 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?
> 
> 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

	Regards
		Oliver

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

[Ming Lei]

On Mon, Dec 12, 2011 at 6:12 PM, Peter Zijlstra <peterz@infradead.org> wrote:

> 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...

thanks,
-- 
Ming Lei

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

[Peter Zijlstra]

On Mon, 2011-12-12 at 18:32 +0800, Ming Lei wrote:
> On Mon, Dec 12, 2011 at 6:12 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > 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.

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

[Eric Dumazet]

Le lundi 12 décembre 2011 à 18:32 +0800, Ming Lei a écrit :
> On Mon, Dec 12, 2011 at 6:12 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > 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...

Really, kref is not magical per itself.

Using kref (or any refcounting) doesnt avoid bugs.

In the example you gave, there is a clear bug :

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)


because _nothing_ prevents CPU0 accessing obj right after F:

To solve this problem, you need another way to synchronize accesses.

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

[Oliver Neukum]

Am Montag, 12. Dezember 2011, 11:32:58 schrieb Ming Lei:
> On Mon, Dec 12, 2011 at 6:12 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > 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?

You don't have a CPU ordering problem. If CPU A can do a kfree() you need to
make sure CPU B doesn't get a pointer to that object. Basically your race is:

CPU A							CPU B

p = a;
								p = a;
p->counter--;
if (!p->counter) kfree(p);
a = NULL;
								p->counter++;

This is not an ordering problem. You have a real critical section here.
It doesn't matter when CPU B sees the decrement.
You must make sure there are no pointers to objects you might free
if you intend to use the pointers without locks.

	Regards
		Oliver

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

[Ming Lei]

On Mon, Dec 12, 2011 at 7:05 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> 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

Indeed, that is the famous open/disconnect race.  In fact, there is the
bug in usb-skeleton.c. Suppose usb device is disconnected between
usb_get_intfdata() and kref_get() in skel_open(), kref_get may access
a freed object.

> 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.
>
>


thanks,
-- 
Ming Lei

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

[Greg KH]

On Mon, Dec 12, 2011 at 09:55:57AM +0100, Peter Zijlstra wrote:
> On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
> > 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.

I agree, and I'll apply your patches later today, thanks for doing them.

greg k-h

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

[Greg KH]

On Mon, Dec 12, 2011 at 11:20:16AM +0100, Oliver Neukum wrote:
> Am Montag, 12. Dezember 2011, 10:57:31 schrieb Ming Lei:
> > On Mon, Dec 12, 2011 at 4:54 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
> > >> 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?
> > 
> > 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

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.

thanks,

greg k-h

Re: [PATCH 1/3] kref: Inline all functions

[Greg KH]

On Sat, Dec 10, 2011 at 11:43:42AM +0100, Peter Zijlstra wrote:
> These are tiny functions, there's no point in having them out-of-line.

Nice, but this breaks the build:

  CC      init/main.o
In file included from include/linux/kobject.h:24:0,
                 from include/linux/module.h:16,
                 from init/main.c:13:
include/linux/kref.h: In function ‘kref_put’:
include/linux/kref.h:62:2: error: ‘kfree’ undeclared (first use in this function)
include/linux/kref.h:62:2: note: each undeclared identifier is reported only once for each function it appears in
include/linux/kref.h: In function ‘kref_sub’:
include/linux/kref.h:91:2: error: ‘kfree’ undeclared (first use in this function)
make[1]: *** [init/main.o] Error 1
make: *** [init] Error 2

Sorry, I can't apply this.

This was on an x86-64 build, my "stock" desktop system, nothing "odd" in the
config at all...

greg k-h

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

[Oliver Neukum]

Am Montag, 12. Dezember 2011, 20:30:40 schrieb Greg KH:
> On Mon, Dec 12, 2011 at 11:20:16AM +0100, Oliver Neukum wrote:
> > Am Montag, 12. Dezember 2011, 10:57:31 schrieb Ming Lei:
> > > On Mon, Dec 12, 2011 at 4:54 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
> > > >> 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?
> > > 
> > > 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
> 
> 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.

	Regards
		Oliver

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

[Greg KH]

On Mon, Dec 12, 2011 at 11:56:47PM +0100, Oliver Neukum wrote:
> Am Montag, 12. Dezember 2011, 20:30:40 schrieb Greg KH:
> > On Mon, Dec 12, 2011 at 11:20:16AM +0100, Oliver Neukum wrote:
> > > Am Montag, 12. Dezember 2011, 10:57:31 schrieb Ming Lei:
> > > > On Mon, Dec 12, 2011 at 4:54 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > > On Mon, 2011-12-12 at 11:48 +0800, Ming Lei wrote:
> > > > >> 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?
> > > > 
> > > > 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
> > 
> > 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.

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

> I think I was worried about it missing.

I'd be worried as well, but I don't think that can really happen, or am
I totally wrong?

greg k-h

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

[Peter Zijlstra]

On Mon, 2011-12-12 at 23:56 +0100, Oliver Neukum wrote:
> 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.

Re: [PATCH 1/3] kref: Inline all functions

[Peter Zijlstra]

On Mon, 2011-12-12 at 14:11 -0800, Greg KH wrote:
> 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 <a.p.zijlstra@chello.nl>
Date: Sat Dec 10 11:29:57 CET 2011

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

Cc: adobriyan@gmail.com
Cc: eric.dumazet@gmail.com
Cc: mingo@elte.hu
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/n/tip-8eccvi2ur2fzgi00xdjlbf5z@git.kernel.org
---
 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
===================================================================
--- linux-2.6.orig/include/linux/kref.h
+++ linux-2.6/include/linux/kref.h
@@ -16,15 +16,85 @@
 #define _KREF_H_
 
 #include <linux/types.h>
+#include <linux/slab.h>
 
 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.
+ *
+ * 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.
+ */
+static inline 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.
+ */
+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
===================================================================
--- linux-2.6.orig/lib/kref.c
+++ /dev/null
@@ -1,97 +0,0 @@
-/*
- * kref.c - library routines for handling generic reference counted objects
- *
- * Copyright (C) 2004 Greg Kroah-Hartman <greg@kroah.com>
- * Copyright (C) 2004 IBM Corp.
- *
- * based on lib/kobject.c which was:
- * Copyright (C) 2002-2003 Patrick Mochel <mochel@osdl.org>
- *
- * 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
===================================================================
--- linux-2.6.orig/lib/Makefile
+++ linux-2.6/lib/Makefile
@@ -17,7 +17,7 @@ lib-y := ctype.o string.o vsprintf.o cmd
 lib-$(CONFIG_MMU) += ioremap.o
 lib-$(CONFIG_SMP) += cpumask.o
 
-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 \

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

[Oliver Neukum]

Am Dienstag, 13. Dezember 2011, 10:12:20 schrieb Peter Zijlstra:
> On Mon, 2011-12-12 at 23:56 +0100, Oliver Neukum wrote:
> > 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.

I agree, so let's remove it.

	Regards
		

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

[Oliver Neukum]

Am Dienstag, 13. Dezember 2011, 00:14:19 schrieb Greg KH:
> > 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.
> 
> Really?  On an atomic variable?  I didn't think this was needed for
> atomics to ensure this type of thing couldn't happen.

If you use an atomic variable you can be sure that the result will be
mathematically correct, even if you touch the variable from many CPUs.
(with add & sub of course) That is, refering to that variable.

It does not guarantee ordering

CPU A								CPU B
atomic_set(&a, 1);
atomic_set(&b, 1);
atomic_set(&c, 1);
									while (!atomic_read(&c));
									d = atomic_read(&a) + atomic_read(&b);

is asking for trouble. You need to do:

CPU A								CPU B
atomic_set(&a, 1);
atomic_set(&b, 1);
smp_wmb();
atomic_set(&c, 1);
									while (!atomic_read(&c));
									smp_rmb();
									d = atomic_read(&a) + atomic_read(&b);

Now replace c with an interrupt and you see the problem. It definitely exists,
but my solution was quite bad. The wmb() must be in start_io() in the first example
I gave. Putting it into kref was the wrong place.

	Regards
		Oliver

PS: even in the example I first gave the result will eventually be 0.
But that is useless because the check for zero is done only in kref_put()


-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 

Re: [PATCH 1/3] kref: Inline all functions

[Greg KH]

On Tue, Dec 13, 2011 at 10:36:20AM +0100, Peter Zijlstra wrote:
> On Mon, 2011-12-12 at 14:11 -0800, Greg KH wrote:
> > Nice, but this breaks the build:
> 
> Sorry about that, this one includes the missing include and just booted
> on my test-box.

Thanks, that worked.

> 
> ---
> Subject: kref: Inline all functions
> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Date: Sat Dec 10 11:29:57 CET 2011
> 
> These are tiny functions, there's no point in having them out-of-line.
> 
> Cc: adobriyan@gmail.com
> Cc: eric.dumazet@gmail.com
> Cc: mingo@elte.hu
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Link: http://lkml.kernel.org/n/tip-8eccvi2ur2fzgi00xdjlbf5z@git.kernel.org
> ---
>  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 :)

greg k-h

Re: [PATCH 1/3] kref: Inline all functions

[Peter Zijlstra]

On Tue, 2011-12-13 at 09:15 -0800, Greg KH wrote:

> > ---
> >  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 :)

Dunno, it looks like quilt generates that for me.

Re: [PATCH 1/3] kref: Inline all functions

[Greg KH]

On Tue, Dec 13, 2011 at 07:52:03PM +0100, Peter Zijlstra wrote:
> On Tue, 2011-12-13 at 09:15 -0800, Greg KH wrote:
> 
> > > ---
> > >  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 :)
> 
> Dunno, it looks like quilt generates that for me.

You need to put:
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"

into your .quiltrc so the index markings will not show up, as well as
using some other "sane" defaults for generating patches.

greg k-h

Re: [PATCH 1/3] kref: Inline all functions

[Peter Zijlstra]

On Tue, 2011-12-13 at 11:11 -0800, Greg KH wrote:
> 
> 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" 

Ah, its the --no-index that does that. I also missed the --sort and -p
ab thing, the rest I already had.

Thanks!