From: Mel Gorman <mgorman@techsingularity.net> To: Michal Hocko <mhocko@kernel.org> Cc: Jia He <hejianet@gmail.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>, Vlastimil Babka <vbabka@suse.cz>, Johannes Weiner <hannes@cmpxchg.org>, Joonsoo Kim <iamjoonsoo.kim@lge.com>, Taku Izumi <izumi.taku@jp.fujitsu.com> Subject: Re: [PATCH RFC 1/1] mm, page_alloc: fix incorrect zone_statistics data Date: Tue, 20 Dec 2016 14:54:35 +0000 [thread overview] Message-ID: <20161220145435.c3htqyfhpjt5uma7@techsingularity.net> (raw) In-Reply-To: <20161220143501.GI3769@dhcp22.suse.cz> On Tue, Dec 20, 2016 at 03:35:02PM +0100, Michal Hocko wrote: > On Tue 20-12-16 14:28:45, Mel Gorman wrote: > > On Tue, Dec 20, 2016 at 02:26:43PM +0100, Michal Hocko wrote: > > > On Tue 20-12-16 13:10:40, Mel Gorman wrote: > > > > On Tue, Dec 20, 2016 at 10:18:14AM +0100, Michal Hocko wrote: > > > > > On Mon 12-12-16 13:59:07, Jia He wrote: > > > > > > In commit b9f00e147f27 ("mm, page_alloc: reduce branches in > > > > > > zone_statistics"), it reconstructed codes to reduce the branch miss rate. > > > > > > Compared with the original logic, it assumed if !(flag & __GFP_OTHER_NODE) > > > > > > z->node would not be equal to preferred_zone->node. That seems to be > > > > > > incorrect. > > > > > > > > > > I am sorry but I have hard time following the changelog. It is clear > > > > > that you are trying to fix a missed NUMA_{HIT,OTHER} accounting > > > > > but it is not really clear when such thing happens. You are adding > > > > > preferred_zone->node check. preferred_zone is the first zone in the > > > > > requested zonelist. So for the most allocations it is a node from the > > > > > local node. But if something request an explicit numa node (without > > > > > __GFP_OTHER_NODE which would be the majority I suspect) then we could > > > > > indeed end up accounting that as a NUMA_MISS, NUMA_FOREIGN so the > > > > > referenced patch indeed caused an unintended change of accounting AFAIU. > > > > > > > > > > > > > This is a similar concern to what I had. If the preferred zone, which is > > > > the first valid usable zone, is not a "hit" for the statistics then I > > > > don't know what "hit" is meant to mean. > > > > > > But the first valid usable zone is defined based on the requested numa > > > node. Unless the requested node is memoryless then we should have a hit, > > > no? > > > > > > > Should be. If the local node is memoryless then there would be a difference > > between hit and whether it's local or not but that to me is a little > > useless. A local vs remote page allocated has a specific meaning and > > consequence. It's hard to see how hit can be meaningfully interpreted if > > there are memoryless nodes. I don't have a strong objection to the patch > > so I didn't nak it, I'm just not convinced it matters. > > So what do you think about > http://lkml.kernel.org/r/20161220091814.GC3769@dhcp22.suse.cz > This doesn't appear to resolve for me and I've 30 minutes left before being offline for 4 days so didn't go digging. > I think that we should get rid of __GFP_OTHER_NODE thingy. It is just > one off thing and the gfp space it rather precious. > However, broadly speaking, I'd be ok with getting rid of __GFP_OTHER_NODE altogether and making it truely only about local vs remote hits because those are the ones that matter in terms of performance. If a user has memoryless nodes or policies that allow local CPUs but forbid local memory and they need to debug an issue, they're going to need tracepoints anyway. Hit/miss/other is not sufficient for most interesting problems involving local or remote memory usage. -- Mel Gorman SUSE Labs
WARNING: multiple messages have this Message-ID (diff)
From: Mel Gorman <mgorman@techsingularity.net> To: Michal Hocko <mhocko@kernel.org> Cc: Jia He <hejianet@gmail.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>, Vlastimil Babka <vbabka@suse.cz>, Johannes Weiner <hannes@cmpxchg.org>, Joonsoo Kim <iamjoonsoo.kim@lge.com>, Taku Izumi <izumi.taku@jp.fujitsu.com> Subject: Re: [PATCH RFC 1/1] mm, page_alloc: fix incorrect zone_statistics data Date: Tue, 20 Dec 2016 14:54:35 +0000 [thread overview] Message-ID: <20161220145435.c3htqyfhpjt5uma7@techsingularity.net> (raw) In-Reply-To: <20161220143501.GI3769@dhcp22.suse.cz> On Tue, Dec 20, 2016 at 03:35:02PM +0100, Michal Hocko wrote: > On Tue 20-12-16 14:28:45, Mel Gorman wrote: > > On Tue, Dec 20, 2016 at 02:26:43PM +0100, Michal Hocko wrote: > > > On Tue 20-12-16 13:10:40, Mel Gorman wrote: > > > > On Tue, Dec 20, 2016 at 10:18:14AM +0100, Michal Hocko wrote: > > > > > On Mon 12-12-16 13:59:07, Jia He wrote: > > > > > > In commit b9f00e147f27 ("mm, page_alloc: reduce branches in > > > > > > zone_statistics"), it reconstructed codes to reduce the branch miss rate. > > > > > > Compared with the original logic, it assumed if !(flag & __GFP_OTHER_NODE) > > > > > > z->node would not be equal to preferred_zone->node. That seems to be > > > > > > incorrect. > > > > > > > > > > I am sorry but I have hard time following the changelog. It is clear > > > > > that you are trying to fix a missed NUMA_{HIT,OTHER} accounting > > > > > but it is not really clear when such thing happens. You are adding > > > > > preferred_zone->node check. preferred_zone is the first zone in the > > > > > requested zonelist. So for the most allocations it is a node from the > > > > > local node. But if something request an explicit numa node (without > > > > > __GFP_OTHER_NODE which would be the majority I suspect) then we could > > > > > indeed end up accounting that as a NUMA_MISS, NUMA_FOREIGN so the > > > > > referenced patch indeed caused an unintended change of accounting AFAIU. > > > > > > > > > > > > > This is a similar concern to what I had. If the preferred zone, which is > > > > the first valid usable zone, is not a "hit" for the statistics then I > > > > don't know what "hit" is meant to mean. > > > > > > But the first valid usable zone is defined based on the requested numa > > > node. Unless the requested node is memoryless then we should have a hit, > > > no? > > > > > > > Should be. If the local node is memoryless then there would be a difference > > between hit and whether it's local or not but that to me is a little > > useless. A local vs remote page allocated has a specific meaning and > > consequence. It's hard to see how hit can be meaningfully interpreted if > > there are memoryless nodes. I don't have a strong objection to the patch > > so I didn't nak it, I'm just not convinced it matters. > > So what do you think about > http://lkml.kernel.org/r/20161220091814.GC3769@dhcp22.suse.cz > This doesn't appear to resolve for me and I've 30 minutes left before being offline for 4 days so didn't go digging. > I think that we should get rid of __GFP_OTHER_NODE thingy. It is just > one off thing and the gfp space it rather precious. > However, broadly speaking, I'd be ok with getting rid of __GFP_OTHER_NODE altogether and making it truely only about local vs remote hits because those are the ones that matter in terms of performance. If a user has memoryless nodes or policies that allow local CPUs but forbid local memory and they need to debug an issue, they're going to need tracepoints anyway. Hit/miss/other is not sufficient for most interesting problems involving local or remote memory usage. -- Mel Gorman SUSE Labs -- 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-12-20 15:03 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-12-12 5:59 [PATCH RFC 0/1] mm, page_alloc: fix incorrect zone_statistics data Jia He 2016-12-12 5:59 ` Jia He 2016-12-12 5:59 ` [PATCH RFC 1/1] " Jia He 2016-12-12 5:59 ` Jia He 2016-12-20 9:18 ` Michal Hocko 2016-12-20 9:18 ` Michal Hocko 2016-12-20 13:10 ` Mel Gorman 2016-12-20 13:10 ` Mel Gorman 2016-12-20 13:26 ` Michal Hocko 2016-12-20 13:26 ` Michal Hocko 2016-12-20 14:28 ` Mel Gorman 2016-12-20 14:28 ` Mel Gorman 2016-12-20 14:35 ` Michal Hocko 2016-12-20 14:35 ` Michal Hocko 2016-12-20 14:49 ` Vlastimil Babka 2016-12-20 14:49 ` Vlastimil Babka 2016-12-20 14:54 ` Mel Gorman [this message] 2016-12-20 14:54 ` Mel Gorman 2016-12-21 7:57 ` Michal Hocko 2016-12-21 7:57 ` Michal Hocko 2016-12-21 8:06 ` [PATCH 1/2] mm: fix remote numa hits statistics Michal Hocko 2016-12-21 8:06 ` [PATCH 2/2] mm: get rid of __GFP_OTHER_NODE Michal Hocko 2017-01-02 14:18 ` Vlastimil Babka 2016-12-29 11:46 ` [PATCH 1/2] mm: fix remote numa hits statistics Mel Gorman 2016-12-29 12:28 ` Michal Hocko 2017-01-02 14:16 ` Vlastimil Babka 2017-01-02 14:46 ` Michal Hocko 2017-01-02 15:07 ` Vlastimil Babka 2016-12-20 14:42 ` [PATCH RFC 1/1] mm, page_alloc: fix incorrect zone_statistics data Mel Gorman 2016-12-20 14:42 ` Mel Gorman 2016-12-20 15:13 ` Vlastimil Babka 2016-12-20 15:13 ` Vlastimil Babka 2016-12-21 3:01 ` hejianet 2016-12-21 3:01 ` hejianet 2016-12-20 12:31 ` Mel Gorman 2016-12-20 12:31 ` Mel Gorman 2016-12-21 3:07 ` hejianet 2016-12-21 3:07 ` hejianet
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=20161220145435.c3htqyfhpjt5uma7@techsingularity.net \ --to=mgorman@techsingularity.net \ --cc=akpm@linux-foundation.org \ --cc=hannes@cmpxchg.org \ --cc=hejianet@gmail.com \ --cc=iamjoonsoo.kim@lge.com \ --cc=izumi.taku@jp.fujitsu.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@kernel.org \ --cc=vbabka@suse.cz \ /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.