Re: Dumb question: BKL on reboot ?

[Andrea Arcangeli <andrea@suse.de>]

On Thu, Aug 21, 2003 at 05:55:49PM +0200, Hannes Reinecke wrote:
> Andrea Arcangeli wrote:
> >On Thu, Aug 21, 2003 at 10:05:20AM +0200, Hannes Reinecke wrote:
> [ .. ]
> >>
> >>Exactly what happened here. CPU#1 entered sys_reboot, got BKL and 
> >>prepared to stop. It will never release BKL, leaving a nice big window 
> >>for CPU#0 to deadlock.
> >
> >
> >if that was really the case it shouldn't deadlock, because CPU0
> >shouldn't depend on a big kernel lock to be able to re-enable the irqs,
> >and to receive the IPI. Nor any IPI handler could ever be allowed to
> >take the big kernel lock.
> >
> >Unless you can demonstrate a dependency on the big kernel lock for CPU0
> >to re-enable the irqs eventually, I can't see how the above could
> >deadlock.
> >
> 
> What happened was this:
> 
> ================================================================
> TASK HAS CPU (0): 0x904000 (kupdated):
>  LOWCORE INFO:
>   -psw      : 0x400100180000000 0x0000000007aa22
>   -function : sync_old_buffers+58
>   -prefix   : 0x009c8000
>   -cpu timer: 0xfffffa77 0x33465f80
>   -clock cmp: 0x00b9e503 0x235324f4
>   -general registers:
>      0000000000000000 0x000000002e1000
>      0x0000000007aa22 0x0000000004919e
>      0x00000000907f28 0x0000000022b428
>      0000000000000000 0000000000000000
>      0x00000000224340 0000000000000000
>      0x00000000970000 0x00000000233b00
>      0x00000000904000 0x000000001deb60
>      0x00000000907e78 0x00000000907dd8
> [ .. ]
>  STACK:
>  0 kupdate+424 [0x7af4c]
>  1 arch_kernel_thread+70 [0x18d72]
> ================================================================
> TASK HAS CPU (1): 0x3318000 (reboot):
>  LOWCORE INFO:
>   -psw      : 0x700100180000000 0x0000007ffe0d8e
>   -function : not found in System map
>   -prefix   : 0x009c2000
>   -cpu timer: 0xfff79e2d 0x014244c0
>   -clock cmp: 0x00b9e503 0x3a5d9504
>   -general registers:
>      0x00000000000001 0000000000000000
>      0000000000000000 0000000000000000
>      0x0000000001f268 0000000000000000
>      0x00000000000003 0x000000000002c0
>      0x000000000490f0 0x00000003318000
>      0x00000000016422 0000000000000000
>      0x00000000000001 0x000000001da280
>      0x0000000001f268 0x0000000331bae0
> [ .. ]
>  STACK:
>  0 machine_restart_smp+62 [0x1f5f2]
>  1 machine_restart+48 [0x19884]
>  2 sys_reboot+306 [0x49222]
>  3 sysc_noemu+16 [0x16242]

This can't be the lock_kernel, you see, there's no lock_kernel
invocation at all in the machine_restart_smp path.

> 
> Not sure why CPU#0 wanted to execute kupdate(), but hey ...

kupdate always runs in the background, once every few seconds.

I perfectly see why it hangs on s390, that's a bug in the arch code
and it's unrelated to whatever lock_kernel and even unrelated to the
potential deadlock prone shutdown of cpus that I mentioned in my
previous email:

static void do_machine_restart(void * __unused)
{
	clear_bit(smp_processor_id(), &cpu_restart_map);
	if (smp_processor_id() == 0) {
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

if the `reboot` program doesn't run by luck on cpu 0, it will be the one
that will hang instead of the others. the above check must be changed to:

	smp_processor_id() == reboot_cpu

and you should snapshot reboot_cpu in machine_restart_smp. then it won't
hang anymore.

The x86 code may be safe in hanging in the IPI if it's extremely careful
in not touching any lock that maybe hold by other cpus, between the IPI
delivery and the hard reboot. So probably we don't need to replace the
IPI with a kernel thread in 2.4, despite I find the kernel thread way
more robust.

Overall the lock_kernel I think is _needed_ (not only if something it's
safer), to serialize reboot against reboot and I wouldn't bother
changing it to another spinlock (at least not in 2.4)

Andrea