Re: [v4 PATCH 2/3] mm/mempolicy: don't handle MPOL_LOCAL like a fake MPOL_PREFERRED policy

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

On Thu, Jun 03, 2021 at 09:41:38AM +0200, Michal Hocko wrote:
> On Tue 01-06-21 23:14:51, Feng Tang wrote:
> > MPOL_LOCAL policy has been setup as a real policy, but it is still handled
> > like a faked POL_PREFERRED policy with one internal MPOL_F_LOCAL flag bit
> > set, and there are many places having to judge the real 'prefer' or the
> > 'local' policy, which are quite confusing.
> > 
> > In current code, there are 4 cases that MPOL_LOCAL are used:
> > 1. user specifies 'local' policy
> > 2. user specifies 'prefer' policy, but with empty nodemask
> > 3. system 'default' policy is used
> > 4. 'prefer' policy + valid 'preferred' node with MPOL_F_STATIC_NODES
> >    flag set, and when it is 'rebind' to a nodemask which doesn't
> >    contains the 'preferred' node, it will perform as 'local' policy
> > 
> > So make 'local' a real policy instead of a fake 'prefer' one, and kill
> > MPOL_F_LOCAL bit, which can greatly reduce the confusion for code reading.
> > 
> > For case 4, the logic of mpol_rebind_preferred() is confusing, as Michal
> > Hocko pointed out:
> > 
> > : I do believe that rebinding preferred policy is just bogus and it should
> > : be dropped altogether on the ground that a preference is a mere hint from
> > : userspace where to start the allocation.  Unless I am missing something
> > : cpusets will be always authoritative for the final placement.  The
> > : preferred node just acts as a starting point and it should be really
> > : preserved when cpusets changes.  Otherwise we have a very subtle behavior
> > : corner cases.
> > 
> > So dump all the tricky transformation between 'prefer' and 'local',
> > and just record the new nodemask of rebinding.
> > 
> > Suggested-by: Michal Hocko <mhocko@suse.com>
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
 
Thanks!

> But this having another pair of eyes would be definitely helpful.
> Still one nit though

Yes, more review and suggestions are welcome and appreciated.

> > @@ -234,30 +229,27 @@ static int mpol_set_nodemask(struct mempolicy *pol,
> >  	/* if mode is MPOL_DEFAULT, pol is NULL. This is right. */
> >  	if (pol == NULL)
> >  		return 0;
> > +
> > +	if (pol->mode == MPOL_LOCAL)
> > +		return 0;
> > +
> 
> This would benefit from a comment. The one above pol NULL check is just
> desperately unhelpful. I would go with the following
> 	/*
> 	 * Default (pol==NULL) resp. local memory policies are not a
> 	 * subject of any remapping. They also do not need any special
> 	 * constructor.
> 	 */
> 	if (!pol || pol->mode == MPOL_LOCAL)
> 		return 0;

Thanks for the imporovement. 


Andrew,

Could you help to take the below add-on patch for the 2/3 patch? thanks!

- Feng

---
From f6023fbbc0833eebde525aa4d93fd3a7a09ddb8b Mon Sep 17 00:00:00 2001
From: Feng Tang <feng.tang@intel.com>
Date: Thu, 3 Jun 2021 16:01:22 +0800
Subject: [PATCH] mm/mempolicy: refine code and comments of mpol_set_nodemask

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 mm/mempolicy.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 32ca8fc..304b8f2 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -226,11 +226,12 @@ static int mpol_set_nodemask(struct mempolicy *pol,
 {
 	int ret;
 
-	/* if mode is MPOL_DEFAULT, pol is NULL. This is right. */
-	if (pol == NULL)
-		return 0;
-
-	if (pol->mode == MPOL_LOCAL)
+	/*
+	 * Default (pol==NULL) resp. local memory policies are not a
+	 * subject of any remapping. They also do not need any special
+	 * constructor.
+	 */
+	if (!pol || pol->mode == MPOL_LOCAL)
 		return 0;
 
 	/* Check N_MEMORY */
-- 
2.7.4