Hi, Peter,
Thanks for comments!
peterz@xxxxxxxxxxxxx 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@xxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: Mel Gorman <mgorman@xxxxxxx>
>> Cc: Rik van Riel <riel@xxxxxxxxxx>
>> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
>> Cc: "Matthew Wilcox (Oracle)" <willy@xxxxxxxxxxxxx>
>> Cc: Dave Hansen <dave.hansen@xxxxxxxxx>
>> Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxx>
>> Cc: David Rientjes <rientjes@xxxxxxxxxx>
>> ---
>> 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
