Re: [PATCH v5 1/2] mm/memory_hotplug: split memmap_on_memory requests across memblocks

[David Hildenbrand]

On 07.10.23 00:01, Verma, Vishal L wrote:
> On Fri, 2023-10-06 at 14:52 +0200, David Hildenbrand wrote:
> > On 05.10.23 20:31, Vishal Verma wrote:
> > >
> <..>
> > > @@ -2167,47 +2221,28 @@ static int __ref try_remove_memory(u64 start, u64 size)
> > >          if (rc)
> > >                  return rc;
> > >    
> > > +       mem_hotplug_begin();
> > > +
> > >          /*
> > > -        * We only support removing memory added with MHP_MEMMAP_ON_MEMORY in
> > > -        * the same granularity it was added - a single memory block.
> > > +        * 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.
> > >           */
> > >          if (mhp_memmap_on_memory()) {
> > > -               rc = walk_memory_blocks(start, size, &mem, test_has_altmap_cb);
> > > -               if (rc) {
> > > -                       if (size != memory_block_size_bytes()) {
> > > -                               pr_warn("Refuse to remove %#llx - %#llx,"
> > > -                                       "wrong granularity\n",
> > > -                                       start, start + size);
> > > -                               return -EINVAL;
> > > -                       }
> > > -                       altmap = mem->altmap;
> > > -                       /*
> > > -                        * Mark altmap NULL so that we can add a debug
> > > -                        * check on memblock free.
> > > -                        */
> > > -                       mem->altmap = NULL;
> > > -               }
> > > +               unsigned long memblock_size = memory_block_size_bytes();
> > > +               u64 cur_start;
> > > +
> > > +               for (cur_start = start; cur_start < start + size;
> > > +                    cur_start += memblock_size)
> > > +                       remove_memory_block_and_altmap(nid, cur_start,
> > > +                                                      memblock_size);
> > > +       } else {
> > > +               remove_memory_block_and_altmap(nid, start, size);
> >
> > Better call remove_memory_block_devices() and arch_remove_memory(start,
> > size, altmap) here explicitly instead of using
> > remove_memory_block_and_altmap() that really can only handle a single
> > memory block with any inputs.
> I'm not sure I follow. Even in the non memmap_on_memory case, we'd have
> to walk_memory_blocks() to get to the memory_block->altmap, right?

See my other reply to, at least with mhp_memmap_on_memory()==false, we 
don't have to worry about the altmap.

> Or is there a more direct way? If we have to walk_memory_blocks, what's
> the advantage of calling those directly instead of calling the helper
> created above?

I think we have two cases to handle

1) All have an altmap. Remove them block-by-block. Probably we should 
call a function remove_memory_blocks(altmap=true) [or alternatively 
remove_memory_blocks_and_altmaps()] and just handle iterating internally.

2) All don't have an altmap. We can remove them in one go. Probably we 
should call that remove_memory_blocks(altmap=false) [or alternatively 
remove_memory_blocks_no_altmaps()].

I guess it's best to do a walk upfront to make sure either all have an 
altmap or none has one. Then we can branch off to the right function 
knowing whether we have to process altmaps or not.

The existing

if (mhp_memmap_on_memory()) {
	...
}

Can be extended for that case.

Please let me know if I failed to express what I mean, then I can 
briefly prototype it on top of your changes.

--
Cheers,

David / dhildenb