From: Pavel Tatashin <pasha.tatashin@oracle.com> To: Michal Hocko <mhocko@kernel.org> Cc: Steve Sistare <steven.sistare@oracle.com>, Daniel Jordan <daniel.m.jordan@oracle.com>, Andrew Morton <akpm@linux-foundation.org>, Mel Gorman <mgorman@techsingularity.net>, Linux Memory Management List <linux-mm@kvack.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Vlastimil Babka <vbabka@suse.cz>, Bharata B Rao <bharata@linux.vnet.ibm.com>, Thomas Gleixner <tglx@linutronix.de>, mingo@redhat.com, hpa@zytor.com, x86@kernel.org, dan.j.williams@intel.com, kirill.shutemov@linux.intel.com, bhe@redhat.com Subject: Re: [PATCH v3 4/4] mm/memory_hotplug: optimize memory hotplug Date: Thu, 15 Feb 2018 08:46:33 -0500 [thread overview] Message-ID: <CAOAebxsf21pKsHoJQ7+5mWnfj=TA_Nd2h=YvuEfj=SmpFfvjxQ@mail.gmail.com> (raw) In-Reply-To: <20180215124320.GE7275@dhcp22.suse.cz> > 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
WARNING: multiple messages have this Message-ID (diff)
From: Pavel Tatashin <pasha.tatashin@oracle.com> To: Michal Hocko <mhocko@kernel.org> Cc: Steve Sistare <steven.sistare@oracle.com>, Daniel Jordan <daniel.m.jordan@oracle.com>, Andrew Morton <akpm@linux-foundation.org>, Mel Gorman <mgorman@techsingularity.net>, Linux Memory Management List <linux-mm@kvack.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Vlastimil Babka <vbabka@suse.cz>, Bharata B Rao <bharata@linux.vnet.ibm.com>, Thomas Gleixner <tglx@linutronix.de>, mingo@redhat.com, hpa@zytor.com, x86@kernel.org, dan.j.williams@intel.com, kirill.shutemov@linux.intel.com, bhe@redhat.com Subject: Re: [PATCH v3 4/4] mm/memory_hotplug: optimize memory hotplug Date: Thu, 15 Feb 2018 08:46:33 -0500 [thread overview] Message-ID: <CAOAebxsf21pKsHoJQ7+5mWnfj=TA_Nd2h=YvuEfj=SmpFfvjxQ@mail.gmail.com> (raw) In-Reply-To: <20180215124320.GE7275@dhcp22.suse.cz> > 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2018-02-15 13:46 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-13 19:31 [PATCH v3 0/4] optimize memory hotplug Pavel Tatashin 2018-02-13 19:31 ` Pavel Tatashin 2018-02-13 19:31 ` [PATCH v3 1/4] mm/memory_hotplug: enforce block size aligned range check Pavel Tatashin 2018-02-13 19:31 ` Pavel Tatashin 2018-02-15 11:34 ` Michal Hocko 2018-02-15 11:34 ` Michal Hocko 2018-02-15 13:36 ` Pavel Tatashin 2018-02-15 13:36 ` Pavel Tatashin 2018-02-15 14:40 ` Michal Hocko 2018-02-15 14:40 ` Michal Hocko 2018-02-15 15:05 ` Pavel Tatashin 2018-02-15 15:05 ` Pavel Tatashin 2018-02-13 19:31 ` [PATCH v3 2/4] x86/mm/memory_hotplug: determine block size based on the end of boot memory Pavel Tatashin 2018-02-13 19:31 ` Pavel Tatashin 2018-02-15 11:37 ` Michal Hocko 2018-02-15 11:37 ` Michal Hocko 2018-02-15 13:39 ` Pavel Tatashin 2018-02-15 13:39 ` Pavel Tatashin 2018-02-15 19:00 ` Ingo Molnar 2018-02-15 19:00 ` Ingo Molnar 2018-02-13 19:31 ` [PATCH v3 3/4] mm: uninitialized struct page poisoning sanity checking Pavel Tatashin 2018-02-13 19:31 ` Pavel Tatashin 2018-02-15 11:53 ` Michal Hocko 2018-02-15 11:53 ` Michal Hocko 2018-02-15 13:41 ` Pavel Tatashin 2018-02-15 13:41 ` Pavel Tatashin 2018-02-13 19:31 ` [PATCH v3 4/4] mm/memory_hotplug: optimize memory hotplug Pavel Tatashin 2018-02-13 19:31 ` Pavel Tatashin 2018-02-15 12:43 ` Michal Hocko 2018-02-15 12:43 ` Michal Hocko 2018-02-15 13:46 ` Pavel Tatashin [this message] 2018-02-15 13:46 ` Pavel Tatashin 2018-02-13 21:53 ` [PATCH v3 0/4] " Andrew Morton 2018-02-13 21:53 ` Andrew Morton 2018-02-14 8:09 ` Ingo Molnar 2018-02-14 8:09 ` Ingo Molnar 2018-02-14 14:14 ` Pavel Tatashin 2018-02-14 14:14 ` Pavel Tatashin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAOAebxsf21pKsHoJQ7+5mWnfj=TA_Nd2h=YvuEfj=SmpFfvjxQ@mail.gmail.com' \ --to=pasha.tatashin@oracle.com \ --cc=akpm@linux-foundation.org \ --cc=bharata@linux.vnet.ibm.com \ --cc=bhe@redhat.com \ --cc=dan.j.williams@intel.com \ --cc=daniel.m.jordan@oracle.com \ --cc=gregkh@linuxfoundation.org \ --cc=hpa@zytor.com \ --cc=kirill.shutemov@linux.intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mgorman@techsingularity.net \ --cc=mhocko@kernel.org \ --cc=mingo@redhat.com \ --cc=steven.sistare@oracle.com \ --cc=tglx@linutronix.de \ --cc=vbabka@suse.cz \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.