archive mirror
 help / color / mirror / Atom feed
From: Vlastimil Babka <>
To: Michal Hocko <>,
	Mel Gorman <>
Cc: Jia He <>,,,
	Andrew Morton <>,
	Johannes Weiner <>,
	Joonsoo Kim <>,
	Taku Izumi <>,
	Andi Kleen <>
Subject: Re: [PATCH RFC 1/1] mm, page_alloc: fix incorrect zone_statistics data
Date: Tue, 20 Dec 2016 15:49:34 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On 12/20/2016 03:35 PM, 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
> 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.

Let's CC Andi who introduced it by commit 78afd5612deb8.
Personally I agree that the reasoning provided by that commit does not
justify the troubles. We already have the HIT and MISS counters to
record if we allocated on the node we explicitly wanted/preferred (local
or remote). The LOCAL and OTHER should thus be true local/remote
statistics, why fake them in some rare cases such as khugepaged? The
only other case is do_huge_pmd_wp_page_fallback() where it perhaps makes
even less sense. No others were added in 5 years so I think the flag
really didn't catch on, let's get rid of it.

  reply	other threads:[~2016-12-20 14:49 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-12  5:59 [PATCH RFC 0/1] " Jia He
2016-12-12  5:59 ` [PATCH RFC 1/1] " Jia He
2016-12-20  9:18   ` Michal Hocko
2016-12-20 13:10     ` Mel Gorman
2016-12-20 13:26       ` Michal Hocko
2016-12-20 14:28         ` Mel Gorman
2016-12-20 14:35           ` Michal Hocko
2016-12-20 14:49             ` Vlastimil Babka [this message]
2016-12-20 14:54             ` Mel Gorman
2016-12-21  7:57               ` Michal Hocko
2016-12-20 14:42           ` Mel Gorman
2016-12-20 15:13     ` Vlastimil Babka
2016-12-21  3:01     ` hejianet
2016-12-20 12:31   ` Mel Gorman
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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH RFC 1/1] mm, page_alloc: fix incorrect zone_statistics data' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).