Message-ID: <20160322021343.GC2279@X58A-UD3R>
Date:	Tue, 22 Mar 2016 11:13:43 +0900
From:	Byungchul Park <byungchul.park@....com>
To:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jan Kara <jack@...e.com>, Petr Mladek <pmladek@...e.com>,
	Tejun Heo <tj@...nel.org>,
	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	linux-kernel@...r.kernel.org, Jan Kara <jack@...e.cz>
Subject: Re: [RFC][PATCH v5 1/2] printk: Make printk() completely async

On Mon, Mar 21, 2016 at 06:28:48PM +0900, Sergey Senozhatsky wrote:
> > Just to be sure, whether you take my suggestion or not is not important.
> > I just suggested it in this thread since it can solve what you worried
> > about. That's all. I can post it in another thread though. Why don't you
> > consider it so that yours don't miss any printk message? Do you think there
> > are any problems in my suggestion?
> 
> we have 2 spin locks in vprintk_emit() -- logbuf_lock and sem->lock. and N
> CPUs can concurrently lockup on those two locks, which already makes a
> single static pointer in spiun_dump() questionable.

As I already said, the static pointer is for an infinite recursion within a
printk() in the case "lockup suspected". It can prevent at least
meaningless *infinite* recursion at the first place the recursion occured.
Once the single static variable works, that is, it solves the recursion,
the next recusion can be solved if it exists, that is, more than two locks
are locked-up. It will be helpful for understanding what I am saying if you
think about how the infinite recursion can happen. So a single static
variable is enough to prevent a *infinite* recursion.

CPU0                    CPU1

spin_lock(A)            printk()
(not release A)            spin_lock(A)
                              spin_dump() // lockup suspected
			         printk()
				    spint_lock(A)
				       ...

where A can be logbuf_lock, sem->lock, rq->lock, p->pi_lock or any lock
which is used within printk(). For your information, logbuf_lock can also
cause an infinite recursion at the first entrance on a CPU.

> logbug_lock *theoretically* can detect and handle recursive printk()s,
> there is no way to catch sem->lock spin_dump() at the moment (but that's
> not the point).

It's the reason why I suggested the way to avoid an inifinit recursion in
the spin debug facitity in the case "lockup suspected".

> there are 2 new spin locks in vprintk_emit() -- p->pi_lock and rq->lock.
> what I want is to put those locks inside the "we can detect recursion 100%"
> region. so these two locks will not add any new possibilities of recursive
> printks, they are covered by the existing recursion detection code thanks
> to logbuf lock and static logbuf_cpu. so we still can say that we have 5

I think, even though we can detect the recursion by the logbuf_cpu, we
should not use it for the purpose of detecting since it's the last resort
preventing an infinite recursion. It seems to be same, it's different
purpose. Now you decided to put it out of the critical section, this
arguing is not much important though.. :-(

> places where printk recursion can happen
> 
> -- lock + unlock logbuf_lock
>    printk() recursion detection code can't help here
> 
> -- inside of logbuf_lock critical section
>    printk() recursion detection code works here
> 
> -- lock + unlock sem->lock
>    printk() recursion detection code can't help here
> 
> 
> note how "inside of logbuf_lock critical section" takes care of nested
> 'lock + unlock p->pi_lock' and 'lock + unlock rq->lock'.

I got what you intended to do. You don't need to explain in more detail.

> moreover, printk() will switch to synchronous mode in recursion handler and
> two misbehaving spin locks (4 places where recursion can happen) will not be
> executed anymore.

This, too.

> what you want to have -- 4 independent spin locks and 9 places where
> recursion can happen, only 1 of which is covered by printk recursion code.
> 
> -- lock + unlock logbuf_lock
>    printk() recursion detection code can't help here
> 
> -- inside of logbuf_lock critical section
>    printk() recursion detection code works here
> 
> -- lock + unlock p->pi_lock
>    printk() recursion detection code can't help here
> 
> -- lock + unlock rq->lock
>    printk() recursion detection code can't help here
> 
> -- lock + unlock sem->lock
>    printk() recursion detection code can't help here
> 
> and there is a static pointer to fix everything up? what if 2

We can use a single static pointer to fix it as I said.

> CPUs will simultaneously printk-recurse in 2 different places?

As I said, 2 different places mean 2 different timings, in the case of
infinite recursion. I think I explained fully. Do I need to explain more?

Thanks,
Byungchul

> why this is better?
> 
> 	-ss

Powered by blists - more mailing lists