Re: Boot failures with "mm/sparse: Remove CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER" on powerpc (was Re: mmotm 2018-07-10-16-50 uploaded)

Oscar Salvador [off-list ref] writes:

El dc., 11 jul. 2018 , 15:56, Pavel Tatashin [off-list ref] va
escriure:

quoted

I am OK, if this patch is removed from Baoquan's series. But, I would
still like to get rid of CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER, I
can work on this in my sparse_init re-write series. ppc64 should
really fallback safely to small chunks allocs, and if it does not
there is some existing bug. Michael please send the config that you
used.

Thank you,
Pavel
On Wed, Jul 11, 2018 at 9:37 AM Oscar Salvador
[off-list ref] wrote:

quoted

On Wed, Jul 11, 2018 at 10:49:58PM +1000, Michael Ellerman wrote:

quoted

akpm@linux-foundation.org writes:

quoted

The mm-of-the-moment snapshot 2018-07-10-16-50 has been uploaded to

   http://www.ozlabs.org/~akpm/mmotm/

...

quoted

* mm-sparse-add-a-static-variable-nr_present_sections.patch
* mm-sparsemem-defer-the-ms-section_mem_map-clearing.patch
* mm-sparsemem-defer-the-ms-section_mem_map-clearing-fix.patch
*

mm-sparse-add-a-new-parameter-data_unit_size-for-alloc_usemap_and_memmap.patch

quoted

quoted

quoted

* mm-sparse-optimize-memmap-allocation-during-sparse_init.patch
*

mm-sparse-optimize-memmap-allocation-during-sparse_init-checkpatch-fixes.patch

quoted

quoted

quoted

* mm-sparse-remove-config_sparsemem_alloc_mem_map_together.patch

This seems to be breaking my powerpc pseries qemu boots.

The boot log with some extra debug shows eg:

  $ make pseries_le_defconfig

Could you please share the config?
I was not able to find such config in the kernel tree.

I just roughly check, but if I checked the right place,
vmemmap_populated() checks for the section to contain the flags we are
setting in sparse_init_one_section().

Yes.

But with this patch, we populate first everything, and then we call
sparse_init_one_section() in sparse_init().
As I said I could be mistaken because I just checked the surface.

Yeah I think that's correct.

This might just be a bug in our code, let me look at it a bit.

cheers

quoted

I just roughly check, but if I checked the right place,
vmemmap_populated() checks for the section to contain the flags we are
setting in sparse_init_one_section().

Yes.

quoted

But with this patch, we populate first everything, and then we call
sparse_init_one_section() in sparse_init().
As I said I could be mistaken because I just checked the surface.

Yeah I think that's correct.

This might just be a bug in our code, let me look at it a bit.

I wonder if something like this could make the trick:

diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index 51ce091914f9..e281651f50cd 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c

@@ -177,6 +177,8 @@ static __meminit void vmemmap_list_populate(unsigned long phys,
        vmemmap_list = vmem_back;
 }
 
+static unsigned long last_addr_populated = 0;
+
 int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
                struct vmem_altmap *altmap)
 {

@@ -191,7 +193,7 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
                void *p;
                int rc;
 
-               if (vmemmap_populated(start, page_size))
+               if (start + page_size <= last_addr_populated)
                        continue;
 
                if (altmap)

@@ -212,6 +214,7 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
                                __func__, rc);
                        return -EFAULT;
                }
+               last_addr_populated = start + page_size;
        }

I know it looks hacky, and chances are that are wrong, but could you give it a try?
I will try to grab a ppc server and try it out too.
 
Thanks
-- 
Oscar Salvador
SUSE L3

On Thu, Jul 12, 2018 at 5:50 AM Oscar Salvador
[off-list ref] wrote:

quoted

quoted

I just roughly check, but if I checked the right place,
vmemmap_populated() checks for the section to contain the flags we are
setting in sparse_init_one_section().

Yes.

quoted

But with this patch, we populate first everything, and then we call
sparse_init_one_section() in sparse_init().
As I said I could be mistaken because I just checked the surface.

Yes, this is right, sparse_init_one_section() is needed after every
populate call on ppc64. I am adding this to my sparse_init re-write,
and it actually simplifies code, as it avoids one extra loop, and
makes ppc64 to work.

Pavel