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

[Verma, Vishal L]

On Thu, 2023-11-02 at 09:16 +0800, Huang, Ying wrote:
> Vishal Verma <vishal.l.verma@xxxxxxxxx> writes:
> 
[..]
> > +
> > +static int create_altmaps_and_memory_blocks(int nid, struct memory_group *group,
> > +                                           u64 start, u64 size)
> > +{
> > +       unsigned long memblock_size = memory_block_size_bytes();
> > +       u64 cur_start;
> > +       int ret;
> > +
> > +       for (cur_start = start; cur_start < start + size;
> > +            cur_start += memblock_size) {
> > +               struct mhp_params params = { .pgprot =
> > +                                                    pgprot_mhp(PAGE_KERNEL) };
> > +               struct vmem_altmap mhp_altmap = {
> > +                       .base_pfn = PHYS_PFN(cur_start),
> > +                       .end_pfn = PHYS_PFN(cur_start + memblock_size - 1),
> > +               };
> > +
> > +               mhp_altmap.free = memory_block_memmap_on_memory_pages();
> > +               params.altmap = kmemdup(&mhp_altmap, sizeof(struct vmem_altmap),
> > +                                       GFP_KERNEL);
> > +               if (!params.altmap)
> > +                       return -ENOMEM;
> 
> Use "goto out" here too?

Hm, yes I suppose we want to clean up previous iterations of the loop -
I'll make this change.

> 
> > +
> > +               /* call arch's memory hotadd */
> > +               ret = arch_add_memory(nid, cur_start, memblock_size, &params);
> > +               if (ret < 0) {
> > +                       kfree(params.altmap);
> > +                       goto out;
> > +               }
> > +
> > +               /* create memory block devices after memory was added */
> > +               ret = create_memory_block_devices(cur_start, memblock_size,
> > +                                                 params.altmap, group);
> > +               if (ret) {
> > +                       arch_remove_memory(cur_start, memblock_size, NULL);
> > +                       kfree(params.altmap);
> 
> How about move arch_remove_memory() and kree() to error path and use
> different label?

I thought of this, but it got slightly awkward because of the scope of
'params' (declared/allocated within the loop), just kfree'ing in that
scope looked cleaner..