Message-ID: <20160127011454.GB1612@swordfish>
Date:	Wed, 27 Jan 2016 10:14:54 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	byungchul.park@....com
Cc:	akpm@...ux-foundation.org, akinobu.mita@...il.com, jack@...e.cz,
	mingo@...nel.org, mm-commits@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: +
 lib-spinlock_debugc-prevent-an-infinite-recursive-cycle-in-spin_dump.patch
 added to -mm tree

On (01/26/16 16:12), akpm@...ux-foundation.org wrote:
[..]
> There is an infinite recursive cycle when using CONFIG_DEBUG_SPINLOCK, in
> spin_dump().  Backtrace prints printk() -> console_trylock() ->
> do_raw_spin_lock() -> spin_bug() -> spin_dump() -> printk()... 
> infinitely.

is it even possible to lockup on a semaphore's spin_lock?

int down_trylock(struct semaphore *sem)
{
	unsigned long flags;
	int count;

	raw_spin_lock_irqsave(&sem->lock, flags);
			^^^^ here?
	count = sem->count - 1;
	if (likely(count >= 0))
		sem->count = count;
	raw_spin_unlock_irqrestore(&sem->lock, flags);

	return (count < 0);
}

under what circumstances and why it should be silenced? a memory corruption?
or is it the 'logbuf_lock' spin_lock that was meant to be in the report?
what if we lockup on `logbuf_lock`, it will generate the same call-chain...

> If the spin_bug() is called from a function like printk() which is trying
> to obtain the console lock, we should prevent the debug spinlock code from
> calling printk() again in that context.

even if it was the 'logbuf_lock' spin_lock then still, we take it for quite
short periods of time with IRQs disabled:

in vprintk_emit(), when sprintf text and store it

	local_irq_save()
	raw_spin_lock()
		vscnprintf()
		log_store()
	raw_spin_unlock()
	local_irq_restore()


and in console_unlock() when we read it back

	for (;;) {
		raw_spin_lock_irqsave(&logbuf_lock, flags);
			msg_print_text
		raw_spin_unlock(&logbuf_lock)
			call_console_drivers()
		local_irq_restore
	}


so if the CPU that owns the spin_lock somehow managed to keep it forever
(due to a memory corruption... or something has powered off the cpu
core???) -- then _this is_ the problem, not the fact that other CPUs will
not lock the spin_lock anymore.

so I don't think this patch does the right thing, sorry.

	-ss

> Signed-off-by: Byungchul Park <byungchul.park@....com>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Akinobu Mita <akinobu.mita@...il.com>
> Cc: Jan Kara <jack@...e.cz>
> Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
> ---
> 
>  kernel/locking/spinlock_debug.c |   11 +++++++++++
>  kernel/printk/printk.c          |    5 +++++
>  2 files changed, 16 insertions(+)
> 
> diff -puN kernel/locking/spinlock_debug.c~lib-spinlock_debugc-prevent-an-infinite-recursive-cycle-in-spin_dump kernel/locking/spinlock_debug.c
> --- a/kernel/locking/spinlock_debug.c~lib-spinlock_debugc-prevent-an-infinite-recursive-cycle-in-spin_dump
> +++ a/kernel/locking/spinlock_debug.c
> @@ -67,11 +67,22 @@ static void spin_dump(raw_spinlock_t *lo
>  	dump_stack();
>  }
>  
> +extern int is_console_lock(raw_spinlock_t *lock);
> +
>  static void spin_bug(raw_spinlock_t *lock, const char *msg)
>  {
>  	if (!debug_locks_off())
>  		return;
>  
> +	/*
> +	 * If this function is called from a function like printk()
> +	 * which is trying to obtain the console lock, then we should
> +	 * not call printk() any more. Or it will cause an infinite
> +	 * recursive cycle!
> +	 */
> +	if (unlikely(is_console_lock(lock)))
> +		return;
> +
>  	spin_dump(lock, msg);
>  }
>  
> diff -puN kernel/printk/printk.c~lib-spinlock_debugc-prevent-an-infinite-recursive-cycle-in-spin_dump kernel/printk/printk.c
> --- a/kernel/printk/printk.c~lib-spinlock_debugc-prevent-an-infinite-recursive-cycle-in-spin_dump
> +++ a/kernel/printk/printk.c
> @@ -120,6 +120,11 @@ static int __down_trylock_console_sem(un
>  	up(&console_sem);\
>  } while (0)
>  
> +int is_console_lock(raw_spinlock_t *lock)
> +{
> +	return &console_sem.lock == lock;
> +}
> +
>  /*
>   * This is used for debugging the mess that is the VT code by
>   * keeping track if we have the console semaphore held. It's
> _
> 
> Patches currently in -mm which might be from byungchul.park@....com are
> 
> lib-spinlock_debugc-prevent-an-infinite-recursive-cycle-in-spin_dump.patch
> 
> --
> To unsubscribe from this list: send the line "unsubscribe mm-commits" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Powered by blists - more mailing lists