Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Jul 2022 17:31:13 +0800
From:   Li Chen <me@linux.beauty>
To:     "David Hildenbrand" <david@redhat.com>
Cc:     "Catalin Marinas" <catalin.marinas@arm.com>,
        "Will Deacon" <will@kernel.org>,
        "Rob Herring" <robh+dt@kernel.org>,
        "Frank Rowand" <frowand.list@gmail.com>,
        "Andrew Morton" <akpm@linux-foundation.org>,
        "Li Chen" <lchen@ambarella.com>,
        "linux-arm-kernel" <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel" <linux-kernel@vger.kernel.org>,
        "devicetree" <devicetree@vger.kernel.org>,
        "linux-mm" <linux-mm@kvack.org>
Message-ID: <181f1bf9584.f463724e580236.5502316582440422915@linux.beauty>
In-Reply-To: <4cf14bde-fb4c-99d9-58ce-a788a700d5f3@redhat.com>
References: <20220711122459.13773-1-me@linux.beauty>
 <20220711122459.13773-3-me@linux.beauty>
 <c2d4c6ac-fad7-d5b3-8cbf-f62a7db5b998@redhat.com>
 <181f0a5f2a5.cf94ce78513585.4158910057206462182@linux.beauty> <4cf14bde-fb4c-99d9-58ce-a788a700d5f3@redhat.com>
Subject: Re: [PATCH 2/4] mm/sparse: skip no-map memblock check when
 fill_subsection_map
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Importance: Medium
User-Agent: Zoho Mail
Precedence: bulk

Hi David,
 ---- On Tue, 12 Jul 2022 15:31:08 +0800  David Hildenbrand <david@redhat.com> wrote --- 
 > On 12.07.22 06:23, Li Chen wrote:
 > > Hi David,
 > >  ---- On Mon, 11 Jul 2022 22:53:36 +0800  David Hildenbrand <david@redhat.com> wrote --- 
 > >  > On 11.07.22 14:24, Li Chen wrote:
 > >  > > From: Li Chen <lchen@ambarella.com>
 > >  > > 
 > >  > > When mhp use sparse_add_section, don't check no-map region,
 > >  > > so that to allow no-map reserved memory to get struct page
 > >  > > support.
 > >  > > 
 > >  > > Signed-off-by: Li Chen <lchen@ambarella.com>
 > >  > > Change-Id: I0d2673cec1b66adf695251037a00c240976b226f
 > >  > > ---
 > >  > >  mm/sparse.c | 4 +++-
 > >  > >  1 file changed, 3 insertions(+), 1 deletion(-)
 > >  > > 
 > >  > > diff --git a/mm/sparse.c b/mm/sparse.c
 > >  > > index 120bc8ea5293..a29cd1e7014f 100644
 > >  > > --- a/mm/sparse.c
 > >  > > +++ b/mm/sparse.c
 > >  > > @@ -690,7 +690,9 @@ static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
 > >  > >  
 > >  > >      if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
 > >  > >          rc = -EINVAL;
 > >  > > -    else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
 > >  > > +    else if (memblock_is_map_memory(PFN_PHYS(pfn)) &&
 > >  > > +         bitmap_intersects(map, subsection_map,
 > >  > > +                   SUBSECTIONS_PER_SECTION))
 > >  > >          rc = -EEXIST;
 > >  > >      else
 > >  > >          bitmap_or(subsection_map, map, subsection_map,
 > >  > 
 > >  > I'm not sure I follow completely what you are trying to achieve. But if
 > >  > you have to add memblock hacks into mm/sparse.c you're most probably
 > >  > doing something wrong.
 > >  > 
 > >  > Please explain why that change is necessary, and why it is safe.
 > > 
 > > In the current sparse memory model, free_area_init will insert all memblock.memory into subsection_map and no-map rmem is also a 
 > > memblock.memory. So, without this change, fill_subsection_map will return -EEXIST.
 > > 
 > > I would say it's not a good idea to insert no-map memblock into subsection_map, and I have no idea why sparse do this.
 > > So, I simply skip no-map region here.
 > 
 > The thing is:
 > 
 > if the subsection map is set, then there already *is* a memmap and you
 > would simply be ignoring it (and overwriting a memmap in e.g.,
 > ZONE_NORMAL to be in ZONE_DEVICE suddenly, which is wrong).
 > 
 > 
 > Reading memblock_mark_nomap():
 > 
 > "The memory regions marked with %MEMBLOCK_NOMAP will not be added to the
 > direct mapping of the physical memory. These regions will still be
 > covered by the memory map. The struct page representing NOMAP memory
 > frames in the memory map will be PageReserved()"
 > 
 > 
 > So having a memmap for these ranges is expected, and a direct map is not
 > desired. What you propose is a hack. You either have to reuse the
 > existing memmap (which is !ZONE_DEVICE -- not sure if that's a problem)
 > or we'd have to look into teaching init code to not allocate a memmap
 > for sub-sections that are fully nomap.
 > 
 > But not sure who depends on the existing memmap for nomap memory.

 Points taken, thanks! I will try to dig into it.

Regards,
Li
 > > 
 > > As for safety:
 > > 1. The caller of fill_subsection_map are mhp and *_memremap_pages functions, no-map regions are not related to them, so existing codes won't be broken.
 > > 2. This change doesn't change memblock and subsection_map.
 > > 
 > 
 > Sorry, but AFAIKT it's a hack and we need a clean way to deal with nomap
 > memory that already has a memmap instead.
 > 
 > 
 > -- 
 > Thanks,
 > 
 > David / dhildenb
 > 
 >