Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1530328 > unrolled thread

Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function Petr Mladek <pmladek@suse.com> - 2016-11-25 16:20 +0100
    Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function Peter Zijlstra <peterz@infradead.org> - 2016-11-25 16:20 +0100
      Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-12-01 03:40 +0100
        Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function Peter Zijlstra <peterz@infradead.org> - 2016-12-01 06:50 +0100
          Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function Petr Mladek <pmladek@suse.com> - 2016-12-01 14:40 +0100
            Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-12-02 02:20 +0100
    Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-12-01 03:20 +0100
    Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-12-01 14:00 +0100
      Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function Petr Mladek <pmladek@suse.com> - 2016-12-01 14:20 +0100

#1530328 — Re: [RFC][PATCHv4 6/6] printk: remove zap_locks() function

On Fri 2016-10-28 00:49:33, Sergey Senozhatsky wrote:
> We use printk-safe now which makes printk-recursion detection code
> in vprintk_emit() is unreachable. The tricky thing here is that,
     		    ^^ superfluous "is"

> apart from detecting and reporting printk recursions, that code also
> used to zap_lockc() in case of panic. However, zap_locks() does not
       	          ^

s/zap_lockc/zap_locks/

> look to be needed anymore:
> 
> 1) Since commit 08d78658f393 ("panic: release stale console lock to
>    always get the logbuf printed out") panic flushing of `logbuf' to
>    console ignores the state of `console_sem' by doing
>    	panic()
> 		console_trylock();
> 		console_unlock();
> 
> 2) Since commit cf9b1106c81c ("printk/nmi: flush NMI messages on the
>    system panic") panic attempts to zap the `logbuf_lock' spin_lock to
>    successfully flush nmi messages to `logbuf'.

Note that the same code is newly used to flush also the printk_safe
per-CPU buffers. It means that logbuf_lock is zapped also when
flushing these new buffers.


> Basically, it seems that we either already do what zap_locks() used to
> do but in other places or we ignore the state of the lock. May be we
> still would want to do sema_init() in printk_safe_flush_on_panic(),
> just in case.

Very good question! I would actually suggest to use printk_deferred()
in printk_safe_flush_on_panic() in any context. It will solve the
problems discussed for the 4th patch of this patchset. And it will
solve also this problem. In case of panic, we should first try to
get all messages into the logbuffer so that they are visible in
the crash dump. We try to push them to the console by
console_flush_on_panic() later because it is more risky.

> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

If we avoid calling console in printk_safe_flush_on_panic(),
feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[toc] | [next] | [standalone]

#1530329

On Fri, Nov 25, 2016 at 04:01:13PM +0100, Petr Mladek wrote:
> On Fri 2016-10-28 00:49:33, Sergey Senozhatsky wrote:
> > 2) Since commit cf9b1106c81c ("printk/nmi: flush NMI messages on the
> >    system panic") panic attempts to zap the `logbuf_lock' spin_lock to
> >    successfully flush nmi messages to `logbuf'.
> 
> Note that the same code is newly used to flush also the printk_safe
> per-CPU buffers. It means that logbuf_lock is zapped also when
> flushing these new buffers.
> 

Note that (raw_)spin_lock_init() as done here and in
printk_nmi_flush_on_panic() can wreck the lock state and doesn't ensure
a subsequent spin_lock() of said lock will actually work.

The very best solution is to simply ignore the lock in panic situations
rather than trying to wreck it.

[toc] | [prev] | [next] | [standalone]

#1533758

On (11/25/16 16:17), Peter Zijlstra wrote:
> On Fri, Nov 25, 2016 at 04:01:13PM +0100, Petr Mladek wrote:
> > On Fri 2016-10-28 00:49:33, Sergey Senozhatsky wrote:
> > > 2) Since commit cf9b1106c81c ("printk/nmi: flush NMI messages on the
> > >    system panic") panic attempts to zap the `logbuf_lock' spin_lock to
> > >    successfully flush nmi messages to `logbuf'.
> > 
> > Note that the same code is newly used to flush also the printk_safe
> > per-CPU buffers. It means that logbuf_lock is zapped also when
> > flushing these new buffers.
> > 
> 
> Note that (raw_)spin_lock_init() as done here and in
> printk_nmi_flush_on_panic() can wreck the lock state and doesn't ensure
> a subsequent spin_lock() of said lock will actually work.
> 
> The very best solution is to simply ignore the lock in panic situations
> rather than trying to wreck it.

do you mean that we can enterily drop the spin_lock_init()? or is there
something else? spin_lock_init() either does not improve anything or let
us to, at least, move the messages from per-CPU buffers to the logbuf. so
it's not like it does some damage, and it can help sometimes. though I
agree that a) we have the messages in the memory already and b) logbuf_lock
is not the one&only troubling lock.

	-ss

[toc] | [prev] | [next] | [standalone]

#1533820

On Thu, Dec 01, 2016 at 11:34:42AM +0900, Sergey Senozhatsky wrote:
> On (11/25/16 16:17), Peter Zijlstra wrote:
> > On Fri, Nov 25, 2016 at 04:01:13PM +0100, Petr Mladek wrote:
> > > On Fri 2016-10-28 00:49:33, Sergey Senozhatsky wrote:
> > > > 2) Since commit cf9b1106c81c ("printk/nmi: flush NMI messages on the
> > > >    system panic") panic attempts to zap the `logbuf_lock' spin_lock to
> > > >    successfully flush nmi messages to `logbuf'.
> > > 
> > > Note that the same code is newly used to flush also the printk_safe
> > > per-CPU buffers. It means that logbuf_lock is zapped also when
> > > flushing these new buffers.
> > > 
> > 
> > Note that (raw_)spin_lock_init() as done here and in
> > printk_nmi_flush_on_panic() can wreck the lock state and doesn't ensure
> > a subsequent spin_lock() of said lock will actually work.
> > 
> > The very best solution is to simply ignore the lock in panic situations
> > rather than trying to wreck it.
> 
> do you mean that we can enterily drop the spin_lock_init()? or is there
> something else?

You should not touch the lock in any way shape or form in the panic
path. Just ignore all locking and do the console writes (which gets you
into whole different pile of crap).

Put another way, don't do silly things like spin_lock() when you're in a
hurry to get your panics out.

> spin_lock_init() either does not improve anything or let
> us to, at least, move the messages from per-CPU buffers to the logbuf.

So spin_lock_init() will completely wreck the lock. And this being the
recursion path, not a panic path, we could have continued running the
kernel no problem.

So yes, its actively harmful. You turn a benign printk() into something
that will wreck the kernel.

[toc] | [prev] | [next] | [standalone]

#1534093

On Thu 2016-12-01 06:42:29, Peter Zijlstra wrote:
> On Thu, Dec 01, 2016 at 11:34:42AM +0900, Sergey Senozhatsky wrote:
> > On (11/25/16 16:17), Peter Zijlstra wrote:
> > > On Fri, Nov 25, 2016 at 04:01:13PM +0100, Petr Mladek wrote:
> > > > On Fri 2016-10-28 00:49:33, Sergey Senozhatsky wrote:
> > > > > 2) Since commit cf9b1106c81c ("printk/nmi: flush NMI messages on the
> > > > >    system panic") panic attempts to zap the `logbuf_lock' spin_lock to
> > > > >    successfully flush nmi messages to `logbuf'.
> > > > 
> > > > Note that the same code is newly used to flush also the printk_safe
> > > > per-CPU buffers. It means that logbuf_lock is zapped also when
> > > > flushing these new buffers.
> > > > 
> > > 
> > > Note that (raw_)spin_lock_init() as done here and in
> > > printk_nmi_flush_on_panic() can wreck the lock state and doesn't ensure
> > > a subsequent spin_lock() of said lock will actually work.
> > > 
> > > The very best solution is to simply ignore the lock in panic situations
> > > rather than trying to wreck it.
> > 
> > do you mean that we can enterily drop the spin_lock_init()? or is there
> > something else?
> 
> You should not touch the lock in any way shape or form in the panic
> path. Just ignore all locking and do the console writes (which gets you
> into whole different pile of crap).

And this is my fear. I am not sure if the other crap is better than
the current one.

One crazy idea. A compromise might be to switch into a timelimed locking
in the panic mode when there are still more CPUs active. If a spin
lock is not available within X thousands of cycles, there is probably
a deadlock and we should just enter the critical section. It would
preserve some reasonable synchronization but it will allow to move
forward.

Another solution would be to use the temporary buffers if the lock
is not available and push it into the main buffer and consoles later
when there is only one CPU running. In this stage, we do not need
to synchronize and could just skip locking as you suggest.


> Put another way, don't do silly things like spin_lock() when you're in a
> hurry to get your panics out.
> 
> > spin_lock_init() either does not improve anything or let
> > us to, at least, move the messages from per-CPU buffers to the logbuf.
> 
> So spin_lock_init() will completely wreck the lock. And this being the
> recursion path, not a panic path, we could have continued running the
> kernel no problem.

printk_nmi_flush_on_panic() is called from panic(). It means that we
will do this only when the system is really going down. Which is a nice
improvement. The current code zaps the locks during any Oops.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1534610

On (12/01/16 14:36), Petr Mladek wrote:
[..]
> > > > > Note that the same code is newly used to flush also the printk_safe
> > > > > per-CPU buffers. It means that logbuf_lock is zapped also when
> > > > > flushing these new buffers.
> > > > > 
> > > > 
> > > > Note that (raw_)spin_lock_init() as done here and in
> > > > printk_nmi_flush_on_panic() can wreck the lock state and doesn't ensure
> > > > a subsequent spin_lock() of said lock will actually work.
> > > > 
> > > > The very best solution is to simply ignore the lock in panic situations
> > > > rather than trying to wreck it.
> > > 
> > > do you mean that we can enterily drop the spin_lock_init()? or is there
> > > something else?
> > 
> > You should not touch the lock in any way shape or form in the panic
> > path. Just ignore all locking and do the console writes (which gets you
> > into whole different pile of crap).
> 
> And this is my fear. I am not sure if the other crap is better than
> the current one.

yeah, that's a good point.

> One crazy idea. A compromise might be to switch into a timelimed locking
> in the panic mode when there are still more CPUs active. If a spin
> lock is not available within X thousands of cycles, there is probably
> a deadlock and we should just enter the critical section. It would
> preserve some reasonable synchronization but it will allow to move
> forward.

logbuf spin_lock is just one of the locks. we also have scheduler spinlocks,
console drivers spinlocks, semaphore spinlock, etc. the messages, on the other
hand, are already in the memory (per-CPU buffers), so they will make it into
the core file (if there will be one).

> Another solution would be to use the temporary buffers if the lock
> is not available and push it into the main buffer and consoles later
> when there is only one CPU running. In this stage, we do not need
> to synchronize and could just skip locking as you suggest.

that's interesting. the problem here is that smp_send_stop() does not
guarantee that all the remaining CPUs will stop by the time it returns

arch/arm/kernel/smp.c

	void smp_send_stop(void)
	{
		unsigned long timeout;
		struct cpumask mask;

		cpumask_copy(&mask, cpu_online_mask);
		cpumask_clear_cpu(smp_processor_id(), &mask);
		if (!cpumask_empty(&mask))
			smp_cross_call(&mask, IPI_CPU_STOP);

		/* Wait up to one second for other CPUs to stop */
		timeout = USEC_PER_SEC;
		while (num_online_cpus() > 1 && timeout--)
			udelay(1);

		if (num_online_cpus() > 1)
			pr_warn("SMP: failed to stop secondary CPUs\n");
	}


> > Put another way, don't do silly things like spin_lock() when you're in a
> > hurry to get your panics out.
> > 
> > > spin_lock_init() either does not improve anything or let
> > > us to, at least, move the messages from per-CPU buffers to the logbuf.
> > 
> > So spin_lock_init() will completely wreck the lock. And this being the
> > recursion path, not a panic path, we could have continued running the
> > kernel no problem.
> 
> printk_nmi_flush_on_panic() is called from panic(). It means that we
> will do this only when the system is really going down. Which is a nice
> improvement. The current code zaps the locks during any Oops.

correct. well, not any oops, but 'oops && printk recursion' combo

	if (unlikely(logbuf_cpu == this_cpu)) {
		/*
		 * If a crash is occurring during printk() on this CPU,
		 * then try to get the crash message out but make sure
		 * we can't deadlock. Otherwise just return to avoid the
		 * recursion and return - but flag the recursion so that
		 * it can be printed at the next appropriate moment:
		 */
		if (!oops_in_progress && !lockdep_recursing(current)) {
			recursion_bug = true;
			local_irq_restore(flags);
			return 0;
		}
		zap_locks();
	}

other than that - yes, now we do (...we are going to do) it only
from the panic() path.

	-ss

[toc] | [prev] | [next] | [standalone]

#1533749

On (11/25/16 16:01), Petr Mladek wrote:
> On Fri 2016-10-28 00:49:33, Sergey Senozhatsky wrote:
> > We use printk-safe now which makes printk-recursion detection code
> > in vprintk_emit() is unreachable. The tricky thing here is that,
>      		    ^^ superfluous "is"
> 
> > apart from detecting and reporting printk recursions, that code also
> > used to zap_lockc() in case of panic. However, zap_locks() does not
>        	          ^
> 
> s/zap_lockc/zap_locks/
> 
> > look to be needed anymore:
> > 
> > 1) Since commit 08d78658f393 ("panic: release stale console lock to
> >    always get the logbuf printed out") panic flushing of `logbuf' to
> >    console ignores the state of `console_sem' by doing
> >    	panic()
> > 		console_trylock();
> > 		console_unlock();
> > 
> > 2) Since commit cf9b1106c81c ("printk/nmi: flush NMI messages on the
> >    system panic") panic attempts to zap the `logbuf_lock' spin_lock to
> >    successfully flush nmi messages to `logbuf'.
> 
> Note that the same code is newly used to flush also the printk_safe
> per-CPU buffers. It means that logbuf_lock is zapped also when
> flushing these new buffers.

yes, and I'm a bit skeptical about the whole re-init logbuf_lock,
because this lock is just one of possibly many looks that can be
'locked'. the messages are already in per-CPU buffers, so they
will present in a core dump file.

but it can be done separately, not in this series.

> > Basically, it seems that we either already do what zap_locks() used to
> > do but in other places or we ignore the state of the lock. May be we
> > still would want to do sema_init() in printk_safe_flush_on_panic(),
> > just in case.
> 
> Very good question! I would actually suggest to use printk_deferred()
> in printk_safe_flush_on_panic() in any context. It will solve the
> problems discussed for the 4th patch of this patchset. And it will
> solve also this problem. In case of panic, we should first try to
> get all messages into the logbuffer so that they are visible in
> the crash dump. We try to push them to the console by
> console_flush_on_panic() later because it is more risky.
>
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> If we avoid calling console in printk_safe_flush_on_panic(),
> feel free to use:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

ok. thanks.

	-ss

[toc] | [prev] | [next] | [standalone]

#1534069

On (11/25/16 16:01), Petr Mladek wrote:
[..]
> > apart from detecting and reporting printk recursions, that code also
> > used to zap_lockc() in case of panic. However, zap_locks() does not
>        	          ^
> 
> s/zap_lockc/zap_locks/
> 
> > look to be needed anymore:
> > 
> > 1) Since commit 08d78658f393 ("panic: release stale console lock to
> >    always get the logbuf printed out") panic flushing of `logbuf' to
> >    console ignores the state of `console_sem' by doing
> >    	panic()
> > 		console_trylock();
> > 		console_unlock();
> > 
> > 2) Since commit cf9b1106c81c ("printk/nmi: flush NMI messages on the
> >    system panic") panic attempts to zap the `logbuf_lock' spin_lock to
> >    successfully flush nmi messages to `logbuf'.
> 
> Note that the same code is newly used to flush also the printk_safe
> per-CPU buffers. It means that logbuf_lock is zapped also when
> flushing these new buffers.


not quite. there are several checks that would prevent multiple logbuf_lock
re-inits:


        if (in_nmi() && raw_spin_is_locked(&logbuf_lock)) {
                if (num_online_cpus() > 1)
                        return;

                debug_locks_off();
                raw_spin_lock_init(&logbuf_lock);
        }


	-ss

[toc] | [prev] | [next] | [standalone]

#1534082

On Thu 2016-12-01 21:50:45, Sergey Senozhatsky wrote:
> On (11/25/16 16:01), Petr Mladek wrote:
> [..]
> > > apart from detecting and reporting printk recursions, that code also
> > > used to zap_lockc() in case of panic. However, zap_locks() does not
> >        	          ^
> > 
> > s/zap_lockc/zap_locks/
> > 
> > > look to be needed anymore:
> > > 
> > > 1) Since commit 08d78658f393 ("panic: release stale console lock to
> > >    always get the logbuf printed out") panic flushing of `logbuf' to
> > >    console ignores the state of `console_sem' by doing
> > >    	panic()
> > > 		console_trylock();
> > > 		console_unlock();
> > > 
> > > 2) Since commit cf9b1106c81c ("printk/nmi: flush NMI messages on the
> > >    system panic") panic attempts to zap the `logbuf_lock' spin_lock to
> > >    successfully flush nmi messages to `logbuf'.
> > 
> > Note that the same code is newly used to flush also the printk_safe
> > per-CPU buffers. It means that logbuf_lock is zapped also when
> > flushing these new buffers.
> 
> 
> not quite. there are several checks that would prevent multiple logbuf_lock
> re-inits:

Ah, I did not meant that the lock was zapped twice. I wanted to say
that we added a new path and that it was handled as well :-)

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web