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, ¶ms);
> > + 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..
