[PATCH][RFC] Handle improbable possibility of io_context->refcount overflow

[PATCH][RFC] Handle improbable possibility of io_context->refcount overflow

[Nikanth Karthikesan]

Hi Jens

Currently io_context has an atomic_t(int) as refcount. In case of cfq, for
each device a task does I/O, a reference to the io_context would be taken. And
when there are multiple process sharing io_contexts(CLONE_IO) would also have
a reference to the same io_context. Theoretically the possible maximum number
of processes sharing the same io_context + the number of disks/cfq_data
referring to the same io_context can overflow the 32-bit counter on a very
high-end machine. Even though it is an improbable case, let us make it
difficult by changing the refcount to atomic64_t(long).

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 012f065..4f5ff79 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -35,9 +35,9 @@ int put_io_context(struct io_context *ioc)
 	if (ioc == NULL)
 		return 1;
 
-	BUG_ON(atomic_read(&ioc->refcount) == 0);
+	BUG_ON(atomic64_read(&ioc->refcount) == 0);
 
-	if (atomic_dec_and_test(&ioc->refcount)) {
+	if (atomic64_dec_and_test(&ioc->refcount)) {
 		rcu_read_lock();
 		if (ioc->aic && ioc->aic->dtor)
 			ioc->aic->dtor(ioc->aic);
@@ -151,7 +151,7 @@ struct io_context *get_io_context(gfp_t gfp_flags, int node)
 		ret = current_io_context(gfp_flags, node);
 		if (unlikely(!ret))
 			break;
-	} while (!atomic_inc_not_zero(&ret->refcount));
+	} while (!atomic64_inc_not_zero(&ret->refcount));
 
 	return ret;
 }
@@ -163,8 +163,8 @@ void copy_io_context(struct io_context **pdst, struct io_context **psrc)
 	struct io_context *dst = *pdst;
 
 	if (src) {
-		BUG_ON(atomic_read(&src->refcount) == 0);
-		atomic_inc(&src->refcount);
+		BUG_ON(atomic64_read(&src->refcount) == 0);
+		atomic64_inc(&src->refcount);
 		put_io_context(dst);
 		*pdst = src;
 	}
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index a55a9bd..f829bd2 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1282,7 +1282,7 @@ static void cfq_dispatch_request(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 	if (!cfqd->active_cic) {
 		struct cfq_io_context *cic = RQ_CIC(rq);
 
-		atomic_inc(&cic->ioc->refcount);
+		atomic64_inc(&cic->ioc->refcount);
 		cfqd->active_cic = cic;
 	}
 }
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 08b987b..f1a57f4 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -64,7 +64,7 @@ struct cfq_io_context {
  * and kmalloc'ed. These could be shared between processes.
  */
 struct io_context {
-	atomic_t refcount;
+	atomic64_t refcount;
 	atomic_t nr_tasks;
 
 	/* all the fields below are protected by this lock */
@@ -91,7 +91,7 @@ static inline struct io_context *ioc_task_link(struct io_context *ioc)
 	 * if ref count is zero, don't allow sharing (ioc is going away, it's
 	 * a race).
 	 */
-	if (ioc && atomic_inc_not_zero(&ioc->refcount)) {
+	if (ioc && atomic64_inc_not_zero(&ioc->refcount)) {
 		atomic_inc(&ioc->nr_tasks);
 		return ioc;
 	}

Re: [PATCH][RFC] Handle improbable possibility of io_context->refcount overflow

[Andrew Morton]

On Wed, 29 Apr 2009 12:21:39 +0530 Nikanth Karthikesan <knikanth@novell.com> wrote:

> Hi Jens
> 
> Currently io_context has an atomic_t(int) as refcount. In case of cfq, for
> each device a task does I/O, a reference to the io_context would be taken. And
> when there are multiple process sharing io_contexts(CLONE_IO) would also have
> a reference to the same io_context. Theoretically the possible maximum number
> of processes sharing the same io_context + the number of disks/cfq_data
> referring to the same io_context can overflow the 32-bit counter on a very
> high-end machine. Even though it is an improbable case, let us make it
> difficult by changing the refcount to atomic64_t(long).
> 

Sorry, atomic64_t isn't implemented on 32 bit architectures.

Perhaps it should be, but I expect it'd be pretty slow.

Re: [PATCH][RFC] Handle improbable possibility of io_context->refcount overflow

[Nikanth Karthikesan]

On Wednesday 29 April 2009 13:29:30 Andrew Morton wrote:
> On Wed, 29 Apr 2009 12:21:39 +0530 Nikanth Karthikesan <knikanth@novell.com> wrote:
> > Hi Jens
> >
> > Currently io_context has an atomic_t(int) as refcount. In case of cfq,
> > for each device a task does I/O, a reference to the io_context would be
> > taken. And when there are multiple process sharing io_contexts(CLONE_IO)
> > would also have a reference to the same io_context. Theoretically the
> > possible maximum number of processes sharing the same io_context + the
> > number of disks/cfq_data referring to the same io_context can overflow
> > the 32-bit counter on a very high-end machine. Even though it is an
> > improbable case, let us make it difficult by changing the refcount to
> > atomic64_t(long).
>
> Sorry, atomic64_t isn't implemented on 32 bit architectures.
>
> Perhaps it should be, but I expect it'd be pretty slow.

Oh! Sorry, I didn't notice the #ifdef earlier. I guess thats why there is only
a single in-tree user for atomic64_t!

In this case, could we make it atomic64_t only on 64-bit architectures and
keep it as atomic_t on 32-bit machines? Something like the attached patch.

I wonder whether we should also add BUG_ON's whenever the refcount is about to
wrap? Or try to handle it gracefully. Another approach would be to impose an
artificial limit on the no of tasks that could share an io_context. Or resort
to lock protection. The problem is not very serious/common.

Thanks
Nikanth

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 012f065..5be4585 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -35,9 +35,9 @@ int put_io_context(struct io_context *ioc)
 	if (ioc == NULL)
 		return 1;
 
-	BUG_ON(atomic_read(&ioc->refcount) == 0);
+	BUG_ON(atomic_read_ioc_refcount(ioc) == 0);
 
-	if (atomic_dec_and_test(&ioc->refcount)) {
+	if (atomic_dec_and_test_ioc_refcount(ioc)) {
 		rcu_read_lock();
 		if (ioc->aic && ioc->aic->dtor)
 			ioc->aic->dtor(ioc->aic);
@@ -151,7 +151,7 @@ struct io_context *get_io_context(gfp_t gfp_flags, int node)
 		ret = current_io_context(gfp_flags, node);
 		if (unlikely(!ret))
 			break;
-	} while (!atomic_inc_not_zero(&ret->refcount));
+	} while (!atomic_inc_not_zero_ioc_refcount(ret));
 
 	return ret;
 }
@@ -163,8 +163,8 @@ void copy_io_context(struct io_context **pdst, struct io_context **psrc)
 	struct io_context *dst = *pdst;
 
 	if (src) {
-		BUG_ON(atomic_read(&src->refcount) == 0);
-		atomic_inc(&src->refcount);
+		BUG_ON(atomic_read_ioc_refcount(src) == 0);
+		atomic_inc_ioc_refcount(src);
 		put_io_context(dst);
 		*pdst = src;
 	}
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index a55a9bd..42d5018 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1282,7 +1282,7 @@ static void cfq_dispatch_request(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 	if (!cfqd->active_cic) {
 		struct cfq_io_context *cic = RQ_CIC(rq);
 
-		atomic_inc(&cic->ioc->refcount);
+		atomic_inc_ioc_refcount(cic->ioc);
 		cfqd->active_cic = cic;
 	}
 }
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 08b987b..bdc7156 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -64,7 +64,11 @@ struct cfq_io_context {
  * and kmalloc'ed. These could be shared between processes.
  */
 struct io_context {
+#ifdef CONFIG_64BIT
+	atomic64_t refcount;
+#else
 	atomic_t refcount;
+#endif
 	atomic_t nr_tasks;
 
 	/* all the fields below are protected by this lock */
@@ -85,14 +89,30 @@ struct io_context {
 	void *ioc_data;
 };
 
+#ifdef CONFIG_64BIT
+
+#define atomic_read_ioc_refcount(ioc)	atomic64_read(&ioc->refcount)
+#define atomic_inc_ioc_refcount(ioc)	atomic64_inc(&ioc->refcount)
+#define atomic_dec_and_test_ioc_refcount(ioc)	atomic64_dec_and_test(&ioc->refcount)
+#define atomic_inc_not_zero_ioc_refcount(ioc)	atomic64_inc_not_zero(&ioc->refcount)
+
+#else
+
+#define atomic_read_ioc_refcount(ioc)	atomic_read(&ioc->refcount)
+#define atomic_inc_ioc_refcount(ioc)	atomic_inc(&ioc->refcount)
+#define atomic_dec_and_test_ioc_refcount(ioc)	atomic_dec_and_test(&ioc->refcount)
+#define atomic_inc_not_zero_ioc_refcount(ioc)	atomic_inc_not_zero(&ioc->refcount)
+
+#endif
+
 static inline struct io_context *ioc_task_link(struct io_context *ioc)
 {
 	/*
 	 * if ref count is zero, don't allow sharing (ioc is going away, it's
 	 * a race).
 	 */
-	if (ioc && atomic_inc_not_zero(&ioc->refcount)) {
-		atomic_inc(&ioc->nr_tasks);
+	if (ioc && atomic_inc_not_zero_ioc_refcount(ioc)) {
+		atomic_inc_ioc_refcount(ioc);
 		return ioc;
 	}
 

Re: [PATCH][RFC] Handle improbable possibility of io_context->refcount overflow

[Andrew Morton]

On Wed, 29 Apr 2009 15:33:06 +0530 Nikanth Karthikesan <knikanth@novell.com> wrote:

> On Wednesday 29 April 2009 13:29:30 Andrew Morton wrote:
> > On Wed, 29 Apr 2009 12:21:39 +0530 Nikanth Karthikesan <knikanth@novell.com> wrote:
> > > Hi Jens
> > >
> > > Currently io_context has an atomic_t(int) as refcount. In case of cfq,
> > > for each device a task does I/O, a reference to the io_context would be
> > > taken. And when there are multiple process sharing io_contexts(CLONE_IO)
> > > would also have a reference to the same io_context. Theoretically the
> > > possible maximum number of processes sharing the same io_context + the
> > > number of disks/cfq_data referring to the same io_context can overflow
> > > the 32-bit counter on a very high-end machine. Even though it is an
> > > improbable case, let us make it difficult by changing the refcount to
> > > atomic64_t(long).
> >
> > Sorry, atomic64_t isn't implemented on 32 bit architectures.
> >
> > Perhaps it should be, but I expect it'd be pretty slow.
> 
> Oh! Sorry, I didn't notice the #ifdef earlier. I guess thats why there is only
> a single in-tree user for atomic64_t!

Yes, it's a bit irritating.

> In this case, could we make it atomic64_t only on 64-bit architectures and
> keep it as atomic_t on 32-bit machines?

Sure.

> Something like the attached patch.

Check out atomic_long_t ;)

> I wonder whether we should also add BUG_ON's whenever the refcount is about to
> wrap? Or try to handle it gracefully. Another approach would be to impose an
> artificial limit on the no of tasks that could share an io_context. Or resort
> to lock protection. The problem is not very serious/common.
> 

For a long time there was a debug patch in -mm which would warn if
atomic_dec() ever took any atomic_t from zero to -1.  I don't think it
ever triggered false positives and it did find a couple of bugs.

I forget what happened to the patch - probably it died when the atomic
code got altered.

It could well be that a similar kernel-wide check for atomic_inc()
overflows would be similarly useful.

Re: [PATCH][RFC] Handle improbable possibility of io_context->refcount overflow

[Nikanth Karthikesan]

On Wednesday 29 April 2009 20:45:58 Andrew Morton wrote:
> On Wed, 29 Apr 2009 15:33:06 +0530 Nikanth Karthikesan <knikanth@novell.com> 
wrote:
> > On Wednesday 29 April 2009 13:29:30 Andrew Morton wrote:
> > > On Wed, 29 Apr 2009 12:21:39 +0530 Nikanth Karthikesan 
<knikanth@novell.com> wrote:
> > > > Hi Jens
> > > >
> > > > Currently io_context has an atomic_t(int) as refcount. In case of
> > > > cfq, for each device a task does I/O, a reference to the io_context
> > > > would be taken. And when there are multiple process sharing
> > > > io_contexts(CLONE_IO) would also have a reference to the same
> > > > io_context. Theoretically the possible maximum number of processes
> > > > sharing the same io_context + the number of disks/cfq_data referring
> > > > to the same io_context can overflow the 32-bit counter on a very
> > > > high-end machine. Even though it is an improbable case, let us make
> > > > it difficult by changing the refcount to atomic64_t(long).
> > >
> > > Sorry, atomic64_t isn't implemented on 32 bit architectures.
> > >
> > > Perhaps it should be, but I expect it'd be pretty slow.
> >
> > Oh! Sorry, I didn't notice the #ifdef earlier. I guess thats why there is
> > only a single in-tree user for atomic64_t!
>
> Yes, it's a bit irritating.
>
> > In this case, could we make it atomic64_t only on 64-bit architectures
> > and keep it as atomic_t on 32-bit machines?
>
> Sure.
>
> > Something like the attached patch.
>
> Check out atomic_long_t ;)
>

Oh, thanks! I was about to re-invent it. :) Sending a patch using that in a 
seperate mail.

> > I wonder whether we should also add BUG_ON's whenever the refcount is
> > about to wrap? Or try to handle it gracefully. Another approach would be
> > to impose an artificial limit on the no of tasks that could share an
> > io_context. Or resort to lock protection. The problem is not very
> > serious/common.
>
> For a long time there was a debug patch in -mm which would warn if
> atomic_dec() ever took any atomic_t from zero to -1.  I don't think it
> ever triggered false positives and it did find a couple of bugs.
>
> I forget what happened to the patch - probably it died when the atomic
> code got altered.
>
> It could well be that a similar kernel-wide check for atomic_inc()
> overflows would be similarly useful.

Sending a patch for this as well as a seperate mail.

Thanks
Nikanth

[PATCH v2] Handle improbable possibility of io_context->refcount overflow

[Nikanth Karthikesan]

Currently io_context has an atomic_t(32-bit) as refcount. In case of cfq, for
each device a task does I/O, a reference to the io_context would be taken. And
when there are multiple process sharing io_contexts(CLONE_IO) would also have
a reference to the same io_context. Theoretically the possible maximum number
of processes sharing the same io_context + the number of disks/cfq_data
referring to the same io_context can overflow the 32-bit counter on a very
high-end machine. Even though it is an improbable case, let us make it
difficult atleast on 64-bit architectures by changing the refcount to
atomic_long_t.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 012f065..d4ed600 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -35,9 +35,9 @@ int put_io_context(struct io_context *ioc)
 	if (ioc == NULL)
 		return 1;
 
-	BUG_ON(atomic_read(&ioc->refcount) == 0);
+	BUG_ON(atomic_long_read(&ioc->refcount) == 0);
 
-	if (atomic_dec_and_test(&ioc->refcount)) {
+	if (atomic_long_dec_and_test(&ioc->refcount)) {
 		rcu_read_lock();
 		if (ioc->aic && ioc->aic->dtor)
 			ioc->aic->dtor(ioc->aic);
@@ -90,7 +90,7 @@ struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
 
 	ret = kmem_cache_alloc_node(iocontext_cachep, gfp_flags, node);
 	if (ret) {
-		atomic_set(&ret->refcount, 1);
+		atomic_long_set(&ret->refcount, 1);
 		atomic_set(&ret->nr_tasks, 1);
 		spin_lock_init(&ret->lock);
 		ret->ioprio_changed = 0;
@@ -151,7 +151,7 @@ struct io_context *get_io_context(gfp_t gfp_flags, int node)
 		ret = current_io_context(gfp_flags, node);
 		if (unlikely(!ret))
 			break;
-	} while (!atomic_inc_not_zero(&ret->refcount));
+	} while (!atomic_long_inc_not_zero(&ret->refcount));
 
 	return ret;
 }
@@ -163,8 +163,8 @@ void copy_io_context(struct io_context **pdst, struct io_context **psrc)
 	struct io_context *dst = *pdst;
 
 	if (src) {
-		BUG_ON(atomic_read(&src->refcount) == 0);
-		atomic_inc(&src->refcount);
+		BUG_ON(atomic_long_read(&src->refcount) == 0);
+		atomic_long_inc(&src->refcount);
 		put_io_context(dst);
 		*pdst = src;
 	}
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index a55a9bd..6a79c5b 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1282,7 +1282,7 @@ static void cfq_dispatch_request(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 	if (!cfqd->active_cic) {
 		struct cfq_io_context *cic = RQ_CIC(rq);
 
-		atomic_inc(&cic->ioc->refcount);
+		atomic_long_inc(&cic->ioc->refcount);
 		cfqd->active_cic = cic;
 	}
 }
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 08b987b..dd05434 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -64,7 +64,7 @@ struct cfq_io_context {
  * and kmalloc'ed. These could be shared between processes.
  */
 struct io_context {
-	atomic_t refcount;
+	atomic_long_t refcount;
 	atomic_t nr_tasks;
 
 	/* all the fields below are protected by this lock */
@@ -91,8 +91,8 @@ static inline struct io_context *ioc_task_link(struct io_context *ioc)
 	 * if ref count is zero, don't allow sharing (ioc is going away, it's
 	 * a race).
 	 */
-	if (ioc && atomic_inc_not_zero(&ioc->refcount)) {
-		atomic_inc(&ioc->nr_tasks);
+	if (ioc && atomic_long_inc_not_zero(&ioc->refcount)) {
+		atomic_long_inc(&ioc->refcount);
 		return ioc;
 	}
 

[PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Nikanth Karthikesan]

Add a debug option to detect and warn when the 32-bit atomic_t wraps around
during atomic_inc and atomic_dec.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
index 85b46fb..92b898f 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -3,8 +3,10 @@
 
 #include <linux/compiler.h>
 #include <linux/types.h>
+#include <linux/kernel.h>
 #include <asm/processor.h>
 #include <asm/cmpxchg.h>
+#include <asm/bug.h>
 
 /*
  * Atomic operations that C can't guarantee us.  Useful for
@@ -77,6 +79,8 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 	return c;
 }
 
+static inline int atomic_add_unless(atomic_t *v, int a, int u);
+
 /**
  * atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
@@ -85,8 +89,12 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
  */
 static inline void atomic_inc(atomic_t *v)
 {
+#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
+	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
+#else
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "+m" (v->counter));
+#endif
 }
 
 /**
@@ -97,8 +105,12 @@ static inline void atomic_inc(atomic_t *v)
  */
 static inline void atomic_dec(atomic_t *v)
 {
+#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
+	WARN_ON(atomic_add_unless(v, -1, INT_MIN) == 0);
+#else
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "+m" (v->counter));
+#endif
 }
 
 /**
diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
index 8c21731..c34a6fa 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -2,8 +2,10 @@
 #define _ASM_X86_ATOMIC_64_H
 
 #include <linux/types.h>
+#include <linux/kernel.h>
 #include <asm/alternative.h>
 #include <asm/cmpxchg.h>
+#include <asm/bug.h>
 
 /*
  * Atomic operations that C can't guarantee us.  Useful for
@@ -76,6 +78,8 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 	return c;
 }
 
+static inline int atomic_add_unless(atomic_t *v, int a, int u);
+
 /**
  * atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
@@ -84,9 +88,13 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
  */
 static inline void atomic_inc(atomic_t *v)
 {
+#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
+	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
+#else
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "=m" (v->counter)
 		     : "m" (v->counter));
+#endif
 }
 
 /**
@@ -97,9 +105,13 @@ static inline void atomic_inc(atomic_t *v)
  */
 static inline void atomic_dec(atomic_t *v)
 {
+#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
+	WARN_ON(atomic_add_unless(v, -1, INT_MIN) == 0);
+#else
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "=m" (v->counter)
 		     : "m" (v->counter));
+#endif
 }
 
 /**
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 812c282..a446a98 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -17,6 +17,13 @@ config ENABLE_WARN_DEPRECATED
 	  Disable this to suppress the "warning: 'foo' is deprecated
 	  (declared at kernel/power/somefile.c:1234)" messages.
 
+config ENABLE_WARN_ATOMIC_INC_WRAP
+	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
+	default y
+	help
+	  Enable printing a warning when atomic_inc() or atomic_dec()
+	  operation wraps around the 32-bit value.
+
 config ENABLE_MUST_CHECK
 	bool "Enable __must_check logic"
 	default y

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Ingo Molnar]

* Nikanth Karthikesan <knikanth@novell.com> wrote:

> Add a debug option to detect and warn when the 32-bit atomic_t 
> wraps around during atomic_inc and atomic_dec.
> 
> Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

hm, what's the motivation?

As a generic debug helper this is not appropriate i think - counts 
can easily have a meaning when going negative as well. (we have no 
signed-atomic primitives)

>  static inline void atomic_inc(atomic_t *v)
>  {
> +#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
> +	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> +#else
>  	asm volatile(LOCK_PREFIX "incl %0"
>  		     : "+m" (v->counter));
> +#endif
>  }

also looks a bit ugly - this ugly #ifdef would spread into every 
architecture.

If we want to restrict atomic_t value ranges like that then the 
clean solution would be to add generic wrappers doing the debug 
(once, in generic code), and renaming the arch primitives to 
raw_atomic_inc() (etc), doing the lowlevel bits cleanly.

Do we really want this?

	Ingo

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Nikanth Karthikesan]

On Thursday 30 April 2009 13:53:50 Ingo Molnar wrote:
> * Nikanth Karthikesan <knikanth@novell.com> wrote:
> > Add a debug option to detect and warn when the 32-bit atomic_t
> > wraps around during atomic_inc and atomic_dec.
> >
> > Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
>
> hm, what's the motivation?
>

See http://lkml.org/lkml/2009/4/29/424 
Andrew said that a generic atomic_t overflow checker might be useful.

> As a generic debug helper this is not appropriate i think - counts
> can easily have a meaning when going negative as well. (we have no
> signed-atomic primitives)
>

This doesn't warn when it becomes negative/positive from zero, but only
when it wraps around^W^Woverflows, trying to add past INT_MAX or
subtract from INT_MIN.

> >  static inline void atomic_inc(atomic_t *v)
> >  {
> > +#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
> > +	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> > +#else
> >  	asm volatile(LOCK_PREFIX "incl %0"
> >
> >  		     : "+m" (v->counter));
> >
> > +#endif
> >  }
>
> also looks a bit ugly - this ugly #ifdef would spread into every
> architecture.
>
> If we want to restrict atomic_t value ranges like that then the
> clean solution would be to add generic wrappers doing the debug
> (once, in generic code), and renaming the arch primitives to
> raw_atomic_inc() (etc), doing the lowlevel bits cleanly.
>

Here is a patch which does it this way.

Thanks
Nikanth

Detect and warn on atomic_inc/atomic_dec overflow.

Add a debug option to detect and warn when the 32-bit atomic_t overflows
during atomic_inc and atomic_dec.

diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
index 85b46fb..c6a17bb 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "+m" (v->counter));
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "+m" (v->counter));
diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
index 8c21731..1183b85 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "=m" (v->counter)
@@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "=m" (v->counter)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index 7abdaa9..6eda22b 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -4,15 +4,52 @@
  * Copyright (C) 2005 Silicon Graphics, Inc.
  *	Christoph Lameter
  *
- * Allows to provide arch independent atomic definitions without the need to
- * edit all arch specific atomic.h files.
  */
 
+#include <linux/kernel.h>
 #include <asm/types.h>
+#include <asm/bug.h>
+
+#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ * Prints a warning if it wraps around.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
+}
+
+/**
+ * atomic_dec - decrement atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ * Prints a warning if it wraps around.
+ */
+static inline void atomic_dec(atomic_t *v)
+{
+	WARN_ON(atomic_add_unless(v, -1, INT_MIN) == 0);
+}
+
+#else
+
+#define atomic_inc(v)	raw_atomic_inc(v)
+#define atomic_dec(v)	raw_atomic_dec(v)
+
+#endif
+
 
 /*
  * Suppport for atomic_long_t
  *
+ * Allows to provide arch independent atomic definitions without the need to
+ * edit all arch specific atomic.h files.
+ *
  * Casts for parameters are avoided for existing atomic functions in order to
  * avoid issues with cast-as-lval under gcc 4.x and other limitations that the
  * macros of a platform may have.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 812c282..a446a98 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -17,6 +17,13 @@ config ENABLE_WARN_DEPRECATED
 	  Disable this to suppress the "warning: 'foo' is deprecated
 	  (declared at kernel/power/somefile.c:1234)" messages.
 
+config ENABLE_WARN_ATOMIC_INC_WRAP
+	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
+	default y
+	help
+	  Enable printing a warning when atomic_inc() or atomic_dec()
+	  operation wraps around the 32-bit value.
+
 config ENABLE_MUST_CHECK
 	bool "Enable __must_check logic"
 	default y

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Ingo Molnar]

* Nikanth Karthikesan <knikanth@novell.com> wrote:

> > As a generic debug helper this is not appropriate i think - 
> > counts can easily have a meaning when going negative as well. 
> > (we have no signed-atomic primitives)
> 
> This doesn't warn when it becomes negative/positive from zero, but 
> only when it wraps around^W^Woverflows, trying to add past INT_MAX 
> or subtract from INT_MIN.

ah! The small difference between INT_MAX and UINT_MAX.

Sure, this debug check makes a lot of sense. -mm even had such 
stuff, many years ago? Never went upstream.

> > >  static inline void atomic_inc(atomic_t *v)
> > >  {
> > > +#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
> > > +	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> > > +#else
> > >  	asm volatile(LOCK_PREFIX "incl %0"
> > >
> > >  		     : "+m" (v->counter));
> > >
> > > +#endif
> > >  }
> >
> > also looks a bit ugly - this ugly #ifdef would spread into every
> > architecture.
> >
> > If we want to restrict atomic_t value ranges like that then the
> > clean solution would be to add generic wrappers doing the debug
> > (once, in generic code), and renaming the arch primitives to
> > raw_atomic_inc() (etc), doing the lowlevel bits cleanly.
> >
> 
> Here is a patch which does it this way.
> 
> Thanks
> Nikanth
> 
> Detect and warn on atomic_inc/atomic_dec overflow.
> 
> Add a debug option to detect and warn when the 32-bit atomic_t overflows
> during atomic_inc and atomic_dec.
> 
> diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
> index 85b46fb..c6a17bb 100644
> --- a/arch/x86/include/asm/atomic_32.h
> +++ b/arch/x86/include/asm/atomic_32.h
> @@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
>  }
>  
>  /**
> - * atomic_inc - increment atomic variable
> + * raw_atomic_inc - increment atomic variable
>   * @v: pointer of type atomic_t
>   *
>   * Atomically increments @v by 1.
>   */
> -static inline void atomic_inc(atomic_t *v)
> +static inline void raw_atomic_inc(atomic_t *v)
>  {
>  	asm volatile(LOCK_PREFIX "incl %0"
>  		     : "+m" (v->counter));
>  }
>  
>  /**
> - * atomic_dec - decrement atomic variable
> + * raw_atomic_dec - decrement atomic variable
>   * @v: pointer of type atomic_t
>   *
>   * Atomically decrements @v by 1.
>   */
> -static inline void atomic_dec(atomic_t *v)
> +static inline void raw_atomic_dec(atomic_t *v)
>  {
>  	asm volatile(LOCK_PREFIX "decl %0"
>  		     : "+m" (v->counter));
> diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
> index 8c21731..1183b85 100644
> --- a/arch/x86/include/asm/atomic_64.h
> +++ b/arch/x86/include/asm/atomic_64.h
> @@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
>  }
>  
>  /**
> - * atomic_inc - increment atomic variable
> + * raw_atomic_inc - increment atomic variable
>   * @v: pointer of type atomic_t
>   *
>   * Atomically increments @v by 1.
>   */
> -static inline void atomic_inc(atomic_t *v)
> +static inline void raw_atomic_inc(atomic_t *v)
>  {
>  	asm volatile(LOCK_PREFIX "incl %0"
>  		     : "=m" (v->counter)
> @@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
>  }
>  
>  /**
> - * atomic_dec - decrement atomic variable
> + * raw_atomic_dec - decrement atomic variable
>   * @v: pointer of type atomic_t
>   *
>   * Atomically decrements @v by 1.
>   */
> -static inline void atomic_dec(atomic_t *v)
> +static inline void raw_atomic_dec(atomic_t *v)
>  {
>  	asm volatile(LOCK_PREFIX "decl %0"
>  		     : "=m" (v->counter)
> diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
> index 7abdaa9..6eda22b 100644
> --- a/include/asm-generic/atomic.h
> +++ b/include/asm-generic/atomic.h
> @@ -4,15 +4,52 @@
>   * Copyright (C) 2005 Silicon Graphics, Inc.
>   *	Christoph Lameter
>   *
> - * Allows to provide arch independent atomic definitions without the need to
> - * edit all arch specific atomic.h files.
>   */
>  
> +#include <linux/kernel.h>
>  #include <asm/types.h>
> +#include <asm/bug.h>
> +
> +#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)

#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP

> +
> +/**
> + * atomic_inc - increment atomic variable
> + * @v: pointer of type atomic_t
> + *
> + * Atomically increments @v by 1.
> + * Prints a warning if it wraps around.
> + */
> +static inline void atomic_inc(atomic_t *v)
> +{
> +	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> +}
> +
> +/**
> + * atomic_dec - decrement atomic variable
> + * @v: pointer of type atomic_t
> + *
> + * Atomically decrements @v by 1.
> + * Prints a warning if it wraps around.
> + */
> +static inline void atomic_dec(atomic_t *v)
> +{
> +	WARN_ON(atomic_add_unless(v, -1, INT_MIN) == 0);
> +}
> +
> +#else
> +
> +#define atomic_inc(v)	raw_atomic_inc(v)
> +#define atomic_dec(v)	raw_atomic_dec(v)

Please turn these into proper, type aware inline functions.

> +config ENABLE_WARN_ATOMIC_INC_WRAP
> +	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
> +	default y
> +	help
> +	  Enable printing a warning when atomic_inc() or atomic_dec()
> +	  operation wraps around the 32-bit value.
> +

this needs HAVE_ARCH_DEBUG_ATOMIC added to arch/x86/Kconfig, and a 
depends on HAVE_ARCH_DEBUG_ATOMIC. Otherwise this wont build very 
well on non-x86 when enabled, right?

With those small details fixed:

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Nikanth Karthikesan]

On Thursday 30 April 2009 16:17:29 Ingo Molnar wrote:
> * Nikanth Karthikesan <knikanth@novell.com> wrote:
> > > As a generic debug helper this is not appropriate i think -
> > > counts can easily have a meaning when going negative as well.
> > > (we have no signed-atomic primitives)
> >
> > This doesn't warn when it becomes negative/positive from zero, but
> > only when it wraps around^W^Woverflows, trying to add past INT_MAX
> > or subtract from INT_MIN.
>
> ah! The small difference between INT_MAX and UINT_MAX.
>
> Sure, this debug check makes a lot of sense. -mm even had such
> stuff, many years ago? Never went upstream.
>
> > > >  static inline void atomic_inc(atomic_t *v)
> > > >  {
> > > > +#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
> > > > +	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> > > > +#else
> > > >  	asm volatile(LOCK_PREFIX "incl %0"
> > > >
> > > >  		     : "+m" (v->counter));
> > > >
> > > > +#endif
> > > >  }
> > >
> > > also looks a bit ugly - this ugly #ifdef would spread into every
> > > architecture.
> > >
> > > If we want to restrict atomic_t value ranges like that then the
> > > clean solution would be to add generic wrappers doing the debug
> > > (once, in generic code), and renaming the arch primitives to
> > > raw_atomic_inc() (etc), doing the lowlevel bits cleanly.
> >
> > Here is a patch which does it this way.
> >
> > Thanks
> > Nikanth
> >
> > Detect and warn on atomic_inc/atomic_dec overflow.
> >
> > Add a debug option to detect and warn when the 32-bit atomic_t overflows
> > during atomic_inc and atomic_dec.
> >
> > diff --git a/arch/x86/include/asm/atomic_32.h
> > b/arch/x86/include/asm/atomic_32.h index 85b46fb..c6a17bb 100644
> > --- a/arch/x86/include/asm/atomic_32.h
> > +++ b/arch/x86/include/asm/atomic_32.h
> > @@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t
> > *v) }
> >
> >  /**
> > - * atomic_inc - increment atomic variable
> > + * raw_atomic_inc - increment atomic variable
> >   * @v: pointer of type atomic_t
> >   *
> >   * Atomically increments @v by 1.
> >   */
> > -static inline void atomic_inc(atomic_t *v)
> > +static inline void raw_atomic_inc(atomic_t *v)
> >  {
> >  	asm volatile(LOCK_PREFIX "incl %0"
> >
> >  		     : "+m" (v->counter));
> >
> >  }
> >
> >  /**
> > - * atomic_dec - decrement atomic variable
> > + * raw_atomic_dec - decrement atomic variable
> >   * @v: pointer of type atomic_t
> >   *
> >   * Atomically decrements @v by 1.
> >   */
> > -static inline void atomic_dec(atomic_t *v)
> > +static inline void raw_atomic_dec(atomic_t *v)
> >  {
> >  	asm volatile(LOCK_PREFIX "decl %0"
> >
> >  		     : "+m" (v->counter));
> >
> > diff --git a/arch/x86/include/asm/atomic_64.h
> > b/arch/x86/include/asm/atomic_64.h index 8c21731..1183b85 100644
> > --- a/arch/x86/include/asm/atomic_64.h
> > +++ b/arch/x86/include/asm/atomic_64.h
> > @@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t
> > *v) }
> >
> >  /**
> > - * atomic_inc - increment atomic variable
> > + * raw_atomic_inc - increment atomic variable
> >   * @v: pointer of type atomic_t
> >   *
> >   * Atomically increments @v by 1.
> >   */
> > -static inline void atomic_inc(atomic_t *v)
> > +static inline void raw_atomic_inc(atomic_t *v)
> >  {
> >  	asm volatile(LOCK_PREFIX "incl %0"
> >
> >  		     : "=m" (v->counter)
> >
> > @@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
> >  }
> >
> >  /**
> > - * atomic_dec - decrement atomic variable
> > + * raw_atomic_dec - decrement atomic variable
> >   * @v: pointer of type atomic_t
> >   *
> >   * Atomically decrements @v by 1.
> >   */
> > -static inline void atomic_dec(atomic_t *v)
> > +static inline void raw_atomic_dec(atomic_t *v)
> >  {
> >  	asm volatile(LOCK_PREFIX "decl %0"
> >
> >  		     : "=m" (v->counter)
> >
> > diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
> > index 7abdaa9..6eda22b 100644
> > --- a/include/asm-generic/atomic.h
> > +++ b/include/asm-generic/atomic.h
> > @@ -4,15 +4,52 @@
> >   * Copyright (C) 2005 Silicon Graphics, Inc.
> >   *	Christoph Lameter
> >   *
> > - * Allows to provide arch independent atomic definitions without the
> > need to - * edit all arch specific atomic.h files.
> >   */
> >
> > +#include <linux/kernel.h>
> >  #include <asm/types.h>
> > +#include <asm/bug.h>
> > +
> > +#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
>
> #ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
>
> > +
> > +/**
> > + * atomic_inc - increment atomic variable
> > + * @v: pointer of type atomic_t
> > + *
> > + * Atomically increments @v by 1.
> > + * Prints a warning if it wraps around.
> > + */
> > +static inline void atomic_inc(atomic_t *v)
> > +{
> > +	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> > +}
> > +
> > +/**
> > + * atomic_dec - decrement atomic variable
> > + * @v: pointer of type atomic_t
> > + *
> > + * Atomically decrements @v by 1.
> > + * Prints a warning if it wraps around.
> > + */
> > +static inline void atomic_dec(atomic_t *v)
> > +{
> > +	WARN_ON(atomic_add_unless(v, -1, INT_MIN) == 0);
> > +}
> > +
> > +#else
> > +
> > +#define atomic_inc(v)	raw_atomic_inc(v)
> > +#define atomic_dec(v)	raw_atomic_dec(v)
>
> Please turn these into proper, type aware inline functions.
>

Ok, done.

> > +config ENABLE_WARN_ATOMIC_INC_WRAP
> > +	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
> > +	default y
> > +	help
> > +	  Enable printing a warning when atomic_inc() or atomic_dec()
> > +	  operation wraps around the 32-bit value.
> > +
>
> this needs HAVE_ARCH_DEBUG_ATOMIC added to arch/x86/Kconfig, and a
> depends on HAVE_ARCH_DEBUG_ATOMIC. Otherwise this wont build very
> well on non-x86 when enabled, right?
>

Ok, done.

> With those small details fixed:
>
> Acked-by: Ingo Molnar <mingo@elte.hu>
>
> 	Ingo

Here is the patch with all those changes incorporated.

Thanks
Nikanth

Detect and warn on atomic_inc/atomic_dec overflow.

Add a debug option to detect and warn when the 32-bit atomic_t overflows
during atomic_inc and atomic_dec.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
Acked-by: Ingo Molnar <mingo@elte.hu>

---

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..dd49be3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -46,6 +46,7 @@ config X86
 	select HAVE_KERNEL_GZIP
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
+	select HAVE_ARCH_DEBUG_ATOMIC
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
index 85b46fb..c6a17bb 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "+m" (v->counter));
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "+m" (v->counter));
diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
index 8c21731..1183b85 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "=m" (v->counter)
@@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "=m" (v->counter)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index 7abdaa9..9f476d7 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -4,15 +4,71 @@
  * Copyright (C) 2005 Silicon Graphics, Inc.
  *	Christoph Lameter
  *
- * Allows to provide arch independent atomic definitions without the need to
- * edit all arch specific atomic.h files.
  */
 
+#include <linux/kernel.h>
 #include <asm/types.h>
+#include <asm/bug.h>
+
+#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ * Prints a warning if it wraps around.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
+}
+
+/**
+ * atomic_dec - decrement atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ * Prints a warning if it wraps around.
+ */
+static inline void atomic_dec(atomic_t *v)
+{
+	WARN_ON(atomic_add_unless(v, -1, INT_MIN) == 0);
+}
+
+#else
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+	raw_atomic_inc(v);
+}
+
+/**
+ * atomic_dec - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ */
+static inline void atomic_dec(atomic_t *v)
+{
+	raw_atomic_dec(v);
+}
+
+#endif
+
 
 /*
  * Suppport for atomic_long_t
  *
+ * Allows to provide arch independent atomic definitions without the need to
+ * edit all arch specific atomic.h files.
+ *
  * Casts for parameters are avoided for existing atomic functions in order to
  * avoid issues with cast-as-lval under gcc 4.x and other limitations that the
  * macros of a platform may have.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 812c282..773c1a4 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -17,6 +17,17 @@ config ENABLE_WARN_DEPRECATED
 	  Disable this to suppress the "warning: 'foo' is deprecated
 	  (declared at kernel/power/somefile.c:1234)" messages.
 
+config HAVE_ARCH_DEBUG_ATOMIC
+	bool
+
+config ENABLE_WARN_ATOMIC_INC_WRAP
+	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
+	depends on HAVE_ARCH_DEBUG_ATOMIC
+	default y
+	help
+	  Enable printing a warning when atomic_inc() or atomic_dec()
+	  operation wraps around the 32-bit value.
+
 config ENABLE_MUST_CHECK
 	bool "Enable __must_check logic"
 	default y

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Ingo Molnar]

* Nikanth Karthikesan <knikanth@novell.com> wrote:

> On Thursday 30 April 2009 16:17:29 Ingo Molnar wrote:
> > * Nikanth Karthikesan <knikanth@novell.com> wrote:
> > > > As a generic debug helper this is not appropriate i think -
> > > > counts can easily have a meaning when going negative as well.
> > > > (we have no signed-atomic primitives)
> > >
> > > This doesn't warn when it becomes negative/positive from zero, but
> > > only when it wraps around^W^Woverflows, trying to add past INT_MAX
> > > or subtract from INT_MIN.
> >
> > ah! The small difference between INT_MAX and UINT_MAX.
> >
> > Sure, this debug check makes a lot of sense. -mm even had such
> > stuff, many years ago? Never went upstream.
> >
> > > > >  static inline void atomic_inc(atomic_t *v)
> > > > >  {
> > > > > +#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
> > > > > +	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> > > > > +#else
> > > > >  	asm volatile(LOCK_PREFIX "incl %0"
> > > > >
> > > > >  		     : "+m" (v->counter));
> > > > >
> > > > > +#endif
> > > > >  }
> > > >
> > > > also looks a bit ugly - this ugly #ifdef would spread into every
> > > > architecture.
> > > >
> > > > If we want to restrict atomic_t value ranges like that then the
> > > > clean solution would be to add generic wrappers doing the debug
> > > > (once, in generic code), and renaming the arch primitives to
> > > > raw_atomic_inc() (etc), doing the lowlevel bits cleanly.
> > >
> > > Here is a patch which does it this way.
> > >
> > > Thanks
> > > Nikanth
> > >
> > > Detect and warn on atomic_inc/atomic_dec overflow.
> > >
> > > Add a debug option to detect and warn when the 32-bit atomic_t overflows
> > > during atomic_inc and atomic_dec.
> > >
> > > diff --git a/arch/x86/include/asm/atomic_32.h
> > > b/arch/x86/include/asm/atomic_32.h index 85b46fb..c6a17bb 100644
> > > --- a/arch/x86/include/asm/atomic_32.h
> > > +++ b/arch/x86/include/asm/atomic_32.h
> > > @@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t
> > > *v) }
> > >
> > >  /**
> > > - * atomic_inc - increment atomic variable
> > > + * raw_atomic_inc - increment atomic variable
> > >   * @v: pointer of type atomic_t
> > >   *
> > >   * Atomically increments @v by 1.
> > >   */
> > > -static inline void atomic_inc(atomic_t *v)
> > > +static inline void raw_atomic_inc(atomic_t *v)
> > >  {
> > >  	asm volatile(LOCK_PREFIX "incl %0"
> > >
> > >  		     : "+m" (v->counter));
> > >
> > >  }
> > >
> > >  /**
> > > - * atomic_dec - decrement atomic variable
> > > + * raw_atomic_dec - decrement atomic variable
> > >   * @v: pointer of type atomic_t
> > >   *
> > >   * Atomically decrements @v by 1.
> > >   */
> > > -static inline void atomic_dec(atomic_t *v)
> > > +static inline void raw_atomic_dec(atomic_t *v)
> > >  {
> > >  	asm volatile(LOCK_PREFIX "decl %0"
> > >
> > >  		     : "+m" (v->counter));
> > >
> > > diff --git a/arch/x86/include/asm/atomic_64.h
> > > b/arch/x86/include/asm/atomic_64.h index 8c21731..1183b85 100644
> > > --- a/arch/x86/include/asm/atomic_64.h
> > > +++ b/arch/x86/include/asm/atomic_64.h
> > > @@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t
> > > *v) }
> > >
> > >  /**
> > > - * atomic_inc - increment atomic variable
> > > + * raw_atomic_inc - increment atomic variable
> > >   * @v: pointer of type atomic_t
> > >   *
> > >   * Atomically increments @v by 1.
> > >   */
> > > -static inline void atomic_inc(atomic_t *v)
> > > +static inline void raw_atomic_inc(atomic_t *v)
> > >  {
> > >  	asm volatile(LOCK_PREFIX "incl %0"
> > >
> > >  		     : "=m" (v->counter)
> > >
> > > @@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
> > >  }
> > >
> > >  /**
> > > - * atomic_dec - decrement atomic variable
> > > + * raw_atomic_dec - decrement atomic variable
> > >   * @v: pointer of type atomic_t
> > >   *
> > >   * Atomically decrements @v by 1.
> > >   */
> > > -static inline void atomic_dec(atomic_t *v)
> > > +static inline void raw_atomic_dec(atomic_t *v)
> > >  {
> > >  	asm volatile(LOCK_PREFIX "decl %0"
> > >
> > >  		     : "=m" (v->counter)
> > >
> > > diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
> > > index 7abdaa9..6eda22b 100644
> > > --- a/include/asm-generic/atomic.h
> > > +++ b/include/asm-generic/atomic.h
> > > @@ -4,15 +4,52 @@
> > >   * Copyright (C) 2005 Silicon Graphics, Inc.
> > >   *	Christoph Lameter
> > >   *
> > > - * Allows to provide arch independent atomic definitions without the
> > > need to - * edit all arch specific atomic.h files.
> > >   */
> > >
> > > +#include <linux/kernel.h>
> > >  #include <asm/types.h>
> > > +#include <asm/bug.h>
> > > +
> > > +#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
> >
> > #ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP

you missed this feedback :-)

	Ingo

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Nikanth Karthikesan]

On Thursday 30 April 2009 17:51:59 Ingo Molnar wrote:
> * Nikanth Karthikesan <knikanth@novell.com> wrote:
> > On Thursday 30 April 2009 16:17:29 Ingo Molnar wrote:
> > > * Nikanth Karthikesan <knikanth@novell.com> wrote:
> > > > > As a generic debug helper this is not appropriate i think -
> > > > > counts can easily have a meaning when going negative as well.
> > > > > (we have no signed-atomic primitives)
> > > >
> > > > This doesn't warn when it becomes negative/positive from zero, but
> > > > only when it wraps around^W^Woverflows, trying to add past INT_MAX
> > > > or subtract from INT_MIN.
> > >
> > > ah! The small difference between INT_MAX and UINT_MAX.
> > >
> > > Sure, this debug check makes a lot of sense. -mm even had such
> > > stuff, many years ago? Never went upstream.
> > >
> > > > > >  static inline void atomic_inc(atomic_t *v)
> > > > > >  {
> > > > > > +#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
> > > > > > +	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> > > > > > +#else
> > > > > >  	asm volatile(LOCK_PREFIX "incl %0"
> > > > > >
> > > > > >  		     : "+m" (v->counter));
> > > > > >
> > > > > > +#endif
> > > > > >  }
> > > > >
> > > > > also looks a bit ugly - this ugly #ifdef would spread into every
> > > > > architecture.
> > > > >
> > > > > If we want to restrict atomic_t value ranges like that then the
> > > > > clean solution would be to add generic wrappers doing the debug
> > > > > (once, in generic code), and renaming the arch primitives to
> > > > > raw_atomic_inc() (etc), doing the lowlevel bits cleanly.
> > > >
> > > > Here is a patch which does it this way.
> > > >
> > > > Thanks
> > > > Nikanth
> > > >
> > > > Detect and warn on atomic_inc/atomic_dec overflow.
> > > >
> > > > Add a debug option to detect and warn when the 32-bit atomic_t
> > > > overflows during atomic_inc and atomic_dec.
> > > >
> > > > diff --git a/arch/x86/include/asm/atomic_32.h
> > > > b/arch/x86/include/asm/atomic_32.h index 85b46fb..c6a17bb 100644
> > > > --- a/arch/x86/include/asm/atomic_32.h
> > > > +++ b/arch/x86/include/asm/atomic_32.h
> > > > @@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i,
> > > > atomic_t *v) }
> > > >
> > > >  /**
> > > > - * atomic_inc - increment atomic variable
> > > > + * raw_atomic_inc - increment atomic variable
> > > >   * @v: pointer of type atomic_t
> > > >   *
> > > >   * Atomically increments @v by 1.
> > > >   */
> > > > -static inline void atomic_inc(atomic_t *v)
> > > > +static inline void raw_atomic_inc(atomic_t *v)
> > > >  {
> > > >  	asm volatile(LOCK_PREFIX "incl %0"
> > > >
> > > >  		     : "+m" (v->counter));
> > > >
> > > >  }
> > > >
> > > >  /**
> > > > - * atomic_dec - decrement atomic variable
> > > > + * raw_atomic_dec - decrement atomic variable
> > > >   * @v: pointer of type atomic_t
> > > >   *
> > > >   * Atomically decrements @v by 1.
> > > >   */
> > > > -static inline void atomic_dec(atomic_t *v)
> > > > +static inline void raw_atomic_dec(atomic_t *v)
> > > >  {
> > > >  	asm volatile(LOCK_PREFIX "decl %0"
> > > >
> > > >  		     : "+m" (v->counter));
> > > >
> > > > diff --git a/arch/x86/include/asm/atomic_64.h
> > > > b/arch/x86/include/asm/atomic_64.h index 8c21731..1183b85 100644
> > > > --- a/arch/x86/include/asm/atomic_64.h
> > > > +++ b/arch/x86/include/asm/atomic_64.h
> > > > @@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i,
> > > > atomic_t *v) }
> > > >
> > > >  /**
> > > > - * atomic_inc - increment atomic variable
> > > > + * raw_atomic_inc - increment atomic variable
> > > >   * @v: pointer of type atomic_t
> > > >   *
> > > >   * Atomically increments @v by 1.
> > > >   */
> > > > -static inline void atomic_inc(atomic_t *v)
> > > > +static inline void raw_atomic_inc(atomic_t *v)
> > > >  {
> > > >  	asm volatile(LOCK_PREFIX "incl %0"
> > > >
> > > >  		     : "=m" (v->counter)
> > > >
> > > > @@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
> > > >  }
> > > >
> > > >  /**
> > > > - * atomic_dec - decrement atomic variable
> > > > + * raw_atomic_dec - decrement atomic variable
> > > >   * @v: pointer of type atomic_t
> > > >   *
> > > >   * Atomically decrements @v by 1.
> > > >   */
> > > > -static inline void atomic_dec(atomic_t *v)
> > > > +static inline void raw_atomic_dec(atomic_t *v)
> > > >  {
> > > >  	asm volatile(LOCK_PREFIX "decl %0"
> > > >
> > > >  		     : "=m" (v->counter)
> > > >
> > > > diff --git a/include/asm-generic/atomic.h
> > > > b/include/asm-generic/atomic.h index 7abdaa9..6eda22b 100644
> > > > --- a/include/asm-generic/atomic.h
> > > > +++ b/include/asm-generic/atomic.h
> > > > @@ -4,15 +4,52 @@
> > > >   * Copyright (C) 2005 Silicon Graphics, Inc.
> > > >   *	Christoph Lameter
> > > >   *
> > > > - * Allows to provide arch independent atomic definitions without the
> > > > need to - * edit all arch specific atomic.h files.
> > > >   */
> > > >
> > > > +#include <linux/kernel.h>
> > > >  #include <asm/types.h>
> > > > +#include <asm/bug.h>
> > > > +
> > > > +#if defined(CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP)
> > >
> > > #ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
>
> you missed this feedback :-)
>

Yes, sorry. Fixed Now. Thanks for the review. :-)

Thanks
Nikanth

Detect and warn on atomic_inc/atomic_dec overflow.

Add a debug option to detect and warn when the 32-bit atomic_t overflows
during atomic_inc and atomic_dec.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
Acked-by: Ingo Molnar <mingo@elte.hu>

---

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..dd49be3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -46,6 +46,7 @@ config X86
 	select HAVE_KERNEL_GZIP
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
+	select HAVE_ARCH_DEBUG_ATOMIC
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
index 85b46fb..c6a17bb 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "+m" (v->counter));
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "+m" (v->counter));
diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
index 8c21731..1183b85 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "=m" (v->counter)
@@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "=m" (v->counter)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index 7abdaa9..edf5619 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -4,15 +4,71 @@
  * Copyright (C) 2005 Silicon Graphics, Inc.
  *	Christoph Lameter
  *
- * Allows to provide arch independent atomic definitions without the need to
- * edit all arch specific atomic.h files.
  */
 
+#include <linux/kernel.h>
 #include <asm/types.h>
+#include <asm/bug.h>
+
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ * Prints a warning if it wraps around.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
+}
+
+/**
+ * atomic_dec - decrement atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ * Prints a warning if it wraps around.
+ */
+static inline void atomic_dec(atomic_t *v)
+{
+	WARN_ON(atomic_add_unless(v, -1, INT_MIN) == 0);
+}
+
+#else
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+	raw_atomic_inc(v);
+}
+
+/**
+ * atomic_dec - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ */
+static inline void atomic_dec(atomic_t *v)
+{
+	raw_atomic_dec(v);
+}
+
+#endif
+
 
 /*
  * Suppport for atomic_long_t
  *
+ * Allows to provide arch independent atomic definitions without the need to
+ * edit all arch specific atomic.h files.
+ *
  * Casts for parameters are avoided for existing atomic functions in order to
  * avoid issues with cast-as-lval under gcc 4.x and other limitations that the
  * macros of a platform may have.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 812c282..773c1a4 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -17,6 +17,17 @@ config ENABLE_WARN_DEPRECATED
 	  Disable this to suppress the "warning: 'foo' is deprecated
 	  (declared at kernel/power/somefile.c:1234)" messages.
 
+config HAVE_ARCH_DEBUG_ATOMIC
+	bool
+
+config ENABLE_WARN_ATOMIC_INC_WRAP
+	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
+	depends on HAVE_ARCH_DEBUG_ATOMIC
+	default y
+	help
+	  Enable printing a warning when atomic_inc() or atomic_dec()
+	  operation wraps around the 32-bit value.
+
 config ENABLE_MUST_CHECK
 	bool "Enable __must_check logic"
 	default y

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Ingo Molnar]

* Nikanth Karthikesan <knikanth@novell.com> wrote:

> diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
> index 7abdaa9..edf5619 100644
> --- a/include/asm-generic/atomic.h
> +++ b/include/asm-generic/atomic.h
> @@ -4,15 +4,71 @@
>   * Copyright (C) 2005 Silicon Graphics, Inc.
>   *	Christoph Lameter
>   *
> - * Allows to provide arch independent atomic definitions without the need to
> - * edit all arch specific atomic.h files.
>   */
>  
> +#include <linux/kernel.h>
>  #include <asm/types.h>
> +#include <asm/bug.h>
> +
> +#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
> +
> +/**
> + * atomic_inc - increment atomic variable
> + * @v: pointer of type atomic_t
> + *
> + * Atomically increments @v by 1.
> + * Prints a warning if it wraps around.
> + */
> +static inline void atomic_inc(atomic_t *v)
> +{
> +	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> +}
> +
> +/**
> + * atomic_dec - decrement atomic variable
> + * @v: pointer of type atomic_t
> + *
> + * Atomically decrements @v by 1.
> + * Prints a warning if it wraps around.
> + */
> +static inline void atomic_dec(atomic_t *v)
> +{
> +	WARN_ON(atomic_add_unless(v, -1, INT_MIN) == 0);
> +}
> +
> +#else
> +
> +/**
> + * atomic_inc - increment atomic variable
> + * @v: pointer of type atomic_t
> + *
> + * Atomically increments @v by 1.
> + */
> +static inline void atomic_inc(atomic_t *v)
> +{
> +	raw_atomic_inc(v);
> +}
> +
> +/**
> + * atomic_dec - increment atomic variable
> + * @v: pointer of type atomic_t
> + *
> + * Atomically decrements @v by 1.
> + */
> +static inline void atomic_dec(atomic_t *v)
> +{
> +	raw_atomic_dec(v);
> +}
> +
> +#endif

Here i think it makes sense to have a single definition:

static inline void atomic_inc(atomic_t *v)
{
#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
#else
	raw_atomic_inc(v);
#endif
}

Or, better yet, i'd suggest to define an 'invalid value' range for 
atomic integer types - not restricted to the single value of 
INT_MAX, but in the range of: [ UINT_MAX/4 ... INT_MAX/4*3 ].

Then there could be a single, straightforward value check:

static inline void atomic_inc(atomic_t *v)
{
	debug_atomic_check_value(v);
	raw_atomic_inc(v);
}

Where debug_atomic_check_value() is just an atomic_read():

static inline void debug_atomic_check_value(atomic_t *v)
{
	WARN_ONCE(in_range(atomic_read(v), UINT_MAX/4, UINT_MAX/4*3),
		  KERN_ERR "atomic counter check failure!");
}

It's a constant check.

If are overflowing on such a massive rate, it doesnt matter how 
early or late we check the value.

Agreed?

	Ingo

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Nikanth Karthikesan]

On Thursday 30 April 2009 18:20:04 Ingo Molnar wrote:
> * Nikanth Karthikesan <knikanth@novell.com> wrote:
> > diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
> > index 7abdaa9..edf5619 100644
> > --- a/include/asm-generic/atomic.h
> > +++ b/include/asm-generic/atomic.h
> > @@ -4,15 +4,71 @@
> >   * Copyright (C) 2005 Silicon Graphics, Inc.
> >   *	Christoph Lameter
> >   *
> > - * Allows to provide arch independent atomic definitions without the
> > need to - * edit all arch specific atomic.h files.
> >   */
> >
> > +#include <linux/kernel.h>
> >  #include <asm/types.h>
> > +#include <asm/bug.h>
> > +
> > +#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
> > +
> > +/**
> > + * atomic_inc - increment atomic variable
> > + * @v: pointer of type atomic_t
> > + *
> > + * Atomically increments @v by 1.
> > + * Prints a warning if it wraps around.
> > + */
> > +static inline void atomic_inc(atomic_t *v)
> > +{
> > +	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> > +}
> > +
> > +/**
> > + * atomic_dec - decrement atomic variable
> > + * @v: pointer of type atomic_t
> > + *
> > + * Atomically decrements @v by 1.
> > + * Prints a warning if it wraps around.
> > + */
> > +static inline void atomic_dec(atomic_t *v)
> > +{
> > +	WARN_ON(atomic_add_unless(v, -1, INT_MIN) == 0);
> > +}
> > +
> > +#else
> > +
> > +/**
> > + * atomic_inc - increment atomic variable
> > + * @v: pointer of type atomic_t
> > + *
> > + * Atomically increments @v by 1.
> > + */
> > +static inline void atomic_inc(atomic_t *v)
> > +{
> > +	raw_atomic_inc(v);
> > +}
> > +
> > +/**
> > + * atomic_dec - increment atomic variable
> > + * @v: pointer of type atomic_t
> > + *
> > + * Atomically decrements @v by 1.
> > + */
> > +static inline void atomic_dec(atomic_t *v)
> > +{
> > +	raw_atomic_dec(v);
> > +}
> > +
> > +#endif
>
> Here i think it makes sense to have a single definition:
>
> static inline void atomic_inc(atomic_t *v)
> {
> #ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
> 	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
> #else
> 	raw_atomic_inc(v);
> #endif
> }
>

Ok, done.

> Or, better yet, i'd suggest to define an 'invalid value' range for
> atomic integer types - not restricted to the single value of
> INT_MAX, but in the range of: [ UINT_MAX/4 ... INT_MAX/4*3 ].
>

atomic_t is defined an int, why use UINT_MAX?

> Then there could be a single, straightforward value check:
>
> static inline void atomic_inc(atomic_t *v)
> {
> 	debug_atomic_check_value(v);
> 	raw_atomic_inc(v);
> }
>
> Where debug_atomic_check_value() is just an atomic_read():
>
> static inline void debug_atomic_check_value(atomic_t *v)
> {
> 	WARN_ONCE(in_range(atomic_read(v), UINT_MAX/4, UINT_MAX/4*3),
> 		  KERN_ERR "atomic counter check failure!");
> }
>

I do not understand, why UINT_MAX/4 to UINT_MAX/4*3?
Roughly,
UINT_MAX/4 = INT_MAX/2 
UINT_MAX/4*3 = INT_MAX/2*3 which we will never reach with an int.

> It's a constant check.
>
> If are overflowing on such a massive rate, it doesnt matter how
> early or late we check the value.
>

UINT_MAX/4 early, might be too early. And if it doesn't matter how early or
late, why try to be over-cautious and produce false warnings. ;-)

> Agreed?
>

Hmm.. partly yes, but not the range. Could the range be made as INT_MAX/4*3 to
INT_MAX? But even then, this might trigger false-positives, when atomic_t is
used in places where it was meant to exercise the full int range with the
assumption it would never go above INT_MAX.

If you still think that, it would be better to catch it early, I would send
you another patch with the range checking incorporated.

Thanks
Nikanth

Detect and warn on atomic_inc/atomic_dec overflow.

Add a debug option to detect and warn when the 32-bit atomic_t overflows
during atomic_inc and atomic_dec.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
Acked-by: Ingo Molnar <mingo@elte.hu>

---

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..dd49be3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -46,6 +46,7 @@ config X86
 	select HAVE_KERNEL_GZIP
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
+	select HAVE_ARCH_DEBUG_ATOMIC
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
index 85b46fb..c6a17bb 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "+m" (v->counter));
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "+m" (v->counter));
diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
index 8c21731..1183b85 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "=m" (v->counter)
@@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "=m" (v->counter)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index 7abdaa9..ca47932 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -4,15 +4,52 @@
  * Copyright (C) 2005 Silicon Graphics, Inc.
  *	Christoph Lameter
  *
- * Allows to provide arch independent atomic definitions without the need to
- * edit all arch specific atomic.h files.
  */
 
+#include <linux/kernel.h>
 #include <asm/types.h>
+#include <asm/bug.h>
+
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+	WARN_ON(atomic_add_unless(v, 1, INT_MAX) == 0);
+#else
+	raw_atomic_inc(v);
+#endif
+}
+
+/**
+ * atomic_dec - decrement atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ * Prints a warning if it wraps around.
+ */
+static inline void atomic_dec(atomic_t *v)
+{
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+	WARN_ON(atomic_add_unless(v, -1, INT_MIN) == 0);
+#else
+	raw_atomic_dec(v);
+#endif
+
+}
+
 
 /*
  * Suppport for atomic_long_t
  *
+ * Allows to provide arch independent atomic definitions without the need to
+ * edit all arch specific atomic.h files.
+ *
  * Casts for parameters are avoided for existing atomic functions in order to
  * avoid issues with cast-as-lval under gcc 4.x and other limitations that the
  * macros of a platform may have.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 812c282..773c1a4 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -17,6 +17,17 @@ config ENABLE_WARN_DEPRECATED
 	  Disable this to suppress the "warning: 'foo' is deprecated
 	  (declared at kernel/power/somefile.c:1234)" messages.
 
+config HAVE_ARCH_DEBUG_ATOMIC
+	bool
+
+config ENABLE_WARN_ATOMIC_INC_WRAP
+	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
+	depends on HAVE_ARCH_DEBUG_ATOMIC
+	default y
+	help
+	  Enable printing a warning when atomic_inc() or atomic_dec()
+	  operation wraps around the 32-bit value.
+
 config ENABLE_MUST_CHECK
 	bool "Enable __must_check logic"
 	default y

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Ingo Molnar]

* Nikanth Karthikesan <knikanth@novell.com> wrote:

> > Then there could be a single, straightforward value check:
> >
> > static inline void atomic_inc(atomic_t *v)
> > {
> > 	debug_atomic_check_value(v);
> > 	raw_atomic_inc(v);
> > }
> >
> > Where debug_atomic_check_value() is just an atomic_read():
> >
> > static inline void debug_atomic_check_value(atomic_t *v)
> > {
> > 	WARN_ONCE(in_range(atomic_read(v), UINT_MAX/4, UINT_MAX/4*3),
> > 		  KERN_ERR "atomic counter check failure!");
> > }
> >
> 
> I do not understand, why UINT_MAX/4 to UINT_MAX/4*3?
> Roughly,
> UINT_MAX/4 = INT_MAX/2 
> UINT_MAX/4*3 = INT_MAX/2*3 which we will never reach with an int.

i mean:

 	WARN_ONCE(in_range((u32)atomic_read(v), UINT_MAX/4, UINT_MAX/4*3),
 		  KERN_ERR "atomic counter check failure!");

that's a single range check on an u32, selecting 'too large' and 
'too small' s32 values.

> > It's a constant check.
> >
> > If are overflowing on such a massive rate, it doesnt matter how 
> > early or late we check the value.
> 
> UINT_MAX/4 early, might be too early. And if it doesn't matter how 
> early or late, why try to be over-cautious and produce false 
> warnings. ;-)

UINT_MAX/4 is ~1 billion. If we reach a value of 1 billion we are 
leaking. Your check basically is a sharp test for the specific case 
of overflowing the boundary - but it makes the code slower (it uses 
more complex atomic ops) and uglifies it via #ifdefs as well.

It doesnt matter whether we wrap over at around +2 billion into -2 
billion, or treat the whole above-1-billion and 
below-minus-1-billion range as invalid. (other than we'll catch bugs 
sooner via this method, and have faster and cleaner code)

	Ingo

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Nikanth Karthikesan]

On Thursday 30 April 2009 19:07:57 Ingo Molnar wrote:
> * Nikanth Karthikesan <knikanth@novell.com> wrote:
> > > Then there could be a single, straightforward value check:
> > >
> > > static inline void atomic_inc(atomic_t *v)
> > > {
> > > 	debug_atomic_check_value(v);
> > > 	raw_atomic_inc(v);
> > > }
> > >
> > > Where debug_atomic_check_value() is just an atomic_read():
> > >
> > > static inline void debug_atomic_check_value(atomic_t *v)
> > > {
> > > 	WARN_ONCE(in_range(atomic_read(v), UINT_MAX/4, UINT_MAX/4*3),
> > > 		  KERN_ERR "atomic counter check failure!");
> > > }
> >
> > I do not understand, why UINT_MAX/4 to UINT_MAX/4*3?
> > Roughly,
> > UINT_MAX/4 = INT_MAX/2
> > UINT_MAX/4*3 = INT_MAX/2*3 which we will never reach with an int.
>
> i mean:
>
>  	WARN_ONCE(in_range((u32)atomic_read(v), UINT_MAX/4, UINT_MAX/4*3),
>  		  KERN_ERR "atomic counter check failure!");
>
> that's a single range check on an u32, selecting 'too large' and
> 'too small' s32 values.
>
> > > It's a constant check.
> > >
> > > If are overflowing on such a massive rate, it doesnt matter how
> > > early or late we check the value.
> >
> > UINT_MAX/4 early, might be too early. And if it doesn't matter how
> > early or late, why try to be over-cautious and produce false
> > warnings. ;-)
>
> UINT_MAX/4 is ~1 billion. If we reach a value of 1 billion we are
> leaking. Your check basically is a sharp test for the specific case
> of overflowing the boundary - but it makes the code slower (it uses
> more complex atomic ops) and uglifies it via #ifdefs as well.
>
> It doesnt matter whether we wrap over at around +2 billion into -2
> billion, or treat the whole above-1-billion and
> below-minus-1-billion range as invalid. (other than we'll catch bugs
> sooner via this method, and have faster and cleaner code)
>

Ah.. got it. But, range checking is not required as we are just verifying it
during increment and decrement, not atomic_add, atomic_sub etc... Should we
add debug checks to those operations as well? If we want to test those
operations as well, range check would be useful.

Here is a patch, without the overhead of a costly atomic operation which would
warn if it goes out of [INT_MIN/2 .. INT_MAX/2].

Thanks
Nikanth


Detect and warn on atomic_inc/atomic_dec overflow.

Add a debug option to detect and warn when the 32-bit atomic_t overflows
during atomic_inc and atomic_dec.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
Acked-by: Ingo Molnar <mingo@elte.hu>

---


diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..dd49be3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -46,6 +46,7 @@ config X86
 	select HAVE_KERNEL_GZIP
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
+	select HAVE_ARCH_DEBUG_ATOMIC
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
index 85b46fb..c6a17bb 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "+m" (v->counter));
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "+m" (v->counter));
diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
index 8c21731..1183b85 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "=m" (v->counter)
@@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "=m" (v->counter)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index 7abdaa9..007931a 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -4,15 +4,51 @@
  * Copyright (C) 2005 Silicon Graphics, Inc.
  *	Christoph Lameter
  *
- * Allows to provide arch independent atomic definitions without the need to
- * edit all arch specific atomic.h files.
  */
 
+#include <linux/kernel.h>
 #include <asm/types.h>
+#include <asm/bug.h>
+
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+	WARN_ONCE((atomic_read(v) > (INT_MAX / 2)),
+		KERN_ERR "atomic counter check failure!");
+#endif
+	raw_atomic_inc(v);
+}
+
+/**
+ * atomic_dec - decrement atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ */
+static inline void atomic_dec(atomic_t *v)
+{
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+	WARN_ONCE((atomic_read(v) < (INT_MIN / 2)),
+		KERN_ERR "atomic counter check failure!");
+#endif
+	raw_atomic_dec(v);
+
+}
+
 
 /*
  * Suppport for atomic_long_t
  *
+ * Allows to provide arch independent atomic definitions without the need to
+ * edit all arch specific atomic.h files.
+ *
  * Casts for parameters are avoided for existing atomic functions in order to
  * avoid issues with cast-as-lval under gcc 4.x and other limitations that the
  * macros of a platform may have.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 812c282..773c1a4 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -17,6 +17,17 @@ config ENABLE_WARN_DEPRECATED
 	  Disable this to suppress the "warning: 'foo' is deprecated
 	  (declared at kernel/power/somefile.c:1234)" messages.
 
+config HAVE_ARCH_DEBUG_ATOMIC
+	bool
+
+config ENABLE_WARN_ATOMIC_INC_WRAP
+	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
+	depends on HAVE_ARCH_DEBUG_ATOMIC
+	default y
+	help
+	  Enable printing a warning when atomic_inc() or atomic_dec()
+	  operation wraps around the 32-bit value.
+
 config ENABLE_MUST_CHECK
 	bool "Enable __must_check logic"
 	default y

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Ingo Molnar]

* Nikanth Karthikesan <knikanth@novell.com> wrote:

> On Thursday 30 April 2009 19:07:57 Ingo Molnar wrote:
> > * Nikanth Karthikesan <knikanth@novell.com> wrote:
> > > > Then there could be a single, straightforward value check:
> > > >
> > > > static inline void atomic_inc(atomic_t *v)
> > > > {
> > > > 	debug_atomic_check_value(v);
> > > > 	raw_atomic_inc(v);
> > > > }
> > > >
> > > > Where debug_atomic_check_value() is just an atomic_read():
> > > >
> > > > static inline void debug_atomic_check_value(atomic_t *v)
> > > > {
> > > > 	WARN_ONCE(in_range(atomic_read(v), UINT_MAX/4, UINT_MAX/4*3),
> > > > 		  KERN_ERR "atomic counter check failure!");
> > > > }
> > >
> > > I do not understand, why UINT_MAX/4 to UINT_MAX/4*3?
> > > Roughly,
> > > UINT_MAX/4 = INT_MAX/2
> > > UINT_MAX/4*3 = INT_MAX/2*3 which we will never reach with an int.
> >
> > i mean:
> >
> >  	WARN_ONCE(in_range((u32)atomic_read(v), UINT_MAX/4, UINT_MAX/4*3),
> >  		  KERN_ERR "atomic counter check failure!");
> >
> > that's a single range check on an u32, selecting 'too large' and
> > 'too small' s32 values.
> >
> > > > It's a constant check.
> > > >
> > > > If are overflowing on such a massive rate, it doesnt matter how
> > > > early or late we check the value.
> > >
> > > UINT_MAX/4 early, might be too early. And if it doesn't matter how
> > > early or late, why try to be over-cautious and produce false
> > > warnings. ;-)
> >
> > UINT_MAX/4 is ~1 billion. If we reach a value of 1 billion we are
> > leaking. Your check basically is a sharp test for the specific case
> > of overflowing the boundary - but it makes the code slower (it uses
> > more complex atomic ops) and uglifies it via #ifdefs as well.
> >
> > It doesnt matter whether we wrap over at around +2 billion into -2
> > billion, or treat the whole above-1-billion and
> > below-minus-1-billion range as invalid. (other than we'll catch bugs
> > sooner via this method, and have faster and cleaner code)
> >
> 
> Ah.. got it. But, range checking is not required as we are just 
> verifying it during increment and decrement, not atomic_add, 
> atomic_sub etc... Should we add debug checks to those operations 
> as well? If we want to test those operations as well, range check 
> would be useful.

Good point! Indeed the checks can be even simpler that way - a 
single test.

> Here is a patch, without the overhead of a costly atomic operation 
> which would warn if it goes out of [INT_MIN/2 .. INT_MAX/2].

> +static inline void atomic_inc(atomic_t *v)
> +{
> +#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
> +	WARN_ONCE((atomic_read(v) > (INT_MAX / 2)),
> +		KERN_ERR "atomic counter check failure!");

here the message can be more specific i think:

		KERN_ERR "atomic inc overflow!");

> +#endif
> +	raw_atomic_inc(v);
> +}
> +
> +/**
> + * atomic_dec - decrement atomic variable
> + * @v: pointer of type atomic_t
> + *
> + * Atomically decrements @v by 1.
> + */
> +static inline void atomic_dec(atomic_t *v)
> +{
> +#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
> +	WARN_ONCE((atomic_read(v) < (INT_MIN / 2)),
> +		KERN_ERR "atomic counter check failure!");
> +#endif

and here:

		KERN_ERR "atomic inc underflow!");

other than these two small details this is looking really nice now.

	Ingo

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Nikanth Karthikesan]

On Thursday 30 April 2009 19:35:59 Ingo Molnar wrote:
> * Nikanth Karthikesan <knikanth@novell.com> wrote:
> > On Thursday 30 April 2009 19:07:57 Ingo Molnar wrote:
> > > * Nikanth Karthikesan <knikanth@novell.com> wrote:
> > > > > Then there could be a single, straightforward value check:
> > > > >
> > > > > static inline void atomic_inc(atomic_t *v)
> > > > > {
> > > > > 	debug_atomic_check_value(v);
> > > > > 	raw_atomic_inc(v);
> > > > > }
> > > > >
> > > > > Where debug_atomic_check_value() is just an atomic_read():
> > > > >
> > > > > static inline void debug_atomic_check_value(atomic_t *v)
> > > > > {
> > > > > 	WARN_ONCE(in_range(atomic_read(v), UINT_MAX/4, UINT_MAX/4*3),
> > > > > 		  KERN_ERR "atomic counter check failure!");
> > > > > }
> > > >
> > > > I do not understand, why UINT_MAX/4 to UINT_MAX/4*3?
> > > > Roughly,
> > > > UINT_MAX/4 = INT_MAX/2
> > > > UINT_MAX/4*3 = INT_MAX/2*3 which we will never reach with an int.
> > >
> > > i mean:
> > >
> > >  	WARN_ONCE(in_range((u32)atomic_read(v), UINT_MAX/4, UINT_MAX/4*3),
> > >  		  KERN_ERR "atomic counter check failure!");
> > >
> > > that's a single range check on an u32, selecting 'too large' and
> > > 'too small' s32 values.
> > >
> > > > > It's a constant check.
> > > > >
> > > > > If are overflowing on such a massive rate, it doesnt matter how
> > > > > early or late we check the value.
> > > >
> > > > UINT_MAX/4 early, might be too early. And if it doesn't matter how
> > > > early or late, why try to be over-cautious and produce false
> > > > warnings. ;-)
> > >
> > > UINT_MAX/4 is ~1 billion. If we reach a value of 1 billion we are
> > > leaking. Your check basically is a sharp test for the specific case
> > > of overflowing the boundary - but it makes the code slower (it uses
> > > more complex atomic ops) and uglifies it via #ifdefs as well.
> > >
> > > It doesnt matter whether we wrap over at around +2 billion into -2
> > > billion, or treat the whole above-1-billion and
> > > below-minus-1-billion range as invalid. (other than we'll catch bugs
> > > sooner via this method, and have faster and cleaner code)
> >
> > Ah.. got it. But, range checking is not required as we are just
> > verifying it during increment and decrement, not atomic_add,
> > atomic_sub etc... Should we add debug checks to those operations
> > as well? If we want to test those operations as well, range check
> > would be useful.
>
> Good point! Indeed the checks can be even simpler that way - a
> single test.
>
> > Here is a patch, without the overhead of a costly atomic operation
> > which would warn if it goes out of [INT_MIN/2 .. INT_MAX/2].
> >
> > +static inline void atomic_inc(atomic_t *v)
> > +{
> > +#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
> > +	WARN_ONCE((atomic_read(v) > (INT_MAX / 2)),
> > +		KERN_ERR "atomic counter check failure!");
>
> here the message can be more specific i think:
>
> 		KERN_ERR "atomic inc overflow!");
>
> > +#endif
> > +	raw_atomic_inc(v);
> > +}
> > +
> > +/**
> > + * atomic_dec - decrement atomic variable
> > + * @v: pointer of type atomic_t
> > + *
> > + * Atomically decrements @v by 1.
> > + */
> > +static inline void atomic_dec(atomic_t *v)
> > +{
> > +#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
> > +	WARN_ONCE((atomic_read(v) < (INT_MIN / 2)),
> > +		KERN_ERR "atomic counter check failure!");
> > +#endif
>
> and here:
>
> 		KERN_ERR "atomic inc underflow!");
>
> other than these two small details this is looking really nice now.
>

Done.

Thanks
Nikanth

Detect and warn on atomic_inc/atomic_dec overflow.

Add a debug option to detect and warn when the 32-bit atomic_t overflows
during atomic_inc and atomic_dec.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
Acked-by: Ingo Molnar <mingo@elte.hu>

---

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..dd49be3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -46,6 +46,7 @@ config X86
 	select HAVE_KERNEL_GZIP
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
+	select HAVE_ARCH_DEBUG_ATOMIC
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/x86/include/asm/atomic_32.h 
b/arch/x86/include/asm/atomic_32.h
index 85b46fb..c6a17bb 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "+m" (v->counter));
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "+m" (v->counter));
diff --git a/arch/x86/include/asm/atomic_64.h 
b/arch/x86/include/asm/atomic_64.h
index 8c21731..1183b85 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "=m" (v->counter)
@@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "=m" (v->counter)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index 7abdaa9..e0ffa32 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -4,15 +4,51 @@
  * Copyright (C) 2005 Silicon Graphics, Inc.
  *	Christoph Lameter
  *
- * Allows to provide arch independent atomic definitions without the need to
- * edit all arch specific atomic.h files.
  */
 
+#include <linux/kernel.h>
 #include <asm/types.h>
+#include <asm/bug.h>
+
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+	WARN_ONCE((atomic_read(v) > (INT_MAX / 2)),
+		KERN_ERR "atomic inc overflow!");
+#endif
+	raw_atomic_inc(v);
+}
+
+/**
+ * atomic_dec - decrement atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ */
+static inline void atomic_dec(atomic_t *v)
+{
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+	WARN_ONCE((atomic_read(v) < (INT_MIN / 2)),
+		KERN_ERR "atomic dec underflow!");
+#endif
+	raw_atomic_dec(v);
+
+}
+
 
 /*
  * Suppport for atomic_long_t
  *
+ * Allows to provide arch independent atomic definitions without the need to
+ * edit all arch specific atomic.h files.
+ *
  * Casts for parameters are avoided for existing atomic functions in order to
  * avoid issues with cast-as-lval under gcc 4.x and other limitations that 
the
  * macros of a platform may have.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 812c282..773c1a4 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -17,6 +17,17 @@ config ENABLE_WARN_DEPRECATED
 	  Disable this to suppress the "warning: 'foo' is deprecated
 	  (declared at kernel/power/somefile.c:1234)" messages.
 
+config HAVE_ARCH_DEBUG_ATOMIC
+	bool
+
+config ENABLE_WARN_ATOMIC_INC_WRAP
+	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
+	depends on HAVE_ARCH_DEBUG_ATOMIC
+	default y
+	help
+	  Enable printing a warning when atomic_inc() or atomic_dec()
+	  operation wraps around the 32-bit value.
+
 config ENABLE_MUST_CHECK
 	bool "Enable __must_check logic"
 	default y

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Ingo Molnar]

* Nikanth Karthikesan <knikanth@novell.com> wrote:

> > and here:
> >
> > 		KERN_ERR "atomic inc underflow!");
> >
> > other than these two small details this is looking really nice now.
> >
> 
> Done.

Thanks. Looks unconditionally good now.

Andrew, this looks like it fits -mm's profile best - can you see any 
other problems in the patch? (Perhaps CONFIG_VM_DEBUG should 
auto-select this option?)

	Ingo

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Andrew Morton]

On Thu, 30 Apr 2009 19:39:50 +0530
Nikanth Karthikesan <knikanth@novell.com> wrote:

> 

If I had a dollar for each wordwrapped patch I get sent...

> Detect and warn on atomic_inc/atomic_dec overflow.
> 
> Add a debug option to detect and warn when the 32-bit atomic_t overflows
> during atomic_inc and atomic_dec.
> 

OK.  

I'll beef the changelog up a bit - this one is wimpy.

The question is: do we put this in mainline?  I guess we might as well
give it a shot.  It may well find bugs and it might also trigger false
positives.  We can then fix the bugs and decide whether the false
positives warrant reverting it again, all very easy.

> +#include <asm/bug.h>

checkpatch says

WARNING: Use #include <linux/bug.h> instead of <asm/bug.h>
#215: FILE: include/asm-generic/atomic.h:11:
+#include <asm/bug.h>

Was this an oversight, or did you try using linux/bug.h and discovered
some problem?

> index 812c282..773c1a4 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -17,6 +17,17 @@ config ENABLE_WARN_DEPRECATED
>  	  Disable this to suppress the "warning: 'foo' is deprecated
>  	  (declared at kernel/power/somefile.c:1234)" messages.
>  
> +config HAVE_ARCH_DEBUG_ATOMIC
> +	bool
> +
> +config ENABLE_WARN_ATOMIC_INC_WRAP
> +	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
> +	depends on HAVE_ARCH_DEBUG_ATOMIC
> +	default y
> +	help
> +	  Enable printing a warning when atomic_inc() or atomic_dec()
> +	  operation wraps around the 32-bit value.
> +

Yes, I agree with `default y' for now.  But we might want to turn it
off again later.  Adding that WARN to every atomic_inc/atomic_dec site
must do terrible things to the kernel text footprint.

Of course, if we make if `default y' for a while and then switch it to
`default n', the `y' state will linger for a very long time in all the
kernel developers' .configs.  Good!  Very sneaky.

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Nikanth Karthikesan]

On Friday 01 May 2009 03:15:26 Andrew Morton wrote:
> On Thu, 30 Apr 2009 19:39:50 +0530
> Nikanth Karthikesan <knikanth@novell.com> wrote:
>
>
>
> If I had a dollar for each wordwrapped patch I get sent...
>

Ah..very sorry, was a bit hasty. Would pay it, when I get to meet you. ;-)

> > Detect and warn on atomic_inc/atomic_dec overflow.
> >
> > Add a debug option to detect and warn when the 32-bit atomic_t overflows
> > during atomic_inc and atomic_dec.
>
> OK.
>
> I'll beef the changelog up a bit - this one is wimpy.
>

Thanks.

> The question is: do we put this in mainline?  I guess we might as well
> give it a shot.  It may well find bugs and it might also trigger false
> positives.  We can then fix the bugs and decide whether the false
> positives warrant reverting it again, all very easy.
>
> > +#include <asm/bug.h>
>
> checkpatch says
>
> WARNING: Use #include <linux/bug.h> instead of <asm/bug.h>
> #215: FILE: include/asm-generic/atomic.h:11:
> +#include <asm/bug.h>
>
> Was this an oversight, or did you try using linux/bug.h and discovered
> some problem?

I tried with linux/bug.h. But it creates a cyclic dependency. linux/bug.h 
pulls in linux/module.h => linux/spinlock.h => asm/spinlock.h (which uses 
atomic_inc) => asm/atomic.h,.

>
> > index 812c282..773c1a4 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -17,6 +17,17 @@ config ENABLE_WARN_DEPRECATED
> >  	  Disable this to suppress the "warning: 'foo' is deprecated
> >  	  (declared at kernel/power/somefile.c:1234)" messages.
> >
> > +config HAVE_ARCH_DEBUG_ATOMIC
> > +	bool
> > +
> > +config ENABLE_WARN_ATOMIC_INC_WRAP
> > +	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
> > +	depends on HAVE_ARCH_DEBUG_ATOMIC
> > +	default y
> > +	help
> > +	  Enable printing a warning when atomic_inc() or atomic_dec()
> > +	  operation wraps around the 32-bit value.
> > +
>
> Yes, I agree with `default y' for now.  But we might want to turn it
> off again later.  Adding that WARN to every atomic_inc/atomic_dec site
> must do terrible things to the kernel text footprint.
>
> Of course, if we make if `default y' for a while and then switch it to
> `default n', the `y' state will linger for a very long time in all the
> kernel developers' .configs.  Good!  Very sneaky.

:)

Thanks
Nikanth

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Andrew Morton]

On Fri, 1 May 2009 10:27:46 +0530 Nikanth Karthikesan <knikanth@novell.com> wrote:

> I tried with linux/bug.h. But it creates a cyclic dependency. linux/bug.h 
> pulls in linux/module.h => linux/spinlock.h => asm/spinlock.h (which uses 
> atomic_inc) => asm/atomic.h,.

And why on earth does bug.h need module.h?

int  module_bug_finalize(const Elf_Ehdr *, const Elf_Shdr *,
			 struct module *);

The ever-smelly Elf_Ehdr and Elf_Shdr.

Probably the module_bug_finalize/module_bug_cleanup declarations and
definitions should be in module.h.

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Andrew Morton]

On Thu, 30 Apr 2009 22:06:48 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 1 May 2009 10:27:46 +0530 Nikanth Karthikesan <knikanth@novell.com> wrote:
> 
> > I tried with linux/bug.h. But it creates a cyclic dependency. linux/bug.h 
> > pulls in linux/module.h => linux/spinlock.h => asm/spinlock.h (which uses 
> > atomic_inc) => asm/atomic.h,.
> 
> And why on earth does bug.h need module.h?
> 
> int  module_bug_finalize(const Elf_Ehdr *, const Elf_Shdr *,
> 			 struct module *);
> 
> The ever-smelly Elf_Ehdr and Elf_Shdr.
> 
> Probably the module_bug_finalize/module_bug_cleanup declarations and
> definitions should be in module.h.
> 

I queued the below.  I _guess_ it's an improvement - bug.h is a quite
low-level thing.

I don't yet know what will explode as a result.


From: Andrew Morton <akpm@linux-foundation.org>

They're in linux/bug.h at present, which causes include order tangles.  In
particular, linux/bug.h cannot be used by linux/atomic.h because,
according to Nikanth:

linux/bug.h pulls in linux/module.h => linux/spinlock.h => asm/spinlock.h
(which uses atomic_inc) => asm/atomic.h.

bug.h is a pretty low-level thing and module.h is a higher-level thing,
IMO.

Cc: Nikanth Karthikesan <knikanth@novell.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/bug.h    |   12 ------------
 include/linux/module.h |   17 +++++++++++++++++
 2 files changed, 17 insertions(+), 12 deletions(-)

diff -puN include/linux/bug.h~headers-move-module_bug_finalize-module_bug_cleanup-definitions-into-moduleh include/linux/bug.h
--- a/include/linux/bug.h~headers-move-module_bug_finalize-module_bug_cleanup-definitions-into-moduleh
+++ a/include/linux/bug.h
@@ -1,7 +1,6 @@
 #ifndef _LINUX_BUG_H
 #define _LINUX_BUG_H
 
-#include <linux/module.h>
 #include <asm/bug.h>
 
 enum bug_trap_type {
@@ -24,10 +23,6 @@ const struct bug_entry *find_bug(unsigne
 
 enum bug_trap_type report_bug(unsigned long bug_addr, struct pt_regs *regs);
 
-int  module_bug_finalize(const Elf_Ehdr *, const Elf_Shdr *,
-			 struct module *);
-void module_bug_cleanup(struct module *);
-
 /* These are defined by the architecture */
 int is_valid_bugaddr(unsigned long addr);
 
@@ -38,13 +33,6 @@ static inline enum bug_trap_type report_
 {
 	return BUG_TRAP_TYPE_BUG;
 }
-static inline int  module_bug_finalize(const Elf_Ehdr *hdr,
-					const Elf_Shdr *sechdrs,
-					struct module *mod)
-{
-	return 0;
-}
-static inline void module_bug_cleanup(struct module *mod) {}
 
 #endif	/* CONFIG_GENERIC_BUG */
 #endif	/* _LINUX_BUG_H */
diff -puN include/linux/module.h~headers-move-module_bug_finalize-module_bug_cleanup-definitions-into-moduleh include/linux/module.h
--- a/include/linux/module.h~headers-move-module_bug_finalize-module_bug_cleanup-definitions-into-moduleh
+++ a/include/linux/module.h
@@ -696,4 +696,21 @@ static inline void module_remove_modinfo
 
 #define __MODULE_STRING(x) __stringify(x)
 
+
+#ifdef CONFIG_GENERIC_BUG
+int  module_bug_finalize(const Elf_Ehdr *, const Elf_Shdr *,
+			 struct module *);
+void module_bug_cleanup(struct module *);
+
+#else	/* !CONFIG_GENERIC_BUG */
+
+static inline int  module_bug_finalize(const Elf_Ehdr *hdr,
+					const Elf_Shdr *sechdrs,
+					struct module *mod)
+{
+	return 0;
+}
+static inline void module_bug_cleanup(struct module *mod) {}
+#endif	/* CONFIG_GENERIC_BUG */
+
 #endif /* _LINUX_MODULE_H */
_

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Andrew Morton]

On Thu, 30 Apr 2009 19:39:50 +0530
Nikanth Karthikesan <knikanth@novell.com> wrote:

> 
> Detect and warn on atomic_inc/atomic_dec overflow.
> 
> Add a debug option to detect and warn when the 32-bit atomic_t overflows
> during atomic_inc and atomic_dec.
> 
>
> ...
>
> --- a/include/asm-generic/atomic.h
> +++ b/include/asm-generic/atomic.h
> @@ -4,15 +4,51 @@
>   * Copyright (C) 2005 Silicon Graphics, Inc.
>   *	Christoph Lameter
>   *
> - * Allows to provide arch independent atomic definitions without the need to
> - * edit all arch specific atomic.h files.
>   */
>  
> +#include <linux/kernel.h>
>  #include <asm/types.h>
> +#include <asm/bug.h>

We're going to have real trouble making changes like this to a
low-level header file - sparc64 results below.

> +/**
> + * atomic_inc - increment atomic variable
> + * @v: pointer of type atomic_t
> + *
> + * Atomically increments @v by 1.
> + */
> +static inline void atomic_inc(atomic_t *v)
> +{
> +#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
> +	WARN_ONCE((atomic_read(v) > (INT_MAX / 2)),
> +		KERN_ERR "atomic inc overflow!");
> +#endif
> +	raw_atomic_inc(v);
> +}

Are we allowed to assume that atomic_inc==raw_atomic_inc for all
architectures which use this definition?

Do we know that atomic_read() is defined at this point?


We can avoid the problematic includes via

extern void atomic_inc_screwed_up(atomic_t *v);

static inline void atomic_inc(atomic_t *v)
{
#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
	if (atomic_read(v) > (INT_MAX / 2))
		atomic_inc_screwed_up(v);
#endif
	raw_atomic_inc(v);
}


In file included from /usr/src/devel/arch/sparc/include/asm/atomic_64.h:117,
                 from /usr/src/devel/arch/sparc/include/asm/atomic.h:4,
                 from include/linux/debug_locks.h:5,
                 from include/linux/lockdep.h:19,
                 from include/linux/spinlock_types.h:18,
                 from include/linux/spinlock.h:80,
                 from include/linux/seqlock.h:29,
                 from include/linux/time.h:8,
                 from include/linux/timex.h:56,
                 from include/linux/sched.h:54,
                 from arch/sparc/kernel/asm-offsets.c:13:
include/asm-generic/atomic.h:20: error: syntax error before numeric constant
include/asm-generic/atomic.h:21: warning: static declaration of 'atomic_add' follows non-static declaration
/usr/src/devel/arch/sparc/include/asm/atomic_64.h:22: warning: previous declaration of 'atomic_add' was here
include/asm-generic/atomic.h: In function `atomic_add':
include/asm-generic/atomic.h:21: error: number of arguments doesn't match prototype
/usr/src/devel/arch/sparc/include/asm/atomic_64.h:22: error: prototype declaration
include/asm-generic/atomic.h:26: error: implicit declaration of function `raw_atomic_inc'
include/asm-generic/atomic.h:26: error: `v' undeclared (first use in this function)
include/asm-generic/atomic.h:26: error: (Each undeclared identifier is reported only once
include/asm-generic/atomic.h:26: error: for each function it appears in.)
include/asm-generic/atomic.h: At top level:
include/asm-generic/atomic.h:35: error: syntax error before numeric constant
include/asm-generic/atomic.h:36: warning: static declaration of 'atomic_sub' follows non-static declaration
/usr/src/devel/arch/sparc/include/asm/atomic_64.h:24: warning: previous declaration of 'atomic_sub' was here
include/asm-generic/atomic.h: In function `atomic_sub':
include/asm-generic/atomic.h:36: error: number of arguments doesn't match prototype
/usr/src/devel/arch/sparc/include/asm/atomic_64.h:24: error: prototype declaration
include/asm-generic/atomic.h:41: error: implicit declaration of function `raw_atomic_dec'
include/asm-generic/atomic.h:41: error: `v' undeclared (first use in this function)

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Nikanth Karthikesan]

On Friday 08 May 2009 05:53:16 Andrew Morton wrote:
> On Thu, 30 Apr 2009 19:39:50 +0530
>
> Nikanth Karthikesan <knikanth@novell.com> wrote:
> > Detect and warn on atomic_inc/atomic_dec overflow.
> >
> > Add a debug option to detect and warn when the 32-bit atomic_t overflows
> > during atomic_inc and atomic_dec.
> >
> >
> > ...
> >
> > --- a/include/asm-generic/atomic.h
> > +++ b/include/asm-generic/atomic.h
> > @@ -4,15 +4,51 @@
> >   * Copyright (C) 2005 Silicon Graphics, Inc.
> >   *	Christoph Lameter
> >   *
> > - * Allows to provide arch independent atomic definitions without the
> > need to - * edit all arch specific atomic.h files.
> >   */
> >
> > +#include <linux/kernel.h>
> >  #include <asm/types.h>
> > +#include <asm/bug.h>
>
> We're going to have real trouble making changes like this to a
> low-level header file - sparc64 results below.
>
> > +/**
> > + * atomic_inc - increment atomic variable
> > + * @v: pointer of type atomic_t
> > + *
> > + * Atomically increments @v by 1.
> > + */
> > +static inline void atomic_inc(atomic_t *v)
> > +{
> > +#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
> > +	WARN_ONCE((atomic_read(v) > (INT_MAX / 2)),
> > +		KERN_ERR "atomic inc overflow!");
> > +#endif
> > +	raw_atomic_inc(v);
> > +}
>
> Are we allowed to assume that atomic_inc==raw_atomic_inc for all
> architectures which use this definition?
>

No. Only for architectures that have HAVE_ARCH_DEBUG_ATOMIC. I missed
enclosing the change with #ifdef HAVE_ARCH_DEBUG_ATOMIC. That should solve the
problem as it wont be built for other architectures at all.

> Do we know that atomic_read() is defined at this point?
>

Yes.

>
> We can avoid the problematic includes via
>
> extern void atomic_inc_screwed_up(atomic_t *v);
>

This and even having atomic_inc defined as a c function instead of a macro has
a problem. WARN_ONCE would warn only once even if more than 1 variable wraps
around. So should we have atomic_inc itself as a macro instead?

Also it would report the location in source as include/asm-generic/atomic.h
instead of the caller. But it should be fine as stack trace would be printed.

Anyway I have converted warn_once as an extern function.

Changes from the previous version:
1. Enclose everything with #ifdef CONFIG_HAVE_ARCH_DEBUG_ATOMIC.
2. Make the warn_once as an extern function and remove the
#include<asm/bug.h>. This is not required as CONFIG_HAVE_ARCH_DEBUG_ATOMIC is
defined onlf for x86 and it does not have problem with this include. I think
we could remove this and also make everything as macro?

The linux/kernel.h is used for INT_MAX which can also be avoided by
redefining, if required.

Thanks
Nikanth

Subject: atomic: detect and warn on atomic_inc/atomic_dec wrapping around
From: Nikanth Karthikesan <knikanth@novell.com>

Add a debug option to detect and warn when the 32-bit atomic_t overflows
during atomic_inc and atomic_dec.

We expect that overflow and underflow of an atomic_t is usually a bug. 
There was a patch of this nature in -mm for a long time and it never
triggered false positives and it did find a couple of real bugs.

The option is default-y for now.  It will have a fairly nasty impact on
.text size so we may decide to switch it to default-n prior to a kernel
release.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
Acked-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..dd49be3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -46,6 +46,7 @@ config X86
 	select HAVE_KERNEL_GZIP
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
+	select HAVE_ARCH_DEBUG_ATOMIC
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
index 85b46fb..c6a17bb 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "+m" (v->counter));
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "+m" (v->counter));
diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
index 8c21731..1183b85 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "=m" (v->counter)
@@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "=m" (v->counter)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index 3673a13..b14e8ce 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -4,15 +4,56 @@
  * Copyright (C) 2005 Silicon Graphics, Inc.
  *	Christoph Lameter
  *
- * Allows to provide arch independent atomic definitions without the need to
- * edit all arch specific atomic.h files.
  */
 
 #include <asm/types.h>
 
+#ifdef CONFIG_HAVE_ARCH_DEBUG_ATOMIC
+
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+#include <linux/kernel.h>
+extern void warn_once(bool condition, char *msg);
+#endif
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ */
+static inline void atomic_inc(atomic_t *v)
+{
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+	warn_once((atomic_read(v) > (INT_MAX / 2)),
+		KERN_ERR "atomic inc overflow!");
+#endif
+	raw_atomic_inc(v);
+}
+
+/**
+ * atomic_dec - decrement atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ */
+static inline void atomic_dec(atomic_t *v)
+{
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+	warn_once((atomic_read(v) < (INT_MIN / 2)),
+		KERN_ERR "atomic dec underflow!");
+#endif
+	raw_atomic_dec(v);
+
+}
+
+#endif /* CONFIG_HAVE_ARCH_DEBUG_ATOMIC */
+
 /*
  * Suppport for atomic_long_t
  *
+ * Allows to provide arch independent atomic definitions without the need to
+ * edit all arch specific atomic.h files.
+ *
  * Casts for parameters are avoided for existing atomic functions in order to
  * avoid issues with cast-as-lval under gcc 4.x and other limitations that the
  * macros of a platform may have.
diff --git a/kernel/printk.c b/kernel/printk.c
index 5052b54..48fef2c 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -33,6 +33,7 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/kexec.h>
+#include <linux/bug.h>
 
 #include <asm/uaccess.h>
 
@@ -1322,3 +1323,13 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 }
 EXPORT_SYMBOL(printk_timed_ratelimit);
 #endif
+
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+
+void warn_once(bool condition, char *msg)
+{
+        WARN_ONCE(condition, msg);
+}
+EXPORT_SYMBOL(warn_once);
+
+#endif
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 812c282..773c1a4 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -17,6 +17,17 @@ config ENABLE_WARN_DEPRECATED
 	  Disable this to suppress the "warning: 'foo' is deprecated
 	  (declared at kernel/power/somefile.c:1234)" messages.
 
+config HAVE_ARCH_DEBUG_ATOMIC
+	bool
+
+config ENABLE_WARN_ATOMIC_INC_WRAP
+	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
+	depends on HAVE_ARCH_DEBUG_ATOMIC
+	default y
+	help
+	  Enable printing a warning when atomic_inc() or atomic_dec()
+	  operation wraps around the 32-bit value.
+
 config ENABLE_MUST_CHECK
 	bool "Enable __must_check logic"
 	default y

Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around

[Nikanth Karthikesan]

On Friday 08 May 2009 16:10:49 Nikanth Karthikesan wrote:
> On Friday 08 May 2009 05:53:16 Andrew Morton wrote:
> > On Thu, 30 Apr 2009 19:39:50 +0530
> >
> > Nikanth Karthikesan <knikanth@novell.com> wrote:
> > > Detect and warn on atomic_inc/atomic_dec overflow.
> > >
> > > Add a debug option to detect and warn when the 32-bit atomic_t
> > > overflows during atomic_inc and atomic_dec.
> > >
> > >
> > > ...
> > >
> > > --- a/include/asm-generic/atomic.h
> > > +++ b/include/asm-generic/atomic.h
> > > @@ -4,15 +4,51 @@
> > >   * Copyright (C) 2005 Silicon Graphics, Inc.
> > >   *	Christoph Lameter
> > >   *
> > > - * Allows to provide arch independent atomic definitions without the
> > > need to - * edit all arch specific atomic.h files.
> > >   */
> > >
> > > +#include <linux/kernel.h>
> > >  #include <asm/types.h>
> > > +#include <asm/bug.h>
> >
> > We're going to have real trouble making changes like this to a
> > low-level header file - sparc64 results below.
> >
> > > +/**
> > > + * atomic_inc - increment atomic variable
> > > + * @v: pointer of type atomic_t
> > > + *
> > > + * Atomically increments @v by 1.
> > > + */
> > > +static inline void atomic_inc(atomic_t *v)
> > > +{
> > > +#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
> > > +	WARN_ONCE((atomic_read(v) > (INT_MAX / 2)),
> > > +		KERN_ERR "atomic inc overflow!");
> > > +#endif
> > > +	raw_atomic_inc(v);
> > > +}
> >
> > Are we allowed to assume that atomic_inc==raw_atomic_inc for all
> > architectures which use this definition?
>
> No. Only for architectures that have HAVE_ARCH_DEBUG_ATOMIC. I missed
> enclosing the change with #ifdef HAVE_ARCH_DEBUG_ATOMIC. That should solve
> the problem as it wont be built for other architectures at all.
>
> > Do we know that atomic_read() is defined at this point?
>
> Yes.
>
> > We can avoid the problematic includes via
> >
> > extern void atomic_inc_screwed_up(atomic_t *v);
>
> This and even having atomic_inc defined as a c function instead of a macro
> has a problem. WARN_ONCE would warn only once even if more than 1 variable
> wraps around. So should we have atomic_inc itself as a macro instead?
>
> Also it would report the location in source as include/asm-generic/atomic.h
> instead of the caller. But it should be fine as stack trace would be
> printed.
>
> Anyway I have converted warn_once as an extern function.
>
> Changes from the previous version:
> 1. Enclose everything with #ifdef CONFIG_HAVE_ARCH_DEBUG_ATOMIC.
> 2. Make the warn_once as an extern function and remove the
> #include<asm/bug.h>. This is not required as CONFIG_HAVE_ARCH_DEBUG_ATOMIC
> is defined onlf for x86 and it does not have problem with this include. I
> think we could remove this and also make everything as macro?
>
If you think this would be better, here is a patch that does this. Having it 
as a macro would warn for every variable that could overflow.

Thanks
Nikanth

Subject: atomic: detect and warn on atomic_inc/atomic_dec wrapping around
From: Nikanth Karthikesan <knikanth@novell.com>

Add a debug option to detect and warn when the 32-bit atomic_t overflows
during atomic_inc and atomic_dec.

We expect that overflow and underflow of an atomic_t is usually a bug. 
There was a patch of this nature in -mm for a long time and it never
triggered false positives and it did find a couple of real bugs.

The option is default-y for now.  It will have a fairly nasty impact on
.text size so we may decide to switch it to default-n prior to a kernel
release.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
Acked-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..dd49be3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -46,6 +46,7 @@ config X86
 	select HAVE_KERNEL_GZIP
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
+	select HAVE_ARCH_DEBUG_ATOMIC
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/x86/include/asm/atomic_32.h 
b/arch/x86/include/asm/atomic_32.h
index 85b46fb..c6a17bb 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -78,24 +78,24 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "+m" (v->counter));
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "+m" (v->counter));
diff --git a/arch/x86/include/asm/atomic_64.h 
b/arch/x86/include/asm/atomic_64.h
index 8c21731..1183b85 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -77,12 +77,12 @@ static inline int atomic_sub_and_test(int i, atomic_t *v)
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * raw_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static inline void atomic_inc(atomic_t *v)
+static inline void raw_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "=m" (v->counter)
@@ -90,12 +90,12 @@ static inline void atomic_inc(atomic_t *v)
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * raw_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static inline void atomic_dec(atomic_t *v)
+static inline void raw_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "=m" (v->counter)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index 3673a13..004b018 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -4,15 +4,56 @@
  * Copyright (C) 2005 Silicon Graphics, Inc.
  *	Christoph Lameter
  *
- * Allows to provide arch independent atomic definitions without the need to
- * edit all arch specific atomic.h files.
  */
 
 #include <asm/types.h>
 
+#ifdef CONFIG_HAVE_ARCH_DEBUG_ATOMIC
+
+#include <linux/kernel.h>
+#include <asm/bug.h>
+
+#ifdef CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP
+
+/**
+ * atomic_inc - increment atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically increments @v by 1.
+ */
+#define atomic_inc(v) ({				\
+	WARN_ONCE((atomic_read(v) > (INT_MAX / 2)),	\
+		KERN_ERR "atomic inc overflow!");	\
+	raw_atomic_inc(v);				\
+})
+
+/**
+ * atomic_dec - decrement atomic variable
+ * @v: pointer of type atomic_t
+ *
+ * Atomically decrements @v by 1.
+ */
+#define atomic_dec(v) ({				\
+	WARN_ONCE((atomic_read(v) < (INT_MIN / 2)),	\
+		KERN_ERR "atomic dec underflow!");	\
+	raw_atomic_dec(v);				\
+})
+
+#else /* CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP */
+
+#define atomic_inc(v) raw_atomic_inc(v)
+#define atomic_dec(v) raw_atomic_dec(v)
+
+#endif /* !CONFIG_ENABLE_WARN_ATOMIC_INC_WRAP */
+
+#endif /* CONFIG_HAVE_ARCH_DEBUG_ATOMIC */
+
 /*
  * Suppport for atomic_long_t
  *
+ * Allows to provide arch independent atomic definitions without the need to
+ * edit all arch specific atomic.h files.
+ *
  * Casts for parameters are avoided for existing atomic functions in order to
  * avoid issues with cast-as-lval under gcc 4.x and other limitations that 
the
  * macros of a platform may have.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 812c282..773c1a4 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -17,6 +17,17 @@ config ENABLE_WARN_DEPRECATED
 	  Disable this to suppress the "warning: 'foo' is deprecated
 	  (declared at kernel/power/somefile.c:1234)" messages.
 
+config HAVE_ARCH_DEBUG_ATOMIC
+	bool
+
+config ENABLE_WARN_ATOMIC_INC_WRAP
+	bool "Enable warning on atomic_inc()/atomic_dec() wrap"
+	depends on HAVE_ARCH_DEBUG_ATOMIC
+	default y
+	help
+	  Enable printing a warning when atomic_inc() or atomic_dec()
+	  operation wraps around the 32-bit value.
+
 config ENABLE_MUST_CHECK
 	bool "Enable __must_check logic"
 	default y