diff mbox series

[2/4] mm/sparse: skip no-map memblock check when fill_subsection_map

Message ID 20220711122459.13773-3-me@linux.beauty (mailing list archive)
State New, archived
Headers show
Series add struct page and Direct I/O support to reserved memory | expand

Commit Message

Li Chen July 11, 2022, 12:24 p.m. UTC 
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(-)

Comments

David Hildenbrand July 11, 2022, 2:53 p.m. UTC | #1 
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.

If the subsection map already spans memory (iow, subsection map is set)
you intend to add, then something already added memory in that range?
Li Chen July 12, 2022, 4:23 a.m. UTC | #2 
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.

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.

 > 
 > If the subsection map already spans memory (iow, subsection map is set)
 > you intend to add, then something already added memory in that range?

No matter with or without this patch, fill_subsection_map will always return -EEXIST for mapped memory, so that's not a problem.
The key point here is I allowed no-map region to pass the check and didn't change anything to the mapped region.

Regards,
Li
David Hildenbrand July 12, 2022, 7:31 a.m. UTC | #3 
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.

> 
> 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.
Li Chen July 12, 2022, 9:31 a.m. UTC | #4 
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
 > 
 >
kernel test robot July 14, 2022, 6:45 p.m. UTC | #5 
Hi Li,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on arm64/for-next/core arm-perf/for-next/perf linus/master v5.19-rc6 next-20220714]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Li-Chen/add-struct-page-and-Direct-I-O-support-to-reserved-memory/20220711-202957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-randconfig-k001 (https://download.01.org/0day-ci/archive/20220715/202207150209.3Svjqq9D-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 5e61b9c556267086ef9b743a0b57df302eef831b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d9809d17afee6693084b417325807c7123432fab
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Li-Chen/add-struct-page-and-Direct-I-O-support-to-reserved-memory/20220711-202957
        git checkout d9809d17afee6693084b417325807c7123432fab
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> WARNING: modpost: vmlinux.o(.text+0x346d8f): Section mismatch in reference from the function fill_subsection_map() to the function .meminit.text:memblock_is_map_memory()
The function fill_subsection_map() references
the function __meminit memblock_is_map_memory().
This is often because fill_subsection_map lacks a __meminit
annotation or the annotation of memblock_is_map_memory is wrong.
diff mbox series

Patch

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,