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=-6.5 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 8E748CA9ED3 for ; Tue, 5 Nov 2019 02:28:19 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 02C7F214E0 for ; Tue, 5 Nov 2019 02:28:18 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=intel-com.20150623.gappssmtp.com header.i=@intel-com.20150623.gappssmtp.com header.b="qktoZDnm" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 02C7F214E0 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from bilbo.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 476YVm0r3CzDqDb for ; Tue, 5 Nov 2019 13:28:16 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=intel.com (client-ip=2607:f8b0:4864:20::844; helo=mail-qt1-x844.google.com; envelope-from=dan.j.williams@intel.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=intel-com.20150623.gappssmtp.com header.i=@intel-com.20150623.gappssmtp.com header.b="qktoZDnm"; dkim-atps=neutral Received: from mail-qt1-x844.google.com (mail-qt1-x844.google.com [IPv6:2607:f8b0:4864:20::844]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 476XDN50BjzF40r for ; Tue, 5 Nov 2019 12:30:40 +1100 (AEDT) Received: by mail-qt1-x844.google.com with SMTP id u22so27003243qtq.13 for ; Mon, 04 Nov 2019 17:30:40 -0800 (PST) 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=aR+2eBnDheAMkwOXsNXJUEiyYXIkOGnGgWNkPzd4tdQ=; b=qktoZDnmGaINw22JlCwMcZG1PDeQczipLlMB42AGNsv72JUru4Yl1D6oIEjBA38dzh ZS2PnoJ0nLiGYI/i2K3YUYXaOLydyfU1Nh9OJDcQC7tLDTMa0R1lDv4ems+WcI//b7L2 wR2dAsnPnZxJaxO7jVkMEQp5o3ksMp8LZBvOc6NF8jCZcTkkpEBzr7Bf+awpNyVVG6Ua qooyd6G9st5lQ4zbkKWFjUk8VEjPyKkcrE3NKGhOt3BMQgdbeqhbp+AtPIgHxyLjREHU SGLgLVGJs/nQldloF34in/meweNbuplR476f3VH7ViKAcI+PvQUy3TCespjZxP2iYT+N 0/3g== 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=aR+2eBnDheAMkwOXsNXJUEiyYXIkOGnGgWNkPzd4tdQ=; b=RffoJ5mVmAYWbo714iucootUnXAlhZ+kn0O5HBGEMoDinyzjeIVw6MPDf/qJcuwcSk opktXV1CdOjbPEo23nfxs6UUNcYGaq5L3U/0uR1O+iDsKIF4F1PUdfB+ODXtN81UA0q8 KOj8CK1WplXCbzoIOdf1HN1gPtJNGEOeS7pCylyEnYxVMGm18mIlFADL8rxxQk423g52 PK4E5v8M4xNojELToBw/KFFEIFPdOXdFHb4i6ndP26a6pYYV9kIotKhkr4ldq05kVD3/ Y/8AntW7KunuVaRTgGMUVpkEUfM5wFrB5xyh0i1s8c1421l4EjZSlQjOnK5m6+3/E4/7 xeTQ== X-Gm-Message-State: APjAAAWjYJHMs8bRb4mDx/YlVGsYy/gSNv4KRDk9EvBmVw8DRP/Q/RwI 48Q1uI4ufWp/R9ZhJJSid6mSWNF1ZYe4eQTt10rzGA== X-Google-Smtp-Source: APXvYqz7Bk4kRduwfrclstPYyEuP//nuwTqg9TTsoV0HtiW5xPO5/4utgjFzQW53c0oeh/hiUJTxgAj86G8lI66LWNo= X-Received: by 2002:ac8:424d:: with SMTP id r13mr15689594qtm.111.1572917436967; Mon, 04 Nov 2019 17:30:36 -0800 (PST) MIME-Version: 1.0 References: <20191024120938.11237-1-david@redhat.com> <20191024120938.11237-2-david@redhat.com> In-Reply-To: <20191024120938.11237-2-david@redhat.com> From: Dan Williams Date: Mon, 4 Nov 2019 17:30:25 -0800 Message-ID: Subject: Re: [PATCH v1 01/10] mm/memory_hotplug: Don't allow to online/offline memory blocks with holes To: David Hildenbrand Content-Type: text/plain; charset="UTF-8" X-Mailman-Approved-At: Tue, 05 Nov 2019 13:26:42 +1100 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-hyperv@vger.kernel.org, Michal Hocko , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , KVM list , Pavel Tatashin , KarimAllah Ahmed , Dave Hansen , Alexander Duyck , Michal Hocko , Linux MM , Paul Mackerras , "H. Peter Anvin" , Wanpeng Li , Alexander Duyck , "K. Y. Srinivasan" , Thomas Gleixner , Kees Cook , devel@driverdev.osuosl.org, Stefano Stabellini , Stephen Hemminger , "Aneesh Kumar K.V" , Joerg Roedel , X86 ML , YueHaibing , "Matthew Wilcox \(Oracle\)" , Mike Rapoport , Peter Zijlstra , Ingo Molnar , Vlastimil Babka , Anthony Yznaga , Oscar Salvador , "Isaac J. Manjarres" , Matt Sickler , Juergen Gross , Anshuman Khandual , Haiyang Zhang , Sasha Levin , kvm-ppc@vger.kernel.org, Qian Cai , Alex Williamson , Mike Rapoport , Borislav Petkov , Nicholas Piggin , Andy Lutomirski , xen-devel , Boris Ostrovsky , Vitaly Kuznetsov , Allison Randal , Jim Mattson , Mel Gorman , Cornelia Huck , Pavel Tatashin , Linux Kernel Mailing List , Sean Christopherson , Johannes Weiner , Paolo Bonzini , Andrew Morton , linuxppc-dev Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Thu, Oct 24, 2019 at 5:10 AM David Hildenbrand wrote: > > Our onlining/offlining code is unnecessarily complicated. Only memory > blocks added during boot can have holes (a range that is not > IORESOURCE_SYSTEM_RAM). Hotplugged memory never has holes (e.g., see > add_memory_resource()). All boot memory is alread online. s/alread/already/ ...also perhaps clarify "already online" by what point in time and why that is relevant. For example a description of the difference between the SetPageReserved() in the bootmem path and the one in the hotplug path. > Therefore, when we stop allowing to offline memory blocks with holes, we > implicitly no longer have to deal with onlining memory blocks with holes. Maybe an explicit reference of the code areas that deal with holes would help to back up that assertion. Certainly it would have saved me some time for the review. > This allows to simplify the code. For example, we no longer have to > worry about marking pages that fall into memory holes PG_reserved when > onlining memory. We can stop setting pages PG_reserved. ...but not for bootmem, right? > > Offlining memory blocks added during boot is usually not guranteed to work s/guranteed/guaranteed/ > either way (unmovable data might have easily ended up on that memory during > boot). So stopping to do that should not really hurt (+ people are not > even aware of a setup where that used to work Maybe put a "Link: https://lkml.kernel.org/r/$msg_id" to that discussion? > and that the existing code > still works correctly with memory holes). For the use case of offlining > memory to unplug DIMMs, we should see no change. (holes on DIMMs would be > weird). However, less memory can be offlined than was theoretically allowed previously, so I don't understand the "we should see no change" comment. I still agree that's a price worth paying to get the code cleanups and if someone screams we can look at adding it back, but the fact that it was already fragile seems decent enough protection. > > Please note that hardware errors (PG_hwpoison) are not memory holes and > not affected by this change when offlining. > > Cc: Andrew Morton > Cc: Michal Hocko > Cc: Oscar Salvador > Cc: Pavel Tatashin > Cc: Dan Williams > Cc: Anshuman Khandual > Signed-off-by: David Hildenbrand > --- > mm/memory_hotplug.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 561371ead39a..8d81730cf036 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -1447,10 +1447,19 @@ static void node_states_clear_node(int node, struct memory_notify *arg) > node_clear_state(node, N_MEMORY); > } > > +static int count_system_ram_pages_cb(unsigned long start_pfn, > + unsigned long nr_pages, void *data) > +{ > + unsigned long *nr_system_ram_pages = data; > + > + *nr_system_ram_pages += nr_pages; > + return 0; > +} > + > static int __ref __offline_pages(unsigned long start_pfn, > unsigned long end_pfn) > { > - unsigned long pfn, nr_pages; > + unsigned long pfn, nr_pages = 0; > unsigned long offlined_pages = 0; > int ret, node, nr_isolate_pageblock; > unsigned long flags; > @@ -1461,6 +1470,20 @@ static int __ref __offline_pages(unsigned long start_pfn, > > mem_hotplug_begin(); > > + /* > + * Don't allow to offline memory blocks that contain holes. > + * Consecuently, memory blocks with holes can never get onlined s/Consecuently/Consequently/ > + * (hotplugged memory has no holes and all boot memory is online). > + * This allows to simplify the onlining/offlining code quite a lot. > + */ The last sentence of this comment makes sense in the context of this patch, but I don't think it stands by itself in the code base after the fact. The person reading the comment can't see the simplifications because the code is already gone. I'd clarify it to talk about why it is safe to not mess around with PG_Reserved in the hotplug path because of this check. After those clarifications you can add: Reviewed-by: Dan Williams