All of lore.kernel.org
 help / color / mirror / Atom feed
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 13:10:40 +0000	[thread overview]
Message-ID: <20161220131040.f5ga5426dduh3mhu@techsingularity.net> (raw)
In-Reply-To: <20161220091814.GC3769@dhcp22.suse.cz>

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.

-- 
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 13:10:40 +0000	[thread overview]
Message-ID: <20161220131040.f5ga5426dduh3mhu@techsingularity.net> (raw)
In-Reply-To: <20161220091814.GC3769@dhcp22.suse.cz>

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.

-- 
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>

  reply	other threads:[~2016-12-20 13:10 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 [this message]
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
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=20161220131040.f5ga5426dduh3mhu@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: link
Be 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.