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=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 55AAAC2D0BF for ; Tue, 10 Dec 2019 23:53:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2AAED20663 for ; Tue, 10 Dec 2019 23:53:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726874AbfLJXx2 (ORCPT ); Tue, 10 Dec 2019 18:53:28 -0500 Received: from ale.deltatee.com ([207.54.116.67]:45848 "EHLO ale.deltatee.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726631AbfLJXx1 (ORCPT ); Tue, 10 Dec 2019 18:53:27 -0500 Received: from guinness.priv.deltatee.com ([172.16.1.162]) by ale.deltatee.com with esmtp (Exim 4.92) (envelope-from ) id 1iepJ1-0006ir-2c; Tue, 10 Dec 2019 16:52:40 -0700 To: David Hildenbrand , Michal Hocko Cc: Dan Williams , Linux Kernel Mailing List , Linux ARM , linux-ia64@vger.kernel.org, linuxppc-dev , linux-s390 , Linux-sh , platform-driver-x86@vger.kernel.org, Linux MM , Christoph Hellwig , Andrew Morton , Catalin Marinas , Will Deacon , Benjamin Herrenschmidt , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , Andy Lutomirski , Peter Zijlstra References: <20191209191346.5197-1-logang@deltatee.com> <20191209191346.5197-6-logang@deltatee.com> <20191210100432.GC10404@dhcp22.suse.cz> <6da2b279-6a6d-d89c-a34c-962ed021d91d@redhat.com> <20191210103452.GF10404@dhcp22.suse.cz> From: Logan Gunthorpe Message-ID: <297b7cc0-c5bc-a4c6-83eb-afc008395234@deltatee.com> Date: Tue, 10 Dec 2019 16:52:31 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Language: en-CA Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 172.16.1.162 X-SA-Exim-Rcpt-To: peterz@infradead.org, luto@kernel.org, dave.hansen@linux.intel.com, bp@alien8.de, mingo@redhat.com, tglx@linutronix.de, benh@kernel.crashing.org, will@kernel.org, catalin.marinas@arm.com, akpm@linux-foundation.org, hch@lst.de, linux-mm@kvack.org, platform-driver-x86@vger.kernel.org, linux-sh@vger.kernel.org, linux-s390@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-ia64@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, dan.j.williams@intel.com, mhocko@kernel.org, david@redhat.com X-SA-Exim-Mail-From: logang@deltatee.com Subject: Re: [PATCH 5/6] mm, memory_hotplug: Provide argument for the pgprot_t in arch_add_memory() X-SA-Exim-Version: 4.2.1 (built Wed, 08 May 2019 21:11:16 +0000) X-SA-Exim-Scanned: Yes (on ale.deltatee.com) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2019-12-10 4:25 a.m., David Hildenbrand wrote: > On 10.12.19 11:34, Michal Hocko wrote: >> On Tue 10-12-19 11:09:46, David Hildenbrand wrote: >>> On 10.12.19 11:04, Michal Hocko wrote: >>>> On Mon 09-12-19 12:43:40, Dan Williams wrote: >>>>> On Mon, Dec 9, 2019 at 12:24 PM Logan Gunthorpe wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 2019-12-09 12:23 p.m., David Hildenbrand wrote: >>>>>>> On 09.12.19 20:13, Logan Gunthorpe wrote: >>>> [...] >>>>>>>> #ifdef CONFIG_MEMORY_HOTPLUG >>>>>>>> -int arch_add_memory(int nid, u64 start, u64 size, >>>>>>>> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, >>>>>>>> struct mhp_restrictions *restrictions) >>>>>>> >>>>>>> Can we fiddle that into "struct mhp_restrictions" instead? >>>>>> >>>>>> Yes, if that's what people want, it's pretty trivial to do. I chose not >>>>>> to do it that way because it doesn't get passed down to add_pages() and >>>>>> it's not really a "restriction". If I don't hear any objections, I will >>>>>> do that for v2. >>>>> >>>>> +1 to storing this information alongside the altmap in that structure. >>>>> However, I agree struct mhp_restrictions, with the MHP_MEMBLOCK_API >>>>> flag now gone, has lost all of its "restrictions". How about dropping >>>>> the 'flags' property and renaming the struct to 'struct >>>>> mhp_modifiers'? >>>> >>>> Hmm, this email somehow didn't end up in my inbox so I have missed it >>>> before replying. >>>> >>>> Well, mhp_modifiers makes some sense and it would reduce the API >>>> proliferation but how do you expect the prot part to be handled? >>>> I really do not want people to think about PAGE_KERNEL or which >>>> protection to use because my experience tells that this will get copied >>>> without much thinking or simply will break with some odd usecases. >>>> So how exactly this would be used? >>> >>> I was thinking about exactly the same "issue". >>> >>> 1. default initialization via a function >>> >>> memhp_modifier_default_init(&modified); >>> >>> 2. a flag that unlocks the prot field (default:0). Without the flag, it >>> is ignored. We can keep the current initialization then. >>> >>> Other ideas? >> >> 3. a prot mask to apply on top of PAGE_KERNEL? Or would that be >> insufficient/clumsy? >> > > If it works for the given use case, I guess this would be simple and ok. I don't see how we can do that without a ton of work. The pgport_t is architecture specific so we'd need to add mask functions to every architecture for every page cache type we need to use. I don't think that's a good idea. I think I slightly prefer option 2 over the above. But I'd actually prefer callers have to think about the caching type seeing when we grew the second user (memremap_pages()) and it was paired with track_pfn_remap(), it was actually subtly wrong because it could create a mapping that track_pfn_remap() disagreed with. In fact, PAGE_KERNEL has already been specified in memremap_pages() for ages, it was just ignored when it got to the arch_add_memory() step which is were this issue comes from. In my opinion, having a coder and reviewer see PAGE_KERNEL and ask if that makes sense is a benefit. Having it hidden because we don't want people to think about it is worse, harder to understand and results in bugs that are more difficult to spot. Though, we may be overthinking this: arch_add_memory() is a low level non-exported API that's currently used in exactly two places. I don't think there's going to be many, if any, valid new use cases coming up for it in the future. That's more what memremap_pages() is for. Logan