On Thu, Jun 03, 2021 at 09:41:38AM +0200, Michal Hocko wrote:

On Tue 01-06-21 23:14:51, Feng Tang wrote:

quoted

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 <redacted>

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.

quoted

@@ -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 <redacted>
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 <redacted>
---
 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