* Re: [PATCH v3 2/2] powerpc/mm/hugetlb: Add support for 1G huge pages
From: Aneesh Kumar K.V @ 2017-05-18 6:07 UTC (permalink / raw)
To: Michael Ellerman, akpm, Anshuman Khandual
Cc: linux-mm, linux-kernel, linuxppc-dev
In-Reply-To: <87fug2loze.fsf@concordia.ellerman.id.au>
On Thursday 18 May 2017 10:51 AM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
>
>> POWER9 supports hugepages of size 2M and 1G in radix MMU mode. This patch
>> enables the usage of 1G page size for hugetlbfs. This also update the helper
>> such we can do 1G page allocation at runtime.
>>
>> We still don't enable 1G page size on DD1 version. This is to avoid doing
>> workaround mentioned in commit: 6d3a0379ebdc8 (powerpc/mm: Add
>> radix__tlb_flush_pte_p9_dd1()
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/book3s/64/hugetlb.h | 10 ++++++++++
>> arch/powerpc/mm/hugetlbpage.c | 7 +++++--
>> arch/powerpc/platforms/Kconfig.cputype | 1 +
>> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> I think this patch is OK, but it's very confusing because it doesn't
> mention that it's only talking about *generic* gigantic page support.
What you mean by generic gigantic page ? what is supported here is the
gigantic page with size 1G alone ?
>
> We have existing support for gigantic pages on powerpc, on several
> platforms. This patch appears to break that, but I think doesn't in
> practice?
>
What is broken ?
> So can you make it a bit clearer in the commit message, and the code,
> that this is only about enabling the generic gigantic page support, and
> is unrelated to the arch-specific gigantic page support.
>
> cheers
>
>> diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h
>> index 6666cd366596..5c28bd6f2ae1 100644
>> --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
>> +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
>> @@ -50,4 +50,14 @@ static inline pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct *vma,
>> else
>> return entry;
>> }
>> +
>> +#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
>> +static inline bool gigantic_page_supported(void)
>> +{
>> + if (radix_enabled())
>> + return true;
>> + return false;
>> +}
>> +#endif
>> +
>> #endif
>> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
>> index a4f33de4008e..80f6d2ed551a 100644
>> --- a/arch/powerpc/mm/hugetlbpage.c
>> +++ b/arch/powerpc/mm/hugetlbpage.c
>> @@ -763,8 +763,11 @@ static int __init add_huge_page_size(unsigned long long size)
>> * Hash: 16M and 16G
>> */
>> if (radix_enabled()) {
>> - if (mmu_psize != MMU_PAGE_2M)
>> - return -EINVAL;
>> + if (mmu_psize != MMU_PAGE_2M) {
>> + if (cpu_has_feature(CPU_FTR_POWER9_DD1) ||
>> + (mmu_psize != MMU_PAGE_1G))
>> + return -EINVAL;
>> + }
>> } else {
>> if (mmu_psize != MMU_PAGE_16M && mmu_psize != MMU_PAGE_16G)
>> return -EINVAL;
>> diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
>> index 684e886eaae4..b76ef6637016 100644
>> --- a/arch/powerpc/platforms/Kconfig.cputype
>> +++ b/arch/powerpc/platforms/Kconfig.cputype
>> @@ -344,6 +344,7 @@ config PPC_STD_MMU_64
>> config PPC_RADIX_MMU
>> bool "Radix MMU Support"
>> depends on PPC_BOOK3S_64
>> + select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
>> default y
>> help
>> Enable support for the Power ISA 3.0 Radix style MMU. Currently this
>> --
>> 2.7.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
* Re: [PATCH] Patch for remapping pages around the fault page
From: Mike Rapoport @ 2017-05-18 5:53 UTC (permalink / raw)
To: Sarunya Pumma
Cc: akpm, kirill.shutemov, jack, ross.zwisler, mhocko, aneesh.kumar,
lstoakes, dave.jiang, linux-mm
In-Reply-To: <CAC2c7Jts5uZOLXVi9N7xYXxxycv9xM1TBxcC3nMyn0NL-O+spw@mail.gmail.com>
Hello,
On Tue, May 16, 2017 at 12:16:00PM -0400, Sarunya Pumma wrote:
> After the fault handler performs the __do_fault function to read a fault
> page when a page fault occurs, it does not map other pages that have been
> read together with the fault page. This can cause a number of minor page
> faults to be large. Therefore, this patch is developed to remap pages
> around the fault page by aiming to map the pages that have been read
> with the fault page.
[...]
> Thank you very much for your time for reviewing the patch.
>
> Signed-off-by: Sarunya Pumma <sarunya@vt.edu>
> ---
> include/linux/mm.h | 2 ++
> kernel/sysctl.c | 8 +++++
> mm/memory.c | 90
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 100 insertions(+)
The patch is completely unreadable :(
Please use a mail client that does not break whitespace, e.g 'git
send-email'
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7cb17c6..2d533a3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -34,6 +34,8 @@ struct bdi_writeback;
>
> void init_mm_internals(void);
>
> +extern unsigned long vm_nr_remapping;
> +
> #ifndef CONFIG_NEED_MULTIPLE_NODES /* Don't use mapnrs, do it properly */
> extern unsigned long max_mapnr;
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 4dfba1a..16c7efe 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1332,6 +1332,14 @@ static struct ctl_table vm_table[] = {
> .extra1 = &zero,
> .extra2 = &one_hundred,
> },
> + {
> + .procname = "nr_remapping",
> + .data = &vm_nr_remapping,
> + .maxlen = sizeof(vm_nr_remapping),
> + .mode = 0644,
> + .proc_handler = proc_doulongvec_minmax,
> + .extra1 = &zero,
> + },
> #ifdef CONFIG_HUGETLB_PAGE
> {
> .procname = "nr_hugepages",
> diff --git a/mm/memory.c b/mm/memory.c
> index 6ff5d72..3d0dca9 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -83,6 +83,9 @@
> #warning Unfortunate NUMA and NUMA Balancing config, growing page-frame
> for last_cpupid.
> #endif
>
> +/* A preset threshold for considering page remapping */
> +unsigned long vm_nr_remapping = 32;
> +
> #ifndef CONFIG_NEED_MULTIPLE_NODES
> /* use the per-pgdat data instead for discontigmem - mbligh */
> unsigned long max_mapnr;
> @@ -3374,6 +3377,82 @@ static int do_fault_around(struct vm_fault *vmf)
> return ret;
> }
>
> +static int redo_fault_around(struct vm_fault *vmf)
> +{
> + unsigned long address = vmf->address, nr_pages, mask;
> + pgoff_t start_pgoff = vmf->pgoff;
> + pgoff_t end_pgoff;
> + pte_t *lpte, *rpte;
> + int off, ret = 0, is_mapped = 0;
> +
> + nr_pages = READ_ONCE(fault_around_bytes) >> PAGE_SHIFT;
> + mask = ~(nr_pages * PAGE_SIZE - 1) & PAGE_MASK;
> +
> + vmf->address = max(address & mask, vmf->vma->vm_start);
> + off = ((address - vmf->address) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1);
> + start_pgoff -= off;
> +
> + /*
> + * end_pgoff is either end of page table or end of vma
> + * or fault_around_pages() from start_pgoff, depending what is nearest.
> + */
> + end_pgoff = start_pgoff -
> + ((vmf->address >> PAGE_SHIFT) & (PTRS_PER_PTE - 1)) +
> + PTRS_PER_PTE - 1;
> + end_pgoff = min3(end_pgoff, vma_pages(vmf->vma) + vmf->vma->vm_pgoff - 1,
> + start_pgoff + nr_pages - 1);
> +
> + if (nr_pages < vm_nr_remapping) {
> + int i, start_off = 0, end_off = 0;
> +
> + lpte = vmf->pte - off;
> + for (i = 0; i < nr_pages; i++) {
> + if (!pte_none(*lpte)) {
> + is_mapped++;
> + } else {
> + if (!start_off)
> + start_off = i;
> + end_off = i;
> + }
> + lpte++;
> + }
> + if (is_mapped != nr_pages) {
> + is_mapped = 0;
> + end_pgoff = start_pgoff + end_off;
> + start_pgoff += start_off;
> + vmf->pte += start_off;
> + }
> + lpte = NULL;
> + } else {
> + lpte = vmf->pte - 1;
> + rpte = vmf->pte + 1;
> + if (!pte_none(*lpte) && !pte_none(*rpte))
> + is_mapped = 1;
> + lpte = NULL;
> + rpte = NULL;
> + }
> +
> + if (!is_mapped) {
> + vmf->pte -= off;
> + vmf->vma->vm_ops->map_pages(vmf, start_pgoff, end_pgoff);
> + vmf->pte -= (vmf->address >> PAGE_SHIFT) - (address >> PAGE_SHIFT);
> + }
> +
> + /* Huge page is mapped? Page fault is solved */
> + if (pmd_trans_huge(*vmf->pmd)) {
> + ret = VM_FAULT_NOPAGE;
> + goto out;
> + }
> +
> + if (vmf->pte)
> + pte_unmap_unlock(vmf->pte, vmf->ptl);
> +
> +out:
> + vmf->address = address;
> + vmf->pte = NULL;
> + return ret;
> +}
> +
> static int do_read_fault(struct vm_fault *vmf)
> {
> struct vm_area_struct *vma = vmf->vma;
> @@ -3394,6 +3473,17 @@ static int do_read_fault(struct vm_fault *vmf)
> if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY)))
> return ret;
>
> + /*
> + * Remap pages after read
> + */
> + if (!(vma->vm_flags & VM_RAND_READ) && vma->vm_ops->map_pages
> + && fault_around_bytes >> PAGE_SHIFT > 1) {
> + ret |= alloc_set_pte(vmf, vmf->memcg, vmf->page);
> + unlock_page(vmf->page);
> + redo_fault_around(vmf);
> + return ret;
> + }
> +
> ret |= finish_fault(vmf);
> unlock_page(vmf->page);
> if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY)))
> --
> 2.7.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
* Re: [PATCH -mm -v11 1/5] mm, THP, swap: Delay splitting THP during swap out
From: Huang, Ying @ 2017-05-18 5:33 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Andrea Arcangeli, Ebru Akagunduz,
Johannes Weiner, Michal Hocko, Tejun Heo, Hugh Dickins,
Shaohua Li, Minchan Kim, Rik van Riel, cgroups
In-Reply-To: <20170515112522.32457-2-ying.huang@intel.com>
"Huang, Ying" <ying.huang@intel.com> writes:
> From: Huang Ying <ying.huang@intel.com>
>
> In this patch, splitting huge page is delayed from almost the first
> step of swapping out to after allocating the swap space for the
> THP (Transparent Huge Page) and adding the THP into the swap cache.
> This will batch the corresponding operation, thus improve THP swap out
> throughput.
>
> This is the first step for the THP swap optimization. The plan is to
> delay splitting the THP step by step and avoid splitting the THP
> finally.
>
I found two issues in this patch, could you fold the following fix patch
into the original patch?
Best Regards,
Huang, Ying
------------------------------------------------------------->
From: Huang Ying <ying.huang@intel.com>
Subject: [PATCH] mm, THP, swap: Fix two issues in THP optimize patch
When changing the logic for cluster allocation for THP in
get_swap_page(), I made a mistake so that a normal swap slot may be
allocated for a THP instead of return with failure. This is fixed in
the patch.
And I found two likely/unlikely annotation is wrong in
get_swap_pages(), because that is slow path, I just removed the
likely/unlikely annotation.
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Minchan Kim <minchan@kernel.org>
---
mm/swap_slots.c | 5 +++--
mm/swapfile.c | 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/mm/swap_slots.c b/mm/swap_slots.c
index 78047d1efedd..90c1032a8ac3 100644
--- a/mm/swap_slots.c
+++ b/mm/swap_slots.c
@@ -309,8 +309,9 @@ swp_entry_t get_swap_page(struct page *page)
entry.val = 0;
- if (IS_ENABLED(CONFIG_THP_SWAP) && PageTransHuge(page)) {
- get_swap_pages(1, true, &entry);
+ if (PageTransHuge(page)) {
+ if (IS_ENABLED(CONFIG_THP_SWAP))
+ get_swap_pages(1, true, &entry);
return entry;
}
diff --git a/mm/swapfile.c b/mm/swapfile.c
index f4c0f2a92bf0..984f0dd94948 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -937,13 +937,13 @@ int get_swap_pages(int n_goal, bool cluster, swp_entry_t swp_entries[])
spin_unlock(&si->lock);
goto nextsi;
}
- if (likely(cluster))
+ if (cluster)
n_ret = swap_alloc_cluster(si, swp_entries);
else
n_ret = scan_swap_map_slots(si, SWAP_HAS_CACHE,
n_goal, swp_entries);
spin_unlock(&si->lock);
- if (n_ret || unlikely(cluster))
+ if (n_ret || cluster)
goto check_out;
pr_debug("scan_swap_map of si %d failed to find offset\n",
si->type);
--
2.11.0
--
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 v3 2/2] powerpc/mm/hugetlb: Add support for 1G huge pages
From: Michael Ellerman @ 2017-05-18 5:21 UTC (permalink / raw)
To: Aneesh Kumar K.V, akpm, Anshuman Khandual
Cc: linux-mm, linux-kernel, linuxppc-dev
In-Reply-To: <1494995292-4443-2-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
> POWER9 supports hugepages of size 2M and 1G in radix MMU mode. This patch
> enables the usage of 1G page size for hugetlbfs. This also update the helper
> such we can do 1G page allocation at runtime.
>
> We still don't enable 1G page size on DD1 version. This is to avoid doing
> workaround mentioned in commit: 6d3a0379ebdc8 (powerpc/mm: Add
> radix__tlb_flush_pte_p9_dd1()
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/book3s/64/hugetlb.h | 10 ++++++++++
> arch/powerpc/mm/hugetlbpage.c | 7 +++++--
> arch/powerpc/platforms/Kconfig.cputype | 1 +
> 3 files changed, 16 insertions(+), 2 deletions(-)
I think this patch is OK, but it's very confusing because it doesn't
mention that it's only talking about *generic* gigantic page support.
We have existing support for gigantic pages on powerpc, on several
platforms. This patch appears to break that, but I think doesn't in
practice?
So can you make it a bit clearer in the commit message, and the code,
that this is only about enabling the generic gigantic page support, and
is unrelated to the arch-specific gigantic page support.
cheers
> diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h
> index 6666cd366596..5c28bd6f2ae1 100644
> --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
> +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
> @@ -50,4 +50,14 @@ static inline pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct *vma,
> else
> return entry;
> }
> +
> +#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
> +static inline bool gigantic_page_supported(void)
> +{
> + if (radix_enabled())
> + return true;
> + return false;
> +}
> +#endif
> +
> #endif
> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
> index a4f33de4008e..80f6d2ed551a 100644
> --- a/arch/powerpc/mm/hugetlbpage.c
> +++ b/arch/powerpc/mm/hugetlbpage.c
> @@ -763,8 +763,11 @@ static int __init add_huge_page_size(unsigned long long size)
> * Hash: 16M and 16G
> */
> if (radix_enabled()) {
> - if (mmu_psize != MMU_PAGE_2M)
> - return -EINVAL;
> + if (mmu_psize != MMU_PAGE_2M) {
> + if (cpu_has_feature(CPU_FTR_POWER9_DD1) ||
> + (mmu_psize != MMU_PAGE_1G))
> + return -EINVAL;
> + }
> } else {
> if (mmu_psize != MMU_PAGE_16M && mmu_psize != MMU_PAGE_16G)
> return -EINVAL;
> diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
> index 684e886eaae4..b76ef6637016 100644
> --- a/arch/powerpc/platforms/Kconfig.cputype
> +++ b/arch/powerpc/platforms/Kconfig.cputype
> @@ -344,6 +344,7 @@ config PPC_STD_MMU_64
> config PPC_RADIX_MMU
> bool "Radix MMU Support"
> depends on PPC_BOOK3S_64
> + select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
> default y
> help
> Enable support for the Power ISA 3.0 Radix style MMU. Currently this
> --
> 2.7.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
* Re: [PATCH] mm,oom: fix oom invocation issues
From: Tetsuo Handa @ 2017-05-17 22:03 UTC (permalink / raw)
To: guro, mhocko; +Cc: hannes, vdavydov.dev, kernel-team, linux-mm, linux-kernel
In-Reply-To: <20170517194316.GA30517@castle>
Roman Gushchin wrote:
> On Wed, May 17, 2017 at 06:14:46PM +0200, Michal Hocko wrote:
> > On Wed 17-05-17 16:26:20, Roman Gushchin wrote:
> > [...]
> > > [ 25.781882] Out of memory: Kill process 492 (allocate) score 899 or sacrifice child
> > > [ 25.783874] Killed process 492 (allocate) total-vm:2052368kB, anon-rss:1894576kB, file-rss:4kB, shmem-rss:0kB
> >
> > Are there any oom_reaper messages? Could you provide the full kernel log
> > please?
>
> Sure. Sorry, it was too bulky, so I've cut the line about oom_reaper by mistake.
> Here it is:
> --------------------------------------------------------------------------------
> [ 25.721494] allocate invoked oom-killer: gfp_mask=0x14280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), nodemask=(null), order=0, oom_score_adj=0
> [ 25.725658] allocate cpuset=/ mems_allowed=0
> [ 25.759892] Node 0 DMA32 free:44700kB min:44704kB low:55880kB high:67056kB active_anon:1944216kB inactive_anon:204kB active_file:592kB inactive_file:0kB unevictable:0kB writepending:304kB present:2080640kB managed:2031972kB mlocked:0kB slab_reclaimable:11336kB slab_unreclaimable:9784kB kernel_stack:1776kB pagetables:6932kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
> [ 25.781882] Out of memory: Kill process 492 (allocate) score 899 or sacrifice child
> [ 25.783874] Killed process 492 (allocate) total-vm:2052368kB, anon-rss:1894576kB, file-rss:4kB, shmem-rss:0kB
> [ 25.785680] allocate: page allocation failure: order:0, mode:0x14280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), nodemask=(null)
> [ 25.786797] allocate cpuset=/ mems_allowed=0
This is a side effect of commit 9a67f6488eca926f ("mm: consolidate GFP_NOFAIL
checks in the allocator slowpath") which I noticed at
http://lkml.kernel.org/r/e7f932bf-313a-917d-6304-81528aca5994@I-love.SAKURA.ne.jp .
> [ 25.804499] Node 0 DMA32 free:251876kB min:44704kB low:55880kB high:67056kB active_anon:1737368kB inactive_anon:204kB active_file:592kB inactive_file:0kB unevictable:0kB writepending:304kB present:2080640kB managed:2031972kB mlocked:0kB slab_reclaimable:10312kB slab_unreclaimable:9784kB kernel_stack:1776kB pagetables:6932kB bounce:0kB free_pcp:700kB local_pcp:0kB free_cma:0kB
> [ 25.817589] allocate invoked oom-killer: gfp_mask=0x0(), nodemask=(null), order=0, oom_score_adj=0
> [ 25.818821] allocate cpuset=/ mems_allowed=0
Since pagefault_out_of_memory() is unconditionally called if a normal allocation failed,
that commit made pagefault_out_of_memory() to be called if current thread was selected as
an OOM victim, despite the system is no longer under memory pressure because the OOM reaper
has reclaimed memory.
> [ 25.835784] Node 0 DMA32 free:1934360kB min:44704kB low:55880kB high:67056kB active_anon:57104kB inactive_anon:204kB active_file:416kB inactive_file:2476kB unevictable:0kB writepending:424kB present:2080640kB managed:2031972kB mlocked:0kB slab_reclaimable:10236kB slab_unreclaimable:9584kB kernel_stack:1776kB pagetables:3604kB bounce:0kB free_pcp:144kB local_pcp:0kB free_cma:0kB
> [ 25.863078] Out of memory: Kill process 233 (firewalld) score 10 or sacrifice child
> [ 25.863634] Killed process 233 (firewalld) total-vm:246076kB, anon-rss:20956kB, file-rss:0kB, shmem-rss:0kB
> --------------------------------------------------------------------------------
--
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: [bug report] ksm: introduce ksm_max_page_sharing per page deduplication limit
From: Andrea Arcangeli @ 2017-05-17 21:52 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mm, Hugh Dickins
In-Reply-To: <20170517200255.67kvej2onwv54psi@mwanda>
Hello Dan,
On Wed, May 17, 2017 at 11:02:55PM +0300, Dan Carpenter wrote:
> Hello Andrea Arcangeli,
>
> The patch 1073fbb7013b: "ksm: introduce ksm_max_page_sharing per page
> deduplication limit" from May 13, 2017, leads to the following static
> checker warning:
>
> mm/ksm.c:1442 __stable_node_chain()
> warn: 'stable_node' was already freed.
>
> mm/ksm.c
> 1433 static struct stable_node *__stable_node_chain(struct stable_node **_stable_node,
> 1434 struct page **tree_page,
> 1435 struct rb_root *root,
> 1436 bool prune_stale_stable_nodes)
> 1437 {
> 1438 struct stable_node *stable_node = *_stable_node;
> 1439 if (!is_stable_node_chain(stable_node)) {
> 1440 if (is_page_sharing_candidate(stable_node)) {
> 1441 *tree_page = get_ksm_page(stable_node, false);
> 1442 return stable_node;
>
> There is a comment about this somewhere down the call tree but if
> get_ksm_page() fails then we're returning a freed pointer here which is
> gnarly.
>
> 1443 }
> 1444 return NULL;
> 1445 }
> 1446 return stable_node_dup(_stable_node, tree_page, root,
> 1447 prune_stale_stable_nodes);
> 1448 }
__stable_node_chain is invoked by chain and chain_prune.
stable_node_dup = chain(stable_node, &tree_page, root);
if (!stable_node_dup) {
[..]
}
VM_BUG_ON(!stable_node_dup ^ !!stable_node_any);
if (!tree_page) {
goto again;
stable_node_dup = chain_prune(&stable_node, &tree_page, root);
if (!stable_node_dup) {
[..]
}
VM_BUG_ON(!stable_node_dup ^ !!stable_node_any);
if (!tree_page) {
goto again;
If the stable_node was freed tree_page is NULL. So it's a false
positive.
I agree it's not great to return a stale pointer but if we return
NULL, stable_node_dup_any would then run on the stale stable_node
which would then be a kernel crashing bug. There's a reason why this
isn't returned as NULL and we depend on the following tree_page check
to bail out.
I noticed in the fix for the real stale stable_node corruption with
merge_across_node = 0, it may be enough to set *_stable_node to
"found" (instead of NULL as I implemented in the fix). This way when
the chain is collapsed to the caller it will look like the chain never
existed and there's no special !stable_node check to care about later
(the check can return "stable_node == stable_node_dup"). Considering
such bug was real I wanted to set it to NULL to be sure the stale
stable_node was never accessed by mistake following such collapse
(NULL provided extra safety). However now that such bug seems fixed,
it may be enough to hide the collapse and it'll remove one branch in
the code as there will be no need to check !stable_node to detect the
collapse. It's a bit more risky than my initial fix but it could be
also considered a cleanup.
I thought above the above in the context of you report, because then
we could proceed to set *_stable_node = NULL in the case you worried
about, which would get its own new meaning. In the current code
setting *_stable_node to NULL under the chain*() calls means the
collapse happened (which doesn't rebalance the tree and doesn't
require to restart the look), while it could then mean the regular
stable_node was freed (which rebalances the tree and the lookup must
restart). Then we could return NULL in chain*() if the KSM page was
freed and then bail out immediately if stable_node also become NULL.
Ideally we could go further, and change get_ksm_page to get
&stable_node a parameter and set it to NULL in the caller if it's
freed by the callee to wipe out all stale pointers from all callees.
Considering these are purely cleanups, comments are welcome, as I've
no strong particular preference about this but I can implement if
agreed.
Thanks,
Andrea
--
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-17 21:47 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:
> The current thread mode semantics aren't sufficient to fully support
> threaded controllers like cpu. The main problem is that when thread
> mode is enabled at root (mainly for performance reason), all the
> non-threaded controllers cannot be supported at all.
>
> To alleviate this problem, the roles of thread root and threaded
> cgroups are now further separated. Now thread mode can only be enabled
> on a non-root leaf cgroup whose parent will then become the thread
> root. All the descendants of a threaded cgroup will still need to be
> threaded. All the non-threaded resource will be accounted for in the
> thread root. Unlike the previous thread mode, however, a thread root
> can have non-threaded children where system resources like memory
> can be further split down the hierarchy.
>
> 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.
I do like the approach and it does address the issue with requiring at
least one level of nesting for the thread mode to be used with other
controllers. I need to think a bit more about it and mull over what
Peterz was suggesting in the old thread. I'll get back to you soon
but I'd really prefer this and the earlier related patches to be in
its own patchset so that we aren't dealing with different things at
the same time.
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 10/17] cgroup: Make debug cgroup support v2 and thread mode
From: Tejun Heo @ 2017-05-17 21:43 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-11-git-send-email-longman@redhat.com>
Hello,
On Mon, May 15, 2017 at 09:34:09AM -0400, Waiman Long wrote:
> Besides supporting cgroup v2 and thread mode, the following changes
> are also made:
> 1) current_* cgroup files now resides only at the root as we don't
> need duplicated files of the same function all over the cgroup
> hierarchy.
> 2) The cgroup_css_links_read() function is modified to report
> the number of tasks that are skipped because of overflow.
> 3) The relationship between proc_cset and threaded_csets are displayed.
> 4) The number of extra unaccounted references are displayed.
> 5) The status of being a thread root or threaded cgroup is displayed.
> 6) The current_css_set_read() function now prints out the addresses of
> the css'es associated with the current css_set.
> 7) A new cgroup_subsys_states file is added to display the css objects
> associated with a cgroup.
> 8) A new cgroup_masks file is added to display the various controller
> bit masks in the cgroup.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
As noted before, please make it clear that this is a debug feature and
not expected to be stable. Also, I don't see why this and the
previous two patches are in this series. Can you please split these
out to a separate patchset?
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 09/17] cgroup: Keep accurate count of tasks in each css_set
From: Tejun Heo @ 2017-05-17 21:40 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-10-git-send-email-longman@redhat.com>
Hello,
On Mon, May 15, 2017 at 09:34:08AM -0400, Waiman Long wrote:
> The reference count in the css_set data structure was used as a
> proxy of the number of tasks attached to that css_set. However, that
> count is actually not an accurate measure especially with thread mode
> support. So a new variable task_count is added to the css_set to keep
> track of the actual task count. This new variable is protected by
> the css_set_lock. Functions that require the actual task count are
> updated to use the new variable.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
Looks good. We probably should replace css_set_populated() to use
this too.
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-17 21:36 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-9-git-send-email-longman@redhat.com>
Hello, Waiman.
On Mon, May 15, 2017 at 09:34:07AM -0400, Waiman Long wrote:
> The debug cgroup currently resides within cgroup-v1.c and is enabled
> only for v1 cgroup. To enable the debug cgroup also for v2, it
> makes sense to put the code into its own file as it will no longer
> be v1 specific. The only change in this patch is the expansion of
> cgroup_task_count() within the debug_taskcount_read() function.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
I don't mind enabling the debug controller for v2 but let's please
hide it behind an unwieldy boot param / controller name so that it's
clear that its interface isn't expected to be stable.
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 07/17] cgroup: Prevent kill_css() from being called more than once
From: Tejun Heo @ 2017-05-17 21:34 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: <c541638f-b302-8c96-0dcd-f4b758a4a81f@redhat.com>
On Wed, May 17, 2017 at 04:24:32PM -0400, Waiman Long wrote:
> On 05/17/2017 03:23 PM, Tejun Heo wrote:
> > Hello,
> >
> > On Mon, May 15, 2017 at 09:34:06AM -0400, Waiman Long wrote:
> >> The kill_css() function may be called more than once under the condition
> >> that the css was killed but not physically removed yet followed by the
> >> removal of the cgroup that is hosting the css. This patch prevents any
> >> harmm from being done when that happens.
> >>
> >> Signed-off-by: Waiman Long <longman@redhat.com>
> > So, this is a bug fix which isn't really related to this patchset.
> > I'm applying it to cgroup/for-4.12-fixes w/ stable cc'd.
> >
> > Thanks.
> >
> Actually, this bug can be easily triggered with the resource domain
> patch later in the series. I guess it can also happen in the current
> code base, but I don't have a test that can reproduce it.
I can reproduce it easily.
[test /sys/fs/cgroup/asdf]# while true; do mkdir asdf; echo +memory > cgroup.subtree_control; echo -memory
[ 66.159258] percpu_ref_kill_and_confirm called more than once on css_release!
[ 66.159293] ------------[ cut here ]------------
[ 66.160966] WARNING: CPU: 1 PID: 1802 at lib/percpu-refcount.c:334 percpu_ref_kill_and_confirm+0x190/0x1a0
[ 66.162406] Modules linked in:
[ 66.162686] CPU: 1 PID: 1802 Comm: rmdir Not tainted 4.12.0-rc1-work+ #42
[ 66.163279] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1.fc26 04/01/2014
[ 66.164043] task: ffff880018240040 task.stack: ffffc90000478000
[ 66.164571] RIP: 0010:percpu_ref_kill_and_confirm+0x190/0x1a0
[ 66.165106] RSP: 0018:ffffc9000047bde8 EFLAGS: 00010092
[ 66.165664] RAX: 0000000000000041 RBX: ffff88001a0fc148 RCX: 0000000000000002
[ 66.166443] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff810acc2e
[ 66.167083] RBP: ffffc9000047be00 R08: 0000000000000000 R09: 0000000000000001
[ 66.167696] R10: ffffc9000047bd50 R11: 0000000000000000 R12: 0000000000000286
[ 66.168293] R13: ffffffff810e7c50 R14: ffff88001a106bb8 R15: 0000000000000000
[ 66.168897] FS: 00007fba87594700(0000) GS:ffff88001fc80000(0000) knlGS:0000000000000000
[ 66.169613] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 66.170204] CR2: 000056167ee4e008 CR3: 000000001820e000 CR4: 00000000000006a0
[ 66.170861] Call Trace:
[ 66.171075] kill_css+0x3e/0x170
[ 66.171352] cgroup_destroy_locked+0xac/0x170
[ 66.171732] cgroup_rmdir+0x2c/0x150
[ 66.172037] kernfs_iop_rmdir+0x48/0x70
[ 66.172377] vfs_rmdir+0x73/0x150
[ 66.172679] do_rmdir+0x16d/0x1c0
[ 66.172962] SyS_rmdir+0x16/0x20
[ 66.173244] entry_SYSCALL_64_fastpath+0x18/0xad
[ 66.173682] RIP: 0033:0x7fba870c1487
[ 66.173985] RSP: 002b:00007ffec57e43a8 EFLAGS: 00000246 ORIG_RAX: 0000000000000054
[ 66.174719] RAX: ffffffffffffffda RBX: 00007fba87388b38 RCX: 00007fba870c1487
[ 66.175439] RDX: 00007fba8738ae80 RSI: 0000000000000000 RDI: 00007ffec57e5751
[ 66.176281] RBP: 00007fba87388ae0 R08: 0000000000000000 R09: 0000000000000000
[ 66.177046] R10: 000056167ee4e010 R11: 0000000000000246 R12: 00007fba87388b38
[ 66.177821] R13: 0000000000000030 R14: 00007fba87388b58 R15: 0000000000002710
[ 66.178574] Code: 80 3d f5 d3 89 00 00 0f 85 b8 fe ff ff 48 8b 53 10 48 c7 c6 e0 c0 83 81 48 c7 c7 68 e0 9c 81 c6 05 d6 d3 89 00 01 e8 09 0a d4 ff <0f> ff 48 8b 43 08 e9 8f fe ff ff 0f 1f 44 00 00 55 ba ff ffff
[ 66.181059] ---[ end trace 50ce5cd95cda7a2c ]---
--
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 18/32] x86, mpparse: Use memremap to map the mpf and mpc data
From: Tom Lendacky @ 2017-05-17 20:26 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: <20170516083658.fq2h4ysmrbgn23cs@pd.tnic>
On 5/16/2017 3:36 AM, Borislav Petkov wrote:
> On Tue, Apr 18, 2017 at 04:19:30PM -0500, Tom Lendacky wrote:
>> The SMP MP-table is built by UEFI and placed in memory in a decrypted
>> state. These tables are accessed using a mix of early_memremap(),
>> early_memunmap(), phys_to_virt() and virt_to_phys(). Change all accesses
>> to use early_memremap()/early_memunmap(). This allows for proper setting
>> of the encryption mask so that the data can be successfully accessed when
>> SME is active.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> arch/x86/kernel/mpparse.c | 102 +++++++++++++++++++++++++++++++--------------
>> 1 file changed, 71 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
>> index fd37f39..afbda41d 100644
>> --- a/arch/x86/kernel/mpparse.c
>> +++ b/arch/x86/kernel/mpparse.c
>> @@ -429,7 +429,21 @@ static inline void __init construct_default_ISA_mptable(int mpc_default_type)
>> }
>> }
>>
>> -static struct mpf_intel *mpf_found;
>> +static unsigned long mpf_base;
>> +
>> +static void __init unmap_mpf(struct mpf_intel *mpf)
>> +{
>> + early_memunmap(mpf, sizeof(*mpf));
>> +}
>> +
>> +static struct mpf_intel * __init map_mpf(unsigned long paddr)
>> +{
>> + struct mpf_intel *mpf;
>> +
>> + mpf = early_memremap(paddr, sizeof(*mpf));
>> +
>> + return mpf;
>
> return early_memremap(paddr, sizeof(*mpf));
>
Ok.
> ...
>
>> @@ -842,25 +873,26 @@ static int __init update_mp_table(void)
>> if (!enable_update_mptable)
>> return 0;
>>
>> - mpf = mpf_found;
>> - if (!mpf)
>> + if (!mpf_base)
>> return 0;
>>
>> + mpf = map_mpf(mpf_base);
>> +
>> /*
>> * Now see if we need to go further.
>> */
>> if (mpf->feature1 != 0)
>
> You're kidding, right? map_mpf() *can* return NULL.
Ugh... don't know how I forgot about that. Will fix everywhere.
>
> Also, simplify that test:
>
> if (mpf->feature1)
> ...
Ok, I can do that but I hope no one says anything about it being
unrelated to the patch. :)
>
>
>> - return 0;
>> + goto do_unmap_mpf;
>>
>> if (!mpf->physptr)
>> - return 0;
>> + goto do_unmap_mpf;
>>
>> - mpc = phys_to_virt(mpf->physptr);
>> + mpc = map_mpc(mpf->physptr);
>
> Again: error checking !!!
>
> You have other calls to early_memremap()/map_mpf() in this patch. Please
> add error checking everywhere.
Yup.
>
>>
>> if (!smp_check_mpc(mpc, oem, str))
>> - return 0;
>> + goto do_unmap_mpc;
>>
>> - pr_info("mpf: %llx\n", (u64)virt_to_phys(mpf));
>> + pr_info("mpf: %llx\n", (u64)mpf_base);
>> pr_info("physptr: %x\n", mpf->physptr);
>>
>> if (mpc_new_phys && mpc->length > mpc_new_length) {
>> @@ -878,21 +910,23 @@ static int __init update_mp_table(void)
>> new = mpf_checksum((unsigned char *)mpc, mpc->length);
>> if (old == new) {
>> pr_info("mpc is readonly, please try alloc_mptable instead\n");
>> - return 0;
>> + goto do_unmap_mpc;
>> }
>> pr_info("use in-position replacing\n");
>> } else {
>> mpf->physptr = mpc_new_phys;
>> - mpc_new = phys_to_virt(mpc_new_phys);
>> + mpc_new = map_mpc(mpc_new_phys);
>
> Ditto.
>
>> memcpy(mpc_new, mpc, mpc->length);
>> + unmap_mpc(mpc);
>> mpc = mpc_new;
>> /* check if we can modify that */
>> if (mpc_new_phys - mpf->physptr) {
>> struct mpf_intel *mpf_new;
>> /* steal 16 bytes from [0, 1k) */
>> pr_info("mpf new: %x\n", 0x400 - 16);
>> - mpf_new = phys_to_virt(0x400 - 16);
>> + mpf_new = map_mpf(0x400 - 16);
>
> Ditto.
>
>> memcpy(mpf_new, mpf, 16);
>> + unmap_mpf(mpf);
>> mpf = mpf_new;
>> mpf->physptr = mpc_new_phys;
>> }
>
--
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 07/17] cgroup: Prevent kill_css() from being called more than once
From: Waiman Long @ 2017-05-17 20:24 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: <20170517192357.GC942@htj.duckdns.org>
On 05/17/2017 03:23 PM, Tejun Heo wrote:
> Hello,
>
> On Mon, May 15, 2017 at 09:34:06AM -0400, Waiman Long wrote:
>> The kill_css() function may be called more than once under the condition
>> that the css was killed but not physically removed yet followed by the
>> removal of the cgroup that is hosting the css. This patch prevents any
>> harmm from being done when that happens.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> So, this is a bug fix which isn't really related to this patchset.
> I'm applying it to cgroup/for-4.12-fixes w/ stable cc'd.
>
> Thanks.
>
Actually, this bug can be easily triggered with the resource domain
patch later in the series. I guess it can also happen in the current
code base, but I don't have a test that can reproduce it.
Regards,
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
* [bug report] ksm: introduce ksm_max_page_sharing per page deduplication limit
From: Dan Carpenter @ 2017-05-17 20:02 UTC (permalink / raw)
To: aarcange; +Cc: linux-mm
Hello Andrea Arcangeli,
The patch 1073fbb7013b: "ksm: introduce ksm_max_page_sharing per page
deduplication limit" from May 13, 2017, leads to the following static
checker warning:
mm/ksm.c:1442 __stable_node_chain()
warn: 'stable_node' was already freed.
mm/ksm.c
1433 static struct stable_node *__stable_node_chain(struct stable_node **_stable_node,
1434 struct page **tree_page,
1435 struct rb_root *root,
1436 bool prune_stale_stable_nodes)
1437 {
1438 struct stable_node *stable_node = *_stable_node;
1439 if (!is_stable_node_chain(stable_node)) {
1440 if (is_page_sharing_candidate(stable_node)) {
1441 *tree_page = get_ksm_page(stable_node, false);
1442 return stable_node;
There is a comment about this somewhere down the call tree but if
get_ksm_page() fails then we're returning a freed pointer here which is
gnarly.
1443 }
1444 return NULL;
1445 }
1446 return stable_node_dup(_stable_node, tree_page, root,
1447 prune_stale_stable_nodes);
1448 }
regards,
dan carpenter
--
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] mm,oom: fix oom invocation issues
From: Roman Gushchin @ 2017-05-17 19:43 UTC (permalink / raw)
To: Michal Hocko
Cc: Tetsuo Handa, Johannes Weiner, Vladimir Davydov, kernel-team,
linux-mm, linux-kernel
In-Reply-To: <20170517161446.GB20660@dhcp22.suse.cz>
On Wed, May 17, 2017 at 06:14:46PM +0200, Michal Hocko wrote:
> On Wed 17-05-17 16:26:20, Roman Gushchin wrote:
> [...]
> > [ 25.781882] Out of memory: Kill process 492 (allocate) score 899 or sacrifice child
> > [ 25.783874] Killed process 492 (allocate) total-vm:2052368kB, anon-rss:1894576kB, file-rss:4kB, shmem-rss:0kB
>
> Are there any oom_reaper messages? Could you provide the full kernel log
> please?
Sure. Sorry, it was too bulky, so I've cut the line about oom_reaper by mistake.
Here it is:
--------------------------------------------------------------------------------
[ 25.721494] allocate invoked oom-killer: gfp_mask=0x14280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), nodemask=(null), order=0, oom_score_adj=0
[ 25.725658] allocate cpuset=/ mems_allowed=0
[ 25.727033] CPU: 1 PID: 492 Comm: allocate Not tainted 4.12.0-rc1-mm1+ #181
[ 25.729215] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[ 25.729598] Call Trace:
[ 25.729598] dump_stack+0x63/0x82
[ 25.729598] dump_header+0x97/0x21a
[ 25.729598] ? do_try_to_free_pages+0x2d7/0x360
[ 25.729598] ? security_capable_noaudit+0x45/0x60
[ 25.729598] oom_kill_process+0x219/0x3e0
[ 25.729598] out_of_memory+0x11d/0x480
[ 25.729598] __alloc_pages_slowpath+0xc84/0xd40
[ 25.729598] __alloc_pages_nodemask+0x245/0x260
[ 25.729598] alloc_pages_vma+0xa2/0x270
[ 25.729598] __handle_mm_fault+0xca9/0x10c0
[ 25.729598] handle_mm_fault+0xf3/0x210
[ 25.729598] __do_page_fault+0x240/0x4e0
[ 25.729598] trace_do_page_fault+0x37/0xe0
[ 25.729598] do_async_page_fault+0x19/0x70
[ 25.729598] async_page_fault+0x28/0x30
[ 25.729598] RIP: 0033:0x400760
[ 25.729598] RSP: 002b:00007ffe30dd9970 EFLAGS: 00010287
[ 25.729598] RAX: 00007fd9bd760010 RBX: 0000000070800000 RCX: 0000000000000006
[ 25.729598] RDX: 00007fd9c6d2a010 RSI: 000000000c801000 RDI: 0000000000000000
[ 25.729598] RBP: 000000000c800000 R08: ffffffffffffffff R09: 0000000000000000
[ 25.729598] R10: 00007fd9ba52a010 R11: 0000000000000246 R12: 00000000004007b0
[ 25.729598] R13: 00007ffe30dd9a60 R14: 0000000000000000 R15: 0000000000000000
[ 25.750476] Mem-Info:
[ 25.750746] active_anon:487992 inactive_anon:51 isolated_anon:0
[ 25.750746] active_file:30 inactive_file:12 isolated_file:0
[ 25.750746] unevictable:0 dirty:30 writeback:0 unstable:0
[ 25.750746] slab_reclaimable:2834 slab_unreclaimable:2448
[ 25.750746] mapped:27 shmem:123 pagetables:1739 bounce:0
[ 25.750746] free:13239 free_pcp:0 free_cma:0
[ 25.754758] Node 0 active_anon:1951968kB inactive_anon:204kB active_file:120kB inactive_file:48kB unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:108kB dirty:120kB writeback:0kB shmem:492kB shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 1726464kB writeback_tmp:0kB unstable:0kB all_unreclaimable? no
[ 25.757328] Node 0 DMA free:8256kB min:348kB low:432kB high:516kB active_anon:7588kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB present:15992kB managed:15908kB mlocked:0kB slab_reclaimable:0kB slab_unreclaimable:8kB kernel_stack:0kB pagetables:24kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
[ 25.759531] lowmem_reserve[]: 0 1981 1981 1981 1981
[ 25.759892] Node 0 DMA32 free:44700kB min:44704kB low:55880kB high:67056kB active_anon:1944216kB inactive_anon:204kB active_file:592kB inactive_file:0kB unevictable:0kB writepending:304kB present:2080640kB managed:2031972kB mlocked:0kB slab_reclaimable:11336kB slab_unreclaimable:9784kB kernel_stack:1776kB pagetables:6932kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
[ 25.762570] lowmem_reserve[]: 0 0 0 0 0
[ 25.762867] Node 0 DMA: 2*4kB (UM) 1*8kB (M) 1*16kB (M) 1*32kB (U) 2*64kB (UM) 1*128kB (U) 1*256kB (U) 1*512kB (M) 1*1024kB (U) 1*2048kB (M) 1*4096kB (M) = 8256kB
[ 25.763947] Node 0 DMA32: 319*4kB (UME) 192*8kB (UME) 81*16kB (UE) 32*32kB (UME) 13*64kB (UME) 81*128kB (UME) 43*256kB (M) 23*512kB (UME) 6*1024kB (UME) 0*2048kB 0*4096kB = 45260kB
[ 25.765134] Node 0 hugepages_total=0 hugepages_free=0 hugepages_surp=0 hugepages_size=2048kB
[ 25.765752] 166 total pagecache pages
[ 25.766025] 0 pages in swap cache
[ 25.766273] Swap cache stats: add 0, delete 0, find 0/0
[ 25.766658] Free swap = 0kB
[ 25.766874] Total swap = 0kB
[ 25.767091] 524158 pages RAM
[ 25.767308] 0 pages HighMem/MovableOnly
[ 25.767602] 12188 pages reserved
[ 25.767844] 0 pages cma reserved
[ 25.768083] 0 pages hwpoisoned
[ 25.768293] [ pid ] uid tgid total_vm rss nr_ptes nr_pmds swapents oom_score_adj name
[ 25.768860] [ 121] 0 121 25672 133 50 3 0 0 systemd-journal
[ 25.769530] [ 156] 0 156 11157 197 22 3 0 -1000 systemd-udevd
[ 25.770206] [ 206] 0 206 13896 99 29 3 0 -1000 auditd
[ 25.770822] [ 227] 0 227 11874 124 27 3 0 0 systemd-logind
[ 25.771494] [ 229] 81 229 11577 146 28 3 0 -900 dbus-daemon
[ 25.772126] [ 231] 997 231 27502 102 25 3 0 0 chronyd
[ 25.772731] [ 233] 0 233 61519 5239 85 3 0 0 firewalld
[ 25.773345] [ 238] 0 238 123495 529 74 4 0 0 NetworkManager
[ 25.773988] [ 265] 0 265 25117 231 52 3 0 -1000 sshd
[ 25.774569] [ 271] 0 271 6092 154 17 3 0 0 crond
[ 25.775137] [ 277] 0 277 11297 93 26 3 0 0 systemd-hostnam
[ 25.775766] [ 284] 0 284 1716 29 9 3 0 0 agetty
[ 25.776342] [ 285] 0 285 2030 34 9 4 0 0 agetty
[ 25.776919] [ 302] 998 302 133102 2578 58 3 0 0 polkitd
[ 25.777505] [ 394] 0 394 21785 3076 45 3 0 0 dhclient
[ 25.778092] [ 444] 0 444 36717 312 74 3 0 0 sshd
[ 25.778744] [ 446] 0 446 15966 223 36 3 0 0 systemd
[ 25.779304] [ 447] 0 447 23459 384 47 3 0 0 (sd-pam)
[ 25.779877] [ 451] 0 451 36717 316 72 3 0 0 sshd
[ 25.780450] [ 452] 0 452 3611 315 11 3 0 0 bash
[ 25.781107] [ 492] 0 492 513092 473645 934 5 0 0 allocate
[ 25.781882] Out of memory: Kill process 492 (allocate) score 899 or sacrifice child
[ 25.783874] Killed process 492 (allocate) total-vm:2052368kB, anon-rss:1894576kB, file-rss:4kB, shmem-rss:0kB
[ 25.785680] allocate: page allocation failure: order:0, mode:0x14280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), nodemask=(null)
[ 25.786797] allocate cpuset=/ mems_allowed=0
[ 25.787246] CPU: 1 PID: 492 Comm: allocate Not tainted 4.12.0-rc1-mm1+ #181
[ 25.787935] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[ 25.788867] Call Trace:
[ 25.789119] dump_stack+0x63/0x82
[ 25.789451] warn_alloc+0x114/0x1b0
[ 25.789451] __alloc_pages_slowpath+0xd32/0xd40
[ 25.789451] __alloc_pages_nodemask+0x245/0x260
[ 25.789451] alloc_pages_vma+0xa2/0x270
[ 25.789451] __handle_mm_fault+0xca9/0x10c0
[ 25.789451] handle_mm_fault+0xf3/0x210
[ 25.789451] __do_page_fault+0x240/0x4e0
[ 25.789451] trace_do_page_fault+0x37/0xe0
[ 25.789451] do_async_page_fault+0x19/0x70
[ 25.789451] async_page_fault+0x28/0x30
[ 25.789451] RIP: 0033:0x400760
[ 25.789451] RSP: 002b:00007ffe30dd9970 EFLAGS: 00010287
[ 25.789451] RAX: 00007fd9bd760010 RBX: 0000000070800000 RCX: 0000000000000006
[ 25.789451] RDX: 00007fd9c6d2a010 RSI: 000000000c801000 RDI: 0000000000000000
[ 25.789451] RBP: 000000000c800000 R08: ffffffffffffffff R09: 0000000000000000
[ 25.789451] R10: 00007fd9ba52a010 R11: 0000000000000246 R12: 00000000004007b0
[ 25.789451] R13: 00007ffe30dd9a60 R14: 0000000000000000 R15: 0000000000000000
[ 25.797570] Mem-Info:
[ 25.797796] active_anon:476253 inactive_anon:51 isolated_anon:0
[ 25.797796] active_file:30 inactive_file:12 isolated_file:0
[ 25.797796] unevictable:0 dirty:30 writeback:0 unstable:0
[ 25.797796] slab_reclaimable:2578 slab_unreclaimable:2448
[ 25.797796] mapped:27 shmem:123 pagetables:1739 bounce:0
[ 25.797796] free:24856 free_pcp:174 free_cma:0
[ 25.799701] Node 0 active_anon:1790620kB inactive_anon:204kB active_file:120kB inactive_file:48kB unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:108kB dirty:120kB writeback:0kB shmem:492kB shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 1662976kB writeback_tmp:0kB unstable:0kB all_unreclaimable? yes
[ 25.801880] Node 0 DMA free:15808kB min:348kB low:432kB high:516kB active_anon:36kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB present:15992kB managed:15908kB mlocked:0kB slab_reclaimable:0kB slab_unreclaimable:8kB kernel_stack:0kB pagetables:24kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
[ 25.804130] lowmem_reserve[]: 0 1981 1981 1981 1981
[ 25.804499] Node 0 DMA32 free:251876kB min:44704kB low:55880kB high:67056kB active_anon:1737368kB inactive_anon:204kB active_file:592kB inactive_file:0kB unevictable:0kB writepending:304kB present:2080640kB managed:2031972kB mlocked:0kB slab_reclaimable:10312kB slab_unreclaimable:9784kB kernel_stack:1776kB pagetables:6932kB bounce:0kB free_pcp:700kB local_pcp:0kB free_cma:0kB
[ 25.807087] lowmem_reserve[]: 0 0 0 0 0
[ 25.807456] Node 0 DMA: 2*4kB (U) 2*8kB (U) 0*16kB 1*32kB (U) 1*64kB (U) 1*128kB (U) 1*256kB (U) 0*512kB 1*1024kB (U) 1*2048kB (M) 3*4096kB (M) = 15864kB
[ 25.808777] Node 0 DMA32: 2413*4kB (UME) 1622*8kB (UME) 1197*16kB (UME) 935*32kB (UME) 661*64kB (UME) 268*128kB (UME) 107*256kB (UM) 46*512kB (UME) 18*1024kB (UME) 7*2048kB (M) 143*4096kB (M) = 817748kB
[ 25.810517] Node 0 hugepages_total=0 hugepages_free=0 hugepages_surp=0 hugepages_size=2048kB
[ 25.810868] oom_reaper: reaped process 492 (allocate), now anon-rss:0kB, file-rss:0kB, shmem-rss:0kB
[ 25.812656] 290 total pagecache pages
[ 25.813043] 0 pages in swap cache
[ 25.813398] Swap cache stats: add 0, delete 0, find 0/0
[ 25.813756] Free swap = 0kB
[ 25.813966] Total swap = 0kB
[ 25.814249] 524158 pages RAM
[ 25.814547] 0 pages HighMem/MovableOnly
[ 25.814957] 12188 pages reserved
[ 25.815791] 0 pages cma reserved
[ 25.816993] 0 pages hwpoisoned
[ 25.817589] allocate invoked oom-killer: gfp_mask=0x0(), nodemask=(null), order=0, oom_score_adj=0
[ 25.818821] allocate cpuset=/ mems_allowed=0
[ 25.819259] CPU: 1 PID: 492 Comm: allocate Not tainted 4.12.0-rc1-mm1+ #181
[ 25.819847] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[ 25.820549] Call Trace:
[ 25.820733] dump_stack+0x63/0x82
[ 25.820961] dump_header+0x97/0x21a
[ 25.820961] ? security_capable_noaudit+0x45/0x60
[ 25.820961] oom_kill_process+0x219/0x3e0
[ 25.820961] out_of_memory+0x11d/0x480
[ 25.820961] pagefault_out_of_memory+0x68/0x80
[ 25.820961] mm_fault_error+0x8f/0x190
[ 25.820961] ? handle_mm_fault+0xf3/0x210
[ 25.820961] __do_page_fault+0x4b2/0x4e0
[ 25.820961] trace_do_page_fault+0x37/0xe0
[ 25.820961] do_async_page_fault+0x19/0x70
[ 25.820961] async_page_fault+0x28/0x30
[ 25.820961] RIP: 0033:0x400760
[ 25.820961] RSP: 002b:00007ffe30dd9970 EFLAGS: 00010287
[ 25.820961] RAX: 00007fd9bd760010 RBX: 0000000070800000 RCX: 0000000000000006
[ 25.820961] RDX: 00007fd9c6d2a010 RSI: 000000000c801000 RDI: 0000000000000000
[ 25.820961] RBP: 000000000c800000 R08: ffffffffffffffff R09: 0000000000000000
[ 25.820961] R10: 00007fd9ba52a010 R11: 0000000000000246 R12: 00000000004007b0
[ 25.820961] R13: 00007ffe30dd9a60 R14: 0000000000000000 R15: 0000000000000000
[ 25.827189] Mem-Info:
[ 25.827440] active_anon:14317 inactive_anon:51 isolated_anon:0
[ 25.827440] active_file:28 inactive_file:468 isolated_file:0
[ 25.827440] unevictable:0 dirty:12 writeback:1 unstable:0
[ 25.827440] slab_reclaimable:2559 slab_unreclaimable:2398
[ 25.827440] mapped:274 shmem:123 pagetables:902 bounce:0
[ 25.827440] free:487556 free_pcp:19 free_cma:0
[ 25.829867] Node 0 active_anon:57268kB inactive_anon:204kB active_file:112kB inactive_file:2040kB unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:1180kB dirty:48kB writeback:4kB shmem:492kB shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 12288kB writeback_tmp:0kB unstable:0kB all_unreclaimable? yes
[ 25.832174] Node 0 DMA free:15864kB min:348kB low:432kB high:516kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB present:15992kB managed:15908kB mlocked:0kB slab_reclaimable:0kB slab_unreclaimable:8kB kernel_stack:0kB pagetables:4kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
[ 25.835358] lowmem_reserve[]: 0 1981 1981 1981 1981
[ 25.835784] Node 0 DMA32 free:1934360kB min:44704kB low:55880kB high:67056kB active_anon:57104kB inactive_anon:204kB active_file:416kB inactive_file:2476kB unevictable:0kB writepending:424kB present:2080640kB managed:2031972kB mlocked:0kB slab_reclaimable:10236kB slab_unreclaimable:9584kB kernel_stack:1776kB pagetables:3604kB bounce:0kB free_pcp:144kB local_pcp:0kB free_cma:0kB
[ 25.838014] lowmem_reserve[]: 0 0 0 0 0
[ 25.838365] Node 0 DMA: 2*4kB (U) 2*8kB (U) 0*16kB 1*32kB (U) 1*64kB (U) 1*128kB (U) 1*256kB (U) 0*512kB 1*1024kB (U) 1*2048kB (M) 3*4096kB (M) = 15864kB
[ 25.839339] Node 0 DMA32: 1708*4kB (UME) 1431*8kB (UME) 1005*16kB (UME) 735*32kB (UME) 502*64kB (UME) 236*128kB (UME) 114*256kB (UM) 45*512kB (UME) 20*1024kB (UME) 10*2048kB (M) 420*4096kB (M) = 1933720kB
[ 25.840727] Node 0 hugepages_total=0 hugepages_free=0 hugepages_surp=0 hugepages_size=2048kB
[ 25.841463] 938 total pagecache pages
[ 25.841800] 0 pages in swap cache
[ 25.842110] Swap cache stats: add 0, delete 0, find 0/0
[ 25.842613] Free swap = 0kB
[ 25.842936] Total swap = 0kB
[ 25.843206] 524158 pages RAM
[ 25.843542] 0 pages HighMem/MovableOnly
[ 25.843949] 12188 pages reserved
[ 25.844248] 0 pages cma reserved
[ 25.844522] 0 pages hwpoisoned
[ 25.844732] [ pid ] uid tgid total_vm rss nr_ptes nr_pmds swapents oom_score_adj name
[ 25.845390] [ 121] 0 121 25672 473 50 3 0 0 systemd-journal
[ 25.846511] [ 156] 0 156 11157 197 22 3 0 -1000 systemd-udevd
[ 25.847471] [ 206] 0 206 13896 99 29 3 0 -1000 auditd
[ 25.848224] [ 227] 0 227 11874 124 27 3 0 0 systemd-logind
[ 25.850604] [ 229] 81 229 11577 146 28 3 0 -900 dbus-daemon
[ 25.852160] [ 231] 997 231 27502 102 25 3 0 0 chronyd
[ 25.852875] [ 233] 0 233 61519 5239 85 3 0 0 firewalld
[ 25.853578] [ 238] 0 238 123495 529 74 4 0 0 NetworkManager
[ 25.854327] [ 265] 0 265 25117 231 52 3 0 -1000 sshd
[ 25.855001] [ 271] 0 271 6092 154 17 3 0 0 crond
[ 25.855683] [ 277] 0 277 11297 93 26 3 0 0 systemd-hostnam
[ 25.856429] [ 284] 0 284 1716 29 9 3 0 0 agetty
[ 25.857130] [ 285] 0 285 2030 34 9 4 0 0 agetty
[ 25.857798] [ 302] 998 302 133102 2578 58 3 0 0 polkitd
[ 25.858500] [ 394] 0 394 21785 3076 45 3 0 0 dhclient
[ 25.859162] [ 444] 0 444 36717 312 74 3 0 0 sshd
[ 25.859803] [ 446] 0 446 15966 223 36 3 0 0 systemd
[ 25.860456] [ 447] 0 447 23459 384 47 3 0 0 (sd-pam)
[ 25.861101] [ 451] 0 451 36717 316 72 3 0 0 sshd
[ 25.861746] [ 452] 0 452 3611 315 11 3 0 0 bash
[ 25.862456] [ 492] 0 492 513092 0 97 5 0 0 allocate
[ 25.863078] Out of memory: Kill process 233 (firewalld) score 10 or sacrifice child
[ 25.863634] Killed process 233 (firewalld) total-vm:246076kB, anon-rss:20956kB, file-rss:0kB, shmem-rss:0kB
--------------------------------------------------------------------------------
>
> > <cut>
> > [ 25.817589] allocate invoked oom-killer: gfp_mask=0x0(), nodemask=(null), order=0, oom_score_adj=0
> > [ 25.818821] allocate cpuset=/ mems_allowed=0
> > [ 25.819259] CPU: 1 PID: 492 Comm: allocate Not tainted 4.12.0-rc1-mm1+ #181
> > [ 25.819847] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
> > [ 25.820549] Call Trace:
> > [ 25.820733] dump_stack+0x63/0x82
> > [ 25.820961] dump_header+0x97/0x21a
> > [ 25.820961] ? security_capable_noaudit+0x45/0x60
> > [ 25.820961] oom_kill_process+0x219/0x3e0
> > [ 25.820961] out_of_memory+0x11d/0x480
>
> This is interesting. OOM usually happens from the page allocator path.
> Hitting it from here means that somebody has returned VM_FAULT_OOM. Who
> was that and is there any preceeding OOM before?
It looks to me that one pagefault is causing two OOMs.
One is called from page allocation path, second from
pagefault_out_of_memory().
>
> > [ 25.820961] pagefault_out_of_memory+0x68/0x80
> > [ 25.820961] mm_fault_error+0x8f/0x190
> > [ 25.820961] ? handle_mm_fault+0xf3/0x210
> > [ 25.820961] __do_page_fault+0x4b2/0x4e0
> > [ 25.820961] trace_do_page_fault+0x37/0xe0
> > [ 25.820961] do_async_page_fault+0x19/0x70
> > [ 25.820961] async_page_fault+0x28/0x30
> > <cut>
> > [ 25.863078] Out of memory: Kill process 233 (firewalld) score 10 or sacrifice child
> > [ 25.863634] Killed process 233 (firewalld) total-vm:246076kB, anon-rss:20956kB, file-rss:0kB, shmem-rss:0kB
> >
> > After some investigations I've found some issues:
> >
> > 1) Prior to commit 1af8bb432695 ("mm, oom: fortify task_will_free_mem()"),
> > if a process with a pending SIGKILL was calling out_of_memory(),
> > it was always immediately selected as a victim.
>
> Yes but this had its own issues. Mainly picking the same victim again
> without making a further progress.
That is why I've added this check into the pagefault_out_of_memory(),
rather than out_of_memory(), where it was earlier.
>
> > But now, after some changes, it's not always a case.
> > If a process has been reaped at the moment, MMF_SKIP_FLAG is set,
> > task_will_free_mem() will return false, and a new
> > victim selection logic will be started.
>
> right. The point is that it doesn't make any sense to consider such a
> task because it either cannot be reaped or it has been reaped and there
> is not much left to consider. It would be interesting to see what
> happened in your case.
>
> > This actually happens if a userspace pagefault causing an OOM.
> > pagefault_out_of_memory() is called in a context of a faulting
> > process after it has been selected as OOM victim (assuming, it
> > has), and killed. With some probability (there is a race with
> > oom_reaper thread) this process will be passed to the oom reaper
> > again, or an innocent victim will be selected and killed.
> >
> > 2) We clear up the task->oom_reaper_list before setting
> > the MMF_OOM_SKIP flag, so there is a race.
>
> I am not sure what you mean here. Why would a race matter?
oom_reaper_list pointer is zeroed before MMF_OOM_SKIP flag is set.
Inbetween this process can be selected again and added to the
oom reaper queue. It's not a big issue, still.
>
> >
> > 3) We skip the MMF_OOM_SKIP flag check in case of
> > an sysrq-triggered OOM.
>
> yes because we we always want to pick a new victim when sysrq is
> invoked.
>
> > To address these issues, the following is proposed:
> > 1) If task is already an oom victim, skip out_of_memory() call
> > from the pagefault_out_of_memory().
>
> Hmm, this alone doesn't look all that bad. It would be better to simply
> let the task die than go over the oom handling. But I am still not sure
> what is going on in your case so I do not see how could this help.
>
> > 2) Set the MMF_OOM_SKIP bit in wake_oom_reaper() before adding a
> > process to the oom_reaper list. If it's already set, do nothing.
> > Do not rely on tsk->oom_reaper_list value.
>
> This is wrong. The sole purpose of MMF_OOM_SKIP is to let the oom
> selection logic know that this task is not interesting anymore. Setting
> it in wake_oom_reaper means it would be set _before_ the oom_reaper had
> any chance to free any memory from the task. So we would
But if have selected a task once, it has no way back.
Anyway it will be reaped or will quit by itself soon. Right?
So, under no circumstances we should consider choosing them
as an OOM victim again.
There are no reasons to calculate it's badness again, etc.
>
> > 3) Check the MMF_OOM_SKIP even if OOM is triggered by a sysrq.
>
> The code is a bit messy here but we do check MMF_OOM_SKIP in that case.
> We just do it in oom_badness(). So this is not needed, strictly
> speaking.
>
> That being said I would like to here more about the cause of the OOM and
> the full dmesg would be interesting. The proposed setting of
> MMF_OOM_SKIP before the task is reaped is a nogo, though.
If so, how you will prevent putting a process again into the reaper list,
if it's already reaped?
> 1) would be
> acceptable I think but I would have to think about it some more.
Actually, the first problem is much more serious, as it leads
to a killing of second process.
The second one can lead only to a unnecessary wake up of
the oom reaper thread, which is not great, but acceptable.
Thank you!
Roman
--
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 summary] Enable Coherent Device Memory
From: Benjamin Herrenschmidt @ 2017-05-17 19:39 UTC (permalink / raw)
To: Christoph Lameter
Cc: Mel Gorman, Balbir Singh, linux-mm, akpm@linux-foundation.org,
Anshuman Khandual, Aneesh Kumar KV, Paul E. McKenney,
Srikar Dronamraju, Haren Myneni, Jérôme Glisse,
Reza Arbab, Vlastimil Babka, Rik van Riel
In-Reply-To: <alpine.DEB.2.20.1705170853470.7925@east.gentwo.org>
On Wed, 2017-05-17 at 08:54 -0500, Christoph Lameter wrote:
> You can provide a library that does it?
>
> > The base idea behind the counters we have on the link is for the HW to
> > know when memory is accessed "remotely", so that the device driver can
> > make decision about migrating pages into or away from the device,
> > especially so that applications don't have to concern themselves with
> > memory placement.
>
> Library can enquire about the current placement of the pages and move them
> if necessary?
No, doing that from a library would not work. It should be done by the
driver, but that's not a problem in the proposed scheme and doesn't
require new MM hooks afaik so I don't think there's a debate here.
>From my understanding, the main discussion revolves around isolation,
ie, whether to change the NUMA core to add nodes on which no allocation
will take place by default or not.
Ben.
--
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 summary] Enable Coherent Device Memory
From: Benjamin Herrenschmidt @ 2017-05-17 19:37 UTC (permalink / raw)
To: Mel Gorman
Cc: Balbir Singh, linux-mm, akpm@linux-foundation.org,
Anshuman Khandual, Aneesh Kumar KV, Paul E. McKenney,
Srikar Dronamraju, Haren Myneni, Jérôme Glisse,
Reza Arbab, Vlastimil Babka, Christoph Lameter, Rik van Riel
In-Reply-To: <20170517105812.plj54qwbr334w5r5@techsingularity.net>
On Wed, 2017-05-17 at 11:58 +0100, Mel Gorman wrote:
> The race is a non-issue unless for some reason you decide to hot-add the node
> when the machine is already heavily loaded and under memory pressure. Do it
> near boot time and no CPU-local allocation is going to hit it. In itself,
> special casing the core VM is overkill.
>
> If you decide to use ZONE_MOVABLE and take the remote hit penalty of page
> tables, then you can also migrate all the pages away after the onlining
> and isolation is complete if it's a serious concern in practice.
>
> > Unless we have a way to create a node without actually making it
> > available for allocations, so we get a chance to establish policies for
> > it, then "online" it ?
> >
>
> Conceivably, that could be done although again it's somewhat overkill
> as the race only applies if hot-adding CDM under heavy memory pressure
> sufficient to overflow to a very remote node.
I wouldn't dismiss the problem that readily. It might by ok for our
initial customer needs but long run, there's a lot of demand for SR-IOV
GPUs and pass-through.
It's not far fetched to have GPU being dynamically added/removed from
partitions based on usage, which means possibly under significant
pressure.
That said, this can be solved later if needed.
Cheers,
Ben.
--
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 summary] Enable Coherent Device Memory
From: Benjamin Herrenschmidt @ 2017-05-17 19:35 UTC (permalink / raw)
To: Mel Gorman
Cc: Balbir Singh, linux-mm, akpm@linux-foundation.org,
Anshuman Khandual, Aneesh Kumar KV, Paul E. McKenney,
Srikar Dronamraju, Haren Myneni, Jérôme Glisse,
Reza Arbab, Vlastimil Babka, Christoph Lameter, Rik van Riel
In-Reply-To: <20170517105812.plj54qwbr334w5r5@techsingularity.net>
On Wed, 2017-05-17 at 11:58 +0100, Mel Gorman wrote:
> Remember that this will include the page table pages which may or may
> not be what you want.
It is fine. The GPU does translation using ATS, so the page tables are
effectively accessed by the nest MMU in the corresponding P9 chip, not
by the GPU itself. Thus we do want them to reside in system memory.
Cheers,
Ben.
--
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 07/17] cgroup: Prevent kill_css() from being called more than once
From: Tejun Heo @ 2017-05-17 19:23 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-8-git-send-email-longman@redhat.com>
Hello,
On Mon, May 15, 2017 at 09:34:06AM -0400, Waiman Long wrote:
> The kill_css() function may be called more than once under the condition
> that the css was killed but not physically removed yet followed by the
> removal of the cgroup that is hosting the css. This patch prevents any
> harmm from being done when that happens.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
So, this is a bug fix which isn't really related to this patchset.
I'm applying it to cgroup/for-4.12-fixes w/ stable cc'd.
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 06/17] cgroup: Fix reference counting bug in cgroup_procs_write()
From: Tejun Heo @ 2017-05-17 19:20 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-7-git-send-email-longman@redhat.com>
On Mon, May 15, 2017 at 09:34:05AM -0400, Waiman Long wrote:
> The cgroup_procs_write_start() took a reference to the task structure
> which was not properly released within cgroup_procs_write() and so
> on. So a put_task_struct() call is added to cgroup_procs_write_finish()
> to match the get_task_struct() in cgroup_procs_write_start() to fix
> this reference counting error.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
Acked-by: Tejun Heo <tj@kernel.org>
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
* [PATCH v2] PATCH [PATCH 001] memory management: spelling and grammar
From: Michael DeGuzis @ 2017-05-17 19:19 UTC (permalink / raw)
To: linux-mm; +Cc: trivial, Michael DeGuzis
This patch fixes up some grammar and spelling in the
information block for huge_memory.c.
* Fix grammary/spelling in mm/huge_memory.c
Signed-off-by: Michael DeGuzis <mdeguzis@gmail.com>
---
mm/huge_memory.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a84909cf20d3..b75b8f08eb86 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -38,10 +38,10 @@
#include "internal.h"
/*
- * By default transparent hugepage support is disabled in order that avoid
- * to risk increase the memory footprint of applications without a guaranteed
- * benefit. When transparent hugepage support is enabled, is for all mappings,
- * and khugepaged scans all mappings.
+ * By default, transparent hugepage support is disabled in order to avoid
+ * risking an increased memory footprint for applications that are not
+ * guaranteed to benefit from it. When transparent hugepage support is
+ * enabled, it is for all mappings, and khugepaged scans all mappings.
* Defrag is invoked by khugepaged hugepage allocations and by page faults
* for all hugepage allocations.
*/
--
2.12.2
--
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 v5 28/32] x86/mm, kexec: Allow kexec to be used with SME
From: Borislav Petkov @ 2017-05-17 19:17 UTC (permalink / raw)
To: Tom Lendacky
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: <20170418212121.10190.94885.stgit@tlendack-t1.amdoffice.net>
On Tue, Apr 18, 2017 at 04:21:21PM -0500, Tom Lendacky wrote:
> Provide support so that kexec can be used to boot a kernel when SME is
> enabled.
>
> Support is needed to allocate pages for kexec without encryption. This
> is needed in order to be able to reboot in the kernel in the same manner
> as originally booted.
>
> Additionally, when shutting down all of the CPUs we need to be sure to
> flush the caches and then halt. This is needed when booting from a state
> where SME was not active into a state where SME is active (or vice-versa).
> Without these steps, it is possible for cache lines to exist for the same
> physical location but tagged both with and without the encryption bit. This
> can cause random memory corruption when caches are flushed depending on
> which cacheline is written last.
>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> arch/x86/include/asm/init.h | 1 +
> arch/x86/include/asm/irqflags.h | 5 +++++
> arch/x86/include/asm/kexec.h | 8 ++++++++
> arch/x86/include/asm/pgtable_types.h | 1 +
> arch/x86/kernel/machine_kexec_64.c | 35 +++++++++++++++++++++++++++++++++-
> arch/x86/kernel/process.c | 26 +++++++++++++++++++++++--
> arch/x86/mm/ident_map.c | 11 +++++++----
> include/linux/kexec.h | 14 ++++++++++++++
> kernel/kexec_core.c | 7 +++++++
> 9 files changed, 101 insertions(+), 7 deletions(-)
...
> @@ -86,7 +86,7 @@ static int init_transition_pgtable(struct kimage *image, pgd_t *pgd)
> set_pmd(pmd, __pmd(__pa(pte) | _KERNPG_TABLE));
> }
> pte = pte_offset_kernel(pmd, vaddr);
> - set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, PAGE_KERNEL_EXEC));
> + set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, PAGE_KERNEL_EXEC_NOENC));
> return 0;
> err:
> free_transition_pgtable(image);
> @@ -114,6 +114,7 @@ static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
> .alloc_pgt_page = alloc_pgt_page,
> .context = image,
> .pmd_flag = __PAGE_KERNEL_LARGE_EXEC,
> + .kernpg_flag = _KERNPG_TABLE_NOENC,
> };
> unsigned long mstart, mend;
> pgd_t *level4p;
> @@ -597,3 +598,35 @@ void arch_kexec_unprotect_crashkres(void)
> {
> kexec_mark_crashkres(false);
> }
> +
> +int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, gfp_t gfp)
> +{
> + int ret;
> +
> + if (sme_active()) {
if (!sme_active())
return 0;
/*
* If SME...
> + /*
> + * If SME is active we need to be sure that kexec pages are
> + * not encrypted because when we boot to the new kernel the
> + * pages won't be accessed encrypted (initially).
> + */
> + ret = set_memory_decrypted((unsigned long)vaddr, pages);
> + if (ret)
> + return ret;
> +
> + if (gfp & __GFP_ZERO)
> + memset(vaddr, 0, pages * PAGE_SIZE);
This function is called after alloc_pages() which already zeroes memory
when __GFP_ZERO is supplied.
If you need to clear the memory *after* set_memory_encrypted() happens,
then you should probably mask out __GFP_ZERO before the alloc_pages()
call so as not to do it twice.
> + }
> +
> + return 0;
> +}
> +
> +void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages)
> +{
> + if (sme_active()) {
> + /*
> + * If SME is active we need to reset the pages back to being
> + * an encrypted mapping before freeing them.
> + */
> + set_memory_encrypted((unsigned long)vaddr, pages);
> + }
> +}
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 0bb8842..f4e5de6 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -24,6 +24,7 @@
> #include <linux/cpuidle.h>
> #include <trace/events/power.h>
> #include <linux/hw_breakpoint.h>
> +#include <linux/kexec.h>
> #include <asm/cpu.h>
> #include <asm/apic.h>
> #include <asm/syscalls.h>
> @@ -355,8 +356,25 @@ bool xen_set_default_idle(void)
> return ret;
> }
> #endif
> +
> void stop_this_cpu(void *dummy)
> {
> + bool do_wbinvd_halt = false;
> +
> + if (kexec_in_progress && boot_cpu_has(X86_FEATURE_SME)) {
> + /*
> + * If we are performing a kexec and the processor supports
> + * SME then we need to clear out cache information before
> + * halting. With kexec, going from SME inactive to SME active
> + * requires clearing cache entries so that addresses without
> + * the encryption bit set don't corrupt the same physical
> + * address that has the encryption bit set when caches are
> + * flushed. Perform a wbinvd followed by a halt to achieve
> + * this.
> + */
> + do_wbinvd_halt = true;
> + }
> +
> local_irq_disable();
> /*
> * Remove this CPU:
> @@ -365,8 +383,12 @@ void stop_this_cpu(void *dummy)
> disable_local_APIC();
> mcheck_cpu_clear(this_cpu_ptr(&cpu_info));
>
> - for (;;)
> - halt();
> + for (;;) {
> + if (do_wbinvd_halt)
> + native_wbinvd_halt();
No need for that native_wbinvd_halt() thing:
for (;;) {
if (do_wbinvd)
wbinvd();
halt();
}
> /*
> diff --git a/arch/x86/mm/ident_map.c b/arch/x86/mm/ident_map.c
> index 04210a2..2c9fd3e 100644
> --- a/arch/x86/mm/ident_map.c
> +++ b/arch/x86/mm/ident_map.c
> @@ -20,6 +20,7 @@ static void ident_pmd_init(struct x86_mapping_info *info, pmd_t *pmd_page,
> static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page,
> unsigned long addr, unsigned long end)
> {
> + unsigned long kernpg_flag = info->kernpg_flag ? : _KERNPG_TABLE;
You're already supplying a x86_mapping_info and thus you can init
kernpg_flag to default _KERNPG_TABLE and override it in the SME+kexec
case, as you already do. And this way you can simply do:
set_pud(pud, __pud(__pa(pmd) | info->kernpg_flag));
here and in the other pagetable functions I've snipped below, and save
yourself some lines.
...
--
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
* Re: [PATCH v5 17/32] x86/mm: Add support to access boot related data in the clear
From: Tom Lendacky @ 2017-05-17 18: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: <20170515183517.mb4k2gp2qobbuvtm@pd.tnic>
On 5/15/2017 1:35 PM, Borislav Petkov wrote:
> On Tue, Apr 18, 2017 at 04:19:21PM -0500, Tom Lendacky wrote:
>> Boot data (such as EFI related data) is not encrypted when the system is
>> booted because UEFI/BIOS does not run with SME active. In order to access
>> this data properly it needs to be mapped decrypted.
>>
>> The early_memremap() support is updated to provide an arch specific
>
> "Update early_memremap() to provide... "
Will do.
>
>> routine to modify the pagetable protection attributes before they are
>> applied to the new mapping. This is used to remove the encryption mask
>> for boot related data.
>>
>> The memremap() support is updated to provide an arch specific routine
>
> Ditto. Passive tone always reads harder than an active tone,
> "doer"-sentence.
Ditto.
>
>> to determine if RAM remapping is allowed. RAM remapping will cause an
>> encrypted mapping to be generated. By preventing RAM remapping,
>> ioremap_cache() will be used instead, which will provide a decrypted
>> mapping of the boot related data.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> arch/x86/include/asm/io.h | 4 +
>> arch/x86/mm/ioremap.c | 182 +++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/io.h | 2
>> kernel/memremap.c | 20 ++++-
>> mm/early_ioremap.c | 18 ++++
>> 5 files changed, 219 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>> index 7afb0e2..75f2858 100644
>> --- a/arch/x86/include/asm/io.h
>> +++ b/arch/x86/include/asm/io.h
>> @@ -381,4 +381,8 @@ extern int __must_check arch_phys_wc_add(unsigned long base,
>> #define arch_io_reserve_memtype_wc arch_io_reserve_memtype_wc
>> #endif
>>
>> +extern bool arch_memremap_do_ram_remap(resource_size_t offset, size_t size,
>> + unsigned long flags);
>> +#define arch_memremap_do_ram_remap arch_memremap_do_ram_remap
>> +
>> #endif /* _ASM_X86_IO_H */
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 9bfcb1f..bce0604 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -13,6 +13,7 @@
>> #include <linux/slab.h>
>> #include <linux/vmalloc.h>
>> #include <linux/mmiotrace.h>
>> +#include <linux/efi.h>
>>
>> #include <asm/cacheflush.h>
>> #include <asm/e820/api.h>
>> @@ -21,6 +22,7 @@
>> #include <asm/tlbflush.h>
>> #include <asm/pgalloc.h>
>> #include <asm/pat.h>
>> +#include <asm/setup.h>
>>
>> #include "physaddr.h"
>>
>> @@ -419,6 +421,186 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
>> iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
>> }
>>
>> +/*
>> + * 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.
>> + */
>> +static bool memremap_should_map_decrypted(resource_size_t phys_addr,
>> + unsigned long size)
>> +{
>> + /* 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:
>> + return true;
>> + default:
>> + break;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +/*
>> + * Examine the physical address to determine if it is EFI data. Check
>> + * it against the boot params structure and EFI tables and memory types.
>> + */
>> +static bool memremap_is_efi_data(resource_size_t phys_addr,
>> + unsigned long size)
>> +{
>> + u64 paddr;
>> +
>> + /* Check if the address is part of EFI boot/runtime data */
>> + if (efi_enabled(EFI_BOOT)) {
>
> Save indentation level:
>
> if (!efi_enabled(EFI_BOOT))
> return false;
>
I was worried what the compiler might do when CONFIG_EFI is not set,
but it appears to take care of it. I'll double check though.
>
>> + paddr = boot_params.efi_info.efi_memmap_hi;
>> + paddr <<= 32;
>> + paddr |= boot_params.efi_info.efi_memmap;
>> + if (phys_addr == paddr)
>> + return true;
>> +
>> + paddr = boot_params.efi_info.efi_systab_hi;
>> + paddr <<= 32;
>> + paddr |= boot_params.efi_info.efi_systab;
>
> So those two above look like could be two global vars which are
> initialized somewhere in the EFI init path:
>
> efi_memmap_phys and efi_systab_phys or so.
>
> Matt ?
>
> And then you won't need to create that paddr each time on the fly. I
> mean, it's not a lot of instructions but still...
>
>> + if (phys_addr == paddr)
>> + return true;
>> +
>> + if (efi_table_address_match(phys_addr))
>> + return true;
>> +
>> + switch (efi_mem_type(phys_addr)) {
>> + case EFI_BOOT_SERVICES_DATA:
>> + case EFI_RUNTIME_SERVICES_DATA:
>> + return true;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> + return false;
>> +}
>> +
>> +/*
>> + * Examine the physical address to determine if it is boot data by checking
>> + * it against the boot params setup_data chain.
>> + */
>> +static bool memremap_is_setup_data(resource_size_t phys_addr,
>> + unsigned long size)
>> +{
>> + struct setup_data *data;
>> + u64 paddr, paddr_next;
>> +
>> + paddr = boot_params.hdr.setup_data;
>> + while (paddr) {
>> + bool is_setup_data = false;
>
> You don't need that bool:
>
> static bool memremap_is_setup_data(resource_size_t phys_addr,
> unsigned long size)
> {
> struct setup_data *data;
> u64 paddr, paddr_next;
>
> paddr = boot_params.hdr.setup_data;
> while (paddr) {
> if (phys_addr == paddr)
> return true;
>
> data = memremap(paddr, sizeof(*data), MEMREMAP_WB | MEMREMAP_DEC);
>
> paddr_next = data->next;
>
> if ((phys_addr > paddr) && (phys_addr < (paddr + data->len))) {
> memunmap(data);
> return true;
> }
>
> memunmap(data);
>
> paddr = paddr_next;
> }
> return false;
> }
>
> Flow is a bit clearer.
I may introduce a length variable to capture data->len right after
paddr_next is set and then have just a single memunmap() call before
the if check.
>
>> +/*
>> + * Examine the physical address to determine if it is boot data by checking
>> + * it against the boot params setup_data chain (early boot version).
>> + */
>> +static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
>> + unsigned long size)
>> +{
>> + struct setup_data *data;
>> + u64 paddr, paddr_next;
>> +
>> + paddr = boot_params.hdr.setup_data;
>> + while (paddr) {
>> + bool is_setup_data = false;
>> +
>> + if (phys_addr == paddr)
>> + return true;
>> +
>> + data = early_memremap_decrypted(paddr, sizeof(*data));
>> +
>> + paddr_next = data->next;
>> +
>> + if ((phys_addr > paddr) && (phys_addr < (paddr + data->len)))
>> + is_setup_data = true;
>> +
>> + early_memunmap(data, sizeof(*data));
>> +
>> + if (is_setup_data)
>> + return true;
>> +
>> + paddr = paddr_next;
>> + }
>> +
>> + return false;
>> +}
>
> This one is begging to be unified with memremap_is_setup_data() to both
> call a __ worker function.
I tried that, but calling an "__init" function (early_memremap()) from
a non "__init" function generated warnings. I suppose I can pass in a
function for the map and unmap but that looks worse to me (also the
unmap functions take different arguments).
>
>> +
>> +/*
>> + * Architecture function to determine if RAM remap is allowed. By default, a
>> + * RAM remap will map the data as encrypted. Determine if a RAM remap should
>> + * not be done so that the data will be mapped decrypted.
>> + */
>> +bool arch_memremap_do_ram_remap(resource_size_t phys_addr, unsigned long size,
>> + unsigned long flags)
>
> So this function doesn't do anything - it replies to a yes/no question.
> So the name should not say "do" but sound like a question. Maybe:
>
> if (arch_memremap_can_remap( ... ))
>
> or so...
Ok, I'll change that.
>
>> +{
>> + if (!sme_active())
>> + return true;
>> +
>> + if (flags & MEMREMAP_ENC)
>> + return true;
>> +
>> + if (flags & MEMREMAP_DEC)
>> + return false;
>
> So this looks strange to me: both flags MEMREMAP_ENC and _DEC override
> setup and efi data checking. But we want to remap setup and EFI data
> *always* decrypted because that data was not encrypted as, as you say,
> firmware doesn't run with SME active.
>
> So my simple logic says that EFI stuff should *always* be mapped DEC,
> regardless of flags. Ditto for setup data. So that check below should
> actually *override* the flags checks and go before them, no?
This is like the chicken and the egg scenario. In order to determine if
an address is setup data I have to explicitly map the setup data chain
as decrypted. In order to do that I have to supply a flag to explicitly
map the data decrypted otherwise I wind up back in the
memremap_is_setup_data() function again and again and again...
>
>> +
>> + if (memremap_is_setup_data(phys_addr, size) ||
>> + memremap_is_efi_data(phys_addr, size) ||
>> + memremap_should_map_decrypted(phys_addr, size))
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> +/*
>> + * Architecture override of __weak function to adjust the protection attributes
>> + * used when remapping memory. By default, early_memremp() will map the data
>
> early_memremAp() - a is missing.
Got it.
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 1/2] mm: avoid spurious 'bad pmd' warning messages
From: Ross Zwisler @ 2017-05-17 18:23 UTC (permalink / raw)
To: Dave Hansen
Cc: Ross Zwisler, Andrew Morton, linux-kernel, Darrick J. Wong,
Alexander Viro, Christoph Hellwig, Dan Williams, Jan Kara,
Matthew Wilcox, linux-fsdevel, linux-mm, linux-nvdimm,
Kirill A . Shutemov, Pawel Lebioda, Dave Jiang, Xiong Zhou,
Eryu Guan, stable
In-Reply-To: <9c45c769-2f5e-9327-c39e-1df7744fa633@intel.com>
On Wed, May 17, 2017 at 10:33:58AM -0700, Dave Hansen wrote:
> On 05/17/2017 10:16 AM, Ross Zwisler wrote:
> > @@ -3061,7 +3061,7 @@ static int pte_alloc_one_map(struct vm_fault *vmf)
> > * through an atomic read in C, which is what pmd_trans_unstable()
> > * provides.
> > */
> > - if (pmd_trans_unstable(vmf->pmd) || pmd_devmap(*vmf->pmd))
> > + if (pmd_devmap(*vmf->pmd) || pmd_trans_unstable(vmf->pmd))
> > return VM_FAULT_NOPAGE;
>
> I'm worried we are very unlikely to get this right in the future. It's
> totally not obvious what the ordering requirement is here.
>
> Could we move pmd_devmap() and pmd_trans_unstable() into a helper that
> gets the ordering right and also spells out the ordering requirement?
Sure, I'll fix this for v2.
Thanks for the review.
--
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 1/2] mm: avoid spurious 'bad pmd' warning messages
From: Dave Hansen @ 2017-05-17 17:33 UTC (permalink / raw)
To: Ross Zwisler, Andrew Morton, linux-kernel
Cc: Darrick J. Wong, Alexander Viro, Christoph Hellwig, Dan Williams,
Jan Kara, Matthew Wilcox, linux-fsdevel, linux-mm, linux-nvdimm,
Kirill A . Shutemov, Pawel Lebioda, Dave Jiang, Xiong Zhou,
Eryu Guan, stable
In-Reply-To: <20170517171639.14501-1-ross.zwisler@linux.intel.com>
On 05/17/2017 10:16 AM, Ross Zwisler wrote:
> @@ -3061,7 +3061,7 @@ static int pte_alloc_one_map(struct vm_fault *vmf)
> * through an atomic read in C, which is what pmd_trans_unstable()
> * provides.
> */
> - if (pmd_trans_unstable(vmf->pmd) || pmd_devmap(*vmf->pmd))
> + if (pmd_devmap(*vmf->pmd) || pmd_trans_unstable(vmf->pmd))
> return VM_FAULT_NOPAGE;
I'm worried we are very unlikely to get this right in the future. It's
totally not obvious what the ordering requirement is here.
Could we move pmd_devmap() and pmd_trans_unstable() into a helper that
gets the ordering right and also spells out the ordering requirement?
--
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