Re: [v2 PATCH] mm: shmem: allow split THP when truncating THP partially

[Hugh Dickins]

On Wed, 26 Feb 2020, Yang Shi wrote:
> On 2/21/20 4:24 PM, Alexander Duyck wrote:
> > On Fri, Feb 21, 2020 at 10:24 AM Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
> > wrote:
> > > On 2/20/20 10:16 AM, Alexander Duyck wrote:
> > > > On Tue, Dec 3, 2019 at 4:43 PM Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
> > > > wrote:
> > > > > Currently when truncating shmem file, if the range is partial of THP
> > > > > (start or end is in the middle of THP), the pages actually will just
> > > > > get
> > > > > cleared rather than being freed unless the range cover the whole THP.
> > > > > Even though all the subpages are truncated (randomly or
> > > > > sequentially),
> > > > > the THP may still be kept in page cache.  This might be fine for some
> > > > > usecases which prefer preserving THP.
> > > > > 
> > > > > But, when doing balloon inflation in QEMU, QEMU actually does hole
> > > > > punch
> > > > > or MADV_DONTNEED in base page size granulairty if hugetlbfs is not
> > > > > used.
> > > > > So, when using shmem THP as memory backend QEMU inflation actually
> > > > > doesn't
> > > > > work as expected since it doesn't free memory.  But, the inflation
> > > > > usecase really needs get the memory freed.  Anonymous THP will not
> > > > > get
> > > > > freed right away too but it will be freed eventually when all
> > > > > subpages are
> > > > > unmapped, but shmem THP would still stay in page cache.
> > > > > 
> > > > > Split THP right away when doing partial hole punch, and if split
> > > > > fails
> > > > > just clear the page so that read to the hole punched area would
> > > > > return
> > > > > zero.
> > > > > 
> > > > > Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> > > > > Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > > > > Cc: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> > > > > Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
> > > > One question I would have is if this is really the desired behavior we
> > > > are looking for?
> > > > 
> > > > By proactively splitting the THP you are likely going to see a
> > > > performance regression with the virtio-balloon driver enabled in QEMU.
> > > > I would suspect the response to that would be to update the QEMU code
> > > > to  identify the page size of the shared memory ramblock. At that
> > > > point I suspect it would start behaving the same as how it currently
> > > > handles anonymous memory, and the work done here would essentially
> > > > have been wasted other than triggering the desire to resolve this in
> > > > QEMU to avoid a performance regression.
> > > > 
> > > > The code for inflating a the balloon in virtio-balloon in QEMU can be
> > > > found here:
> > > > https://github.com/qemu/qemu/blob/master/hw/virtio/virtio-balloon.c#L66
> > > > 
> > > > If there is a way for us to just populate the value obtained via
> > > > qemu_ram_pagesize with the THP page size instead of leaving it at 4K,
> > > > which is the size I am assuming it is at since you indicated that it
> > > > is just freeing the base page size, then we could address the same
> > > > issue and likely get the desired outcome of freeing the entire THP
> > > > page when it is no longer used.
> > > If qemu could punch hole (this is how qemu free file-backed memory) in
> > > THP unit, either w/ or w/o the patch the THP won't get split since the
> > > whole THP will get truncated. But, if qemu has to free memory in sub-THP
> > > size due to whatever reason (for example, 1MB for every 2MB section),
> > > then we have to split THP otherwise no memory will be freed actually
> > > with the current code. It is not about performance, it is about really
> > > giving memory back to host.
> > I get that, but at the same time I am not sure if everyone will be
> > happy with the trade-off. That is my concern.
> > 
> > You may want to change the patch description above if that is the
> > case. Based on the description above it makes it sound as if the issue
> > is that QEMU is using hole punch or MADV_DONTNEED with the wrong
> > granularity. Based on your comment here it sounds like you want to
> > have the ability to break up the larger THP page as soon as you want
> > to push out a single 4K page from it.
> 
> Yes, you are right. The commit log may be confusing. What I wanted to convey
> is QEMU has no idea if THP is used or not so it treats memory with base size
> unless hugetlbfs is used since QEMU is aware huge page is used in this case.
> This may sounds irrelevant to the problem, I would just remove that.

Oh, I'm sad to read that, since I was yanking most of your commit
message (as "Yang Shi writes") into my version, to give stronger
and independent justification for the change.

If I try to write about QEMU and ballooning myself, nonsense is sure to
emerge; but I don't know what part "I would just remove that" refers to.

May I beg you for an updated paragraph or two, explaining why you
want to see the change?

Thanks,
Hugh