From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.7 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7892FCA9EB9 for ; Wed, 23 Oct 2019 19:39:46 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 1EFA321928 for ; Wed, 23 Oct 2019 19:39:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=intel-com.20150623.gappssmtp.com header.i=@intel-com.20150623.gappssmtp.com header.b="bcYrgPfU" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1EFA321928 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id A7FCF6B0003; Wed, 23 Oct 2019 15:39:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A0A206B0006; Wed, 23 Oct 2019 15:39:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8A9FE6B0007; Wed, 23 Oct 2019 15:39:45 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0054.hostedemail.com [216.40.44.54]) by kanga.kvack.org (Postfix) with ESMTP id 620926B0003 for ; Wed, 23 Oct 2019 15:39:45 -0400 (EDT) Received: from smtpin21.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id 04E258249980 for ; Wed, 23 Oct 2019 19:39:45 +0000 (UTC) X-FDA: 76076064330.21.smell58_54f05bbcca258 X-HE-Tag: smell58_54f05bbcca258 X-Filterd-Recvd-Size: 13430 Received: from mail-ot1-f68.google.com (mail-ot1-f68.google.com [209.85.210.68]) by imf34.hostedemail.com (Postfix) with ESMTP for ; Wed, 23 Oct 2019 19:39:43 +0000 (UTC) Received: by mail-ot1-f68.google.com with SMTP id g13so18471566otp.8 for ; Wed, 23 Oct 2019 12:39:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=8ApkkYruavPD6VaVfU4QHr5pRzVSLLe/BmCajbyD76I=; b=bcYrgPfUve+/e7PA/COpa9cpiotkNE8e14cXEXZpBzSMlJpSompwdbTdWjjkQJddx0 Uw+3WqPKAUioj4/4XPm7cJ24FTR0IET9Naz1ZwjQyErO2IZvSkGMpp/gew8XlZpggjwU urspNOSnz3kU9GTyqmzURLWDOMJHHwAlk9WGjmcgjm+VaVmSdvOU+um4KKw70KPHFsUp FTUvgk6gpDTS/o2wCQdq+8Gi3txZF11FZsotj80J3+OxzF3vILzEFTeGcvRDQy1jMFEi SRx85TI+B5z5ijy9jfA6ym8yiuLiEhXgUKJRjaghu/8ovT45aTiORONSiCHuKkbYvhPp fMSQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=8ApkkYruavPD6VaVfU4QHr5pRzVSLLe/BmCajbyD76I=; b=qEqbW8ORta8zjmGnZAbaU6a1yCLSBx3+tDzZYNdwKzjDIwr5g7kKmVaG7aepMm4z6x DKMxgh7gLmlzCMNMYUsFH0UJdb/MQgvnS2b6j4t5dFnR3QscOSR8TL3qF75dLjFglkzw 0lHXWnhhPieXml58tnMt5e4WPEeMvRMJ0052bhzxuXSMfBUYreFa5709NgBDYysKg0tY clbnAFCGmlnfH0Qype6vCxG4x/fDTwRZKPLIovPXKKxMqp4EZyfo4F6y/IBPYbWoZfbz UaNHnvP5KUIJ/FdR+PtaLZXfuqmQsXkSsOZ9a9/tVztzSk3PIce1scAVcrOI/suvtyto jaaA== X-Gm-Message-State: APjAAAVnKVG4WEj4O/9E0vt2Wofie2HEixcqTUk8MYz6LrkQ2ANAUjEH VKhZOzz+BMnjsgXQliiC0Uo6RVkczHq8hRNDrnjg+Q== X-Google-Smtp-Source: APXvYqwijxA4UnPG5Tbj7UXgroNEpqpPtlp/BzJIeWUQUfe3F9HUg3GXu8gWZWfrY7GBmsZ1h9DSM1JTtNPajl0t3Ss= X-Received: by 2002:a05:6830:617:: with SMTP id w23mr8183922oti.247.1571859582403; Wed, 23 Oct 2019 12:39:42 -0700 (PDT) MIME-Version: 1.0 References: <20191022171239.21487-1-david@redhat.com> <55640861-bbcb-95f0-766a-95bc961f1b0e@redhat.com> In-Reply-To: <55640861-bbcb-95f0-766a-95bc961f1b0e@redhat.com> From: Dan Williams Date: Wed, 23 Oct 2019 12:39:31 -0700 Message-ID: Subject: Re: [PATCH RFC v1 00/12] mm: Don't mark hotplugged pages PG_reserved (including ZONE_DEVICE) To: David Hildenbrand Cc: Linux Kernel Mailing List , Linux MM , Michal Hocko , Andrew Morton , kvm-ppc@vger.kernel.org, linuxppc-dev , KVM list , linux-hyperv@vger.kernel.org, devel@driverdev.osuosl.org, xen-devel , X86 ML , Alexander Duyck , Kees Cook , Alex Williamson , Allison Randal , Andy Lutomirski , "Aneesh Kumar K.V" , Anshuman Khandual , Anthony Yznaga , Ben Chan , Benjamin Herrenschmidt , Borislav Petkov , Boris Ostrovsky , Christophe Leroy , Cornelia Huck , Dan Carpenter , Dave Hansen , Fabio Estevam , Greg Kroah-Hartman , Haiyang Zhang , "H. Peter Anvin" , Ingo Molnar , "Isaac J. Manjarres" , Jeremy Sowden , Jim Mattson , Joerg Roedel , Johannes Weiner , Juergen Gross , KarimAllah Ahmed , Kate Stewart , "K. Y. Srinivasan" , Madhumitha Prabakaran , Matt Sickler , Mel Gorman , Michael Ellerman , Michal Hocko , Mike Rapoport , Mike Rapoport , Nicholas Piggin , Nishka Dasgupta , Oscar Salvador , Paolo Bonzini , Paul Mackerras , Paul Mackerras , Pavel Tatashin , Pavel Tatashin , Peter Zijlstra , Qian Cai , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Rob Springer , Sasha Levin , Sean Christopherson , =?UTF-8?Q?Simon_Sandstr=C3=B6m?= , Stefano Stabellini , Stephen Hemminger , Thomas Gleixner , Todd Poynor , Vandana BN , Vitaly Kuznetsov , Vlastimil Babka , Wanpeng Li , YueHaibing Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Oct 23, 2019 at 10:28 AM David Hildenbrand wrote: > > >> I dislike this for three reasons > >> > >> a) It does not protect against any races, really, it does not improve things. > >> b) We do have the exact same problem with pfn_to_online_page(). As long as we > >> don't hold the memory hotplug lock, memory can get offlined and remove any time. Racy. > > > > True, we need to solve that problem too. That seems to want something > > lighter weight than the hotplug lock that can be held over pfn lookups > > + use rather than requiring a page lookup in paths where it's not > > clear that a page reference would prevent unplug. > > > >> c) We mix in ZONE specific stuff into the core. It should be "just another zone" > > > > Not sure I grok this when the RFC is sprinkling zone-specific > > is_zone_device_page() throughout the core? > > Most users should not care about the zone. pfn_active() would be enough > in most situations, especially most PFN walkers - "this memmap is valid > and e.g., contains a valid zone ...". Oh, I see, you're saying convert most users to pfn_active() (and some TBD rcu locking), but only pfn_to_online_page() users would need the zone lookup? I can get on board with that. > > > > >> > >> What I propose instead (already discussed in https://lkml.org/lkml/2019/10/10/87) > > > > Sorry I missed this earlier... > > > >> > >> 1. Convert SECTION_IS_ONLINE to SECTION_IS_ACTIVE > >> 2. Convert SECTION_IS_ACTIVE to a subsection bitmap > >> 3. Introduce pfn_active() that checks against the subsection bitmap > >> 4. Once the memmap was initialized / prepared, set the subsection active > >> (similar to SECTION_IS_ONLINE in the buddy right now) > >> 5. Before the memmap gets invalidated, set the subsection inactive > >> (similar to SECTION_IS_ONLINE in the buddy right now) > >> 5. pfn_to_online_page() = pfn_active() && zone != ZONE_DEVICE > >> 6. pfn_to_device_page() = pfn_active() && zone == ZONE_DEVICE > > > > This does not seem to reduce any complexity because it still requires > > a pfn to zone lookup at the end of the process. > > > > I.e. converting pfn_to_online_page() to use a new pfn_active() > > subsection map plus looking up the zone from pfn_to_page() is more > > steps than just doing a direct pfn to zone lookup. What am I missing? > > That a real "pfn to zone" lookup without going via the struct page will > require to have more than just a single bitmap. IMHO, keeping the > information at a single place (memmap) is the clean thing to do (not > replicating it somewhere else). Going via the memmap might not be as > fast as a direct lookup, but do we actually care? We are already looking > at "random PFNs we are not sure if there is a valid memmap". True, we only care about the validity of the check, and as you pointed out moving the check to the pfn level does not solve the validity race. It needs a lock. > > >> > >> Especially, driver-reserved device memory will not get set active in > >> the subsection bitmap. > >> > >> Now to the race. Taking the memory hotplug lock at random places is ugly. I do > >> wonder if we can use RCU: > > > > Ah, yes, exactly what I was thinking above. > > > >> > >> The user of pfn_active()/pfn_to_online_page()/pfn_to_device_page(): > >> > >> /* the memmap is guaranteed to remain active under RCU */ > >> rcu_read_lock(); > >> if (pfn_active(random_pfn)) { > >> page = pfn_to_page(random_pfn); > >> ... use the page, stays valid > >> } > >> rcu_unread_lock(); > >> > >> Memory offlining/memremap code: > >> > >> set_subsections_inactive(pfn, nr_pages); /* clears the bit atomically */ > >> synchronize_rcu(); > >> /* all users saw the bitmap update, we can invalide the memmap */ > >> remove_pfn_range_from_zone(zone, pfn, nr_pages); > > > > Looks good to me. > > > >> > >>> > >>>> > >>>> I only gave it a quick test with DIMMs on x86-64, but didn't test the > >>>> ZONE_DEVICE part at all (any tips for a nice QEMU setup?). Compile-tested > >>>> on x86-64 and PPC. > >>> > >>> I'll give it a spin, but I don't think the kernel wants to grow more > >>> is_zone_device_page() users. > >> > >> Let's recap. In this RFC, I introduce a total of 4 (!) users only. > >> The other parts can rely on pfn_to_online_page() only. > >> > >> 1. "staging: kpc2000: Prepare transfer_complete_cb() for PG_reserved changes" > >> - Basically never used with ZONE_DEVICE. > >> - We hold a reference! > >> - All it protects is a SetPageDirty(page); > >> > >> 2. "staging/gasket: Prepare gasket_release_page() for PG_reserved changes" > >> - Same as 1. > >> > >> 3. "mm/usercopy.c: Prepare check_page_span() for PG_reserved changes" > >> - We come via virt_to_head_page() / virt_to_head_page(), not sure about > >> references (I assume this should be fine as we don't come via random > >> PFNs) > >> - We check that we don't mix Reserved (including device memory) and CMA > >> pages when crossing compound pages. > >> > >> I think we can drop 1. and 2., resulting in a total of 2 new users in > >> the same context. I think that is totally tolerable to finally clean > >> this up. > > > > ...but more is_zone_device_page() doesn't "finally clean this up". > > Like we discussed above it's the missing locking that's the real > > cleanup, the pfn_to_online_page() internals are secondary. > > It's a different cleanup IMHO. We can't do everything in one shot. But > maybe I can drop the is_zone_device_page() parts from this patch and > completely rely on pfn_to_online_page(). Yes, that needs fixing to, but > it's a different story. > > The important part of this patch: > > While pfn_to_online_page() will always exclude ZONE_DEVICE pages, > checking PG_reserved on ZONE_DEVICE pages (what we do right now!) is > racy as hell (especially when concurrently initializing the memmap). > > This does improve the situation. True that's a race a vector I was not considering. > > >> > >> However, I think we also have to clarify if we need the change in 3 at all. > >> It comes from > >> > >> commit f5509cc18daa7f82bcc553be70df2117c8eedc16 > >> Author: Kees Cook > >> Date: Tue Jun 7 11:05:33 2016 -0700 > >> > >> mm: Hardened usercopy > >> > >> This is the start of porting PAX_USERCOPY into the mainline kernel. This > >> is the first set of features, controlled by CONFIG_HARDENED_USERCOPY. The > >> work is based on code by PaX Team and Brad Spengler, and an earlier port > >> from Casey Schaufler. Additional non-slab page tests are from Rik van Riel. > >> [...] > >> - otherwise, object must not span page allocations (excepting Reserved > >> and CMA ranges) > >> > >> Not sure if we really have to care about ZONE_DEVICE at this point. > > > > That check needs to be careful to ignore ZONE_DEVICE pages. There's > > nothing wrong with a copy spanning ZONE_DEVICE and typical pages. > > Please note that the current check would *forbid* this (AFAIKs for a > single heap object). As discussed in the relevant patch, we might be > able to just stop doing that and limit it to real PG_reserved pages > (without ZONE_DEVICE). I'd be happy to not introduce new > is_zone_device_page() users. At least for non-HMM ZONE_DEVICE usage, i.e. the dax + pmem stuff, is excluded from this path by: 52f476a323f9 libnvdimm/pmem: Bypass CONFIG_HARDENED_USERCOPY overhead So this case is one more to add to the pile of HMM auditing.