Messages in this thread | | | Date | Thu, 30 Apr 2009 16:05:59 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH] Detect and warn on atomic_inc/atomic_dec wrapping around |
| |
* 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
| |