All of lore.kernel.org
 help / color / mirror / Atom feed
From: Qian Cai <cai@lca.pw>
To: Michal Hocko <mhocko@kernel.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>,
	Petr Mladek <pmladek@suse.com>,
	Peter Oberparleiter <oberpar@linux.ibm.com>,
	akpm@linux-foundation.org, sergey.senozhatsky.work@gmail.com,
	rostedt@goodmis.org, peterz@infradead.org, linux-mm@kvack.org,
	john.ogness@linutronix.de, david@redhat.com,
	linux-kernel@vger.kernel.org,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>
Subject: Re: [PATCH v2] mm/page_isolation: fix a deadlock with printk()
Date: Tue, 08 Oct 2019 15:06:13 -0400	[thread overview]
Message-ID: <1570561573.5576.307.camel@lca.pw> (raw)
In-Reply-To: <20191008183525.GQ6681@dhcp22.suse.cz>

On Tue, 2019-10-08 at 20:35 +0200, Michal Hocko wrote:
> On Tue 08-10-19 12:08:37, Qian Cai wrote:
> > On Tue, 2019-10-08 at 14:56 +0200, Christian Borntraeger wrote:
> > > Adding Peter Oberparleiter.
> > > Peter, can you have a look?
> > > 
> > > On 08.10.19 10:27, Michal Hocko wrote:
> > > > On Tue 08-10-19 09:43:57, Petr Mladek wrote:
> > > > > On Mon 2019-10-07 16:49:37, Michal Hocko wrote:
> > > > > > [Cc s390 maintainers - the lockdep is http://lkml.kernel.org/r/1570228005-24979-1-git-send-email-cai@lca.pw
> > > > > >  Petr has explained it is a false positive
> > > > > >  http://lkml.kernel.org/r/20191007143002.l37bt2lzqtnqjqxu@pathway.suse.cz]
> > > > > > On Mon 07-10-19 16:30:02, Petr Mladek wrote:
> > > > > > [...]
> > > > > > > I believe that it cannot really happen because:
> > > > > > > 
> > > > > > > 	static int __init
> > > > > > > 	sclp_console_init(void)
> > > > > > > 	{
> > > > > > > 	[...]
> > > > > > > 		rc = sclp_rw_init();
> > > > > > > 	[...]
> > > > > > > 		register_console(&sclp_console);
> > > > > > > 		return 0;
> > > > > > > 	}
> > > > > > > 
> > > > > > > sclp_rw_init() is called before register_console(). And
> > > > > > > console_unlock() will never call sclp_console_write() before
> > > > > > > the console is registered.
> > > > > > > 
> > > > > > > AFAIK, lockdep only compares existing chain of locks. It does
> > > > > > > not know about console registration that would make some
> > > > > > > code paths mutually exclusive.
> > > > > > > 
> > > > > > > I believe that it is a false positive. I do not know how to
> > > > > > > avoid this lockdep report. I hope that it will disappear
> > > > > > > by deferring all printk() calls rather soon.
> > > > > > 
> > > > > > Thanks a lot for looking into this Petr. I have also checked the code
> > > > > > and I really fail to see why the allocation has to be done under the
> > > > > > lock in the first place. sclp_read_sccb and sclp_init_sccb are global
> > > > > > variables but I strongly suspect that they need a synchronization during
> > > > > > early init, callbacks are registered only later IIUC:
> > > > > 
> > > > > Good idea. It would work when the init function is called only once.
> > > > > But see below.
> > > > > 
> > > > > > diff --git a/drivers/s390/char/sclp.c b/drivers/s390/char/sclp.c
> > > > > > index d2ab3f07c008..4b1c033e3255 100644
> > > > > > --- a/drivers/s390/char/sclp.c
> > > > > > +++ b/drivers/s390/char/sclp.c
> > > > > > @@ -1169,13 +1169,13 @@ sclp_init(void)
> > > > > >  	unsigned long flags;
> > > > > >  	int rc = 0;
> > > > > >  
> > > > > > +	sclp_read_sccb = (void *) __get_free_page(GFP_ATOMIC | GFP_DMA);
> > > > > > +	sclp_init_sccb = (void *) __get_free_page(GFP_ATOMIC | GFP_DMA);
> > > > > >  	spin_lock_irqsave(&sclp_lock, flags);
> > > > > >  	/* Check for previous or running initialization */
> > > > > >  	if (sclp_init_state != sclp_init_state_uninitialized)
> > > > > >  		goto fail_unlock;
> > > > > 
> > > > > It seems that sclp_init() could be called several times in parallel.
> > > > > I see it called from sclp_register() and sclp_initcall().
> > > > 
> > > > Interesting. Something for s390 people to answer I guess.
> > > > Anyway, this should be quite trivial to workaround by a cmpxch or alike.
> > > > 
> > 
> > The above fix is simply insufficient,
> 
> Isn't this yet another init time lockdep false possitive?

Again, this is not 100% false positive for sure yet.

> 
> > 00: [    3.654337] -> #3 (console_owner){....}:                                 
> > 00: [    3.654343]        lock_acquire+0x21a/0x468                              
> > 00: [    3.654345]        console_unlock+0x3a6/0xa30                            
> > 00: [    3.654346]        vprintk_emit+0x184/0x3c8                              
> > 00: [    3.654348]        vprintk_default+0x44/0x50                             
> > 00: [    3.654349]        printk+0xa8/0xc0                                      
> > 00: [    3.654351]        get_random_u64+0x40/0x108                             
> > 00: [    3.654360]        add_to_free_area_random+0x188/0x1c0                   
> > 00: [    3.654364]        free_one_page+0x72/0x128                              
> > 00: [    3.654366]        __free_pages_ok+0x51c/0xca0                           
> > 00: [    3.654368]        memblock_free_all+0x30a/0x3b0                         
> > 00: [    3.654370]        mem_init+0x84/0x200                                   
> > 00: [    3.654371]        start_kernel+0x384/0x6a0                              
> > 00: [    3.654373]        startup_continue+0x70/0xd0                            
> 
> This one is actually a nice example why trying to get printk out of the
> zone->lock is simply not viable. This one is likely a printk to warn
> that the random pool is not fully intiailized. Just because the
> allocator tries to randomize the initial free memory pool. You are not
> going to remove that printk, right?

Well, Sergey had a patch to convert that one to printk_deferred(), but even with
his patch, it will still trigger the lockdep splat here because the lock
dependency between zone->lock --> console_owner is still there from memory
offline.

> 
> I fully agree that this class of lockdep splats are annoying especially
> when they make the lockdep unusable but please discuss this with lockdep
> maintainers and try to find some solution rather than go and try to
> workaround the problem all over the place. If there are places that
> would result in a cleaner code then go for it but please do not make the
> code worse just because of a non existent problem flagged by a false
> positive.

It makes me wonder what make you think it is a false positive for sure.

  reply	other threads:[~2019-10-08 19:06 UTC|newest]

Thread overview: 97+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-04 22:26 [PATCH v2] mm/page_isolation: fix a deadlock with printk() Qian Cai
2019-10-07  8:07 ` Michal Hocko
2019-10-07  9:05   ` Petr Mladek
2019-10-07 11:33     ` Michal Hocko
2019-10-07 12:34     ` Qian Cai
2019-10-07 12:34       ` Qian Cai
2019-10-07 11:04   ` Qian Cai
2019-10-07 11:37     ` Michal Hocko
2019-10-07 12:11       ` Qian Cai
2019-10-07 12:11         ` Qian Cai
2019-10-07 12:43         ` Michal Hocko
2019-10-07 13:07           ` Qian Cai
2019-10-07 13:07             ` Qian Cai
2019-10-07 14:10             ` Petr Mladek
2019-10-07 14:30 ` Petr Mladek
2019-10-07 14:49   ` Michal Hocko
2019-10-08  7:43     ` Petr Mladek
2019-10-08  8:27       ` Michal Hocko
2019-10-08 12:56         ` Christian Borntraeger
2019-10-08 16:08           ` Qian Cai
2019-10-08 16:08             ` Qian Cai
2019-10-08 18:35             ` Michal Hocko
2019-10-08 19:06               ` Qian Cai [this message]
2019-10-08 19:06                 ` Qian Cai
2019-10-08 19:17                 ` Michal Hocko
2019-10-08 19:35                   ` Qian Cai
2019-10-08 19:35                     ` Qian Cai
2019-10-09 11:49                     ` Petr Mladek
2019-10-09 13:06                       ` Qian Cai
2019-10-09 13:06                         ` Qian Cai
2019-10-09 13:27                         ` Michal Hocko
2019-10-09 13:43                           ` Qian Cai
2019-10-09 13:43                             ` Qian Cai
2019-10-09 13:51                             ` Michal Hocko
2019-10-09 14:19                               ` Qian Cai
2019-10-09 14:19                                 ` Qian Cai
2019-10-09 14:34                                 ` Michal Hocko
2019-10-09 15:08                                   ` Qian Cai
2019-10-09 15:08                                     ` Qian Cai
2019-10-09 16:23                                     ` Michal Hocko
2019-10-09 16:23                                       ` Michal Hocko
2019-10-10  9:01                                       ` Qian Cai
2019-10-10 10:59                                         ` Michal Hocko
2019-10-10 13:11                                           ` Qian Cai
2019-10-10 13:11                                             ` Qian Cai
2019-10-10 14:18                                             ` Michal Hocko
2019-10-10 14:47                                               ` Qian Cai
2019-10-10 14:47                                                 ` Qian Cai
2019-10-10 17:30                                                 ` Michal Hocko
2019-10-10 17:48                                                   ` Qian Cai
2019-10-10 17:48                                                     ` Qian Cai
2019-10-10 18:06                                                     ` Michal Hocko
2019-10-10 18:59                                                       ` David Hildenbrand
2019-10-09 14:24                             ` Petr Mladek
2019-10-09 14:46                               ` Qian Cai
2019-10-09 14:46                                 ` Qian Cai
2019-10-10  7:57                                 ` Petr Mladek
2019-10-09 11:39                 ` Petr Mladek
2019-10-09 13:56             ` Peter Oberparleiter
2019-10-09 14:26               ` Michal Hocko
2019-10-10  5:12                 ` Sergey Senozhatsky
2019-10-10  7:40                   ` Michal Hocko
2019-10-10  8:16                     ` Sergey Senozhatsky
2019-10-10  8:37                       ` Michal Hocko
2019-10-10  8:21                   ` Petr Mladek
2019-10-10  8:39                     ` Sergey Senozhatsky
2019-10-10 11:11                       ` Petr Mladek
2019-10-09 15:25               ` Qian Cai
2019-10-09 15:25                 ` Qian Cai
2019-10-09 15:25                 ` Qian Cai
2019-10-07 14:59   ` Qian Cai
2019-10-07 14:59     ` Qian Cai
2019-10-07 15:12     ` Michal Hocko
2019-10-07 15:33       ` Qian Cai
2019-10-07 15:33         ` Qian Cai
2019-10-08  8:15         ` Petr Mladek
2019-10-08  9:32           ` Qian Cai
2019-10-08 13:13           ` Steven Rostedt
2019-10-08 13:23             ` Qian Cai
2019-10-08 13:23               ` Qian Cai
2019-10-08 13:33               ` Steven Rostedt
2019-10-08 13:42               ` Petr Mladek
2019-10-08 13:48                 ` Michal Hocko
2019-10-08 14:03                 ` Qian Cai
2019-10-08 14:03                   ` Qian Cai
2019-10-08 14:08                   ` Michal Hocko
2019-10-08  8:40         ` Michal Hocko
2019-10-08 10:04           ` Qian Cai
2019-10-08 10:39             ` Michal Hocko
2019-10-08 12:00               ` Qian Cai
2019-10-08 12:39                 ` Michal Hocko
2019-10-08 13:06                   ` Qian Cai
2019-10-08 13:06                     ` Qian Cai
2019-10-08 13:37                     ` Michal Hocko
2019-10-08 13:08     ` Petr Mladek
2019-10-08 13:33       ` Qian Cai
2019-10-08 13:33         ` Qian Cai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1570561573.5576.307.camel@lca.pw \
    --to=cai@lca.pw \
    --cc=akpm@linux-foundation.org \
    --cc=borntraeger@de.ibm.com \
    --cc=david@redhat.com \
    --cc=gor@linux.ibm.com \
    --cc=heiko.carstens@de.ibm.com \
    --cc=john.ogness@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=oberpar@linux.ibm.com \
    --cc=peterz@infradead.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    --cc=sergey.senozhatsky.work@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.