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

[Ingo Molnar <mingo@elte.hu>]

* 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