Re: [PATCH] mm: mempolicy: fix policy_nodemask() for MPOL_PREFERRED_MANY case

[Feng Tang <feng.tang@intel.com>]

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