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@xxxxxxxx>
> > Signed-off-by: Feng Tang <feng.tang@xxxxxxxxx>
>
> Acked-by: Michal Hocko <mhocko@xxxxxxxx>
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@xxxxxxxxx>
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@xxxxxxxxx>
---
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
