linux-kernel - Re: [PATCH v3 4/4] mm/memory_hotplug: optimize memory hotplug

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: <CAOAebxsf21pKsHoJQ7+5mWnfj=TA_Nd2h=YvuEfj=SmpFfvjxQ@mail.gmail.com>
Date:   Thu, 15 Feb 2018 08:46:33 -0500
From:   Pavel Tatashin <pasha.tatashin@...cle.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     Steve Sistare <steven.sistare@...cle.com>,
        Daniel Jordan <daniel.m.jordan@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Linux Memory Management List <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Bharata B Rao <bharata@...ux.vnet.ibm.com>,
        Thomas Gleixner <tglx@...utronix.de>, mingo@...hat.com,
        hpa@...or.com, x86@...nel.org, dan.j.williams@...el.com,
        kirill.shutemov@...ux.intel.com, bhe@...hat.com
Subject: Re: [PATCH v3 4/4] mm/memory_hotplug: optimize memory hotplug

> This should be a separate patch IMHO. It is an optimization on its
> own. The original code tries to be sparse neutral but we do depend on
> sparse anyway.

Yes, Mingo already asked me to split this patch. I've done just that
and will send it out soon.

>
> [...]
>>  /* register memory section under specified node if it spans that node */
>> -int register_mem_sect_under_node(struct memory_block *mem_blk, int nid)
>> +int register_mem_sect_under_node(struct memory_block *mem_blk, int nid,
>> +                              bool check_nid)
>
> This check_nid begs for a documentation. When do we need to set it? I
> can see that register_new_memory path doesn't check node id. It is quite
> reasonable to expect that a new memblock doesn't span multiple numa
> nodes which can be the case for register_one_node but a word or two are
> really due.

OK, I will add a comment, and BTW, this is also going to be a separate
patch for ease of review.

>
>>  {
>>       int ret;
>>       unsigned long pfn, sect_start_pfn, sect_end_pfn;
>> @@ -423,11 +424,13 @@ int register_mem_sect_under_node(struct memory_block *mem_blk, int nid)
>>                       continue;
>>               }
>>
>> -             page_nid = get_nid_for_pfn(pfn);
>> -             if (page_nid < 0)
>> -                     continue;
>> -             if (page_nid != nid)
>> -                     continue;
>> +             if (check_nid) {
>> +                     page_nid = get_nid_for_pfn(pfn);
>> +                     if (page_nid < 0)
>> +                             continue;
>> +                     if (page_nid != nid)
>> +                             continue;
>> +             }
>>               ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>>                                       &mem_blk->dev.kobj,
>>                                       kobject_name(&mem_blk->dev.kobj));
>> @@ -502,7 +505,7 @@ int link_mem_sections(int nid, unsigned long start_pfn, unsigned long nr_pages)
>>
>>               mem_blk = find_memory_block_hinted(mem_sect, mem_blk);
>>
>> -             ret = register_mem_sect_under_node(mem_blk, nid);
>> +             ret = register_mem_sect_under_node(mem_blk, nid, true);
>>               if (!err)
>>                       err = ret;
>>
>
> I would be tempted to split this into a separate patch as well. The
> review will be much easier.

Yes, but that would be the last patch in the series.

> This is quite ugly. You allocate 256MB for small numa systems and 512MB
> for larger NUMAs unconditionally for MEMORY_HOTPLUG. I see you need it
> to safely replace page_to_nid by get_section_nid but this is just too
> high of the price. Please note that this shouldn't be really needed. At
> least not for onlining. We already _do_ know the node association with
> the pfn range. So we should be able to get the nid from memblock.

OK, I will think for a different place to store nid temporarily, or
how to get it.

Thank you,
Pavel