| Message ID | 1583999395-9131-1-git-send-email-teawater@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,for,Linux] virtio_balloon: Add VIRTIO_BALLOON_F_THP_ORDER to handle THP spilt issue | expand |
On Thu, Mar 12, 2020 at 03:49:54PM +0800, Hui Zhu wrote: > If the guest kernel has many fragmentation pages, use virtio_balloon > will split THP of QEMU when it calls MADV_DONTNEED madvise to release > the balloon pages. > This is an example in a VM with 1G memory 1CPU: > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 0 kB > > usemem --punch-holes -s -1 800m & > > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 976896 kB > > (qemu) device_add virtio-balloon-pci,id=balloon1 > (qemu) info balloon > balloon: actual=1024 > (qemu) balloon 624 > (qemu) info balloon > balloon: actual=624 > > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 153600 kB > > THP number decreased more than 800M. > The reason is usemem with punch-holes option will free every other page > after allocation. Then 400M free memory inside the guest kernel is > fragmentation pages. > The guest kernel will use them to inflate the balloon. When these > fragmentation pages are freed, THP will be split. > > This commit tries to handle this with add a new flag > VIRTIO_BALLOON_F_THP_ORDER. > When this flag is set, the balloon page order will be set to the THP order. > Then THP pages will be freed together in the host. > This is an example in a VM with 1G memory 1CPU: > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 0 kB > > usemem --punch-holes -s -1 800m & > > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 976896 kB > > (qemu) device_add virtio-balloon-pci,id=balloon1,thp-order=on > (qemu) info balloon > balloon: actual=1024 > (qemu) balloon 624 > (qemu) info balloon > balloon: actual=624 > > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 583680 kB > > The THP number decreases 384M. This shows that VIRTIO_BALLOON_F_THP_ORDER > can help handle the THP split issue. > > Signed-off-by: Hui Zhu <teawaterz@linux.alibaba.com> > --- > drivers/virtio/virtio_balloon.c | 57 ++++++++++++++++++++++++++----------- > include/linux/balloon_compaction.h | 14 ++++++--- > include/uapi/linux/virtio_balloon.h | 4 +++ > 3 files changed, 54 insertions(+), 21 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 7bfe365..1e1dc76 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -175,18 +175,31 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > unsigned num_pfns; > struct page *page; > LIST_HEAD(pages); > + int page_order = 0; > > /* We can only do one array worth at a time. */ > num = min(num, ARRAY_SIZE(vb->pfns)); > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_THP_ORDER)) > + page_order = VIRTIO_BALLOON_THP_ORDER; > + > for (num_pfns = 0; num_pfns < num; > num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > - struct page *page = balloon_page_alloc(); > + struct page *page; > + > + if (page_order) > + page = alloc_pages(__GFP_HIGHMEM | > + __GFP_KSWAPD_RECLAIM | > + __GFP_RETRY_MAYFAIL | > + __GFP_NOWARN | __GFP_NOMEMALLOC, The set of flags is inconsistent with balloon_page_alloc. Pls extend that do not bypass it. > + page_order); > + else > + page = balloon_page_alloc(); > > if (!page) { > dev_info_ratelimited(&vb->vdev->dev, > - "Out of puff! Can't get %u pages\n", > - VIRTIO_BALLOON_PAGES_PER_PAGE); > + "Out of puff! Can't get %u pages\n", > + VIRTIO_BALLOON_PAGES_PER_PAGE << page_order); > /* Sleep for at least 1/5 of a second before retry. */ > msleep(200); > break; I suggest we do something guest side only for starters: if we need a power of two pages, try to get them in a single chunk, with no retrying. If that fails go back to a single page. > @@ -206,7 +219,7 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; > if (!virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) > - adjust_managed_page_count(page, -1); > + adjust_managed_page_count(page, -(1 << page_order)); > vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE; > } > > @@ -223,13 +236,20 @@ static void release_pages_balloon(struct virtio_balloon *vb, > struct list_head *pages) > { > struct page *page, *next; > + int page_order = 0; > + > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_THP_ORDER)) > + page_order = VIRTIO_BALLOON_THP_ORDER; > > list_for_each_entry_safe(page, next, pages, lru) { > if (!virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) > - adjust_managed_page_count(page, 1); > + adjust_managed_page_count(page, 1 << page_order); > list_del(&page->lru); > - put_page(page); /* balloon reference */ > + if (page_order) > + __free_pages(page, page_order); > + else > + put_page(page); /* balloon reference */ > } > } > > @@ -893,19 +913,21 @@ static int virtballoon_probe(struct virtio_device *vdev) > goto out_free_vb; > > #ifdef CONFIG_BALLOON_COMPACTION > - balloon_mnt = kern_mount(&balloon_fs); > - if (IS_ERR(balloon_mnt)) { > - err = PTR_ERR(balloon_mnt); > - goto out_del_vqs; > - } > + if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_THP_ORDER)) { > + balloon_mnt = kern_mount(&balloon_fs); > + if (IS_ERR(balloon_mnt)) { > + err = PTR_ERR(balloon_mnt); > + goto out_del_vqs; > + } > > - vb->vb_dev_info.migratepage = virtballoon_migratepage; > - vb->vb_dev_info.inode = alloc_anon_inode(balloon_mnt->mnt_sb); > - if (IS_ERR(vb->vb_dev_info.inode)) { > - err = PTR_ERR(vb->vb_dev_info.inode); > - goto out_kern_unmount; > + vb->vb_dev_info.migratepage = virtballoon_migratepage; > + vb->vb_dev_info.inode = alloc_anon_inode(balloon_mnt->mnt_sb); > + if (IS_ERR(vb->vb_dev_info.inode)) { > + err = PTR_ERR(vb->vb_dev_info.inode); > + goto out_kern_unmount; > + } > + vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops; > } > - vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops; > #endif > if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { > /* I doubt this fixed all code. Anything using VIRTIO_BALLOON_PAGES_PER_PAGE would be suspect. Also, the result might not fit in the pfns array. > @@ -1058,6 +1080,7 @@ static unsigned int features[] = { > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, > VIRTIO_BALLOON_F_FREE_PAGE_HINT, > VIRTIO_BALLOON_F_PAGE_POISON, > + VIRTIO_BALLOON_F_THP_ORDER, > }; > > static struct virtio_driver virtio_balloon_driver = { > diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h > index 338aa27..4c9164e 100644 > --- a/include/linux/balloon_compaction.h > +++ b/include/linux/balloon_compaction.h > @@ -100,8 +100,12 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon, > struct page *page) > { > __SetPageOffline(page); > - __SetPageMovable(page, balloon->inode->i_mapping); > - set_page_private(page, (unsigned long)balloon); > + if (balloon->inode) { > + __SetPageMovable(page, balloon->inode->i_mapping); > + set_page_private(page, (unsigned long)balloon); > + } else { > + set_page_private(page, 0); > + } > list_add(&page->lru, &balloon->pages); > } > > @@ -116,8 +120,10 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon, > static inline void balloon_page_delete(struct page *page) > { > __ClearPageOffline(page); > - __ClearPageMovable(page); > - set_page_private(page, 0); > + if (page_private(page)) { > + __ClearPageMovable(page); > + set_page_private(page, 0); > + } > /* > * No touch page.lru field once @page has been isolated > * because VM is using the field. > diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h > index a1966cd7..a2998a9 100644 > --- a/include/uapi/linux/virtio_balloon.h > +++ b/include/uapi/linux/virtio_balloon.h > @@ -36,10 +36,14 @@ > #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */ > #define VIRTIO_BALLOON_F_FREE_PAGE_HINT 3 /* VQ to report free pages */ > #define VIRTIO_BALLOON_F_PAGE_POISON 4 /* Guest is using page poisoning */ > +#define VIRTIO_BALLOON_F_THP_ORDER 5 /* Balloon page order to thp order */ > > /* Size of a PFN in the balloon interface. */ > #define VIRTIO_BALLOON_PFN_SHIFT 12 > > +/* The order of the balloon page */ > +#define VIRTIO_BALLOON_THP_ORDER 9 > + Why 9? > #define VIRTIO_BALLOON_CMD_ID_STOP 0 > #define VIRTIO_BALLOON_CMD_ID_DONE 1 > struct virtio_balloon_config { Assuming the idea is to also allow passing larger chunks to host, I think we need to switch to using regular virtio S/G for starters. That involves spec work though. > -- > 2.7.4
On 12.03.20 08:49, Hui Zhu wrote: > If the guest kernel has many fragmentation pages, use virtio_balloon > will split THP of QEMU when it calls MADV_DONTNEED madvise to release > the balloon pages. > This is an example in a VM with 1G memory 1CPU: > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 0 kB > > usemem --punch-holes -s -1 800m & > > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 976896 kB > > (qemu) device_add virtio-balloon-pci,id=balloon1 > (qemu) info balloon > balloon: actual=1024 > (qemu) balloon 624 > (qemu) info balloon > balloon: actual=624 > > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 153600 kB > > THP number decreased more than 800M. > The reason is usemem with punch-holes option will free every other page > after allocation. Then 400M free memory inside the guest kernel is > fragmentation pages. > The guest kernel will use them to inflate the balloon. When these > fragmentation pages are freed, THP will be split. > > This commit tries to handle this with add a new flag > VIRTIO_BALLOON_F_THP_ORDER. > When this flag is set, the balloon page order will be set to the THP order. > Then THP pages will be freed together in the host. > This is an example in a VM with 1G memory 1CPU: > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 0 kB > > usemem --punch-holes -s -1 800m & > > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 976896 kB > > (qemu) device_add virtio-balloon-pci,id=balloon1,thp-order=on > (qemu) info balloon > balloon: actual=1024 > (qemu) balloon 624 > (qemu) info balloon > balloon: actual=624 > > cat /proc/meminfo | grep AnonHugePages: > AnonHugePages: 583680 kB > > The THP number decreases 384M. This shows that VIRTIO_BALLOON_F_THP_ORDER > can help handle the THP split issue. Multiple things: I recently had a similar discussion with Alex [1] and I think this needs more thought. My thoughts: 1. You most certainly want to fallback to allocating pages in a smaller granularity once you run out of bigger allocations. Sacrifice performance for memory inflation, which has always been the case and which is what people expect to happen. (e.g., to shrink the page cache properly) 2. You are essentially stealing THPs in the guest. So the fastest mapping (THP in guest and host) is gone. The guest won't be able to make use of THP where it previously was able to. I can imagine this implies a performance degradation for some workloads. This needs a proper performance evaluation. 3. The pages you allocate are not migrateable, e.g., for memory offlining or alloc_contig_range() users like gigantic pages or soon virtio-mem. I strongly dislike that. This is IMHO a step backwards. We want to be able to migrate or even split-up and migrate such pages. Assume the guest could make good use of a THP somewhere. Who says it wouldn't be better to sacrifice a huge balloon page to be able to use THP both in the guest and the host for that mapping? I am not convinced stealing possible THPs in the guest and not being able to split them up is really what we want performance wise. 4. I think we also want a better mechanism to directly inflate/deflate higher/order pages and not reuse the 4k inflate/deflate queues. 5. I think we don't want to hard code such THP values but let the host tell us the THP size instead, which can easily differ between guest and host. Also, I do wonder if balloon compaction in the guest will already result in more THP getting used again long term. Assume the guest compacts balloon pages into a single THP again. This will result in a bunch of DONTNEED/WILLNEED in the hypervisor due to inflation/deflation. I wonder if the WILLNEED on the sub-pages of a candidate THP in the host will allow to use a THP in the host again. [1] https://lore.kernel.org/linux-mm/939de9de-d82a-aed2-6a51-57a55d81cbff@redhat.com/ > > Signed-off-by: Hui Zhu <teawaterz@linux.alibaba.com> > --- > drivers/virtio/virtio_balloon.c | 57 ++++++++++++++++++++++++++----------- > include/linux/balloon_compaction.h | 14 ++++++--- > include/uapi/linux/virtio_balloon.h | 4 +++ > 3 files changed, 54 insertions(+), 21 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 7bfe365..1e1dc76 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -175,18 +175,31 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > unsigned num_pfns; > struct page *page; > LIST_HEAD(pages); > + int page_order = 0; > > /* We can only do one array worth at a time. */ > num = min(num, ARRAY_SIZE(vb->pfns)); > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_THP_ORDER)) > + page_order = VIRTIO_BALLOON_THP_ORDER; > + > for (num_pfns = 0; num_pfns < num; > num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > - struct page *page = balloon_page_alloc(); > + struct page *page; > + > + if (page_order) > + page = alloc_pages(__GFP_HIGHMEM | > + __GFP_KSWAPD_RECLAIM | > + __GFP_RETRY_MAYFAIL | > + __GFP_NOWARN | __GFP_NOMEMALLOC, > + page_order); > + else > + page = balloon_page_alloc(); > > if (!page) { > dev_info_ratelimited(&vb->vdev->dev, > - "Out of puff! Can't get %u pages\n", > - VIRTIO_BALLOON_PAGES_PER_PAGE); > + "Out of puff! Can't get %u pages\n", > + VIRTIO_BALLOON_PAGES_PER_PAGE << page_order); > /* Sleep for at least 1/5 of a second before retry. */ > msleep(200); > break; > @@ -206,7 +219,7 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; > if (!virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) > - adjust_managed_page_count(page, -1); > + adjust_managed_page_count(page, -(1 << page_order)); > vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE; > } > > @@ -223,13 +236,20 @@ static void release_pages_balloon(struct virtio_balloon *vb, > struct list_head *pages) > { > struct page *page, *next; > + int page_order = 0; > + > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_THP_ORDER)) > + page_order = VIRTIO_BALLOON_THP_ORDER; > > list_for_each_entry_safe(page, next, pages, lru) { > if (!virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) > - adjust_managed_page_count(page, 1); > + adjust_managed_page_count(page, 1 << page_order); > list_del(&page->lru); > - put_page(page); /* balloon reference */ > + if (page_order) > + __free_pages(page, page_order); > + else > + put_page(page); /* balloon reference */ > } > } > > @@ -893,19 +913,21 @@ static int virtballoon_probe(struct virtio_device *vdev) > goto out_free_vb; > > #ifdef CONFIG_BALLOON_COMPACTION > - balloon_mnt = kern_mount(&balloon_fs); > - if (IS_ERR(balloon_mnt)) { > - err = PTR_ERR(balloon_mnt); > - goto out_del_vqs; > - } > + if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_THP_ORDER)) { > + balloon_mnt = kern_mount(&balloon_fs); > + if (IS_ERR(balloon_mnt)) { > + err = PTR_ERR(balloon_mnt); > + goto out_del_vqs; > + } > > - vb->vb_dev_info.migratepage = virtballoon_migratepage; > - vb->vb_dev_info.inode = alloc_anon_inode(balloon_mnt->mnt_sb); > - if (IS_ERR(vb->vb_dev_info.inode)) { > - err = PTR_ERR(vb->vb_dev_info.inode); > - goto out_kern_unmount; > + vb->vb_dev_info.migratepage = virtballoon_migratepage; > + vb->vb_dev_info.inode = alloc_anon_inode(balloon_mnt->mnt_sb); > + if (IS_ERR(vb->vb_dev_info.inode)) { > + err = PTR_ERR(vb->vb_dev_info.inode); > + goto out_kern_unmount; > + } > + vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops; > } > - vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops; > #endif > if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { > /* > @@ -1058,6 +1080,7 @@ static unsigned int features[] = { > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, > VIRTIO_BALLOON_F_FREE_PAGE_HINT, > VIRTIO_BALLOON_F_PAGE_POISON, > + VIRTIO_BALLOON_F_THP_ORDER, > }; > > static struct virtio_driver virtio_balloon_driver = { > diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h > index 338aa27..4c9164e 100644 > --- a/include/linux/balloon_compaction.h > +++ b/include/linux/balloon_compaction.h > @@ -100,8 +100,12 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon, > struct page *page) > { > __SetPageOffline(page); > - __SetPageMovable(page, balloon->inode->i_mapping); > - set_page_private(page, (unsigned long)balloon); > + if (balloon->inode) { > + __SetPageMovable(page, balloon->inode->i_mapping); > + set_page_private(page, (unsigned long)balloon); > + } else { > + set_page_private(page, 0); > + } > list_add(&page->lru, &balloon->pages); > } > > @@ -116,8 +120,10 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon, > static inline void balloon_page_delete(struct page *page) > { > __ClearPageOffline(page); > - __ClearPageMovable(page); > - set_page_private(page, 0); > + if (page_private(page)) { > + __ClearPageMovable(page); > + set_page_private(page, 0); > + } > /* > * No touch page.lru field once @page has been isolated > * because VM is using the field. > diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h > index a1966cd7..a2998a9 100644 > --- a/include/uapi/linux/virtio_balloon.h > +++ b/include/uapi/linux/virtio_balloon.h > @@ -36,10 +36,14 @@ > #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */ > #define VIRTIO_BALLOON_F_FREE_PAGE_HINT 3 /* VQ to report free pages */ > #define VIRTIO_BALLOON_F_PAGE_POISON 4 /* Guest is using page poisoning */ > +#define VIRTIO_BALLOON_F_THP_ORDER 5 /* Balloon page order to thp order */ > > /* Size of a PFN in the balloon interface. */ > #define VIRTIO_BALLOON_PFN_SHIFT 12 > > +/* The order of the balloon page */ > +#define VIRTIO_BALLOON_THP_ORDER 9 > + > #define VIRTIO_BALLOON_CMD_ID_STOP 0 > #define VIRTIO_BALLOON_CMD_ID_DONE 1 > struct virtio_balloon_config { >
On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: > 2. You are essentially stealing THPs in the guest. So the fastest > mapping (THP in guest and host) is gone. The guest won't be able to make > use of THP where it previously was able to. I can imagine this implies a > performance degradation for some workloads. This needs a proper > performance evaluation. I think the problem is more with the alloc_pages API. That gives you exactly the given order, and if there's a larger chunk available, it will split it up. But for balloon - I suspect lots of other users, we do not want to stress the system but if a large chunk is available anyway, then we could handle that more optimally by getting it all in one go. So if we want to address this, IMHO this calls for a new API. Along the lines of struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, unsigned int max_order, unsigned int *order) the idea would then be to return at a number of pages in the given range. What do you think? Want to try implementing that?
On 12.03.20 09:47, Michael S. Tsirkin wrote: > On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >> 2. You are essentially stealing THPs in the guest. So the fastest >> mapping (THP in guest and host) is gone. The guest won't be able to make >> use of THP where it previously was able to. I can imagine this implies a >> performance degradation for some workloads. This needs a proper >> performance evaluation. > > I think the problem is more with the alloc_pages API. > That gives you exactly the given order, and if there's > a larger chunk available, it will split it up. > > But for balloon - I suspect lots of other users, > we do not want to stress the system but if a large > chunk is available anyway, then we could handle > that more optimally by getting it all in one go. > > > So if we want to address this, IMHO this calls for a new API. > Along the lines of > > struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > unsigned int max_order, unsigned int *order) > > the idea would then be to return at a number of pages in the given > range. > > What do you think? Want to try implementing that? You can just start with the highest order and decrement the order until your allocation succeeds using alloc_pages(), which would be enough for a first version. At least I don't see the immediate need for a new kernel API.
On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: > On 12.03.20 09:47, Michael S. Tsirkin wrote: > > On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: > >> 2. You are essentially stealing THPs in the guest. So the fastest > >> mapping (THP in guest and host) is gone. The guest won't be able to make > >> use of THP where it previously was able to. I can imagine this implies a > >> performance degradation for some workloads. This needs a proper > >> performance evaluation. > > > > I think the problem is more with the alloc_pages API. > > That gives you exactly the given order, and if there's > > a larger chunk available, it will split it up. > > > > But for balloon - I suspect lots of other users, > > we do not want to stress the system but if a large > > chunk is available anyway, then we could handle > > that more optimally by getting it all in one go. > > > > > > So if we want to address this, IMHO this calls for a new API. > > Along the lines of > > > > struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > > unsigned int max_order, unsigned int *order) > > > > the idea would then be to return at a number of pages in the given > > range. > > > > What do you think? Want to try implementing that? > > You can just start with the highest order and decrement the order until > your allocation succeeds using alloc_pages(), which would be enough for > a first version. At least I don't see the immediate need for a new > kernel API. Well there's still a chance of splitting a big page if one becomes available meanwhile. But OK. > -- > Thanks, > > David / dhildenb
On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: > On 12.03.20 09:47, Michael S. Tsirkin wrote: > > On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: > >> 2. You are essentially stealing THPs in the guest. So the fastest > >> mapping (THP in guest and host) is gone. The guest won't be able to make > >> use of THP where it previously was able to. I can imagine this implies a > >> performance degradation for some workloads. This needs a proper > >> performance evaluation. > > > > I think the problem is more with the alloc_pages API. > > That gives you exactly the given order, and if there's > > a larger chunk available, it will split it up. > > > > But for balloon - I suspect lots of other users, > > we do not want to stress the system but if a large > > chunk is available anyway, then we could handle > > that more optimally by getting it all in one go. > > > > > > So if we want to address this, IMHO this calls for a new API. > > Along the lines of > > > > struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > > unsigned int max_order, unsigned int *order) > > > > the idea would then be to return at a number of pages in the given > > range. > > > > What do you think? Want to try implementing that? > > You can just start with the highest order and decrement the order until > your allocation succeeds using alloc_pages(), which would be enough for > a first version. At least I don't see the immediate need for a new > kernel API. OK I remember now. The problem is with reclaim. Unless reclaim is completely disabled, any of these calls can sleep. After it wakes up, we would like to get the larger order that has become available meanwhile. > -- > Thanks, > > David / dhildenb
> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: > > On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>> use of THP where it previously was able to. I can imagine this implies a >>>> performance degradation for some workloads. This needs a proper >>>> performance evaluation. >>> >>> I think the problem is more with the alloc_pages API. >>> That gives you exactly the given order, and if there's >>> a larger chunk available, it will split it up. >>> >>> But for balloon - I suspect lots of other users, >>> we do not want to stress the system but if a large >>> chunk is available anyway, then we could handle >>> that more optimally by getting it all in one go. >>> >>> >>> So if we want to address this, IMHO this calls for a new API. >>> Along the lines of >>> >>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>> unsigned int max_order, unsigned int *order) >>> >>> the idea would then be to return at a number of pages in the given >>> range. >>> >>> What do you think? Want to try implementing that? >> >> You can just start with the highest order and decrement the order until >> your allocation succeeds using alloc_pages(), which would be enough for >> a first version. At least I don't see the immediate need for a new >> kernel API. > > OK I remember now. The problem is with reclaim. Unless reclaim is > completely disabled, any of these calls can sleep. After it wakes up, > we would like to get the larger order that has become available > meanwhile. > Yes, but that‘s a pure optimization IMHO. So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. > >> -- >> Thanks, >> >> David / dhildenb >
On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: > > > > Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: > > > > On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: > >>> On 12.03.20 09:47, Michael S. Tsirkin wrote: > >>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: > >>>> 2. You are essentially stealing THPs in the guest. So the fastest > >>>> mapping (THP in guest and host) is gone. The guest won't be able to make > >>>> use of THP where it previously was able to. I can imagine this implies a > >>>> performance degradation for some workloads. This needs a proper > >>>> performance evaluation. > >>> > >>> I think the problem is more with the alloc_pages API. > >>> That gives you exactly the given order, and if there's > >>> a larger chunk available, it will split it up. > >>> > >>> But for balloon - I suspect lots of other users, > >>> we do not want to stress the system but if a large > >>> chunk is available anyway, then we could handle > >>> that more optimally by getting it all in one go. > >>> > >>> > >>> So if we want to address this, IMHO this calls for a new API. > >>> Along the lines of > >>> > >>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > >>> unsigned int max_order, unsigned int *order) > >>> > >>> the idea would then be to return at a number of pages in the given > >>> range. > >>> > >>> What do you think? Want to try implementing that? > >> > >> You can just start with the highest order and decrement the order until > >> your allocation succeeds using alloc_pages(), which would be enough for > >> a first version. At least I don't see the immediate need for a new > >> kernel API. > > > > OK I remember now. The problem is with reclaim. Unless reclaim is > > completely disabled, any of these calls can sleep. After it wakes up, > > we would like to get the larger order that has become available > > meanwhile. > > > > Yes, but that‘s a pure optimization IMHO. > So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. > Well how do you propose implement the necessary semantics? I think we are both agreed that alloc_page_range is more or less what's necessary anyway - so how would you approximate it on top of existing APIs? > > > >> -- > >> Thanks, > >> > >> David / dhildenb > >
On 26.03.20 10:49, Michael S. Tsirkin wrote: > On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: >> >> >>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: >>> >>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>>>> use of THP where it previously was able to. I can imagine this implies a >>>>>> performance degradation for some workloads. This needs a proper >>>>>> performance evaluation. >>>>> >>>>> I think the problem is more with the alloc_pages API. >>>>> That gives you exactly the given order, and if there's >>>>> a larger chunk available, it will split it up. >>>>> >>>>> But for balloon - I suspect lots of other users, >>>>> we do not want to stress the system but if a large >>>>> chunk is available anyway, then we could handle >>>>> that more optimally by getting it all in one go. >>>>> >>>>> >>>>> So if we want to address this, IMHO this calls for a new API. >>>>> Along the lines of >>>>> >>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>> unsigned int max_order, unsigned int *order) >>>>> >>>>> the idea would then be to return at a number of pages in the given >>>>> range. >>>>> >>>>> What do you think? Want to try implementing that? >>>> >>>> You can just start with the highest order and decrement the order until >>>> your allocation succeeds using alloc_pages(), which would be enough for >>>> a first version. At least I don't see the immediate need for a new >>>> kernel API. >>> >>> OK I remember now. The problem is with reclaim. Unless reclaim is >>> completely disabled, any of these calls can sleep. After it wakes up, >>> we would like to get the larger order that has become available >>> meanwhile. >>> >> >> Yes, but that‘s a pure optimization IMHO. >> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. >> > > Well how do you propose implement the necessary semantics? > I think we are both agreed that alloc_page_range is more or > less what's necessary anyway - so how would you approximate it > on top of existing APIs? Looking at drivers/misc/vmw_balloon.c:vmballoon_inflate(), it first tries to allocate huge pages using alloc_pages(__GFP_HIGHMEM|__GFP_NOWARN| __GFP_NOMEMALLOC, VMW_BALLOON_2M_ORDER) And then falls back to 4k allocations (balloon_page_alloc()) in case allocation fails. I'm roughly thinking of something like the following, but with an optimized reporting interface/bigger pfn array so we can report > 1MB at a time. Also, it might make sense to remember the order that succeeded across some fill_balloon() calls. Don't even expect it to compile ... From 4305f989672ccca4be9293e6d4167e929f3e299b Mon Sep 17 00:00:00 2001 From: David Hildenbrand <david@redhat.com> Date: Tue, 31 Mar 2020 12:28:07 +0200 Subject: [PATCH RFC] tmp Signed-off-by: David Hildenbrand <david@redhat.com> --- drivers/virtio/virtio_balloon.c | 38 ++++++++++++++++++-------- include/linux/balloon_compaction.h | 7 ++++- mm/balloon_compaction.c | 43 +++++++++++++++++++++++------- 3 files changed, 67 insertions(+), 21 deletions(-) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 8511d258dbb4..0660b1b988f0 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -187,7 +187,7 @@ int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_info, } static void set_page_pfns(struct virtio_balloon *vb, - __virtio32 pfns[], struct page *page) + __virtio32 pfns[], struct page *page, int order) { unsigned int i; @@ -197,7 +197,7 @@ static void set_page_pfns(struct virtio_balloon *vb, * Set balloon pfns pointing at this page. * Note that the first pfn points at start of the page. */ - for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++) + for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); i++) pfns[i] = cpu_to_virtio32(vb->vdev, page_to_balloon_pfn(page) + i); } @@ -205,6 +205,7 @@ static void set_page_pfns(struct virtio_balloon *vb, static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) { unsigned num_allocated_pages; + int order = MAX_ORDER - 1; unsigned num_pfns; struct page *page; LIST_HEAD(pages); @@ -212,9 +213,20 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) /* We can only do one array worth at a time. */ num = min(num, ARRAY_SIZE(vb->pfns)); + /* + * Note: we will currently never allocate more than 1MB due to the + * pfn array size, so we will not allocate MAX_ORDER - 1 ... + */ + for (num_pfns = 0; num_pfns < num; - num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { - struct page *page = balloon_page_alloc(); + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order)) { + const unsigned long remaining = num - num_pfns; + + order = MIN(order, + get_order(remaining << VIRTIO_BALLOON_PFN_SHIFT)); + if ((1 << order) * VIRTIO_BALLOON_PAGES_PER_PAGE > remaining) + order--; + page = balloon_pages_alloc(order); if (!page) { dev_info_ratelimited(&vb->vdev->dev, @@ -225,6 +237,8 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) break; } + /* Continue with the actual order that succeeded. */ + order = page_private(page); balloon_page_push(&pages, page); } @@ -233,14 +247,16 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) vb->num_pfns = 0; while ((page = balloon_page_pop(&pages))) { + order = page_order(page); + /* enqueuing will split the page and clear the order */ balloon_page_enqueue(&vb->vb_dev_info, page); - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); - vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; + set_page_pfns(vb, vb->pfns + vb->num_pfns, page, order); + vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) - adjust_managed_page_count(page, -1); - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE; + adjust_managed_page_count(page, -1 * (1 << order)); + vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); } num_allocated_pages = vb->num_pfns; @@ -284,7 +300,7 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num) page = balloon_page_dequeue(vb_dev_info); if (!page) break; - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); + set_page_pfns(vb, vb->pfns + vb->num_pfns, page, 0); list_add(&page->lru, &pages); vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE; } @@ -786,7 +802,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, __count_vm_event(BALLOON_MIGRATE); spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; - set_page_pfns(vb, vb->pfns, newpage); + set_page_pfns(vb, vb->pfns, newpage, 0); tell_host(vb, vb->inflate_vq); /* balloon's page migration 2nd step -- deflate "page" */ @@ -794,7 +810,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, balloon_page_delete(page); spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; - set_page_pfns(vb, vb->pfns, page); + set_page_pfns(vb, vb->pfns, page, 0); tell_host(vb, vb->deflate_vq); mutex_unlock(&vb->balloon_lock); diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h index 338aa27e4773..ed93fe5704d1 100644 --- a/include/linux/balloon_compaction.h +++ b/include/linux/balloon_compaction.h @@ -60,7 +60,7 @@ struct balloon_dev_info { struct inode *inode; }; -extern struct page *balloon_page_alloc(void); +extern struct page *balloon_pages_alloc(int order); extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info, struct page *page); extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info); @@ -78,6 +78,11 @@ static inline void balloon_devinfo_init(struct balloon_dev_info *balloon) balloon->inode = NULL; } +static inline struct page *balloon_page_alloc(void) +{ + return balloon_pages_alloc(0); +} + #ifdef CONFIG_BALLOON_COMPACTION extern const struct address_space_operations balloon_aops; extern bool balloon_page_isolate(struct page *page, diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c index 26de020aae7b..067810b32813 100644 --- a/mm/balloon_compaction.c +++ b/mm/balloon_compaction.c @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); /* - * balloon_page_alloc - allocates a new page for insertion into the balloon - * page list. + * balloon_pages_alloc - allocates a new page (of at most the given order) + * for insertion into the balloon page list. * * Driver must call this function to properly allocate a new balloon page. * Driver must call balloon_page_enqueue before definitively removing the page * from the guest system. * + * Will fall back to smaller orders if allocation fails. The order of the + * allocated page is stored in page->private. + * * Return: struct page for the allocated page or NULL on allocation failure. */ -struct page *balloon_page_alloc(void) +struct page *balloon_pages_alloc(int order) { - struct page *page = alloc_page(balloon_mapping_gfp_mask() | - __GFP_NOMEMALLOC | __GFP_NORETRY | - __GFP_NOWARN); - return page; + struct page *page; + + while (order >= 0) { + page = alloc_pages(balloon_mapping_gfp_mask() | + __GFP_NOMEMALLOC | __GFP_NORETRY | + __GFP_NOWARN, order); + if (page) { + set_page_private(page, order); + return page; + } + order--; + } + return NULL; } -EXPORT_SYMBOL_GPL(balloon_page_alloc); +EXPORT_SYMBOL_GPL(balloon_pages_alloc); /* * balloon_page_enqueue - inserts a new page into the balloon page list. @@ -146,10 +158,23 @@ EXPORT_SYMBOL_GPL(balloon_page_alloc); void balloon_page_enqueue(struct balloon_dev_info *b_dev_info, struct page *page) { + const int order = page_private(page); unsigned long flags; + int i; + + /* + * We can only migrate single pages - and even if we could migrate + * bigger ones, we would want to split them on demand instead of + * trying to move around big chunks. + */ + if (order > 0) + split_page(page, order); + set_page_private(page, order); spin_lock_irqsave(&b_dev_info->pages_lock, flags); - balloon_page_enqueue_one(b_dev_info, page); + for (i = 0; i < (1 << order); i++) + balloon_page_enqueue_one(b_dev_info, page + i); + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags); } EXPORT_SYMBOL_GPL(balloon_page_enqueue);
On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: > On 26.03.20 10:49, Michael S. Tsirkin wrote: > > On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: > >> > >> > >>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: > >>> > >>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: > >>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: > >>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: > >>>>>> 2. You are essentially stealing THPs in the guest. So the fastest > >>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make > >>>>>> use of THP where it previously was able to. I can imagine this implies a > >>>>>> performance degradation for some workloads. This needs a proper > >>>>>> performance evaluation. > >>>>> > >>>>> I think the problem is more with the alloc_pages API. > >>>>> That gives you exactly the given order, and if there's > >>>>> a larger chunk available, it will split it up. > >>>>> > >>>>> But for balloon - I suspect lots of other users, > >>>>> we do not want to stress the system but if a large > >>>>> chunk is available anyway, then we could handle > >>>>> that more optimally by getting it all in one go. > >>>>> > >>>>> > >>>>> So if we want to address this, IMHO this calls for a new API. > >>>>> Along the lines of > >>>>> > >>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > >>>>> unsigned int max_order, unsigned int *order) > >>>>> > >>>>> the idea would then be to return at a number of pages in the given > >>>>> range. > >>>>> > >>>>> What do you think? Want to try implementing that? > >>>> > >>>> You can just start with the highest order and decrement the order until > >>>> your allocation succeeds using alloc_pages(), which would be enough for > >>>> a first version. At least I don't see the immediate need for a new > >>>> kernel API. > >>> > >>> OK I remember now. The problem is with reclaim. Unless reclaim is > >>> completely disabled, any of these calls can sleep. After it wakes up, > >>> we would like to get the larger order that has become available > >>> meanwhile. > >>> > >> > >> Yes, but that‘s a pure optimization IMHO. > >> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. > >> > > > > Well how do you propose implement the necessary semantics? > > I think we are both agreed that alloc_page_range is more or > > less what's necessary anyway - so how would you approximate it > > on top of existing APIs? > > Looking at drivers/misc/vmw_balloon.c:vmballoon_inflate(), it first > tries to allocate huge pages using > > alloc_pages(__GFP_HIGHMEM|__GFP_NOWARN| __GFP_NOMEMALLOC, > VMW_BALLOON_2M_ORDER) > > And then falls back to 4k allocations (balloon_page_alloc()) in case > allocation fails. > > I'm roughly thinking of something like the following, but with an > optimized reporting interface/bigger pfn array so we can report > > 1MB at a time. Also, it might make sense to remember the order that > succeeded across some fill_balloon() calls. > > Don't even expect it to compile ... > > > > >From 4305f989672ccca4be9293e6d4167e929f3e299b Mon Sep 17 00:00:00 2001 > From: David Hildenbrand <david@redhat.com> > Date: Tue, 31 Mar 2020 12:28:07 +0200 > Subject: [PATCH RFC] tmp > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > drivers/virtio/virtio_balloon.c | 38 ++++++++++++++++++-------- > include/linux/balloon_compaction.h | 7 ++++- > mm/balloon_compaction.c | 43 +++++++++++++++++++++++------- > 3 files changed, 67 insertions(+), 21 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 8511d258dbb4..0660b1b988f0 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -187,7 +187,7 @@ int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_info, > } > > static void set_page_pfns(struct virtio_balloon *vb, > - __virtio32 pfns[], struct page *page) > + __virtio32 pfns[], struct page *page, int order) > { > unsigned int i; > > @@ -197,7 +197,7 @@ static void set_page_pfns(struct virtio_balloon *vb, > * Set balloon pfns pointing at this page. > * Note that the first pfn points at start of the page. > */ > - for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++) > + for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); i++) > pfns[i] = cpu_to_virtio32(vb->vdev, > page_to_balloon_pfn(page) + i); > } > @@ -205,6 +205,7 @@ static void set_page_pfns(struct virtio_balloon *vb, > static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > { > unsigned num_allocated_pages; > + int order = MAX_ORDER - 1; > unsigned num_pfns; > struct page *page; > LIST_HEAD(pages); > @@ -212,9 +213,20 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > /* We can only do one array worth at a time. */ > num = min(num, ARRAY_SIZE(vb->pfns)); > > + /* > + * Note: we will currently never allocate more than 1MB due to the > + * pfn array size, so we will not allocate MAX_ORDER - 1 ... > + */ > + > for (num_pfns = 0; num_pfns < num; > - num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > - struct page *page = balloon_page_alloc(); > + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order)) { > + const unsigned long remaining = num - num_pfns; > + > + order = MIN(order, > + get_order(remaining << VIRTIO_BALLOON_PFN_SHIFT)); > + if ((1 << order) * VIRTIO_BALLOON_PAGES_PER_PAGE > remaining) > + order--; > + page = balloon_pages_alloc(order); > > if (!page) { > dev_info_ratelimited(&vb->vdev->dev, > @@ -225,6 +237,8 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > break; > } > > + /* Continue with the actual order that succeeded. */ > + order = page_private(page); > balloon_page_push(&pages, page); > } > > @@ -233,14 +247,16 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > vb->num_pfns = 0; > > while ((page = balloon_page_pop(&pages))) { > + order = page_order(page); > + /* enqueuing will split the page and clear the order */ > balloon_page_enqueue(&vb->vb_dev_info, page); > > - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); > - vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; > + set_page_pfns(vb, vb->pfns + vb->num_pfns, page, order); > + vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); > if (!virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) > - adjust_managed_page_count(page, -1); > - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE; > + adjust_managed_page_count(page, -1 * (1 << order)); > + vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); > } > > num_allocated_pages = vb->num_pfns; > @@ -284,7 +300,7 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num) > page = balloon_page_dequeue(vb_dev_info); > if (!page) > break; > - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); > + set_page_pfns(vb, vb->pfns + vb->num_pfns, page, 0); > list_add(&page->lru, &pages); > vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE; > } > @@ -786,7 +802,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, > __count_vm_event(BALLOON_MIGRATE); > spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); > vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; > - set_page_pfns(vb, vb->pfns, newpage); > + set_page_pfns(vb, vb->pfns, newpage, 0); > tell_host(vb, vb->inflate_vq); > > /* balloon's page migration 2nd step -- deflate "page" */ > @@ -794,7 +810,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, > balloon_page_delete(page); > spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); > vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; > - set_page_pfns(vb, vb->pfns, page); > + set_page_pfns(vb, vb->pfns, page, 0); > tell_host(vb, vb->deflate_vq); > > mutex_unlock(&vb->balloon_lock); > diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h > index 338aa27e4773..ed93fe5704d1 100644 > --- a/include/linux/balloon_compaction.h > +++ b/include/linux/balloon_compaction.h > @@ -60,7 +60,7 @@ struct balloon_dev_info { > struct inode *inode; > }; > > -extern struct page *balloon_page_alloc(void); > +extern struct page *balloon_pages_alloc(int order); > extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info, > struct page *page); > extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info); > @@ -78,6 +78,11 @@ static inline void balloon_devinfo_init(struct balloon_dev_info *balloon) > balloon->inode = NULL; > } > > +static inline struct page *balloon_page_alloc(void) > +{ > + return balloon_pages_alloc(0); > +} > + > #ifdef CONFIG_BALLOON_COMPACTION > extern const struct address_space_operations balloon_aops; > extern bool balloon_page_isolate(struct page *page, > diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c > index 26de020aae7b..067810b32813 100644 > --- a/mm/balloon_compaction.c > +++ b/mm/balloon_compaction.c > @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, > EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); > > /* > - * balloon_page_alloc - allocates a new page for insertion into the balloon > - * page list. > + * balloon_pages_alloc - allocates a new page (of at most the given order) > + * for insertion into the balloon page list. > * > * Driver must call this function to properly allocate a new balloon page. > * Driver must call balloon_page_enqueue before definitively removing the page > * from the guest system. > * > + * Will fall back to smaller orders if allocation fails. The order of the > + * allocated page is stored in page->private. > + * > * Return: struct page for the allocated page or NULL on allocation failure. > */ > -struct page *balloon_page_alloc(void) > +struct page *balloon_pages_alloc(int order) > { > - struct page *page = alloc_page(balloon_mapping_gfp_mask() | > - __GFP_NOMEMALLOC | __GFP_NORETRY | > - __GFP_NOWARN); > - return page; > + struct page *page; > + > + while (order >= 0) { > + page = alloc_pages(balloon_mapping_gfp_mask() | > + __GFP_NOMEMALLOC | __GFP_NORETRY | > + __GFP_NOWARN, order); > + if (page) { > + set_page_private(page, order); > + return page; > + } > + order--; > + } > + return NULL; > } > -EXPORT_SYMBOL_GPL(balloon_page_alloc); > +EXPORT_SYMBOL_GPL(balloon_pages_alloc); > > /* > * balloon_page_enqueue - inserts a new page into the balloon page list. I think this will try to invoke direct reclaim from the first iteration to free up the max order. > @@ -146,10 +158,23 @@ EXPORT_SYMBOL_GPL(balloon_page_alloc); > void balloon_page_enqueue(struct balloon_dev_info *b_dev_info, > struct page *page) > { > + const int order = page_private(page); > unsigned long flags; > + int i; > + > + /* > + * We can only migrate single pages - and even if we could migrate > + * bigger ones, we would want to split them on demand instead of > + * trying to move around big chunks. > + */ > + if (order > 0) > + split_page(page, order); > + set_page_private(page, order); > > spin_lock_irqsave(&b_dev_info->pages_lock, flags); > - balloon_page_enqueue_one(b_dev_info, page); > + for (i = 0; i < (1 << order); i++) > + balloon_page_enqueue_one(b_dev_info, page + i); > + > spin_unlock_irqrestore(&b_dev_info->pages_lock, flags); > } > EXPORT_SYMBOL_GPL(balloon_page_enqueue); > -- > 2.25.1 > > -- > Thanks, > > David / dhildenb
On 31.03.20 15:24, Michael S. Tsirkin wrote: > On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: >> On 26.03.20 10:49, Michael S. Tsirkin wrote: >>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: >>>> >>>> >>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: >>>>> >>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>>>>>> use of THP where it previously was able to. I can imagine this implies a >>>>>>>> performance degradation for some workloads. This needs a proper >>>>>>>> performance evaluation. >>>>>>> >>>>>>> I think the problem is more with the alloc_pages API. >>>>>>> That gives you exactly the given order, and if there's >>>>>>> a larger chunk available, it will split it up. >>>>>>> >>>>>>> But for balloon - I suspect lots of other users, >>>>>>> we do not want to stress the system but if a large >>>>>>> chunk is available anyway, then we could handle >>>>>>> that more optimally by getting it all in one go. >>>>>>> >>>>>>> >>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>> Along the lines of >>>>>>> >>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>> unsigned int max_order, unsigned int *order) >>>>>>> >>>>>>> the idea would then be to return at a number of pages in the given >>>>>>> range. >>>>>>> >>>>>>> What do you think? Want to try implementing that? >>>>>> >>>>>> You can just start with the highest order and decrement the order until >>>>>> your allocation succeeds using alloc_pages(), which would be enough for >>>>>> a first version. At least I don't see the immediate need for a new >>>>>> kernel API. >>>>> >>>>> OK I remember now. The problem is with reclaim. Unless reclaim is >>>>> completely disabled, any of these calls can sleep. After it wakes up, >>>>> we would like to get the larger order that has become available >>>>> meanwhile. >>>>> >>>> >>>> Yes, but that‘s a pure optimization IMHO. >>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. >>>> >>> >>> Well how do you propose implement the necessary semantics? >>> I think we are both agreed that alloc_page_range is more or >>> less what's necessary anyway - so how would you approximate it >>> on top of existing APIs? >> >> Looking at drivers/misc/vmw_balloon.c:vmballoon_inflate(), it first >> tries to allocate huge pages using >> >> alloc_pages(__GFP_HIGHMEM|__GFP_NOWARN| __GFP_NOMEMALLOC, >> VMW_BALLOON_2M_ORDER) >> >> And then falls back to 4k allocations (balloon_page_alloc()) in case >> allocation fails. >> >> I'm roughly thinking of something like the following, but with an >> optimized reporting interface/bigger pfn array so we can report > >> 1MB at a time. Also, it might make sense to remember the order that >> succeeded across some fill_balloon() calls. >> >> Don't even expect it to compile ... >> >> >> >> >From 4305f989672ccca4be9293e6d4167e929f3e299b Mon Sep 17 00:00:00 2001 >> From: David Hildenbrand <david@redhat.com> >> Date: Tue, 31 Mar 2020 12:28:07 +0200 >> Subject: [PATCH RFC] tmp >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> drivers/virtio/virtio_balloon.c | 38 ++++++++++++++++++-------- >> include/linux/balloon_compaction.h | 7 ++++- >> mm/balloon_compaction.c | 43 +++++++++++++++++++++++------- >> 3 files changed, 67 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c >> index 8511d258dbb4..0660b1b988f0 100644 >> --- a/drivers/virtio/virtio_balloon.c >> +++ b/drivers/virtio/virtio_balloon.c >> @@ -187,7 +187,7 @@ int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_info, >> } >> >> static void set_page_pfns(struct virtio_balloon *vb, >> - __virtio32 pfns[], struct page *page) >> + __virtio32 pfns[], struct page *page, int order) >> { >> unsigned int i; >> >> @@ -197,7 +197,7 @@ static void set_page_pfns(struct virtio_balloon *vb, >> * Set balloon pfns pointing at this page. >> * Note that the first pfn points at start of the page. >> */ >> - for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++) >> + for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); i++) >> pfns[i] = cpu_to_virtio32(vb->vdev, >> page_to_balloon_pfn(page) + i); >> } >> @@ -205,6 +205,7 @@ static void set_page_pfns(struct virtio_balloon *vb, >> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >> { >> unsigned num_allocated_pages; >> + int order = MAX_ORDER - 1; >> unsigned num_pfns; >> struct page *page; >> LIST_HEAD(pages); >> @@ -212,9 +213,20 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >> /* We can only do one array worth at a time. */ >> num = min(num, ARRAY_SIZE(vb->pfns)); >> >> + /* >> + * Note: we will currently never allocate more than 1MB due to the >> + * pfn array size, so we will not allocate MAX_ORDER - 1 ... >> + */ >> + >> for (num_pfns = 0; num_pfns < num; >> - num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { >> - struct page *page = balloon_page_alloc(); >> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order)) { >> + const unsigned long remaining = num - num_pfns; >> + >> + order = MIN(order, >> + get_order(remaining << VIRTIO_BALLOON_PFN_SHIFT)); >> + if ((1 << order) * VIRTIO_BALLOON_PAGES_PER_PAGE > remaining) >> + order--; >> + page = balloon_pages_alloc(order); >> >> if (!page) { >> dev_info_ratelimited(&vb->vdev->dev, >> @@ -225,6 +237,8 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >> break; >> } >> >> + /* Continue with the actual order that succeeded. */ >> + order = page_private(page); >> balloon_page_push(&pages, page); >> } >> >> @@ -233,14 +247,16 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >> vb->num_pfns = 0; >> >> while ((page = balloon_page_pop(&pages))) { >> + order = page_order(page); >> + /* enqueuing will split the page and clear the order */ >> balloon_page_enqueue(&vb->vb_dev_info, page); >> >> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); >> - vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; >> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page, order); >> + vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); >> if (!virtio_has_feature(vb->vdev, >> VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) >> - adjust_managed_page_count(page, -1); >> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE; >> + adjust_managed_page_count(page, -1 * (1 << order)); >> + vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); >> } >> >> num_allocated_pages = vb->num_pfns; >> @@ -284,7 +300,7 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num) >> page = balloon_page_dequeue(vb_dev_info); >> if (!page) >> break; >> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); >> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page, 0); >> list_add(&page->lru, &pages); >> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE; >> } >> @@ -786,7 +802,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, >> __count_vm_event(BALLOON_MIGRATE); >> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); >> vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; >> - set_page_pfns(vb, vb->pfns, newpage); >> + set_page_pfns(vb, vb->pfns, newpage, 0); >> tell_host(vb, vb->inflate_vq); >> >> /* balloon's page migration 2nd step -- deflate "page" */ >> @@ -794,7 +810,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, >> balloon_page_delete(page); >> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); >> vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; >> - set_page_pfns(vb, vb->pfns, page); >> + set_page_pfns(vb, vb->pfns, page, 0); >> tell_host(vb, vb->deflate_vq); >> >> mutex_unlock(&vb->balloon_lock); >> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h >> index 338aa27e4773..ed93fe5704d1 100644 >> --- a/include/linux/balloon_compaction.h >> +++ b/include/linux/balloon_compaction.h >> @@ -60,7 +60,7 @@ struct balloon_dev_info { >> struct inode *inode; >> }; >> >> -extern struct page *balloon_page_alloc(void); >> +extern struct page *balloon_pages_alloc(int order); >> extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info, >> struct page *page); >> extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info); >> @@ -78,6 +78,11 @@ static inline void balloon_devinfo_init(struct balloon_dev_info *balloon) >> balloon->inode = NULL; >> } >> >> +static inline struct page *balloon_page_alloc(void) >> +{ >> + return balloon_pages_alloc(0); >> +} >> + >> #ifdef CONFIG_BALLOON_COMPACTION >> extern const struct address_space_operations balloon_aops; >> extern bool balloon_page_isolate(struct page *page, >> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c >> index 26de020aae7b..067810b32813 100644 >> --- a/mm/balloon_compaction.c >> +++ b/mm/balloon_compaction.c >> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, >> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); >> >> /* >> - * balloon_page_alloc - allocates a new page for insertion into the balloon >> - * page list. >> + * balloon_pages_alloc - allocates a new page (of at most the given order) >> + * for insertion into the balloon page list. >> * >> * Driver must call this function to properly allocate a new balloon page. >> * Driver must call balloon_page_enqueue before definitively removing the page >> * from the guest system. >> * >> + * Will fall back to smaller orders if allocation fails. The order of the >> + * allocated page is stored in page->private. >> + * >> * Return: struct page for the allocated page or NULL on allocation failure. >> */ >> -struct page *balloon_page_alloc(void) >> +struct page *balloon_pages_alloc(int order) >> { >> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | >> - __GFP_NOMEMALLOC | __GFP_NORETRY | >> - __GFP_NOWARN); >> - return page; >> + struct page *page; >> + >> + while (order >= 0) { >> + page = alloc_pages(balloon_mapping_gfp_mask() | >> + __GFP_NOMEMALLOC | __GFP_NORETRY | >> + __GFP_NOWARN, order); >> + if (page) { >> + set_page_private(page, order); >> + return page; >> + } >> + order--; >> + } >> + return NULL; >> } >> -EXPORT_SYMBOL_GPL(balloon_page_alloc); >> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); >> >> /* >> * balloon_page_enqueue - inserts a new page into the balloon page list. > > > I think this will try to invoke direct reclaim from the first iteration > to free up the max order. %__GFP_NORETRY: The VM implementation will try only very lightweight memory direct reclaim to get some memory under memory pressure (thus it can sleep). It will avoid disruptive actions like OOM killer. Certainly good enough for a first version I would say, no? Looking at the vmware balloon, they don't even set __GFP_NORETRY.
On Tue, Mar 31, 2020 at 03:32:05PM +0200, David Hildenbrand wrote: > On 31.03.20 15:24, Michael S. Tsirkin wrote: > > On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: > >> On 26.03.20 10:49, Michael S. Tsirkin wrote: > >>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: > >>>> > >>>> > >>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: > >>>>> > >>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: > >>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: > >>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: > >>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest > >>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make > >>>>>>>> use of THP where it previously was able to. I can imagine this implies a > >>>>>>>> performance degradation for some workloads. This needs a proper > >>>>>>>> performance evaluation. > >>>>>>> > >>>>>>> I think the problem is more with the alloc_pages API. > >>>>>>> That gives you exactly the given order, and if there's > >>>>>>> a larger chunk available, it will split it up. > >>>>>>> > >>>>>>> But for balloon - I suspect lots of other users, > >>>>>>> we do not want to stress the system but if a large > >>>>>>> chunk is available anyway, then we could handle > >>>>>>> that more optimally by getting it all in one go. > >>>>>>> > >>>>>>> > >>>>>>> So if we want to address this, IMHO this calls for a new API. > >>>>>>> Along the lines of > >>>>>>> > >>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > >>>>>>> unsigned int max_order, unsigned int *order) > >>>>>>> > >>>>>>> the idea would then be to return at a number of pages in the given > >>>>>>> range. > >>>>>>> > >>>>>>> What do you think? Want to try implementing that? > >>>>>> > >>>>>> You can just start with the highest order and decrement the order until > >>>>>> your allocation succeeds using alloc_pages(), which would be enough for > >>>>>> a first version. At least I don't see the immediate need for a new > >>>>>> kernel API. > >>>>> > >>>>> OK I remember now. The problem is with reclaim. Unless reclaim is > >>>>> completely disabled, any of these calls can sleep. After it wakes up, > >>>>> we would like to get the larger order that has become available > >>>>> meanwhile. > >>>>> > >>>> > >>>> Yes, but that‘s a pure optimization IMHO. > >>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. > >>>> > >>> > >>> Well how do you propose implement the necessary semantics? > >>> I think we are both agreed that alloc_page_range is more or > >>> less what's necessary anyway - so how would you approximate it > >>> on top of existing APIs? > >> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h ..... > >> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c > >> index 26de020aae7b..067810b32813 100644 > >> --- a/mm/balloon_compaction.c > >> +++ b/mm/balloon_compaction.c > >> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, > >> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); > >> > >> /* > >> - * balloon_page_alloc - allocates a new page for insertion into the balloon > >> - * page list. > >> + * balloon_pages_alloc - allocates a new page (of at most the given order) > >> + * for insertion into the balloon page list. > >> * > >> * Driver must call this function to properly allocate a new balloon page. > >> * Driver must call balloon_page_enqueue before definitively removing the page > >> * from the guest system. > >> * > >> + * Will fall back to smaller orders if allocation fails. The order of the > >> + * allocated page is stored in page->private. > >> + * > >> * Return: struct page for the allocated page or NULL on allocation failure. > >> */ > >> -struct page *balloon_page_alloc(void) > >> +struct page *balloon_pages_alloc(int order) > >> { > >> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | > >> - __GFP_NOMEMALLOC | __GFP_NORETRY | > >> - __GFP_NOWARN); > >> - return page; > >> + struct page *page; > >> + > >> + while (order >= 0) { > >> + page = alloc_pages(balloon_mapping_gfp_mask() | > >> + __GFP_NOMEMALLOC | __GFP_NORETRY | > >> + __GFP_NOWARN, order); > >> + if (page) { > >> + set_page_private(page, order); > >> + return page; > >> + } > >> + order--; > >> + } > >> + return NULL; > >> } > >> -EXPORT_SYMBOL_GPL(balloon_page_alloc); > >> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); > >> > >> /* > >> * balloon_page_enqueue - inserts a new page into the balloon page list. > > > > > > I think this will try to invoke direct reclaim from the first iteration > > to free up the max order. > > %__GFP_NORETRY: The VM implementation will try only very lightweight > memory direct reclaim to get some memory under memory pressure (thus it > can sleep). It will avoid disruptive actions like OOM killer. > > Certainly good enough for a first version I would say, no? Frankly how well that behaves would depend a lot on the workload. Can regress just as well. For the 1st version I'd prefer something that is the least disruptive, and that IMHO means we only trigger reclaim at all in the same configuration as now - when we can't satisfy the lowest order allocation. Anything else would be a huge amount of testing with all kind of workloads.
On 31.03.20 15:37, Michael S. Tsirkin wrote: > On Tue, Mar 31, 2020 at 03:32:05PM +0200, David Hildenbrand wrote: >> On 31.03.20 15:24, Michael S. Tsirkin wrote: >>> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: >>>> On 26.03.20 10:49, Michael S. Tsirkin wrote: >>>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: >>>>>> >>>>>> >>>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: >>>>>>> >>>>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>>>>>>>> use of THP where it previously was able to. I can imagine this implies a >>>>>>>>>> performance degradation for some workloads. This needs a proper >>>>>>>>>> performance evaluation. >>>>>>>>> >>>>>>>>> I think the problem is more with the alloc_pages API. >>>>>>>>> That gives you exactly the given order, and if there's >>>>>>>>> a larger chunk available, it will split it up. >>>>>>>>> >>>>>>>>> But for balloon - I suspect lots of other users, >>>>>>>>> we do not want to stress the system but if a large >>>>>>>>> chunk is available anyway, then we could handle >>>>>>>>> that more optimally by getting it all in one go. >>>>>>>>> >>>>>>>>> >>>>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>>>> Along the lines of >>>>>>>>> >>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>>>> unsigned int max_order, unsigned int *order) >>>>>>>>> >>>>>>>>> the idea would then be to return at a number of pages in the given >>>>>>>>> range. >>>>>>>>> >>>>>>>>> What do you think? Want to try implementing that? >>>>>>>> >>>>>>>> You can just start with the highest order and decrement the order until >>>>>>>> your allocation succeeds using alloc_pages(), which would be enough for >>>>>>>> a first version. At least I don't see the immediate need for a new >>>>>>>> kernel API. >>>>>>> >>>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is >>>>>>> completely disabled, any of these calls can sleep. After it wakes up, >>>>>>> we would like to get the larger order that has become available >>>>>>> meanwhile. >>>>>>> >>>>>> >>>>>> Yes, but that‘s a pure optimization IMHO. >>>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. >>>>>> >>>>> >>>>> Well how do you propose implement the necessary semantics? >>>>> I think we are both agreed that alloc_page_range is more or >>>>> less what's necessary anyway - so how would you approximate it >>>>> on top of existing APIs? >>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h > > ..... > > >>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c >>>> index 26de020aae7b..067810b32813 100644 >>>> --- a/mm/balloon_compaction.c >>>> +++ b/mm/balloon_compaction.c >>>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, >>>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); >>>> >>>> /* >>>> - * balloon_page_alloc - allocates a new page for insertion into the balloon >>>> - * page list. >>>> + * balloon_pages_alloc - allocates a new page (of at most the given order) >>>> + * for insertion into the balloon page list. >>>> * >>>> * Driver must call this function to properly allocate a new balloon page. >>>> * Driver must call balloon_page_enqueue before definitively removing the page >>>> * from the guest system. >>>> * >>>> + * Will fall back to smaller orders if allocation fails. The order of the >>>> + * allocated page is stored in page->private. >>>> + * >>>> * Return: struct page for the allocated page or NULL on allocation failure. >>>> */ >>>> -struct page *balloon_page_alloc(void) >>>> +struct page *balloon_pages_alloc(int order) >>>> { >>>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | >>>> - __GFP_NOMEMALLOC | __GFP_NORETRY | >>>> - __GFP_NOWARN); >>>> - return page; >>>> + struct page *page; >>>> + >>>> + while (order >= 0) { >>>> + page = alloc_pages(balloon_mapping_gfp_mask() | >>>> + __GFP_NOMEMALLOC | __GFP_NORETRY | >>>> + __GFP_NOWARN, order); >>>> + if (page) { >>>> + set_page_private(page, order); >>>> + return page; >>>> + } >>>> + order--; >>>> + } >>>> + return NULL; >>>> } >>>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); >>>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); >>>> >>>> /* >>>> * balloon_page_enqueue - inserts a new page into the balloon page list. >>> >>> >>> I think this will try to invoke direct reclaim from the first iteration >>> to free up the max order. >> >> %__GFP_NORETRY: The VM implementation will try only very lightweight >> memory direct reclaim to get some memory under memory pressure (thus it >> can sleep). It will avoid disruptive actions like OOM killer. >> >> Certainly good enough for a first version I would say, no? > > Frankly how well that behaves would depend a lot on the workload. > Can regress just as well. > > For the 1st version I'd prefer something that is the least disruptive, > and that IMHO means we only trigger reclaim at all in the same configuration > as now - when we can't satisfy the lowest order allocation. Agreed. > > Anything else would be a huge amount of testing with all kind of > workloads. > So doing a "& ~__GFP_RECLAIM" in case order > 0? (as done in GFP_TRANSHUGE_LIGHT)
On Tue, Mar 31, 2020 at 04:03:18PM +0200, David Hildenbrand wrote: > On 31.03.20 15:37, Michael S. Tsirkin wrote: > > On Tue, Mar 31, 2020 at 03:32:05PM +0200, David Hildenbrand wrote: > >> On 31.03.20 15:24, Michael S. Tsirkin wrote: > >>> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: > >>>> On 26.03.20 10:49, Michael S. Tsirkin wrote: > >>>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: > >>>>>> > >>>>>> > >>>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: > >>>>>>> > >>>>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: > >>>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: > >>>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: > >>>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest > >>>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make > >>>>>>>>>> use of THP where it previously was able to. I can imagine this implies a > >>>>>>>>>> performance degradation for some workloads. This needs a proper > >>>>>>>>>> performance evaluation. > >>>>>>>>> > >>>>>>>>> I think the problem is more with the alloc_pages API. > >>>>>>>>> That gives you exactly the given order, and if there's > >>>>>>>>> a larger chunk available, it will split it up. > >>>>>>>>> > >>>>>>>>> But for balloon - I suspect lots of other users, > >>>>>>>>> we do not want to stress the system but if a large > >>>>>>>>> chunk is available anyway, then we could handle > >>>>>>>>> that more optimally by getting it all in one go. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> So if we want to address this, IMHO this calls for a new API. > >>>>>>>>> Along the lines of > >>>>>>>>> > >>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > >>>>>>>>> unsigned int max_order, unsigned int *order) > >>>>>>>>> > >>>>>>>>> the idea would then be to return at a number of pages in the given > >>>>>>>>> range. > >>>>>>>>> > >>>>>>>>> What do you think? Want to try implementing that? > >>>>>>>> > >>>>>>>> You can just start with the highest order and decrement the order until > >>>>>>>> your allocation succeeds using alloc_pages(), which would be enough for > >>>>>>>> a first version. At least I don't see the immediate need for a new > >>>>>>>> kernel API. > >>>>>>> > >>>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is > >>>>>>> completely disabled, any of these calls can sleep. After it wakes up, > >>>>>>> we would like to get the larger order that has become available > >>>>>>> meanwhile. > >>>>>>> > >>>>>> > >>>>>> Yes, but that‘s a pure optimization IMHO. > >>>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. > >>>>>> > >>>>> > >>>>> Well how do you propose implement the necessary semantics? > >>>>> I think we are both agreed that alloc_page_range is more or > >>>>> less what's necessary anyway - so how would you approximate it > >>>>> on top of existing APIs? > >>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h > > > > ..... > > > > > >>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c > >>>> index 26de020aae7b..067810b32813 100644 > >>>> --- a/mm/balloon_compaction.c > >>>> +++ b/mm/balloon_compaction.c > >>>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, > >>>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); > >>>> > >>>> /* > >>>> - * balloon_page_alloc - allocates a new page for insertion into the balloon > >>>> - * page list. > >>>> + * balloon_pages_alloc - allocates a new page (of at most the given order) > >>>> + * for insertion into the balloon page list. > >>>> * > >>>> * Driver must call this function to properly allocate a new balloon page. > >>>> * Driver must call balloon_page_enqueue before definitively removing the page > >>>> * from the guest system. > >>>> * > >>>> + * Will fall back to smaller orders if allocation fails. The order of the > >>>> + * allocated page is stored in page->private. > >>>> + * > >>>> * Return: struct page for the allocated page or NULL on allocation failure. > >>>> */ > >>>> -struct page *balloon_page_alloc(void) > >>>> +struct page *balloon_pages_alloc(int order) > >>>> { > >>>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | > >>>> - __GFP_NOMEMALLOC | __GFP_NORETRY | > >>>> - __GFP_NOWARN); > >>>> - return page; > >>>> + struct page *page; > >>>> + > >>>> + while (order >= 0) { > >>>> + page = alloc_pages(balloon_mapping_gfp_mask() | > >>>> + __GFP_NOMEMALLOC | __GFP_NORETRY | > >>>> + __GFP_NOWARN, order); > >>>> + if (page) { > >>>> + set_page_private(page, order); > >>>> + return page; > >>>> + } > >>>> + order--; > >>>> + } > >>>> + return NULL; > >>>> } > >>>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); > >>>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); > >>>> > >>>> /* > >>>> * balloon_page_enqueue - inserts a new page into the balloon page list. > >>> > >>> > >>> I think this will try to invoke direct reclaim from the first iteration > >>> to free up the max order. > >> > >> %__GFP_NORETRY: The VM implementation will try only very lightweight > >> memory direct reclaim to get some memory under memory pressure (thus it > >> can sleep). It will avoid disruptive actions like OOM killer. > >> > >> Certainly good enough for a first version I would say, no? > > > > Frankly how well that behaves would depend a lot on the workload. > > Can regress just as well. > > > > For the 1st version I'd prefer something that is the least disruptive, > > and that IMHO means we only trigger reclaim at all in the same configuration > > as now - when we can't satisfy the lowest order allocation. > > Agreed. > > > > > Anything else would be a huge amount of testing with all kind of > > workloads. > > > > So doing a "& ~__GFP_RECLAIM" in case order > 0? (as done in > GFP_TRANSHUGE_LIGHT) That will improve the situation when reclaim is not needed, but leave the problem in place for when it's needed: if reclaim does trigger, we can get a huge free page and immediately break it up. So it's ok as a first step but it will make the second step harder as we'll need to test with reclaim :). > -- > Thanks, > > David / dhildenb
On 31.03.20 16:07, Michael S. Tsirkin wrote: > On Tue, Mar 31, 2020 at 04:03:18PM +0200, David Hildenbrand wrote: >> On 31.03.20 15:37, Michael S. Tsirkin wrote: >>> On Tue, Mar 31, 2020 at 03:32:05PM +0200, David Hildenbrand wrote: >>>> On 31.03.20 15:24, Michael S. Tsirkin wrote: >>>>> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: >>>>>> On 26.03.20 10:49, Michael S. Tsirkin wrote: >>>>>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: >>>>>>>> >>>>>>>> >>>>>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: >>>>>>>>> >>>>>>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>>>>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>>>>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>>>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>>>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>>>>>>>>>> use of THP where it previously was able to. I can imagine this implies a >>>>>>>>>>>> performance degradation for some workloads. This needs a proper >>>>>>>>>>>> performance evaluation. >>>>>>>>>>> >>>>>>>>>>> I think the problem is more with the alloc_pages API. >>>>>>>>>>> That gives you exactly the given order, and if there's >>>>>>>>>>> a larger chunk available, it will split it up. >>>>>>>>>>> >>>>>>>>>>> But for balloon - I suspect lots of other users, >>>>>>>>>>> we do not want to stress the system but if a large >>>>>>>>>>> chunk is available anyway, then we could handle >>>>>>>>>>> that more optimally by getting it all in one go. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>>>>>> Along the lines of >>>>>>>>>>> >>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>>>>>> unsigned int max_order, unsigned int *order) >>>>>>>>>>> >>>>>>>>>>> the idea would then be to return at a number of pages in the given >>>>>>>>>>> range. >>>>>>>>>>> >>>>>>>>>>> What do you think? Want to try implementing that? >>>>>>>>>> >>>>>>>>>> You can just start with the highest order and decrement the order until >>>>>>>>>> your allocation succeeds using alloc_pages(), which would be enough for >>>>>>>>>> a first version. At least I don't see the immediate need for a new >>>>>>>>>> kernel API. >>>>>>>>> >>>>>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is >>>>>>>>> completely disabled, any of these calls can sleep. After it wakes up, >>>>>>>>> we would like to get the larger order that has become available >>>>>>>>> meanwhile. >>>>>>>>> >>>>>>>> >>>>>>>> Yes, but that‘s a pure optimization IMHO. >>>>>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. >>>>>>>> >>>>>>> >>>>>>> Well how do you propose implement the necessary semantics? >>>>>>> I think we are both agreed that alloc_page_range is more or >>>>>>> less what's necessary anyway - so how would you approximate it >>>>>>> on top of existing APIs? >>>>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h >>> >>> ..... >>> >>> >>>>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c >>>>>> index 26de020aae7b..067810b32813 100644 >>>>>> --- a/mm/balloon_compaction.c >>>>>> +++ b/mm/balloon_compaction.c >>>>>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, >>>>>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); >>>>>> >>>>>> /* >>>>>> - * balloon_page_alloc - allocates a new page for insertion into the balloon >>>>>> - * page list. >>>>>> + * balloon_pages_alloc - allocates a new page (of at most the given order) >>>>>> + * for insertion into the balloon page list. >>>>>> * >>>>>> * Driver must call this function to properly allocate a new balloon page. >>>>>> * Driver must call balloon_page_enqueue before definitively removing the page >>>>>> * from the guest system. >>>>>> * >>>>>> + * Will fall back to smaller orders if allocation fails. The order of the >>>>>> + * allocated page is stored in page->private. >>>>>> + * >>>>>> * Return: struct page for the allocated page or NULL on allocation failure. >>>>>> */ >>>>>> -struct page *balloon_page_alloc(void) >>>>>> +struct page *balloon_pages_alloc(int order) >>>>>> { >>>>>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | >>>>>> - __GFP_NOMEMALLOC | __GFP_NORETRY | >>>>>> - __GFP_NOWARN); >>>>>> - return page; >>>>>> + struct page *page; >>>>>> + >>>>>> + while (order >= 0) { >>>>>> + page = alloc_pages(balloon_mapping_gfp_mask() | >>>>>> + __GFP_NOMEMALLOC | __GFP_NORETRY | >>>>>> + __GFP_NOWARN, order); >>>>>> + if (page) { >>>>>> + set_page_private(page, order); >>>>>> + return page; >>>>>> + } >>>>>> + order--; >>>>>> + } >>>>>> + return NULL; >>>>>> } >>>>>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); >>>>>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); >>>>>> >>>>>> /* >>>>>> * balloon_page_enqueue - inserts a new page into the balloon page list. >>>>> >>>>> >>>>> I think this will try to invoke direct reclaim from the first iteration >>>>> to free up the max order. >>>> >>>> %__GFP_NORETRY: The VM implementation will try only very lightweight >>>> memory direct reclaim to get some memory under memory pressure (thus it >>>> can sleep). It will avoid disruptive actions like OOM killer. >>>> >>>> Certainly good enough for a first version I would say, no? >>> >>> Frankly how well that behaves would depend a lot on the workload. >>> Can regress just as well. >>> >>> For the 1st version I'd prefer something that is the least disruptive, >>> and that IMHO means we only trigger reclaim at all in the same configuration >>> as now - when we can't satisfy the lowest order allocation. >> >> Agreed. >> >>> >>> Anything else would be a huge amount of testing with all kind of >>> workloads. >>> >> >> So doing a "& ~__GFP_RECLAIM" in case order > 0? (as done in >> GFP_TRANSHUGE_LIGHT) > > That will improve the situation when reclaim is not needed, but leave > the problem in place for when it's needed: if reclaim does trigger, we > can get a huge free page and immediately break it up. > > So it's ok as a first step but it will make the second step harder as > we'll need to test with reclaim :). I expect the whole "steal huge pages from your guest" to be problematic, as I already mentioned to Alex. This needs a performance evaluation. This all smells like a lot of workload dependent fine-tuning. :)
On Tue, Mar 31, 2020 at 04:09:59PM +0200, David Hildenbrand wrote: ... > >>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. > >>>>>>>>>>> Along the lines of > >>>>>>>>>>> > >>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > >>>>>>>>>>> unsigned int max_order, unsigned int *order) > >>>>>>>>>>> > >>>>>>>>>>> the idea would then be to return at a number of pages in the given > >>>>>>>>>>> range. > >>>>>>>>>>> > >>>>>>>>>>> What do you think? Want to try implementing that? .. > I expect the whole "steal huge pages from your guest" to be problematic, > as I already mentioned to Alex. This needs a performance evaluation. > > This all smells like a lot of workload dependent fine-tuning. :) So that's why I proposed the API above. The idea is that *if we are allocating a huge page anyway*, rather than break it up let's send it whole to the device. If we have smaller pages, return smaller pages. That seems like it would always be an improvement, whatever the workload.
On 31.03.20 16:18, Michael S. Tsirkin wrote: > On Tue, Mar 31, 2020 at 04:09:59PM +0200, David Hildenbrand wrote: > > ... > >>>>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>>>>>>>> Along the lines of >>>>>>>>>>>>> >>>>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>>>>>>>> unsigned int max_order, unsigned int *order) >>>>>>>>>>>>> >>>>>>>>>>>>> the idea would then be to return at a number of pages in the given >>>>>>>>>>>>> range. >>>>>>>>>>>>> >>>>>>>>>>>>> What do you think? Want to try implementing that? > > .. > >> I expect the whole "steal huge pages from your guest" to be problematic, >> as I already mentioned to Alex. This needs a performance evaluation. >> >> This all smells like a lot of workload dependent fine-tuning. :) > > > So that's why I proposed the API above. > > The idea is that *if we are allocating a huge page anyway*, > rather than break it up let's send it whole to the device. > If we have smaller pages, return smaller pages. > Sorry, I still fail to see why you cannot do that with my version of balloon_pages_alloc(). But maybe I haven't understood the magic you expect to happen in alloc_page_range() :) It's just going via a different inflate queue once we have that page, as I stated in front of my draft patch "but with an optimized reporting interface". > That seems like it would always be an improvement, whatever the > workload. > Don't think so. Assume there are plenty of 4k pages lying around. It might actually be *bad* for guest performance if you take a huge page instead of all the leftover 4k pages that cannot be merged. Only at the point where you would want to break a bigger page up and report it in pieces, where it would definitely make no difference. I guess Hui Zhu now has something to look into/work on :)
On 31.03.20 16:29, David Hildenbrand wrote: > On 31.03.20 16:18, Michael S. Tsirkin wrote: >> On Tue, Mar 31, 2020 at 04:09:59PM +0200, David Hildenbrand wrote: >> >> ... >> >>>>>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>>>>>>>>> Along the lines of >>>>>>>>>>>>>> >>>>>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>>>>>>>>> unsigned int max_order, unsigned int *order) >>>>>>>>>>>>>> >>>>>>>>>>>>>> the idea would then be to return at a number of pages in the given >>>>>>>>>>>>>> range. >>>>>>>>>>>>>> >>>>>>>>>>>>>> What do you think? Want to try implementing that? >> >> .. >> >>> I expect the whole "steal huge pages from your guest" to be problematic, >>> as I already mentioned to Alex. This needs a performance evaluation. >>> >>> This all smells like a lot of workload dependent fine-tuning. :) >> >> >> So that's why I proposed the API above. >> >> The idea is that *if we are allocating a huge page anyway*, >> rather than break it up let's send it whole to the device. >> If we have smaller pages, return smaller pages. >> > > Sorry, I still fail to see why you cannot do that with my version of > balloon_pages_alloc(). But maybe I haven't understood the magic you > expect to happen in alloc_page_range() :) > > It's just going via a different inflate queue once we have that page, as > I stated in front of my draft patch "but with an > optimized reporting interface". > >> That seems like it would always be an improvement, whatever the >> workload. >> > > Don't think so. Assume there are plenty of 4k pages lying around. It > might actually be *bad* for guest performance if you take a huge page > instead of all the leftover 4k pages that cannot be merged. Only at the > point where you would want to break a bigger page up and report it in > pieces, where it would definitely make no difference. I just understood what you mean :) and now it makes sense - it avoids exactly that. Basically 1. Try to allocate order-0. No split necessary? return the page 2. Try to allocate order-1. No split necessary? return the page ... up to MAX_ORDER - 1. Yeah, I guess this will need a new kernel API.
On Tue, Mar 31, 2020 at 04:34:48PM +0200, David Hildenbrand wrote: > On 31.03.20 16:29, David Hildenbrand wrote: > > On 31.03.20 16:18, Michael S. Tsirkin wrote: > >> On Tue, Mar 31, 2020 at 04:09:59PM +0200, David Hildenbrand wrote: > >> > >> ... > >> > >>>>>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. > >>>>>>>>>>>>>> Along the lines of > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > >>>>>>>>>>>>>> unsigned int max_order, unsigned int *order) > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> the idea would then be to return at a number of pages in the given > >>>>>>>>>>>>>> range. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> What do you think? Want to try implementing that? > >> > >> .. > >> > >>> I expect the whole "steal huge pages from your guest" to be problematic, > >>> as I already mentioned to Alex. This needs a performance evaluation. > >>> > >>> This all smells like a lot of workload dependent fine-tuning. :) > >> > >> > >> So that's why I proposed the API above. > >> > >> The idea is that *if we are allocating a huge page anyway*, > >> rather than break it up let's send it whole to the device. > >> If we have smaller pages, return smaller pages. > >> > > > > Sorry, I still fail to see why you cannot do that with my version of > > balloon_pages_alloc(). But maybe I haven't understood the magic you > > expect to happen in alloc_page_range() :) > > > > It's just going via a different inflate queue once we have that page, as > > I stated in front of my draft patch "but with an > > optimized reporting interface". > > > >> That seems like it would always be an improvement, whatever the > >> workload. > >> > > > > Don't think so. Assume there are plenty of 4k pages lying around. It > > might actually be *bad* for guest performance if you take a huge page > > instead of all the leftover 4k pages that cannot be merged. Only at the > > point where you would want to break a bigger page up and report it in > > pieces, where it would definitely make no difference. > > I just understood what you mean :) and now it makes sense - it avoids > exactly that. Basically > > 1. Try to allocate order-0. No split necessary? return the page > 2. Try to allocate order-1. No split necessary? return the page > ... > > up to MAX_ORDER - 1. > > Yeah, I guess this will need a new kernel API. Exactly what I meant. And whever we fail and block for reclaim, we restart this. > > -- > Thanks, > > David / dhildenb
> On Mar 31, 2020, at 6:32 AM, David Hildenbrand <david@redhat.com> wrote: > > On 31.03.20 15:24, Michael S. Tsirkin wrote: >> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: >>> On 26.03.20 10:49, Michael S. Tsirkin wrote: >>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: >>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: >>>>>> >>>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>>>>>>> use of THP where it previously was able to. I can imagine this implies a >>>>>>>>> performance degradation for some workloads. This needs a proper >>>>>>>>> performance evaluation. >>>>>>>> >>>>>>>> I think the problem is more with the alloc_pages API. >>>>>>>> That gives you exactly the given order, and if there's >>>>>>>> a larger chunk available, it will split it up. >>>>>>>> >>>>>>>> But for balloon - I suspect lots of other users, >>>>>>>> we do not want to stress the system but if a large >>>>>>>> chunk is available anyway, then we could handle >>>>>>>> that more optimally by getting it all in one go. >>>>>>>> >>>>>>>> >>>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>>> Along the lines of >>>>>>>> >>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>>> unsigned int max_order, unsigned int *order) >>>>>>>> >>>>>>>> the idea would then be to return at a number of pages in the given >>>>>>>> range. >>>>>>>> >>>>>>>> What do you think? Want to try implementing that? >>>>>>> >>>>>>> You can just start with the highest order and decrement the order until >>>>>>> your allocation succeeds using alloc_pages(), which would be enough for >>>>>>> a first version. At least I don't see the immediate need for a new >>>>>>> kernel API. >>>>>> >>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is >>>>>> completely disabled, any of these calls can sleep. After it wakes up, >>>>>> we would like to get the larger order that has become available >>>>>> meanwhile. >>>>> >>>>> Yes, but that‘s a pure optimization IMHO. >>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. >>>> >>>> Well how do you propose implement the necessary semantics? >>>> I think we are both agreed that alloc_page_range is more or >>>> less what's necessary anyway - so how would you approximate it >>>> on top of existing APIs? >>> >>> Looking at drivers/misc/vmw_balloon.c:vmballoon_inflate(), it first >>> tries to allocate huge pages using >>> >>> alloc_pages(__GFP_HIGHMEM|__GFP_NOWARN| __GFP_NOMEMALLOC, >>> VMW_BALLOON_2M_ORDER) >>> >>> And then falls back to 4k allocations (balloon_page_alloc()) in case >>> allocation fails. >>> >>> I'm roughly thinking of something like the following, but with an >>> optimized reporting interface/bigger pfn array so we can report > >>> 1MB at a time. Also, it might make sense to remember the order that >>> succeeded across some fill_balloon() calls. >>> >>> Don't even expect it to compile ... >>> >>> >>> >>>> From 4305f989672ccca4be9293e6d4167e929f3e299b Mon Sep 17 00:00:00 2001 >>> From: David Hildenbrand <david@redhat.com> >>> Date: Tue, 31 Mar 2020 12:28:07 +0200 >>> Subject: [PATCH RFC] tmp >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> drivers/virtio/virtio_balloon.c | 38 ++++++++++++++++++-------- >>> include/linux/balloon_compaction.h | 7 ++++- >>> mm/balloon_compaction.c | 43 +++++++++++++++++++++++------- >>> 3 files changed, 67 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c >>> index 8511d258dbb4..0660b1b988f0 100644 >>> --- a/drivers/virtio/virtio_balloon.c >>> +++ b/drivers/virtio/virtio_balloon.c >>> @@ -187,7 +187,7 @@ int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_info, >>> } >>> >>> static void set_page_pfns(struct virtio_balloon *vb, >>> - __virtio32 pfns[], struct page *page) >>> + __virtio32 pfns[], struct page *page, int order) >>> { >>> unsigned int i; >>> >>> @@ -197,7 +197,7 @@ static void set_page_pfns(struct virtio_balloon *vb, >>> * Set balloon pfns pointing at this page. >>> * Note that the first pfn points at start of the page. >>> */ >>> - for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++) >>> + for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); i++) >>> pfns[i] = cpu_to_virtio32(vb->vdev, >>> page_to_balloon_pfn(page) + i); >>> } >>> @@ -205,6 +205,7 @@ static void set_page_pfns(struct virtio_balloon *vb, >>> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >>> { >>> unsigned num_allocated_pages; >>> + int order = MAX_ORDER - 1; >>> unsigned num_pfns; >>> struct page *page; >>> LIST_HEAD(pages); >>> @@ -212,9 +213,20 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >>> /* We can only do one array worth at a time. */ >>> num = min(num, ARRAY_SIZE(vb->pfns)); >>> >>> + /* >>> + * Note: we will currently never allocate more than 1MB due to the >>> + * pfn array size, so we will not allocate MAX_ORDER - 1 ... >>> + */ >>> + >>> for (num_pfns = 0; num_pfns < num; >>> - num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { >>> - struct page *page = balloon_page_alloc(); >>> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order)) { >>> + const unsigned long remaining = num - num_pfns; >>> + >>> + order = MIN(order, >>> + get_order(remaining << VIRTIO_BALLOON_PFN_SHIFT)); >>> + if ((1 << order) * VIRTIO_BALLOON_PAGES_PER_PAGE > remaining) >>> + order--; >>> + page = balloon_pages_alloc(order); >>> >>> if (!page) { >>> dev_info_ratelimited(&vb->vdev->dev, >>> @@ -225,6 +237,8 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >>> break; >>> } >>> >>> + /* Continue with the actual order that succeeded. */ >>> + order = page_private(page); >>> balloon_page_push(&pages, page); >>> } >>> >>> @@ -233,14 +247,16 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >>> vb->num_pfns = 0; >>> >>> while ((page = balloon_page_pop(&pages))) { >>> + order = page_order(page); >>> + /* enqueuing will split the page and clear the order */ >>> balloon_page_enqueue(&vb->vb_dev_info, page); >>> >>> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); >>> - vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; >>> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page, order); >>> + vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); >>> if (!virtio_has_feature(vb->vdev, >>> VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) >>> - adjust_managed_page_count(page, -1); >>> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE; >>> + adjust_managed_page_count(page, -1 * (1 << order)); >>> + vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); >>> } >>> >>> num_allocated_pages = vb->num_pfns; >>> @@ -284,7 +300,7 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num) >>> page = balloon_page_dequeue(vb_dev_info); >>> if (!page) >>> break; >>> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); >>> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page, 0); >>> list_add(&page->lru, &pages); >>> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE; >>> } >>> @@ -786,7 +802,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, >>> __count_vm_event(BALLOON_MIGRATE); >>> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); >>> vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; >>> - set_page_pfns(vb, vb->pfns, newpage); >>> + set_page_pfns(vb, vb->pfns, newpage, 0); >>> tell_host(vb, vb->inflate_vq); >>> >>> /* balloon's page migration 2nd step -- deflate "page" */ >>> @@ -794,7 +810,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, >>> balloon_page_delete(page); >>> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); >>> vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; >>> - set_page_pfns(vb, vb->pfns, page); >>> + set_page_pfns(vb, vb->pfns, page, 0); >>> tell_host(vb, vb->deflate_vq); >>> >>> mutex_unlock(&vb->balloon_lock); >>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h >>> index 338aa27e4773..ed93fe5704d1 100644 >>> --- a/include/linux/balloon_compaction.h >>> +++ b/include/linux/balloon_compaction.h >>> @@ -60,7 +60,7 @@ struct balloon_dev_info { >>> struct inode *inode; >>> }; >>> >>> -extern struct page *balloon_page_alloc(void); >>> +extern struct page *balloon_pages_alloc(int order); >>> extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info, >>> struct page *page); >>> extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info); >>> @@ -78,6 +78,11 @@ static inline void balloon_devinfo_init(struct balloon_dev_info *balloon) >>> balloon->inode = NULL; >>> } >>> >>> +static inline struct page *balloon_page_alloc(void) >>> +{ >>> + return balloon_pages_alloc(0); >>> +} >>> + >>> #ifdef CONFIG_BALLOON_COMPACTION >>> extern const struct address_space_operations balloon_aops; >>> extern bool balloon_page_isolate(struct page *page, >>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c >>> index 26de020aae7b..067810b32813 100644 >>> --- a/mm/balloon_compaction.c >>> +++ b/mm/balloon_compaction.c >>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, >>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); >>> >>> /* >>> - * balloon_page_alloc - allocates a new page for insertion into the balloon >>> - * page list. >>> + * balloon_pages_alloc - allocates a new page (of at most the given order) >>> + * for insertion into the balloon page list. >>> * >>> * Driver must call this function to properly allocate a new balloon page. >>> * Driver must call balloon_page_enqueue before definitively removing the page >>> * from the guest system. >>> * >>> + * Will fall back to smaller orders if allocation fails. The order of the >>> + * allocated page is stored in page->private. >>> + * >>> * Return: struct page for the allocated page or NULL on allocation failure. >>> */ >>> -struct page *balloon_page_alloc(void) >>> +struct page *balloon_pages_alloc(int order) >>> { >>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | >>> - __GFP_NOMEMALLOC | __GFP_NORETRY | >>> - __GFP_NOWARN); >>> - return page; >>> + struct page *page; >>> + >>> + while (order >= 0) { >>> + page = alloc_pages(balloon_mapping_gfp_mask() | >>> + __GFP_NOMEMALLOC | __GFP_NORETRY | >>> + __GFP_NOWARN, order); >>> + if (page) { >>> + set_page_private(page, order); >>> + return page; >>> + } >>> + order--; >>> + } >>> + return NULL; >>> } >>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); >>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); >>> >>> /* >>> * balloon_page_enqueue - inserts a new page into the balloon page list. >> >> >> I think this will try to invoke direct reclaim from the first iteration >> to free up the max order. > > %__GFP_NORETRY: The VM implementation will try only very lightweight > memory direct reclaim to get some memory under memory pressure (thus it > can sleep). It will avoid disruptive actions like OOM killer. > > Certainly good enough for a first version I would say, no? Looking at > the vmware balloon, they don't even set __GFP_NORETRY. Yes, it does seem that we are missing __GFP_NORETRY. I really do not know what I was thinking when I did not add it for huge-pages allocation. I will send a patch. Thanks for noticing :) In regard to your patch, I would be happy to consolidate the allocation mechanisms, so VMware balloon driver would also use your code. In general your code looks good, take-away some style issues.
> On Mar 31, 2020, at 7:09 AM, David Hildenbrand <david@redhat.com> wrote: > > On 31.03.20 16:07, Michael S. Tsirkin wrote: >> On Tue, Mar 31, 2020 at 04:03:18PM +0200, David Hildenbrand wrote: >>> On 31.03.20 15:37, Michael S. Tsirkin wrote: >>>> On Tue, Mar 31, 2020 at 03:32:05PM +0200, David Hildenbrand wrote: >>>>> On 31.03.20 15:24, Michael S. Tsirkin wrote: >>>>>> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: >>>>>>> On 26.03.20 10:49, Michael S. Tsirkin wrote: >>>>>>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: >>>>>>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: >>>>>>>>>> >>>>>>>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>>>>>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>>>>>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>>>>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>>>>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>>>>>>>>>>> use of THP where it previously was able to. I can imagine this implies a >>>>>>>>>>>>> performance degradation for some workloads. This needs a proper >>>>>>>>>>>>> performance evaluation. >>>>>>>>>>>> >>>>>>>>>>>> I think the problem is more with the alloc_pages API. >>>>>>>>>>>> That gives you exactly the given order, and if there's >>>>>>>>>>>> a larger chunk available, it will split it up. >>>>>>>>>>>> >>>>>>>>>>>> But for balloon - I suspect lots of other users, >>>>>>>>>>>> we do not want to stress the system but if a large >>>>>>>>>>>> chunk is available anyway, then we could handle >>>>>>>>>>>> that more optimally by getting it all in one go. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>>>>>>> Along the lines of >>>>>>>>>>>> >>>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>>>>>>> unsigned int max_order, unsigned int *order) >>>>>>>>>>>> >>>>>>>>>>>> the idea would then be to return at a number of pages in the given >>>>>>>>>>>> range. >>>>>>>>>>>> >>>>>>>>>>>> What do you think? Want to try implementing that? >>>>>>>>>>> >>>>>>>>>>> You can just start with the highest order and decrement the order until >>>>>>>>>>> your allocation succeeds using alloc_pages(), which would be enough for >>>>>>>>>>> a first version. At least I don't see the immediate need for a new >>>>>>>>>>> kernel API. >>>>>>>>>> >>>>>>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is >>>>>>>>>> completely disabled, any of these calls can sleep. After it wakes up, >>>>>>>>>> we would like to get the larger order that has become available >>>>>>>>>> meanwhile. >>>>>>>>> >>>>>>>>> Yes, but that‘s a pure optimization IMHO. >>>>>>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. >>>>>>>> >>>>>>>> Well how do you propose implement the necessary semantics? >>>>>>>> I think we are both agreed that alloc_page_range is more or >>>>>>>> less what's necessary anyway - so how would you approximate it >>>>>>>> on top of existing APIs? >>>>>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h >>>> >>>> ..... >>>> >>>> >>>>>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c >>>>>>> index 26de020aae7b..067810b32813 100644 >>>>>>> --- a/mm/balloon_compaction.c >>>>>>> +++ b/mm/balloon_compaction.c >>>>>>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, >>>>>>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); >>>>>>> >>>>>>> /* >>>>>>> - * balloon_page_alloc - allocates a new page for insertion into the balloon >>>>>>> - * page list. >>>>>>> + * balloon_pages_alloc - allocates a new page (of at most the given order) >>>>>>> + * for insertion into the balloon page list. >>>>>>> * >>>>>>> * Driver must call this function to properly allocate a new balloon page. >>>>>>> * Driver must call balloon_page_enqueue before definitively removing the page >>>>>>> * from the guest system. >>>>>>> * >>>>>>> + * Will fall back to smaller orders if allocation fails. The order of the >>>>>>> + * allocated page is stored in page->private. >>>>>>> + * >>>>>>> * Return: struct page for the allocated page or NULL on allocation failure. >>>>>>> */ >>>>>>> -struct page *balloon_page_alloc(void) >>>>>>> +struct page *balloon_pages_alloc(int order) >>>>>>> { >>>>>>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | >>>>>>> - __GFP_NOMEMALLOC | __GFP_NORETRY | >>>>>>> - __GFP_NOWARN); >>>>>>> - return page; >>>>>>> + struct page *page; >>>>>>> + >>>>>>> + while (order >= 0) { >>>>>>> + page = alloc_pages(balloon_mapping_gfp_mask() | >>>>>>> + __GFP_NOMEMALLOC | __GFP_NORETRY | >>>>>>> + __GFP_NOWARN, order); >>>>>>> + if (page) { >>>>>>> + set_page_private(page, order); >>>>>>> + return page; >>>>>>> + } >>>>>>> + order--; >>>>>>> + } >>>>>>> + return NULL; >>>>>>> } >>>>>>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); >>>>>>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); >>>>>>> >>>>>>> /* >>>>>>> * balloon_page_enqueue - inserts a new page into the balloon page list. >>>>>> >>>>>> >>>>>> I think this will try to invoke direct reclaim from the first iteration >>>>>> to free up the max order. >>>>> >>>>> %__GFP_NORETRY: The VM implementation will try only very lightweight >>>>> memory direct reclaim to get some memory under memory pressure (thus it >>>>> can sleep). It will avoid disruptive actions like OOM killer. >>>>> >>>>> Certainly good enough for a first version I would say, no? >>>> >>>> Frankly how well that behaves would depend a lot on the workload. >>>> Can regress just as well. >>>> >>>> For the 1st version I'd prefer something that is the least disruptive, >>>> and that IMHO means we only trigger reclaim at all in the same configuration >>>> as now - when we can't satisfy the lowest order allocation. >>> >>> Agreed. >>> >>>> Anything else would be a huge amount of testing with all kind of >>>> workloads. >>> >>> So doing a "& ~__GFP_RECLAIM" in case order > 0? (as done in >>> GFP_TRANSHUGE_LIGHT) >> >> That will improve the situation when reclaim is not needed, but leave >> the problem in place for when it's needed: if reclaim does trigger, we >> can get a huge free page and immediately break it up. >> >> So it's ok as a first step but it will make the second step harder as >> we'll need to test with reclaim :). > > I expect the whole "steal huge pages from your guest" to be problematic, > as I already mentioned to Alex. This needs a performance evaluation. > > This all smells like a lot of workload dependent fine-tuning. :) AFAIK the hardware overheads of keeping huge-pages in the guest and backing them with 4KB pages are non-negligible. Did you take those into account?
On 31.03.20 18:37, Nadav Amit wrote: >> On Mar 31, 2020, at 7:09 AM, David Hildenbrand <david@redhat.com> wrote: >> >> On 31.03.20 16:07, Michael S. Tsirkin wrote: >>> On Tue, Mar 31, 2020 at 04:03:18PM +0200, David Hildenbrand wrote: >>>> On 31.03.20 15:37, Michael S. Tsirkin wrote: >>>>> On Tue, Mar 31, 2020 at 03:32:05PM +0200, David Hildenbrand wrote: >>>>>> On 31.03.20 15:24, Michael S. Tsirkin wrote: >>>>>>> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: >>>>>>>> On 26.03.20 10:49, Michael S. Tsirkin wrote: >>>>>>>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: >>>>>>>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: >>>>>>>>>>> >>>>>>>>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>>>>>>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>>>>>>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>>>>>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>>>>>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>>>>>>>>>>>> use of THP where it previously was able to. I can imagine this implies a >>>>>>>>>>>>>> performance degradation for some workloads. This needs a proper >>>>>>>>>>>>>> performance evaluation. >>>>>>>>>>>>> >>>>>>>>>>>>> I think the problem is more with the alloc_pages API. >>>>>>>>>>>>> That gives you exactly the given order, and if there's >>>>>>>>>>>>> a larger chunk available, it will split it up. >>>>>>>>>>>>> >>>>>>>>>>>>> But for balloon - I suspect lots of other users, >>>>>>>>>>>>> we do not want to stress the system but if a large >>>>>>>>>>>>> chunk is available anyway, then we could handle >>>>>>>>>>>>> that more optimally by getting it all in one go. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>>>>>>>> Along the lines of >>>>>>>>>>>>> >>>>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>>>>>>>> unsigned int max_order, unsigned int *order) >>>>>>>>>>>>> >>>>>>>>>>>>> the idea would then be to return at a number of pages in the given >>>>>>>>>>>>> range. >>>>>>>>>>>>> >>>>>>>>>>>>> What do you think? Want to try implementing that? >>>>>>>>>>>> >>>>>>>>>>>> You can just start with the highest order and decrement the order until >>>>>>>>>>>> your allocation succeeds using alloc_pages(), which would be enough for >>>>>>>>>>>> a first version. At least I don't see the immediate need for a new >>>>>>>>>>>> kernel API. >>>>>>>>>>> >>>>>>>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is >>>>>>>>>>> completely disabled, any of these calls can sleep. After it wakes up, >>>>>>>>>>> we would like to get the larger order that has become available >>>>>>>>>>> meanwhile. >>>>>>>>>> >>>>>>>>>> Yes, but that‘s a pure optimization IMHO. >>>>>>>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. >>>>>>>>> >>>>>>>>> Well how do you propose implement the necessary semantics? >>>>>>>>> I think we are both agreed that alloc_page_range is more or >>>>>>>>> less what's necessary anyway - so how would you approximate it >>>>>>>>> on top of existing APIs? >>>>>>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h >>>>> >>>>> ..... >>>>> >>>>> >>>>>>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c >>>>>>>> index 26de020aae7b..067810b32813 100644 >>>>>>>> --- a/mm/balloon_compaction.c >>>>>>>> +++ b/mm/balloon_compaction.c >>>>>>>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, >>>>>>>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); >>>>>>>> >>>>>>>> /* >>>>>>>> - * balloon_page_alloc - allocates a new page for insertion into the balloon >>>>>>>> - * page list. >>>>>>>> + * balloon_pages_alloc - allocates a new page (of at most the given order) >>>>>>>> + * for insertion into the balloon page list. >>>>>>>> * >>>>>>>> * Driver must call this function to properly allocate a new balloon page. >>>>>>>> * Driver must call balloon_page_enqueue before definitively removing the page >>>>>>>> * from the guest system. >>>>>>>> * >>>>>>>> + * Will fall back to smaller orders if allocation fails. The order of the >>>>>>>> + * allocated page is stored in page->private. >>>>>>>> + * >>>>>>>> * Return: struct page for the allocated page or NULL on allocation failure. >>>>>>>> */ >>>>>>>> -struct page *balloon_page_alloc(void) >>>>>>>> +struct page *balloon_pages_alloc(int order) >>>>>>>> { >>>>>>>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | >>>>>>>> - __GFP_NOMEMALLOC | __GFP_NORETRY | >>>>>>>> - __GFP_NOWARN); >>>>>>>> - return page; >>>>>>>> + struct page *page; >>>>>>>> + >>>>>>>> + while (order >= 0) { >>>>>>>> + page = alloc_pages(balloon_mapping_gfp_mask() | >>>>>>>> + __GFP_NOMEMALLOC | __GFP_NORETRY | >>>>>>>> + __GFP_NOWARN, order); >>>>>>>> + if (page) { >>>>>>>> + set_page_private(page, order); >>>>>>>> + return page; >>>>>>>> + } >>>>>>>> + order--; >>>>>>>> + } >>>>>>>> + return NULL; >>>>>>>> } >>>>>>>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); >>>>>>>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); >>>>>>>> >>>>>>>> /* >>>>>>>> * balloon_page_enqueue - inserts a new page into the balloon page list. >>>>>>> >>>>>>> >>>>>>> I think this will try to invoke direct reclaim from the first iteration >>>>>>> to free up the max order. >>>>>> >>>>>> %__GFP_NORETRY: The VM implementation will try only very lightweight >>>>>> memory direct reclaim to get some memory under memory pressure (thus it >>>>>> can sleep). It will avoid disruptive actions like OOM killer. >>>>>> >>>>>> Certainly good enough for a first version I would say, no? >>>>> >>>>> Frankly how well that behaves would depend a lot on the workload. >>>>> Can regress just as well. >>>>> >>>>> For the 1st version I'd prefer something that is the least disruptive, >>>>> and that IMHO means we only trigger reclaim at all in the same configuration >>>>> as now - when we can't satisfy the lowest order allocation. >>>> >>>> Agreed. >>>> >>>>> Anything else would be a huge amount of testing with all kind of >>>>> workloads. >>>> >>>> So doing a "& ~__GFP_RECLAIM" in case order > 0? (as done in >>>> GFP_TRANSHUGE_LIGHT) >>> >>> That will improve the situation when reclaim is not needed, but leave >>> the problem in place for when it's needed: if reclaim does trigger, we >>> can get a huge free page and immediately break it up. >>> >>> So it's ok as a first step but it will make the second step harder as >>> we'll need to test with reclaim :). >> >> I expect the whole "steal huge pages from your guest" to be problematic, >> as I already mentioned to Alex. This needs a performance evaluation. >> >> This all smells like a lot of workload dependent fine-tuning. :) > > AFAIK the hardware overheads of keeping huge-pages in the guest and backing > them with 4KB pages are non-negligible. Did you take those into account? Of course, the fastest mapping will be huge pages in host and guest. Having huge pages in your guest but not in your host cannot really be solved using ballooning AFAIKs. Hopefully THP in the host will be doing its job properly :) ... however, so far, we haven't done any performance comparisons at all. The only numbers from Hui Zhu that I can spot are number of THP in the host, which is not really expressing actual guest performance IMHO. That definitely has to be done to evaluate the different optimizations we might want to try out.
On 31.03.20 18:27, Nadav Amit wrote: >> On Mar 31, 2020, at 6:32 AM, David Hildenbrand <david@redhat.com> wrote: >> >> On 31.03.20 15:24, Michael S. Tsirkin wrote: >>> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: >>>> On 26.03.20 10:49, Michael S. Tsirkin wrote: >>>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: >>>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: >>>>>>> >>>>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>>>>>>>> use of THP where it previously was able to. I can imagine this implies a >>>>>>>>>> performance degradation for some workloads. This needs a proper >>>>>>>>>> performance evaluation. >>>>>>>>> >>>>>>>>> I think the problem is more with the alloc_pages API. >>>>>>>>> That gives you exactly the given order, and if there's >>>>>>>>> a larger chunk available, it will split it up. >>>>>>>>> >>>>>>>>> But for balloon - I suspect lots of other users, >>>>>>>>> we do not want to stress the system but if a large >>>>>>>>> chunk is available anyway, then we could handle >>>>>>>>> that more optimally by getting it all in one go. >>>>>>>>> >>>>>>>>> >>>>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>>>> Along the lines of >>>>>>>>> >>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>>>> unsigned int max_order, unsigned int *order) >>>>>>>>> >>>>>>>>> the idea would then be to return at a number of pages in the given >>>>>>>>> range. >>>>>>>>> >>>>>>>>> What do you think? Want to try implementing that? >>>>>>>> >>>>>>>> You can just start with the highest order and decrement the order until >>>>>>>> your allocation succeeds using alloc_pages(), which would be enough for >>>>>>>> a first version. At least I don't see the immediate need for a new >>>>>>>> kernel API. >>>>>>> >>>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is >>>>>>> completely disabled, any of these calls can sleep. After it wakes up, >>>>>>> we would like to get the larger order that has become available >>>>>>> meanwhile. >>>>>> >>>>>> Yes, but that‘s a pure optimization IMHO. >>>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. >>>>> >>>>> Well how do you propose implement the necessary semantics? >>>>> I think we are both agreed that alloc_page_range is more or >>>>> less what's necessary anyway - so how would you approximate it >>>>> on top of existing APIs? >>>> >>>> Looking at drivers/misc/vmw_balloon.c:vmballoon_inflate(), it first >>>> tries to allocate huge pages using >>>> >>>> alloc_pages(__GFP_HIGHMEM|__GFP_NOWARN| __GFP_NOMEMALLOC, >>>> VMW_BALLOON_2M_ORDER) >>>> >>>> And then falls back to 4k allocations (balloon_page_alloc()) in case >>>> allocation fails. >>>> >>>> I'm roughly thinking of something like the following, but with an >>>> optimized reporting interface/bigger pfn array so we can report > >>>> 1MB at a time. Also, it might make sense to remember the order that >>>> succeeded across some fill_balloon() calls. >>>> >>>> Don't even expect it to compile ... >>>> >>>> >>>> >>>>> From 4305f989672ccca4be9293e6d4167e929f3e299b Mon Sep 17 00:00:00 2001 >>>> From: David Hildenbrand <david@redhat.com> >>>> Date: Tue, 31 Mar 2020 12:28:07 +0200 >>>> Subject: [PATCH RFC] tmp >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> drivers/virtio/virtio_balloon.c | 38 ++++++++++++++++++-------- >>>> include/linux/balloon_compaction.h | 7 ++++- >>>> mm/balloon_compaction.c | 43 +++++++++++++++++++++++------- >>>> 3 files changed, 67 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c >>>> index 8511d258dbb4..0660b1b988f0 100644 >>>> --- a/drivers/virtio/virtio_balloon.c >>>> +++ b/drivers/virtio/virtio_balloon.c >>>> @@ -187,7 +187,7 @@ int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_info, >>>> } >>>> >>>> static void set_page_pfns(struct virtio_balloon *vb, >>>> - __virtio32 pfns[], struct page *page) >>>> + __virtio32 pfns[], struct page *page, int order) >>>> { >>>> unsigned int i; >>>> >>>> @@ -197,7 +197,7 @@ static void set_page_pfns(struct virtio_balloon *vb, >>>> * Set balloon pfns pointing at this page. >>>> * Note that the first pfn points at start of the page. >>>> */ >>>> - for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++) >>>> + for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); i++) >>>> pfns[i] = cpu_to_virtio32(vb->vdev, >>>> page_to_balloon_pfn(page) + i); >>>> } >>>> @@ -205,6 +205,7 @@ static void set_page_pfns(struct virtio_balloon *vb, >>>> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >>>> { >>>> unsigned num_allocated_pages; >>>> + int order = MAX_ORDER - 1; >>>> unsigned num_pfns; >>>> struct page *page; >>>> LIST_HEAD(pages); >>>> @@ -212,9 +213,20 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >>>> /* We can only do one array worth at a time. */ >>>> num = min(num, ARRAY_SIZE(vb->pfns)); >>>> >>>> + /* >>>> + * Note: we will currently never allocate more than 1MB due to the >>>> + * pfn array size, so we will not allocate MAX_ORDER - 1 ... >>>> + */ >>>> + >>>> for (num_pfns = 0; num_pfns < num; >>>> - num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { >>>> - struct page *page = balloon_page_alloc(); >>>> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order)) { >>>> + const unsigned long remaining = num - num_pfns; >>>> + >>>> + order = MIN(order, >>>> + get_order(remaining << VIRTIO_BALLOON_PFN_SHIFT)); >>>> + if ((1 << order) * VIRTIO_BALLOON_PAGES_PER_PAGE > remaining) >>>> + order--; >>>> + page = balloon_pages_alloc(order); >>>> >>>> if (!page) { >>>> dev_info_ratelimited(&vb->vdev->dev, >>>> @@ -225,6 +237,8 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >>>> break; >>>> } >>>> >>>> + /* Continue with the actual order that succeeded. */ >>>> + order = page_private(page); >>>> balloon_page_push(&pages, page); >>>> } >>>> >>>> @@ -233,14 +247,16 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) >>>> vb->num_pfns = 0; >>>> >>>> while ((page = balloon_page_pop(&pages))) { >>>> + order = page_order(page); >>>> + /* enqueuing will split the page and clear the order */ >>>> balloon_page_enqueue(&vb->vb_dev_info, page); >>>> >>>> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); >>>> - vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; >>>> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page, order); >>>> + vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); >>>> if (!virtio_has_feature(vb->vdev, >>>> VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) >>>> - adjust_managed_page_count(page, -1); >>>> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE; >>>> + adjust_managed_page_count(page, -1 * (1 << order)); >>>> + vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE * (1 << order); >>>> } >>>> >>>> num_allocated_pages = vb->num_pfns; >>>> @@ -284,7 +300,7 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num) >>>> page = balloon_page_dequeue(vb_dev_info); >>>> if (!page) >>>> break; >>>> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); >>>> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page, 0); >>>> list_add(&page->lru, &pages); >>>> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE; >>>> } >>>> @@ -786,7 +802,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, >>>> __count_vm_event(BALLOON_MIGRATE); >>>> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); >>>> vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; >>>> - set_page_pfns(vb, vb->pfns, newpage); >>>> + set_page_pfns(vb, vb->pfns, newpage, 0); >>>> tell_host(vb, vb->inflate_vq); >>>> >>>> /* balloon's page migration 2nd step -- deflate "page" */ >>>> @@ -794,7 +810,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, >>>> balloon_page_delete(page); >>>> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); >>>> vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; >>>> - set_page_pfns(vb, vb->pfns, page); >>>> + set_page_pfns(vb, vb->pfns, page, 0); >>>> tell_host(vb, vb->deflate_vq); >>>> >>>> mutex_unlock(&vb->balloon_lock); >>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h >>>> index 338aa27e4773..ed93fe5704d1 100644 >>>> --- a/include/linux/balloon_compaction.h >>>> +++ b/include/linux/balloon_compaction.h >>>> @@ -60,7 +60,7 @@ struct balloon_dev_info { >>>> struct inode *inode; >>>> }; >>>> >>>> -extern struct page *balloon_page_alloc(void); >>>> +extern struct page *balloon_pages_alloc(int order); >>>> extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info, >>>> struct page *page); >>>> extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info); >>>> @@ -78,6 +78,11 @@ static inline void balloon_devinfo_init(struct balloon_dev_info *balloon) >>>> balloon->inode = NULL; >>>> } >>>> >>>> +static inline struct page *balloon_page_alloc(void) >>>> +{ >>>> + return balloon_pages_alloc(0); >>>> +} >>>> + >>>> #ifdef CONFIG_BALLOON_COMPACTION >>>> extern const struct address_space_operations balloon_aops; >>>> extern bool balloon_page_isolate(struct page *page, >>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c >>>> index 26de020aae7b..067810b32813 100644 >>>> --- a/mm/balloon_compaction.c >>>> +++ b/mm/balloon_compaction.c >>>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, >>>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); >>>> >>>> /* >>>> - * balloon_page_alloc - allocates a new page for insertion into the balloon >>>> - * page list. >>>> + * balloon_pages_alloc - allocates a new page (of at most the given order) >>>> + * for insertion into the balloon page list. >>>> * >>>> * Driver must call this function to properly allocate a new balloon page. >>>> * Driver must call balloon_page_enqueue before definitively removing the page >>>> * from the guest system. >>>> * >>>> + * Will fall back to smaller orders if allocation fails. The order of the >>>> + * allocated page is stored in page->private. >>>> + * >>>> * Return: struct page for the allocated page or NULL on allocation failure. >>>> */ >>>> -struct page *balloon_page_alloc(void) >>>> +struct page *balloon_pages_alloc(int order) >>>> { >>>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | >>>> - __GFP_NOMEMALLOC | __GFP_NORETRY | >>>> - __GFP_NOWARN); >>>> - return page; >>>> + struct page *page; >>>> + >>>> + while (order >= 0) { >>>> + page = alloc_pages(balloon_mapping_gfp_mask() | >>>> + __GFP_NOMEMALLOC | __GFP_NORETRY | >>>> + __GFP_NOWARN, order); >>>> + if (page) { >>>> + set_page_private(page, order); >>>> + return page; >>>> + } >>>> + order--; >>>> + } >>>> + return NULL; >>>> } >>>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); >>>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); >>>> >>>> /* >>>> * balloon_page_enqueue - inserts a new page into the balloon page list. >>> >>> >>> I think this will try to invoke direct reclaim from the first iteration >>> to free up the max order. >> >> %__GFP_NORETRY: The VM implementation will try only very lightweight >> memory direct reclaim to get some memory under memory pressure (thus it >> can sleep). It will avoid disruptive actions like OOM killer. >> >> Certainly good enough for a first version I would say, no? Looking at >> the vmware balloon, they don't even set __GFP_NORETRY. > > Yes, it does seem that we are missing __GFP_NORETRY. I really do not know > what I was thinking when I did not add it for huge-pages allocation. I will > send a patch. Thanks for noticing :) > > In regard to your patch, I would be happy to consolidate the allocation > mechanisms, so VMware balloon driver would also use your code. In general > your code looks good, take-away some style issues. Yeah, let's see in which direction we'll be bringing balloon_page_alloc(), I think there are still some questions to be answered (mostly performance implications). Cheers!
> 2020年4月1日 17:48，David Hildenbrand <david@redhat.com> 写道： > > On 31.03.20 18:37, Nadav Amit wrote: >>> On Mar 31, 2020, at 7:09 AM, David Hildenbrand <david@redhat.com> wrote: >>> >>> On 31.03.20 16:07, Michael S. Tsirkin wrote: >>>> On Tue, Mar 31, 2020 at 04:03:18PM +0200, David Hildenbrand wrote: >>>>> On 31.03.20 15:37, Michael S. Tsirkin wrote: >>>>>> On Tue, Mar 31, 2020 at 03:32:05PM +0200, David Hildenbrand wrote: >>>>>>> On 31.03.20 15:24, Michael S. Tsirkin wrote: >>>>>>>> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: >>>>>>>>> On 26.03.20 10:49, Michael S. Tsirkin wrote: >>>>>>>>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: >>>>>>>>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: >>>>>>>>>>>> >>>>>>>>>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>>>>>>>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>>>>>>>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>>>>>>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>>>>>>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>>>>>>>>>>>>> use of THP where it previously was able to. I can imagine this implies a >>>>>>>>>>>>>>> performance degradation for some workloads. This needs a proper >>>>>>>>>>>>>>> performance evaluation. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I think the problem is more with the alloc_pages API. >>>>>>>>>>>>>> That gives you exactly the given order, and if there's >>>>>>>>>>>>>> a larger chunk available, it will split it up. >>>>>>>>>>>>>> >>>>>>>>>>>>>> But for balloon - I suspect lots of other users, >>>>>>>>>>>>>> we do not want to stress the system but if a large >>>>>>>>>>>>>> chunk is available anyway, then we could handle >>>>>>>>>>>>>> that more optimally by getting it all in one go. >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>>>>>>>>> Along the lines of >>>>>>>>>>>>>> >>>>>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>>>>>>>>> unsigned int max_order, unsigned int *order) >>>>>>>>>>>>>> >>>>>>>>>>>>>> the idea would then be to return at a number of pages in the given >>>>>>>>>>>>>> range. >>>>>>>>>>>>>> >>>>>>>>>>>>>> What do you think? Want to try implementing that? >>>>>>>>>>>>> >>>>>>>>>>>>> You can just start with the highest order and decrement the order until >>>>>>>>>>>>> your allocation succeeds using alloc_pages(), which would be enough for >>>>>>>>>>>>> a first version. At least I don't see the immediate need for a new >>>>>>>>>>>>> kernel API. >>>>>>>>>>>> >>>>>>>>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is >>>>>>>>>>>> completely disabled, any of these calls can sleep. After it wakes up, >>>>>>>>>>>> we would like to get the larger order that has become available >>>>>>>>>>>> meanwhile. >>>>>>>>>>> >>>>>>>>>>> Yes, but that‘s a pure optimization IMHO. >>>>>>>>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. >>>>>>>>>> >>>>>>>>>> Well how do you propose implement the necessary semantics? >>>>>>>>>> I think we are both agreed that alloc_page_range is more or >>>>>>>>>> less what's necessary anyway - so how would you approximate it >>>>>>>>>> on top of existing APIs? >>>>>>>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h >>>>>> >>>>>> ..... >>>>>> >>>>>> >>>>>>>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c >>>>>>>>> index 26de020aae7b..067810b32813 100644 >>>>>>>>> --- a/mm/balloon_compaction.c >>>>>>>>> +++ b/mm/balloon_compaction.c >>>>>>>>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, >>>>>>>>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); >>>>>>>>> >>>>>>>>> /* >>>>>>>>> - * balloon_page_alloc - allocates a new page for insertion into the balloon >>>>>>>>> - * page list. >>>>>>>>> + * balloon_pages_alloc - allocates a new page (of at most the given order) >>>>>>>>> + * for insertion into the balloon page list. >>>>>>>>> * >>>>>>>>> * Driver must call this function to properly allocate a new balloon page. >>>>>>>>> * Driver must call balloon_page_enqueue before definitively removing the page >>>>>>>>> * from the guest system. >>>>>>>>> * >>>>>>>>> + * Will fall back to smaller orders if allocation fails. The order of the >>>>>>>>> + * allocated page is stored in page->private. >>>>>>>>> + * >>>>>>>>> * Return: struct page for the allocated page or NULL on allocation failure. >>>>>>>>> */ >>>>>>>>> -struct page *balloon_page_alloc(void) >>>>>>>>> +struct page *balloon_pages_alloc(int order) >>>>>>>>> { >>>>>>>>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | >>>>>>>>> - __GFP_NOMEMALLOC | __GFP_NORETRY | >>>>>>>>> - __GFP_NOWARN); >>>>>>>>> - return page; >>>>>>>>> + struct page *page; >>>>>>>>> + >>>>>>>>> + while (order >= 0) { >>>>>>>>> + page = alloc_pages(balloon_mapping_gfp_mask() | >>>>>>>>> + __GFP_NOMEMALLOC | __GFP_NORETRY | >>>>>>>>> + __GFP_NOWARN, order); >>>>>>>>> + if (page) { >>>>>>>>> + set_page_private(page, order); >>>>>>>>> + return page; >>>>>>>>> + } >>>>>>>>> + order--; >>>>>>>>> + } >>>>>>>>> + return NULL; >>>>>>>>> } >>>>>>>>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); >>>>>>>>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); >>>>>>>>> >>>>>>>>> /* >>>>>>>>> * balloon_page_enqueue - inserts a new page into the balloon page list. >>>>>>>> >>>>>>>> >>>>>>>> I think this will try to invoke direct reclaim from the first iteration >>>>>>>> to free up the max order. >>>>>>> >>>>>>> %__GFP_NORETRY: The VM implementation will try only very lightweight >>>>>>> memory direct reclaim to get some memory under memory pressure (thus it >>>>>>> can sleep). It will avoid disruptive actions like OOM killer. >>>>>>> >>>>>>> Certainly good enough for a first version I would say, no? >>>>>> >>>>>> Frankly how well that behaves would depend a lot on the workload. >>>>>> Can regress just as well. >>>>>> >>>>>> For the 1st version I'd prefer something that is the least disruptive, >>>>>> and that IMHO means we only trigger reclaim at all in the same configuration >>>>>> as now - when we can't satisfy the lowest order allocation. >>>>> >>>>> Agreed. >>>>> >>>>>> Anything else would be a huge amount of testing with all kind of >>>>>> workloads. >>>>> >>>>> So doing a "& ~__GFP_RECLAIM" in case order > 0? (as done in >>>>> GFP_TRANSHUGE_LIGHT) >>>> >>>> That will improve the situation when reclaim is not needed, but leave >>>> the problem in place for when it's needed: if reclaim does trigger, we >>>> can get a huge free page and immediately break it up. >>>> >>>> So it's ok as a first step but it will make the second step harder as >>>> we'll need to test with reclaim :). >>> >>> I expect the whole "steal huge pages from your guest" to be problematic, >>> as I already mentioned to Alex. This needs a performance evaluation. >>> >>> This all smells like a lot of workload dependent fine-tuning. :) >> >> AFAIK the hardware overheads of keeping huge-pages in the guest and backing >> them with 4KB pages are non-negligible. Did you take those into account? > > Of course, the fastest mapping will be huge pages in host and guest. > Having huge pages in your guest but not in your host cannot really be > solved using ballooning AFAIKs. Hopefully THP in the host will be doing > its job properly :) > > ... however, so far, we haven't done any performance comparisons at all. > The only numbers from Hui Zhu that I can spot are number of THP in the > host, which is not really expressing actual guest performance IMHO. That > definitely has to be done to evaluate the different optimizations we > might want to try out. > I did some tests with vm-scalability on Monday comparing their performance in VM: //4 processes random r/w usemem -R -a -Z -n 4 1g write: hugepage: 146367 KB/s thp: 133550 KB/s normal: 124248 KB/s read: hugepage: 103969 KB/s thp: 100622 KB/s normal: 88755 KB/s Best, Hui > -- > Thanks, > > David / dhildenb
> 2020年3月31日 22:07，Michael S. Tsirkin <mst@redhat.com> 写道： > > On Tue, Mar 31, 2020 at 04:03:18PM +0200, David Hildenbrand wrote: >> On 31.03.20 15:37, Michael S. Tsirkin wrote: >>> On Tue, Mar 31, 2020 at 03:32:05PM +0200, David Hildenbrand wrote: >>>> On 31.03.20 15:24, Michael S. Tsirkin wrote: >>>>> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: >>>>>> On 26.03.20 10:49, Michael S. Tsirkin wrote: >>>>>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: >>>>>>>> >>>>>>>> >>>>>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: >>>>>>>>> >>>>>>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: >>>>>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: >>>>>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: >>>>>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest >>>>>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make >>>>>>>>>>>> use of THP where it previously was able to. I can imagine this implies a >>>>>>>>>>>> performance degradation for some workloads. This needs a proper >>>>>>>>>>>> performance evaluation. >>>>>>>>>>> >>>>>>>>>>> I think the problem is more with the alloc_pages API. >>>>>>>>>>> That gives you exactly the given order, and if there's >>>>>>>>>>> a larger chunk available, it will split it up. >>>>>>>>>>> >>>>>>>>>>> But for balloon - I suspect lots of other users, >>>>>>>>>>> we do not want to stress the system but if a large >>>>>>>>>>> chunk is available anyway, then we could handle >>>>>>>>>>> that more optimally by getting it all in one go. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. >>>>>>>>>>> Along the lines of >>>>>>>>>>> >>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, >>>>>>>>>>> unsigned int max_order, unsigned int *order) >>>>>>>>>>> >>>>>>>>>>> the idea would then be to return at a number of pages in the given >>>>>>>>>>> range. >>>>>>>>>>> >>>>>>>>>>> What do you think? Want to try implementing that? >>>>>>>>>> >>>>>>>>>> You can just start with the highest order and decrement the order until >>>>>>>>>> your allocation succeeds using alloc_pages(), which would be enough for >>>>>>>>>> a first version. At least I don't see the immediate need for a new >>>>>>>>>> kernel API. >>>>>>>>> >>>>>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is >>>>>>>>> completely disabled, any of these calls can sleep. After it wakes up, >>>>>>>>> we would like to get the larger order that has become available >>>>>>>>> meanwhile. >>>>>>>>> >>>>>>>> >>>>>>>> Yes, but that‘s a pure optimization IMHO. >>>>>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. >>>>>>>> >>>>>>> >>>>>>> Well how do you propose implement the necessary semantics? >>>>>>> I think we are both agreed that alloc_page_range is more or >>>>>>> less what's necessary anyway - so how would you approximate it >>>>>>> on top of existing APIs? >>>>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h >>> >>> ..... >>> >>> >>>>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c >>>>>> index 26de020aae7b..067810b32813 100644 >>>>>> --- a/mm/balloon_compaction.c >>>>>> +++ b/mm/balloon_compaction.c >>>>>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, >>>>>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); >>>>>> >>>>>> /* >>>>>> - * balloon_page_alloc - allocates a new page for insertion into the balloon >>>>>> - * page list. >>>>>> + * balloon_pages_alloc - allocates a new page (of at most the given order) >>>>>> + * for insertion into the balloon page list. >>>>>> * >>>>>> * Driver must call this function to properly allocate a new balloon page. >>>>>> * Driver must call balloon_page_enqueue before definitively removing the page >>>>>> * from the guest system. >>>>>> * >>>>>> + * Will fall back to smaller orders if allocation fails. The order of the >>>>>> + * allocated page is stored in page->private. >>>>>> + * >>>>>> * Return: struct page for the allocated page or NULL on allocation failure. >>>>>> */ >>>>>> -struct page *balloon_page_alloc(void) >>>>>> +struct page *balloon_pages_alloc(int order) >>>>>> { >>>>>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | >>>>>> - __GFP_NOMEMALLOC | __GFP_NORETRY | >>>>>> - __GFP_NOWARN); >>>>>> - return page; >>>>>> + struct page *page; >>>>>> + >>>>>> + while (order >= 0) { >>>>>> + page = alloc_pages(balloon_mapping_gfp_mask() | >>>>>> + __GFP_NOMEMALLOC | __GFP_NORETRY | >>>>>> + __GFP_NOWARN, order); >>>>>> + if (page) { >>>>>> + set_page_private(page, order); >>>>>> + return page; >>>>>> + } >>>>>> + order--; >>>>>> + } >>>>>> + return NULL; >>>>>> } >>>>>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); >>>>>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); >>>>>> >>>>>> /* >>>>>> * balloon_page_enqueue - inserts a new page into the balloon page list. >>>>> >>>>> >>>>> I think this will try to invoke direct reclaim from the first iteration >>>>> to free up the max order. >>>> >>>> %__GFP_NORETRY: The VM implementation will try only very lightweight >>>> memory direct reclaim to get some memory under memory pressure (thus it >>>> can sleep). It will avoid disruptive actions like OOM killer. >>>> >>>> Certainly good enough for a first version I would say, no? >>> >>> Frankly how well that behaves would depend a lot on the workload. >>> Can regress just as well. >>> >>> For the 1st version I'd prefer something that is the least disruptive, >>> and that IMHO means we only trigger reclaim at all in the same configuration >>> as now - when we can't satisfy the lowest order allocation. >> >> Agreed. >> >>> >>> Anything else would be a huge amount of testing with all kind of >>> workloads. >>> >> >> So doing a "& ~__GFP_RECLAIM" in case order > 0? (as done in >> GFP_TRANSHUGE_LIGHT) > > That will improve the situation when reclaim is not needed, but leave > the problem in place for when it's needed: if reclaim does trigger, we > can get a huge free page and immediately break it up. > > So it's ok as a first step but it will make the second step harder as > we'll need to test with reclaim :). I worry that will increases the allocation failure rate for large pages. I tried alloc 2M memory without __GFP_RECLAIM when I wrote the VIRTIO_BALLOON_F_THP_ORDER first version. It will fail when I use usemem punch-holes function generates 400m fragmentation pages in the guest kernel. What about add another option to balloon to control with __GFP_RECLAIM or without it? Best, Hui > > >> -- >> Thanks, >> >> David / dhildenb
On Thu, Apr 02, 2020 at 04:00:05PM +0800, teawater wrote: > > > > 2020年3月31日 22:07，Michael S. Tsirkin <mst@redhat.com> 写道： > > > > On Tue, Mar 31, 2020 at 04:03:18PM +0200, David Hildenbrand wrote: > >> On 31.03.20 15:37, Michael S. Tsirkin wrote: > >>> On Tue, Mar 31, 2020 at 03:32:05PM +0200, David Hildenbrand wrote: > >>>> On 31.03.20 15:24, Michael S. Tsirkin wrote: > >>>>> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: > >>>>>> On 26.03.20 10:49, Michael S. Tsirkin wrote: > >>>>>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst@redhat.com>: > >>>>>>>>> > >>>>>>>>> On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: > >>>>>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: > >>>>>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: > >>>>>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest > >>>>>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make > >>>>>>>>>>>> use of THP where it previously was able to. I can imagine this implies a > >>>>>>>>>>>> performance degradation for some workloads. This needs a proper > >>>>>>>>>>>> performance evaluation. > >>>>>>>>>>> > >>>>>>>>>>> I think the problem is more with the alloc_pages API. > >>>>>>>>>>> That gives you exactly the given order, and if there's > >>>>>>>>>>> a larger chunk available, it will split it up. > >>>>>>>>>>> > >>>>>>>>>>> But for balloon - I suspect lots of other users, > >>>>>>>>>>> we do not want to stress the system but if a large > >>>>>>>>>>> chunk is available anyway, then we could handle > >>>>>>>>>>> that more optimally by getting it all in one go. > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. > >>>>>>>>>>> Along the lines of > >>>>>>>>>>> > >>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > >>>>>>>>>>> unsigned int max_order, unsigned int *order) > >>>>>>>>>>> > >>>>>>>>>>> the idea would then be to return at a number of pages in the given > >>>>>>>>>>> range. > >>>>>>>>>>> > >>>>>>>>>>> What do you think? Want to try implementing that? > >>>>>>>>>> > >>>>>>>>>> You can just start with the highest order and decrement the order until > >>>>>>>>>> your allocation succeeds using alloc_pages(), which would be enough for > >>>>>>>>>> a first version. At least I don't see the immediate need for a new > >>>>>>>>>> kernel API. > >>>>>>>>> > >>>>>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is > >>>>>>>>> completely disabled, any of these calls can sleep. After it wakes up, > >>>>>>>>> we would like to get the larger order that has become available > >>>>>>>>> meanwhile. > >>>>>>>>> > >>>>>>>> > >>>>>>>> Yes, but that‘s a pure optimization IMHO. > >>>>>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. > >>>>>>>> > >>>>>>> > >>>>>>> Well how do you propose implement the necessary semantics? > >>>>>>> I think we are both agreed that alloc_page_range is more or > >>>>>>> less what's necessary anyway - so how would you approximate it > >>>>>>> on top of existing APIs? > >>>>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h > >>> > >>> ..... > >>> > >>> > >>>>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c > >>>>>> index 26de020aae7b..067810b32813 100644 > >>>>>> --- a/mm/balloon_compaction.c > >>>>>> +++ b/mm/balloon_compaction.c > >>>>>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, > >>>>>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); > >>>>>> > >>>>>> /* > >>>>>> - * balloon_page_alloc - allocates a new page for insertion into the balloon > >>>>>> - * page list. > >>>>>> + * balloon_pages_alloc - allocates a new page (of at most the given order) > >>>>>> + * for insertion into the balloon page list. > >>>>>> * > >>>>>> * Driver must call this function to properly allocate a new balloon page. > >>>>>> * Driver must call balloon_page_enqueue before definitively removing the page > >>>>>> * from the guest system. > >>>>>> * > >>>>>> + * Will fall back to smaller orders if allocation fails. The order of the > >>>>>> + * allocated page is stored in page->private. > >>>>>> + * > >>>>>> * Return: struct page for the allocated page or NULL on allocation failure. > >>>>>> */ > >>>>>> -struct page *balloon_page_alloc(void) > >>>>>> +struct page *balloon_pages_alloc(int order) > >>>>>> { > >>>>>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | > >>>>>> - __GFP_NOMEMALLOC | __GFP_NORETRY | > >>>>>> - __GFP_NOWARN); > >>>>>> - return page; > >>>>>> + struct page *page; > >>>>>> + > >>>>>> + while (order >= 0) { > >>>>>> + page = alloc_pages(balloon_mapping_gfp_mask() | > >>>>>> + __GFP_NOMEMALLOC | __GFP_NORETRY | > >>>>>> + __GFP_NOWARN, order); > >>>>>> + if (page) { > >>>>>> + set_page_private(page, order); > >>>>>> + return page; > >>>>>> + } > >>>>>> + order--; > >>>>>> + } > >>>>>> + return NULL; > >>>>>> } > >>>>>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); > >>>>>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); > >>>>>> > >>>>>> /* > >>>>>> * balloon_page_enqueue - inserts a new page into the balloon page list. > >>>>> > >>>>> > >>>>> I think this will try to invoke direct reclaim from the first iteration > >>>>> to free up the max order. > >>>> > >>>> %__GFP_NORETRY: The VM implementation will try only very lightweight > >>>> memory direct reclaim to get some memory under memory pressure (thus it > >>>> can sleep). It will avoid disruptive actions like OOM killer. > >>>> > >>>> Certainly good enough for a first version I would say, no? > >>> > >>> Frankly how well that behaves would depend a lot on the workload. > >>> Can regress just as well. > >>> > >>> For the 1st version I'd prefer something that is the least disruptive, > >>> and that IMHO means we only trigger reclaim at all in the same configuration > >>> as now - when we can't satisfy the lowest order allocation. > >> > >> Agreed. > >> > >>> > >>> Anything else would be a huge amount of testing with all kind of > >>> workloads. > >>> > >> > >> So doing a "& ~__GFP_RECLAIM" in case order > 0? (as done in > >> GFP_TRANSHUGE_LIGHT) > > > > That will improve the situation when reclaim is not needed, but leave > > the problem in place for when it's needed: if reclaim does trigger, we > > can get a huge free page and immediately break it up. > > > > So it's ok as a first step but it will make the second step harder as > > we'll need to test with reclaim :). > > > I worry that will increases the allocation failure rate for large pages. > > I tried alloc 2M memory without __GFP_RECLAIM when I wrote the VIRTIO_BALLOON_F_THP_ORDER first version. > It will fail when I use usemem punch-holes function generates 400m fragmentation pages in the guest kernel. > > What about add another option to balloon to control with __GFP_RECLAIM or without it? > > Best, > Hui That is why I suggested a new API so we do not fragment memory. > > > > > >> -- > >> Thanks, > >> > >> David / dhildenb
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 7bfe365..1e1dc76 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -175,18 +175,31 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) unsigned num_pfns; struct page *page; LIST_HEAD(pages); + int page_order = 0; /* We can only do one array worth at a time. */ num = min(num, ARRAY_SIZE(vb->pfns)); + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_THP_ORDER)) + page_order = VIRTIO_BALLOON_THP_ORDER; + for (num_pfns = 0; num_pfns < num; num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { - struct page *page = balloon_page_alloc(); + struct page *page; + + if (page_order) + page = alloc_pages(__GFP_HIGHMEM | + __GFP_KSWAPD_RECLAIM | + __GFP_RETRY_MAYFAIL | + __GFP_NOWARN | __GFP_NOMEMALLOC, + page_order); + else + page = balloon_page_alloc(); if (!page) { dev_info_ratelimited(&vb->vdev->dev, - "Out of puff! Can't get %u pages\n", - VIRTIO_BALLOON_PAGES_PER_PAGE); + "Out of puff! Can't get %u pages\n", + VIRTIO_BALLOON_PAGES_PER_PAGE << page_order); /* Sleep for at least 1/5 of a second before retry. */ msleep(200); break; @@ -206,7 +219,7 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) - adjust_managed_page_count(page, -1); + adjust_managed_page_count(page, -(1 << page_order)); vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE; } @@ -223,13 +236,20 @@ static void release_pages_balloon(struct virtio_balloon *vb, struct list_head *pages) { struct page *page, *next; + int page_order = 0; + + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_THP_ORDER)) + page_order = VIRTIO_BALLOON_THP_ORDER; list_for_each_entry_safe(page, next, pages, lru) { if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) - adjust_managed_page_count(page, 1); + adjust_managed_page_count(page, 1 << page_order); list_del(&page->lru); - put_page(page); /* balloon reference */ + if (page_order) + __free_pages(page, page_order); + else + put_page(page); /* balloon reference */ } } @@ -893,19 +913,21 @@ static int virtballoon_probe(struct virtio_device *vdev) goto out_free_vb; #ifdef CONFIG_BALLOON_COMPACTION - balloon_mnt = kern_mount(&balloon_fs); - if (IS_ERR(balloon_mnt)) { - err = PTR_ERR(balloon_mnt); - goto out_del_vqs; - } + if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_THP_ORDER)) { + balloon_mnt = kern_mount(&balloon_fs); + if (IS_ERR(balloon_mnt)) { + err = PTR_ERR(balloon_mnt); + goto out_del_vqs; + } - vb->vb_dev_info.migratepage = virtballoon_migratepage; - vb->vb_dev_info.inode = alloc_anon_inode(balloon_mnt->mnt_sb); - if (IS_ERR(vb->vb_dev_info.inode)) { - err = PTR_ERR(vb->vb_dev_info.inode); - goto out_kern_unmount; + vb->vb_dev_info.migratepage = virtballoon_migratepage; + vb->vb_dev_info.inode = alloc_anon_inode(balloon_mnt->mnt_sb); + if (IS_ERR(vb->vb_dev_info.inode)) { + err = PTR_ERR(vb->vb_dev_info.inode); + goto out_kern_unmount; + } + vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops; } - vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops; #endif if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { /* @@ -1058,6 +1080,7 @@ static unsigned int features[] = { VIRTIO_BALLOON_F_DEFLATE_ON_OOM, VIRTIO_BALLOON_F_FREE_PAGE_HINT, VIRTIO_BALLOON_F_PAGE_POISON, + VIRTIO_BALLOON_F_THP_ORDER, }; static struct virtio_driver virtio_balloon_driver = { diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h index 338aa27..4c9164e 100644 --- a/include/linux/balloon_compaction.h +++ b/include/linux/balloon_compaction.h @@ -100,8 +100,12 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon, struct page *page) { __SetPageOffline(page); - __SetPageMovable(page, balloon->inode->i_mapping); - set_page_private(page, (unsigned long)balloon); + if (balloon->inode) { + __SetPageMovable(page, balloon->inode->i_mapping); + set_page_private(page, (unsigned long)balloon); + } else { + set_page_private(page, 0); + } list_add(&page->lru, &balloon->pages); } @@ -116,8 +120,10 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon, static inline void balloon_page_delete(struct page *page) { __ClearPageOffline(page); - __ClearPageMovable(page); - set_page_private(page, 0); + if (page_private(page)) { + __ClearPageMovable(page); + set_page_private(page, 0); + } /* * No touch page.lru field once @page has been isolated * because VM is using the field. diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h index a1966cd7..a2998a9 100644 --- a/include/uapi/linux/virtio_balloon.h +++ b/include/uapi/linux/virtio_balloon.h @@ -36,10 +36,14 @@ #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */ #define VIRTIO_BALLOON_F_FREE_PAGE_HINT 3 /* VQ to report free pages */ #define VIRTIO_BALLOON_F_PAGE_POISON 4 /* Guest is using page poisoning */ +#define VIRTIO_BALLOON_F_THP_ORDER 5 /* Balloon page order to thp order */ /* Size of a PFN in the balloon interface. */ #define VIRTIO_BALLOON_PFN_SHIFT 12 +/* The order of the balloon page */ +#define VIRTIO_BALLOON_THP_ORDER 9 + #define VIRTIO_BALLOON_CMD_ID_STOP 0 #define VIRTIO_BALLOON_CMD_ID_DONE 1 struct virtio_balloon_config {
If the guest kernel has many fragmentation pages, use virtio_balloon will split THP of QEMU when it calls MADV_DONTNEED madvise to release the balloon pages. This is an example in a VM with 1G memory 1CPU: cat /proc/meminfo | grep AnonHugePages: AnonHugePages: 0 kB usemem --punch-holes -s -1 800m & cat /proc/meminfo | grep AnonHugePages: AnonHugePages: 976896 kB (qemu) device_add virtio-balloon-pci,id=balloon1 (qemu) info balloon balloon: actual=1024 (qemu) balloon 624 (qemu) info balloon balloon: actual=624 cat /proc/meminfo | grep AnonHugePages: AnonHugePages: 153600 kB THP number decreased more than 800M. The reason is usemem with punch-holes option will free every other page after allocation. Then 400M free memory inside the guest kernel is fragmentation pages. The guest kernel will use them to inflate the balloon. When these fragmentation pages are freed, THP will be split. This commit tries to handle this with add a new flag VIRTIO_BALLOON_F_THP_ORDER. When this flag is set, the balloon page order will be set to the THP order. Then THP pages will be freed together in the host. This is an example in a VM with 1G memory 1CPU: cat /proc/meminfo | grep AnonHugePages: AnonHugePages: 0 kB usemem --punch-holes -s -1 800m & cat /proc/meminfo | grep AnonHugePages: AnonHugePages: 976896 kB (qemu) device_add virtio-balloon-pci,id=balloon1,thp-order=on (qemu) info balloon balloon: actual=1024 (qemu) balloon 624 (qemu) info balloon balloon: actual=624 cat /proc/meminfo | grep AnonHugePages: AnonHugePages: 583680 kB The THP number decreases 384M. This shows that VIRTIO_BALLOON_F_THP_ORDER can help handle the THP split issue. Signed-off-by: Hui Zhu <teawaterz@linux.alibaba.com> --- drivers/virtio/virtio_balloon.c | 57 ++++++++++++++++++++++++++----------- include/linux/balloon_compaction.h | 14 ++++++--- include/uapi/linux/virtio_balloon.h | 4 +++ 3 files changed, 54 insertions(+), 21 deletions(-)