Re: [PATCH v3 04/16] mm/balloon: use general movable page feature into balloon
On Tue, Apr 05, 2016 at 02:03:05PM +0200, Vlastimil Babka wrote: > On 03/30/2016 09:12 AM, Minchan Kim wrote: > >Now, VM has a feature to migrate non-lru movable pages so > >balloon doesn't need custom migration hooks in migrate.c > >and compact.c. Instead, this patch implements page->mapping > >->{isolate|migrate|putback} functions. > > > >With that, we could remove hooks for ballooning in general > >migration functions and make balloon compaction simple. > > > >Cc: virtualization@lists.linux-foundation.org > >Cc: Rafael Aquini > >Cc: Konstantin Khlebnikov > >Signed-off-by: Gioh Kim > >Signed-off-by: Minchan Kim > > I'm not familiar with the inode and pseudofs stuff, so just some > things I noticed: > > >-#define PAGE_MOVABLE_MAPCOUNT_VALUE (-255) > >+#define PAGE_MOVABLE_MAPCOUNT_VALUE (-256) > >+#define PAGE_BALLOON_MAPCOUNT_VALUE PAGE_MOVABLE_MAPCOUNT_VALUE > > > > static inline int PageMovable(struct page *page) > > { > >-return ((test_bit(PG_movable, &(page)->flags) && > >-atomic_read(&page->_mapcount) == PAGE_MOVABLE_MAPCOUNT_VALUE) > >-|| PageBalloon(page)); > >+return (test_bit(PG_movable, &(page)->flags) && > >+atomic_read(&page->_mapcount) == PAGE_MOVABLE_MAPCOUNT_VALUE); > > } > > > > /* Caller should hold a PG_lock */ > >@@ -645,6 +626,35 @@ static inline void __ClearPageMovable(struct page *page) > > > > PAGEFLAG(Isolated, isolated, PF_ANY); > > > >+static inline int PageBalloon(struct page *page) > >+{ > >+return atomic_read(&page->_mapcount) == PAGE_BALLOON_MAPCOUNT_VALUE > >+&& PagePrivate2(page); > >+} > > Hmm so you are now using PG_private_2 flag here, but it's not > documented. Also the only caller of PageBalloon() seems to be > stable_page_flags(). Which will now report all movable pages with > PG_private_2 as KPF_BALOON. Seems like an overkill and also not > reliable. Could it test e.g. page->mapping instead? Thanks for pointing out. I will not use page->_mapcount in next version so it should be okay. > > Or maybe if we manage to get rid of PAGE_MOVABLE_MAPCOUNT_VALUE, we > can keep PAGE_BALLOON_MAPCOUNT_VALUE to simply distinguish balloon > pages for stable_page_flags(). Yeb. > > >@@ -1033,7 +1019,7 @@ static int __unmap_and_move(struct page *page, struct > >page *newpage, > > out: > > /* If migration is successful, move newpage to right list */ > > if (rc == MIGRATEPAGE_SUCCESS) { > >-if (unlikely(__is_movable_balloon_page(newpage))) > >+if (unlikely(PageMovable(newpage))) > > put_page(newpage); > > else > > putback_lru_page(newpage); > > Hmm shouldn't the condition have been changed to > > if (unlikely(__is_movable_balloon_page(newpage)) || PageMovable(newpage) > > by patch 02/16? And this patch should be just removing the > balloon-specific check? Otherwise it seems like between patches 02 > and 04, other kinds of PageMovable pages were unnecessarily/wrongly > routed through putback_lru_page()? Fixed. > > >diff --git a/mm/vmscan.c b/mm/vmscan.c > >index d82196244340..c7696a2e11c7 100644 > >--- a/mm/vmscan.c > >+++ b/mm/vmscan.c > >@@ -1254,7 +1254,7 @@ unsigned long reclaim_clean_pages_from_list(struct > >zone *zone, > > > > list_for_each_entry_safe(page, next, page_list, lru) { > > if (page_is_file_cache(page) && !PageDirty(page) && > >-!isolated_balloon_page(page)) { > >+!PageIsolated(page)) { > > ClearPageActive(page); > > list_move(&page->lru, &clean_pages); > > } > > This looks like the same comment as above at first glance. But > looking closer, it's even weirder. isolated_balloon_page() was > simply PageBalloon() after d6d86c0a7f8dd... weird already. You > replace it with check for !PageIsolated() which looks like a more > correct check, so ok. Except the potential false positive with > PG_owner_priv_1. I will change it next version so it shouldn't be a problem. Thanks for the review! ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 04/16] mm/balloon: use general movable page feature into balloon
On 03/30/2016 09:12 AM, Minchan Kim wrote: Now, VM has a feature to migrate non-lru movable pages so balloon doesn't need custom migration hooks in migrate.c and compact.c. Instead, this patch implements page->mapping ->{isolate|migrate|putback} functions. With that, we could remove hooks for ballooning in general migration functions and make balloon compaction simple. Cc: virtualization@lists.linux-foundation.org Cc: Rafael Aquini Cc: Konstantin Khlebnikov Signed-off-by: Gioh Kim Signed-off-by: Minchan Kim I'm not familiar with the inode and pseudofs stuff, so just some things I noticed: -#define PAGE_MOVABLE_MAPCOUNT_VALUE (-255) +#define PAGE_MOVABLE_MAPCOUNT_VALUE (-256) +#define PAGE_BALLOON_MAPCOUNT_VALUE PAGE_MOVABLE_MAPCOUNT_VALUE static inline int PageMovable(struct page *page) { - return ((test_bit(PG_movable, &(page)->flags) && - atomic_read(&page->_mapcount) == PAGE_MOVABLE_MAPCOUNT_VALUE) - || PageBalloon(page)); + return (test_bit(PG_movable, &(page)->flags) && + atomic_read(&page->_mapcount) == PAGE_MOVABLE_MAPCOUNT_VALUE); } /* Caller should hold a PG_lock */ @@ -645,6 +626,35 @@ static inline void __ClearPageMovable(struct page *page) PAGEFLAG(Isolated, isolated, PF_ANY); +static inline int PageBalloon(struct page *page) +{ + return atomic_read(&page->_mapcount) == PAGE_BALLOON_MAPCOUNT_VALUE + && PagePrivate2(page); +} Hmm so you are now using PG_private_2 flag here, but it's not documented. Also the only caller of PageBalloon() seems to be stable_page_flags(). Which will now report all movable pages with PG_private_2 as KPF_BALOON. Seems like an overkill and also not reliable. Could it test e.g. page->mapping instead? Or maybe if we manage to get rid of PAGE_MOVABLE_MAPCOUNT_VALUE, we can keep PAGE_BALLOON_MAPCOUNT_VALUE to simply distinguish balloon pages for stable_page_flags(). @@ -1033,7 +1019,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage, out: /* If migration is successful, move newpage to right list */ if (rc == MIGRATEPAGE_SUCCESS) { - if (unlikely(__is_movable_balloon_page(newpage))) + if (unlikely(PageMovable(newpage))) put_page(newpage); else putback_lru_page(newpage); Hmm shouldn't the condition have been changed to if (unlikely(__is_movable_balloon_page(newpage)) || PageMovable(newpage) by patch 02/16? And this patch should be just removing the balloon-specific check? Otherwise it seems like between patches 02 and 04, other kinds of PageMovable pages were unnecessarily/wrongly routed through putback_lru_page()? diff --git a/mm/vmscan.c b/mm/vmscan.c index d82196244340..c7696a2e11c7 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1254,7 +1254,7 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone, list_for_each_entry_safe(page, next, page_list, lru) { if (page_is_file_cache(page) && !PageDirty(page) && - !isolated_balloon_page(page)) { + !PageIsolated(page)) { ClearPageActive(page); list_move(&page->lru, &clean_pages); } This looks like the same comment as above at first glance. But looking closer, it's even weirder. isolated_balloon_page() was simply PageBalloon() after d6d86c0a7f8dd... weird already. You replace it with check for !PageIsolated() which looks like a more correct check, so ok. Except the potential false positive with PG_owner_priv_1. Thanks. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 04/16] mm/balloon: use general movable page feature into balloon
Now, VM has a feature to migrate non-lru movable pages so balloon doesn't need custom migration hooks in migrate.c and compact.c. Instead, this patch implements page->mapping ->{isolate|migrate|putback} functions. With that, we could remove hooks for ballooning in general migration functions and make balloon compaction simple. Cc: virtualization@lists.linux-foundation.org Cc: Rafael Aquini Cc: Konstantin Khlebnikov Signed-off-by: Gioh Kim Signed-off-by: Minchan Kim --- drivers/virtio/virtio_balloon.c| 53 --- include/linux/balloon_compaction.h | 49 -- include/linux/page-flags.h | 56 +++- include/uapi/linux/magic.h | 1 + mm/balloon_compaction.c| 101 - mm/compaction.c| 7 --- mm/migrate.c | 22 ++-- mm/vmscan.c| 2 +- 8 files changed, 119 insertions(+), 172 deletions(-) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 7b6d74f0c72f..0c16192d2684 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -30,6 +30,7 @@ #include #include #include +#include /* * Balloon device works in 4K page units. So each page is pointed to by @@ -45,6 +46,10 @@ static int oom_pages = OOM_VBALLOON_DEFAULT_PAGES; module_param(oom_pages, int, S_IRUSR | S_IWUSR); MODULE_PARM_DESC(oom_pages, "pages to free on OOM"); +#ifdef CONFIG_BALLOON_COMPACTION +static struct vfsmount *balloon_mnt; +#endif + struct virtio_balloon { struct virtio_device *vdev; struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; @@ -482,10 +487,29 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, mutex_unlock(&vb->balloon_lock); + ClearPageIsolated(page); put_page(page); /* balloon reference */ return MIGRATEPAGE_SUCCESS; } + +static struct dentry *balloon_mount(struct file_system_type *fs_type, + int flags, const char *dev_name, void *data) +{ + static const struct dentry_operations ops = { + .d_dname = simple_dname, + }; + + return mount_pseudo(fs_type, "balloon-kvm:", NULL, &ops, + BALLOON_KVM_MAGIC); +} + +static struct file_system_type balloon_fs = { + .name = "balloon-kvm", + .mount = balloon_mount, + .kill_sb= kill_anon_super, +}; + #endif /* CONFIG_BALLOON_COMPACTION */ static int virtballoon_probe(struct virtio_device *vdev) @@ -515,10 +539,6 @@ static int virtballoon_probe(struct virtio_device *vdev) vb->vdev = vdev; balloon_devinfo_init(&vb->vb_dev_info); -#ifdef CONFIG_BALLOON_COMPACTION - vb->vb_dev_info.migratepage = virtballoon_migratepage; -#endif - err = init_vqs(vb); if (err) goto out_free_vb; @@ -527,13 +547,32 @@ static int virtballoon_probe(struct virtio_device *vdev) vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY; err = register_oom_notifier(&vb->nb); if (err < 0) - goto out_oom_notify; + goto out_del_vqs; + +#ifdef CONFIG_BALLOON_COMPACTION + balloon_mnt = kern_mount(&balloon_fs); + if (IS_ERR(balloon_mnt)) { + err = PTR_ERR(balloon_mnt); + unregister_oom_notifier(&vb->nb); + 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); + kern_unmount(balloon_mnt); + unregister_oom_notifier(&vb->nb); + vb->vb_dev_info.inode = NULL; + goto out_del_vqs; + } + vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops; +#endif virtio_device_ready(vdev); return 0; -out_oom_notify: +out_del_vqs: vdev->config->del_vqs(vdev); out_free_vb: kfree(vb); @@ -567,6 +606,8 @@ static void virtballoon_remove(struct virtio_device *vdev) cancel_work_sync(&vb->update_balloon_stats_work); remove_common(vb); + if (vb->vb_dev_info.inode) + iput(vb->vb_dev_info.inode); kfree(vb); } diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h index 9b0a15d06a4f..4c693bf3abdf 100644 --- a/include/linux/balloon_compaction.h +++ b/include/linux/balloon_compaction.h @@ -48,6 +48,7 @@ #include #include #include +#include /* * Balloon device information descriptor. @@ -62,6 +63,7 @@ struct balloon_dev_info { struct list_head pages; /* Pages enqueued & handled to Host */ int (*migratepage)(struct balloon_dev_info *, struct page *newpage, struct page *page, enum migrate_mode mode); + st