Re: [PATCH v7 2/3] mm/memory_hotplug: split memmap_on_memory requests across memblocks

[Verma, Vishal L]

On Mon, 2023-10-30 at 11:20 +0100, David Hildenbrand wrote:
> On 26.10.23 00:44, Vishal Verma wrote:
> > 
[..]

> > @@ -2146,11 +2186,69 @@ void try_offline_node(int nid)
> >   }
> >   EXPORT_SYMBOL(try_offline_node);
> >   
> > -static int __ref try_remove_memory(u64 start, u64 size)
> > +static void __ref remove_memory_blocks_and_altmaps(u64 start, u64 size)
> >   {
> > -       struct memory_block *mem;
> > -       int rc = 0, nid = NUMA_NO_NODE;
> > +       unsigned long memblock_size = memory_block_size_bytes();
> >         struct vmem_altmap *altmap = NULL;
> > +       struct memory_block *mem;
> > +       u64 cur_start;
> > +       int rc;
> > +
> > +       /*
> > +        * For memmap_on_memory, the altmaps could have been added on
> > +        * a per-memblock basis. Loop through the entire range if so,
> > +        * and remove each memblock and its altmap.
> > +        */
> 
> /*
>   * altmaps where added on a per-memblock basis; we have to process
>   * each individual memory block.
>   */
> 
> > +       for (cur_start = start; cur_start < start + size;
> > +            cur_start += memblock_size) {
> > +               rc = walk_memory_blocks(cur_start, memblock_size, &mem,
> > +                                       test_has_altmap_cb);
> > +               if (rc) {
> > +                       altmap = mem->altmap;
> > +                       /*
> > +                        * Mark altmap NULL so that we can add a debug
> > +                        * check on memblock free.
> > +                        */
> > +                       mem->altmap = NULL;
> > +               }
> 
> Simpler (especially, we know that there must be an altmap):
> 
> mem = find_memory_block(pfn_to_section_nr(cur_start));
> altmap = mem->altmap;
> mem->altmap = NULL;
> 
> I think we might be able to remove test_has_altmap_cb() then.
> 
> > +
> > +               remove_memory_block_devices(cur_start, memblock_size);
> > +
> > +               arch_remove_memory(cur_start, memblock_size, altmap);
> > +
> > +               /* Verify that all vmemmap pages have actually been freed. */
> > +               if (altmap) {
> 
> There must be an altmap, so this can be done unconditionally.

Hi David,

All other comments make sense, making those changes now.

However for this one, does the WARN() below go away then?

I was wondering if maybe arch_remove_memory() is responsible for
freeing the altmap here, and at this stage we're just checking if that
happened. If it didn't WARN and then free it.

I drilled down the path, and I don't see altmap actually getting freed
in vmem_altmap_free(), but I wasn't sure if <something else> was meant
to free it as altmap->alloc went down to 0.

> 
> > +                       WARN(altmap->alloc, "Altmap not fully unmapped");
> > +                       kfree(altmap);
> > +               }
> > +       }
> > +}
> 
>