Re: [PATCH v3 04/16] mm/balloon: use general movable page feature into balloon

2016-04-10 Thread Minchan Kim 
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(>_mapcount) == PAGE_MOVABLE_MAPCOUNT_VALUE)
> >-|| PageBalloon(page));
> >+return (test_bit(PG_movable, &(page)->flags) &&
> >+atomic_read(>_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(>_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(>lru, _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

2016-04-05 Thread Vlastimil Babka 

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(>_mapcount) == PAGE_MOVABLE_MAPCOUNT_VALUE)
-   || PageBalloon(page));
+   return (test_bit(PG_movable, &(page)->flags) &&
+   atomic_read(>_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(>_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(>lru, _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

2016-03-30 Thread Minchan Kim 
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(>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, ,
+   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_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(>nb);
if (err < 0)
-   goto out_oom_notify;
+   goto out_del_vqs;
+
+#ifdef CONFIG_BALLOON_COMPACTION
+   balloon_mnt = kern_mount(_fs);
+   if (IS_ERR(balloon_mnt)) {
+   err = PTR_ERR(balloon_mnt);
+   unregister_oom_notifier(>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(>nb);
+   vb->vb_dev_info.inode = NULL;
+   goto out_del_vqs;
+   }
+   vb->vb_dev_info.inode->i_mapping->a_ops = _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(>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,