Re: [PATCH] Fix WARN_ON / WARN_ON_ONCE regression

[Andrew Morton <akpm@osdl.org>]

On Tue, 03 Oct 2006 17:09:29 -0700
Tim Chen <tim.c.chen@linux.intel.com> wrote:

> On Tue, 2006-10-03 at 17:07 -0700, Andrew Morton wrote:
> 
> > 
> > Perhaps the `static int __warn_once' is getting put in the same cacheline
> > as some frequently-modified thing.   Perhaps try marking that as __read_mostly?
> > 
> 
> I've tried marking static int __warn_once as __read_mostly.  However, it
> did not help with reducing the cache miss :(
> 
> So I would suggest reversing the "Let WARN_ON/WARN_ON_ONCE return the
> condition" patch.  It has just been added 3 days ago so reversing it
> should not be a problem.
> 

Not yet, please.  This is presently a mystery, and we need to work out
what's going on.

First up, is it due to WARN_ON, or WARN_ON_ONCE?  Please try reverting each
one separately.

Let's look at WARN_ON.

Before:

#define WARN_ON(condition) do { \
	if (unlikely((condition)!=0)) { \
		printk("BUG: warning at %s:%d/%s()\n", __FILE__, __LINE__, __FUNCTION__); \
		dump_stack(); \
	} \
} while (0)

After:

#define WARN_ON(condition) ({						\
	typeof(condition) __ret_warn_on = (condition);			\
	if (unlikely(__ret_warn_on)) {					\
		printk("BUG: warning at %s:%d/%s()\n", __FILE__,	\
			__LINE__, __FUNCTION__);			\
		dump_stack();						\
	}								\
	unlikely(__ret_warn_on);					\
})

There's no difference, except we return the temporary. 


And WARN_ON_ONCE.

Before:

#define WARN_ON_ONCE(condition)				\
({							\
	static int __warn_once = 1;			\
	int __ret = 0;					\
							\
	if (unlikely((condition) && __warn_once)) {	\
		__warn_once = 0;			\
		WARN_ON(1);				\
		__ret = 1;				\
	}						\
	__ret;						\
})

After:

#define WARN_ON_ONCE(condition)	({			\
	static int __warn_once = 1;			\
	typeof(condition) __ret_warn_once = (condition);\
							\
	if (likely(__warn_once))			\
		if (WARN_ON(__ret_warn_once)) 		\
			__warn_once = 0;		\
	unlikely(__ret_warn_once);			\
})

There are changes here: in the old code we'll avoid reading the static
variable.  In the new code we'll read the static variable, but we'll avoid
evaluating the condition.

Why would that make a measurable difference?

Do you know which WARN_ON (or is it WARN_ON_ONCE?) callsite is causing a
problem?