Date: Sat, 09 Oct 2010 12:48:50 +0300 From: Andriy Gapon <avg@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r213648 - head/sys/kern Message-ID: <4CB03A82.6040701@freebsd.org> In-Reply-To: <20101009191600.N3531@besplex.bde.org> References: <201010090807.o9987nG0030939@svn.freebsd.org> <20101009191600.N3531@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
on 09/10/2010 12:33 Bruce Evans said the following:
> 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
I agree about panicstr.
But I am not sure if we have places in code (beyond panic function) itself where
volatile vs. non-volatile would make any actual difference.
But still.
> 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 */
Yes, it's exactly the last loop that had the problem.
On amd64 with -O2:
.loc 1 544 0
movl panic_cpu(%rip), %eax
.LVL134:
.p2align 4,,7
.L210:
cmpl $255, %eax
jne .L210
jmp .L225
> 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
I discussed this with kib and the idea is that atomic operation is not needed in
that place and volatile is sufficient, because we don't need barrier semantics
there and only want to ensure that variable is reloaded from memory.
atomic_store_rel at the end seems to be needed because of inter-dependency
between panicstr and panic_cpu. That is we want changes to panicstr to becomes
visible at the same time (before actually) changes to panic_cpu are visible.
> 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
--
Andriy Gapon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4CB03A82.6040701>