From: Vlastimil Babka <vbabka@suse.cz> To: Andrew Morton <akpm@linux-foundation.org> Cc: Joonsoo Kim <js1304@gmail.com>, Aaron Lu <aaron.lu@intel.com>, Mel Gorman <mgorman@suse.de>, Rik van Riel <riel@redhat.com>, David Rientjes <rientjes@google.com>, LKML <linux-kernel@vger.kernel.org>, Linux Memory Management List <linux-mm@kvack.org>, Joonsoo Kim <iamjoonsoo.kim@lge.com> Subject: Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous Date: Wed, 10 Feb 2016 14:42:57 +0100 [thread overview] Message-ID: <56BB3E61.50707@suse.cz> (raw) In-Reply-To: <20160209125301.c7e6067558c321cfb87602b5@linux-foundation.org> On 02/09/2016 09:53 PM, Andrew Morton wrote: > On Tue, 9 Feb 2016 18:58:32 +0100 Vlastimil Babka <vbabka@suse.cz> wrote: > >> On 02/05/2016 05:11 PM, Joonsoo Kim wrote: >>> Yeah, it seems wrong to me. :) >>> Here goes fix. >> >> Doesn't apply for me, even after fixing the most obvious line wraps. >> Seems like the version in mmotm is still your original patch and >> Andrew's hotfix? > > Yes, that patch was hopelessly mailer-mangled. I painstakingly fixed > it up and generated the incremental: Thanks a lot. My review of the final patch also involved pain (due to the cold, not the patch!). You can take my Acked-by, but I also find the definitions of set_zone_contiguous/clear_zone_contiguous() "in the header of the consumer" (hotplug) somewhat unusual. It works, but e.g. mm/internal.h would be more expected. Then there's this: > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -509,6 +509,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn, > int start_sec, end_sec; > struct vmem_altmap *altmap; > > + clear_zone_contiguous(zone); > + > /* during initialize mem_map, align hot-added range to section */ > start_sec = pfn_to_section_nr(phys_start_pfn); > end_sec = pfn_to_section_nr(phys_start_pfn + nr_pages - 1); > @@ -540,6 +542,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn, > } > vmemmap_populate_print_last(); > > + set_zone_contiguous(zone); > + > return err; > } > EXPORT_SYMBOL_GPL(__add_pages); Between the clear and set, __add_pages() might return with -EINVAL, leaving the flag cleared potentially forever. Not critical, probably rare, but it should be possible to avoid this by moving the clear below the altmap check? Thanks, Vlastimil
WARNING: multiple messages have this Message-ID (diff)
From: Vlastimil Babka <vbabka@suse.cz> To: Andrew Morton <akpm@linux-foundation.org> Cc: Joonsoo Kim <js1304@gmail.com>, Aaron Lu <aaron.lu@intel.com>, Mel Gorman <mgorman@suse.de>, Rik van Riel <riel@redhat.com>, David Rientjes <rientjes@google.com>, LKML <linux-kernel@vger.kernel.org>, Linux Memory Management List <linux-mm@kvack.org>, Joonsoo Kim <iamjoonsoo.kim@lge.com> Subject: Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous Date: Wed, 10 Feb 2016 14:42:57 +0100 [thread overview] Message-ID: <56BB3E61.50707@suse.cz> (raw) In-Reply-To: <20160209125301.c7e6067558c321cfb87602b5@linux-foundation.org> On 02/09/2016 09:53 PM, Andrew Morton wrote: > On Tue, 9 Feb 2016 18:58:32 +0100 Vlastimil Babka <vbabka@suse.cz> wrote: > >> On 02/05/2016 05:11 PM, Joonsoo Kim wrote: >>> Yeah, it seems wrong to me. :) >>> Here goes fix. >> >> Doesn't apply for me, even after fixing the most obvious line wraps. >> Seems like the version in mmotm is still your original patch and >> Andrew's hotfix? > > Yes, that patch was hopelessly mailer-mangled. I painstakingly fixed > it up and generated the incremental: Thanks a lot. My review of the final patch also involved pain (due to the cold, not the patch!). You can take my Acked-by, but I also find the definitions of set_zone_contiguous/clear_zone_contiguous() "in the header of the consumer" (hotplug) somewhat unusual. It works, but e.g. mm/internal.h would be more expected. Then there's this: > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -509,6 +509,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn, > int start_sec, end_sec; > struct vmem_altmap *altmap; > > + clear_zone_contiguous(zone); > + > /* during initialize mem_map, align hot-added range to section */ > start_sec = pfn_to_section_nr(phys_start_pfn); > end_sec = pfn_to_section_nr(phys_start_pfn + nr_pages - 1); > @@ -540,6 +542,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn, > } > vmemmap_populate_print_last(); > > + set_zone_contiguous(zone); > + > return err; > } > EXPORT_SYMBOL_GPL(__add_pages); Between the clear and set, __add_pages() might return with -EINVAL, leaving the flag cleared potentially forever. Not critical, probably rare, but it should be possible to avoid this by moving the clear below the altmap check? Thanks, Vlastimil -- 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:[~2016-02-10 13:43 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-02-04 6:19 [PATCH v2 1/3] mm/compaction: fix invalid free_pfn and compact_cached_free_pfn Joonsoo Kim 2016-02-04 6:19 ` Joonsoo Kim 2016-02-04 6:19 ` [PATCH v2 2/3] mm/compaction: pass only pageblock aligned range to pageblock_pfn_to_page Joonsoo Kim 2016-02-04 6:19 ` Joonsoo Kim 2016-02-10 12:52 ` Vlastimil Babka 2016-02-10 12:52 ` Vlastimil Babka 2016-02-04 6:19 ` [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous Joonsoo Kim 2016-02-04 6:19 ` Joonsoo Kim 2016-02-05 0:49 ` Andrew Morton 2016-02-05 0:49 ` Andrew Morton 2016-02-05 16:11 ` Joonsoo Kim 2016-02-05 16:11 ` Joonsoo Kim 2016-02-09 17:58 ` Vlastimil Babka 2016-02-09 17:58 ` Vlastimil Babka 2016-02-09 20:53 ` Andrew Morton 2016-02-09 20:53 ` Andrew Morton 2016-02-10 13:42 ` Vlastimil Babka [this message] 2016-02-10 13:42 ` Vlastimil Babka 2016-02-10 18:58 ` Andrew Morton 2016-02-10 18:58 ` Andrew Morton 2016-02-11 1:58 ` Joonsoo Kim 2016-02-11 1:58 ` Joonsoo Kim 2016-02-14 10:21 ` zhong jiang 2016-02-14 10:21 ` zhong jiang 2016-02-15 2:42 ` Joonsoo Kim 2016-02-15 2:42 ` Joonsoo Kim 2016-02-15 10:06 ` Xishi Qiu 2016-02-15 10:06 ` Xishi Qiu 2016-02-15 14:24 ` Joonsoo Kim 2016-02-15 14:24 ` Joonsoo Kim
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=56BB3E61.50707@suse.cz \ --to=vbabka@suse.cz \ --cc=aaron.lu@intel.com \ --cc=akpm@linux-foundation.org \ --cc=iamjoonsoo.kim@lge.com \ --cc=js1304@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mgorman@suse.de \ --cc=riel@redhat.com \ --cc=rientjes@google.com \ /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.