Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Oct 2022 12:45:06 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Mike Kravetz <mike.kravetz@oracle.com>
cc:     Albert Huang <huangjie.albert@bytedance.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Andi Kleen <ak@linux.intel.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm: hugetlb: support get/set_policy for hugetlb_vm_ops
In-Reply-To: <20221012081526.73067-1-huangjie.albert@bytedance.com>
Message-ID: <5f7ef6ee-6241-9912-f434-962be53272c@google.com>
References: <20221012081526.73067-1-huangjie.albert@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 12 Oct 2022, Albert Huang wrote:

> From: "huangjie.albert" <huangjie.albert@bytedance.com>
> 
> implement these two functions so that we can set the mempolicy to
> the inode of the hugetlb file. This ensures that the mempolicy of
> all processes sharing this huge page file is consistent.
> 
> In some scenarios where huge pages are shared:
> if we need to limit the memory usage of vm within node0, so I set qemu's
> mempilciy bind to node0, but if there is a process (such as virtiofsd)
> shared memory with the vm, in this case. If the page fault is triggered
> by virtiofsd, the allocated memory may go to node1 which  depends on
> virtiofsd.
> 
> Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>

Aha!  Congratulations for noticing, after all this time.  hugetlbfs
contains various little pieces of code that pretend to be supporting
shared NUMA mempolicy, but in fact there was nothing connecting it up.

It will be for Mike to decide, but personally I oppose adding
shared NUMA mempolicy support to hugetlbfs, after eighteen years.

The thing is, it will change the behaviour of NUMA on hugetlbfs:
in ways that would have been sensible way back then, yes; but surely
those who have invested in NUMA and hugetlbfs have developed other
ways of administering it successfully, without shared NUMA mempolicy.

At the least, I would expect some tests to break (I could easily be
wrong), and there's a chance that some app or tool would break too.

I have carried the reverse of Albert's patch for a long time, stripping
out the pretence of shared NUMA mempolicy support from hugetlbfs: I
wanted that, so that I could work on modifying the tmpfs implementation,
without having to worry about other users.

Mike, if you would prefer to see my patch stripping out the pretence,
let us know: it has never been a priority to send in, but I can update
it to 6.1-rc1 if you'd like to see it.  (Once upon a time, it removed
all need for struct hugetlbfs_inode_info, but nowadays that's still
required for the memfd seals.)

Whether Albert's patch is complete and correct, I haven't begun to think
about: I am not saying it isn't, but shared NUMA mempolicy adds another
dimension of complexity, and need for support, that I think hugetlbfs
would be better off continuing to survive without.

Hugh

> ---
>  mm/hugetlb.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 0ad53ad98e74..ed7599821655 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4678,6 +4678,24 @@ static vm_fault_t hugetlb_vm_op_fault(struct vm_fault *vmf)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_NUMA
> +int hugetlb_vm_op_set_policy(struct vm_area_struct *vma, struct mempolicy *mpol)
> +{
> +	struct inode *inode = file_inode(vma->vm_file);
> +
> +	return mpol_set_shared_policy(&HUGETLBFS_I(inode)->policy, vma, mpol);
> +}
> +
> +struct mempolicy *hugetlb_vm_op_get_policy(struct vm_area_struct *vma, unsigned long addr)
> +{
> +	struct inode *inode = file_inode(vma->vm_file);
> +	pgoff_t index;
> +
> +	index = ((addr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> +	return mpol_shared_policy_lookup(&HUGETLBFS_I(inode)->policy, index);
> +}
> +#endif
> +
>  /*
>   * When a new function is introduced to vm_operations_struct and added
>   * to hugetlb_vm_ops, please consider adding the function to shm_vm_ops.
> @@ -4691,6 +4709,10 @@ const struct vm_operations_struct hugetlb_vm_ops = {
>  	.close = hugetlb_vm_op_close,
>  	.may_split = hugetlb_vm_op_split,
>  	.pagesize = hugetlb_vm_op_pagesize,
> +#ifdef CONFIG_NUMA
> +	.set_policy = hugetlb_vm_op_set_policy,
> +	.get_policy = hugetlb_vm_op_get_policy,
> +#endif
>  };
>  
>  static pte_t make_huge_pte(struct vm_area_struct *vma, struct page *page,
> -- 
> 2.31.1