linux-kernel - Re: [PATCH v8 2/3] mm/memory_hotplug: split memmap_on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <725291282836a82d2ac9c62dd5d5d2eedfd24c0b.camel@intel.com>
Date:   Thu, 2 Nov 2023 01:27:24 +0000
From:   "Verma, Vishal L" <vishal.l.verma@...el.com>
To:     "Huang, Ying" <ying.huang@...el.com>
CC:     "david@...hat.com" <david@...hat.com>,
        "Jiang, Dave" <dave.jiang@...el.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "osalvador@...e.de" <osalvador@...e.de>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "Jonathan.Cameron@...wei.com" <Jonathan.Cameron@...wei.com>,
        "nvdimm@...ts.linux.dev" <nvdimm@...ts.linux.dev>,
        "aneesh.kumar@...ux.ibm.com" <aneesh.kumar@...ux.ibm.com>,
        "Hocko, Michal" <mhocko@...e.com>,
        "jmoyer@...hat.com" <jmoyer@...hat.com>,
        "linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>
Subject: Re: [PATCH v8 2/3] mm/memory_hotplug: split memmap_on_memory requests
 across memblocks

On Thu, 2023-11-02 at 09:16 +0800, Huang, Ying wrote:
> Vishal Verma <vishal.l.verma@...el.com> 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..