Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
From: Vlastimil Babka <vbabka@suse.cz>
To: Michal Hocko <mhocko@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: "Kirill A. Shutemov" <kirill@shutemov.name>,
	Mel Gorman <mgorman@suse.de>,
	David Rientjes <rientjes@google.com>,
	Andrea Argangeli <andrea@kernel.org>,
	Zi Yan <zi.yan@cs.rutgers.edu>,
	Stefan Priebe - Profihost AG <s.priebe@profihost.ag>,
	linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] mm, thp: consolidate THP gfp handling into alloc_hugepage_direct_gfpmask
Date: Mon, 22 Oct 2018 15:27:54 +0200
Message-ID: <583b20e5-4925-e175-1533-5c2d2bab9192@suse.cz> (raw)
In-Reply-To: <20181019080657.GJ18839@dhcp22.suse.cz>

On 10/19/18 10:06 AM, Michal Hocko wrote:
> On Thu 18-10-18 19:11:47, Andrew Morton wrote:
>> On Wed, 26 Sep 2018 16:22:27 +0200 Michal Hocko <mhocko@kernel.org> wrote:
>>
>>>> MPOL_PREFERRED is handled by policy_node() before we call __alloc_pages_nodemask.
>>>> __GFP_THISNODE is applied only when we are not using
>>>> __GFP_DIRECT_RECLAIM which is handled in alloc_hugepage_direct_gfpmask
>>>> now.
>>>> Lastly MPOL_BIND wasn't handled explicitly but in the end the removed
>>>> late check would remove __GFP_THISNODE for it as well. So in the end we
>>>> are doing the same thing unless I miss something
>>>
>>> Forgot to add. One notable exception would be that the previous code
>>> would allow to hit
>>> 	WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));
>>> in policy_node if the requested node (e.g. cpu local one) was outside of
>>> the mbind nodemask. This is not possible now. We haven't heard about any
>>> such warning yet so it is unlikely that it happens though.
>>
>> Perhaps a changelog addition is needed to cover the above?
> 
> : THP allocation mode is quite complex and it depends on the defrag
> : mode. This complexity is hidden in alloc_hugepage_direct_gfpmask from a
> : large part currently. The NUMA special casing (namely __GFP_THISNODE) is
> : however independent and placed in alloc_pages_vma currently. This both
> : adds an unnecessary branch to all vma based page allocation requests and
> : it makes the code more complex unnecessarily as well. Not to mention
> : that e.g. shmem THP used to do the node reclaiming unconditionally
> : regardless of the defrag mode until recently. This was not only
> : unexpected behavior but it was also hardly a good default behavior and I
> : strongly suspect it was just a side effect of the code sharing more than
> : a deliberate decision which suggests that such a layering is wrong.
> : 
> : Moreover the oriinal code allowed to trigger
> : 	WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));
> : in policy_node if the requested node (e.g. cpu local one) was outside of
> : the mbind nodemask. This is not possible now. We haven't heard about any
> : such warning yet so it is unlikely that it happens but still a signal of
> : a wrong code layering.

Ah, as I said in the other mail, I think it's inaccurate, the warning
was not possible to hit.

There's also a slight difference wrt MPOL_BIND. The previous code would
avoid using __GFP_THISNODE if the local node was outside of
policy_nodemask(). After your patch __GFP_THISNODE is avoided for all
MPOL_BIND policies. So there's a difference that if local node is
actually allowed by the bind policy's nodemask, previously
__GFP_THISNODE would be added, but now it won't be. I don't think it
matters that much though, but maybe the changelog could say that
(instead of the inaccurate note about warning). Note the other policy
where nodemask is relevant is MPOL_INTERLEAVE, and that's unchanged by
this patch.

When that's addressed, you can add

Acked-by: Vlastimil Babka <vbabka@suse.cz>

(Note I also agree with patch 1/2 but didn't think it was useful to
formally ack it on top of Mel's ack supported by actual measurements, as
we're all from the same company).

> : Get rid of the thp special casing from alloc_pages_vma and move the logic
> : to alloc_hugepage_direct_gfpmask. __GFP_THISNODE is applied to
> : the resulting gfp mask only when the direct reclaim is not requested and
> : when there is no explicit numa binding to preserve the current logic.
> : 
> : This allows for removing alloc_hugepage_vma as well.
> 
> Better?
>  
>> I assume that David's mbind() concern has gone away.
> 
> Either I've misunderstood it or it was not really a real issue.
> 

  reply index

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-25 12:03 [PATCH 0/2] thp nodereclaim fixes Michal Hocko
2018-09-25 12:03 ` [PATCH 1/2] mm: thp: relax __GFP_THISNODE for MADV_HUGEPAGE mappings Michal Hocko
2018-09-25 12:20   ` Mel Gorman
2018-09-25 12:30     ` Michal Hocko
2018-10-04 20:16   ` David Rientjes
2018-10-04 21:10     ` Andrea Arcangeli
2018-10-04 23:05       ` David Rientjes
2018-10-06  3:19         ` Andrea Arcangeli
2018-10-05  7:38     ` Mel Gorman
2018-10-05 20:35       ` David Rientjes
2018-10-05 23:21         ` Andrea Arcangeli
2018-10-08 20:41           ` David Rientjes
2018-10-09  9:48             ` Mel Gorman
2018-10-09 12:27               ` Michal Hocko
2018-10-09 13:00                 ` Mel Gorman
2018-10-09 14:25                   ` Michal Hocko
2018-10-09 15:16                     ` Mel Gorman
2018-10-09 23:03                     ` Andrea Arcangeli
2018-10-10 21:19                       ` David Rientjes
2018-10-15 22:30                         ` David Rientjes
2018-10-15 22:44                           ` Andrew Morton
2018-10-15 23:19                             ` Andrea Arcangeli
2018-10-22 20:54                               ` David Rientjes
2018-10-16  7:46                             ` Mel Gorman
2018-10-16 22:37                               ` Andrew Morton
2018-10-16 23:11                                 ` Andrea Arcangeli
2018-10-16 23:16                                   ` Andrew Morton
2018-10-17  7:08                                     ` Michal Hocko
2018-10-17  9:00                                 ` Mel Gorman
2018-10-22 21:04                               ` David Rientjes
2018-10-23  1:27                                 ` Zi Yan
2018-10-28 21:45                                   ` David Rientjes
2018-10-23  7:57                                 ` Mel Gorman
2018-10-23  8:38                                   ` Mel Gorman
2018-10-15 22:57                           ` Andrea Arcangeli
2018-10-22 20:45                             ` David Rientjes
2018-10-09 22:17               ` David Rientjes
2018-10-09 22:51                 ` Andrea Arcangeli
2018-10-10  7:54                   ` Vlastimil Babka
2018-10-10 21:00                   ` David Rientjes
2018-10-09 13:08             ` Vlastimil Babka
2018-10-09 22:21             ` Andrea Arcangeli
2018-10-29  5:17   ` Balbir Singh
2018-10-29  9:00     ` Michal Hocko
2018-10-29  9:42       ` Balbir Singh
2018-10-29 10:08         ` Michal Hocko
2018-10-29 10:56           ` Andrea Arcangeli
2018-09-25 12:03 ` [PATCH 2/2] mm, thp: consolidate THP gfp handling into alloc_hugepage_direct_gfpmask Michal Hocko
2018-09-26 13:30   ` Kirill A. Shutemov
2018-09-26 14:17     ` Michal Hocko
2018-09-26 14:22       ` Michal Hocko
2018-10-19  2:11         ` Andrew Morton
2018-10-19  8:06           ` Michal Hocko
2018-10-22 13:27             ` Vlastimil Babka [this message]
2018-10-24 23:17               ` Andrew Morton
2018-10-25  4:56                 ` Vlastimil Babka
2018-10-25 16:14                   ` Michal Hocko
2018-10-25 16:18                     ` Andrew Morton
2018-10-25 16:45                       ` Michal Hocko
2018-10-22 13:15         ` Vlastimil Babka
2018-10-22 13:30           ` Michal Hocko
2018-10-22 13:35             ` Vlastimil Babka
2018-10-22 13:46               ` Michal Hocko
2018-10-22 13:53                 ` Vlastimil Babka
2018-10-04 20:17     ` David Rientjes
2018-10-04 21:49       ` Zi Yan
2018-10-09 12:36       ` Michal Hocko
2018-09-26 13:08 ` linux-mm@ archive on lore.kernel.org (Was: [PATCH 0/2] thp nodereclaim fixes) Kirill A. Shutemov
2018-09-26 13:14   ` Michal Hocko
2018-09-26 22:22     ` Andrew Morton
2018-09-26 23:08       ` Mel Gorman
2018-09-27  0:47         ` Konstantin Ryabitsev
2018-09-26 15:25   ` Konstantin Ryabitsev
2018-09-27 11:30     ` Kirill A. Shutemov

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=583b20e5-4925-e175-1533-5c2d2bab9192@suse.cz \
    --to=vbabka@suse.cz \
    --cc=akpm@linux-foundation.org \
    --cc=andrea@kernel.org \
    --cc=kirill@shutemov.name \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@suse.de \
    --cc=mhocko@kernel.org \
    --cc=rientjes@google.com \
    --cc=s.priebe@profihost.ag \
    --cc=zi.yan@cs.rutgers.edu \
    /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

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git