Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932157AbbBTSCq (ORCPT ); Fri, 20 Feb 2015 13:02:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43043 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932089AbbBTSCj (ORCPT ); Fri, 20 Feb 2015 13:02:39 -0500 Date: Fri, 20 Feb 2015 19:02:18 +0100 From: Andrea Arcangeli To: Andrew Morton Cc: Ebru Akagunduz , linux-mm@kvack.org, kirill@shutemov.name, mhocko@suse.cz, mgorman@suse.de, rientjes@google.com, sasha.levin@oracle.com, hughd@google.com, hannes@cmpxchg.org, vbabka@suse.cz, linux-kernel@vger.kernel.org, riel@redhat.com Subject: Re: [PATCH v2] mm: incorporate zero pages into transparent huge pages Message-ID: <20150220180218.GA4285@redhat.com> References: <1423688635-4306-1-git-send-email-ebru.akagunduz@gmail.com> <20150218153119.0bcd0bf8b4e7d30d99f00a3b@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150218153119.0bcd0bf8b4e7d30d99f00a3b@linux-foundation.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6421 Lines: 144 On Wed, Feb 18, 2015 at 03:31:19PM -0800, Andrew Morton wrote: > On Wed, 11 Feb 2015 23:03:55 +0200 Ebru Akagunduz wrote: > > > This patch improves THP collapse rates, by allowing zero pages. > > > > Currently THP can collapse 4kB pages into a THP when there > > are up to khugepaged_max_ptes_none pte_none ptes in a 2MB > > range. This patch counts pte none and mapped zero pages > > with the same variable. > > So if I'm understanding this correctly, with the default value of > khugepaged_max_ptes_none (HPAGE_PMD_NR-1), if an application creates a > 2MB area which contains 511 mappings of the zero page and one real > page, the kernel will proceed to turn that area into a real, physical > huge page. So it consumes 2MB of memory which would not have > previously been allocated? Correct. > > If so, this might be rather undesirable behaviour in some situations > (and ditto the current behaviour for pte_none ptes)? > > This can be tuned by adjusting khugepaged_max_ptes_none, but not many > people are likely to do that because we didn't document the damn thing. khugepaged checks !hugepage_vma_check, so those apps that don't want it can opt out with MADV_NOHUGEPAGE. The sysctl allows to tune for the default behavior. > At all. Can we please rectify this, and update it for the is_zero_pfn > feature? The documentation should include an explanation telling > people how to decide what setting to use, how to observe its effects, > etc. Agreed, documentation for the sysfs control would be good to have indeed. In the meantime I've got a more urgent issue, for which the fix is appended below. Thanks, Andrea == >From aaa03f8c142c9a486e3e49de80f52d01a930ba3d Mon Sep 17 00:00:00 2001 From: Andrea Arcangeli Date: Fri, 20 Feb 2015 18:08:57 +0100 Subject: [PATCH] mm: incorporate zero pages into transparent huge pages fix After applying the "incorporate zero pages into transparent huge pages" feature, I've got an oops on a overnight stress test: ------------[ cut here ]------------ kernel BUG at mm/huge_memory.c:1920! invalid opcode: 0000 [#1] SMP Modules linked in: tun usbhid snd_hda_codec_realtek x86_pkg_temp_thermal snd_hda_codec_generic kvm_intel kvm snd_hda_intel crc32c_intel snd_hda_controller ghash_clmulni_intel xhci_pci snd_hda_codec xhci_hcd ehci_pci ehci_hcd snd_pcm usbcore psmouse sr_mod snd_timer snd cdrom pcspkr usb_common [last unloaded: microcode] CPU: 4 PID: 4250 Comm: Analysis Helper Not tainted 3.19.0+ #5 Hardware name: /DH61BE, BIOS BEH6110H.86A.0120.2013.1112.1412 11/12/2013 task: ffff88040c520840 ti: ffff880406070000 task.ti: ffff880406070000 RIP: 0010:[] [] split_huge_page_to_list+0x6a2/0x7c0 RSP: 0018:ffff880406073c58 EFLAGS: 00010282 RAX: 8000000163b2f067 RBX: ffff880406ac5f90 RCX: ffff880404f70978 RDX: ffffea0000000000 RSI: ffff880404f70000 RDI: 8000000163b2f047 RBP: ffff880408de25c0 R08: 00000000058ecbc0 R09: 00007f2dfe52f000 R10: 0000000000000000 R11: 00007f2dfe600000 R12: 00007f2dfe400000 R13: 0000000404f70067 R14: ffffc00000000fff R15: ffff8800d04da2e0 FS: 00007f2e12168700(0000) GS:ffff88041f300000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f03b9871000 CR3: 0000000407c9d000 CR4: 00000000000427e0 Stack: ffff880406073d08 00000007f2dfe400 00007f2d00000001 ffff880407f2dcd0 ffffea00058e8000 ffff880400000000 00000000058e8000 ffffffff81b8bf40 0000000000000004 ffffea00101ab170 ffff880408d273e8 ffff880406ac5f90 Call Trace: [] ? __split_huge_page_pmd+0xfa/0x2a0 [] ? unmap_single_vma+0x2b2/0x810 [] ? try_to_wake_up+0xbb/0x2d0 [] ? get_futex_key+0x1c8/0x2c0 [] ? zap_page_range+0x89/0xe0 [] ? handle_mm_fault+0xe70/0x1110 [] ? set_next_entity+0x4e/0x60 [] ? find_vma+0x5c/0x70 [] ? SyS_madvise+0x4f3/0x760 [] ? system_call_fastpath+0x12/0x17 Code: ff ff 0f 1f 80 00 00 00 00 c7 44 24 10 00 00 00 00 e9 ef fa ff ff b8 01 00 00 00 e9 3f fb ff ff 48 83 c8 40 e9 f7 fe ff ff 0f 0b <0f> 0b 48 c7 c6 28 b8 a0 81 4c 89 f7 e8 8d 3c fd ff 0f 0b 48 8b RIP [] split_huge_page_to_list+0x6a2/0x7c0 RSP ---[ end trace 6ca92529e1de43ba ]--- The oops happens here: BUG_ON(!pte_none(*pte)); In short when we do the split_huge_page_map we withdraw from the pgtable deposit of the MM and we find that pgtable isn't fully zero. That is most certainly because we didn't clear it if it was a zero page before putting it in the deposit. This adds the pte_clear to fix the bug. The PT lock could be actually not be taken, as the pte is already private to us and not visible to any other CPU (we'll be adding it to the deposit later), but because it's private the lock can't create any contention. Considering the paravirt calls (which also should be superfluous) may end up being invoked and make assumptions, I thought it was safer to keep the locking protocol the same, even if the pgtable is already private. In order to drop it however, we should drop it from the other path too. If we want to optimize away the lock from both branches, it's better to do it in a separate patch. Signed-off-by: Andrea Arcangeli --- mm/huge_memory.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/mm/huge_memory.c b/mm/huge_memory.c index a63da02..f0207cf 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -2205,6 +2205,18 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page, if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) { clear_user_highpage(page, address); add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1); + if (is_zero_pfn(pte_pfn(pteval))) { + /* + * ptl mostly unnecessary. + */ + spin_lock(ptl); + /* + * paravirt calls inside pte_clear here are + * superfluous. + */ + pte_clear(vma->vm_mm, address, _pte); + spin_unlock(ptl); + } } else { src_page = pte_page(pteval); copy_user_highpage(page, src_page, address, vma); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/