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;
MIME-Version: 1.0
References: <20221012081526.73067-1-huangjie.albert@bytedance.com> <5f7ef6ee-6241-9912-f434-962be53272c@google.com>
In-Reply-To: <5f7ef6ee-6241-9912-f434-962be53272c@google.com>
From:   =?UTF-8?B?6buE5p2w?= <huangjie.albert@bytedance.com>
Date:   Wed, 19 Oct 2022 17:33:17 +0800
Message-ID: <CABKxMyNEWVBd8hvZfOHqxgtLi7gts+X53pVvag-zEsvqYqRnhw@mail.gmail.com>
Subject: Re: [PATCH] mm: hugetlb: support get/set_policy for hugetlb_vm_ops
To:     Hugh Dickins <hughd@google.com>
Cc:     Mike Kravetz <mike.kravetz@oracle.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Andi Kleen <ak@linux.intel.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hugh Dickins <hughd@google.com> =E4=BA=8E2022=E5=B9=B410=E6=9C=8813=E6=97=
=A5=E5=91=A8=E5=9B=9B 03:45=E5=86=99=E9=81=93=EF=BC=9A
>
> 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.

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 mempol=
icy *mpol)
> > +{
> > +     struct inode *inode =3D file_inode(vma->vm_file);
> > +
> > +     return mpol_set_shared_policy(&HUGETLBFS_I(inode)->policy, vma, m=
pol);
> > +}
> > +
> > +struct mempolicy *hugetlb_vm_op_get_policy(struct vm_area_struct *vma,=
 unsigned long addr)
> > +{
> > +     struct inode *inode =3D file_inode(vma->vm_file);
> > +     pgoff_t index;
> > +
> > +     index =3D ((addr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> > +     return mpol_shared_policy_lookup(&HUGETLBFS_I(inode)->policy, ind=
ex);
> > +}
> > +#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_op=
s.
> > @@ -4691,6 +4709,10 @@ const struct vm_operations_struct hugetlb_vm_ops=
 =3D {
> >       .close =3D hugetlb_vm_op_close,
> >       .may_split =3D hugetlb_vm_op_split,
> >       .pagesize =3D hugetlb_vm_op_pagesize,
> > +#ifdef CONFIG_NUMA
> > +     .set_policy =3D hugetlb_vm_op_set_policy,
> > +     .get_policy =3D hugetlb_vm_op_get_policy,
> > +#endif
> >  };
> >
> >  static pte_t make_huge_pte(struct vm_area_struct *vma, struct page *pa=
ge,
> > --
> > 2.31.1