Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1702714 > unrolled thread
| Started by | Wei Wang <wei.w.wang@intel.com> |
|---|---|
| First post | 2017-08-03 08:50 +0200 |
| Last post | 2017-08-10 10:00 +0200 |
| Articles | 20 on this page of 26 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH v13 0/5] Virtio-balloon Enhancement Wei Wang <wei.w.wang@intel.com> - 2017-08-03 08:50 +0200
[PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG Wei Wang <wei.w.wang@intel.com> - 2017-08-03 09:00 +0200
Re: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG "Michael S. Tsirkin" <mst@redhat.com> - 2017-08-03 16:30 +0200
RE: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG "Wang, Wei W" <wei.w.wang@intel.com> - 2017-08-03 17:20 +0200
Re: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG "Michael S. Tsirkin" <mst@redhat.com> - 2017-08-03 18:00 +0200
[PATCH v13 4/5] mm: support reporting free page blocks Wei Wang <wei.w.wang@intel.com> - 2017-08-03 09:00 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Michal Hocko <mhocko@kernel.org> - 2017-08-03 11:20 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Wei Wang <wei.w.wang@intel.com> - 2017-08-03 12:40 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Michal Hocko <mhocko@kernel.org> - 2017-08-03 12:50 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Wei Wang <wei.w.wang@intel.com> - 2017-08-03 13:30 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Michal Hocko <mhocko@kernel.org> - 2017-08-03 13:30 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Wei Wang <wei.w.wang@intel.com> - 2017-08-03 14:10 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Michal Hocko <mhocko@kernel.org> - 2017-08-03 14:50 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Wei Wang <wei.w.wang@intel.com> - 2017-08-03 15:20 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Michal Hocko <mhocko@kernel.org> - 2017-08-03 16:00 +0200
RE: [PATCH v13 4/5] mm: support reporting free page blocks "Wang, Wei W" <wei.w.wang@intel.com> - 2017-08-03 17:30 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks "Michael S. Tsirkin" <mst@redhat.com> - 2017-08-03 23:10 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Michal Hocko <mhocko@kernel.org> - 2017-08-04 10:00 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Wei Wang <wei.w.wang@intel.com> - 2017-08-04 10:20 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Michal Hocko <mhocko@kernel.org> - 2017-08-04 10:30 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Wei Wang <wei.w.wang@intel.com> - 2017-08-04 11:00 +0200
Re: [PATCH v13 4/5] mm: support reporting free page blocks Wei Wang <wei.w.wang@intel.com> - 2017-08-08 08:20 +0200
Re: [virtio-dev] Re: [PATCH v13 4/5] mm: support reporting free page blocks Wei Wang <wei.w.wang@intel.com> - 2017-08-08 08:40 +0200
Re: [virtio-dev] Re: [PATCH v13 4/5] mm: support reporting free page blocks Michal Hocko <mhocko@kernel.org> - 2017-08-10 09:10 +0200
Re: [virtio-dev] Re: [PATCH v13 4/5] mm: support reporting free page blocks Wei Wang <wei.w.wang@intel.com> - 2017-08-10 09:40 +0200
Re: [virtio-dev] Re: [PATCH v13 4/5] mm: support reporting free page blocks Michal Hocko <mhocko@kernel.org> - 2017-08-10 10:00 +0200
Page 1 of 2 [1] 2 Next page →
| From | Wei Wang <wei.w.wang@intel.com> |
|---|---|
| Date | 2017-08-03 08:50 +0200 |
| Subject | [PATCH v13 0/5] Virtio-balloon Enhancement |
| Message-ID | <uai7g-4AX-9@gated-at.bofh.it> |
This patch series enhances the existing virtio-balloon with the following new features: 1) fast ballooning: transfer ballooned pages between the guest and host in chunks using sgs, instead of one by one; and 2) free_page_vq: a new virtqueue to report guest free pages to the host. The second feature can be used to accelerate live migration of VMs. Here are some details: Live migration needs to transfer the VM's memory from the source machine to the destination round by round. For the 1st round, all the VM's memory is transferred. From the 2nd round, only the pieces of memory that were written by the guest (after the 1st round) are transferred. One method that is popularly used by the hypervisor to track which part of memory is written is to write-protect all the guest memory. The second feature enables the optimization of the 1st round memory transfer - the hypervisor can skip the transfer of guest free pages in the 1st round. It is not concerned that the memory pages are used after they are given to the hypervisor as a hint of the free pages, because they will be tracked by the hypervisor and transferred in the next round if they are used and written. Change Log: v12->v13: 1) mm: use a callback function to handle the the free page blocks from the report function. This avoids exposing the zone internal to a kernel module. 2) virtio-balloon: send balloon pages or a free page block using a single sg each time. This has the benefits of simpler implementation with no new APIs. 3) virtio-balloon: the free_page_vq is used to report free pages only (no multiple usages interleaving) 4) virtio-balloon: Balloon pages and free page blocks are sent via input sgs, and the completion signal to the host is sent via an output sg. v11->v12: 1) xbitmap: use the xbitmap from Matthew Wilcox to record ballooned pages. 2) virtio-ring: enable the driver to build up a desc chain using vring desc. 3) virtio-ring: Add locking to the existing START_USE() and END_USE() macro to lock/unlock the vq when a vq operation starts/ends. 4) virtio-ring: add virtqueue_kick_sync() and virtqueue_kick_async() 5) virtio-balloon: describe chunks of ballooned pages and free pages blocks directly using one or more chains of desc from the vq. v10->v11: 1) virtio_balloon: use vring_desc to describe a chunk; 2) virtio_ring: support to add an indirect desc table to virtqueue; 3) virtio_balloon: use cmdq to report guest memory statistics. v9->v10: 1) mm: put report_unused_page_block() under CONFIG_VIRTIO_BALLOON; 2) virtio-balloon: add virtballoon_validate(); 3) virtio-balloon: msg format change; 4) virtio-balloon: move miscq handling to a task on system_freezable_wq; 5) virtio-balloon: code cleanup. v8->v9: 1) Split the two new features, VIRTIO_BALLOON_F_BALLOON_CHUNKS and VIRTIO_BALLOON_F_MISC_VQ, which were mixed together in the previous implementation; 2) Simpler function to get the free page block. v7->v8: 1) Use only one chunk format, instead of two. 2) re-write the virtio-balloon implementation patch. 3) commit changes 4) patch re-org Matthew Wilcox (1): Introduce xbitmap Wei Wang (4): xbitmap: add xb_find_next_bit() and xb_zero() virtio-balloon: VIRTIO_BALLOON_F_SG mm: support reporting free page blocks virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ drivers/virtio/virtio_balloon.c | 302 +++++++++++++++++++++++++++++++----- include/linux/mm.h | 7 + include/linux/mmzone.h | 5 + include/linux/radix-tree.h | 2 + include/linux/xbitmap.h | 53 +++++++ include/uapi/linux/virtio_balloon.h | 2 + lib/radix-tree.c | 167 +++++++++++++++++++- mm/page_alloc.c | 109 +++++++++++++ 8 files changed, 609 insertions(+), 38 deletions(-) create mode 100644 include/linux/xbitmap.h -- 2.7.4
[toc] | [next] | [standalone]
| From | Wei Wang <wei.w.wang@intel.com> |
|---|---|
| Date | 2017-08-03 09:00 +0200 |
| Subject | [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG |
| Message-ID | <uaigV-4Gy-7@gated-at.bofh.it> |
| In reply to | #1702714 |
Add a new feature, VIRTIO_BALLOON_F_SG, which enables the transfer
of balloon (i.e. inflated/deflated) pages using scatter-gather lists
to the host.
The implementation of the previous virtio-balloon is not very
efficient, because the balloon pages are transferred to the
host one by one. Here is the breakdown of the time in percentage
spent on each step of the balloon inflating process (inflating
7GB of an 8GB idle guest).
1) allocating pages (6.5%)
2) sending PFNs to host (68.3%)
3) address translation (6.1%)
4) madvise (19%)
It takes about 4126ms for the inflating process to complete.
The above profiling shows that the bottlenecks are stage 2)
and stage 4).
This patch optimizes step 2) by transferring pages to the host in
sgs. An sg describes a chunk of guest physically continuous pages.
With this mechanism, step 4) can also be optimized by doing address
translation and madvise() in chunks rather than page by page.
With this new feature, the above ballooning process takes ~541ms
resulting in an improvement of ~87%.
TODO: optimize stage 1) by allocating/freeing a chunk of pages
instead of a single page each time.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_balloon.c | 150 ++++++++++++++++++++++++++++++++----
include/uapi/linux/virtio_balloon.h | 1 +
2 files changed, 134 insertions(+), 17 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f0b3a0b..29aca0c 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -32,6 +32,7 @@
#include <linux/mm.h>
#include <linux/mount.h>
#include <linux/magic.h>
+#include <linux/xbitmap.h>
/*
* Balloon device works in 4K page units. So each page is pointed to by
@@ -79,6 +80,9 @@ struct virtio_balloon {
/* Synchronize access/update to this struct virtio_balloon elements */
struct mutex balloon_lock;
+ /* The xbitmap used to record ballooned pages */
+ struct xb page_xb;
+
/* The array of pfns we tell the Host about. */
unsigned int num_pfns;
__virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
@@ -141,13 +145,90 @@ static void set_page_pfns(struct virtio_balloon *vb,
page_to_balloon_pfn(page) + i);
}
+static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
+ void *addr, uint32_t size)
+{
+ struct scatterlist sg;
+ unsigned int len;
+
+ sg_init_one(&sg, addr, size);
+ while (unlikely(virtqueue_add_inbuf(vq, &sg, 1, vb, GFP_KERNEL)
+ == -ENOSPC)) {
+ /*
+ * It is uncommon to see the vq is full, because the sg is sent
+ * one by one and the device is able to handle it in time. But
+ * if that happens, we kick and wait for an entry is released.
+ */
+ virtqueue_kick(vq);
+ while (!virtqueue_get_buf(vq, &len) &&
+ !virtqueue_is_broken(vq))
+ cpu_relax();
+ }
+ virtqueue_kick(vq);
+ wait_event(vb->acked, virtqueue_get_buf(vq, &len));
+}
+
+/*
+ * Send balloon pages in sgs to host. The balloon pages are recorded in the
+ * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
+ * The page xbitmap is searched for continuous "1" bits, which correspond
+ * to continuous pages, to chunk into sgs.
+ *
+ * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
+ * need to be searched.
+ */
+static void tell_host_sgs(struct virtio_balloon *vb,
+ struct virtqueue *vq,
+ unsigned long page_xb_start,
+ unsigned long page_xb_end)
+{
+ unsigned long sg_pfn_start, sg_pfn_end;
+ void *sg_addr;
+ uint32_t sg_len, sg_max_len = round_down(UINT_MAX, PAGE_SIZE);
+
+ sg_pfn_start = page_xb_start;
+ while (sg_pfn_start < page_xb_end) {
+ sg_pfn_start = xb_find_next_bit(&vb->page_xb, sg_pfn_start,
+ page_xb_end, 1);
+ if (sg_pfn_start == page_xb_end + 1)
+ break;
+ sg_pfn_end = xb_find_next_bit(&vb->page_xb, sg_pfn_start + 1,
+ page_xb_end, 0);
+ sg_addr = pfn_to_kaddr(sg_pfn_start);
+ sg_len = (sg_pfn_end - sg_pfn_start) << PAGE_SHIFT;
+ while (sg_len > sg_max_len) {
+ send_one_sg(vb, vq, sg_addr, sg_max_len);
+ sg_addr += sg_max_len;
+ sg_len -= sg_max_len;
+ }
+ send_one_sg(vb, vq, sg_addr, sg_len);
+ xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
+ sg_pfn_start = sg_pfn_end + 1;
+ }
+}
+
+static inline void xb_set_page(struct virtio_balloon *vb,
+ struct page *page,
+ unsigned long *pfn_min,
+ unsigned long *pfn_max)
+{
+ unsigned long pfn = page_to_pfn(page);
+
+ *pfn_min = min(pfn, *pfn_min);
+ *pfn_max = max(pfn, *pfn_max);
+ xb_set_bit(&vb->page_xb, pfn);
+}
+
static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
{
struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
unsigned num_allocated_pages;
+ bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
+ unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
/* We can only do one array worth at a time. */
- num = min(num, ARRAY_SIZE(vb->pfns));
+ if (!use_sg)
+ num = min(num, ARRAY_SIZE(vb->pfns));
mutex_lock(&vb->balloon_lock);
for (vb->num_pfns = 0; vb->num_pfns < num;
@@ -162,7 +243,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
msleep(200);
break;
}
- set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
+
+ if (use_sg)
+ xb_set_page(vb, page, &pfn_min, &pfn_max);
+ else
+ set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
+
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
if (!virtio_has_feature(vb->vdev,
VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
@@ -171,8 +257,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
num_allocated_pages = vb->num_pfns;
/* Did we get any? */
- if (vb->num_pfns != 0)
- tell_host(vb, vb->inflate_vq);
+ if (vb->num_pfns) {
+ if (use_sg)
+ tell_host_sgs(vb, vb->inflate_vq, pfn_min, pfn_max);
+ else
+ tell_host(vb, vb->inflate_vq);
+ }
mutex_unlock(&vb->balloon_lock);
return num_allocated_pages;
@@ -198,9 +288,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
struct page *page;
struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
LIST_HEAD(pages);
+ bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
+ unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
- /* We can only do one array worth at a time. */
- num = min(num, ARRAY_SIZE(vb->pfns));
+ /* Traditionally, we can only do one array worth at a time. */
+ if (!use_sg)
+ num = min(num, ARRAY_SIZE(vb->pfns));
mutex_lock(&vb->balloon_lock);
/* We can't release more pages than taken */
@@ -210,7 +303,11 @@ 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);
+ if (use_sg)
+ xb_set_page(vb, page, &pfn_min, &pfn_max);
+ else
+ set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
+
list_add(&page->lru, &pages);
vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
}
@@ -221,8 +318,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
* virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
* is true, we *have* to do it in this order
*/
- if (vb->num_pfns != 0)
- tell_host(vb, vb->deflate_vq);
+ if (vb->num_pfns) {
+ if (use_sg)
+ tell_host_sgs(vb, vb->deflate_vq, pfn_min, pfn_max);
+ else
+ tell_host(vb, vb->deflate_vq);
+ }
release_pages_balloon(vb, &pages);
mutex_unlock(&vb->balloon_lock);
return num_freed_pages;
@@ -441,6 +542,7 @@ static int init_vqs(struct virtio_balloon *vb)
}
#ifdef CONFIG_BALLOON_COMPACTION
+
/*
* virtballoon_migratepage - perform the balloon page migration on behalf of
* a compation thread. (called under page lock)
@@ -464,6 +566,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
{
struct virtio_balloon *vb = container_of(vb_dev_info,
struct virtio_balloon, vb_dev_info);
+ bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
unsigned long flags;
/*
@@ -485,16 +588,24 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
vb_dev_info->isolated_pages--;
__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);
- tell_host(vb, vb->inflate_vq);
-
+ if (use_sg) {
+ send_one_sg(vb, vb->inflate_vq, page_address(newpage),
+ PAGE_SIZE);
+ } else {
+ vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+ set_page_pfns(vb, vb->pfns, newpage);
+ tell_host(vb, vb->inflate_vq);
+ }
/* balloon's page migration 2nd step -- deflate "page" */
balloon_page_delete(page);
- vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
- set_page_pfns(vb, vb->pfns, page);
- tell_host(vb, vb->deflate_vq);
-
+ if (use_sg) {
+ send_one_sg(vb, vb->deflate_vq, page_address(page),
+ PAGE_SIZE);
+ } else {
+ vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+ set_page_pfns(vb, vb->pfns, page);
+ tell_host(vb, vb->deflate_vq);
+ }
mutex_unlock(&vb->balloon_lock);
put_page(page); /* balloon reference */
@@ -553,6 +664,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
if (err)
goto out_free_vb;
+ if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
+ xb_init(&vb->page_xb);
+
vb->nb.notifier_call = virtballoon_oom_notify;
vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
err = register_oom_notifier(&vb->nb);
@@ -618,6 +732,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
cancel_work_sync(&vb->update_balloon_size_work);
cancel_work_sync(&vb->update_balloon_stats_work);
+ xb_empty(&vb->page_xb);
remove_common(vb);
#ifdef CONFIG_BALLOON_COMPACTION
if (vb->vb_dev_info.inode)
@@ -669,6 +784,7 @@ static unsigned int features[] = {
VIRTIO_BALLOON_F_MUST_TELL_HOST,
VIRTIO_BALLOON_F_STATS_VQ,
VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
+ VIRTIO_BALLOON_F_SG,
};
static struct virtio_driver virtio_balloon_driver = {
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 343d7dd..37780a7 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -34,6 +34,7 @@
#define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
#define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
#define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
+#define VIRTIO_BALLOON_F_SG 3 /* Use sg instead of PFN lists */
/* Size of a PFN in the balloon interface. */
#define VIRTIO_BALLOON_PFN_SHIFT 12
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | "Michael S. Tsirkin" <mst@redhat.com> |
|---|---|
| Date | 2017-08-03 16:30 +0200 |
| Subject | Re: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG |
| Message-ID | <uapip-1cj-3@gated-at.bofh.it> |
| In reply to | #1702716 |
On Thu, Aug 03, 2017 at 02:38:17PM +0800, Wei Wang wrote:
> Add a new feature, VIRTIO_BALLOON_F_SG, which enables the transfer
> of balloon (i.e. inflated/deflated) pages using scatter-gather lists
> to the host.
>
> The implementation of the previous virtio-balloon is not very
> efficient, because the balloon pages are transferred to the
> host one by one. Here is the breakdown of the time in percentage
> spent on each step of the balloon inflating process (inflating
> 7GB of an 8GB idle guest).
>
> 1) allocating pages (6.5%)
> 2) sending PFNs to host (68.3%)
> 3) address translation (6.1%)
> 4) madvise (19%)
>
> It takes about 4126ms for the inflating process to complete.
> The above profiling shows that the bottlenecks are stage 2)
> and stage 4).
>
> This patch optimizes step 2) by transferring pages to the host in
> sgs. An sg describes a chunk of guest physically continuous pages.
> With this mechanism, step 4) can also be optimized by doing address
> translation and madvise() in chunks rather than page by page.
>
> With this new feature, the above ballooning process takes ~541ms
> resulting in an improvement of ~87%.
>
> TODO: optimize stage 1) by allocating/freeing a chunk of pages
> instead of a single page each time.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 150 ++++++++++++++++++++++++++++++++----
> include/uapi/linux/virtio_balloon.h | 1 +
> 2 files changed, 134 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f0b3a0b..29aca0c 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -32,6 +32,7 @@
> #include <linux/mm.h>
> #include <linux/mount.h>
> #include <linux/magic.h>
> +#include <linux/xbitmap.h>
>
> /*
> * Balloon device works in 4K page units. So each page is pointed to by
> @@ -79,6 +80,9 @@ struct virtio_balloon {
> /* Synchronize access/update to this struct virtio_balloon elements */
> struct mutex balloon_lock;
>
> + /* The xbitmap used to record ballooned pages */
> + struct xb page_xb;
> +
> /* The array of pfns we tell the Host about. */
> unsigned int num_pfns;
> __virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
> @@ -141,13 +145,90 @@ static void set_page_pfns(struct virtio_balloon *vb,
> page_to_balloon_pfn(page) + i);
> }
>
> +static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> + void *addr, uint32_t size)
> +{
> + struct scatterlist sg;
> + unsigned int len;
> +
> + sg_init_one(&sg, addr, size);
> + while (unlikely(virtqueue_add_inbuf(vq, &sg, 1, vb, GFP_KERNEL)
> + == -ENOSPC)) {
> + /*
> + * It is uncommon to see the vq is full, because the sg is sent
> + * one by one and the device is able to handle it in time. But
> + * if that happens, we kick and wait for an entry is released.
is released -> to get used.
> + */
> + virtqueue_kick(vq);
> + while (!virtqueue_get_buf(vq, &len) &&
> + !virtqueue_is_broken(vq))
> + cpu_relax();
Please rework to use wait_event in that case too.
> + }
> + virtqueue_kick(vq);
> + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> +}
> +
> +/*
> + * Send balloon pages in sgs to host. The balloon pages are recorded in the
> + * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
> + * The page xbitmap is searched for continuous "1" bits, which correspond
> + * to continuous pages, to chunk into sgs.
> + *
> + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
> + * need to be searched.
> + */
> +static void tell_host_sgs(struct virtio_balloon *vb,
> + struct virtqueue *vq,
> + unsigned long page_xb_start,
> + unsigned long page_xb_end)
> +{
> + unsigned long sg_pfn_start, sg_pfn_end;
> + void *sg_addr;
> + uint32_t sg_len, sg_max_len = round_down(UINT_MAX, PAGE_SIZE);
> +
> + sg_pfn_start = page_xb_start;
> + while (sg_pfn_start < page_xb_end) {
> + sg_pfn_start = xb_find_next_bit(&vb->page_xb, sg_pfn_start,
> + page_xb_end, 1);
> + if (sg_pfn_start == page_xb_end + 1)
> + break;
> + sg_pfn_end = xb_find_next_bit(&vb->page_xb, sg_pfn_start + 1,
> + page_xb_end, 0);
> + sg_addr = pfn_to_kaddr(sg_pfn_start);
> + sg_len = (sg_pfn_end - sg_pfn_start) << PAGE_SHIFT;
> + while (sg_len > sg_max_len) {
> + send_one_sg(vb, vq, sg_addr, sg_max_len);
> + sg_addr += sg_max_len;
> + sg_len -= sg_max_len;
> + }
> + send_one_sg(vb, vq, sg_addr, sg_len);
> + xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
> + sg_pfn_start = sg_pfn_end + 1;
> + }
> +}
> +
> +static inline void xb_set_page(struct virtio_balloon *vb,
> + struct page *page,
> + unsigned long *pfn_min,
> + unsigned long *pfn_max)
> +{
> + unsigned long pfn = page_to_pfn(page);
> +
> + *pfn_min = min(pfn, *pfn_min);
> + *pfn_max = max(pfn, *pfn_max);
> + xb_set_bit(&vb->page_xb, pfn);
> +}
> +
> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> unsigned num_allocated_pages;
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + if (!use_sg)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> @@ -162,7 +243,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> msleep(200);
> break;
> }
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> + if (use_sg)
> + xb_set_page(vb, page, &pfn_min, &pfn_max);
> + else
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> if (!virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> @@ -171,8 +257,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
>
> num_allocated_pages = vb->num_pfns;
> /* Did we get any? */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->inflate_vq);
> + if (vb->num_pfns) {
> + if (use_sg)
> + tell_host_sgs(vb, vb->inflate_vq, pfn_min, pfn_max);
> + else
> + tell_host(vb, vb->inflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> return num_allocated_pages;
> @@ -198,9 +288,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> struct page *page;
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> LIST_HEAD(pages);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> - /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + /* Traditionally, we can only do one array worth at a time. */
> + if (!use_sg)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> /* We can't release more pages than taken */
> @@ -210,7 +303,11 @@ 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);
> + if (use_sg)
> + xb_set_page(vb, page, &pfn_min, &pfn_max);
> + else
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> list_add(&page->lru, &pages);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> }
> @@ -221,8 +318,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> * is true, we *have* to do it in this order
> */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->deflate_vq);
> + if (vb->num_pfns) {
> + if (use_sg)
> + tell_host_sgs(vb, vb->deflate_vq, pfn_min, pfn_max);
> + else
> + tell_host(vb, vb->deflate_vq);
> + }
> release_pages_balloon(vb, &pages);
> mutex_unlock(&vb->balloon_lock);
> return num_freed_pages;
> @@ -441,6 +542,7 @@ static int init_vqs(struct virtio_balloon *vb)
> }
>
> #ifdef CONFIG_BALLOON_COMPACTION
> +
> /*
> * virtballoon_migratepage - perform the balloon page migration on behalf of
> * a compation thread. (called under page lock)
> @@ -464,6 +566,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> {
> struct virtio_balloon *vb = container_of(vb_dev_info,
> struct virtio_balloon, vb_dev_info);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> unsigned long flags;
>
> /*
> @@ -485,16 +588,24 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> vb_dev_info->isolated_pages--;
> __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);
> - tell_host(vb, vb->inflate_vq);
> -
> + if (use_sg) {
> + send_one_sg(vb, vb->inflate_vq, page_address(newpage),
> + PAGE_SIZE);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, newpage);
> + tell_host(vb, vb->inflate_vq);
> + }
> /* balloon's page migration 2nd step -- deflate "page" */
> balloon_page_delete(page);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, page);
> - tell_host(vb, vb->deflate_vq);
> -
> + if (use_sg) {
> + send_one_sg(vb, vb->deflate_vq, page_address(page),
> + PAGE_SIZE);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, page);
> + tell_host(vb, vb->deflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> put_page(page); /* balloon reference */
> @@ -553,6 +664,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (err)
> goto out_free_vb;
>
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
> + xb_init(&vb->page_xb);
> +
> vb->nb.notifier_call = virtballoon_oom_notify;
> vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
> err = register_oom_notifier(&vb->nb);
> @@ -618,6 +732,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
> cancel_work_sync(&vb->update_balloon_size_work);
> cancel_work_sync(&vb->update_balloon_stats_work);
>
> + xb_empty(&vb->page_xb);
> remove_common(vb);
> #ifdef CONFIG_BALLOON_COMPACTION
> if (vb->vb_dev_info.inode)
> @@ -669,6 +784,7 @@ static unsigned int features[] = {
> VIRTIO_BALLOON_F_MUST_TELL_HOST,
> VIRTIO_BALLOON_F_STATS_VQ,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> + VIRTIO_BALLOON_F_SG,
> };
>
> static struct virtio_driver virtio_balloon_driver = {
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..37780a7 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -34,6 +34,7 @@
> #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_SG 3 /* Use sg instead of PFN lists */
>
> /* Size of a PFN in the balloon interface. */
> #define VIRTIO_BALLOON_PFN_SHIFT 12
> --
> 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | "Wang, Wei W" <wei.w.wang@intel.com> |
|---|---|
| Date | 2017-08-03 17:20 +0200 |
| Subject | RE: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG |
| Message-ID | <uaq4O-1Mc-25@gated-at.bofh.it> |
| In reply to | #1703160 |
On Thursday, August 3, 2017 10:23 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 03, 2017 at 02:38:17PM +0800, Wei Wang wrote:
> > +static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> > + void *addr, uint32_t size)
> > +{
> > + struct scatterlist sg;
> > + unsigned int len;
> > +
> > + sg_init_one(&sg, addr, size);
> > + while (unlikely(virtqueue_add_inbuf(vq, &sg, 1, vb, GFP_KERNEL)
> > + == -ENOSPC)) {
> > + /*
> > + * It is uncommon to see the vq is full, because the sg is sent
> > + * one by one and the device is able to handle it in time. But
> > + * if that happens, we kick and wait for an entry is released.
>
> is released -> to get used.
>
> > + */
> > + virtqueue_kick(vq);
> > + while (!virtqueue_get_buf(vq, &len) &&
> > + !virtqueue_is_broken(vq))
> > + cpu_relax();
>
> Please rework to use wait_event in that case too.
For the balloon page case here, it is fine to use wait_event. But for the free page
case, I think it might not be suitable because the mm lock is being held.
Best,
Wei
[toc] | [prev] | [next] | [standalone]
| From | "Michael S. Tsirkin" <mst@redhat.com> |
|---|---|
| Date | 2017-08-03 18:00 +0200 |
| Subject | Re: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG |
| Message-ID | <uaqHw-22a-11@gated-at.bofh.it> |
| In reply to | #1703190 |
On Thu, Aug 03, 2017 at 03:17:59PM +0000, Wang, Wei W wrote:
> On Thursday, August 3, 2017 10:23 PM, Michael S. Tsirkin wrote:
> > On Thu, Aug 03, 2017 at 02:38:17PM +0800, Wei Wang wrote:
> > > +static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> > > + void *addr, uint32_t size)
> > > +{
> > > + struct scatterlist sg;
> > > + unsigned int len;
> > > +
> > > + sg_init_one(&sg, addr, size);
> > > + while (unlikely(virtqueue_add_inbuf(vq, &sg, 1, vb, GFP_KERNEL)
> > > + == -ENOSPC)) {
> > > + /*
> > > + * It is uncommon to see the vq is full, because the sg is sent
> > > + * one by one and the device is able to handle it in time. But
> > > + * if that happens, we kick and wait for an entry is released.
> >
> > is released -> to get used.
> >
> > > + */
> > > + virtqueue_kick(vq);
> > > + while (!virtqueue_get_buf(vq, &len) &&
> > > + !virtqueue_is_broken(vq))
> > > + cpu_relax();
> >
> > Please rework to use wait_event in that case too.
>
> For the balloon page case here, it is fine to use wait_event. But for the free page
> case, I think it might not be suitable because the mm lock is being held.
>
> Best,
> Wei
You will have to find a way to drop the lock and restart from where you
stopped then.
--
MST
[toc] | [prev] | [next] | [standalone]
| From | Wei Wang <wei.w.wang@intel.com> |
|---|---|
| Date | 2017-08-03 09:00 +0200 |
| Subject | [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uaigV-4Gy-9@gated-at.bofh.it> |
| In reply to | #1702714 |
This patch adds support to walk through the free page blocks in the
system and report them via a callback function. Some page blocks may
leave the free list after the report function returns, so it is the
caller's responsibility to either detect or prevent the use of such
pages.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/mm.h | 7 ++++
include/linux/mmzone.h | 5 +++
mm/page_alloc.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 121 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 46b9ac5..24481e3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1835,6 +1835,13 @@ extern void free_area_init_node(int nid, unsigned long * zones_size,
unsigned long zone_start_pfn, unsigned long *zholes_size);
extern void free_initmem(void);
+#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
+extern void walk_free_mem_block(void *opaque1,
+ unsigned int min_order,
+ void (*visit)(void *opaque2,
+ unsigned long pfn,
+ unsigned long nr_pages));
+#endif
/*
* Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
* into the buddy system. The freed pages will be poisoned with pattern
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index fc14b8b..59eacf2 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -83,6 +83,11 @@ static inline bool is_migrate_movable(int mt)
for (order = 0; order < MAX_ORDER; order++) \
for (type = 0; type < MIGRATE_TYPES; type++)
+#define for_each_migratetype_order_decend(min_order, order, type) \
+ for (order = MAX_ORDER - 1; order < MAX_ORDER && order >= min_order; \
+ order--) \
+ for (type = 0; type < MIGRATE_TYPES; type++)
+
extern int page_group_by_mobility_disabled;
#define NR_MIGRATETYPE_BITS (PB_migrate_end - PB_migrate + 1)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6d30e91..b90b513 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4761,6 +4761,115 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
show_swap_cache_info();
}
+#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
+
+/*
+ * Heuristically get a free page block in the system.
+ *
+ * It is possible that pages from the page block are used immediately after
+ * report_free_page_block() returns. It is the caller's responsibility to
+ * either detect or prevent the use of such pages.
+ *
+ * The input parameters specify the free list to check for a free page block:
+ * zone->free_area[order].free_list[migratetype]
+ *
+ * If the caller supplied page block (i.e. **page) is on the free list, offer
+ * the next page block on the list to the caller. Otherwise, offer the first
+ * page block on the list.
+ *
+ * Return 0 when a page block is found on the caller specified free list.
+ * Otherwise, no page block is found.
+ */
+static int report_free_page_block(struct zone *zone, unsigned int order,
+ unsigned int migratetype, struct page **page)
+{
+ struct list_head *free_list;
+ int ret = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(&zone->lock, flags);
+
+ free_list = &zone->free_area[order].free_list[migratetype];
+ if (list_empty(free_list)) {
+ *page = NULL;
+ ret = -EAGAIN;
+ goto out;
+ }
+
+ /* The caller is asking for the first free page block on the list */
+ if (!(*page)) {
+ *page = list_first_entry(free_list, struct page, lru);
+ ret = 0;
+ goto out;
+ }
+
+ /*
+ * The page block passed from the caller is not on this free list
+ * anymore (e.g. a 1MB free page block has been split). In this case,
+ * offer the first page block on the free list that the caller is
+ * asking for.
+ */
+ if (PageBuddy(*page) && order != page_order(*page)) {
+ *page = list_first_entry(free_list, struct page, lru);
+ ret = 0;
+ goto out;
+ }
+
+ /*
+ * The page block passed from the caller has been the last page block
+ * on the list.
+ */
+ if ((*page)->lru.next == free_list) {
+ *page = NULL;
+ ret = -EAGAIN;
+ goto out;
+ }
+
+ /*
+ * Finally, fall into the regular case: the page block passed from the
+ * caller is still on the free list. Offer the next one.
+ */
+ *page = list_next_entry((*page), lru);
+out:
+ spin_unlock_irqrestore(&zone->lock, flags);
+ return ret;
+}
+
+/*
+ * Walk through the free page blocks in the system. The @visit callback is
+ * invoked to handle each free page block.
+ *
+ * Note: some page blocks may be used after the report function returns, so it
+ * is not safe for the callback to use any pages or discard data on such page
+ * blocks.
+ */
+void walk_free_mem_block(void *opaque1,
+ unsigned int min_order,
+ void (*visit)(void *opaque2,
+ unsigned long pfn,
+ unsigned long nr_pages))
+{
+ struct zone *zone = NULL;
+ struct page *page = NULL;
+ unsigned int order;
+ unsigned long pfn, nr_pages;
+ int type;
+
+ for_each_populated_zone(zone) {
+ for_each_migratetype_order_decend(min_order, order, type) {
+ while (!report_free_page_block(zone, order, type,
+ &page)) {
+ pfn = page_to_pfn(page);
+ nr_pages = 1 << order;
+ visit(opaque1, pfn, nr_pages);
+ }
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(walk_free_mem_block);
+
+#endif
+
static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
{
zoneref->zone = zone;
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-08-03 11:20 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uaksr-6mu-35@gated-at.bofh.it> |
| In reply to | #1702717 |
On Thu 03-08-17 14:38:18, Wei Wang wrote:
> This patch adds support to walk through the free page blocks in the
> system and report them via a callback function. Some page blocks may
> leave the free list after the report function returns, so it is the
> caller's responsibility to either detect or prevent the use of such
> pages.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/linux/mm.h | 7 ++++
> include/linux/mmzone.h | 5 +++
> mm/page_alloc.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 121 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 46b9ac5..24481e3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1835,6 +1835,13 @@ extern void free_area_init_node(int nid, unsigned long * zones_size,
> unsigned long zone_start_pfn, unsigned long *zholes_size);
> extern void free_initmem(void);
>
> +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
> +extern void walk_free_mem_block(void *opaque1,
> + unsigned int min_order,
> + void (*visit)(void *opaque2,
> + unsigned long pfn,
> + unsigned long nr_pages));
> +#endif
Is the ifdef necessary. Sure only virtio balloon driver will use this
currently but this looks like a generic functionality not specific to
virtio at all so the ifdef is rather confusing.
> /*
> * Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
> * into the buddy system. The freed pages will be poisoned with pattern
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index fc14b8b..59eacf2 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -83,6 +83,11 @@ static inline bool is_migrate_movable(int mt)
> for (order = 0; order < MAX_ORDER; order++) \
> for (type = 0; type < MIGRATE_TYPES; type++)
>
> +#define for_each_migratetype_order_decend(min_order, order, type) \
> + for (order = MAX_ORDER - 1; order < MAX_ORDER && order >= min_order; \
> + order--) \
> + for (type = 0; type < MIGRATE_TYPES; type++)
> +
Is there going to be any other user outside of mm/page_alloc.c? If not
then do not export this.
> extern int page_group_by_mobility_disabled;
>
> #define NR_MIGRATETYPE_BITS (PB_migrate_end - PB_migrate + 1)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6d30e91..b90b513 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4761,6 +4761,115 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
> show_swap_cache_info();
> }
>
> +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
> +
> +/*
> + * Heuristically get a free page block in the system.
> + *
> + * It is possible that pages from the page block are used immediately after
> + * report_free_page_block() returns. It is the caller's responsibility to
> + * either detect or prevent the use of such pages.
> + *
> + * The input parameters specify the free list to check for a free page block:
> + * zone->free_area[order].free_list[migratetype]
> + *
> + * If the caller supplied page block (i.e. **page) is on the free list, offer
> + * the next page block on the list to the caller. Otherwise, offer the first
> + * page block on the list.
> + *
> + * Return 0 when a page block is found on the caller specified free list.
> + * Otherwise, no page block is found.
> + */
> +static int report_free_page_block(struct zone *zone, unsigned int order,
> + unsigned int migratetype, struct page **page)
This is just too ugly and wrong actually. Never provide struct page
pointers outside of the zone->lock. What I've had in mind was to simply
walk free lists of the suitable order and call the callback for each one.
Something as simple as
for (i = 0; i < MAX_NR_ZONES; i++) {
struct zone *zone = &pgdat->node_zones[i];
if (!populated_zone(zone))
continue;
spin_lock_irqsave(&zone->lock, flags);
for (order = min_order; order < MAX_ORDER; ++order) {
struct free_area *free_area = &zone->free_area[order];
enum migratetype mt;
struct page *page;
if (!free_area->nr_pages)
continue;
for_each_migratetype_order(order, mt) {
list_for_each_entry(page,
&free_area->free_list[mt], lru) {
pfn = page_to_pfn(page);
visit(opaque2, prn, 1<<order);
}
}
}
spin_unlock_irqrestore(&zone->lock, flags);
}
[...]
> +/*
> + * Walk through the free page blocks in the system. The @visit callback is
> + * invoked to handle each free page block.
> + *
> + * Note: some page blocks may be used after the report function returns, so it
> + * is not safe for the callback to use any pages or discard data on such page
> + * blocks.
> + */
> +void walk_free_mem_block(void *opaque1,
> + unsigned int min_order,
> + void (*visit)(void *opaque2,
> + unsigned long pfn,
> + unsigned long nr_pages))
Is there any reason why there is no node id? I guess you just do not
care for your particular use case. Not that I care too much either. If
somebody wants this per node then it would be trivial to extend I was
just wondering whether this is a deliberate decision or an omission.
> +{
> + struct zone *zone = NULL;
> + struct page *page = NULL;
> + unsigned int order;
> + unsigned long pfn, nr_pages;
> + int type;
> +
> + for_each_populated_zone(zone) {
> + for_each_migratetype_order_decend(min_order, order, type) {
> + while (!report_free_page_block(zone, order, type,
> + &page)) {
> + pfn = page_to_pfn(page);
> + nr_pages = 1 << order;
> + visit(opaque1, pfn, nr_pages);
> + }
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(walk_free_mem_block);
> +
> +#endif
> +
> static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
> {
> zoneref->zone = zone;
> --
> 2.7.4
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Wei Wang <wei.w.wang@intel.com> |
|---|---|
| Date | 2017-08-03 12:40 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <ualHP-770-9@gated-at.bofh.it> |
| In reply to | #1702820 |
On 08/03/2017 05:11 PM, Michal Hocko wrote:
> On Thu 03-08-17 14:38:18, Wei Wang wrote:
>> This patch adds support to walk through the free page blocks in the
>> system and report them via a callback function. Some page blocks may
>> leave the free list after the report function returns, so it is the
>> caller's responsibility to either detect or prevent the use of such
>> pages.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Signed-off-by: Liang Li <liang.z.li@intel.com>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> include/linux/mm.h | 7 ++++
>> include/linux/mmzone.h | 5 +++
>> mm/page_alloc.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 121 insertions(+)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 46b9ac5..24481e3 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1835,6 +1835,13 @@ extern void free_area_init_node(int nid, unsigned long * zones_size,
>> unsigned long zone_start_pfn, unsigned long *zholes_size);
>> extern void free_initmem(void);
>>
>> +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
>> +extern void walk_free_mem_block(void *opaque1,
>> + unsigned int min_order,
>> + void (*visit)(void *opaque2,
>> + unsigned long pfn,
>> + unsigned long nr_pages));
>> +#endif
> Is the ifdef necessary. Sure only virtio balloon driver will use this
> currently but this looks like a generic functionality not specific to
> virtio at all so the ifdef is rather confusing.
OK. We can remove the condition if no objection from others.
>
>> extern int page_group_by_mobility_disabled;
>>
>> #define NR_MIGRATETYPE_BITS (PB_migrate_end - PB_migrate + 1)
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 6d30e91..b90b513 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -4761,6 +4761,115 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
>> show_swap_cache_info();
>> }
>>
>> +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
>> +
>> +/*
>> + * Heuristically get a free page block in the system.
>> + *
>> + * It is possible that pages from the page block are used immediately after
>> + * report_free_page_block() returns. It is the caller's responsibility to
>> + * either detect or prevent the use of such pages.
>> + *
>> + * The input parameters specify the free list to check for a free page block:
>> + * zone->free_area[order].free_list[migratetype]
>> + *
>> + * If the caller supplied page block (i.e. **page) is on the free list, offer
>> + * the next page block on the list to the caller. Otherwise, offer the first
>> + * page block on the list.
>> + *
>> + * Return 0 when a page block is found on the caller specified free list.
>> + * Otherwise, no page block is found.
>> + */
>> +static int report_free_page_block(struct zone *zone, unsigned int order,
>> + unsigned int migratetype, struct page **page)
> This is just too ugly and wrong actually. Never provide struct page
> pointers outside of the zone->lock. What I've had in mind was to simply
> walk free lists of the suitable order and call the callback for each one.
> Something as simple as
>
> for (i = 0; i < MAX_NR_ZONES; i++) {
> struct zone *zone = &pgdat->node_zones[i];
>
> if (!populated_zone(zone))
> continue;
> spin_lock_irqsave(&zone->lock, flags);
> for (order = min_order; order < MAX_ORDER; ++order) {
> struct free_area *free_area = &zone->free_area[order];
> enum migratetype mt;
> struct page *page;
>
> if (!free_area->nr_pages)
> continue;
>
> for_each_migratetype_order(order, mt) {
> list_for_each_entry(page,
> &free_area->free_list[mt], lru) {
>
> pfn = page_to_pfn(page);
> visit(opaque2, prn, 1<<order);
> }
> }
> }
>
> spin_unlock_irqrestore(&zone->lock, flags);
> }
>
> [...]
I think the above would take the lock for too long time. That's why we
prefer
to take one free page block each time, and taking it one by one also doesn't
make a difference, in terms of the performance that we need.
The struct page is used as a "state" to get the next free page block. It
is only
given for an internal implementation of a function in mm ( not seen by the
outside caller). Would this be OK?
If not, how about pfn - we can also pass in pfn to the function, and do
pfn_to_page each time the function starts, and then do page_to_pfn when
returns.
>> +/*
>> + * Walk through the free page blocks in the system. The @visit callback is
>> + * invoked to handle each free page block.
>> + *
>> + * Note: some page blocks may be used after the report function returns, so it
>> + * is not safe for the callback to use any pages or discard data on such page
>> + * blocks.
>> + */
>> +void walk_free_mem_block(void *opaque1,
>> + unsigned int min_order,
>> + void (*visit)(void *opaque2,
>> + unsigned long pfn,
>> + unsigned long nr_pages))
> Is there any reason why there is no node id? I guess you just do not
> care for your particular use case. Not that I care too much either. If
> somebody wants this per node then it would be trivial to extend I was
> just wondering whether this is a deliberate decision or an omission.
>
Right, we don't care about the node id. Live migration transfers all the
guest
system memory, so we just want to get the hint of all the free page blocks
from the system.
Best,
Wei
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-08-03 12:50 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <ualRw-7bw-1@gated-at.bofh.it> |
| In reply to | #1702915 |
On Thu 03-08-17 18:42:15, Wei Wang wrote:
> On 08/03/2017 05:11 PM, Michal Hocko wrote:
> >On Thu 03-08-17 14:38:18, Wei Wang wrote:
[...]
> >>+static int report_free_page_block(struct zone *zone, unsigned int order,
> >>+ unsigned int migratetype, struct page **page)
> >This is just too ugly and wrong actually. Never provide struct page
> >pointers outside of the zone->lock. What I've had in mind was to simply
> >walk free lists of the suitable order and call the callback for each one.
> >Something as simple as
> >
> > for (i = 0; i < MAX_NR_ZONES; i++) {
> > struct zone *zone = &pgdat->node_zones[i];
> >
> > if (!populated_zone(zone))
> > continue;
> > spin_lock_irqsave(&zone->lock, flags);
> > for (order = min_order; order < MAX_ORDER; ++order) {
> > struct free_area *free_area = &zone->free_area[order];
> > enum migratetype mt;
> > struct page *page;
> >
> > if (!free_area->nr_pages)
> > continue;
> >
> > for_each_migratetype_order(order, mt) {
> > list_for_each_entry(page,
> > &free_area->free_list[mt], lru) {
> >
> > pfn = page_to_pfn(page);
> > visit(opaque2, prn, 1<<order);
> > }
> > }
> > }
> >
> > spin_unlock_irqrestore(&zone->lock, flags);
> > }
> >
> >[...]
>
>
> I think the above would take the lock for too long time. That's why we
> prefer to take one free page block each time, and taking it one by one
> also doesn't make a difference, in terms of the performance that we
> need.
I think you should start with simple approach and impove incrementally
if this turns out to be not optimal. I really detest taking struct pages
outside of the lock. You never know what might happen after the lock is
dropped. E.g. can you race with the memory hotremove?
> The struct page is used as a "state" to get the next free page block. It is
> only
> given for an internal implementation of a function in mm ( not seen by the
> outside caller). Would this be OK?
> If not, how about pfn - we can also pass in pfn to the function, and do
> pfn_to_page each time the function starts, and then do page_to_pfn when
> returns.
No, just do not try to play tricks with struct pages which might have
gone away.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Wei Wang <wei.w.wang@intel.com> |
|---|---|
| Date | 2017-08-03 13:30 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uamud-7FZ-9@gated-at.bofh.it> |
| In reply to | #1702922 |
On 08/03/2017 06:44 PM, Michal Hocko wrote:
> On Thu 03-08-17 18:42:15, Wei Wang wrote:
>> On 08/03/2017 05:11 PM, Michal Hocko wrote:
>>> On Thu 03-08-17 14:38:18, Wei Wang wrote:
> [...]
>>>> +static int report_free_page_block(struct zone *zone, unsigned int order,
>>>> + unsigned int migratetype, struct page **page)
>>> This is just too ugly and wrong actually. Never provide struct page
>>> pointers outside of the zone->lock. What I've had in mind was to simply
>>> walk free lists of the suitable order and call the callback for each one.
>>> Something as simple as
>>>
>>> for (i = 0; i < MAX_NR_ZONES; i++) {
>>> struct zone *zone = &pgdat->node_zones[i];
>>>
>>> if (!populated_zone(zone))
>>> continue;
>>> spin_lock_irqsave(&zone->lock, flags);
>>> for (order = min_order; order < MAX_ORDER; ++order) {
>>> struct free_area *free_area = &zone->free_area[order];
>>> enum migratetype mt;
>>> struct page *page;
>>>
>>> if (!free_area->nr_pages)
>>> continue;
>>>
>>> for_each_migratetype_order(order, mt) {
>>> list_for_each_entry(page,
>>> &free_area->free_list[mt], lru) {
>>>
>>> pfn = page_to_pfn(page);
>>> visit(opaque2, prn, 1<<order);
>>> }
>>> }
>>> }
>>>
>>> spin_unlock_irqrestore(&zone->lock, flags);
>>> }
>>>
>>> [...]
>>
>> I think the above would take the lock for too long time. That's why we
>> prefer to take one free page block each time, and taking it one by one
>> also doesn't make a difference, in terms of the performance that we
>> need.
> I think you should start with simple approach and impove incrementally
> if this turns out to be not optimal. I really detest taking struct pages
> outside of the lock. You never know what might happen after the lock is
> dropped. E.g. can you race with the memory hotremove?
The caller won't use pages returned from the function, so I think there
shouldn't be an issue or race if the returned pages are used (i.e. not free
anymore) or simply gone due to hotremove.
Best,
Wei
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-08-03 13:30 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uamue-7FZ-15@gated-at.bofh.it> |
| In reply to | #1702953 |
On Thu 03-08-17 19:27:19, Wei Wang wrote:
> On 08/03/2017 06:44 PM, Michal Hocko wrote:
> >On Thu 03-08-17 18:42:15, Wei Wang wrote:
> >>On 08/03/2017 05:11 PM, Michal Hocko wrote:
> >>>On Thu 03-08-17 14:38:18, Wei Wang wrote:
> >[...]
> >>>>+static int report_free_page_block(struct zone *zone, unsigned int order,
> >>>>+ unsigned int migratetype, struct page **page)
> >>>This is just too ugly and wrong actually. Never provide struct page
> >>>pointers outside of the zone->lock. What I've had in mind was to simply
> >>>walk free lists of the suitable order and call the callback for each one.
> >>>Something as simple as
> >>>
> >>> for (i = 0; i < MAX_NR_ZONES; i++) {
> >>> struct zone *zone = &pgdat->node_zones[i];
> >>>
> >>> if (!populated_zone(zone))
> >>> continue;
> >>> spin_lock_irqsave(&zone->lock, flags);
> >>> for (order = min_order; order < MAX_ORDER; ++order) {
> >>> struct free_area *free_area = &zone->free_area[order];
> >>> enum migratetype mt;
> >>> struct page *page;
> >>>
> >>> if (!free_area->nr_pages)
> >>> continue;
> >>>
> >>> for_each_migratetype_order(order, mt) {
> >>> list_for_each_entry(page,
> >>> &free_area->free_list[mt], lru) {
> >>>
> >>> pfn = page_to_pfn(page);
> >>> visit(opaque2, prn, 1<<order);
> >>> }
> >>> }
> >>> }
> >>>
> >>> spin_unlock_irqrestore(&zone->lock, flags);
> >>> }
> >>>
> >>>[...]
> >>
> >>I think the above would take the lock for too long time. That's why we
> >>prefer to take one free page block each time, and taking it one by one
> >>also doesn't make a difference, in terms of the performance that we
> >>need.
> >I think you should start with simple approach and impove incrementally
> >if this turns out to be not optimal. I really detest taking struct pages
> >outside of the lock. You never know what might happen after the lock is
> >dropped. E.g. can you race with the memory hotremove?
>
>
> The caller won't use pages returned from the function, so I think there
> shouldn't be an issue or race if the returned pages are used (i.e. not free
> anymore) or simply gone due to hotremove.
No, this is just too error prone. Consider that struct page pointer
itself could get invalid in the meantime. Please always keep robustness
in mind first. Optimizations are nice but it is even not clear whether
the simple variant will cause any problems.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Wei Wang <wei.w.wang@intel.com> |
|---|---|
| Date | 2017-08-03 14:10 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uan6W-8co-21@gated-at.bofh.it> |
| In reply to | #1702955 |
On 08/03/2017 07:28 PM, Michal Hocko wrote:
> On Thu 03-08-17 19:27:19, Wei Wang wrote:
>> On 08/03/2017 06:44 PM, Michal Hocko wrote:
>>> On Thu 03-08-17 18:42:15, Wei Wang wrote:
>>>> On 08/03/2017 05:11 PM, Michal Hocko wrote:
>>>>> On Thu 03-08-17 14:38:18, Wei Wang wrote:
>>> [...]
>>>>>> +static int report_free_page_block(struct zone *zone, unsigned int order,
>>>>>> + unsigned int migratetype, struct page **page)
>>>>> This is just too ugly and wrong actually. Never provide struct page
>>>>> pointers outside of the zone->lock. What I've had in mind was to simply
>>>>> walk free lists of the suitable order and call the callback for each one.
>>>>> Something as simple as
>>>>>
>>>>> for (i = 0; i < MAX_NR_ZONES; i++) {
>>>>> struct zone *zone = &pgdat->node_zones[i];
>>>>>
>>>>> if (!populated_zone(zone))
>>>>> continue;
>>>>> spin_lock_irqsave(&zone->lock, flags);
>>>>> for (order = min_order; order < MAX_ORDER; ++order) {
>>>>> struct free_area *free_area = &zone->free_area[order];
>>>>> enum migratetype mt;
>>>>> struct page *page;
>>>>>
>>>>> if (!free_area->nr_pages)
>>>>> continue;
>>>>>
>>>>> for_each_migratetype_order(order, mt) {
>>>>> list_for_each_entry(page,
>>>>> &free_area->free_list[mt], lru) {
>>>>>
>>>>> pfn = page_to_pfn(page);
>>>>> visit(opaque2, prn, 1<<order);
>>>>> }
>>>>> }
>>>>> }
>>>>>
>>>>> spin_unlock_irqrestore(&zone->lock, flags);
>>>>> }
>>>>>
>>>>> [...]
>>>> I think the above would take the lock for too long time. That's why we
>>>> prefer to take one free page block each time, and taking it one by one
>>>> also doesn't make a difference, in terms of the performance that we
>>>> need.
>>> I think you should start with simple approach and impove incrementally
>>> if this turns out to be not optimal. I really detest taking struct pages
>>> outside of the lock. You never know what might happen after the lock is
>>> dropped. E.g. can you race with the memory hotremove?
>>
>> The caller won't use pages returned from the function, so I think there
>> shouldn't be an issue or race if the returned pages are used (i.e. not free
>> anymore) or simply gone due to hotremove.
> No, this is just too error prone. Consider that struct page pointer
> itself could get invalid in the meantime. Please always keep robustness
> in mind first. Optimizations are nice but it is even not clear whether
> the simple variant will cause any problems.
how about this:
for_each_populated_zone(zone) {
for_each_migratetype_order_decend(min_order, order, type) {
do {
=> spin_lock_irqsave(&zone->lock, flags);
ret = report_free_page_block(zone, order, type,
&page)) {
pfn = page_to_pfn(page);
nr_pages = 1 << order;
visit(opaque1, pfn, nr_pages);
}
=> spin_unlock_irqrestore(&zone->lock, flags);
} while (!ret)
}
In this way, we can still keep the lock granularity at one free page block
while having the struct page operated under the lock.
Best,
Wei
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-08-03 14:50 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uanJF-8s6-31@gated-at.bofh.it> |
| In reply to | #1703007 |
On Thu 03-08-17 20:11:58, Wei Wang wrote:
> On 08/03/2017 07:28 PM, Michal Hocko wrote:
> >On Thu 03-08-17 19:27:19, Wei Wang wrote:
> >>On 08/03/2017 06:44 PM, Michal Hocko wrote:
> >>>On Thu 03-08-17 18:42:15, Wei Wang wrote:
> >>>>On 08/03/2017 05:11 PM, Michal Hocko wrote:
> >>>>>On Thu 03-08-17 14:38:18, Wei Wang wrote:
> >>>[...]
> >>>>>>+static int report_free_page_block(struct zone *zone, unsigned int order,
> >>>>>>+ unsigned int migratetype, struct page **page)
> >>>>>This is just too ugly and wrong actually. Never provide struct page
> >>>>>pointers outside of the zone->lock. What I've had in mind was to simply
> >>>>>walk free lists of the suitable order and call the callback for each one.
> >>>>>Something as simple as
> >>>>>
> >>>>> for (i = 0; i < MAX_NR_ZONES; i++) {
> >>>>> struct zone *zone = &pgdat->node_zones[i];
> >>>>>
> >>>>> if (!populated_zone(zone))
> >>>>> continue;
> >>>>> spin_lock_irqsave(&zone->lock, flags);
> >>>>> for (order = min_order; order < MAX_ORDER; ++order) {
> >>>>> struct free_area *free_area = &zone->free_area[order];
> >>>>> enum migratetype mt;
> >>>>> struct page *page;
> >>>>>
> >>>>> if (!free_area->nr_pages)
> >>>>> continue;
> >>>>>
> >>>>> for_each_migratetype_order(order, mt) {
> >>>>> list_for_each_entry(page,
> >>>>> &free_area->free_list[mt], lru) {
> >>>>>
> >>>>> pfn = page_to_pfn(page);
> >>>>> visit(opaque2, prn, 1<<order);
> >>>>> }
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> spin_unlock_irqrestore(&zone->lock, flags);
> >>>>> }
> >>>>>
> >>>>>[...]
> >>>>I think the above would take the lock for too long time. That's why we
> >>>>prefer to take one free page block each time, and taking it one by one
> >>>>also doesn't make a difference, in terms of the performance that we
> >>>>need.
> >>>I think you should start with simple approach and impove incrementally
> >>>if this turns out to be not optimal. I really detest taking struct pages
> >>>outside of the lock. You never know what might happen after the lock is
> >>>dropped. E.g. can you race with the memory hotremove?
> >>
> >>The caller won't use pages returned from the function, so I think there
> >>shouldn't be an issue or race if the returned pages are used (i.e. not free
> >>anymore) or simply gone due to hotremove.
> >No, this is just too error prone. Consider that struct page pointer
> >itself could get invalid in the meantime. Please always keep robustness
> >in mind first. Optimizations are nice but it is even not clear whether
> >the simple variant will cause any problems.
>
>
> how about this:
>
> for_each_populated_zone(zone) {
> for_each_migratetype_order_decend(min_order, order, type) {
> do {
> => spin_lock_irqsave(&zone->lock, flags);
> ret = report_free_page_block(zone, order, type,
> &page)) {
> pfn = page_to_pfn(page);
> nr_pages = 1 << order;
> visit(opaque1, pfn, nr_pages);
> }
> => spin_unlock_irqrestore(&zone->lock, flags);
> } while (!ret)
> }
>
> In this way, we can still keep the lock granularity at one free page block
> while having the struct page operated under the lock.
How can you continue iteration of free_list after the lock has been
dropped? If you want to keep the lock held for each migrate type then
why not. Just push the lock inside for_each_migratetype_order loop from
my example.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Wei Wang <wei.w.wang@intel.com> |
|---|---|
| Date | 2017-08-03 15:20 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uaocG-rc-25@gated-at.bofh.it> |
| In reply to | #1703079 |
On 08/03/2017 08:41 PM, Michal Hocko wrote:
> On Thu 03-08-17 20:11:58, Wei Wang wrote:
>> On 08/03/2017 07:28 PM, Michal Hocko wrote:
>>> On Thu 03-08-17 19:27:19, Wei Wang wrote:
>>>> On 08/03/2017 06:44 PM, Michal Hocko wrote:
>>>>> On Thu 03-08-17 18:42:15, Wei Wang wrote:
>>>>>> On 08/03/2017 05:11 PM, Michal Hocko wrote:
>>>>>>> On Thu 03-08-17 14:38:18, Wei Wang wrote:
>>>>> [...]
>>>>>>>> +static int report_free_page_block(struct zone *zone, unsigned int order,
>>>>>>>> + unsigned int migratetype, struct page **page)
>>>>>>> This is just too ugly and wrong actually. Never provide struct page
>>>>>>> pointers outside of the zone->lock. What I've had in mind was to simply
>>>>>>> walk free lists of the suitable order and call the callback for each one.
>>>>>>> Something as simple as
>>>>>>>
>>>>>>> for (i = 0; i < MAX_NR_ZONES; i++) {
>>>>>>> struct zone *zone = &pgdat->node_zones[i];
>>>>>>>
>>>>>>> if (!populated_zone(zone))
>>>>>>> continue;
>>>>>>> spin_lock_irqsave(&zone->lock, flags);
>>>>>>> for (order = min_order; order < MAX_ORDER; ++order) {
>>>>>>> struct free_area *free_area = &zone->free_area[order];
>>>>>>> enum migratetype mt;
>>>>>>> struct page *page;
>>>>>>>
>>>>>>> if (!free_area->nr_pages)
>>>>>>> continue;
>>>>>>>
>>>>>>> for_each_migratetype_order(order, mt) {
>>>>>>> list_for_each_entry(page,
>>>>>>> &free_area->free_list[mt], lru) {
>>>>>>>
>>>>>>> pfn = page_to_pfn(page);
>>>>>>> visit(opaque2, prn, 1<<order);
>>>>>>> }
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> spin_unlock_irqrestore(&zone->lock, flags);
>>>>>>> }
>>>>>>>
>>>>>>> [...]
>>>>>> I think the above would take the lock for too long time. That's why we
>>>>>> prefer to take one free page block each time, and taking it one by one
>>>>>> also doesn't make a difference, in terms of the performance that we
>>>>>> need.
>>>>> I think you should start with simple approach and impove incrementally
>>>>> if this turns out to be not optimal. I really detest taking struct pages
>>>>> outside of the lock. You never know what might happen after the lock is
>>>>> dropped. E.g. can you race with the memory hotremove?
>>>> The caller won't use pages returned from the function, so I think there
>>>> shouldn't be an issue or race if the returned pages are used (i.e. not free
>>>> anymore) or simply gone due to hotremove.
>>> No, this is just too error prone. Consider that struct page pointer
>>> itself could get invalid in the meantime. Please always keep robustness
>>> in mind first. Optimizations are nice but it is even not clear whether
>>> the simple variant will cause any problems.
>>
>> how about this:
>>
>> for_each_populated_zone(zone) {
>> for_each_migratetype_order_decend(min_order, order, type) {
>> do {
>> => spin_lock_irqsave(&zone->lock, flags);
>> ret = report_free_page_block(zone, order, type,
>> &page)) {
>> pfn = page_to_pfn(page);
>> nr_pages = 1 << order;
>> visit(opaque1, pfn, nr_pages);
>> }
>> => spin_unlock_irqrestore(&zone->lock, flags);
>> } while (!ret)
>> }
>>
>> In this way, we can still keep the lock granularity at one free page block
>> while having the struct page operated under the lock.
> How can you continue iteration of free_list after the lock has been
> dropped?
report_free_page_block() has handled all the possible cases after the
lock is
dropped. For example, if the previous reported page has not been on the free
list, then the first node from the list of this order will be given.
This is because
page allocation takes page blocks from the head to end, for example:
1,2,3,4,5,6
if the previous reported free block is 2, when we give 2 to the report
function
to get the next page block, and find 1,2,3 have all gone, it will report
4, which
is the head of the free list.
> If you want to keep the lock held for each migrate type then
> why not. Just push the lock inside for_each_migratetype_order loop from
> my example.
>
The above lock is held for each free page block, instead of each migrate
type, since
the report function only reports one page block each time.
Best,
Wei
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-08-03 16:00 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uaoPn-Jx-9@gated-at.bofh.it> |
| In reply to | #1703103 |
On Thu 03-08-17 21:17:25, Wei Wang wrote:
> On 08/03/2017 08:41 PM, Michal Hocko wrote:
> >On Thu 03-08-17 20:11:58, Wei Wang wrote:
> >>On 08/03/2017 07:28 PM, Michal Hocko wrote:
> >>>On Thu 03-08-17 19:27:19, Wei Wang wrote:
> >>>>On 08/03/2017 06:44 PM, Michal Hocko wrote:
> >>>>>On Thu 03-08-17 18:42:15, Wei Wang wrote:
> >>>>>>On 08/03/2017 05:11 PM, Michal Hocko wrote:
> >>>>>>>On Thu 03-08-17 14:38:18, Wei Wang wrote:
> >>>>>[...]
> >>>>>>>>+static int report_free_page_block(struct zone *zone, unsigned int order,
> >>>>>>>>+ unsigned int migratetype, struct page **page)
> >>>>>>>This is just too ugly and wrong actually. Never provide struct page
> >>>>>>>pointers outside of the zone->lock. What I've had in mind was to simply
> >>>>>>>walk free lists of the suitable order and call the callback for each one.
> >>>>>>>Something as simple as
> >>>>>>>
> >>>>>>> for (i = 0; i < MAX_NR_ZONES; i++) {
> >>>>>>> struct zone *zone = &pgdat->node_zones[i];
> >>>>>>>
> >>>>>>> if (!populated_zone(zone))
> >>>>>>> continue;
> >>>>>>> spin_lock_irqsave(&zone->lock, flags);
> >>>>>>> for (order = min_order; order < MAX_ORDER; ++order) {
> >>>>>>> struct free_area *free_area = &zone->free_area[order];
> >>>>>>> enum migratetype mt;
> >>>>>>> struct page *page;
> >>>>>>>
> >>>>>>> if (!free_area->nr_pages)
> >>>>>>> continue;
> >>>>>>>
> >>>>>>> for_each_migratetype_order(order, mt) {
> >>>>>>> list_for_each_entry(page,
> >>>>>>> &free_area->free_list[mt], lru) {
> >>>>>>>
> >>>>>>> pfn = page_to_pfn(page);
> >>>>>>> visit(opaque2, prn, 1<<order);
> >>>>>>> }
> >>>>>>> }
> >>>>>>> }
> >>>>>>>
> >>>>>>> spin_unlock_irqrestore(&zone->lock, flags);
> >>>>>>> }
> >>>>>>>
> >>>>>>>[...]
> >>>>>>I think the above would take the lock for too long time. That's why we
> >>>>>>prefer to take one free page block each time, and taking it one by one
> >>>>>>also doesn't make a difference, in terms of the performance that we
> >>>>>>need.
> >>>>>I think you should start with simple approach and impove incrementally
> >>>>>if this turns out to be not optimal. I really detest taking struct pages
> >>>>>outside of the lock. You never know what might happen after the lock is
> >>>>>dropped. E.g. can you race with the memory hotremove?
> >>>>The caller won't use pages returned from the function, so I think there
> >>>>shouldn't be an issue or race if the returned pages are used (i.e. not free
> >>>>anymore) or simply gone due to hotremove.
> >>>No, this is just too error prone. Consider that struct page pointer
> >>>itself could get invalid in the meantime. Please always keep robustness
> >>>in mind first. Optimizations are nice but it is even not clear whether
> >>>the simple variant will cause any problems.
> >>
> >>how about this:
> >>
> >>for_each_populated_zone(zone) {
> >> for_each_migratetype_order_decend(min_order, order, type) {
> >> do {
> >> => spin_lock_irqsave(&zone->lock, flags);
> >> ret = report_free_page_block(zone, order, type,
> >> &page)) {
> >> pfn = page_to_pfn(page);
> >> nr_pages = 1 << order;
> >> visit(opaque1, pfn, nr_pages);
> >> }
> >> => spin_unlock_irqrestore(&zone->lock, flags);
> >> } while (!ret)
> >>}
> >>
> >>In this way, we can still keep the lock granularity at one free page block
> >>while having the struct page operated under the lock.
> >How can you continue iteration of free_list after the lock has been
> >dropped?
>
> report_free_page_block() has handled all the possible cases after the lock
> is
> dropped. For example, if the previous reported page has not been on the free
> list, then the first node from the list of this order will be given. This is
> because
> page allocation takes page blocks from the head to end, for example:
>
> 1,2,3,4,5,6
> if the previous reported free block is 2, when we give 2 to the report
> function
> to get the next page block, and find 1,2,3 have all gone, it will report 4,
> which
> is the head of the free list.
As I've said earlier. Start simple optimize incrementally with some
numbers to justify a more subtle code.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | "Wang, Wei W" <wei.w.wang@intel.com> |
|---|---|
| Date | 2017-08-03 17:30 +0200 |
| Subject | RE: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uaqet-1Q1-1@gated-at.bofh.it> |
| In reply to | #1703143 |
On Thursday, August 3, 2017 9:51 PM, Michal Hocko: > As I've said earlier. Start simple optimize incrementally with some numbers to > justify a more subtle code. > -- OK. Let's start with the simple implementation as you suggested. Best, Wei
[toc] | [prev] | [next] | [standalone]
| From | "Michael S. Tsirkin" <mst@redhat.com> |
|---|---|
| Date | 2017-08-03 23:10 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uavxw-5BE-11@gated-at.bofh.it> |
| In reply to | #1703193 |
On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote: > On Thursday, August 3, 2017 9:51 PM, Michal Hocko: > > As I've said earlier. Start simple optimize incrementally with some numbers to > > justify a more subtle code. > > -- > > OK. Let's start with the simple implementation as you suggested. > > Best, > Wei The tricky part is when you need to drop the lock and then restart because the device is busy. Would it maybe make sense to rotate the list so that new head will consist of pages not yet sent to device? -- MST
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-08-04 10:00 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uaFGx-4bb-7@gated-at.bofh.it> |
| In reply to | #1703418 |
On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote: > On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote: > > On Thursday, August 3, 2017 9:51 PM, Michal Hocko: > > > As I've said earlier. Start simple optimize incrementally with some numbers to > > > justify a more subtle code. > > > -- > > > > OK. Let's start with the simple implementation as you suggested. > > > > Best, > > Wei > > The tricky part is when you need to drop the lock and > then restart because the device is busy. Would it maybe > make sense to rotate the list so that new head > will consist of pages not yet sent to device? No, I this should be strictly non-modifying API. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Wei Wang <wei.w.wang@intel.com> |
|---|---|
| Date | 2017-08-04 10:20 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uaFZU-4wN-19@gated-at.bofh.it> |
| In reply to | #1703764 |
On 08/04/2017 03:53 PM, Michal Hocko wrote:
> On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote:
>> On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
>>> On Thursday, August 3, 2017 9:51 PM, Michal Hocko:
>>>> As I've said earlier. Start simple optimize incrementally with some numbers to
>>>> justify a more subtle code.
>>>> --
>>> OK. Let's start with the simple implementation as you suggested.
>>>
>>> Best,
>>> Wei
>> The tricky part is when you need to drop the lock and
>> then restart because the device is busy. Would it maybe
>> make sense to rotate the list so that new head
>> will consist of pages not yet sent to device?
> No, I this should be strictly non-modifying API.
Just get the context here for discussion:
spin_lock_irqsave(&zone->lock, flags);
...
visit(opaque2, pfn, 1<<order);
spin_unlock_irqrestore(&zone->lock, flags);
The concern is that the callback may cause the lock be
taken too long.
I think here we can have two options:
- Option 1: Put a Note for the callback: the callback function
should not block and it should finish as soon as possible.
(when implementing an interrupt handler, we also have
such similar rules in mind, right?).
For our use case, the callback just puts the reported page
block to the ring, then returns. If the ring is full as the host
is busy, then I think it should skip this one, and just return.
Because:
A. This is an optimization feature, losing a couple of free
pages to report isn't that important;
B. In reality, I think it's uncommon to see this ring getting
full (I didn't observe ring full in the tests), since the host
(consumer) is notified to take out the page block right
after it is added.
- Option 2: Put the callback function outside the lock
What's input into the callback is just a pfn, and the callback
won't access the corresponding pages. So, I still think it won't
be an issue no matter what status of the pages is after they
are reported (even they doesn't exit due to hot-remove).
What would you guys think?
Best,
Wei
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-08-04 10:30 +0200 |
| Subject | Re: [PATCH v13 4/5] mm: support reporting free page blocks |
| Message-ID | <uaG9z-4A4-3@gated-at.bofh.it> |
| In reply to | #1703783 |
On Fri 04-08-17 16:15:24, Wei Wang wrote: > On 08/04/2017 03:53 PM, Michal Hocko wrote: > >On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote: > >>On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote: > >>>On Thursday, August 3, 2017 9:51 PM, Michal Hocko: > >>>>As I've said earlier. Start simple optimize incrementally with some numbers to > >>>>justify a more subtle code. > >>>>-- > >>>OK. Let's start with the simple implementation as you suggested. > >>> > >>>Best, > >>>Wei > >>The tricky part is when you need to drop the lock and > >>then restart because the device is busy. Would it maybe > >>make sense to rotate the list so that new head > >>will consist of pages not yet sent to device? > >No, I this should be strictly non-modifying API. > > > Just get the context here for discussion: > > spin_lock_irqsave(&zone->lock, flags); > ... > visit(opaque2, pfn, 1<<order); > spin_unlock_irqrestore(&zone->lock, flags); > > The concern is that the callback may cause the lock be > taken too long. > > > I think here we can have two options: > - Option 1: Put a Note for the callback: the callback function > should not block and it should finish as soon as possible. > (when implementing an interrupt handler, we also have > such similar rules in mind, right?). absolutely > For our use case, the callback just puts the reported page > block to the ring, then returns. If the ring is full as the host > is busy, then I think it should skip this one, and just return. > Because: > A. This is an optimization feature, losing a couple of free > pages to report isn't that important; > B. In reality, I think it's uncommon to see this ring getting > full (I didn't observe ring full in the tests), since the host > (consumer) is notified to take out the page block right > after it is added. I thought you only updated a pre allocated bitmat... Anyway, I cannot comment on this part much as I am not familiar with your usecase. > - Option 2: Put the callback function outside the lock > What's input into the callback is just a pfn, and the callback > won't access the corresponding pages. So, I still think it won't > be an issue no matter what status of the pages is after they > are reported (even they doesn't exit due to hot-remove). This would make the API implementation more complex and I am not yet convinced we really need that. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web