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
