svn commit: r213648 - head/sys/kern

[Bruce Evans]

On Sat, 9 Oct 2010, Andriy Gapon wrote:

> Log:
>  panic_cpu variable should be volatile
>
>  This is to prevent caching of its value in a register when it is checked
>  and modified by multiple CPUs in parallel.
>  Also, move the variable  into the scope of the only function that uses it.
>
>  Reviewed by:	jhb
>  Hint from:	mdf
>  MFC after:	1 week

I doubt that this is either necessary or sufficient.  Most variables
aren't volatile while they are locked by a mutex.  But panic() cannot use
normal mutex locking, so it should access this variable using atomic
ops with memory barriers.  The compiler part of the memory barriers
effectively make _all_ variables temporily volatile.  However, 2 of the
the 4 accesses to this variable doesn't use an atomic op.

% #ifdef SMP
% 	/*
% 	 * We don't want multiple CPU's to panic at the same time, so we
% 	 * use panic_cpu as a simple spinlock.  We have to keep checking
% 	 * panic_cpu if we are spinning in case the panic on the first
% 	 * CPU is canceled.
% 	 */
% 	if (panic_cpu != PCPU_GET(cpuid))

This access doesn't use an atomic op.

% 		while (atomic_cmpset_int(&panic_cpu, NOCPU,
% 		    PCPU_GET(cpuid)) == 0)

This access uses an atomic op.  Not all atomic ops use memory barriers,
at least on i386.  However, this one does, at least on i386.

   (I'm always confused about what atomic_any() without acq or release
   means.  Do they mean that you don't want a memory barrier (this is
   what the mostly give, at least on i386), and if so, what use are
   they?  There are far too many atomic ops, for far too many never-used
   widths, with alternative spellings to encourage using a wrong one.
   cmpset is is especially confusing since it you can spell it as cmpset,
   cmpset_acq or compset_rel and always get the barrier, at least on
   i386.  At least cmpset only supports 1 width, at least on i386.)

% 			while (panic_cpu != NOCPU)

This access doesn't use an atomic op.

% 				; /* nothing */
% #endif
% ...
% #ifdef RESTARTABLE_PANICS
% 	/* See if the user aborted the panic, in which case we continue. */
% 	if (panicstr == NULL) {
% #ifdef SMP
% 		atomic_store_rel_int(&panic_cpu, NOCPU);

This access uses an atomic op with a memory barrier, at least on i386.
Now its rel semantics are clear.

panicstr is non-volatile and never accessed by an atomic op, so it probably
strictly needs to be declared volatile even more than panic_cpu.  I think
the only thing that makes it work now is that it is bogusly pubic, and
compilers can't analyze the whole program yet -- if they could, then they
would see that it is only set in panic().

% #endif
% 		return;
% 	}
% #endif
% #endif

Now, why don't the partial memory barriers prevent caching the variable?

% 	if (panic_cpu != PCPU_GET(cpuid))
% 		while (atomic_cmpset_int(&panic_cpu, NOCPU,
% 		    PCPU_GET(cpuid)) == 0)
% 			while (panic_cpu != NOCPU)
% 				; /* nothing */

The very first access can't reasonably use a cachec value.  atomic_cmpset()
can change panic_cpu, so even without the memory barrier, panic_cpu must
be reloaded for the third access the first time.  But then when the third
access is repeated in the second while loop, the missing atomic op with
barrier makes things very broken.  panic_cpu isn't changed by the loop,
and the compiler thinks that it isn't changed by anything else either, so
the compiler may reduce the loop to:

% 			if (panic_cpu != NOCPU)
% 				for (;;)
% 					; /* nothing */

except I've seen claims that even an endless for loop can be optimized
to nothing.  Declaring panic_cpu as volatile prevents the compiler doing
this, but I think it is insufficient.  We really do want to see panic_cpu
changed by other CPUs, and what is the point of atomic_load_acq*() if not
to use for this -- if declaring things volatile were sufficient, then we
could just use *(volatile *)&var.  atomic_load/store_acq/rel*() on i386
used to do exactly that for the UP case, but use a lock prefix and also
a memory barrier for the SMP case.  Current versions also use the memory
barrier in the UP case, and have a relevant comment aboat this:

% Index: atomic.h
% ===================================================================
% RCS file: /home/ncvs/src/sys/i386/include/atomic.h,v
% retrieving revision 1.32.2.1
% retrieving revision 1.55
% diff -u -1 -r1.32.2.1 -r1.55
% --- atomic.h	24 Nov 2004 18:10:02 -0000	1.32.2.1
% +++ atomic.h	20 May 2010 06:18:03 -0000	1.55
% @@ -181,5 +207,5 @@
%   * SMP kernels.  For UP kernels, however, the cache of the single processor
% - * is always consistent, so we don't need any memory barriers.
% + * is always consistent, so we only need to take care of compiler.
%   */

i386 doesn't really have memory barriers (maybe l/s/mfence are, but we
don't use them even on amd64), but we have to use "memoy" clobbers to
take care of the compiler, or we would have to declare too many things
as volatile.

% -#define ATOMIC_STORE_LOAD(TYPE, LOP, SOP)		\
% +#define	ATOMIC_STORE_LOAD(TYPE, LOP, SOP)		\
%  static __inline u_##TYPE				\
% @@ -187,3 +213,7 @@
%  {							\
% -	return (*p);					\
% +	u_##TYPE tmp;					\
% +							\
% +	tmp = *p;					\
% +	__asm __volatile("" : : : "memory");		\
% +	return (tmp);					\
%  }							\
% @@ -193,2 +223,3 @@
%  {							\
% +	__asm __volatile("" : : : "memory");		\
%  	*p = v;						\

5-STABLE is still missing the memory clobber, so a newer compiler on it
might compile away the while loop even when it is fixed to use
atomic_load_acq_int().

% ...
% 	if (panicstr == NULL) {
% 		atomic_store_rel_int(&panic_cpu, NOCPU);

Smaller problems with this.  Just the unlocked access to panicstr.
Maybe not a problem, except to understand why it isn't one.  I think
the earlier while loops prevent concurrent modification of panicstr.
Recursive panics are possible, but since they are possible the compiler
should see that they are possible even if it analyzes the whole program,
and thus know that it cannot cache panicstr.

Bruce