Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 4 Aug 2022 01:14:32 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     Michal Hocko <mhocko@suse.com>
CC:     Muchun Song <songmuchun@bytedance.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "bwidawsk@kernel.org" <bwidawsk@kernel.org>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Mike Kravetz <mike.kravetz@oracle.com>
Subject: Re: [PATCH] mm: mempolicy: fix policy_nodemask() for
 MPOL_PREFERRED_MANY case
Message-ID: <Yuqs+BTpfh9/PjtP@feng-clx>
References: <20220801084207.39086-1-songmuchun@bytedance.com>
 <YueXhmiFcI8iw3OI@dhcp22.suse.cz>
 <YuecP/RKXWz7QAs0@feng-skl>
 <YuidPA9knCOoaT0c@FVFYT0MHHV2J>
 <Yui7hWZYMX31ktOr@feng-skl>
 <YujGy8EIeZc1Avc7@FVFYT0MHHV2J>
 <YujUyCIBjFj+FzX5@feng-skl>
 <YujoLQt09Js/sSQL@dhcp22.suse.cz>
 <YuoYkPk+YzdPNvmN@feng-snb>
 <YuolieBmdaIzoS3t@dhcp22.suse.cz>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <YuolieBmdaIzoS3t@dhcp22.suse.cz>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?o4Zo9/lh3qeFroNafkkdvHFqMyMRHlRnfnB4JOtVz07VBVmdVozfB8UVgFjI?=
 =?us-ascii?Q?KZIObBsG1VJenWlvSrvkFLKAU/j9ic9yoZPLwBfi0PcACE9H0kB8pjaSHG0I?=
 =?us-ascii?Q?4k3lP7TKa7q4PL/Ic5dJ0RDn7FZNOaiZ614x2oQpbxmD3h7faP3ffDahF3Jc?=
 =?us-ascii?Q?rtR3DlxoylIaNjGUAVsqH0FaXXhA6DI2dxlfEAzXN/OZdCKFviOjGs1yofRK?=
 =?us-ascii?Q?bkB2CvWnV1MYulaZswhuuuHVYkqgY5JMxETLjy74ZpP4V6xA2qubye+C4lI5?=
 =?us-ascii?Q?VbiTdTHz2sQcZ+zGXyKrjQW91xliPnB2noz8HnWz2Dn0dnWJUxYqXMRyRQB7?=
 =?us-ascii?Q?WhVRq2pG5zDb6Zv7IGnHqGpz2W8RwX+Y9QbNHu7646IyRXJc+idvq/nUBXL5?=
 =?us-ascii?Q?8Wr85shU1lwDMciko17PS1ojxstSI7bTyy70yhBQlTunxko/CmyH6mNHF4hR?=
 =?us-ascii?Q?I9x3SpynPy1zy4mfryJvMzqBeLwdwGYMnKrD78QIqfOg+x/0Koz5HS/rAssx?=
 =?us-ascii?Q?x8KZGj1wQjmn64bf8KYj2p1AnvePzQ9muwD6AT89O+5kI7ZWUF1iQwq7Kqr/?=
 =?us-ascii?Q?jlyjMnQD/evZicbwpEfiPJ3GbigEwT6nbHKHohs+dlKwYXEkAdAWZcyjLjmX?=
 =?us-ascii?Q?/ePoh6i4d2lclotW6uuDVTiLDc3wUeDaKYrB0bT3wo6O08B+fI/G6T68TJv6?=
 =?us-ascii?Q?jqiGHYGWqYv95zqiwc6b/oZlnL8QSBPbswj/MQ/n/P4NXgIoOkjV324+r6E8?=
 =?us-ascii?Q?92/GVcEdtl+N9v0fmEhLWzvrrWY2ODdBIEUcz0PsIoTz+QYZ888nQqm2OEQ3?=
 =?us-ascii?Q?EZ86v4PNkOnwUiT5qGHu6JP8oHYpWKnNIAXEvjYeqqugM1R/+jwoKwoe5XSF?=
 =?us-ascii?Q?C/JFV7+1NFKdI2MaCUt6HEsQ5fe9MyLWXlCN+qDD91eWY2BtLLSdgmuYQ5M/?=
 =?us-ascii?Q?pod8J3swyvooL+/LNcBRNn8V6nXtex4Sz5UmXS7pYe/8OaGtcnxwRvFkQ6ut?=
 =?us-ascii?Q?+fbYNQ2wkG69nVTL404loOWTNIhc2TsH35zS2v1bLlmjxcz0pBnvstkrAG2O?=
 =?us-ascii?Q?TwOdgTRgZBxlFJqyhM15tJW4lJ56t0duD0T/z1mNDrsMk7pJc0OTZCLCXfuQ?=
 =?us-ascii?Q?Gfuiji9+d8p9EL8Z6cfk6FPvGHCb9z7YMkdtLIoAdsU6rvlY31R4GlXJtPoY?=
 =?us-ascii?Q?9Nc1MaSFJTibjY1EfJ2oxaT+nesD4RLxQmPPAJTPkiEhaqIi5fE9jTbp7tOF?=
 =?us-ascii?Q?wBP+p/lNsU45ihhuJ9HkoEXh4nCnrFKYcxvUp4VFP5ra2nJ/xPxaW5BkC7Ao?=
 =?us-ascii?Q?6ZMBFFnKpFrqz5JtBistkXSVz7T7ocy6qoE2YwJpkSbcNEOBVmi96VMHo2Or?=
 =?us-ascii?Q?iX6/Db0OpFc+4HLRam9VkzaoZ/247XBCheGZzYuA2KWH6WNvJPSXS5HyaeXG?=
 =?us-ascii?Q?o3YlWvBEELM0alO+ZlQ7Qq9D2r7nGIgfcKtCDQhwV7s4QdayIhrANy3ePMtg?=
 =?us-ascii?Q?+IpqnpSDrp0Jxr+vPpGBDZp4FD94si5Zi6vYFkLu/Felgxr+6vww8NjKwexS?=
 =?us-ascii?Q?+HZi6uZAlt4cjSqIC5dJy2+FEJ7x/H833PXIpUJC?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0506408b-66b0-4609-a429-08da7530dd91
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Aug 2022 09:16:36.4410
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KKiyCKJG+tVVKfoO124RhooWaSKkOaTc/jMAcVFZgTTdee0oztjiBDoTmITLVYplYYdnO8+xGxx4kukjbAqecQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR11MB1959
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.7 required=5.0 tests=BAYES_00,DATE_IN_FUTURE_06_12,
        DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_HI,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Aug 03, 2022 at 03:36:41PM +0800, Michal Hocko wrote:
> On Wed 03-08-22 14:41:20, Feng Tang wrote:
> > On Tue, Aug 02, 2022 at 05:02:37PM +0800, Michal Hocko wrote:
> > > Please make sure to CC Mike on hugetlb related changes.
> > 
> > OK.
> > 
> > > I didn't really get to grasp your proposed solution but it feels goind
> > > sideways. The real issue is that hugetlb uses a dedicated allocation
> > > scheme which is not fully MPOL_PREFERRED_MANY aware AFAICS. I do not
> > > think we should be tricking that by providing some fake nodemasks and
> > > what not.
> > > 
> > > The good news is that allocation from the pool is MPOL_PREFERRED_MANY
> > > aware because it first tries to allocation from the preffered node mask
> > > and then fall back to the full nodemask (dequeue_huge_page_vma).
> > > If the existing pools cannot really satisfy that allocation then it
> > > tries to allocate a new hugetlb page (alloc_fresh_huge_page) which also
> > > performs 2 stage allocation with the node mask and no node masks. But
> > > both of them might fail.
> > > 
> > > The bad news is that other allocation functions - including those that
> > > allocate to the pool are not fully MPOL_PREFERRED_MANY aware. E.g.
> > > __nr_hugepages_store_common paths which use the allocating process
> > > policy to fill up the pool so the pool could be under provisioned if
> > > that context is using MPOL_PREFERRED_MANY.
> > 
> > Thanks for the check!
> > 
> > So you mean if the prferred nodes don't have enough pages, we should
> > also fallback to all like dequeue_huge_page_vma() does?
> > 
> > Or we can user a policy API which return nodemask for MPOL_BIND and 
> > NULL for all other policies, like allowed_mems_nr() needs.
> > 
> > --- a/include/linux/mempolicy.h
> > +++ b/include/linux/mempolicy.h
> > @@ -158,6 +158,18 @@ static inline nodemask_t *policy_nodemask_current(gfp_t gfp)
> >  	return policy_nodemask(gfp, mpol);
> >  }
> >  
> > +#ifdef CONFIG_HUGETLB_FS
> > +static inline nodemask_t *strict_policy_nodemask_current(void)
> > +{
> > +	struct mempolicy *mpol = get_task_policy(current);
> > +
> > +	if (mpol->mode == MPOL_BIND)
> > +		return &mpol->nodes;
> > +
> > +	return NULL;
> > +}
> > +#endif
> 
> Yes something like this, except that I would also move this into hugetlb
> proper because this doesn't seem generally useful.
  

Ok, I change it as below:

---
 mm/hugetlb.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 14be38822cf8..ef1d4ffa733f 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -91,6 +91,24 @@ struct mutex *hugetlb_fault_mutex_table ____cacheline_aligned_in_smp;
 /* Forward declaration */
 static int hugetlb_acct_memory(struct hstate *h, long delta);
 
+/*
+ * Return nodemask of what is allowed by current process' memory
+ * policy, as MPOL_BIND is the only 'strict' policy, return NULL
+ * for all other policies
+ */
+static inline nodemask_t *allowed_policy_nodemask_current(void)
+{
+#ifdef CONFIG_NUMA
+	struct mempolicy *mpol = get_task_policy(current);
+
+	if (mpol->mode == MPOL_BIND)
+		return &mpol->nodes;
+	return NULL;
+#else
+	return NULL;
+#endif
+}
+
 static inline bool subpool_is_free(struct hugepage_subpool *spool)
 {
 	if (spool->count)
@@ -3556,7 +3574,7 @@ static ssize_t __nr_hugepages_store_common(bool obey_mempolicy,
 					   unsigned long count, size_t len)
 {
 	int err;
-	nodemask_t nodes_allowed, *n_mask;
+	nodemask_t nodes_allowed, *n_mask = NULL;
 
 	if (hstate_is_gigantic(h) && !gigantic_page_runtime_supported())
 		return -EINVAL;
@@ -3565,11 +3583,11 @@ static ssize_t __nr_hugepages_store_common(bool obey_mempolicy,
 		/*
 		 * global hstate attribute
 		 */
-		if (!(obey_mempolicy &&
-				init_nodemask_of_mempolicy(&nodes_allowed)))
+		if (obey_mempolicy)
+			n_mask = allowed_policy_nodemask_current();
+
+		if (!n_mask)
 			n_mask = &node_states[N_MEMORY];
-		else
-			n_mask = &nodes_allowed;
 	} else {
 		/*
 		 * Node specific request.  count adjustment happens in
-- 
2.27.0

> > > Wrt. allowed_mems_nr (i.e. hugetlb_acct_memory) this is a reservation
> > > code and I have to admit I do not really remember details there. This is
> > > a subtle code and my best guess would be that policy_nodemask_current
> > > should be hugetlb specific and only care about MPOL_BIND.
> > 
> > The API needed by allowed_mem_nr() is a little different as it has gfp
> > flag and cpuset config to consider.
> 
> Why would gfp mask matter? 

I'm not very familiar with the old semantics (will check more), from current
code, it checks both the gfp flags and cpuset limit.

Thanks,
Feng

> -- 
> Michal Hocko
> SUSE Labs