* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Paolo Bonzini @ 2012-08-21 8:48 UTC (permalink / raw)
To: Cong Meng; +Cc: stefanha, zwanp, linuxram, qemu-devel, virtualization
In-Reply-To: <1345537427-21601-1-git-send-email-mc@linux.vnet.ibm.com>
Il 21/08/2012 10:23, Cong Meng ha scritto:
> +static void sg_get_queue_limits(BlockDriverState *bs, const char *filename)
> +{
> + DIR *ffs;
> + struct dirent *d;
> + char path[MAXPATHLEN];
> +
> + snprintf(path, MAXPATHLEN,
> + "/sys/class/scsi_generic/sg%s/device/block/",
> + filename + strlen("/dev/sg"));
> +
> + ffs = opendir(path);
> + if (!ffs) {
> + return;
> + }
> +
> + for (;;) {
> + d = readdir(ffs);
> + if (!d) {
> + return;
> + }
> +
> + if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0) {
> + continue;
> + }
> +
> + break;
> + }
> +
> + closedir(ffs);
> +
> + pstrcat(path, MAXPATHLEN, d->d_name);
> + pstrcat(path, MAXPATHLEN, "/queue/");
> +
> + read_queue_limit(path, "max_sectors_kb", &bs->max_sectors);
> + read_queue_limit(path, "max_segments", &bs->max_segments);
> + read_queue_limit(path, "max_segment_size", &bs->max_segment_size);
> +}
Using /sys/dev/block or /sys/dev/char seems easier, and lets you
retrieve the parameters for block devices too.
However, I'm worried of the consequences this has for migration. You
could have the same physical disk accessed with two different HBAs, with
different limits. So I don't know if this can really be solved at all.
Paolo
^ permalink raw reply
* Re: [PATCH v1] virtio-scsi: get and set the queue limits for sg device
From: Stefan Hajnoczi @ 2012-08-21 8:53 UTC (permalink / raw)
To: Cong Meng
Cc: stefanha, zwanp, linuxram, qemu-devel, linux-kernel,
Paolo Bonzini, virtualization
In-Reply-To: <1345537609-21676-1-git-send-email-mc@linux.vnet.ibm.com>
On Tue, Aug 21, 2012 at 9:26 AM, Cong Meng <mc@linux.vnet.ibm.com> wrote:
> Each virtio scsi HBA has global request queue limits. But the passthrough
> LUNs (scsi-generic) come from different host HBAs may have different request
> queue limits. If the guest sends commands that exceed the host limits, the
> commands will be rejected by host HAB.
>
> This patch addresses this issue by getting the per-LUN queue limits via the the
> newly added virtio control request, then setting them properly.
>
> Signed-off-by: Cong Meng <mc@linux.vnet.ibm.com>
> ---
> drivers/scsi/virtio_scsi.c | 113 +++++++++++++++++++++++++++++++++++++------
> include/linux/virtio_scsi.h | 18 +++++++
> 2 files changed, 116 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 173cb39..ec5066f 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -35,12 +35,14 @@ struct virtio_scsi_cmd {
> struct virtio_scsi_cmd_req cmd;
> struct virtio_scsi_ctrl_tmf_req tmf;
> struct virtio_scsi_ctrl_an_req an;
> + struct virtio_scsi_ctrl_lq_req lq;
> } req;
> union {
> struct virtio_scsi_cmd_resp cmd;
> struct virtio_scsi_ctrl_tmf_resp tmf;
> struct virtio_scsi_ctrl_an_resp an;
> struct virtio_scsi_event evt;
> + struct virtio_scsi_ctrl_lq_resp lq;
> } resp;
> } ____cacheline_aligned_in_smp;
>
> @@ -469,6 +471,46 @@ out:
> return ret;
> }
>
> +static u32 virtscsi_lun_query(struct scsi_device *sdev, u32 *value, u32 subtype)
> +{
> + struct Scsi_Host *shost = sdev->host;
> + struct virtio_scsi *vscsi = shost_priv(shost);
> + DECLARE_COMPLETION_ONSTACK(comp);
> + struct virtio_scsi_cmd *cmd;
> + struct virtio_scsi_target_state *tgt = vscsi->tgt[sdev->id];
> + unsigned int ret = VIRTIO_SCSI_S_FAILURE;
> +
> + cmd = mempool_alloc(virtscsi_cmd_pool, GFP_ATOMIC);
> + if (!cmd)
> + goto out;
> +
> + memset(cmd, 0, sizeof(*cmd));
> + cmd->comp = ∁
> + cmd->req.lq = (struct virtio_scsi_ctrl_lq_req){
> + .type = VIRTIO_SCSI_T_LUN_QUERY,
> + .subtype = subtype,
> + .lun[0] = 1,
> + .lun[1] = sdev->id,
> + .lun[2] = (sdev->lun >> 8) | 0x40,
> + .lun[3] = sdev->lun & 0xff,
The LUN addressing code has been duplicated several times now. How
about replacing it with something like
static void virtio_scsi_set_lun(u8 *lun, struct scsi_device *sdev)
{
lun[0] = 1;
lun[1] = sdev->id;
lun[2] = (sdev->lun >> 8) | 0x40;
lun[3] = sdev->lun & 0xff;
lun[4] = lun[5] = lun[6] = lun[7] = 0;
}
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Cong Meng @ 2012-08-21 9:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: stefanha, zwanp, linuxram, qemu-devel, virtualization
In-Reply-To: <50334B51.6050900@redhat.com>
On Tue 21 Aug 2012 04:48:17 PM CST, Paolo Bonzini wrote:
> Il 21/08/2012 10:23, Cong Meng ha scritto:
>> +static void sg_get_queue_limits(BlockDriverState *bs, const char *filename)
>> +{
>> + DIR *ffs;
>> + struct dirent *d;
>> + char path[MAXPATHLEN];
>> +
>> + snprintf(path, MAXPATHLEN,
>> + "/sys/class/scsi_generic/sg%s/device/block/",
>> + filename + strlen("/dev/sg"));
>> +
>> + ffs = opendir(path);
>> + if (!ffs) {
>> + return;
>> + }
>> +
>> + for (;;) {
>> + d = readdir(ffs);
>> + if (!d) {
>> + return;
>> + }
>> +
>> + if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0) {
>> + continue;
>> + }
>> +
>> + break;
>> + }
>> +
>> + closedir(ffs);
>> +
>> + pstrcat(path, MAXPATHLEN, d->d_name);
>> + pstrcat(path, MAXPATHLEN, "/queue/");
>> +
>> + read_queue_limit(path, "max_sectors_kb", &bs->max_sectors);
>> + read_queue_limit(path, "max_segments", &bs->max_segments);
>> + read_queue_limit(path, "max_segment_size", &bs->max_segment_size);
>> +}
>
> Using /sys/dev/block or /sys/dev/char seems easier, and lets you
> retrieve the parameters for block devices too.
>
what do you mean with "block devices"? Using "/dev/sda" instead of
"/dev/sg0"?
> However, I'm worried of the consequences this has for migration. You
> could have the same physical disk accessed with two different HBAs, with
> different limits. So I don't know if this can really be solved at all.
>
I know little about qemu migration now. The pending scsi commands will
be saved and
transfered to remote machine when starting migration?
Cong.
> Paolo
>
^ permalink raw reply
* Re: [PATCH v1] virtio-scsi: get and set the queue limits for sg device
From: Cong Meng @ 2012-08-21 9:42 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: stefanha, zwanp, linuxram, qemu-devel, linux-kernel,
Paolo Bonzini, virtualization
In-Reply-To: <CAJSP0QXwFZdoDsizTozmXa2P=iRT7RQ_yQcVRC4FemiSpKG_bA@mail.gmail.com>
On Tue 21 Aug 2012 04:53:59 PM CST, Stefan Hajnoczi wrote:
> On Tue, Aug 21, 2012 at 9:26 AM, Cong Meng <mc@linux.vnet.ibm.com> wrote:
>> Each virtio scsi HBA has global request queue limits. But the passthrough
>> LUNs (scsi-generic) come from different host HBAs may have different request
>> queue limits. If the guest sends commands that exceed the host limits, the
>> commands will be rejected by host HAB.
>>
>> This patch addresses this issue by getting the per-LUN queue limits via the the
>> newly added virtio control request, then setting them properly.
>>
>> Signed-off-by: Cong Meng <mc@linux.vnet.ibm.com>
>> ---
>> drivers/scsi/virtio_scsi.c | 113 +++++++++++++++++++++++++++++++++++++------
>> include/linux/virtio_scsi.h | 18 +++++++
>> 2 files changed, 116 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
>> index 173cb39..ec5066f 100644
>> --- a/drivers/scsi/virtio_scsi.c
>> +++ b/drivers/scsi/virtio_scsi.c
>> @@ -35,12 +35,14 @@ struct virtio_scsi_cmd {
>> struct virtio_scsi_cmd_req cmd;
>> struct virtio_scsi_ctrl_tmf_req tmf;
>> struct virtio_scsi_ctrl_an_req an;
>> + struct virtio_scsi_ctrl_lq_req lq;
>> } req;
>> union {
>> struct virtio_scsi_cmd_resp cmd;
>> struct virtio_scsi_ctrl_tmf_resp tmf;
>> struct virtio_scsi_ctrl_an_resp an;
>> struct virtio_scsi_event evt;
>> + struct virtio_scsi_ctrl_lq_resp lq;
>> } resp;
>> } ____cacheline_aligned_in_smp;
>>
>> @@ -469,6 +471,46 @@ out:
>> return ret;
>> }
>>
>> +static u32 virtscsi_lun_query(struct scsi_device *sdev, u32 *value, u32 subtype)
>> +{
>> + struct Scsi_Host *shost = sdev->host;
>> + struct virtio_scsi *vscsi = shost_priv(shost);
>> + DECLARE_COMPLETION_ONSTACK(comp);
>> + struct virtio_scsi_cmd *cmd;
>> + struct virtio_scsi_target_state *tgt = vscsi->tgt[sdev->id];
>> + unsigned int ret = VIRTIO_SCSI_S_FAILURE;
>> +
>> + cmd = mempool_alloc(virtscsi_cmd_pool, GFP_ATOMIC);
>> + if (!cmd)
>> + goto out;
>> +
>> + memset(cmd, 0, sizeof(*cmd));
>> + cmd->comp = ∁
>> + cmd->req.lq = (struct virtio_scsi_ctrl_lq_req){
>> + .type = VIRTIO_SCSI_T_LUN_QUERY,
>> + .subtype = subtype,
>> + .lun[0] = 1,
>> + .lun[1] = sdev->id,
>> + .lun[2] = (sdev->lun >> 8) | 0x40,
>> + .lun[3] = sdev->lun & 0xff,
>
> The LUN addressing code has been duplicated several times now. How
> about replacing it with something like
sure. I will include it.
Thnaks.
Cong.
>
> static void virtio_scsi_set_lun(u8 *lun, struct scsi_device *sdev)
> {
> lun[0] = 1;
> lun[1] = sdev->id;
> lun[2] = (sdev->lun >> 8) | 0x40;
> lun[3] = sdev->lun & 0xff;
> lun[4] = lun[5] = lun[6] = lun[7] = 0;
> }
>
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Stefan Hajnoczi @ 2012-08-21 9:49 UTC (permalink / raw)
To: Cong Meng
Cc: stefanha, zwanp, linuxram, qemu-devel, virtualization,
Paolo Bonzini
In-Reply-To: <1345537427-21601-1-git-send-email-mc@linux.vnet.ibm.com>
On Tue, Aug 21, 2012 at 9:23 AM, Cong Meng <mc@linux.vnet.ibm.com> wrote:
> This patch adds some queue limit parameters into block drive. And inits them
> for sg block drive. Some interfaces are also added for accessing them.
>
> Signed-off-by: Cong Meng <mc@linux.vnet.ibm.com>
> ---
> block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> block_int.h | 4 +++
> blockdev.c | 15 +++++++++++++
> hw/block-common.h | 3 ++
> 4 files changed, 80 insertions(+), 0 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 0dce089..a086f89 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -53,6 +53,7 @@
> #include <linux/cdrom.h>
> #include <linux/fd.h>
> #include <linux/fs.h>
> +#include <dirent.h>
> #endif
> #ifdef CONFIG_FIEMAP
> #include <linux/fiemap.h>
> @@ -829,6 +830,62 @@ static int hdev_probe_device(const char *filename)
> return 0;
> }
>
> +static void read_queue_limit(char *path, const char *filename, unsigned int *val)
> +{
> + FILE *f;
> + char *tail = path + strlen(path);
> +
> + pstrcat(path, MAXPATHLEN, filename);
> + f = fopen(path, "r");
> + if (!f) {
> + goto out;
> + }
> +
> + fscanf(f, "%u", val);
> + fclose(f);
> +
> +out:
> + *tail = 0;
> +}
> +
> +static void sg_get_queue_limits(BlockDriverState *bs, const char *filename)
> +{
> + DIR *ffs;
> + struct dirent *d;
> + char path[MAXPATHLEN];
> +
> + snprintf(path, MAXPATHLEN,
> + "/sys/class/scsi_generic/sg%s/device/block/",
> + filename + strlen("/dev/sg"));
> +
> + ffs = opendir(path);
> + if (!ffs) {
> + return;
> + }
> +
> + for (;;) {
> + d = readdir(ffs);
> + if (!d) {
> + return;
Leaks ffs
> + }
> +
> + if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0) {
> + continue;
> + }
> +
> + break;
> + }
Not sure where in the kernel the block/ sysfs directory is created for
the device. I wasn't able to check that there is only ever 1
directory entry for a SCSI device's block/. Any ideas whether it's
safe to grab the first directory entry?
> +
> + closedir(ffs);
> +
> + pstrcat(path, MAXPATHLEN, d->d_name);
> + pstrcat(path, MAXPATHLEN, "/queue/");
> +
> + read_queue_limit(path, "max_sectors_kb", &bs->max_sectors);
> + read_queue_limit(path, "max_segments", &bs->max_segments);
> + read_queue_limit(path, "max_segment_size", &bs->max_segment_size);
> +}
> +
> static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
> {
> BDRVRawState *s = bs->opaque;
> @@ -868,6 +925,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
> temp = realpath(filename, resolved_path);
> if (temp && strstart(temp, "/dev/sg", NULL)) {
> bs->sg = 1;
> + sg_get_queue_limits(bs, temp);
> }
> }
> #endif
> diff --git a/block_int.h b/block_int.h
> index d72317f..a9d07a2 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -333,6 +333,10 @@ struct BlockDriverState {
>
> /* long-running background operation */
> BlockJob *job;
> +
> + unsigned int max_sectors;
> + unsigned int max_segments;
> + unsigned int max_segment_size;
> };
>
> int get_tmp_filename(char *filename, int size);
> diff --git a/blockdev.c b/blockdev.c
> index 3d75015..e17edbf 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1191,3 +1191,18 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
> bdrv_iterate(do_qmp_query_block_jobs_one, &prev);
> return dummy.next;
> }
> +
> +unsigned int get_queue_max_sectors(BlockDriverState *bs)
These should be bdrv_get_queue_max_sectors(BlockDriverState *bs) and
should live in block.c.
> +{
> + return (bs->file && bs->file->sg) ? bs->file->max_sectors : 0;
The BlockDriver should be able to provide these values, we shouldn't
reach into bs->file.
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Stefan Hajnoczi @ 2012-08-21 9:52 UTC (permalink / raw)
To: Cong Meng
Cc: stefanha, zwanp, linuxram, qemu-devel, virtualization,
Paolo Bonzini
In-Reply-To: <503357B2.5040901@linux.vnet.ibm.com>
On Tue, Aug 21, 2012 at 10:41 AM, Cong Meng <mc@linux.vnet.ibm.com> wrote:
>
>
> On Tue 21 Aug 2012 04:48:17 PM CST, Paolo Bonzini wrote:
>>
>> Il 21/08/2012 10:23, Cong Meng ha scritto:
>>>
>>> +static void sg_get_queue_limits(BlockDriverState *bs, const char
>>> *filename)
>>> +{
>>> + DIR *ffs;
>>> + struct dirent *d;
>>> + char path[MAXPATHLEN];
>>> +
>>> + snprintf(path, MAXPATHLEN,
>>> + "/sys/class/scsi_generic/sg%s/device/block/",
>>> + filename + strlen("/dev/sg"));
>>> +
>>> + ffs = opendir(path);
>>> + if (!ffs) {
>>> + return;
>>> + }
>>> +
>>> + for (;;) {
>>> + d = readdir(ffs);
>>> + if (!d) {
>>> + return;
>>> + }
>>> +
>>> + if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0)
>>> {
>>> + continue;
>>> + }
>>> +
>>> + break;
>>> + }
>>> +
>>> + closedir(ffs);
>>> +
>>> + pstrcat(path, MAXPATHLEN, d->d_name);
>>> + pstrcat(path, MAXPATHLEN, "/queue/");
>>> +
>>> + read_queue_limit(path, "max_sectors_kb", &bs->max_sectors);
>>> + read_queue_limit(path, "max_segments", &bs->max_segments);
>>> + read_queue_limit(path, "max_segment_size", &bs->max_segment_size);
>>> +}
>>
>>
>> Using /sys/dev/block or /sys/dev/char seems easier, and lets you
>> retrieve the parameters for block devices too.
>>
> what do you mean with "block devices"? Using "/dev/sda" instead of
> "/dev/sg0"?
>
>
>> However, I'm worried of the consequences this has for migration. You
>> could have the same physical disk accessed with two different HBAs, with
>> different limits. So I don't know if this can really be solved at all.
>>
> I know little about qemu migration now. The pending scsi commands will be
> saved and
> transfered to remote machine when starting migration?
Passthrough is already a migration blocker if both hosts do not have
access to the same LUNs.
When both hosts do have access to the same LUNs it's possible to
extract the block queue limits (using sysfs) and compare them.
Today you can start QEMU with different image files on both hosts.
Migration will appear to work but the disk image on the destination
host could be junk. This is a similar case, I don't see a problem
except that there should be a safety check (maybe at the libvirt
level) to make this safe.
Stefan
^ permalink raw reply
* Re: [PATCH 2/2 v1] virtio-scsi: set per-LUN queue limits for sg devices
From: Stefan Hajnoczi @ 2012-08-21 9:56 UTC (permalink / raw)
To: Cong Meng
Cc: stefanha, zwanp, linuxram, qemu-devel, virtualization,
Paolo Bonzini
In-Reply-To: <1345537427-21601-2-git-send-email-mc@linux.vnet.ibm.com>
On Tue, Aug 21, 2012 at 9:23 AM, Cong Meng <mc@linux.vnet.ibm.com> wrote:
> @@ -557,6 +621,7 @@ static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
> {
> requested_features |= (1UL << VIRTIO_SCSI_F_HOTPLUG);
> requested_features |= (1UL << VIRTIO_SCSI_F_CHANGE);
> + requested_features |= (1UL << VIRTIO_SCSI_F_LUN_QUERY);
This should be a QEMU 1.3 and later feature bit. VMs running with -m
pc-1.2 or earlier should not see it by default, this ensure the device
ABI is stable. For more info, see:
http://patchwork.ozlabs.org/patch/177924/
Stefan
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Paolo Bonzini @ 2012-08-21 10:14 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: stefanha, zwanp, linuxram, qemu-devel, virtualization, Cong Meng
In-Reply-To: <CAJSP0QX8BL9R_M0NxisiP+wejjgh6C6-nz9LPAfgHppPpNpYnA@mail.gmail.com>
Il 21/08/2012 11:52, Stefan Hajnoczi ha scritto:
>>> >> Using /sys/dev/block or /sys/dev/char seems easier, and lets you
>>> >> retrieve the parameters for block devices too.
>>> >>
>> > what do you mean with "block devices"? Using "/dev/sda" instead of
>> > "/dev/sg0"?
Yes.
>>> >> However, I'm worried of the consequences this has for migration. You
>>> >> could have the same physical disk accessed with two different HBAs, with
>>> >> different limits. So I don't know if this can really be solved at all.
>>> >>
>> > I know little about qemu migration now. The pending scsi commands will be
>> > saved and
>> > transfered to remote machine when starting migration?
>
> Passthrough is already a migration blocker if both hosts do not have
> access to the same LUNs.
Yes, but requiring the exact same hardware may be too much. I'm trying
to understand the problem better before committing to a threefold
spec/qemu/kernel change.
Cong, what is the limit that the host HBA enforces (and what is the
HBA)? What commands see a problem? Is it fixed by using scsi-block
instead of scsi-generic (if you can use scsi-block at all, i.e. it's not
a tape or similar device)?
With scsi-generic, QEMU uses a bounce buffer for non-I/O commands to a
SCSI passthrough device, so the only problem in that case should be the
maximum segment size. This could change in the future, but max_segments
and max_sectors should not yet be a problem.
With scsi-block, QEMU will use read/write on the block device and the
host kernel will then obey the host HBA's block limits. QEMU will still
use a bounce buffer for non-I/O commands to a scsi-block device, but the
payload is usually small for non-I/O commands.
Paolo
> When both hosts do have access to the same LUNs it's possible to
> extract the block queue limits (using sysfs) and compare them.
>
> Today you can start QEMU with different image files on both hosts.
> Migration will appear to work but the disk image on the destination
> host could be junk. This is a similar case, I don't see a problem
> except that there should be a safety check (maybe at the libvirt
> level) to make this safe.
^ permalink raw reply
* Re: Re: [PATCH V2 0/6] virtio-trace: Support virtio-trace
From: Masami Hiramatsu @ 2012-08-21 12:33 UTC (permalink / raw)
To: Amit Shah
Cc: Arnd Bergmann, qemu-devel, Frederic Weisbecker, linux-kernel,
Borislav Petkov, virtualization, Herbert Xu, Franch Ch. Eigler,
Ingo Molnar, Mathieu Desnoyers, Steven Rostedt, Anthony Liguori,
Greg Kroah-Hartman, yrl.pp-manager.tt
In-Reply-To: <20120821051619.GF29197@amit.redhat.com>
(2012/08/21 14:16), Amit Shah wrote:
> On (Thu) 09 Aug 2012 [21:30:29], Yoshihiro YUNOMAE wrote:
>> Hi All,
>>
>> The following patch set provides a low-overhead system for collecting kernel
>> tracing data of guests by a host in a virtualization environment.
>
> ACK this series.
Thank you!
> I ran it through the virtio-serial test suite, and there's no
> regression in existing functionality.
>
> I encourage you to check out the virtio-serial test suite[1] as well
> as the virtio-serial unit tests in the autotest[2] code, and
> contribute to them to add tests for the new functionality you've
> added.
Ah, OK we'll look into them for adding some tests for
splice support :)
>
> [1]
> http://fedorapeople.org/cgit/amitshah/public_git/test-virtserial.git/
>
> [2]
> https://github.com/autotest/autotest/blob/e91cd67b845b291622c8d079a8289c4b0cb1e6ae/client/tests/kvm/tests/virtio_console.py
>
>
Thank you again,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com
^ permalink raw reply
* [PATCH v8 0/5] make balloon pages movable by compaction
From: Rafael Aquini @ 2012-08-21 12:47 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, Minchan Kim,
Andi Kleen, Andrew Morton
Memory fragmentation introduced by ballooning might reduce significantly
the number of 2MB contiguous memory blocks that can be used within a guest,
thus imposing performance penalties associated with the reduced number of
transparent huge pages that could be used by the guest workload.
This patch-set follows the main idea discussed at 2012 LSFMMS session:
"Ballooning for transparent huge pages" -- http://lwn.net/Articles/490114/
to introduce the required changes to the virtio_balloon driver, as well as
the changes to the core compaction & migration bits, in order to make those
subsystems aware of ballooned pages and allow memory balloon pages become
movable within a guest, thus avoiding the aforementioned fragmentation issue
Rafael Aquini (5):
mm: introduce a common interface for balloon pages mobility
mm: introduce compaction and migration for ballooned pages
virtio_balloon: introduce migration primitives to balloon pages
mm: introduce putback_movable_pages()
mm: add vm event counters for balloon pages compaction
drivers/virtio/virtio_balloon.c | 212 +++++++++++++++++++++++++++++++++++--
include/linux/balloon_compaction.h | 113 ++++++++++++++++++++
include/linux/migrate.h | 2 +
include/linux/pagemap.h | 18 ++++
include/linux/vm_event_item.h | 8 +-
mm/Kconfig | 15 +++
mm/Makefile | 2 +-
mm/balloon_compaction.c | 147 +++++++++++++++++++++++++
mm/compaction.c | 51 +++++----
mm/migrate.c | 52 ++++++++-
mm/page_alloc.c | 2 +-
mm/vmstat.c | 10 +-
12 files changed, 595 insertions(+), 37 deletions(-)
create mode 100644 include/linux/balloon_compaction.h
create mode 100644 mm/balloon_compaction.c
Change log:
v8:
* introduce a common MM interface for balloon driver page compaction (Michael);
* remove the global state preventing multiple balloon device support (Michael);
* introduce RCU protection/syncrhonization to balloon page->mapping (Michael);
v7:
* fix a potential page leak case at 'putback_balloon_page' (Mel);
* adjust vm-events-counter patch and remove its drop-on-merge message (Rik);
* add 'putback_movable_pages' to avoid hacks on 'putback_lru_pages' (Minchan);
v6:
* rename 'is_balloon_page()' to 'movable_balloon_page()' (Rik);
v5:
* address Andrew Morton's review comments on the patch series;
* address a couple extra nitpick suggestions on PATCH 01 (Minchan);
v4:
* address Rusty Russel's review comments on PATCH 02;
* re-base virtio_balloon patch on 9c378abc5c0c6fc8e3acf5968924d274503819b3;
V3:
* address reviewers nitpick suggestions on PATCH 01 (Mel, Minchan);
V2:
* address Mel Gorman's review comments on PATCH 01;
Preliminary test results:
(2 VCPU 2048mB RAM KVM guest running 3.6.0_rc2+ -- after a reboot)
* 64mB balloon:
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 0
compact_pages_moved 0
compact_pagemigrate_failed 0
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_isolated 0
compact_balloon_migrated 0
compact_balloon_returned 0
compact_balloon_released 0
[root@localhost ~]#
[root@localhost ~]# for i in $(seq 1 6); do echo 1 > /proc/sys/vm/compact_memory & done &>/dev/null
[1] Done echo 1 > /proc/sys/vm/compact_memory
[2] Done echo 1 > /proc/sys/vm/compact_memory
[3] Done echo 1 > /proc/sys/vm/compact_memory
[4] Done echo 1 > /proc/sys/vm/compact_memory
[5]- Done echo 1 > /proc/sys/vm/compact_memory
[6]+ Done echo 1 > /proc/sys/vm/compact_memory
[root@localhost ~]#
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 3108
compact_pages_moved 43169
compact_pagemigrate_failed 95
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_isolated 16384
compact_balloon_migrated 16384
compact_balloon_returned 0
compact_balloon_released 16384
* 128 mB balloon:
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 0
compact_pages_moved 0
compact_pagemigrate_failed 0
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_isolated 0
compact_balloon_migrated 0
compact_balloon_returned 0
compact_balloon_released 0
[root@localhost ~]#
[root@localhost ~]# for i in $(seq 1 6); do echo 1 > /proc/sys/vm/compact_memory & done &>/dev/null
[1] Done echo 1 > /proc/sys/vm/compact_memory
[2] Done echo 1 > /proc/sys/vm/compact_memory
[3] Done echo 1 > /proc/sys/vm/compact_memory
[4] Done echo 1 > /proc/sys/vm/compact_memory
[5]- Done echo 1 > /proc/sys/vm/compact_memory
[6]+ Done echo 1 > /proc/sys/vm/compact_memory
[root@localhost ~]#
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 3062
compact_pages_moved 49774
compact_pagemigrate_failed 129
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_isolated 26076
compact_balloon_migrated 25957
compact_balloon_returned 119
compact_balloon_released 25957
--
1.7.11.4
^ permalink raw reply
* [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-21 12:47 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, Minchan Kim,
Andi Kleen, Andrew Morton
In-Reply-To: <cover.1345519422.git.aquini@redhat.com>
Memory fragmentation introduced by ballooning might reduce significantly
the number of 2MB contiguous memory blocks that can be used within a guest,
thus imposing performance penalties associated with the reduced number of
transparent huge pages that could be used by the guest workload.
This patch introduces a common interface to help a balloon driver on
making its page set movable to compaction, and thus allowing the system
to better leverage the compation efforts on memory defragmentation.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
include/linux/balloon_compaction.h | 113 +++++++++++++++++++++++++++++
include/linux/pagemap.h | 18 +++++
mm/Kconfig | 15 ++++
mm/Makefile | 2 +-
mm/balloon_compaction.c | 145 +++++++++++++++++++++++++++++++++++++
5 files changed, 292 insertions(+), 1 deletion(-)
create mode 100644 include/linux/balloon_compaction.h
create mode 100644 mm/balloon_compaction.c
diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
new file mode 100644
index 0000000..5fbf036
--- /dev/null
+++ b/include/linux/balloon_compaction.h
@@ -0,0 +1,113 @@
+/*
+ * include/linux/balloon_compaction.h
+ *
+ * Common interface definitions for making balloon pages movable to compaction.
+ *
+ * Copyright (C) 2012, Red Hat, Inc. Rafael Aquini <aquini@redhat.com>
+ */
+#ifndef _LINUX_BALLOON_COMPACTION_H
+#define _LINUX_BALLOON_COMPACTION_H
+#ifdef __KERNEL__
+
+#include <linux/rcupdate.h>
+#include <linux/pagemap.h>
+#include <linux/gfp.h>
+
+#if defined(CONFIG_BALLOON_COMPACTION)
+extern bool isolate_balloon_page(struct page *);
+extern void putback_balloon_page(struct page *);
+
+static inline bool movable_balloon_page(struct page *page)
+{
+ struct address_space *mapping = NULL;
+ bool ret = false;
+ /*
+ * Before dereferencing and testing mapping->flags, lets make sure
+ * this is not a page that uses ->mapping in a different way
+ */
+ if (!PageSlab(page) && !PageSwapCache(page) &&
+ !PageAnon(page) && !page_mapped(page)) {
+ rcu_read_lock();
+ mapping = rcu_dereference(page->mapping);
+ if (mapping_balloon(mapping))
+ ret = true;
+ rcu_read_unlock();
+ }
+ return ret;
+}
+
+static inline gfp_t balloon_mapping_gfp_mask(void)
+{
+ return GFP_HIGHUSER_MOVABLE;
+}
+
+/*
+ * __page_balloon_device - return the balloon device owing the page.
+ *
+ * This shall only be used at driver callbacks under proper page_lock,
+ * to get access to the balloon device structure that owns @page.
+ */
+static inline void *__page_balloon_device(struct page *page)
+{
+ struct address_space *mapping;
+ mapping = rcu_access_pointer(page->mapping);
+ if (mapping)
+ mapping = mapping->assoc_mapping;
+ return (void *)mapping;
+}
+
+#define count_balloon_event(e) count_vm_event(e)
+/*
+ * DEFINE_BALLOON_MAPPING_AOPS - declare and instantiate a callback descriptor
+ * to be used as balloon page->mapping->a_ops.
+ *
+ * @label : declaration identifier (var name)
+ * @migratepg : callback symbol name for performing the page migration step
+ * @isolatepg : callback symbol name for performing the page isolation step
+ * @putbackpg : callback symbol name for performing the page putback step
+ *
+ * address_space_operations utilized methods for ballooned pages:
+ * .migratepage - used to perform balloon's page migration (as is)
+ * .launder_page - used to isolate a page from balloon's page list
+ * .freepage - used to reinsert an isolated page to balloon's page list
+ */
+#define DEFINE_BALLOON_MAPPING_AOPS(label, migratepg, isolatepg, putbackpg) \
+ const struct address_space_operations (label) = { \
+ .migratepage = (migratepg), \
+ .launder_page = (isolatepg), \
+ .freepage = (putbackpg), \
+ }
+
+#else
+static inline bool isolate_balloon_page(struct page *page) { return false; }
+static inline bool movable_balloon_page(struct page *page) { return false; }
+static inline void putback_balloon_page(struct page *page) { return; }
+
+static inline gfp_t balloon_mapping_gfp_mask(void)
+{
+ return GFP_HIGHUSER;
+}
+
+#define count_balloon_event(e) {}
+#define DEFINE_BALLOON_MAPPING_AOPS(label, migratepg, isolatepg, putbackpg) \
+ const struct address_space_operations *(label) = NULL
+
+#endif /* CONFIG_BALLOON_COMPACTION */
+
+extern struct address_space *alloc_balloon_mapping(void *balloon_device,
+ const struct address_space_operations *a_ops);
+extern void *page_balloon_device(struct page *page);
+
+static inline void assign_balloon_mapping(struct page *page,
+ struct address_space *mapping)
+{
+ rcu_assign_pointer(page->mapping, mapping);
+}
+
+static inline void clear_balloon_mapping(struct page *page)
+{
+ rcu_assign_pointer(page->mapping, NULL);
+}
+
+#endif /* __KERNEL__ */
+#endif /* _LINUX_BALLOON_COMPACTION_H */
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index e42c762..6df0664 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -24,6 +24,7 @@ enum mapping_flags {
AS_ENOSPC = __GFP_BITS_SHIFT + 1, /* ENOSPC on async write */
AS_MM_ALL_LOCKS = __GFP_BITS_SHIFT + 2, /* under mm_take_all_locks() */
AS_UNEVICTABLE = __GFP_BITS_SHIFT + 3, /* e.g., ramdisk, SHM_LOCK */
+ AS_BALLOON_MAP = __GFP_BITS_SHIFT + 4, /* balloon page special map */
};
static inline void mapping_set_error(struct address_space *mapping, int error)
@@ -53,6 +54,23 @@ static inline int mapping_unevictable(struct address_space *mapping)
return !!mapping;
}
+static inline void mapping_set_balloon(struct address_space *mapping)
+{
+ set_bit(AS_BALLOON_MAP, &mapping->flags);
+}
+
+static inline void mapping_clear_balloon(struct address_space *mapping)
+{
+ clear_bit(AS_BALLOON_MAP, &mapping->flags);
+}
+
+static inline int mapping_balloon(struct address_space *mapping)
+{
+ if (mapping)
+ return test_bit(AS_BALLOON_MAP, &mapping->flags);
+ return !!mapping;
+}
+
static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
{
return (__force gfp_t)mapping->flags & __GFP_BITS_MASK;
diff --git a/mm/Kconfig b/mm/Kconfig
index d5c8019..4857899 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -188,6 +188,21 @@ config SPLIT_PTLOCK_CPUS
default "4"
#
+# support for memory balloon compaction
+config BALLOON_COMPACTION
+ bool "Allow for balloon memory compaction/migration"
+ select COMPACTION
+ depends on VIRTIO_BALLOON
+ help
+ Memory fragmentation introduced by ballooning might reduce
+ significantly the number of 2MB contiguous memory blocks that can be
+ used within a guest, thus imposing performance penalties associated
+ with the reduced number of transparent huge pages that could be used
+ by the guest workload. Allowing the compaction & migration for memory
+ pages enlisted as being part of memory balloon devices avoids the
+ scenario aforementioned and helps improving memory defragmentation.
+
+#
# support for memory compaction
config COMPACTION
bool "Allow for memory compaction"
diff --git a/mm/Makefile b/mm/Makefile
index 92753e2..78d8caa 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -16,7 +16,7 @@ obj-y := filemap.o mempool.o oom_kill.o fadvise.o \
readahead.o swap.o truncate.o vmscan.o shmem.o \
prio_tree.o util.o mmzone.o vmstat.o backing-dev.o \
mm_init.o mmu_context.o percpu.o slab_common.o \
- compaction.o $(mmu-y)
+ compaction.o balloon_compaction.o $(mmu-y)
obj-y += init-mm.o
diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
new file mode 100644
index 0000000..d79f13d
--- /dev/null
+++ b/mm/balloon_compaction.c
@@ -0,0 +1,145 @@
+/*
+ * mm/balloon_compaction.c
+ *
+ * Common interface for making balloon pages movable to compaction.
+ *
+ * Copyright (C) 2012, Red Hat, Inc. Rafael Aquini <aquini@redhat.com>
+ */
+#include <linux/slab.h>
+#include <linux/export.h>
+#include <linux/balloon_compaction.h>
+
+/*
+ * alloc_balloon_mapping - allocates a special ->mapping for ballooned pages.
+ * @balloon_device: pointer address that references the balloon device which
+ * owns pages bearing this ->mapping.
+ * @a_ops: balloon_mapping address_space_operations descriptor.
+ *
+ * Users must call it to properly allocate and initialize an instance of
+ * struct address_space which will be used as the special page->mapping for
+ * balloon devices enlisted page instances.
+ */
+struct address_space *alloc_balloon_mapping(void *balloon_device,
+ const struct address_space_operations *a_ops)
+{
+ struct address_space *mapping;
+
+ mapping = kmalloc(sizeof(*mapping), GFP_KERNEL);
+ if (mapping) {
+ /*
+ * Give a clean 'zeroed' status to all elements of this special
+ * balloon page->mapping struct address_space instance.
+ */
+ address_space_init_once(mapping);
+
+ /*
+ * Set mapping->flags appropriately, to allow balloon ->mapping
+ * identification, as well as give a proper hint to the balloon
+ * driver on what GFP allocation mask shall be used.
+ */
+ mapping_set_balloon(mapping);
+ mapping_set_gfp_mask(mapping, balloon_mapping_gfp_mask());
+
+ /* balloon's page->mapping->a_ops callback descriptor */
+ mapping->a_ops = a_ops;
+
+ /*
+ * balloon special page->mapping overloads ->assoc_mapping
+ * to held a reference back to the balloon device wich 'owns'
+ * a given page. This is the way we can cope with multiple
+ * balloon devices without losing reference of several
+ * ballooned pagesets.
+ */
+ mapping->assoc_mapping = (void *)balloon_device;
+
+ return mapping;
+ }
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(alloc_balloon_mapping);
+
+#if defined(CONFIG_BALLOON_COMPACTION)
+static inline int __isolate_balloon_page(struct page *page)
+{
+ struct address_space *mapping;
+ int ret = 0;
+
+ rcu_read_lock();
+ mapping = rcu_dereference(page->mapping);
+ if (mapping)
+ ret = mapping->a_ops->launder_page(page);
+
+ rcu_read_unlock();
+ return ret;
+}
+
+static inline void __putback_balloon_page(struct page *page)
+{
+ struct address_space *mapping;
+
+ rcu_read_lock();
+ mapping = rcu_dereference(page->mapping);
+ /*
+ * If we stumble across a page->mapping NULL here, so something has
+ * messed with the private isolated pageset we are iterating over.
+ */
+ BUG_ON(!mapping);
+
+ mapping->a_ops->freepage(page);
+ /* isolation bumps up the page refcount, time to decrement it */
+ put_page(page);
+ rcu_read_unlock();
+}
+
+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+ if (likely(get_page_unless_zero(page))) {
+ /*
+ * As balloon pages are not isolated from LRU lists, concurrent
+ * compaction threads can race against page migration functions
+ * move_to_new_page() & __unmap_and_move().
+ * In order to avoid having an already isolated balloon page
+ * being (wrongly) re-isolated while it is under migration,
+ * lets be sure we have the page lock before proceeding with
+ * the balloon page isolation steps.
+ */
+ if (likely(trylock_page(page))) {
+ /*
+ * A ballooned page, by default, has just one refcount.
+ * Prevent concurrent compaction threads from isolating
+ * an already isolated balloon page by refcount check.
+ */
+ if (movable_balloon_page(page) &&
+ (page_count(page) == 2)) {
+ if (__isolate_balloon_page(page)) {
+ unlock_page(page);
+ return true;
+ }
+ }
+ unlock_page(page);
+ }
+ /*
+ * This page is either under migration, it is isolated already,
+ * or its isolation step has not happened at this round.
+ * Drop the refcount taken for it.
+ */
+ put_page(page);
+ }
+ return false;
+}
+
+/* putback_lru_page() counterpart for a ballooned page */
+void putback_balloon_page(struct page *page)
+{
+ /*
+ * 'lock_page()' stabilizes the page and prevents races against
+ * concurrent isolation threads attempting to re-isolate it.
+ */
+ lock_page(page);
+ if (movable_balloon_page(page))
+ __putback_balloon_page(page);
+
+ unlock_page(page);
+}
+#endif /* CONFIG_BALLOON_COMPACTION */
--
1.7.11.4
^ permalink raw reply related
* [PATCH v8 2/5] mm: introduce compaction and migration for ballooned pages
From: Rafael Aquini @ 2012-08-21 12:47 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, Minchan Kim,
Andi Kleen, Andrew Morton
In-Reply-To: <cover.1345519422.git.aquini@redhat.com>
Memory fragmentation introduced by ballooning might reduce significantly
the number of 2MB contiguous memory blocks that can be used within a guest,
thus imposing performance penalties associated with the reduced number of
transparent huge pages that could be used by the guest workload.
This patch introduces the helper functions as well as the necessary changes
to teach compaction and migration bits how to cope with pages which are
part of a guest memory balloon, in order to make them movable by memory
compaction procedures.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
mm/compaction.c | 47 ++++++++++++++++++++++++++++-------------------
mm/migrate.c | 31 ++++++++++++++++++++++++++++++-
2 files changed, 58 insertions(+), 20 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index e78cb96..ce43dc2 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -14,6 +14,7 @@
#include <linux/backing-dev.h>
#include <linux/sysctl.h>
#include <linux/sysfs.h>
+#include <linux/balloon_compaction.h>
#include "internal.h"
#if defined CONFIG_COMPACTION || defined CONFIG_CMA
@@ -312,32 +313,40 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
continue;
}
- if (!PageLRU(page))
- continue;
-
/*
- * PageLRU is set, and lru_lock excludes isolation,
- * splitting and collapsing (collapsing has already
- * happened if PageLRU is set).
+ * It is possible to migrate LRU pages and balloon pages.
+ * Skip any other type of page.
*/
- if (PageTransHuge(page)) {
- low_pfn += (1 << compound_order(page)) - 1;
- continue;
- }
+ if (PageLRU(page)) {
+ /*
+ * PageLRU is set, and lru_lock excludes isolation,
+ * splitting and collapsing (collapsing has already
+ * happened if PageLRU is set).
+ */
+ if (PageTransHuge(page)) {
+ low_pfn += (1 << compound_order(page)) - 1;
+ continue;
+ }
- if (!cc->sync)
- mode |= ISOLATE_ASYNC_MIGRATE;
+ if (!cc->sync)
+ mode |= ISOLATE_ASYNC_MIGRATE;
- lruvec = mem_cgroup_page_lruvec(page, zone);
+ lruvec = mem_cgroup_page_lruvec(page, zone);
- /* Try isolate the page */
- if (__isolate_lru_page(page, mode) != 0)
- continue;
+ /* Try isolate the page */
+ if (__isolate_lru_page(page, mode) != 0)
+ continue;
- VM_BUG_ON(PageTransCompound(page));
+ VM_BUG_ON(PageTransCompound(page));
+
+ /* Successfully isolated */
+ del_page_from_lru_list(page, lruvec, page_lru(page));
+ } else if (unlikely(movable_balloon_page(page))) {
+ if (!isolate_balloon_page(page))
+ continue;
+ } else
+ continue;
- /* Successfully isolated */
- del_page_from_lru_list(page, lruvec, page_lru(page));
list_add(&page->lru, migratelist);
cc->nr_migratepages++;
nr_isolated++;
diff --git a/mm/migrate.c b/mm/migrate.c
index 77ed2d7..6392da258 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -35,6 +35,7 @@
#include <linux/hugetlb.h>
#include <linux/hugetlb_cgroup.h>
#include <linux/gfp.h>
+#include <linux/balloon_compaction.h>
#include <asm/tlbflush.h>
@@ -79,7 +80,10 @@ void putback_lru_pages(struct list_head *l)
list_del(&page->lru);
dec_zone_page_state(page, NR_ISOLATED_ANON +
page_is_file_cache(page));
- putback_lru_page(page);
+ if (unlikely(movable_balloon_page(page)))
+ putback_balloon_page(page);
+ else
+ putback_lru_page(page);
}
}
@@ -778,6 +782,17 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
}
}
+ if (unlikely(movable_balloon_page(page))) {
+ /*
+ * A ballooned page does not need any special attention from
+ * physical to virtual reverse mapping procedures.
+ * Skip any attempt to unmap PTEs or to remap swap cache,
+ * in order to avoid burning cycles at rmap level.
+ */
+ remap_swapcache = 0;
+ goto skip_unmap;
+ }
+
/*
* Corner case handling:
* 1. When a new swap-cache page is read into, it is added to the LRU
@@ -846,6 +861,20 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
goto out;
rc = __unmap_and_move(page, newpage, force, offlining, mode);
+
+ if (unlikely(movable_balloon_page(newpage))) {
+ /*
+ * A ballooned page has been migrated already. Now, it is the
+ * time to wrap-up counters, handle the old page back to Buddy
+ * and return.
+ */
+ list_del(&page->lru);
+ dec_zone_page_state(page, NR_ISOLATED_ANON +
+ page_is_file_cache(page));
+ put_page(page);
+ __free_page(page);
+ return rc;
+ }
out:
if (rc != -EAGAIN) {
/*
--
1.7.11.4
^ permalink raw reply related
* [PATCH v8 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-08-21 12:47 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, Minchan Kim,
Andi Kleen, Andrew Morton
In-Reply-To: <cover.1345519422.git.aquini@redhat.com>
Memory fragmentation introduced by ballooning might reduce significantly
the number of 2MB contiguous memory blocks that can be used within a guest,
thus imposing performance penalties associated with the reduced number of
transparent huge pages that could be used by the guest workload.
Besides making balloon pages movable at allocation time and introducing
the necessary primitives to perform balloon page migration/compaction,
this patch also introduces the following locking scheme to provide the
proper synchronization and protection for struct virtio_balloon elements
against concurrent accesses due to parallel operations introduced by
memory compaction / page migration.
- balloon_lock (mutex) : synchronizes the access demand to elements of
struct virtio_balloon and its queue operations;
- pages_lock (spinlock): special protection to balloon pages list against
concurrent list handling operations;
- virtio_baloon->pages list handling sync by RCU operations;
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
drivers/virtio/virtio_balloon.c | 210 +++++++++++++++++++++++++++++++++++++---
1 file changed, 199 insertions(+), 11 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0908e60..bda7bb0 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -27,6 +27,7 @@
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/module.h>
+#include <linux/balloon_compaction.h>
/*
* Balloon device works in 4K page units. So each page is pointed to by
@@ -35,6 +36,12 @@
*/
#define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
+/* flags used to hint compaction procedures about the balloon device status */
+enum balloon_status_flags {
+ BALLOON_REMOVAL = 0, /* balloon device is under removal steps */
+ BALLOON_OK, /* balloon device is up and running */
+};
+
struct virtio_balloon
{
struct virtio_device *vdev;
@@ -46,11 +53,24 @@ struct virtio_balloon
/* The thread servicing the balloon. */
struct task_struct *thread;
+ /* balloon special page->mapping */
+ struct address_space *mapping;
+
+ /* Synchronize access/update to this struct virtio_balloon elements */
+ struct mutex balloon_lock;
+
/* Waiting for host to ack the pages we released. */
wait_queue_head_t acked;
/* Number of balloon pages we've told the Host we're not using. */
unsigned int num_pages;
+
+ /* balloon device status flag */
+ unsigned short balloon_status;
+
+ /* Protect 'pages' list against concurrent handling */
+ spinlock_t pages_lock;
+
/*
* The pages we've told the Host we're not using.
* Each page on this list adds VIRTIO_BALLOON_PAGES_PER_PAGE
@@ -122,13 +142,17 @@ static void set_page_pfns(u32 pfns[], struct page *page)
static void fill_balloon(struct virtio_balloon *vb, size_t num)
{
+ /* Get the proper GFP alloc mask from vb->mapping flags */
+ gfp_t vb_gfp_mask = mapping_gfp_mask(vb->mapping);
+
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
+ mutex_lock(&vb->balloon_lock);
for (vb->num_pfns = 0; vb->num_pfns < num;
vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
- struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
- __GFP_NOMEMALLOC | __GFP_NOWARN);
+ struct page *page = alloc_page(vb_gfp_mask | __GFP_NORETRY |
+ __GFP_NOWARN | __GFP_NOMEMALLOC);
if (!page) {
if (printk_ratelimit())
dev_printk(KERN_INFO, &vb->vdev->dev,
@@ -141,7 +165,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
totalram_pages--;
- list_add(&page->lru, &vb->pages);
+ spin_lock(&vb->pages_lock);
+ list_add_rcu(&page->lru, &vb->pages);
+ assign_balloon_mapping(page, vb->mapping);
+ spin_unlock(&vb->pages_lock);
}
/* Didn't get any? Oh well. */
@@ -149,6 +176,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
return;
tell_host(vb, vb->inflate_vq);
+ mutex_unlock(&vb->balloon_lock);
}
static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
@@ -169,21 +197,48 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
+ mutex_lock(&vb->balloon_lock);
for (vb->num_pfns = 0; vb->num_pfns < num;
vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
- page = list_first_entry(&vb->pages, struct page, lru);
- list_del(&page->lru);
+ /*
+ * We can race against 'virtballoon_isolatepage()' and end up
+ * stumbling across a _temporarily_ empty 'pages' list.
+ */
+ spin_lock(&vb->pages_lock);
+ page = list_first_or_null_rcu(&vb->pages, struct page, lru);
+ if (!page) {
+ spin_unlock(&vb->pages_lock);
+ break;
+ }
+ /*
+ * It is safe now to drop page->mapping and delete this page
+ * from balloon page list, since we are grabbing 'pages_lock'
+ * which prevents 'virtballoon_isolatepage()' from acting.
+ */
+ clear_balloon_mapping(page);
+ list_del_rcu(&page->lru);
+ spin_unlock(&vb->pages_lock);
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
}
+ /*
+ * Syncrhonize RCU grace period and wait for all RCU read critical side
+ * sections to finish before proceeding with page release steps.
+ * This avoids compaction/migration callback races against balloon
+ * device removal steps.
+ */
+ synchronize_rcu();
/*
* Note that if
* virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
* is true, we *have* to do it in this order
*/
- tell_host(vb, vb->deflate_vq);
- release_pages_by_pfn(vb->pfns, vb->num_pfns);
+ if (vb->num_pfns > 0) {
+ tell_host(vb, vb->deflate_vq);
+ release_pages_by_pfn(vb->pfns, vb->num_pfns);
+ }
+ mutex_unlock(&vb->balloon_lock);
}
static inline void update_stat(struct virtio_balloon *vb, int idx,
@@ -239,6 +294,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
struct scatterlist sg;
unsigned int len;
+ mutex_lock(&vb->balloon_lock);
vb->need_stats_update = 0;
update_balloon_stats(vb);
@@ -249,6 +305,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
BUG();
virtqueue_kick(vq);
+ mutex_unlock(&vb->balloon_lock);
}
static void virtballoon_changed(struct virtio_device *vdev)
@@ -261,22 +318,27 @@ static void virtballoon_changed(struct virtio_device *vdev)
static inline s64 towards_target(struct virtio_balloon *vb)
{
__le32 v;
- s64 target;
+ s64 target, actual;
+ mutex_lock(&vb->balloon_lock);
+ actual = vb->num_pages;
vb->vdev->config->get(vb->vdev,
offsetof(struct virtio_balloon_config, num_pages),
&v, sizeof(v));
target = le32_to_cpu(v);
- return target - vb->num_pages;
+ mutex_unlock(&vb->balloon_lock);
+ return target - actual;
}
static void update_balloon_size(struct virtio_balloon *vb)
{
- __le32 actual = cpu_to_le32(vb->num_pages);
-
+ __le32 actual;
+ mutex_lock(&vb->balloon_lock);
+ actual = cpu_to_le32(vb->num_pages);
vb->vdev->config->set(vb->vdev,
offsetof(struct virtio_balloon_config, actual),
&actual, sizeof(actual));
+ mutex_unlock(&vb->balloon_lock);
}
static int balloon(void *_vballoon)
@@ -339,9 +401,117 @@ static int init_vqs(struct virtio_balloon *vb)
return 0;
}
+#ifdef CONFIG_BALLOON_COMPACTION
+/*
+ * Populate balloon_mapping->a_ops callback method to perform the balloon
+ * page migration task.
+ *
+ * After a ballooned page gets isolated by compaction procedures, this is the
+ * function that performs the page migration on behalf of move_to_new_page(),
+ * when the last calls (page)->mapping->a_ops->migratepage.
+ *
+ * Page migration for virtio balloon is done in a simple swap fashion which
+ * follows these two steps:
+ * 1) insert newpage into vb->pages list and update the host about it;
+ * 2) update the host about the removed old page from vb->pages list;
+ */
+int virtballoon_migratepage(struct address_space *mapping,
+ struct page *newpage, struct page *page, enum migrate_mode mode)
+{
+ struct virtio_balloon *vb = (void *)__page_balloon_device(page);
+
+ /* at this point, besides very unlikely, a NULL *vb is a serious bug */
+ BUG_ON(!vb);
+
+ /*
+ * Skip page migration step if the memory balloon device is under its
+ * removal procedure, to avoid racing against module unload.
+ *
+ * If there still are isolated (balloon) pages under migration lists,
+ * 'virtballoon_putbackpage()' will take care of them properly, before
+ * the module unload finishes.
+ */
+ if (vb->balloon_status == BALLOON_REMOVAL)
+ return -EAGAIN;
+
+ mutex_lock(&vb->balloon_lock);
+
+ /* balloon's page migration 1st step */
+ vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+ spin_lock(&vb->pages_lock);
+ list_add_rcu(&newpage->lru, &vb->pages);
+ spin_unlock(&vb->pages_lock);
+ set_page_pfns(vb->pfns, newpage);
+ tell_host(vb, vb->inflate_vq);
+
+ /* balloon's page migration 2nd step */
+ vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+ set_page_pfns(vb->pfns, page);
+ tell_host(vb, vb->deflate_vq);
+
+ mutex_unlock(&vb->balloon_lock);
+ return 0;
+}
+
+/*
+ * Populate balloon_mapping->a_ops callback method to help compaction on
+ * isolating a page from the balloon page list for posterior migration.
+ */
+int virtballoon_isolatepage(struct page *page)
+{
+ struct virtio_balloon *vb = (void *)__page_balloon_device(page);
+ int ret = 0;
+ /*
+ * If we stumble across a NULL *vb here, it means this page has been
+ * already released by 'leak_balloon()'.
+ *
+ * We also skip the page isolation step if the memory balloon device is
+ * under its removal procedure, to avoid racing against module unload.
+ */
+ if (vb && (vb->balloon_status != BALLOON_REMOVAL)) {
+ spin_lock(&vb->pages_lock);
+ /*
+ * virtballoon_isolatepage() can race against leak_balloon(),
+ * and (wrongly) isolate a page that is about to be freed.
+ * Test page->mapping under pages_lock to close that window.
+ */
+ if (rcu_access_pointer(page->mapping) == vb->mapping) {
+ /* It is safe to isolate this page, now */
+ list_del_rcu(&page->lru);
+ ret = 1;
+ }
+ spin_unlock(&vb->pages_lock);
+ }
+ return ret;
+}
+
+/*
+ * Populate balloon_mapping->a_ops callback method to help compaction on
+ * re-inserting a not-migrated isolated page into the balloon page list.
+ */
+void virtballoon_putbackpage(struct page *page)
+{
+ struct virtio_balloon *vb = (void *)__page_balloon_device(page);
+
+ /* at this point, besides very unlikely, a NULL *vb is a serious bug */
+ BUG_ON(!vb);
+
+ spin_lock(&vb->pages_lock);
+ list_add_rcu(&page->lru, &vb->pages);
+ spin_unlock(&vb->pages_lock);
+}
+#endif /* CONFIG_BALLOON_COMPACTION */
+
+/* define the balloon_mapping->a_ops callbacks to allow compaction/migration */
+static DEFINE_BALLOON_MAPPING_AOPS(virtio_balloon_aops,
+ virtballoon_migratepage,
+ virtballoon_isolatepage,
+ virtballoon_putbackpage);
+
static int virtballoon_probe(struct virtio_device *vdev)
{
struct virtio_balloon *vb;
+ struct address_space *vb_mapping;
int err;
vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
@@ -351,12 +521,24 @@ static int virtballoon_probe(struct virtio_device *vdev)
}
INIT_LIST_HEAD(&vb->pages);
+ spin_lock_init(&vb->pages_lock);
+ mutex_init(&vb->balloon_lock);
+
vb->num_pages = 0;
init_waitqueue_head(&vb->config_change);
init_waitqueue_head(&vb->acked);
vb->vdev = vdev;
vb->need_stats_update = 0;
+ /* Allocate a special page->mapping for this balloon device */
+ vb_mapping = alloc_balloon_mapping((void *)vb, &virtio_balloon_aops);
+ if (!vb_mapping) {
+ err = -ENOMEM;
+ goto out_free_vb;
+ }
+ /* Store the page->mapping reference for this balloon device */
+ vb->mapping = vb_mapping;
+
err = init_vqs(vb);
if (err)
goto out_free_vb;
@@ -367,12 +549,14 @@ static int virtballoon_probe(struct virtio_device *vdev)
goto out_del_vqs;
}
+ vb->balloon_status = BALLOON_OK;
return 0;
out_del_vqs:
vdev->config->del_vqs(vdev);
out_free_vb:
kfree(vb);
+ kfree(vb_mapping);
out:
return err;
}
@@ -394,8 +578,10 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
{
struct virtio_balloon *vb = vdev->priv;
+ vb->balloon_status = BALLOON_REMOVAL;
kthread_stop(vb->thread);
remove_common(vb);
+ kfree(vb->mapping);
kfree(vb);
}
@@ -409,6 +595,7 @@ static int virtballoon_freeze(struct virtio_device *vdev)
* function is called.
*/
+ vb->balloon_status = BALLOON_REMOVAL;
remove_common(vb);
return 0;
}
@@ -424,6 +611,7 @@ static int virtballoon_restore(struct virtio_device *vdev)
fill_balloon(vb, towards_target(vb));
update_balloon_size(vb);
+ vb->balloon_status = BALLOON_OK;
return 0;
}
#endif
--
1.7.11.4
^ permalink raw reply related
* [PATCH v8 4/5] mm: introduce putback_movable_pages()
From: Rafael Aquini @ 2012-08-21 12:47 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, Minchan Kim,
Andi Kleen, Andrew Morton
In-Reply-To: <cover.1345519422.git.aquini@redhat.com>
The PATCH "mm: introduce compaction and migration for virtio ballooned pages"
hacks around putback_lru_pages() in order to allow ballooned pages to be
re-inserted on balloon page list as if a ballooned page was like a LRU page.
As ballooned pages are not legitimate LRU pages, this patch introduces
putback_movable_pages() to properly cope with cases where the isolated
pageset contains ballooned pages and LRU pages, thus fixing the mentioned
inelegant hack around putback_lru_pages().
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
include/linux/migrate.h | 2 ++
mm/compaction.c | 4 ++--
mm/migrate.c | 20 ++++++++++++++++++++
mm/page_alloc.c | 2 +-
4 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index ce7e667..ff103a1 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -10,6 +10,7 @@ typedef struct page *new_page_t(struct page *, unsigned long private, int **);
#ifdef CONFIG_MIGRATION
extern void putback_lru_pages(struct list_head *l);
+extern void putback_movable_pages(struct list_head *l);
extern int migrate_page(struct address_space *,
struct page *, struct page *, enum migrate_mode);
extern int migrate_pages(struct list_head *l, new_page_t x,
@@ -33,6 +34,7 @@ extern int migrate_huge_page_move_mapping(struct address_space *mapping,
#else
static inline void putback_lru_pages(struct list_head *l) {}
+static inline void putback_movable_pages(struct list_head *l) {}
static inline int migrate_pages(struct list_head *l, new_page_t x,
unsigned long private, bool offlining,
enum migrate_mode mode) { return -ENOSYS; }
diff --git a/mm/compaction.c b/mm/compaction.c
index ce43dc2..782ed32 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -759,9 +759,9 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
trace_mm_compaction_migratepages(nr_migrate - nr_remaining,
nr_remaining);
- /* Release LRU pages not migrated */
+ /* Release isolated pages not migrated */
if (err) {
- putback_lru_pages(&cc->migratepages);
+ putback_movable_pages(&cc->migratepages);
cc->nr_migratepages = 0;
if (err == -ENOMEM) {
ret = COMPACT_PARTIAL;
diff --git a/mm/migrate.c b/mm/migrate.c
index 6392da258..0bf2caf 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -80,6 +80,26 @@ void putback_lru_pages(struct list_head *l)
list_del(&page->lru);
dec_zone_page_state(page, NR_ISOLATED_ANON +
page_is_file_cache(page));
+ putback_lru_page(page);
+ }
+}
+
+/*
+ * Put previously isolated pages back onto the appropriated lists
+ * from where they were once taken off for compaction/migration.
+ *
+ * This function shall be used instead of putback_lru_pages(),
+ * whenever the isolated pageset has been built by isolate_migratepages_range()
+ */
+void putback_movable_pages(struct list_head *l)
+{
+ struct page *page;
+ struct page *page2;
+
+ list_for_each_entry_safe(page, page2, l, lru) {
+ list_del(&page->lru);
+ dec_zone_page_state(page, NR_ISOLATED_ANON +
+ page_is_file_cache(page));
if (unlikely(movable_balloon_page(page)))
putback_balloon_page(page);
else
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 009ac28..78b7663 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5669,7 +5669,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
0, false, MIGRATE_SYNC);
}
- putback_lru_pages(&cc.migratepages);
+ putback_movable_pages(&cc.migratepages);
return ret > 0 ? 0 : ret;
}
--
1.7.11.4
^ permalink raw reply related
* [PATCH v8 5/5] mm: add vm event counters for balloon pages compaction
From: Rafael Aquini @ 2012-08-21 12:47 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, Minchan Kim,
Andi Kleen, Andrew Morton
In-Reply-To: <cover.1345519422.git.aquini@redhat.com>
This patch introduces a new set of vm event counters to keep track of
ballooned pages compaction activity.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
drivers/virtio/virtio_balloon.c | 2 ++
include/linux/vm_event_item.h | 8 +++++++-
mm/balloon_compaction.c | 6 ++++--
mm/migrate.c | 1 +
mm/vmstat.c | 10 +++++++++-
5 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index bda7bb0..c358ed3 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -449,6 +449,8 @@ int virtballoon_migratepage(struct address_space *mapping,
set_page_pfns(vb->pfns, page);
tell_host(vb, vb->deflate_vq);
+ /* perform vm accountability on this successful page migration */
+ count_balloon_event(COMPACTBALLOONMIGRATED);
mutex_unlock(&vb->balloon_lock);
return 0;
}
diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
index 57f7b10..6868aba 100644
--- a/include/linux/vm_event_item.h
+++ b/include/linux/vm_event_item.h
@@ -41,7 +41,13 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
#ifdef CONFIG_COMPACTION
COMPACTBLOCKS, COMPACTPAGES, COMPACTPAGEFAILED,
COMPACTSTALL, COMPACTFAIL, COMPACTSUCCESS,
-#endif
+#ifdef CONFIG_BALLOON_COMPACTION
+ COMPACTBALLOONISOLATED, /* isolated from balloon pagelist */
+ COMPACTBALLOONMIGRATED, /* balloon page sucessfully migrated */
+ COMPACTBALLOONRETURNED, /* putback to pagelist, not-migrated */
+ COMPACTBALLOONRELEASED, /* old-page released after migration */
+#endif /* CONFIG_BALLOON_COMPACTION */
+#endif /* CONFIG_COMPACTION */
#ifdef CONFIG_HUGETLB_PAGE
HTLB_BUDDY_PGALLOC, HTLB_BUDDY_PGALLOC_FAIL,
#endif
diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
index d79f13d..9186000 100644
--- a/mm/balloon_compaction.c
+++ b/mm/balloon_compaction.c
@@ -114,6 +114,7 @@ bool isolate_balloon_page(struct page *page)
(page_count(page) == 2)) {
if (__isolate_balloon_page(page)) {
unlock_page(page);
+ count_vm_event(COMPACTBALLOONISOLATED);
return true;
}
}
@@ -137,9 +138,10 @@ void putback_balloon_page(struct page *page)
* concurrent isolation threads attempting to re-isolate it.
*/
lock_page(page);
- if (movable_balloon_page(page))
+ if (movable_balloon_page(page)) {
__putback_balloon_page(page);
-
+ count_vm_event(COMPACTBALLOONRETURNED);
+ }
unlock_page(page);
}
#endif /* CONFIG_BALLOON_COMPACTION */
diff --git a/mm/migrate.c b/mm/migrate.c
index 0bf2caf..052e59a 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -893,6 +893,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
page_is_file_cache(page));
put_page(page);
__free_page(page);
+ count_balloon_event(COMPACTBALLOONRELEASED);
return rc;
}
out:
diff --git a/mm/vmstat.c b/mm/vmstat.c
index df7a674..c7919c4 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -768,7 +768,15 @@ const char * const vmstat_text[] = {
"compact_stall",
"compact_fail",
"compact_success",
-#endif
+
+#ifdef CONFIG_BALLOON_COMPACTION
+ "compact_balloon_isolated",
+ "compact_balloon_migrated",
+ "compact_balloon_returned",
+ "compact_balloon_released",
+#endif /* CONFIG_BALLOON_COMPACTION */
+
+#endif /* CONFIG_COMPACTION */
#ifdef CONFIG_HUGETLB_PAGE
"htlb_buddy_alloc_success",
--
1.7.11.4
^ permalink raw reply related
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-21 13:52 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <e24f3073ef539985dea52943dcb84762213a0857.1345519422.git.aquini@redhat.com>
On Tue, Aug 21, 2012 at 09:47:44AM -0300, Rafael Aquini wrote:
> Memory fragmentation introduced by ballooning might reduce significantly
> the number of 2MB contiguous memory blocks that can be used within a guest,
> thus imposing performance penalties associated with the reduced number of
> transparent huge pages that could be used by the guest workload.
>
> This patch introduces a common interface to help a balloon driver on
> making its page set movable to compaction, and thus allowing the system
> to better leverage the compation efforts on memory defragmentation.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> include/linux/balloon_compaction.h | 113 +++++++++++++++++++++++++++++
> include/linux/pagemap.h | 18 +++++
> mm/Kconfig | 15 ++++
> mm/Makefile | 2 +-
> mm/balloon_compaction.c | 145 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 292 insertions(+), 1 deletion(-)
> create mode 100644 include/linux/balloon_compaction.h
> create mode 100644 mm/balloon_compaction.c
>
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> new file mode 100644
> index 0000000..5fbf036
> --- /dev/null
> +++ b/include/linux/balloon_compaction.h
> @@ -0,0 +1,113 @@
> +/*
> + * include/linux/balloon_compaction.h
> + *
> + * Common interface definitions for making balloon pages movable to compaction.
> + *
> + * Copyright (C) 2012, Red Hat, Inc. Rafael Aquini <aquini@redhat.com>
> + */
> +#ifndef _LINUX_BALLOON_COMPACTION_H
> +#define _LINUX_BALLOON_COMPACTION_H
> +#ifdef __KERNEL__
> +
> +#include <linux/rcupdate.h>
> +#include <linux/pagemap.h>
> +#include <linux/gfp.h>
> +
> +#if defined(CONFIG_BALLOON_COMPACTION)
> +extern bool isolate_balloon_page(struct page *);
> +extern void putback_balloon_page(struct page *);
> +
> +static inline bool movable_balloon_page(struct page *page)
> +{
> + struct address_space *mapping = NULL;
> + bool ret = false;
> + /*
> + * Before dereferencing and testing mapping->flags, lets make sure
> + * this is not a page that uses ->mapping in a different way
> + */
> + if (!PageSlab(page) && !PageSwapCache(page) &&
> + !PageAnon(page) && !page_mapped(page)) {
> + rcu_read_lock();
> + mapping = rcu_dereference(page->mapping);
> + if (mapping_balloon(mapping))
> + ret = true;
> + rcu_read_unlock();
This looks suspicious: you drop rcu_read_unlock
so can't page switch from balloon to non balloon?
Even if correct, it's probably cleaner to just make caller
invoke this within an rcu critical section.
You will notice that all callers immediately re-enter
rcu critical section anyway.
Alternatively, I noted that accesses to page->mapping
seem protected by page lock bit.
If true we can rely on that instead of RCU, just need
assign_balloon_mapping to lock_page/unlock_page.
> + }
> + return ret;
> +}
> +
> +static inline gfp_t balloon_mapping_gfp_mask(void)
> +{
> + return GFP_HIGHUSER_MOVABLE;
> +}
> +
> +/*
> + * __page_balloon_device - return the balloon device owing the page.
> + *
> + * This shall only be used at driver callbacks under proper page_lock,
> + * to get access to the balloon device structure that owns @page.
> + */
> +static inline void *__page_balloon_device(struct page *page)
> +{
> + struct address_space *mapping;
> + mapping = rcu_access_pointer(page->mapping);
> + if (mapping)
> + mapping = mapping->assoc_mapping;
> + return (void *)mapping;
> +}
> +
> +#define count_balloon_event(e) count_vm_event(e)
> +/*
> + * DEFINE_BALLOON_MAPPING_AOPS - declare and instantiate a callback descriptor
> + * to be used as balloon page->mapping->a_ops.
> + *
> + * @label : declaration identifier (var name)
> + * @migratepg : callback symbol name for performing the page migration step
> + * @isolatepg : callback symbol name for performing the page isolation step
> + * @putbackpg : callback symbol name for performing the page putback step
> + *
> + * address_space_operations utilized methods for ballooned pages:
> + * .migratepage - used to perform balloon's page migration (as is)
> + * .launder_page - used to isolate a page from balloon's page list
> + * .freepage - used to reinsert an isolated page to balloon's page list
> + */
It would be a good idea to document the assumptions here.
Looks like .launder_page and .freepage are called in rcu critical
section.
But migratepage isn't - why is that safe?
> +#define DEFINE_BALLOON_MAPPING_AOPS(label, migratepg, isolatepg, putbackpg) \
> + const struct address_space_operations (label) = { \
> + .migratepage = (migratepg), \
> + .launder_page = (isolatepg), \
> + .freepage = (putbackpg), \
> + }
> +
> +#else
> +static inline bool isolate_balloon_page(struct page *page) { return false; }
> +static inline bool movable_balloon_page(struct page *page) { return false; }
> +static inline void putback_balloon_page(struct page *page) { return; }
> +
> +static inline gfp_t balloon_mapping_gfp_mask(void)
> +{
> + return GFP_HIGHUSER;
> +}
> +
> +#define count_balloon_event(e) {}
> +#define DEFINE_BALLOON_MAPPING_AOPS(label, migratepg, isolatepg, putbackpg) \
> + const struct address_space_operations *(label) = NULL
> +
> +#endif /* CONFIG_BALLOON_COMPACTION */
> +
> +extern struct address_space *alloc_balloon_mapping(void *balloon_device,
> + const struct address_space_operations *a_ops);
> +extern void *page_balloon_device(struct page *page);
> +
> +static inline void assign_balloon_mapping(struct page *page,
> + struct address_space *mapping)
> +{
> + rcu_assign_pointer(page->mapping, mapping);
> +}
> +
> +static inline void clear_balloon_mapping(struct page *page)
> +{
> + rcu_assign_pointer(page->mapping, NULL);
> +}
> +
> +#endif /* __KERNEL__ */
> +#endif /* _LINUX_BALLOON_COMPACTION_H */
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index e42c762..6df0664 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -24,6 +24,7 @@ enum mapping_flags {
> AS_ENOSPC = __GFP_BITS_SHIFT + 1, /* ENOSPC on async write */
> AS_MM_ALL_LOCKS = __GFP_BITS_SHIFT + 2, /* under mm_take_all_locks() */
> AS_UNEVICTABLE = __GFP_BITS_SHIFT + 3, /* e.g., ramdisk, SHM_LOCK */
> + AS_BALLOON_MAP = __GFP_BITS_SHIFT + 4, /* balloon page special map */
> };
>
> static inline void mapping_set_error(struct address_space *mapping, int error)
> @@ -53,6 +54,23 @@ static inline int mapping_unevictable(struct address_space *mapping)
> return !!mapping;
> }
>
> +static inline void mapping_set_balloon(struct address_space *mapping)
> +{
> + set_bit(AS_BALLOON_MAP, &mapping->flags);
> +}
> +
> +static inline void mapping_clear_balloon(struct address_space *mapping)
> +{
> + clear_bit(AS_BALLOON_MAP, &mapping->flags);
> +}
> +
> +static inline int mapping_balloon(struct address_space *mapping)
> +{
> + if (mapping)
> + return test_bit(AS_BALLOON_MAP, &mapping->flags);
> + return !!mapping;
> +}
> +
> static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
> {
> return (__force gfp_t)mapping->flags & __GFP_BITS_MASK;
> diff --git a/mm/Kconfig b/mm/Kconfig
> index d5c8019..4857899 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -188,6 +188,21 @@ config SPLIT_PTLOCK_CPUS
> default "4"
>
> #
> +# support for memory balloon compaction
> +config BALLOON_COMPACTION
> + bool "Allow for balloon memory compaction/migration"
> + select COMPACTION
> + depends on VIRTIO_BALLOON
> + help
> + Memory fragmentation introduced by ballooning might reduce
> + significantly the number of 2MB contiguous memory blocks that can be
> + used within a guest, thus imposing performance penalties associated
> + with the reduced number of transparent huge pages that could be used
> + by the guest workload. Allowing the compaction & migration for memory
> + pages enlisted as being part of memory balloon devices avoids the
> + scenario aforementioned and helps improving memory defragmentation.
> +
> +#
> # support for memory compaction
> config COMPACTION
> bool "Allow for memory compaction"
> diff --git a/mm/Makefile b/mm/Makefile
> index 92753e2..78d8caa 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -16,7 +16,7 @@ obj-y := filemap.o mempool.o oom_kill.o fadvise.o \
> readahead.o swap.o truncate.o vmscan.o shmem.o \
> prio_tree.o util.o mmzone.o vmstat.o backing-dev.o \
> mm_init.o mmu_context.o percpu.o slab_common.o \
> - compaction.o $(mmu-y)
> + compaction.o balloon_compaction.o $(mmu-y)
>
> obj-y += init-mm.o
>
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> new file mode 100644
> index 0000000..d79f13d
> --- /dev/null
> +++ b/mm/balloon_compaction.c
> @@ -0,0 +1,145 @@
> +/*
> + * mm/balloon_compaction.c
> + *
> + * Common interface for making balloon pages movable to compaction.
> + *
> + * Copyright (C) 2012, Red Hat, Inc. Rafael Aquini <aquini@redhat.com>
> + */
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/balloon_compaction.h>
> +
> +/*
> + * alloc_balloon_mapping - allocates a special ->mapping for ballooned pages.
> + * @balloon_device: pointer address that references the balloon device which
> + * owns pages bearing this ->mapping.
> + * @a_ops: balloon_mapping address_space_operations descriptor.
> + *
> + * Users must call it to properly allocate and initialize an instance of
> + * struct address_space which will be used as the special page->mapping for
> + * balloon devices enlisted page instances.
> + */
> +struct address_space *alloc_balloon_mapping(void *balloon_device,
> + const struct address_space_operations *a_ops)
> +{
> + struct address_space *mapping;
> +
> + mapping = kmalloc(sizeof(*mapping), GFP_KERNEL);
> + if (mapping) {
If !mapping return NULL will make code shorter.
> + /*
> + * Give a clean 'zeroed' status to all elements of this special
> + * balloon page->mapping struct address_space instance.
> + */
> + address_space_init_once(mapping);
> +
> + /*
> + * Set mapping->flags appropriately, to allow balloon ->mapping
> + * identification, as well as give a proper hint to the balloon
> + * driver on what GFP allocation mask shall be used.
> + */
> + mapping_set_balloon(mapping);
> + mapping_set_gfp_mask(mapping, balloon_mapping_gfp_mask());
> +
> + /* balloon's page->mapping->a_ops callback descriptor */
> + mapping->a_ops = a_ops;
> +
> + /*
> + * balloon special page->mapping overloads ->assoc_mapping
> + * to held a reference back to the balloon device wich 'owns'
> + * a given page. This is the way we can cope with multiple
> + * balloon devices without losing reference of several
> + * ballooned pagesets.
> + */
> + mapping->assoc_mapping = (void *)balloon_device;
(void *) is only a good idea if you are casting a long value.
> +
> + return mapping;
> + }
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(alloc_balloon_mapping);
> +
> +#if defined(CONFIG_BALLOON_COMPACTION)
> +static inline int __isolate_balloon_page(struct page *page)
> +{
> + struct address_space *mapping;
> + int ret = 0;
> +
> + rcu_read_lock();
> + mapping = rcu_dereference(page->mapping);
> + if (mapping)
> + ret = mapping->a_ops->launder_page(page);
> +
> + rcu_read_unlock();
> + return ret;
> +}
> +
> +static inline void __putback_balloon_page(struct page *page)
> +{
> + struct address_space *mapping;
> +
> + rcu_read_lock();
> + mapping = rcu_dereference(page->mapping);
> + /*
> + * If we stumble across a page->mapping NULL here, so something has
> + * messed with the private isolated pageset we are iterating over.
> + */
> + BUG_ON(!mapping);
> +
> + mapping->a_ops->freepage(page);
> + /* isolation bumps up the page refcount, time to decrement it */
> + put_page(page);
> + rcu_read_unlock();
> +}
> +
> +/* __isolate_lru_page() counterpart for a ballooned page */
> +bool isolate_balloon_page(struct page *page)
> +{
> + if (likely(get_page_unless_zero(page))) {
> + /*
> + * As balloon pages are not isolated from LRU lists, concurrent
> + * compaction threads can race against page migration functions
> + * move_to_new_page() & __unmap_and_move().
> + * In order to avoid having an already isolated balloon page
> + * being (wrongly) re-isolated while it is under migration,
> + * lets be sure we have the page lock before proceeding with
> + * the balloon page isolation steps.
> + */
> + if (likely(trylock_page(page))) {
> + /*
> + * A ballooned page, by default, has just one refcount.
> + * Prevent concurrent compaction threads from isolating
> + * an already isolated balloon page by refcount check.
> + */
> + if (movable_balloon_page(page) &&
> + (page_count(page) == 2)) {
> + if (__isolate_balloon_page(page)) {
> + unlock_page(page);
> + return true;
> + }
> + }
> + unlock_page(page);
> + }
> + /*
> + * This page is either under migration, it is isolated already,
> + * or its isolation step has not happened at this round.
> + * Drop the refcount taken for it.
> + */
> + put_page(page);
> + }
> + return false;
> +}
> +
> +/* putback_lru_page() counterpart for a ballooned page */
> +void putback_balloon_page(struct page *page)
> +{
> + /*
> + * 'lock_page()' stabilizes the page and prevents races against
> + * concurrent isolation threads attempting to re-isolate it.
> + */
> + lock_page(page);
> + if (movable_balloon_page(page))
> + __putback_balloon_page(page);
> +
> + unlock_page(page);
> +}
> +#endif /* CONFIG_BALLOON_COMPACTION */
> --
> 1.7.11.4
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-21 14:25 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120821135223.GA7117@redhat.com>
On Tue, Aug 21, 2012 at 04:52:23PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 21, 2012 at 09:47:44AM -0300, Rafael Aquini wrote:
> > Memory fragmentation introduced by ballooning might reduce significantly
> > the number of 2MB contiguous memory blocks that can be used within a guest,
> > thus imposing performance penalties associated with the reduced number of
> > transparent huge pages that could be used by the guest workload.
> >
> > This patch introduces a common interface to help a balloon driver on
> > making its page set movable to compaction, and thus allowing the system
> > to better leverage the compation efforts on memory defragmentation.
> >
> > Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > ---
> > include/linux/balloon_compaction.h | 113 +++++++++++++++++++++++++++++
> > include/linux/pagemap.h | 18 +++++
> > mm/Kconfig | 15 ++++
> > mm/Makefile | 2 +-
> > mm/balloon_compaction.c | 145 +++++++++++++++++++++++++++++++++++++
> > 5 files changed, 292 insertions(+), 1 deletion(-)
> > create mode 100644 include/linux/balloon_compaction.h
> > create mode 100644 mm/balloon_compaction.c
> >
> > diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> > new file mode 100644
> > index 0000000..5fbf036
> > --- /dev/null
> > +++ b/include/linux/balloon_compaction.h
> > @@ -0,0 +1,113 @@
> > +/*
> > + * include/linux/balloon_compaction.h
> > + *
> > + * Common interface definitions for making balloon pages movable to compaction.
> > + *
> > + * Copyright (C) 2012, Red Hat, Inc. Rafael Aquini <aquini@redhat.com>
> > + */
> > +#ifndef _LINUX_BALLOON_COMPACTION_H
> > +#define _LINUX_BALLOON_COMPACTION_H
> > +#ifdef __KERNEL__
> > +
> > +#include <linux/rcupdate.h>
> > +#include <linux/pagemap.h>
> > +#include <linux/gfp.h>
> > +
> > +#if defined(CONFIG_BALLOON_COMPACTION)
> > +extern bool isolate_balloon_page(struct page *);
> > +extern void putback_balloon_page(struct page *);
> > +
> > +static inline bool movable_balloon_page(struct page *page)
> > +{
> > + struct address_space *mapping = NULL;
> > + bool ret = false;
> > + /*
> > + * Before dereferencing and testing mapping->flags, lets make sure
> > + * this is not a page that uses ->mapping in a different way
> > + */
> > + if (!PageSlab(page) && !PageSwapCache(page) &&
> > + !PageAnon(page) && !page_mapped(page)) {
> > + rcu_read_lock();
> > + mapping = rcu_dereference(page->mapping);
> > + if (mapping_balloon(mapping))
> > + ret = true;
> > + rcu_read_unlock();
>
> This looks suspicious: you drop rcu_read_unlock
> so can't page switch from balloon to non balloon?
>
> Even if correct, it's probably cleaner to just make caller
> invoke this within an rcu critical section.
> You will notice that all callers immediately re-enter
> rcu critical section anyway.
>
> Alternatively, I noted that accesses to page->mapping
> seem protected by page lock bit.
> If true we can rely on that instead of RCU, just need
> assign_balloon_mapping to lock_page/unlock_page.
Actually not true in all cases: not in case
of putback. Not sure whether code can be changed
to make it true.
> > + }
> > + return ret;
> > +}
> > +
> > +static inline gfp_t balloon_mapping_gfp_mask(void)
> > +{
> > + return GFP_HIGHUSER_MOVABLE;
> > +}
> > +
> > +/*
> > + * __page_balloon_device - return the balloon device owing the page.
> > + *
> > + * This shall only be used at driver callbacks under proper page_lock,
> > + * to get access to the balloon device structure that owns @page.
> > + */
> > +static inline void *__page_balloon_device(struct page *page)
> > +{
> > + struct address_space *mapping;
> > + mapping = rcu_access_pointer(page->mapping);
> > + if (mapping)
> > + mapping = mapping->assoc_mapping;
> > + return (void *)mapping;
> > +}
> > +
> > +#define count_balloon_event(e) count_vm_event(e)
> > +/*
> > + * DEFINE_BALLOON_MAPPING_AOPS - declare and instantiate a callback descriptor
> > + * to be used as balloon page->mapping->a_ops.
> > + *
> > + * @label : declaration identifier (var name)
> > + * @migratepg : callback symbol name for performing the page migration step
> > + * @isolatepg : callback symbol name for performing the page isolation step
> > + * @putbackpg : callback symbol name for performing the page putback step
> > + *
> > + * address_space_operations utilized methods for ballooned pages:
> > + * .migratepage - used to perform balloon's page migration (as is)
> > + * .launder_page - used to isolate a page from balloon's page list
> > + * .freepage - used to reinsert an isolated page to balloon's page list
> > + */
>
> It would be a good idea to document the assumptions here.
> Looks like .launder_page and .freepage are called in rcu critical
> section.
> But migratepage isn't - why is that safe?
>
> > +#define DEFINE_BALLOON_MAPPING_AOPS(label, migratepg, isolatepg, putbackpg) \
> > + const struct address_space_operations (label) = { \
> > + .migratepage = (migratepg), \
> > + .launder_page = (isolatepg), \
> > + .freepage = (putbackpg), \
> > + }
> > +
> > +#else
> > +static inline bool isolate_balloon_page(struct page *page) { return false; }
> > +static inline bool movable_balloon_page(struct page *page) { return false; }
> > +static inline void putback_balloon_page(struct page *page) { return; }
> > +
> > +static inline gfp_t balloon_mapping_gfp_mask(void)
> > +{
> > + return GFP_HIGHUSER;
> > +}
> > +
> > +#define count_balloon_event(e) {}
> > +#define DEFINE_BALLOON_MAPPING_AOPS(label, migratepg, isolatepg, putbackpg) \
> > + const struct address_space_operations *(label) = NULL
> > +
> > +#endif /* CONFIG_BALLOON_COMPACTION */
> > +
> > +extern struct address_space *alloc_balloon_mapping(void *balloon_device,
> > + const struct address_space_operations *a_ops);
> > +extern void *page_balloon_device(struct page *page);
> > +
> > +static inline void assign_balloon_mapping(struct page *page,
> > + struct address_space *mapping)
> > +{
> > + rcu_assign_pointer(page->mapping, mapping);
> > +}
> > +
> > +static inline void clear_balloon_mapping(struct page *page)
> > +{
> > + rcu_assign_pointer(page->mapping, NULL);
> > +}
> > +
> > +#endif /* __KERNEL__ */
> > +#endif /* _LINUX_BALLOON_COMPACTION_H */
> > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > index e42c762..6df0664 100644
> > --- a/include/linux/pagemap.h
> > +++ b/include/linux/pagemap.h
> > @@ -24,6 +24,7 @@ enum mapping_flags {
> > AS_ENOSPC = __GFP_BITS_SHIFT + 1, /* ENOSPC on async write */
> > AS_MM_ALL_LOCKS = __GFP_BITS_SHIFT + 2, /* under mm_take_all_locks() */
> > AS_UNEVICTABLE = __GFP_BITS_SHIFT + 3, /* e.g., ramdisk, SHM_LOCK */
> > + AS_BALLOON_MAP = __GFP_BITS_SHIFT + 4, /* balloon page special map */
> > };
> >
> > static inline void mapping_set_error(struct address_space *mapping, int error)
> > @@ -53,6 +54,23 @@ static inline int mapping_unevictable(struct address_space *mapping)
> > return !!mapping;
> > }
> >
> > +static inline void mapping_set_balloon(struct address_space *mapping)
> > +{
> > + set_bit(AS_BALLOON_MAP, &mapping->flags);
> > +}
> > +
> > +static inline void mapping_clear_balloon(struct address_space *mapping)
> > +{
> > + clear_bit(AS_BALLOON_MAP, &mapping->flags);
> > +}
> > +
> > +static inline int mapping_balloon(struct address_space *mapping)
> > +{
> > + if (mapping)
> > + return test_bit(AS_BALLOON_MAP, &mapping->flags);
> > + return !!mapping;
> > +}
> > +
> > static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
> > {
> > return (__force gfp_t)mapping->flags & __GFP_BITS_MASK;
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index d5c8019..4857899 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -188,6 +188,21 @@ config SPLIT_PTLOCK_CPUS
> > default "4"
> >
> > #
> > +# support for memory balloon compaction
> > +config BALLOON_COMPACTION
> > + bool "Allow for balloon memory compaction/migration"
> > + select COMPACTION
> > + depends on VIRTIO_BALLOON
> > + help
> > + Memory fragmentation introduced by ballooning might reduce
> > + significantly the number of 2MB contiguous memory blocks that can be
> > + used within a guest, thus imposing performance penalties associated
> > + with the reduced number of transparent huge pages that could be used
> > + by the guest workload. Allowing the compaction & migration for memory
> > + pages enlisted as being part of memory balloon devices avoids the
> > + scenario aforementioned and helps improving memory defragmentation.
> > +
> > +#
> > # support for memory compaction
> > config COMPACTION
> > bool "Allow for memory compaction"
> > diff --git a/mm/Makefile b/mm/Makefile
> > index 92753e2..78d8caa 100644
> > --- a/mm/Makefile
> > +++ b/mm/Makefile
> > @@ -16,7 +16,7 @@ obj-y := filemap.o mempool.o oom_kill.o fadvise.o \
> > readahead.o swap.o truncate.o vmscan.o shmem.o \
> > prio_tree.o util.o mmzone.o vmstat.o backing-dev.o \
> > mm_init.o mmu_context.o percpu.o slab_common.o \
> > - compaction.o $(mmu-y)
> > + compaction.o balloon_compaction.o $(mmu-y)
> >
> > obj-y += init-mm.o
> >
> > diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> > new file mode 100644
> > index 0000000..d79f13d
> > --- /dev/null
> > +++ b/mm/balloon_compaction.c
> > @@ -0,0 +1,145 @@
> > +/*
> > + * mm/balloon_compaction.c
> > + *
> > + * Common interface for making balloon pages movable to compaction.
> > + *
> > + * Copyright (C) 2012, Red Hat, Inc. Rafael Aquini <aquini@redhat.com>
> > + */
> > +#include <linux/slab.h>
> > +#include <linux/export.h>
> > +#include <linux/balloon_compaction.h>
> > +
> > +/*
> > + * alloc_balloon_mapping - allocates a special ->mapping for ballooned pages.
> > + * @balloon_device: pointer address that references the balloon device which
> > + * owns pages bearing this ->mapping.
> > + * @a_ops: balloon_mapping address_space_operations descriptor.
> > + *
> > + * Users must call it to properly allocate and initialize an instance of
> > + * struct address_space which will be used as the special page->mapping for
> > + * balloon devices enlisted page instances.
> > + */
> > +struct address_space *alloc_balloon_mapping(void *balloon_device,
> > + const struct address_space_operations *a_ops)
> > +{
> > + struct address_space *mapping;
> > +
> > + mapping = kmalloc(sizeof(*mapping), GFP_KERNEL);
> > + if (mapping) {
>
> If !mapping return NULL will make code shorter.
>
> > + /*
> > + * Give a clean 'zeroed' status to all elements of this special
> > + * balloon page->mapping struct address_space instance.
> > + */
> > + address_space_init_once(mapping);
> > +
> > + /*
> > + * Set mapping->flags appropriately, to allow balloon ->mapping
> > + * identification, as well as give a proper hint to the balloon
> > + * driver on what GFP allocation mask shall be used.
> > + */
> > + mapping_set_balloon(mapping);
> > + mapping_set_gfp_mask(mapping, balloon_mapping_gfp_mask());
> > +
> > + /* balloon's page->mapping->a_ops callback descriptor */
> > + mapping->a_ops = a_ops;
> > +
> > + /*
> > + * balloon special page->mapping overloads ->assoc_mapping
> > + * to held a reference back to the balloon device wich 'owns'
> > + * a given page. This is the way we can cope with multiple
> > + * balloon devices without losing reference of several
> > + * ballooned pagesets.
> > + */
> > + mapping->assoc_mapping = (void *)balloon_device;
>
> (void *) is only a good idea if you are casting a long value.
>
> > +
> > + return mapping;
> > + }
> > + return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(alloc_balloon_mapping);
> > +
> > +#if defined(CONFIG_BALLOON_COMPACTION)
> > +static inline int __isolate_balloon_page(struct page *page)
> > +{
> > + struct address_space *mapping;
> > + int ret = 0;
> > +
> > + rcu_read_lock();
> > + mapping = rcu_dereference(page->mapping);
> > + if (mapping)
> > + ret = mapping->a_ops->launder_page(page);
> > +
> > + rcu_read_unlock();
> > + return ret;
> > +}
> > +
> > +static inline void __putback_balloon_page(struct page *page)
> > +{
> > + struct address_space *mapping;
> > +
> > + rcu_read_lock();
> > + mapping = rcu_dereference(page->mapping);
> > + /*
> > + * If we stumble across a page->mapping NULL here, so something has
> > + * messed with the private isolated pageset we are iterating over.
> > + */
> > + BUG_ON(!mapping);
> > +
> > + mapping->a_ops->freepage(page);
> > + /* isolation bumps up the page refcount, time to decrement it */
> > + put_page(page);
> > + rcu_read_unlock();
> > +}
> > +
> > +/* __isolate_lru_page() counterpart for a ballooned page */
> > +bool isolate_balloon_page(struct page *page)
> > +{
> > + if (likely(get_page_unless_zero(page))) {
> > + /*
> > + * As balloon pages are not isolated from LRU lists, concurrent
> > + * compaction threads can race against page migration functions
> > + * move_to_new_page() & __unmap_and_move().
> > + * In order to avoid having an already isolated balloon page
> > + * being (wrongly) re-isolated while it is under migration,
> > + * lets be sure we have the page lock before proceeding with
> > + * the balloon page isolation steps.
> > + */
> > + if (likely(trylock_page(page))) {
> > + /*
> > + * A ballooned page, by default, has just one refcount.
> > + * Prevent concurrent compaction threads from isolating
> > + * an already isolated balloon page by refcount check.
> > + */
> > + if (movable_balloon_page(page) &&
> > + (page_count(page) == 2)) {
> > + if (__isolate_balloon_page(page)) {
> > + unlock_page(page);
> > + return true;
> > + }
> > + }
> > + unlock_page(page);
> > + }
> > + /*
> > + * This page is either under migration, it is isolated already,
> > + * or its isolation step has not happened at this round.
> > + * Drop the refcount taken for it.
> > + */
> > + put_page(page);
> > + }
> > + return false;
> > +}
> > +
> > +/* putback_lru_page() counterpart for a ballooned page */
> > +void putback_balloon_page(struct page *page)
> > +{
> > + /*
> > + * 'lock_page()' stabilizes the page and prevents races against
> > + * concurrent isolation threads attempting to re-isolate it.
> > + */
> > + lock_page(page);
> > + if (movable_balloon_page(page))
> > + __putback_balloon_page(page);
> > +
> > + unlock_page(page);
> > +}
> > +#endif /* CONFIG_BALLOON_COMPACTION */
> > --
> > 1.7.11.4
^ permalink raw reply
* Re: [PATCH v8 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-21 14:40 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <c5f02c618c99b0da11240c1b504672de6f70a074.1345519422.git.aquini@redhat.com>
On Tue, Aug 21, 2012 at 09:47:46AM -0300, Rafael Aquini wrote:
> Memory fragmentation introduced by ballooning might reduce significantly
> the number of 2MB contiguous memory blocks that can be used within a guest,
> thus imposing performance penalties associated with the reduced number of
> transparent huge pages that could be used by the guest workload.
>
> Besides making balloon pages movable at allocation time and introducing
> the necessary primitives to perform balloon page migration/compaction,
> this patch also introduces the following locking scheme to provide the
> proper synchronization and protection for struct virtio_balloon elements
> against concurrent accesses due to parallel operations introduced by
> memory compaction / page migration.
> - balloon_lock (mutex) : synchronizes the access demand to elements of
> struct virtio_balloon and its queue operations;
> - pages_lock (spinlock): special protection to balloon pages list against
> concurrent list handling operations;
> - virtio_baloon->pages list handling sync by RCU operations;
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 210 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 199 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 0908e60..bda7bb0 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -27,6 +27,7 @@
> #include <linux/delay.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> +#include <linux/balloon_compaction.h>
>
> /*
> * Balloon device works in 4K page units. So each page is pointed to by
> @@ -35,6 +36,12 @@
> */
> #define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
>
> +/* flags used to hint compaction procedures about the balloon device status */
> +enum balloon_status_flags {
> + BALLOON_REMOVAL = 0, /* balloon device is under removal steps */
> + BALLOON_OK, /* balloon device is up and running */
> +};
> +
> struct virtio_balloon
> {
> struct virtio_device *vdev;
> @@ -46,11 +53,24 @@ struct virtio_balloon
> /* The thread servicing the balloon. */
> struct task_struct *thread;
>
> + /* balloon special page->mapping */
> + struct address_space *mapping;
> +
> + /* Synchronize access/update to this struct virtio_balloon elements */
> + struct mutex balloon_lock;
> +
> /* Waiting for host to ack the pages we released. */
> wait_queue_head_t acked;
>
> /* Number of balloon pages we've told the Host we're not using. */
> unsigned int num_pages;
> +
> + /* balloon device status flag */
> + unsigned short balloon_status;
> +
> + /* Protect 'pages' list against concurrent handling */
> + spinlock_t pages_lock;
> +
> /*
> * The pages we've told the Host we're not using.
> * Each page on this list adds VIRTIO_BALLOON_PAGES_PER_PAGE
> @@ -122,13 +142,17 @@ static void set_page_pfns(u32 pfns[], struct page *page)
>
> static void fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> + /* Get the proper GFP alloc mask from vb->mapping flags */
> + gfp_t vb_gfp_mask = mapping_gfp_mask(vb->mapping);
> +
> /* We can only do one array worth at a time. */
> num = min(num, ARRAY_SIZE(vb->pfns));
>
> + mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
> - __GFP_NOMEMALLOC | __GFP_NOWARN);
> + struct page *page = alloc_page(vb_gfp_mask | __GFP_NORETRY |
> + __GFP_NOWARN | __GFP_NOMEMALLOC);
> if (!page) {
> if (printk_ratelimit())
> dev_printk(KERN_INFO, &vb->vdev->dev,
> @@ -141,7 +165,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> set_page_pfns(vb->pfns + vb->num_pfns, page);
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> totalram_pages--;
> - list_add(&page->lru, &vb->pages);
> + spin_lock(&vb->pages_lock);
> + list_add_rcu(&page->lru, &vb->pages);
> + assign_balloon_mapping(page, vb->mapping);
> + spin_unlock(&vb->pages_lock);
> }
>
> /* Didn't get any? Oh well. */
> @@ -149,6 +176,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> return;
>
> tell_host(vb, vb->inflate_vq);
> + mutex_unlock(&vb->balloon_lock);
> }
>
> static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
> @@ -169,21 +197,48 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> /* We can only do one array worth at a time. */
> num = min(num, ARRAY_SIZE(vb->pfns));
>
> + mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - page = list_first_entry(&vb->pages, struct page, lru);
> - list_del(&page->lru);
> + /*
> + * We can race against 'virtballoon_isolatepage()' and end up
> + * stumbling across a _temporarily_ empty 'pages' list.
> + */
> + spin_lock(&vb->pages_lock);
> + page = list_first_or_null_rcu(&vb->pages, struct page, lru);
Why is list_first_or_null_rcu called outside
RCU critical section here?
> + if (!page) {
> + spin_unlock(&vb->pages_lock);
> + break;
> + }
> + /*
> + * It is safe now to drop page->mapping and delete this page
> + * from balloon page list, since we are grabbing 'pages_lock'
> + * which prevents 'virtballoon_isolatepage()' from acting.
> + */
> + clear_balloon_mapping(page);
> + list_del_rcu(&page->lru);
> + spin_unlock(&vb->pages_lock);
> set_page_pfns(vb->pfns + vb->num_pfns, page);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> }
> + /*
> + * Syncrhonize RCU grace period and wait for all RCU read critical side
> + * sections to finish before proceeding with page release steps.
> + * This avoids compaction/migration callback races against balloon
> + * device removal steps.
> + */
> + synchronize_rcu();
>
> /*
> * Note that if
> * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> * is true, we *have* to do it in this order
> */
> - tell_host(vb, vb->deflate_vq);
> - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> + if (vb->num_pfns > 0) {
> + tell_host(vb, vb->deflate_vq);
> + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> + }
> + mutex_unlock(&vb->balloon_lock);
> }
>
> static inline void update_stat(struct virtio_balloon *vb, int idx,
> @@ -239,6 +294,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
> struct scatterlist sg;
> unsigned int len;
>
> + mutex_lock(&vb->balloon_lock);
> vb->need_stats_update = 0;
> update_balloon_stats(vb);
>
> @@ -249,6 +305,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
> if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
> BUG();
> virtqueue_kick(vq);
> + mutex_unlock(&vb->balloon_lock);
> }
>
> static void virtballoon_changed(struct virtio_device *vdev)
> @@ -261,22 +318,27 @@ static void virtballoon_changed(struct virtio_device *vdev)
> static inline s64 towards_target(struct virtio_balloon *vb)
> {
> __le32 v;
> - s64 target;
> + s64 target, actual;
>
> + mutex_lock(&vb->balloon_lock);
> + actual = vb->num_pages;
> vb->vdev->config->get(vb->vdev,
> offsetof(struct virtio_balloon_config, num_pages),
> &v, sizeof(v));
> target = le32_to_cpu(v);
> - return target - vb->num_pages;
> + mutex_unlock(&vb->balloon_lock);
> + return target - actual;
> }
>
> static void update_balloon_size(struct virtio_balloon *vb)
> {
> - __le32 actual = cpu_to_le32(vb->num_pages);
> -
> + __le32 actual;
> + mutex_lock(&vb->balloon_lock);
> + actual = cpu_to_le32(vb->num_pages);
> vb->vdev->config->set(vb->vdev,
> offsetof(struct virtio_balloon_config, actual),
> &actual, sizeof(actual));
> + mutex_unlock(&vb->balloon_lock);
> }
>
> static int balloon(void *_vballoon)
> @@ -339,9 +401,117 @@ static int init_vqs(struct virtio_balloon *vb)
> return 0;
> }
>
> +#ifdef CONFIG_BALLOON_COMPACTION
> +/*
> + * Populate balloon_mapping->a_ops callback method to perform the balloon
> + * page migration task.
> + *
> + * After a ballooned page gets isolated by compaction procedures, this is the
> + * function that performs the page migration on behalf of move_to_new_page(),
> + * when the last calls (page)->mapping->a_ops->migratepage.
> + *
> + * Page migration for virtio balloon is done in a simple swap fashion which
> + * follows these two steps:
> + * 1) insert newpage into vb->pages list and update the host about it;
> + * 2) update the host about the removed old page from vb->pages list;
> + */
> +int virtballoon_migratepage(struct address_space *mapping,
> + struct page *newpage, struct page *page, enum migrate_mode mode)
> +{
> + struct virtio_balloon *vb = (void *)__page_balloon_device(page);
Don't cast to void *.
> +
> + /* at this point, besides very unlikely, a NULL *vb is a serious bug */
A bit of a strange comment. Either it's unlikely or a bug :)
Besides, vb is checked for NULL value, not *vb.
> + BUG_ON(!vb);
> +
> + /*
> + * Skip page migration step if the memory balloon device is under its
> + * removal procedure, to avoid racing against module unload.
> + *
What kind of race does this fix? Why does not
leaking all pages fix it?
> + * If there still are isolated (balloon) pages under migration lists,
> + * 'virtballoon_putbackpage()' will take care of them properly, before
> + * the module unload finishes.
> + */
> + if (vb->balloon_status == BALLOON_REMOVAL)
> + return -EAGAIN;
> +
> + mutex_lock(&vb->balloon_lock);
> +
> + /* balloon's page migration 1st step */
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + spin_lock(&vb->pages_lock);
> + list_add_rcu(&newpage->lru, &vb->pages);
> + spin_unlock(&vb->pages_lock);
> + set_page_pfns(vb->pfns, newpage);
> + tell_host(vb, vb->inflate_vq);
> +
> + /* balloon's page migration 2nd step */
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb->pfns, page);
> + tell_host(vb, vb->deflate_vq);
> +
> + mutex_unlock(&vb->balloon_lock);
> + return 0;
> +}
> +
> +/*
> + * Populate balloon_mapping->a_ops callback method to help compaction on
> + * isolating a page from the balloon page list for posterior migration.
> + */
> +int virtballoon_isolatepage(struct page *page)
> +{
> + struct virtio_balloon *vb = (void *)__page_balloon_device(page);
Cast here looks wrong.
> + int ret = 0;
> + /*
> + * If we stumble across a NULL *vb here, it means this page has been
> + * already released by 'leak_balloon()'.
> + *
> + * We also skip the page isolation step if the memory balloon device is
> + * under its removal procedure, to avoid racing against module unload.
> + */
What kind of race do you have in mind here?
Doesn't leaking all pages in module removal address it?
> + if (vb
How can vb be NULL? Pls document.
> && (vb->balloon_status != BALLOON_REMOVAL)) {
Read of balloon_status needs some kind of memory barrier I think.
> + spin_lock(&vb->pages_lock);
> + /*
> + * virtballoon_isolatepage() can race against leak_balloon(),
> + * and (wrongly) isolate a page that is about to be freed.
> + * Test page->mapping under pages_lock to close that window.
> + */
> + if (rcu_access_pointer(page->mapping) == vb->mapping) {
> + /* It is safe to isolate this page, now */
> + list_del_rcu(&page->lru);
> + ret = 1;
> + }
> + spin_unlock(&vb->pages_lock);
> + }
> + return ret;
> +}
> +
> +/*
> + * Populate balloon_mapping->a_ops callback method to help compaction on
> + * re-inserting a not-migrated isolated page into the balloon page list.
> + */
> +void virtballoon_putbackpage(struct page *page)
> +{
> + struct virtio_balloon *vb = (void *)__page_balloon_device(page);
> +
> + /* at this point, besides very unlikely, a NULL *vb is a serious bug */
A bit of a strange comment. Either it's unlikely or a bug :)
Besides, vb is checked for NULL value, not *vb.
> + BUG_ON(!vb);
> +
> + spin_lock(&vb->pages_lock);
> + list_add_rcu(&page->lru, &vb->pages);
> + spin_unlock(&vb->pages_lock);
> +}
> +#endif /* CONFIG_BALLOON_COMPACTION */
> +
> +/* define the balloon_mapping->a_ops callbacks to allow compaction/migration */
> +static DEFINE_BALLOON_MAPPING_AOPS(virtio_balloon_aops,
> + virtballoon_migratepage,
> + virtballoon_isolatepage,
> + virtballoon_putbackpage);
> +
> static int virtballoon_probe(struct virtio_device *vdev)
> {
> struct virtio_balloon *vb;
> + struct address_space *vb_mapping;
> int err;
>
> vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
> @@ -351,12 +521,24 @@ static int virtballoon_probe(struct virtio_device *vdev)
> }
>
> INIT_LIST_HEAD(&vb->pages);
> + spin_lock_init(&vb->pages_lock);
> + mutex_init(&vb->balloon_lock);
> +
> vb->num_pages = 0;
> init_waitqueue_head(&vb->config_change);
> init_waitqueue_head(&vb->acked);
> vb->vdev = vdev;
> vb->need_stats_update = 0;
>
> + /* Allocate a special page->mapping for this balloon device */
This comment is not helpful, pls remove.
> + vb_mapping = alloc_balloon_mapping((void *)vb, &virtio_balloon_aops);
Cast to void * is not needed.
> + if (!vb_mapping) {
> + err = -ENOMEM;
> + goto out_free_vb;
> + }
> + /* Store the page->mapping reference for this balloon device */
> + vb->mapping = vb_mapping;
This comment is not helpful, pls remove.
> +
> err = init_vqs(vb);
> if (err)
> goto out_free_vb;
> @@ -367,12 +549,14 @@ static int virtballoon_probe(struct virtio_device *vdev)
> goto out_del_vqs;
> }
>
> + vb->balloon_status = BALLOON_OK;
> return 0;
>
> out_del_vqs:
> vdev->config->del_vqs(vdev);
> out_free_vb:
> kfree(vb);
> + kfree(vb_mapping);
I think it's better to wrap free for vb mapping too.
> out:
> return err;
> }
> @@ -394,8 +578,10 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
> {
> struct virtio_balloon *vb = vdev->priv;
>
> + vb->balloon_status = BALLOON_REMOVAL;
This needs some kind of barrier.
> kthread_stop(vb->thread);
> remove_common(vb);
> + kfree(vb->mapping);
> kfree(vb);
> }
>
> @@ -409,6 +595,7 @@ static int virtballoon_freeze(struct virtio_device *vdev)
> * function is called.
> */
>
> + vb->balloon_status = BALLOON_REMOVAL;
Here module is not going away so why change status?
> remove_common(vb);
> return 0;
> }
> @@ -424,6 +611,7 @@ static int virtballoon_restore(struct virtio_device *vdev)
>
> fill_balloon(vb, towards_target(vb));
> update_balloon_size(vb);
> + vb->balloon_status = BALLOON_OK;
Isn't this too late to set status here? We filled the balloon ...
> return 0;
> }
> #endif
> --
> 1.7.11.4
^ permalink raw reply
* Re: [PATCH v8 4/5] mm: introduce putback_movable_pages()
From: Michael S. Tsirkin @ 2012-08-21 14:42 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <16a7bbe6c1d4df624679646ef13566798a82798f.1345519422.git.aquini@redhat.com>
On Tue, Aug 21, 2012 at 09:47:47AM -0300, Rafael Aquini wrote:
> The PATCH "mm: introduce compaction and migration for virtio ballooned pages"
> hacks around putback_lru_pages() in order to allow ballooned pages to be
> re-inserted on balloon page list as if a ballooned page was like a LRU page.
>
> As ballooned pages are not legitimate LRU pages, this patch introduces
> putback_movable_pages() to properly cope with cases where the isolated
> pageset contains ballooned pages and LRU pages, thus fixing the mentioned
> inelegant hack around putback_lru_pages().
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> include/linux/migrate.h | 2 ++
> mm/compaction.c | 4 ++--
> mm/migrate.c | 20 ++++++++++++++++++++
> mm/page_alloc.c | 2 +-
> 4 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index ce7e667..ff103a1 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -10,6 +10,7 @@ typedef struct page *new_page_t(struct page *, unsigned long private, int **);
> #ifdef CONFIG_MIGRATION
>
> extern void putback_lru_pages(struct list_head *l);
> +extern void putback_movable_pages(struct list_head *l);
> extern int migrate_page(struct address_space *,
> struct page *, struct page *, enum migrate_mode);
> extern int migrate_pages(struct list_head *l, new_page_t x,
> @@ -33,6 +34,7 @@ extern int migrate_huge_page_move_mapping(struct address_space *mapping,
> #else
>
> static inline void putback_lru_pages(struct list_head *l) {}
> +static inline void putback_movable_pages(struct list_head *l) {}
> static inline int migrate_pages(struct list_head *l, new_page_t x,
> unsigned long private, bool offlining,
> enum migrate_mode mode) { return -ENOSYS; }
> diff --git a/mm/compaction.c b/mm/compaction.c
> index ce43dc2..782ed32 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -759,9 +759,9 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
> trace_mm_compaction_migratepages(nr_migrate - nr_remaining,
> nr_remaining);
>
> - /* Release LRU pages not migrated */
> + /* Release isolated pages not migrated */
> if (err) {
> - putback_lru_pages(&cc->migratepages);
> + putback_movable_pages(&cc->migratepages);
> cc->nr_migratepages = 0;
> if (err == -ENOMEM) {
> ret = COMPACT_PARTIAL;
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 6392da258..0bf2caf 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -80,6 +80,26 @@ void putback_lru_pages(struct list_head *l)
> list_del(&page->lru);
> dec_zone_page_state(page, NR_ISOLATED_ANON +
> page_is_file_cache(page));
> + putback_lru_page(page);
> + }
> +}
> +
> +/*
> + * Put previously isolated pages back onto the appropriated lists
Do you mean appropriate lists?
> + * from where they were once taken off for compaction/migration.
> + *
> + * This function shall be used instead of putback_lru_pages(),
> + * whenever the isolated pageset has been built by isolate_migratepages_range()
> + */
> +void putback_movable_pages(struct list_head *l)
> +{
> + struct page *page;
> + struct page *page2;
> +
> + list_for_each_entry_safe(page, page2, l, lru) {
> + list_del(&page->lru);
> + dec_zone_page_state(page, NR_ISOLATED_ANON +
> + page_is_file_cache(page));
> if (unlikely(movable_balloon_page(page)))
> putback_balloon_page(page);
> else
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 009ac28..78b7663 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5669,7 +5669,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
> 0, false, MIGRATE_SYNC);
> }
>
> - putback_lru_pages(&cc.migratepages);
> + putback_movable_pages(&cc.migratepages);
> return ret > 0 ? 0 : ret;
> }
>
> --
> 1.7.11.4
^ permalink raw reply
* Re: [PATCH v8 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-21 14:57 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <c5f02c618c99b0da11240c1b504672de6f70a074.1345519422.git.aquini@redhat.com>
On Tue, Aug 21, 2012 at 09:47:46AM -0300, Rafael Aquini wrote:
> Memory fragmentation introduced by ballooning might reduce significantly
> the number of 2MB contiguous memory blocks that can be used within a guest,
> thus imposing performance penalties associated with the reduced number of
> transparent huge pages that could be used by the guest workload.
>
> Besides making balloon pages movable at allocation time and introducing
> the necessary primitives to perform balloon page migration/compaction,
> this patch also introduces the following locking scheme to provide the
> proper synchronization and protection for struct virtio_balloon elements
> against concurrent accesses due to parallel operations introduced by
> memory compaction / page migration.
> - balloon_lock (mutex) : synchronizes the access demand to elements of
> struct virtio_balloon and its queue operations;
> - pages_lock (spinlock): special protection to balloon pages list against
> concurrent list handling operations;
> - virtio_baloon->pages list handling sync by RCU operations;
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
Sorry about sending multiple comments to same patch.
I just thought of some more questions.
> ---
> drivers/virtio/virtio_balloon.c | 210 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 199 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 0908e60..bda7bb0 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -27,6 +27,7 @@
> #include <linux/delay.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> +#include <linux/balloon_compaction.h>
>
> /*
> * Balloon device works in 4K page units. So each page is pointed to by
> @@ -35,6 +36,12 @@
> */
> #define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
>
> +/* flags used to hint compaction procedures about the balloon device status */
> +enum balloon_status_flags {
> + BALLOON_REMOVAL = 0, /* balloon device is under removal steps */
> + BALLOON_OK, /* balloon device is up and running */
> +};
> +
> struct virtio_balloon
> {
> struct virtio_device *vdev;
> @@ -46,11 +53,24 @@ struct virtio_balloon
> /* The thread servicing the balloon. */
> struct task_struct *thread;
>
> + /* balloon special page->mapping */
> + struct address_space *mapping;
> +
> + /* Synchronize access/update to this struct virtio_balloon elements */
> + struct mutex balloon_lock;
> +
> /* Waiting for host to ack the pages we released. */
> wait_queue_head_t acked;
>
> /* Number of balloon pages we've told the Host we're not using. */
> unsigned int num_pages;
> +
> + /* balloon device status flag */
> + unsigned short balloon_status;
> +
> + /* Protect 'pages' list against concurrent handling */
> + spinlock_t pages_lock;
> +
> /*
> * The pages we've told the Host we're not using.
> * Each page on this list adds VIRTIO_BALLOON_PAGES_PER_PAGE
> @@ -122,13 +142,17 @@ static void set_page_pfns(u32 pfns[], struct page *page)
>
> static void fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> + /* Get the proper GFP alloc mask from vb->mapping flags */
> + gfp_t vb_gfp_mask = mapping_gfp_mask(vb->mapping);
> +
> /* We can only do one array worth at a time. */
> num = min(num, ARRAY_SIZE(vb->pfns));
>
> + mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
> - __GFP_NOMEMALLOC | __GFP_NOWARN);
> + struct page *page = alloc_page(vb_gfp_mask | __GFP_NORETRY |
> + __GFP_NOWARN | __GFP_NOMEMALLOC);
> if (!page) {
> if (printk_ratelimit())
> dev_printk(KERN_INFO, &vb->vdev->dev,
> @@ -141,7 +165,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> set_page_pfns(vb->pfns + vb->num_pfns, page);
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> totalram_pages--;
> - list_add(&page->lru, &vb->pages);
> + spin_lock(&vb->pages_lock);
> + list_add_rcu(&page->lru, &vb->pages);
> + assign_balloon_mapping(page, vb->mapping);
> + spin_unlock(&vb->pages_lock);
> }
>
> /* Didn't get any? Oh well. */
> @@ -149,6 +176,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> return;
>
> tell_host(vb, vb->inflate_vq);
> + mutex_unlock(&vb->balloon_lock);
> }
>
> static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
> @@ -169,21 +197,48 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> /* We can only do one array worth at a time. */
> num = min(num, ARRAY_SIZE(vb->pfns));
>
> + mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - page = list_first_entry(&vb->pages, struct page, lru);
> - list_del(&page->lru);
> + /*
> + * We can race against 'virtballoon_isolatepage()' and end up
> + * stumbling across a _temporarily_ empty 'pages' list.
> + */
> + spin_lock(&vb->pages_lock);
> + page = list_first_or_null_rcu(&vb->pages, struct page, lru);
> + if (!page) {
> + spin_unlock(&vb->pages_lock);
> + break;
Now what happens? putback will return the pages here but you have exited
the loop.
> + }
> + /*
> + * It is safe now to drop page->mapping and delete this page
> + * from balloon page list, since we are grabbing 'pages_lock'
> + * which prevents 'virtballoon_isolatepage()' from acting.
> + */
> + clear_balloon_mapping(page);
> + list_del_rcu(&page->lru);
> + spin_unlock(&vb->pages_lock);
> set_page_pfns(vb->pfns + vb->num_pfns, page);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> }
> + /*
> + * Syncrhonize
Typo.
> RCU grace period and wait for all RCU read critical side
> + * sections to finish before proceeding with page release steps.
> + * This avoids compaction/migration callback races against balloon
> + * device removal steps.
> + */
> + synchronize_rcu();
>
> /*
> * Note that if
> * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> * is true, we *have* to do it in this order
> */
> - tell_host(vb, vb->deflate_vq);
> - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> + if (vb->num_pfns > 0) {
> + tell_host(vb, vb->deflate_vq);
> + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> + }
> + mutex_unlock(&vb->balloon_lock);
> }
>
> static inline void update_stat(struct virtio_balloon *vb, int idx,
> @@ -239,6 +294,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
> struct scatterlist sg;
> unsigned int len;
>
> + mutex_lock(&vb->balloon_lock);
> vb->need_stats_update = 0;
> update_balloon_stats(vb);
>
> @@ -249,6 +305,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
> if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
> BUG();
> virtqueue_kick(vq);
> + mutex_unlock(&vb->balloon_lock);
> }
>
> static void virtballoon_changed(struct virtio_device *vdev)
> @@ -261,22 +318,27 @@ static void virtballoon_changed(struct virtio_device *vdev)
> static inline s64 towards_target(struct virtio_balloon *vb)
> {
> __le32 v;
> - s64 target;
> + s64 target, actual;
>
> + mutex_lock(&vb->balloon_lock);
> + actual = vb->num_pages;
> vb->vdev->config->get(vb->vdev,
> offsetof(struct virtio_balloon_config, num_pages),
> &v, sizeof(v));
> target = le32_to_cpu(v);
> - return target - vb->num_pages;
> + mutex_unlock(&vb->balloon_lock);
> + return target - actual;
> }
>
> static void update_balloon_size(struct virtio_balloon *vb)
> {
> - __le32 actual = cpu_to_le32(vb->num_pages);
> -
> + __le32 actual;
> + mutex_lock(&vb->balloon_lock);
> + actual = cpu_to_le32(vb->num_pages);
> vb->vdev->config->set(vb->vdev,
> offsetof(struct virtio_balloon_config, actual),
> &actual, sizeof(actual));
> + mutex_unlock(&vb->balloon_lock);
> }
>
> static int balloon(void *_vballoon)
> @@ -339,9 +401,117 @@ static int init_vqs(struct virtio_balloon *vb)
> return 0;
> }
>
> +#ifdef CONFIG_BALLOON_COMPACTION
> +/*
> + * Populate balloon_mapping->a_ops callback method to perform the balloon
> + * page migration task.
> + *
> + * After a ballooned page gets isolated by compaction procedures, this is the
> + * function that performs the page migration on behalf of move_to_new_page(),
> + * when the last calls (page)->mapping->a_ops->migratepage.
> + *
> + * Page migration for virtio balloon is done in a simple swap fashion which
> + * follows these two steps:
> + * 1) insert newpage into vb->pages list and update the host about it;
> + * 2) update the host about the removed old page from vb->pages list;
> + */
> +int virtballoon_migratepage(struct address_space *mapping,
> + struct page *newpage, struct page *page, enum migrate_mode mode)
> +{
> + struct virtio_balloon *vb = (void *)__page_balloon_device(page);
> +
> + /* at this point, besides very unlikely, a NULL *vb is a serious bug */
> + BUG_ON(!vb);
> +
> + /*
> + * Skip page migration step if the memory balloon device is under its
> + * removal procedure, to avoid racing against module unload.
> + *
> + * If there still are isolated (balloon) pages under migration lists,
> + * 'virtballoon_putbackpage()' will take care of them properly, before
> + * the module unload finishes.
> + */
> + if (vb->balloon_status == BALLOON_REMOVAL)
> + return -EAGAIN;
> +
> + mutex_lock(&vb->balloon_lock);
> +
> + /* balloon's page migration 1st step */
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + spin_lock(&vb->pages_lock);
> + list_add_rcu(&newpage->lru, &vb->pages);
> + spin_unlock(&vb->pages_lock);
> + set_page_pfns(vb->pfns, newpage);
> + tell_host(vb, vb->inflate_vq);
> +
> + /* balloon's page migration 2nd step */
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb->pfns, page);
> + tell_host(vb, vb->deflate_vq);
> +
> + mutex_unlock(&vb->balloon_lock);
> + return 0;
> +}
> +
> +/*
> + * Populate balloon_mapping->a_ops callback method to help compaction on
> + * isolating a page from the balloon page list for posterior migration.
What does posterior migration mean?
> + */
> +int virtballoon_isolatepage(struct page *page)
> +{
Nothing at all in this function seems to be virtio balloon
specific. Can it move to mm core?
> + struct virtio_balloon *vb = (void *)__page_balloon_device(page);
> + int ret = 0;
> + /*
> + * If we stumble across a NULL *vb here, it means this page has been
> + * already released by 'leak_balloon()'.
> + *
> + * We also skip the page isolation step if the memory balloon device is
> + * under its removal procedure, to avoid racing against module unload.
> + */
> + if (vb && (vb->balloon_status != BALLOON_REMOVAL)) {
> + spin_lock(&vb->pages_lock);
> + /*
> + * virtballoon_isolatepage() can race against leak_balloon(),
> + * and (wrongly) isolate a page that is about to be freed.
> + * Test page->mapping under pages_lock to close that window.
> + */
Looks strange. If so could vb have gone away completely?
And if so would not this trigger use after free?
Can we take some lock when leaking page to prevent this, instead?
page_lock comes to mind ...
> + if (rcu_access_pointer(page->mapping) == vb->mapping) {
> + /* It is safe to isolate this page, now */
> + list_del_rcu(&page->lru);
> + ret = 1;
> + }
> + spin_unlock(&vb->pages_lock);
> + }
> + return ret;
> +}
> +
> +/*
> + * Populate balloon_mapping->a_ops callback method to help compaction on
> + * re-inserting a not-migrated isolated page into the balloon page list.
> + */
> +void virtballoon_putbackpage(struct page *page)
> +{
> + struct virtio_balloon *vb = (void *)__page_balloon_device(page);
> +
> + /* at this point, besides very unlikely, a NULL *vb is a serious bug */
> + BUG_ON(!vb);
> +
> + spin_lock(&vb->pages_lock);
> + list_add_rcu(&page->lru, &vb->pages);
> + spin_unlock(&vb->pages_lock);
> +}
> +#endif /* CONFIG_BALLOON_COMPACTION */
> +
> +/* define the balloon_mapping->a_ops callbacks to allow compaction/migration */
> +static DEFINE_BALLOON_MAPPING_AOPS(virtio_balloon_aops,
> + virtballoon_migratepage,
> + virtballoon_isolatepage,
> + virtballoon_putbackpage);
> +
> static int virtballoon_probe(struct virtio_device *vdev)
> {
> struct virtio_balloon *vb;
> + struct address_space *vb_mapping;
> int err;
>
> vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
> @@ -351,12 +521,24 @@ static int virtballoon_probe(struct virtio_device *vdev)
> }
>
> INIT_LIST_HEAD(&vb->pages);
> + spin_lock_init(&vb->pages_lock);
> + mutex_init(&vb->balloon_lock);
> +
> vb->num_pages = 0;
> init_waitqueue_head(&vb->config_change);
> init_waitqueue_head(&vb->acked);
> vb->vdev = vdev;
> vb->need_stats_update = 0;
>
> + /* Allocate a special page->mapping for this balloon device */
> + vb_mapping = alloc_balloon_mapping((void *)vb, &virtio_balloon_aops);
> + if (!vb_mapping) {
> + err = -ENOMEM;
> + goto out_free_vb;
> + }
> + /* Store the page->mapping reference for this balloon device */
> + vb->mapping = vb_mapping;
> +
> err = init_vqs(vb);
> if (err)
> goto out_free_vb;
> @@ -367,12 +549,14 @@ static int virtballoon_probe(struct virtio_device *vdev)
> goto out_del_vqs;
> }
>
> + vb->balloon_status = BALLOON_OK;
> return 0;
>
> out_del_vqs:
> vdev->config->del_vqs(vdev);
> out_free_vb:
> kfree(vb);
> + kfree(vb_mapping);
> out:
> return err;
> }
> @@ -394,8 +578,10 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
> {
> struct virtio_balloon *vb = vdev->priv;
>
> + vb->balloon_status = BALLOON_REMOVAL;
> kthread_stop(vb->thread);
> remove_common(vb);
> + kfree(vb->mapping);
> kfree(vb);
> }
>
> @@ -409,6 +595,7 @@ static int virtballoon_freeze(struct virtio_device *vdev)
> * function is called.
> */
>
> + vb->balloon_status = BALLOON_REMOVAL;
> remove_common(vb);
> return 0;
> }
> @@ -424,6 +611,7 @@ static int virtballoon_restore(struct virtio_device *vdev)
>
> fill_balloon(vb, towards_target(vb));
> update_balloon_size(vb);
> + vb->balloon_status = BALLOON_OK;
> return 0;
> }
> #endif
> --
> 1.7.11.4
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Peter Zijlstra @ 2012-08-21 15:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120821135223.GA7117@redhat.com>
On Tue, 2012-08-21 at 16:52 +0300, Michael S. Tsirkin wrote:
> > + rcu_read_lock();
> > + mapping = rcu_dereference(page->mapping);
> > + if (mapping_balloon(mapping))
> > + ret = true;
> > + rcu_read_unlock();
>
> This looks suspicious: you drop rcu_read_unlock
> so can't page switch from balloon to non balloon?
RCU read lock is a non-exclusive lock, it cannot avoid anything like
that.
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Peter Zijlstra @ 2012-08-21 15:20 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Andrew Morton, paulmck
In-Reply-To: <e24f3073ef539985dea52943dcb84762213a0857.1345519422.git.aquini@redhat.com>
On Tue, 2012-08-21 at 09:47 -0300, Rafael Aquini wrote:
> + mapping = rcu_access_pointer(page->mapping);
> + if (mapping)
> + mapping = mapping->assoc_mapping;
The comment near rcu_access_pointer() explicitly says:
* Return the value of the specified RCU-protected pointer, but omit the
* smp_read_barrier_depends() and keep the ACCESS_ONCE(). This is useful
* when the value of this pointer is accessed, but the pointer is not
* dereferenced,
Yet you dereference the pointer... smells like fail to me.
^ permalink raw reply
* Re: [PATCH v8 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Peter Zijlstra @ 2012-08-21 15:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120821144013.GA7784@redhat.com>
On Tue, 2012-08-21 at 17:40 +0300, Michael S. Tsirkin wrote:
> > + vb->balloon_status = BALLOON_REMOVAL;
>
> This needs some kind of barrier.
>
> > kthread_stop(vb->thread);
kthread_stop() implies an smp_wmb() [ because it needs to do a wakeup ].
^ permalink raw reply
* Re: [PATCH v8 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Peter Zijlstra @ 2012-08-21 15:37 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
paulmck
In-Reply-To: <20120821144013.GA7784@redhat.com>
On Tue, 2012-08-21 at 17:40 +0300, Michael S. Tsirkin wrote:
> > + spin_lock(&vb->pages_lock);
> > + page = list_first_or_null_rcu(&vb->pages, struct page, lru);
>
> Why is list_first_or_null_rcu called outside
> RCU critical section here?
It looks like vb->pages_lock is the exclusive (or modification)
counterpart to the rcu-read-lock in this particular case, so it should
be fine.
Although for that same reason, it seems superfluous to use the RCU list
method since we're exclusive with list manipulations anyway.
> > + if (!page) {
> > + spin_unlock(&vb->pages_lock);
> > + break;
> > + }
> > + /*
> > + * It is safe now to drop page->mapping and delete this page
> > + * from balloon page list, since we are grabbing 'pages_lock'
> > + * which prevents 'virtballoon_isolatepage()' from acting.
> > + */
> > + clear_balloon_mapping(page);
> > + list_del_rcu(&page->lru);
> > + spin_unlock(&vb->pages_lock);
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-21 15:41 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <1345562166.23018.109.camel@twins>
On Tue, Aug 21, 2012 at 05:16:06PM +0200, Peter Zijlstra wrote:
> On Tue, 2012-08-21 at 16:52 +0300, Michael S. Tsirkin wrote:
> > > + rcu_read_lock();
> > > + mapping = rcu_dereference(page->mapping);
> > > + if (mapping_balloon(mapping))
> > > + ret = true;
> > > + rcu_read_unlock();
> >
> > This looks suspicious: you drop rcu_read_unlock
> > so can't page switch from balloon to non balloon?
>
> RCU read lock is a non-exclusive lock, it cannot avoid anything like
> that.
You are right, of course. So even keeping rcu_read_lock across both test
and operation won't be enough - you need to make this function return
the mapping and pass it to isolate_page/putback_page so that it is only
dereferenced once.
--
MST
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox