Re: [PATCH 1/2] UML - Fix I/O hang

[Andrew Morton <akpm@osdl.org>]

On Wed, 01 Nov 2006 12:32:41 -0500
Jeff Dike <jdike@addtoit.com> wrote:

> This patch fixes a UML hang in which everything would just stop until
> some I/O happened - a ping, someone whacking the keyboard - at which
> point everything would start up again as though nothing had happened.
> 
> The cause was gcc reordering some code which absolutely needed to be
> executed in the order in the source.  When unblock_signals switches
> signals from off to on, it needs to see if any interrupts had happened
> in the critical section.  The interrupt handlers check signals_enabled -
> if it is zero, then the handler adds a bit to the "pending" bitmask
> and returns.  unblock_signals checks this mask to see if any signals
> need to be delivered.
> 
> The crucial part is this:
> 	signals_enabled = 1;
> 	save_pending = pending;
> 	if(save_pending == 0)
> 		return;
> 	pending = 0;
> 
> In order to avoid an interrupt arriving between reading pending and
> setting it to zero, in which case, the record of the interrupt would
> be erased, signals are enabled.
> 
> What happened was that gcc reordered this so that 'save_pending = pending'
> came before 'signals_enabled = 1', creating a one-instruction window
> within which an interrupt could arrive, set its bit in pending, and
> have it be immediately erased.

So you need a compiler barrier, not a memory barrier.

> When the I/O workload is purely disk-based, the loss of a block device
> interrupt stops the entire I/O system because the next block request
> will wait for the current one to finish.  Thus the system hangs until
> something else causes some I/O to arrive, such as a network packet or
> console input.
> 
> The fix to this particular problem is a memory barrier between
> enabling signals and reading the pending signal mask.  An xchg would
> also probably work.
> 
> Looking over this code for similar problems led me to do a few more
> things -
> 	make signals_enabled and pending volatile so that they don't
> get cached in registers
> 	add an mb() to the return paths of block_signals and
> unblock_signals so that the modification of signals_enabled doesn't
> get shuffled into the caller in the event that these are inlined in
> the future.
> 
> Signed-off-by: Jeff Dike <jdike@addtoit.com>
> 
> Index: linux-2.6.18-mm/arch/um/include/sysdep-i386/barrier.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.18-mm/arch/um/include/sysdep-i386/barrier.h	2006-10-31 14:41:52.000000000 -0500
> @@ -0,0 +1,9 @@
> +#ifndef __SYSDEP_I386_BARRIER_H
> +#define __SYSDEP_I386_BARRIER_H
> +
> +/* Copied from include/asm-i386 for use by userspace.  i386 has the option
> + * of using mfence, but I'm just using this, which works everywhere, for now.
> + */
> +#define mb() asm volatile("lock; addl $0,0(%esp)")

That's a memory barrier, which is more expensive.

This:

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

should suffice?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/