Re: [PATCH v2 14/20] mm: Make hibernate handle unmapped pages

From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "bp@alien8.de" <bp@alien8.de>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"linux-integrity@vger.kernel.org"
	<linux-integrity@vger.kernel.org>,
	"ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"nadav.amit@gmail.com" <nadav.amit@gmail.com>,
	"Dock, Deneen T" <deneen.t.dock@intel.com>,
	"pavel@ucw.cz" <pavel@ucw.cz>,
	"linux-security-module@vger.kernel.org" 
	<linux-security-module@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"kristen@linux.intel.com" <kristen@linux.intel.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"linux_dti@icloud.com" <linux_dti@icloud.com>,
	"luto@kernel.org" <luto@kernel.org>,
	"will.deacon@arm.com" <will.deacon@arm.com>,
	"kernel-hardening@lists.openwall.com" 
	<kernel-hardening@lists.openwall.com>,
	"rjw@rjwysocki.net" <rjw@rjwysocki.net>
Subject: Re: [PATCH v2 14/20] mm: Make hibernate handle unmapped pages
Date: Tue, 19 Feb 2019 21:28:55 +0000	[thread overview]
Message-ID: <07ea2a4a9f1771f7bad82ad8fe5ee9483b79d115.camel@intel.com> (raw)
In-Reply-To: <20190219110400.GA19514@zn.tnic>

On Tue, 2019-02-19 at 12:04 +0100, Borislav Petkov wrote:
> On Mon, Jan 28, 2019 at 04:34:16PM -0800, Rick Edgecombe wrote:
> > For architectures with CONFIG_ARCH_HAS_SET_ALIAS, pages can be unmapped
> > briefly on the directmap, even when CONFIG_DEBUG_PAGEALLOC is not
> > configured. So this changes kernel_map_pages and kernel_page_present to be
> 
> s/this changes/change/
> 
> From Documentation/process/submitting-patches.rst:
> 
>  "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
>   instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
>   to do frotz", as if you are giving orders to the codebase to change
>   its behaviour."
> 
> Also, please end function names with parentheses.
Yes, gotcha.

> > defined when CONFIG_ARCH_HAS_SET_ALIAS is defined as well. It also changes
> > places (page_alloc.c) where those functions are assumed to only be
> > implemented when CONFIG_DEBUG_PAGEALLOC is defined.
> 
> The commit message doesn't need to say "what" you're doing - that should
> be obvious from the diff below. It should rather say "why" you're doing
> it.
Ok, sorry. I'll change this to be more concise.

> > So now when CONFIG_ARCH_HAS_SET_ALIAS=y, hibernate will handle not present
> > page when saving. Previously this was already done when
> 
> pages
> 
> > CONFIG_DEBUG_PAGEALLOC was configured. It does not appear to have a big
> > hibernating performance impact.
> 
> Comment over safe_copy_page
Oh, yes you are right.

> > Before:
> > [    4.670938] PM: Wrote 171996 kbytes in 0.21 seconds (819.02 MB/s)
> > 
> > After:
> > [    4.504714] PM: Wrote 178932 kbytes in 0.22 seconds (813.32 MB/s)
> 
> IINM, that's like 1734 pages more. How am I to understand this number?
> 
> Code has called set_alias_nv_noflush() on them and safe_copy_page() now
> maps them one by one to copy them to the hibernation image?
> 
> Thx.
> 
These are from logs hibernate generates. The concern was that hibernate could be
slightly slower because of the checking of whether the pages are mapped. The
bandwidth number can be used to compare, 819.02->813.32 MB/s. Some randomness
must have resulted in different amounts of memory used between tests. I can just
remove the log lines and include the bandwidth numbers.

Thanks,

Rick