Re: [PATCH] Fix WARN_ON / WARN_ON_ONCE regression

Andrew Morton
2006-10-04T01:15:20Z

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?