Date:   Wed, 19 Oct 2022 17:33:17 +0800
From:   黄杰 <huangjie.albert@...edance.com>
To:     Hugh Dickins <hughd@...gle.com>
Cc:     Mike Kravetz <mike.kravetz@...cle.com>,
        Muchun Song <songmuchun@...edance.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: hugetlb: support get/set_policy for hugetlb_vm_ops

Hugh Dickins <hughd@...gle.com> 于2022年10月13日周四 03:45写道：
>
> On Wed, 12 Oct 2022, Albert Huang wrote:
>
> > From: "huangjie.albert" <huangjie.albert@...edance.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@...edance.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.

Hi : Hugh

Can you share some issues here?

Thanks.

>
> 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

Powered by blists - more mailing lists