* Re: [patch 2/2] MM: allow per-cpu vmstat_threshold and vmstat_worker configuration
From: Marcelo Tosatti @ 2017-05-19 14:34 UTC (permalink / raw)
To: Christoph Lameter
Cc: Luiz Capitulino, linux-kernel, linux-mm, Rik van Riel,
Linux RT Users, cmetcalf
In-Reply-To: <alpine.DEB.2.20.1705160825480.32761@east.gentwo.org>
Hi Christoph,
On Tue, May 16, 2017 at 08:37:11AM -0500, Christoph Lameter wrote:
> On Mon, 15 May 2017, Marcelo Tosatti wrote:
>
> > > NOHZ already does that. I wanted to know what your problem is that you
> > > see. The latency issue has already been solved as far as I can tell .
> > > Please tell me why the existing solutions are not sufficient for you.
> >
> > We don't want vmstat_worker to execute on a given CPU, even if the local
> > CPU updates vm-statistics.
>
> Instead of responding you repeat describing what you want.
>
> > Because:
> >
> > vmstat_worker increases latency of the application
> > (i can measure it if you want on a given CPU,
> > how many ns's the following takes:
>
> That still is no use case.
Use-case: realtime application on an isolated core which for some reason
updates vmstatistics.
> Just a measurement of vmstat_worker. Pointless.
Shouldnt the focus be on general scenarios rather than particular
usecases, so that the solution covers a wider range of usecases?
The situation as i see is as follows:
Your point of view is: an "isolated CPU" with a set of applications
cannot update vm statistics, otherwise they pay the vmstat_update cost:
kworker/5:1-245 [005] ....1.. 673.454295: workqueue_execute_start: work struct ffffa0cf6e493e20: function vmstat_update
kworker/5:1-245 [005] ....1.. 673.454305: workqueue_execute_end: work struct ffffa0cf6e493e20
Thats 10us for example.
So if want to customize a realtime setup whose code updates vmstatistic,
you are dead. You have to avoid any systemcall which possibly updates
vmstatistics (now and in the future kernel versions).
> If you move the latency from the vmstat worker into the code thats
> updating the counters then you will require increased use of atomics
> which will increase contention which in turn will significantly
> increase the overall latency.
The point is that these vmstat updates are rare. From
http://www.7-cpu.com/cpu/Haswell.html:
RAM Latency = 36 cycles + 57 ns (3.4 GHz i7-4770)
RAM Latency = 62 cycles + 100 ns (3.6 GHz E5-2699 dual)
Lets round to 100ns = 0.1us.
You need 100 vmstat updates (all misses to RAM, the worst possible case)
to have equivalent amount of time of the batching version.
With more than 100 vmstat updates, then the batching is more efficient
(as in total amount of time to transfer to global counters).
But thats not the point. The point is the 10us interruption
to execution of the realtime app (which can either mean
your current deadline requirements are not met, or that
another application with lowest latency requirement can't
be used).
So i'd rather spend more time updating the aggregate of vmstatistics
(with the local->global transfer taking a small amount of time,
therefore not interrupting the realtime application for a long period),
than to batch the updates (which increases overall performance beyond
a certain number of updates, but which is _ONE_ large interruption).
So lets assume i go and count the vmstat updates on the DPDK case
(or any other realtime app), batching is more efficient
for that case.
Still, the one-time interruption of batching is worse than less
efficient one bean at a time vmstatistics accounting.
No?
Also, you could reply that: "oh, there are no vmstat updates
in fact in this setup, but the logic of disabling vmstat_update
is broken". Lets assume thats the case.
Even if its fixed (vmstat_update properly shut down) the proposed patch
deals with both cases: no vmstat updates on isolated cpus, and vmstat
updates on isolated cpus.
So why are you against integrating this simple, isolated patch which
does not affect how current logic works?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH] mm, vmstat: Fix NULL pointer dereference during pagetypeinfo print
From: Firo Yang @ 2017-05-19 14:39 UTC (permalink / raw)
To: akpm
Cc: vbabka, mgorman, hannes, mhocko, bigeasy, iamjoonsoo.kim,
rientjes, hughd, cl, linux-mm, linux-kernel, Firo Yang
During showing the pagetypeinfo, we forgot to save the found page
and dereference a invalid page address from the stack.
To fix it, save and reference the page address returned by
pfn_to_online_page().
Signed-off-by: Firo Yang <firogm@gmail.com>
---
mm/vmstat.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/vmstat.c b/mm/vmstat.c
index c432e58..6dae6b2 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1223,7 +1223,8 @@ static void pagetypeinfo_showblockcount_print(struct seq_file *m,
for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
struct page *page;
- if (!pfn_to_online_page(pfn))
+ page = pfn_to_online_page(pfn);
+ if (!page)
continue;
/* Watch for unexpected holes punched in the memmap */
--
2.9.4
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [PATCH] mm, vmstat: Fix NULL pointer dereference during pagetypeinfo print
From: Michal Hocko @ 2017-05-19 14:48 UTC (permalink / raw)
To: Firo Yang
Cc: akpm, vbabka, mgorman, hannes, bigeasy, iamjoonsoo.kim, rientjes,
hughd, cl, linux-mm, linux-kernel
In-Reply-To: <20170519143936.21209-1-firogm@gmail.com>
On Fri 19-05-17 22:39:36, Firo Yang wrote:
> During showing the pagetypeinfo, we forgot to save the found page
> and dereference a invalid page address from the stack.
>
> To fix it, save and reference the page address returned by
> pfn_to_online_page().
Thanks for taking catching that and your fix. I have already posted a fix
http://lkml.kernel.org/r/20170519072225.GA13041@dhcp22.suse.cz earlier
today. Sorry about troubles
> Signed-off-by: Firo Yang <firogm@gmail.com>
> ---
> mm/vmstat.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index c432e58..6dae6b2 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1223,7 +1223,8 @@ static void pagetypeinfo_showblockcount_print(struct seq_file *m,
> for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
> struct page *page;
>
> - if (!pfn_to_online_page(pfn))
> + page = pfn_to_online_page(pfn);
> + if (!page)
> continue;
>
> /* Watch for unexpected holes punched in the memmap */
> --
> 2.9.4
>
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH 2/2] mm, oom: do not trigger out_of_memory from the #PF
From: Tetsuo Handa @ 2017-05-19 15:22 UTC (permalink / raw)
To: mhocko; +Cc: akpm, hannes, guro, vdavydov.dev, linux-mm, linux-kernel
In-Reply-To: <20170519132209.GG29839@dhcp22.suse.cz>
Michal Hocko wrote:
> On Fri 19-05-17 22:02:44, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > Any allocation failure during the #PF path will return with VM_FAULT_OOM
> > > which in turn results in pagefault_out_of_memory. This can happen for
> > > 2 different reasons. a) Memcg is out of memory and we rely on
> > > mem_cgroup_oom_synchronize to perform the memcg OOM handling or b)
> > > normal allocation fails.
> > >
> > > The later is quite problematic because allocation paths already trigger
> > > out_of_memory and the page allocator tries really hard to not fail
> >
> > We made many memory allocation requests from page fault path (e.g. XFS)
> > __GFP_FS some time ago, didn't we? But if I recall correctly (I couldn't
> > find the message), there are some allocation requests from page fault path
> > which cannot use __GFP_FS. Then, not all allocation requests can call
> > oom_kill_process() and reaching pagefault_out_of_memory() will be
> > inevitable.
>
> Even if such an allocation fail without the OOM killer then we simply
> retry the PF and will do that the same way how we keep retrying the
> allocation inside the page allocator. So how is this any different?
You are trying to remove out_of_memory() from pagefault_out_of_memory()
by this patch. But you also want to make !__GFP_FS allocations not to
keep retrying inside the page allocator in future kernels, don't you?
Then, a thread which need to allocate memory from page fault path but
cannot call oom_kill_process() will spin forever (unless somebody else
calls oom_kill_process() via a __GFP_FS allocation request). I consider
that introducing such possibility is a problem.
>
> > > allocations. Anyway, if the OOM killer has been already invoked there
> > > is no reason to invoke it again from the #PF path. Especially when the
> > > OOM condition might be gone by that time and we have no way to find out
> > > other than allocate.
> > >
> > > Moreover if the allocation failed and the OOM killer hasn't been
> > > invoked then we are unlikely to do the right thing from the #PF context
> > > because we have already lost the allocation context and restictions and
> > > therefore might oom kill a task from a different NUMA domain.
> >
> > If we carry a flag via task_struct that indicates whether it is an memory
> > allocation request from page fault and allocation failure is not acceptable,
> > we can call out_of_memory() from page allocator path.
>
> I do not understand
We need to allocate memory from page fault path in order to avoid spinning forever
(unless somebody else calls oom_kill_process() via a __GFP_FS allocation request),
doesn't it? Then, memory allocation requests from page fault path can pass flags
like __GFP_NOFAIL | __GFP_KILLABLE because retrying the page fault without
allocating memory is pointless. I called such flags as carry a flag via task_struct.
> > By the way, can page fault occur after reaching do_exit()? When a thread
> > reached do_exit(), fatal_signal_pending(current) becomes false, doesn't it?
>
> yes fatal_signal_pending will be false at the time and I believe we can
> perform a page fault past that moment and go via allocation path which would
> trigger the OOM or give this task access to reserves but it is more
> likely that the oom reaper will push to kill another task by that time
> if the situation didn't get resolved. Or did I miss your concern?
How checking fatal_signal_pending() here helps? It only suppresses printk().
If current thread needs to allocate memory because not all allocation requests
can call oom_kill_process(), doing printk() is not the right thing to do.
Allocate memory by some means (e.g. __GFP_NOFAIL | __GFP_KILLABLE) will be
the right thing to do.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH 2/2] mm, oom: do not trigger out_of_memory from the #PF
From: Michal Hocko @ 2017-05-19 15:50 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: akpm, hannes, guro, vdavydov.dev, linux-mm, linux-kernel
In-Reply-To: <201705200022.BFJ12428.JFOSMLFOtFHOVQ@I-love.SAKURA.ne.jp>
On Sat 20-05-17 00:22:30, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Fri 19-05-17 22:02:44, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > Any allocation failure during the #PF path will return with VM_FAULT_OOM
> > > > which in turn results in pagefault_out_of_memory. This can happen for
> > > > 2 different reasons. a) Memcg is out of memory and we rely on
> > > > mem_cgroup_oom_synchronize to perform the memcg OOM handling or b)
> > > > normal allocation fails.
> > > >
> > > > The later is quite problematic because allocation paths already trigger
> > > > out_of_memory and the page allocator tries really hard to not fail
> > >
> > > We made many memory allocation requests from page fault path (e.g. XFS)
> > > __GFP_FS some time ago, didn't we? But if I recall correctly (I couldn't
> > > find the message), there are some allocation requests from page fault path
> > > which cannot use __GFP_FS. Then, not all allocation requests can call
> > > oom_kill_process() and reaching pagefault_out_of_memory() will be
> > > inevitable.
> >
> > Even if such an allocation fail without the OOM killer then we simply
> > retry the PF and will do that the same way how we keep retrying the
> > allocation inside the page allocator. So how is this any different?
>
> You are trying to remove out_of_memory() from pagefault_out_of_memory()
> by this patch. But you also want to make !__GFP_FS allocations not to
> keep retrying inside the page allocator in future kernels, don't you?
I would _love_ to but I am much less optimistic this is achiveable
> Then, a thread which need to allocate memory from page fault path but
> cannot call oom_kill_process() will spin forever (unless somebody else
> calls oom_kill_process() via a __GFP_FS allocation request). I consider
> that introducing such possibility is a problem.
What I am trying to say is that this is already happening. The
difference with the VM_FAULT_OOM would only be that the whole PF path
would be unwinded back to the PF, all locks dropped and then the PF
retries so in principle this would be safer.
> > > > allocations. Anyway, if the OOM killer has been already invoked there
> > > > is no reason to invoke it again from the #PF path. Especially when the
> > > > OOM condition might be gone by that time and we have no way to find out
> > > > other than allocate.
> > > >
> > > > Moreover if the allocation failed and the OOM killer hasn't been
> > > > invoked then we are unlikely to do the right thing from the #PF context
> > > > because we have already lost the allocation context and restictions and
> > > > therefore might oom kill a task from a different NUMA domain.
> > >
> > > If we carry a flag via task_struct that indicates whether it is an memory
> > > allocation request from page fault and allocation failure is not acceptable,
> > > we can call out_of_memory() from page allocator path.
> >
> > I do not understand
>
> We need to allocate memory from page fault path in order to avoid spinning forever
> (unless somebody else calls oom_kill_process() via a __GFP_FS allocation request),
> doesn't it? Then, memory allocation requests from page fault path can pass flags
> like __GFP_NOFAIL | __GFP_KILLABLE because retrying the page fault without
> allocating memory is pointless. I called such flags as carry a flag via task_struct.
>
> > > By the way, can page fault occur after reaching do_exit()? When a thread
> > > reached do_exit(), fatal_signal_pending(current) becomes false, doesn't it?
> >
> > yes fatal_signal_pending will be false at the time and I believe we can
> > perform a page fault past that moment and go via allocation path which would
> > trigger the OOM or give this task access to reserves but it is more
> > likely that the oom reaper will push to kill another task by that time
> > if the situation didn't get resolved. Or did I miss your concern?
>
> How checking fatal_signal_pending() here helps?
It just skips the warning because we know that we would handle the
signal before retrying the page fault and go to exit path. Those that do
not have such a signal should warn just that we know that such a
situation happens. With the current allocator semantic it shouldn't
> It only suppresses printk().
> If current thread needs to allocate memory because not all allocation requests
> can call oom_kill_process(), doing printk() is not the right thing to do.
> Allocate memory by some means (e.g. __GFP_NOFAIL | __GFP_KILLABLE) will be
> the right thing to do.
Why would looping inside an allocator with a restricted context be any
better than retrying the whole thing?
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 01/11] mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7 to bit 1
From: Dave Hansen @ 2017-05-19 15:55 UTC (permalink / raw)
To: Zi Yan, n-horiguchi, kirill.shutemov, linux-kernel, linux-mm
Cc: akpm, minchan, vbabka, mgorman, mhocko, khandual, zi.yan,
dnellans
In-Reply-To: <20170420204752.79703-2-zi.yan@sent.com>
On 04/20/2017 01:47 PM, Zi Yan wrote:
> pmd_present() checks _PAGE_PSE along with _PAGE_PRESENT to avoid
> false negative return when it races with thp spilt
> (during which _PAGE_PRESENT is temporary cleared.) I don't think that
> dropping _PAGE_PSE check in pmd_present() works well because it can
> hurt optimization of tlb handling in thp split.
> In the current kernel, bits 1-4 are not used in non-present format
> since commit 00839ee3b299 ("x86/mm: Move swap offset/type up in PTE to
> work around erratum"). So let's move _PAGE_SWP_SOFT_DIRTY to bit 1.
> Bit 7 is used as reserved (always clear), so please don't use it for
> other purpose.
This description lacks a problem statement. What's the problem?
_PAGE_PSE is used to distinguish between a truly non-present
(_PAGE_PRESENT=0) PMD, and a PMD which is undergoing a THP
split and should be treated as present.
But _PAGE_SWP_SOFT_DIRTY currently uses the _PAGE_PSE bit,
which would cause confusion between one of those PMDs
undergoing a THP split, and a soft-dirty PMD.
Thus, we need to move the bit.
Does that capture it?
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
> ---
> arch/x86/include/asm/pgtable_64.h | 12 +++++++++---
> arch/x86/include/asm/pgtable_types.h | 10 +++++-----
> 2 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
> index 73c7ccc38912..770b5ae271ed 100644
> --- a/arch/x86/include/asm/pgtable_64.h
> +++ b/arch/x86/include/asm/pgtable_64.h
> @@ -157,15 +157,21 @@ static inline int pgd_large(pgd_t pgd) { return 0; }
> /*
> * Encode and de-code a swap entry
> *
> - * | ... | 11| 10| 9|8|7|6|5| 4| 3|2|1|0| <- bit number
> - * | ... |SW3|SW2|SW1|G|L|D|A|CD|WT|U|W|P| <- bit names
> - * | OFFSET (14->63) | TYPE (9-13) |0|X|X|X| X| X|X|X|0| <- swp entry
> + * | ... | 11| 10| 9|8|7|6|5| 4| 3|2| 1|0| <- bit number
> + * | ... |SW3|SW2|SW1|G|L|D|A|CD|WT|U| W|P| <- bit names
> + * | OFFSET (14->63) | TYPE (9-13) |0|0|X|X| X| X|X|SD|0| <- swp entry
So, this diagram was incomplete before? It should have had "SD" under
bit 7 for swap entries?
> * G (8) is aliased and used as a PROT_NONE indicator for
> * !present ptes. We need to start storing swap entries above
> * there. We also need to avoid using A and D because of an
> * erratum where they can be incorrectly set by hardware on
> * non-present PTEs.
> + *
> + * SD (1) in swp entry is used to store soft dirty bit, which helps us
> + * remember soft dirty over page migration
> + *
> + * Bit 7 in swp entry should be 0 because pmd_present checks not only P,
> + * but also L and G.
> */
> #define SWP_TYPE_FIRST_BIT (_PAGE_BIT_PROTNONE + 1)
> #define SWP_TYPE_BITS 5
> diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
> index df08535f774a..9a4ac934659e 100644
> --- a/arch/x86/include/asm/pgtable_types.h
> +++ b/arch/x86/include/asm/pgtable_types.h
> @@ -97,15 +97,15 @@
> /*
> * Tracking soft dirty bit when a page goes to a swap is tricky.
> * We need a bit which can be stored in pte _and_ not conflict
> - * with swap entry format. On x86 bits 6 and 7 are *not* involved
> - * into swap entry computation, but bit 6 is used for nonlinear
> - * file mapping, so we borrow bit 7 for soft dirty tracking.
> + * with swap entry format. On x86 bits 1-4 are *not* involved
> + * into swap entry computation, but bit 7 is used for thp migration,
> + * so we borrow bit 1 for soft dirty tracking.
> *
> * Please note that this bit must be treated as swap dirty page
> - * mark if and only if the PTE has present bit clear!
> + * mark if and only if the PTE/PMD has present bit clear!
> */
> #ifdef CONFIG_MEM_SOFT_DIRTY
> -#define _PAGE_SWP_SOFT_DIRTY _PAGE_PSE
> +#define _PAGE_SWP_SOFT_DIRTY _PAGE_RW
> #else
> #define _PAGE_SWP_SOFT_DIRTY (_AT(pteval_t, 0))
> #endif
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 02/11] mm: mempolicy: add queue_pages_node_check()
From: Mel Gorman @ 2017-05-19 16:02 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Zi Yan, n-horiguchi, kirill.shutemov, linux-kernel, linux-mm,
akpm, minchan, vbabka, mhocko, zi.yan, dnellans
In-Reply-To: <16799a52-8a03-7099-5f95-3016808ae65f@linux.vnet.ibm.com>
On Fri, May 19, 2017 at 06:43:37PM +0530, Anshuman Khandual wrote:
> On 04/21/2017 09:34 AM, Anshuman Khandual wrote:
> > On 04/21/2017 02:17 AM, Zi Yan wrote:
> >> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> >>
> >> Introduce a separate check routine related to MPOL_MF_INVERT flag.
> >> This patch just does cleanup, no behavioral change.
> >
> > Can you please send it separately first, this should be debated
> > and merged quickly and not hang on to the series if we have to
> > respin again.
> >
> > Reviewed-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>
> Mel/Andrew,
>
> This does not have any functional changes and very much independent.
> Can this clean up be accepted as is ? In that case we will have to
> carry one less patch in the series which can make the review process
> simpler.
>
As you say, there is no functional change but the helper name is vague
and gives no hint to what's it's checking for. It's somewhat tolerable as
it is as it's obvious what is being checked but the same is not true with
the helper name.
--
Mel Gorman
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 01/11] mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7 to bit 1
From: Zi Yan @ 2017-05-19 16:31 UTC (permalink / raw)
To: Dave Hansen
Cc: n-horiguchi, kirill.shutemov, linux-kernel, linux-mm, akpm,
minchan, vbabka, mgorman, mhocko, khandual, dnellans
In-Reply-To: <76a36bee-0f1c-a2f4-6f5c-78394ac46ee4@intel.com>
[-- Attachment #1: Type: text/plain, Size: 3181 bytes --]
On 19 May 2017, at 11:55, Dave Hansen wrote:
> On 04/20/2017 01:47 PM, Zi Yan wrote:
>> pmd_present() checks _PAGE_PSE along with _PAGE_PRESENT to avoid
>> false negative return when it races with thp spilt
>> (during which _PAGE_PRESENT is temporary cleared.) I don't think that
>> dropping _PAGE_PSE check in pmd_present() works well because it can
>> hurt optimization of tlb handling in thp split.
>> In the current kernel, bits 1-4 are not used in non-present format
>> since commit 00839ee3b299 ("x86/mm: Move swap offset/type up in PTE to
>> work around erratum"). So let's move _PAGE_SWP_SOFT_DIRTY to bit 1.
>> Bit 7 is used as reserved (always clear), so please don't use it for
>> other purpose.
>
> This description lacks a problem statement. What's the problem?
>
> _PAGE_PSE is used to distinguish between a truly non-present
> (_PAGE_PRESENT=0) PMD, and a PMD which is undergoing a THP
> split and should be treated as present.
>
> But _PAGE_SWP_SOFT_DIRTY currently uses the _PAGE_PSE bit,
> which would cause confusion between one of those PMDs
> undergoing a THP split, and a soft-dirty PMD.
>
> Thus, we need to move the bit.
>
> Does that capture it?
Yes. I will add this in the next version.
>
>> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
>> ---
>> arch/x86/include/asm/pgtable_64.h | 12 +++++++++---
>> arch/x86/include/asm/pgtable_types.h | 10 +++++-----
>> 2 files changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
>> index 73c7ccc38912..770b5ae271ed 100644
>> --- a/arch/x86/include/asm/pgtable_64.h
>> +++ b/arch/x86/include/asm/pgtable_64.h
>> @@ -157,15 +157,21 @@ static inline int pgd_large(pgd_t pgd) { return 0; }
>> /*
>> * Encode and de-code a swap entry
>> *
>> - * | ... | 11| 10| 9|8|7|6|5| 4| 3|2|1|0| <- bit number
>> - * | ... |SW3|SW2|SW1|G|L|D|A|CD|WT|U|W|P| <- bit names
>> - * | OFFSET (14->63) | TYPE (9-13) |0|X|X|X| X| X|X|X|0| <- swp entry
>> + * | ... | 11| 10| 9|8|7|6|5| 4| 3|2| 1|0| <- bit number
>> + * | ... |SW3|SW2|SW1|G|L|D|A|CD|WT|U| W|P| <- bit names
>> + * | OFFSET (14->63) | TYPE (9-13) |0|0|X|X| X| X|X|SD|0| <- swp entry
>
> So, this diagram was incomplete before? It should have had "SD" under
> bit 7 for swap entries?
Right. SD bit is used only when CONFIG_MEM_SOFT_DIRTY is enabled, but it is good
to mark it in the diagram to avoid conflicts.
>
>> * G (8) is aliased and used as a PROT_NONE indicator for
>> * !present ptes. We need to start storing swap entries above
>> * there. We also need to avoid using A and D because of an
>> * erratum where they can be incorrectly set by hardware on
>> * non-present PTEs.
>> + *
>> + * SD (1) in swp entry is used to store soft dirty bit, which helps us
>> + * remember soft dirty over page migration
>> + *
>> + * Bit 7 in swp entry should be 0 because pmd_present checks not only P,
>> + * but also L and G.
>> */
--
Best Regards
Yan Zi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 496 bytes --]
^ permalink raw reply
* Re: [PATCH] mm/oom_kill: count global and memory cgroup oom kills
From: Roman Guschin @ 2017-05-19 16:34 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: linux-mm, Andrew Morton, Tejun Heo, cgroups, linux-kernel,
Vlastimil Babka, Michal Hocko, hannes
In-Reply-To: <149520375057.74196.2843113275800730971.stgit@buzz>
2017-05-19 15:22 GMT+01:00 Konstantin Khlebnikov <khlebnikov@yandex-team.ru>:
> Show count of global oom killer invocations in /proc/vmstat and
> count of oom kills inside memory cgroup in knob "memory.events"
> (in memory.oom_control for v1 cgroup).
>
> Also describe difference between "oom" and "oom_kill" in memory
> cgroup documentation. Currently oom in memory cgroup kills tasks
> iff shortage has happened inside page fault.
>
> These counters helps in monitoring oom kills - for now
> the only way is grepping for magic words in kernel log.
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> ---
> Documentation/cgroup-v2.txt | 12 +++++++++++-
> include/linux/memcontrol.h | 1 +
> include/linux/vm_event_item.h | 1 +
> mm/memcontrol.c | 2 ++
> mm/oom_kill.c | 6 ++++++
> mm/vmstat.c | 1 +
> 6 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/cgroup-v2.txt b/Documentation/cgroup-v2.txt
> index dc5e2dcdbef4..a742008d76aa 100644
> --- a/Documentation/cgroup-v2.txt
> +++ b/Documentation/cgroup-v2.txt
> @@ -830,9 +830,19 @@ PAGE_SIZE multiple when read back.
>
> oom
>
> + The number of time the cgroup's memory usage was
> + reached the limit and allocation was about to fail.
> + Result could be oom kill, -ENOMEM from any syscall or
> + completely ignored in cases like disk readahead.
> + For now oom in memory cgroup kills tasks iff shortage
> + has happened inside page fault.
>From a user's point of view the difference between "oom" and "max"
becomes really vague here,
assuming that "max" is described almost in the same words:
"The number of times the cgroup's memory usage was
about to go over the max boundary. If direct reclaim
fails to bring it down, the OOM killer is invoked."
I wonder, if it's better to fix the existing "oom" value to show what
it has to show, according to docs,
rather than to introduce a new one?
> +
> + oom_kill
> +
> The number of times the OOM killer has been invoked in
> the cgroup. This may not exactly match the number of
> - processes killed but should generally be close.
> + processes killed but should generally be close: each
> + invocation could kill several processes at once.
>
> memory.stat
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 01/11] mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7 to bit 1
From: Dave Hansen @ 2017-05-19 16:36 UTC (permalink / raw)
To: Zi Yan
Cc: n-horiguchi, kirill.shutemov, linux-kernel, linux-mm, akpm,
minchan, vbabka, mgorman, mhocko, khandual, dnellans
In-Reply-To: <07441274-3C64-4376-8225-39CD052399B4@cs.rutgers.edu>
On 05/19/2017 09:31 AM, Zi Yan wrote:
>> This description lacks a problem statement. What's the problem?
>>
>> _PAGE_PSE is used to distinguish between a truly non-present
>> (_PAGE_PRESENT=0) PMD, and a PMD which is undergoing a THP
>> split and should be treated as present.
>>
>> But _PAGE_SWP_SOFT_DIRTY currently uses the _PAGE_PSE bit,
>> which would cause confusion between one of those PMDs
>> undergoing a THP split, and a soft-dirty PMD.
>>
>> Thus, we need to move the bit.
>>
>> Does that capture it?
> Yes. I will add this in the next version.
OK, thanks for clarifying. You can add my acked-by on this.
But, generally, these bits really scare me. We don't have any nice
programmatic way to find conflicts. I really wish we had some
BUILD_BUG_ON()s or something to express these dependencies.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 02/11] mm: mempolicy: add queue_pages_node_check()
From: Zi Yan @ 2017-05-19 16:37 UTC (permalink / raw)
To: Mel Gorman
Cc: Anshuman Khandual, n-horiguchi, kirill.shutemov, linux-kernel,
linux-mm, akpm, minchan, vbabka, mhocko, dnellans
In-Reply-To: <20170519160205.hkte6tlw26lfn74h@techsingularity.net>
[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]
On 19 May 2017, at 12:02, Mel Gorman wrote:
> On Fri, May 19, 2017 at 06:43:37PM +0530, Anshuman Khandual wrote:
>> On 04/21/2017 09:34 AM, Anshuman Khandual wrote:
>>> On 04/21/2017 02:17 AM, Zi Yan wrote:
>>>> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>>>
>>>> Introduce a separate check routine related to MPOL_MF_INVERT flag.
>>>> This patch just does cleanup, no behavioral change.
>>>
>>> Can you please send it separately first, this should be debated
>>> and merged quickly and not hang on to the series if we have to
>>> respin again.
>>>
>>> Reviewed-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>>
>> Mel/Andrew,
>>
>> This does not have any functional changes and very much independent.
>> Can this clean up be accepted as is ? In that case we will have to
>> carry one less patch in the series which can make the review process
>> simpler.
>>
>
> As you say, there is no functional change but the helper name is vague
> and gives no hint to what's it's checking for. It's somewhat tolerable as
> it is as it's obvious what is being checked but the same is not true with
> the helper name.
>
> --
> Mel Gorman
> SUSE Labs
Does queue_pages_invert_nodemask_check() work? I can change the helper name
in the next version.
--
Best Regards
Yan Zi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 496 bytes --]
^ permalink raw reply
* [PATCH] x86/mm: pgds getting out of sync after memory hot remove
From: Jérôme Glisse @ 2017-05-19 18:01 UTC (permalink / raw)
To: linux-kernel, linux-mm
Cc: Jérôme Glisse, Kirill A. Shutemov, Andrew Morton,
Ingo Molnar, Michal Hocko, Mel Gorman
After memory hot remove it seems we do not synchronize pgds for kernel
virtual memory range (on vmemmap_free()). This seems bogus to me as it
means we are left with stall entry for process with mm != mm_init
Yet i am puzzle by the fact that i am only now hitting this issue. It
never was an issue with 4.12 or before ie HMM never triggered following
BUG_ON inside sync_global_pgds():
if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))
BUG_ON(p4d_page_vaddr(*p4d) != p4d_page_vaddr(*p4d_ref));
It seems that Kirill 5 level page table changes play a role in this
behavior change. I could not bisect because HMM is painfull to rebase
for each bisection step so that is just my best guess.
Am i missing something here ? Am i wrong in assuming that should sync
pgd on vmemmap_free() ? If so anyone have a good guess on why i am now
seeing the above BUG_ON ?
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mel Gorman <mgorman@suse.de>
JA(C)rA'me Glisse (1):
x86/mm: synchronize pgd in vmemmap_free()
arch/x86/mm/init_64.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
--
2.4.11
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH] x86/mm: synchronize pgd in vmemmap_free()
From: Jérôme Glisse @ 2017-05-19 18:01 UTC (permalink / raw)
To: linux-kernel, linux-mm
Cc: Jérôme Glisse, Kirill A. Shutemov, Andrew Morton,
Ingo Molnar, Michal Hocko, Mel Gorman
In-Reply-To: <1495216887-3175-1-git-send-email-jglisse@redhat.com>
When we free kernel virtual map we should synchronize p4d/pud for
all the pgds to avoid any stall entry in non canonical pgd.
Signed-off-by: JA(C)rA'me Glisse <jglisse@redhat.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mel Gorman <mgorman@suse.de>
---
arch/x86/mm/init_64.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index ff95fe8..df753f8 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -108,8 +108,6 @@ void sync_global_pgds(unsigned long start, unsigned long end)
BUILD_BUG_ON(pgd_none(*pgd_ref));
p4d_ref = p4d_offset(pgd_ref, address);
- if (p4d_none(*p4d_ref))
- continue;
spin_lock(&pgd_lock);
list_for_each_entry(page, &pgd_list, lru) {
@@ -123,12 +121,16 @@ void sync_global_pgds(unsigned long start, unsigned long end)
pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
spin_lock(pgt_lock);
- if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))
- BUG_ON(p4d_page_vaddr(*p4d)
- != p4d_page_vaddr(*p4d_ref));
-
- if (p4d_none(*p4d))
+ if (p4d_none(*p4d_ref)) {
set_p4d(p4d, *p4d_ref);
+ } else {
+ if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))
+ BUG_ON(p4d_page_vaddr(*p4d)
+ != p4d_page_vaddr(*p4d_ref));
+
+ if (p4d_none(*p4d))
+ set_p4d(p4d, *p4d_ref);
+ }
spin_unlock(pgt_lock);
}
@@ -1024,6 +1026,7 @@ remove_pagetable(unsigned long start, unsigned long end, bool direct)
void __ref vmemmap_free(unsigned long start, unsigned long end)
{
remove_pagetable(start, end, false);
+ sync_global_pgds(start, end - 1);
}
#ifdef CONFIG_MEMORY_HOTREMOVE
--
2.4.11
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [patch 2/2] MM: allow per-cpu vmstat_threshold and vmstat_worker configuration
From: Christoph Lameter @ 2017-05-19 17:13 UTC (permalink / raw)
To: Marcelo Tosatti
Cc: Luiz Capitulino, linux-kernel, linux-mm, Rik van Riel,
Linux RT Users, cmetcalf
In-Reply-To: <20170519143407.GA19282@amt.cnet>
On Fri, 19 May 2017, Marcelo Tosatti wrote:
> Use-case: realtime application on an isolated core which for some reason
> updates vmstatistics.
Ok that is already only happening every 2 seconds by default and that
interval is configurable via the vmstat_interval proc setting.
> > Just a measurement of vmstat_worker. Pointless.
>
> Shouldnt the focus be on general scenarios rather than particular
> usecases, so that the solution covers a wider range of usecases?
Yes indeed and as far as I can tell the wider usecases are covered. Not
sure that there is anything required here.
> The situation as i see is as follows:
>
> Your point of view is: an "isolated CPU" with a set of applications
> cannot update vm statistics, otherwise they pay the vmstat_update cost:
>
> kworker/5:1-245 [005] ....1.. 673.454295: workqueue_execute_start: work struct ffffa0cf6e493e20: function vmstat_update
> kworker/5:1-245 [005] ....1.. 673.454305: workqueue_execute_end: work struct ffffa0cf6e493e20
>
> Thats 10us for example.
Well with a decent cpu that is 3 usec and it occurs infrequently on the
order of once per multiple seconds.
> So if want to customize a realtime setup whose code updates vmstatistic,
> you are dead. You have to avoid any systemcall which possibly updates
> vmstatistics (now and in the future kernel versions).
You are already dead because you allow IPIs and other kernel processing
which creates far more overhead. Still fail to see the point.
> The point is that these vmstat updates are rare. From
> http://www.7-cpu.com/cpu/Haswell.html:
>
> RAM Latency = 36 cycles + 57 ns (3.4 GHz i7-4770)
> RAM Latency = 62 cycles + 100 ns (3.6 GHz E5-2699 dual)
>
> Lets round to 100ns = 0.1us.
That depends on the kernel functionality used.
> You need 100 vmstat updates (all misses to RAM, the worst possible case)
> to have equivalent amount of time of the batching version.
The batching version occurs every couple of seconds if at all.
> But thats not the point. The point is the 10us interruption
> to execution of the realtime app (which can either mean
> your current deadline requirements are not met, or that
> another application with lowest latency requirement can't
> be used).
Ok then you need to get rid of the IPIs and the other stuff that you have
going on with the OS first I think.
> So why are you against integrating this simple, isolated patch which
> does not affect how current logic works?
Frankly the argument does not make sense. Vmstat updates occur very
infrequently (probably even less than you IPIs and the other OS stuff that
also causes additional latencies that you seem to be willing to tolerate).
And you can configure the interval of vmstat updates freely.... Set
the vmstat_interval to 60 seconds instead of 2 for a try? Is that rare
enough?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [patch 2/2] MM: allow per-cpu vmstat_threshold and vmstat_worker configuration
From: Luiz Capitulino @ 2017-05-19 17:49 UTC (permalink / raw)
To: Christoph Lameter
Cc: Marcelo Tosatti, linux-kernel, linux-mm, Rik van Riel,
Linux RT Users, cmetcalf
In-Reply-To: <alpine.DEB.2.20.1705191205580.19631@east.gentwo.org>
On Fri, 19 May 2017 12:13:26 -0500 (CDT)
Christoph Lameter <cl@linux.com> wrote:
> > So why are you against integrating this simple, isolated patch which
> > does not affect how current logic works?
>
> Frankly the argument does not make sense. Vmstat updates occur very
> infrequently (probably even less than you IPIs and the other OS stuff that
> also causes additional latencies that you seem to be willing to tolerate).
Infrequently is not good enough. It only has to happen once to
cause a problem.
Also, IPIs take a few us, usually less. That's not a problem. In our
testing we see the preemption caused by the kworker take 10us or
even more. I've never seeing it take 3us. I'm not saying this is not
true, I'm saying if this is causing a problem to us it will cause
a problem to other people too.
> And you can configure the interval of vmstat updates freely.... Set
> the vmstat_interval to 60 seconds instead of 2 for a try? Is that rare
> enough?
No, we'd have to set it high enough to disable it and this will
affect all CPUs.
Something that crossed my mind was to add a new tunable to set
the vmstat_interval for each CPU, this way we could essentially
disable it to the CPUs where DPDK is running. What's the implications
of doing this besides not getting up to date stats in /proc/vmstat
(which I still have to confirm would be OK)? Can this break anything
in the kernel for example?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file
From: Tejun Heo @ 2017-05-19 19:21 UTC (permalink / raw)
To: Waiman Long
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, linux-mm, kernel-team, pjt, luto, efault
In-Reply-To: <ee36d4f8-9e9d-a5c7-2174-56c21aaf75af@redhat.com>
Hello, Waiman.
On Thu, May 18, 2017 at 11:52:18AM -0400, Waiman Long wrote:
> The controller name is "debug" and so it is obvious what this controller
> is for. However, the config prompt "Example controller" is indeed vague
Yeah but it also shows up as an integral part of stable interface
rather than e.g. /sys/kernel/debug. This isn't of any interest to
people who aren't developing cgroup core code. There is no reason to
risk growing dependencies on it.
> in meaning. So we can make the prompt more descriptive here. As for the
> boot param, are you saying something like "cgroup_debug" has to be
> specified in the command line even if CGROUP_DEBUG config is there for
> the debug controller to be enabled? I am fine with that if you think it
> is necessary.
Yeah, I think that'd be a good idea. cgroup_debug should do. While
at it, can you also please make CGROUP_DEBUG depend on DEBUG_KERNEL?
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file
From: Waiman Long @ 2017-05-19 19:33 UTC (permalink / raw)
To: Tejun Heo
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, linux-mm, kernel-team, pjt, luto, efault
In-Reply-To: <20170519192146.GA9741@wtj.duckdns.org>
On 05/19/2017 03:21 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Thu, May 18, 2017 at 11:52:18AM -0400, Waiman Long wrote:
>> The controller name is "debug" and so it is obvious what this controller
>> is for. However, the config prompt "Example controller" is indeed vague
> Yeah but it also shows up as an integral part of stable interface
> rather than e.g. /sys/kernel/debug. This isn't of any interest to
> people who aren't developing cgroup core code. There is no reason to
> risk growing dependencies on it.
The debug controller is used to show information relevant to the cgroup
its css'es are attached to. So it will be very hard to use if we
relocate to /sys/kernel/debug, for example. Currently, nothing in the
debug controller other than debug_cgrp_subsys are exported. I don't see
any risk of having dependency on that controller from other parts of the
kernel.
>> in meaning. So we can make the prompt more descriptive here. As for the
>> boot param, are you saying something like "cgroup_debug" has to be
>> specified in the command line even if CGROUP_DEBUG config is there for
>> the debug controller to be enabled? I am fine with that if you think it
>> is necessary.
> Yeah, I think that'd be a good idea. cgroup_debug should do. While
> at it, can you also please make CGROUP_DEBUG depend on DEBUG_KERNEL?
>
> Thanks.
>
Sure. I will do that.
Cheers,
Longman
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 19/32] x86/mm: Add support to access persistent memory in the clear
From: Tom Lendacky @ 2017-05-19 19:52 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-arch, linux-efi, kvm, linux-doc, x86, kexec, linux-kernel,
kasan-dev, linux-mm, iommu, Rik van Riel,
Radim Krčmář, Toshimitsu Kani, Arnd Bergmann,
Jonathan Corbet, Matt Fleming, Michael S. Tsirkin, Joerg Roedel,
Konrad Rzeszutek Wilk, Paolo Bonzini, Larry Woodman,
Brijesh Singh, Ingo Molnar, Andy Lutomirski, H. Peter Anvin,
Andrey Ryabinin, Alexander Potapenko, Dave Young, Thomas Gleixner,
Dmitry Vyukov
In-Reply-To: <20170516140449.zmp3sm4krro55bbi@pd.tnic>
On 5/16/2017 9:04 AM, Borislav Petkov wrote:
> On Tue, Apr 18, 2017 at 04:19:42PM -0500, Tom Lendacky wrote:
>> Persistent memory is expected to persist across reboots. The encryption
>> key used by SME will change across reboots which will result in corrupted
>> persistent memory. Persistent memory is handed out by block devices
>> through memory remapping functions, so be sure not to map this memory as
>> encrypted.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> arch/x86/mm/ioremap.c | 31 ++++++++++++++++++++++++++++++-
>> 1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index bce0604..55317ba 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -425,17 +425,46 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
>> * Examine the physical address to determine if it is an area of memory
>> * that should be mapped decrypted. If the memory is not part of the
>> * kernel usable area it was accessed and created decrypted, so these
>> - * areas should be mapped decrypted.
>> + * areas should be mapped decrypted. And since the encryption key can
>> + * change across reboots, persistent memory should also be mapped
>> + * decrypted.
>> */
>> static bool memremap_should_map_decrypted(resource_size_t phys_addr,
>> unsigned long size)
>> {
>> + int is_pmem;
>> +
>> + /*
>> + * Check if the address is part of a persistent memory region.
>> + * This check covers areas added by E820, EFI and ACPI.
>> + */
>> + is_pmem = region_intersects(phys_addr, size, IORESOURCE_MEM,
>> + IORES_DESC_PERSISTENT_MEMORY);
>> + if (is_pmem != REGION_DISJOINT)
>> + return true;
>> +
>> + /*
>> + * Check if the non-volatile attribute is set for an EFI
>> + * reserved area.
>> + */
>> + if (efi_enabled(EFI_BOOT)) {
>> + switch (efi_mem_type(phys_addr)) {
>> + case EFI_RESERVED_TYPE:
>> + if (efi_mem_attributes(phys_addr) & EFI_MEMORY_NV)
>> + return true;
>> + break;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> /* Check if the address is outside kernel usable area */
>> switch (e820__get_entry_type(phys_addr, phys_addr + size - 1)) {
>> case E820_TYPE_RESERVED:
>> case E820_TYPE_ACPI:
>> case E820_TYPE_NVS:
>> case E820_TYPE_UNUSABLE:
>> + case E820_TYPE_PRAM:
>
> Can't you simply add:
>
> case E820_TYPE_PMEM:
>
> here too and thus get rid of the region_intersects() thing above?
>
> Because, for example, e820_type_to_iores_desc() maps E820_TYPE_PMEM to
> IORES_DESC_PERSISTENT_MEMORY so those should be equivalent...
I'll have to double-check this, but I believe that when persistent
memory is identified through the NFIT table it adds it as a resource
but doesn't add it as an e820 entry so I can't rely on the type being
returned as E820_TYPE_PMEM by e820__get_entry_type().
Thanks,
Tom
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 22/32] x86, swiotlb: DMA support for memory encryption
From: Tom Lendacky @ 2017-05-19 19:54 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-arch, linux-efi, kvm, linux-doc, x86, kexec, linux-kernel,
kasan-dev, linux-mm, iommu, Rik van Riel,
Radim Krčmář, Toshimitsu Kani, Arnd Bergmann,
Jonathan Corbet, Matt Fleming, Michael S. Tsirkin, Joerg Roedel,
Konrad Rzeszutek Wilk, Paolo Bonzini, Larry Woodman,
Brijesh Singh, Ingo Molnar, Andy Lutomirski, H. Peter Anvin,
Andrey Ryabinin, Alexander Potapenko, Dave Young, Thomas Gleixner,
Dmitry Vyukov
In-Reply-To: <20170516142740.sxfzkvghxubj2okr@pd.tnic>
On 5/16/2017 9:27 AM, Borislav Petkov wrote:
> On Tue, Apr 18, 2017 at 04:20:10PM -0500, Tom Lendacky wrote:
>> Since DMA addresses will effectively look like 48-bit addresses when the
>> memory encryption mask is set, SWIOTLB is needed if the DMA mask of the
>> device performing the DMA does not support 48-bits. SWIOTLB will be
>> initialized to create decrypted bounce buffers for use by these devices.
>
> Use a verb in the subject:
>
> Subject: x86, swiotlb: Add memory encryption support
>
> or similar.
Will do.
Thanks,
Tom
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 23/32] swiotlb: Add warnings for use of bounce buffers with SME
From: Tom Lendacky @ 2017-05-19 19:55 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-arch, linux-efi, kvm, linux-doc, x86, kexec, linux-kernel,
kasan-dev, linux-mm, iommu, Rik van Riel,
Radim Krčmář, Toshimitsu Kani, Arnd Bergmann,
Jonathan Corbet, Matt Fleming, Michael S. Tsirkin, Joerg Roedel,
Konrad Rzeszutek Wilk, Paolo Bonzini, Larry Woodman,
Brijesh Singh, Ingo Molnar, Andy Lutomirski, H. Peter Anvin,
Andrey Ryabinin, Alexander Potapenko, Dave Young, Thomas Gleixner,
Dmitry Vyukov
In-Reply-To: <20170516145209.ltbmaq3a2teqr2uv@pd.tnic>
On 5/16/2017 9:52 AM, Borislav Petkov wrote:
> On Tue, Apr 18, 2017 at 04:20:19PM -0500, Tom Lendacky wrote:
>> Add warnings to let the user know when bounce buffers are being used for
>> DMA when SME is active. Since the bounce buffers are not in encrypted
>> memory, these notifications are to allow the user to determine some
>> appropriate action - if necessary.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> arch/x86/include/asm/mem_encrypt.h | 11 +++++++++++
>> include/linux/dma-mapping.h | 11 +++++++++++
>> include/linux/mem_encrypt.h | 6 ++++++
>> lib/swiotlb.c | 3 +++
>> 4 files changed, 31 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
>> index 0637b4b..b406df2 100644
>> --- a/arch/x86/include/asm/mem_encrypt.h
>> +++ b/arch/x86/include/asm/mem_encrypt.h
>> @@ -26,6 +26,11 @@ static inline bool sme_active(void)
>> return !!sme_me_mask;
>> }
>>
>> +static inline u64 sme_dma_mask(void)
>> +{
>> + return ((u64)sme_me_mask << 1) - 1;
>> +}
>> +
>> void __init sme_early_encrypt(resource_size_t paddr,
>> unsigned long size);
>> void __init sme_early_decrypt(resource_size_t paddr,
>> @@ -50,6 +55,12 @@ static inline bool sme_active(void)
>> {
>> return false;
>> }
>> +
>> +static inline u64 sme_dma_mask(void)
>> +{
>> + return 0ULL;
>> +}
>> +
>> #endif
>>
>> static inline void __init sme_early_encrypt(resource_size_t paddr,
>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>> index 0977317..f825870 100644
>> --- a/include/linux/dma-mapping.h
>> +++ b/include/linux/dma-mapping.h
>> @@ -10,6 +10,7 @@
>> #include <linux/scatterlist.h>
>> #include <linux/kmemcheck.h>
>> #include <linux/bug.h>
>> +#include <linux/mem_encrypt.h>
>>
>> /**
>> * List of possible attributes associated with a DMA mapping. The semantics
>> @@ -577,6 +578,11 @@ static inline int dma_set_mask(struct device *dev, u64 mask)
>>
>> if (!dev->dma_mask || !dma_supported(dev, mask))
>> return -EIO;
>> +
>> + if (sme_active() && (mask < sme_dma_mask()))
>> + dev_warn_ratelimited(dev,
>> + "SME is active, device will require DMA bounce buffers\n");
>
> Bah, no need to break that line - just let it stick out. Ditto for the
> others.
Ok.
Thanks,
Tom
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 32/32] x86/mm: Add support to make use of Secure Memory Encryption
From: Josh Poimboeuf @ 2017-05-19 20:16 UTC (permalink / raw)
To: Borislav Petkov
Cc: Tom Lendacky, linux-arch, linux-efi, kvm, linux-doc, x86, kexec,
linux-kernel, kasan-dev, linux-mm, iommu, Rik van Riel,
Radim Krčmář, Toshimitsu Kani, Arnd Bergmann,
Jonathan Corbet, Matt Fleming, Michael S. Tsirkin, Joerg Roedel,
Konrad Rzeszutek Wilk, Paolo Bonzini, Larry Woodman,
Brijesh Singh, Ingo Molnar, Andy Lutomirski, H. Peter Anvin,
Andrey Ryabinin, Alexander Potapenko, Dave Young, Thomas Gleixner,
Dmitry Vyukov
In-Reply-To: <20170519113005.3f5kwzg4pgh7j6a5@pd.tnic>
On Fri, May 19, 2017 at 01:30:05PM +0200, Borislav Petkov wrote:
> > it is called so early. I can get past it by adding:
> >
> > CFLAGS_mem_encrypt.o := $(nostackp)
> >
> > in the arch/x86/mm/Makefile, but that obviously eliminates the support
> > for the whole file. Would it be better to split out the sme_enable()
> > and other boot routines into a separate file or just apply the
> > $(nostackp) to the whole file?
>
> Josh might have a better idea here... CCed.
I'm the stack validation guy, not the stack protection guy :-)
But there is a way to disable compiler options on a per-function basis
with the gcc __optimize__ function attribute. For example:
__attribute__((__optimize__("no-stack-protector")))
--
Josh
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics
From: Tejun Heo @ 2017-05-19 20:26 UTC (permalink / raw)
To: Waiman Long
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, linux-mm, kernel-team, pjt, luto, efault
In-Reply-To: <1494855256-12558-12-git-send-email-longman@redhat.com>
Hello, Waiman.
On Mon, May 15, 2017 at 09:34:10AM -0400, Waiman Long wrote:
> Now we could have something like
>
> R -- A -- B
> \
> T1 -- T2
>
> where R is the thread root, A and B are non-threaded cgroups, T1 and
> T2 are threaded cgroups. The cgroups R, T1, T2 form a threaded subtree
> where all the non-threaded resources are accounted for in R. The no
> internal process constraint does not apply in the threaded subtree.
> Non-threaded controllers need to properly handle the competition
> between internal processes and child cgroups at the thread root.
>
> This model will be flexible enough to support the need of the threaded
> controllers.
Maybe I'm misunderstanding the design, but this seems to push the
processes which belong to the threaded subtree to the parent which is
part of the usual resource domain hierarchy thus breaking the no
internal competition constraint. I'm not sure this is something we'd
want. Given that the limitation of the original threaded mode was the
required nesting below root and that we treat root special anyway
(exactly in the way necessary), I wonder whether it'd be better to
simply allow root to be both domain and thread root.
Specific review points below but we'd probably want to discuss the
overall design first.
> +static inline bool cgroup_is_threaded(const struct cgroup *cgrp)
> +{
> + return cgrp->proc_cgrp && (cgrp->proc_cgrp != cgrp);
> +}
> +
> +static inline bool cgroup_is_thread_root(const struct cgroup *cgrp)
> +{
> + return cgrp->proc_cgrp == cgrp;
> +}
Maybe add a bit of comments explaining what's going on with
->proc_cgrp?
> /**
> + * threaded_children_count - returns # of threaded children
> + * @cgrp: cgroup to be tested
> + *
> + * cgroup_mutex must be held by the caller.
> + */
> +static int threaded_children_count(struct cgroup *cgrp)
> +{
> + struct cgroup *child;
> + int count = 0;
> +
> + lockdep_assert_held(&cgroup_mutex);
> + cgroup_for_each_live_child(child, cgrp)
> + if (cgroup_is_threaded(child))
> + count++;
> + return count;
> +}
It probably would be a good idea to keep track of the count so that we
don't have to count them each time. There are cases where people end
up creating a very high number of cgroups and we've already been
bitten a couple times with silly complexity issues.
> @@ -2982,22 +3010,48 @@ static int cgroup_enable_threaded(struct cgroup *cgrp)
> LIST_HEAD(csets);
> struct cgrp_cset_link *link;
> struct css_set *cset, *cset_next;
> + struct cgroup *child;
> int ret;
> + u16 ss_mask;
>
> lockdep_assert_held(&cgroup_mutex);
>
> /* noop if already threaded */
> - if (cgrp->proc_cgrp)
> + if (cgroup_is_threaded(cgrp))
> return 0;
>
> - /* allow only if there are neither children or enabled controllers */
> - if (css_has_online_children(&cgrp->self) || cgrp->subtree_control)
> + /*
> + * Allow only if it is not the root and there are:
> + * 1) no children,
> + * 2) no non-threaded controllers are enabled, and
> + * 3) no attached tasks.
> + *
> + * With no attached tasks, it is assumed that no css_sets will be
> + * linked to the current cgroup. This may not be true if some dead
> + * css_sets linger around due to task_struct leakage, for example.
> + */
It doesn't look like the code is actually making this (incorrect)
assumption. I suppose the comment is from before
cgroup_is_populated() was added?
> spin_lock_irq(&css_set_lock);
> list_for_each_entry(link, &cgrp->cset_links, cset_link) {
> cset = link->cset;
> + if (cset->dead)
> + continue;
Hmm... is this a bug fix which is necessary regardless of whether we
change the threadroot semantics or not?
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file
From: Tejun Heo @ 2017-05-19 20:28 UTC (permalink / raw)
To: Waiman Long
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, linux-mm, kernel-team, pjt, luto, efault
In-Reply-To: <8d942ee6-ebf4-5ba5-5484-60762808f544@redhat.com>
Hello,
On Fri, May 19, 2017 at 03:33:14PM -0400, Waiman Long wrote:
> On 05/19/2017 03:21 PM, Tejun Heo wrote:
> > Yeah but it also shows up as an integral part of stable interface
> > rather than e.g. /sys/kernel/debug. This isn't of any interest to
> > people who aren't developing cgroup core code. There is no reason to
> > risk growing dependencies on it.
>
> The debug controller is used to show information relevant to the cgroup
> its css'es are attached to. So it will be very hard to use if we
> relocate to /sys/kernel/debug, for example. Currently, nothing in the
> debug controller other than debug_cgrp_subsys are exported. I don't see
> any risk of having dependency on that controller from other parts of the
> kernel.
Oh, sure, I wasn't suggesting moving it under /sys/kernel/debug but
that we'd want to take extra precautions as we can't.
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 02/11] mm: mempolicy: add queue_pages_node_check()
From: Mel Gorman @ 2017-05-19 20:28 UTC (permalink / raw)
To: Zi Yan
Cc: Anshuman Khandual, n-horiguchi, kirill.shutemov, linux-kernel,
linux-mm, akpm, minchan, vbabka, mhocko, dnellans
In-Reply-To: <35E3E5BA-2745-4710-A348-B6E5D892DA27@cs.rutgers.edu>
On Fri, May 19, 2017 at 12:37:38PM -0400, Zi Yan wrote:
> > As you say, there is no functional change but the helper name is vague
> > and gives no hint to what's it's checking for. It's somewhat tolerable as
> > it is as it's obvious what is being checked but the same is not true with
> > the helper name.
> >
>
> Does queue_pages_invert_nodemask_check() work? I can change the helper name
> in the next version.
>
Not particularly, maybe queue_pages_required and invert the check with a
comment above it explaining what it's checking for would be ok.
--
Mel Gorman
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v5 32/32] x86/mm: Add support to make use of Secure Memory Encryption
From: Borislav Petkov @ 2017-05-19 20:29 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Tom Lendacky, linux-arch, linux-efi, kvm, linux-doc, x86, kexec,
linux-kernel, kasan-dev, linux-mm, iommu, Rik van Riel,
Radim Krčmář, Toshimitsu Kani, Arnd Bergmann,
Jonathan Corbet, Matt Fleming, Michael S. Tsirkin, Joerg Roedel,
Konrad Rzeszutek Wilk, Paolo Bonzini, Larry Woodman,
Brijesh Singh, Ingo Molnar, Andy Lutomirski, H. Peter Anvin,
Andrey Ryabinin, Alexander Potapenko, Dave Young, Thomas Gleixner,
Dmitry Vyukov
In-Reply-To: <20170519201651.dhayf2pwjlsnouz4@treble>
On Fri, May 19, 2017 at 03:16:51PM -0500, Josh Poimboeuf wrote:
> I'm the stack validation guy, not the stack protection guy :-)
LOL. I thought you were *the* stacks guy. :-)))
But once you've validated it, you could protect it then too. :-)
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox