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> But this having another pair of eyes would be definitely helpful. Still one nit though > @@ -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; > /* Check N_MEMORY */ > nodes_and(nsc->mask1, > cpuset_current_mems_allowed, node_states[N_MEMORY]); -- Michal Hocko SUSE Labs
