Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
From: Qian Cai <cai@lca.pw>
To: Michal Hocko <mhocko@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	pmladek@suse.com, rostedt@goodmis.org, peterz@infradead.org,
	david@redhat.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next v3] mm/hotplug: silence a lockdep splat with printk()
Date: Thu, 16 Jan 2020 09:53:13 -0500
Message-ID: <162DFB9F-247F-4DCA-9B69-535B9D714FBB@lca.pw> (raw)
In-Reply-To: <20200116142827.GU19428@dhcp22.suse.cz>



> On Jan 16, 2020, at 9:28 AM, Michal Hocko <mhocko@kernel.org> wrote:
> 
> On Wed 15-01-20 12:29:16, Qian Cai wrote:
>> It is guaranteed to trigger a lockdep splat if calling printk() with
>> zone->lock held because there are many places (tty, console drivers,
>> debugobjects etc) would allocate some memory with another lock
>> held which is proved to be difficult to fix them all.
> 
> I am still not happy with the above much. What would say about something
> like below instead?
> "
> It is not that hard to trigger lockdep splats by calling printk from
> under zone->lock. Most of them are false positives caused by lock chains
> introduced early in the boot process and they do not cause any real
> problems. There are some console drivers which do allocate from the
> printk context as well and those should be fixed. In any case false
> positives are not that trivial to workaround and it is far from optimal
> to lose lockdep functionality for something that is a non-issue.
> <An example of such a false positive goes here>
> "

I feel like I repeated myself too many times. A call trace for one lock dependency
is sometimes from early boot process because lockdep will save the first one it
encountered, but it does not mean the lock dependency will only not happen in
early boot. I spent some time to study those early boot call traces in the given
lockdep splats, and it looks to me the lock dependency is also possible after
the boot.

> 
>> A common workaround until the onging effort to make all printk() as
>> deferred happens is to use printk_deferred() in those places similar to
>> the recent commit [1] merged into the random and -next trees, but memory
>> offline will call dump_page() which needs to be deferred after the lock.
> 
> I would remove this paragraph altogether. The real problem is that we do
> not really want to make dump_page deferred.
> 
>> So change has_unmovable_pages() so that it no longer calls dump_page()
>> itself - instead it returns a "struct page *" of the unmovable page back
>> to the caller so that in the case of a has_unmovable_pages() failure,
>> the caller can call dump_page() after releasing zone->lock.
> 
> Again this begs for some more explanation why this is ok. Something like
> the following:
> "
> Even though has_unmovable_pages doesn't hold any reference to the
> returned page this should be reasonably safe for the purpose of
> reporting the page (dump_page) because it cannot be hotremoved. The
> state of the page might change but that is the case even with the
> existing code as zone->lock only plays role for free pages.
> "

Looks like a better version. Maybe Andrew could squash that in?

> 
>> Also, make
>> dump_page() is able to report a CMA page as well, so the reason string
>> from has_unmovable_pages() can be removed.
>> 
>> While at it, remove a similar but unnecessary debug-only printk() as
>> well. A few sample lockdep splats can be founnd here [2].
>> 
>> [1] https://lore.kernel.org/lkml/1573679785-21068-1-git-send-email-cai@lca.pw/
>> [2] https://lore.kernel.org/lkml/7CD27FC6-CFFF-4519-A57D-85179E9815FE@lca.pw/
>> 
>> Signed-off-by: Qian Cai <cai@lca.pw>
> 
> With improved changelog and after addressing one more thing below, feel
> free to add
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks for working on this. I have to say I have disliked the initial
> version because they were really hacky. This one can be reasoned about
> at least. has_unmovable_pages returning an unmovable page actually makes
> some conceptual sense to me.
> 
> [...]
>> @@ -8302,12 +8304,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int migratetype,
>> 		 */
>> 		goto unmovable;
>> 	}
>> -	return false;
>> +	return NULL;
>> unmovable:
>> 	WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);
> 
> You want to move this WARN_ON as well. Not only because it is using
> printk as well but also because we do not really want to trigger the
> warning from userspace via is_mem_section_removable. Maybe worth a patch
> on its own?

Yes, I have never triggered the warning there before, so I don’t think that is a problem
belong to here.



  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-15 17:29 Qian Cai
2020-01-16 14:28 ` Michal Hocko
2020-01-16 14:53   ` Qian Cai [this message]
2020-01-16 15:54     ` Michal Hocko
2020-01-16 16:04       ` David Hildenbrand
2020-01-16 16:27         ` Qian Cai
2020-01-16 16:05       ` Qian Cai
2020-01-16 17:43         ` Michal Hocko

Reply instructions:

You may reply publically 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=162DFB9F-247F-4DCA-9B69-535B9D714FBB@lca.pw \
    --to=cai@lca.pw \
    --cc=akpm@linux-foundation.org \
    --cc=david@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --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

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git