Re: Is clobber "memory" in include/asm-i386/system.h necessary?

David S. Miller (davem@caip.rutgers.edu)
Thu, 2 May 1996 14:58:26 -0400


Date: Wed, 1 May 1996 19:46:43 -0700
From: ftom@netcom.com (Tom May)

This is so weird this question comes up now...

In include/asm-i386/system.h there are numerous uses of __asm__ which
clobber "memory". For reference, here they are:

#define mb() __asm__ __volatile__ ("" : : :"memory")
#define sti() __asm__ __volatile__ ("sti": : :"memory")
#define cli() __asm__ __volatile__ ("cli": : :"memory")

#define save_flags(x) \
__asm__ __volatile__("pushfl ; popl %0":"=g" (x): /* no input */ :"memory")

#define restore_flags(x) \
__asm__ __volatile__("pushl %0 ; popfl": /* no output */ :"g" (x):"memory")

#define iret() __asm__ __volatile__ ("iret": : :"memory")

This clobber seems unnecessary since none of these instructions modify
memory that gcc doesn't know about, and any memory that is modified by
other means (hardware, interrupt handlers, etc.) is (should be?)
accounted for in other ways such as by using volatile.

Wrong, volatile is an ugly solution to a problem which is more
elegantly solved by the "memory" clobbering. Consider del_timer() in
kernel/sched.c:

int del_timer(struct timer_list * timer)
{
int ret = 0;
if (timer->next) {
unsigned long flags;
struct timer_list * next;
save_flags(flags);
cli();
if ((next = timer->next) != NULL) {
(next->prev = timer->prev)->next = next;
timer->next = timer->prev = NULL;
ret = 1;
}
restore_flags(flags);
}
return ret;
}

It is pointless to make the next field of the timer_list be volatile,
or any of the local variables, just to tell gcc "yeah, an interrupt
could ruin our day and change stuff you think won't change". It is
much cleaner and better for gcc's optimizer to seed the basic blocks
with "memory" clobbers than making everything volatile. The memory
clobber just causes gcc to reload timer->next again after the cli(),
if we made "next" volatile, for example on the Sparc, it would
allocate stack space to store that local variable in and always
load/store to/from the stack for that variable. Yuck!

As an experiment, I removed the clobbers, built a kernel, and it ran
fine (but if you try this at home, you will trigger the gcc common
subexpression elimination bug, so until it is fixed for real, your
kernel may or may not work).

Check out the assembler output from del_timer(), if you cook your
machine hard enough (the networking benchmarks in lmbench seem to be
good for this on Sparc) you'll get an oops in del_timer() because gcc
doesn't reload timer->next after the cli().

So what's up with these clobbers? If they aren't necessary, they
should be removed because they have a detrimental effect on code
generation. If they are necessary, how about a comment explaining
why?

They are necessary, Linus has thought long and hard about this. I've
only recently come acquainted with how more correct it is than the
"make everyone and his grandmother volatile" solution.

Later,
David S. Miller
davem@caip.rutgers.edu