| Message ID | 20200916005936.232788-1-ying.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] autonuma: Migrate on fault among multiple bound nodes | expand |
On Wed, Sep 16, 2020 at 08:59:36AM +0800, Huang Ying wrote: > So in this patch, if MPOL_BIND is used to bind the memory of the > application to multiple nodes, and in the hint page fault handler both > the faulting page node and the accessing node are in the policy > nodemask, the page will be tried to be migrated to the accessing node > to reduce the cross-node accessing. Seems fair enough.. > Questions: > > Sysctl knob kernel.numa_balancing can enable/disable AutoNUMA > optimizing globally. And now, it appears that the explicit NUMA > memory policy specifying (e.g. via numactl, mbind(), etc.) acts like > an implicit per-thread/VMA knob to enable/disable the AutoNUMA > optimizing for the thread/VMA. Although this looks like a side effect > instead of an API, from commit fc3147245d19 ("mm: numa: Limit NUMA > scanning to migrate-on-fault VMAs"), this is used by some users? So > the question is, do we need an explicit per-thread/VMA knob to > enable/disable AutoNUMA optimizing for the thread/VMA? Or just use > the global knob, either optimize all thread/VMAs as long as the > explicitly specified memory policies are respected, or don't optimize > at all. I don't understand the question; that commit is not about disabling numa balancing, it's about avoiding pointless work and overhead. What's the point of scanning memory if you're not going to be allowed to move it anyway. > Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Mel Gorman <mgorman@suse.de> > Cc: Rik van Riel <riel@redhat.com> > Cc: Johannes Weiner <hannes@cmpxchg.org> > Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org> > Cc: Dave Hansen <dave.hansen@intel.com> > Cc: Andi Kleen <ak@linux.intel.com> > Cc: Michal Hocko <mhocko@suse.com> > Cc: David Rientjes <rientjes@google.com> > --- > mm/mempolicy.c | 43 +++++++++++++++++++++++++++++++------------ > 1 file changed, 31 insertions(+), 12 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index eddbe4e56c73..a941eab2de24 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -1827,6 +1827,13 @@ static struct mempolicy *get_vma_policy(struct vm_area_struct *vma, > return pol; > } > > +static bool mpol_may_mof(struct mempolicy *pol) > +{ > + /* May migrate among bound nodes for MPOL_BIND */ > + return pol->flags & MPOL_F_MOF || > + (pol->mode == MPOL_BIND && nodes_weight(pol->v.nodes) > 1); > +} This is weird, why not just set F_MOF on the policy? In fact, why wouldn't something like: mbind(.mode=MPOL_BIND, .flags=MPOL_MF_LAZY); work today? Afaict MF_LAZY will unconditionally result in M_MOF. > @@ -2494,20 +2503,30 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long > break; > > case MPOL_BIND: > /* > + * Allows binding to multiple nodes. If both current and > + * accessing nodes are in policy nodemask, migrate to > + * accessing node to optimize page placement. Otherwise, > + * use current page if in policy nodemask or MPOL_F_MOF not > + * set, else select nearest allowed node, if any. If no > + * allowed nodes, use current [!misplaced]. > */ > + if (node_isset(curnid, pol->v.nodes)) { > + if (node_isset(thisnid, pol->v.nodes)) { > + moron = true; > + polnid = thisnid; > + } else { > + goto out; > + } > + } else if (!(pol->flags & MPOL_F_MOF)) { > goto out; > + } else { > + z = first_zones_zonelist( > node_zonelist(numa_node_id(), GFP_HIGHUSER), > gfp_zone(GFP_HIGHUSER), > &pol->v.nodes); > + polnid = zone_to_nid(z->zone); > + } > break; > > default: Did that want to be this instead? I don't think I follow the other changes. diff --git a/mm/mempolicy.c b/mm/mempolicy.c index eddbe4e56c73..2a64913f9ac6 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -2501,8 +2501,11 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long * else select nearest allowed node, if any. * If no allowed nodes, use current [!misplaced]. */ - if (node_isset(curnid, pol->v.nodes)) + if (node_isset(curnid, pol->v.nodes)) { + if (node_isset(thisnod, pol->v.nodes)) + goto moron; goto out; + } z = first_zones_zonelist( node_zonelist(numa_node_id(), GFP_HIGHUSER), gfp_zone(GFP_HIGHUSER), @@ -2516,6 +2519,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long /* Migrate the page towards the node whose CPU is referencing it */ if (pol->flags & MPOL_F_MORON) { +moron: polnid = thisnid; if (!should_numa_migrate_memory(current, page, curnid, thiscpu))
Hi, Peter, Thanks for comments! peterz@infradead.org writes: > On Wed, Sep 16, 2020 at 08:59:36AM +0800, Huang Ying wrote: > >> So in this patch, if MPOL_BIND is used to bind the memory of the >> application to multiple nodes, and in the hint page fault handler both >> the faulting page node and the accessing node are in the policy >> nodemask, the page will be tried to be migrated to the accessing node >> to reduce the cross-node accessing. > > Seems fair enough.. > >> Questions: >> >> Sysctl knob kernel.numa_balancing can enable/disable AutoNUMA >> optimizing globally. And now, it appears that the explicit NUMA >> memory policy specifying (e.g. via numactl, mbind(), etc.) acts like >> an implicit per-thread/VMA knob to enable/disable the AutoNUMA >> optimizing for the thread/VMA. Although this looks like a side effect >> instead of an API, from commit fc3147245d19 ("mm: numa: Limit NUMA >> scanning to migrate-on-fault VMAs"), this is used by some users? So >> the question is, do we need an explicit per-thread/VMA knob to >> enable/disable AutoNUMA optimizing for the thread/VMA? Or just use >> the global knob, either optimize all thread/VMAs as long as the >> explicitly specified memory policies are respected, or don't optimize >> at all. > > I don't understand the question; that commit is not about disabling numa > balancing, it's about avoiding pointless work and overhead. What's the > point of scanning memory if you're not going to be allowed to move it > anyway. Because we are going to enable the moving, this makes scanning not pointless, but may also introduce overhead. >> Signed-off-by: "Huang, Ying" <ying.huang@intel.com> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: Mel Gorman <mgorman@suse.de> >> Cc: Rik van Riel <riel@redhat.com> >> Cc: Johannes Weiner <hannes@cmpxchg.org> >> Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org> >> Cc: Dave Hansen <dave.hansen@intel.com> >> Cc: Andi Kleen <ak@linux.intel.com> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: David Rientjes <rientjes@google.com> >> --- >> mm/mempolicy.c | 43 +++++++++++++++++++++++++++++++------------ >> 1 file changed, 31 insertions(+), 12 deletions(-) >> >> diff --git a/mm/mempolicy.c b/mm/mempolicy.c >> index eddbe4e56c73..a941eab2de24 100644 >> --- a/mm/mempolicy.c >> +++ b/mm/mempolicy.c >> @@ -1827,6 +1827,13 @@ static struct mempolicy *get_vma_policy(struct vm_area_struct *vma, >> return pol; >> } >> >> +static bool mpol_may_mof(struct mempolicy *pol) >> +{ >> + /* May migrate among bound nodes for MPOL_BIND */ >> + return pol->flags & MPOL_F_MOF || >> + (pol->mode == MPOL_BIND && nodes_weight(pol->v.nodes) > 1); >> +} > > This is weird, why not just set F_MOF on the policy? > > In fact, why wouldn't something like: > > mbind(.mode=MPOL_BIND, .flags=MPOL_MF_LAZY); > > work today? Afaict MF_LAZY will unconditionally result in M_MOF. There are some subtle difference. - LAZY appears unnecessary for the per-task memory policy via set_mempolicy(). While migrating among multiple bound nodes appears reasonable as a per-task memory policy. - LAZY also means move the pages not on the bound nodes to the bound nodes if the memory is available. Some users may want to do that only if should_numa_migrate_memory() returns true. >> @@ -2494,20 +2503,30 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long >> break; >> >> case MPOL_BIND: >> /* >> + * Allows binding to multiple nodes. If both current and >> + * accessing nodes are in policy nodemask, migrate to >> + * accessing node to optimize page placement. Otherwise, >> + * use current page if in policy nodemask or MPOL_F_MOF not >> + * set, else select nearest allowed node, if any. If no >> + * allowed nodes, use current [!misplaced]. >> */ >> + if (node_isset(curnid, pol->v.nodes)) { >> + if (node_isset(thisnid, pol->v.nodes)) { >> + moron = true; >> + polnid = thisnid; >> + } else { >> + goto out; >> + } >> + } else if (!(pol->flags & MPOL_F_MOF)) { >> goto out; >> + } else { >> + z = first_zones_zonelist( >> node_zonelist(numa_node_id(), GFP_HIGHUSER), >> gfp_zone(GFP_HIGHUSER), >> &pol->v.nodes); >> + polnid = zone_to_nid(z->zone); >> + } >> break; >> >> default: > > Did that want to be this instead? I don't think I follow the other > changes. > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index eddbe4e56c73..2a64913f9ac6 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -2501,8 +2501,11 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long > * else select nearest allowed node, if any. > * If no allowed nodes, use current [!misplaced]. > */ > - if (node_isset(curnid, pol->v.nodes)) > + if (node_isset(curnid, pol->v.nodes)) { > + if (node_isset(thisnod, pol->v.nodes)) > + goto moron; > goto out; > + } > z = first_zones_zonelist( > node_zonelist(numa_node_id(), GFP_HIGHUSER), > gfp_zone(GFP_HIGHUSER), > @@ -2516,6 +2519,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long > > /* Migrate the page towards the node whose CPU is referencing it */ > if (pol->flags & MPOL_F_MORON) { > +moron: > polnid = thisnid; > > if (!should_numa_migrate_memory(current, page, curnid, thiscpu)) Yes. This looks better if we can just use F_MOF. Best Regards, Huang, Ying
On Wed, 2020-09-16 at 08:59 +0800, Huang Ying wrote: > static int apply_policy_zone(struct mempolicy *policy, enum zone_type zone) > @@ -2474,11 +2481,13 @@ int mpol_misplaced(struct page *page, struct > vm_area_struct *vma, unsigned long > int thisnid = cpu_to_node(thiscpu); > int polnid = NUMA_NO_NODE; > int ret = -1; > + bool moron; Are you really going to use that name those days?
On 16.09.20 15:39, Qian Cai wrote: > On Wed, 2020-09-16 at 08:59 +0800, Huang Ying wrote: >> static int apply_policy_zone(struct mempolicy *policy, enum zone_type zone) >> @@ -2474,11 +2481,13 @@ int mpol_misplaced(struct page *page, struct >> vm_area_struct *vma, unsigned long >> int thisnid = cpu_to_node(thiscpu); >> int polnid = NUMA_NO_NODE; >> int ret = -1; >> + bool moron; > > Are you really going to use that name those days? > > include/uapi/linux/mempolicy.h:#define MPOL_F_MORON (1 << 4) /* Migrate On protnone Reference On Node */ Not commenting the decision for that name. It's uapi ... and naming the variable like the uapi flag seems to be a sane thing to do ... hmmm ...
On Wed, 2020-09-16 at 17:29 +0200, David Hildenbrand wrote: > On 16.09.20 15:39, Qian Cai wrote: > > On Wed, 2020-09-16 at 08:59 +0800, Huang Ying wrote: > > > static int apply_policy_zone(struct mempolicy *policy, enum zone_type > > > zone) > > > @@ -2474,11 +2481,13 @@ int mpol_misplaced(struct page *page, struct > > > vm_area_struct *vma, unsigned long > > > int thisnid = cpu_to_node(thiscpu); > > > int polnid = NUMA_NO_NODE; > > > int ret = -1; > > > + bool moron; > > > > Are you really going to use that name those days? > > > > > > include/uapi/linux/mempolicy.h:#define MPOL_F_MORON (1 << 4) /* > Migrate On protnone Reference On Node */ > > Not commenting the decision for that name. It's uapi ... and naming the > variable like the uapi flag seems to be a sane thing to do ... hmmm ... One can argue there is no need to enforce that past decision today. Also, it could be total different thing with a prefix and all capital letters (correct for abbreviation). Anyway, not going to insist on it at all.
peterz@infradead.org writes: > On Wed, Sep 16, 2020 at 08:59:36AM +0800, Huang Ying wrote: >> +static bool mpol_may_mof(struct mempolicy *pol) >> +{ >> + /* May migrate among bound nodes for MPOL_BIND */ >> + return pol->flags & MPOL_F_MOF || >> + (pol->mode == MPOL_BIND && nodes_weight(pol->v.nodes) > 1); >> +} > > This is weird, why not just set F_MOF on the policy? > > In fact, why wouldn't something like: > > mbind(.mode=MPOL_BIND, .flags=MPOL_MF_LAZY); > > work today? Afaict MF_LAZY will unconditionally result in M_MOF. Another question. This means for all VMAs that are mbind() without MPOL_MF_LAZY and tasks which binds memory via set_mempolicy(), we will not try to optimize their page placement among the bound nodes even if sysctl knob kernel.numa_balancing is enabled. Is this the intended behavior? Although we enable AutoNUMA globally, we will not try to use it in any places if possible. In some places, it needs to be enabled again. Best Regards, Huang, Ying
On Wed, Sep 16, 2020 at 05:29:41PM +0200, David Hildenbrand wrote: > On 16.09.20 15:39, Qian Cai wrote: > > On Wed, 2020-09-16 at 08:59 +0800, Huang Ying wrote: > >> static int apply_policy_zone(struct mempolicy *policy, enum zone_type zone) > >> @@ -2474,11 +2481,13 @@ int mpol_misplaced(struct page *page, struct > >> vm_area_struct *vma, unsigned long > >> int thisnid = cpu_to_node(thiscpu); > >> int polnid = NUMA_NO_NODE; > >> int ret = -1; > >> + bool moron; > > > > Are you really going to use that name those days? > > > > > > include/uapi/linux/mempolicy.h:#define MPOL_F_MORON (1 << 4) /* > Migrate On protnone Reference On Node */ > > Not commenting the decision for that name. It's uapi ... and naming the > variable like the uapi flag seems to be a sane thing to do ... hmmm ... Perhaps we could migrate to mopron / MPOL_F_MOPRON?
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index eddbe4e56c73..a941eab2de24 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -1827,6 +1827,13 @@ static struct mempolicy *get_vma_policy(struct vm_area_struct *vma, return pol; } +static bool mpol_may_mof(struct mempolicy *pol) +{ + /* May migrate among bound nodes for MPOL_BIND */ + return pol->flags & MPOL_F_MOF || + (pol->mode == MPOL_BIND && nodes_weight(pol->v.nodes) > 1); +} + bool vma_policy_mof(struct vm_area_struct *vma) { struct mempolicy *pol; @@ -1835,7 +1842,7 @@ bool vma_policy_mof(struct vm_area_struct *vma) bool ret = false; pol = vma->vm_ops->get_policy(vma, vma->vm_start); - if (pol && (pol->flags & MPOL_F_MOF)) + if (pol && mpol_may_mof(pol)) ret = true; mpol_cond_put(pol); @@ -1846,7 +1853,7 @@ bool vma_policy_mof(struct vm_area_struct *vma) if (!pol) pol = get_task_policy(current); - return pol->flags & MPOL_F_MOF; + return mpol_may_mof(pol); } static int apply_policy_zone(struct mempolicy *policy, enum zone_type zone) @@ -2474,11 +2481,13 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long int thisnid = cpu_to_node(thiscpu); int polnid = NUMA_NO_NODE; int ret = -1; + bool moron; pol = get_vma_policy(vma, addr); - if (!(pol->flags & MPOL_F_MOF)) + if (!mpol_may_mof(pol)) goto out; + moron = pol->flags & MPOL_F_MORON; switch (pol->mode) { case MPOL_INTERLEAVE: pgoff = vma->vm_pgoff; @@ -2494,20 +2503,30 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long break; case MPOL_BIND: - /* - * allows binding to multiple nodes. - * use current page if in policy nodemask, - * else select nearest allowed node, if any. - * If no allowed nodes, use current [!misplaced]. + * Allows binding to multiple nodes. If both current and + * accessing nodes are in policy nodemask, migrate to + * accessing node to optimize page placement. Otherwise, + * use current page if in policy nodemask or MPOL_F_MOF not + * set, else select nearest allowed node, if any. If no + * allowed nodes, use current [!misplaced]. */ - if (node_isset(curnid, pol->v.nodes)) + if (node_isset(curnid, pol->v.nodes)) { + if (node_isset(thisnid, pol->v.nodes)) { + moron = true; + polnid = thisnid; + } else { + goto out; + } + } else if (!(pol->flags & MPOL_F_MOF)) { goto out; - z = first_zones_zonelist( + } else { + z = first_zones_zonelist( node_zonelist(numa_node_id(), GFP_HIGHUSER), gfp_zone(GFP_HIGHUSER), &pol->v.nodes); - polnid = zone_to_nid(z->zone); + polnid = zone_to_nid(z->zone); + } break; default: @@ -2515,7 +2534,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long } /* Migrate the page towards the node whose CPU is referencing it */ - if (pol->flags & MPOL_F_MORON) { + if (moron) { polnid = thisnid; if (!should_numa_migrate_memory(current, page, curnid, thiscpu))
Now, AutoNUMA can only optimize the page placement among the NUMA nodes if the default memory policy is used. Because the memory policy specified explicitly should take precedence. But this seems too strict in some situations. For example, on a system with 4 NUMA nodes, if the memory of an application is bound to the node 0 and 1, AutoNUMA can potentially migrate the pages between the node 0 and 1 to reduce cross-node accessing without breaking the explicit memory binding policy. So in this patch, if MPOL_BIND is used to bind the memory of the application to multiple nodes, and in the hint page fault handler both the faulting page node and the accessing node are in the policy nodemask, the page will be tried to be migrated to the accessing node to reduce the cross-node accessing. Questions: Sysctl knob kernel.numa_balancing can enable/disable AutoNUMA optimizing globally. And now, it appears that the explicit NUMA memory policy specifying (e.g. via numactl, mbind(), etc.) acts like an implicit per-thread/VMA knob to enable/disable the AutoNUMA optimizing for the thread/VMA. Although this looks like a side effect instead of an API, from commit fc3147245d19 ("mm: numa: Limit NUMA scanning to migrate-on-fault VMAs"), this is used by some users? So the question is, do we need an explicit per-thread/VMA knob to enable/disable AutoNUMA optimizing for the thread/VMA? Or just use the global knob, either optimize all thread/VMAs as long as the explicitly specified memory policies are respected, or don't optimize at all. Signed-off-by: "Huang, Ying" <ying.huang@intel.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Mel Gorman <mgorman@suse.de> Cc: Rik van Riel <riel@redhat.com> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org> Cc: Dave Hansen <dave.hansen@intel.com> Cc: Andi Kleen <ak@linux.intel.com> Cc: Michal Hocko <mhocko@suse.com> Cc: David Rientjes <rientjes@google.com> --- mm/mempolicy.c | 43 +++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-)