* Re: [Xen-devel] [patch 3/3] xen/privcmd: remove const modifier from declaration
From: Jan Beulich @ 2012-09-10 9:03 UTC (permalink / raw)
To: Andres Lagar-Cavilla, Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, kernel-janitors,
Konrad Rzeszutek Wilk, virtualization
In-Reply-To: <20120908095808.GC608@elgon.mountain>
>>> On 08.09.12 at 11:58, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> When we use this pointer, we cast away the const modifier and modify the
> data. I think it was an accident to declare it as const.
NAK - the const is very valid here, as the v2 interface (as opposed
to the v1 one) does _not_ modify this array (or if it does, it's a
bug). This is a guarantee made to user mode, so it should also be
expressed that way in the interface.
But of course the cast used before this patch isn't right either, as
it indeed inappropriately discards the qualifier. Afaiu this was done
to simplify the internal workings of the code, but I don't think it's
desirable to sacrifice type safety for implementation simplicity.
Jan
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
> index a853168..58ed953 100644
> --- a/include/xen/privcmd.h
> +++ b/include/xen/privcmd.h
> @@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {
> unsigned int num; /* number of pages to populate */
> domid_t dom; /* target domain */
> __u64 addr; /* virtual address */
> - const xen_pfn_t __user *arr; /* array of mfns */
> + xen_pfn_t __user *arr; /* array of mfns */
> int __user *err; /* array of error codes */
> };
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 0ce006a..fceb83e 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata,
> int version)
>
> if (state.global_error && (version == 1)) {
> /* Write back errors in second pass. */
> - state.user_mfn = (xen_pfn_t *)m.arr;
> + state.user_mfn = m.arr;
> state.err = err_array;
> ret = traverse_pages(m.num, sizeof(xen_pfn_t),
> &pagelist, mmap_return_errors_v1, &state);
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply
* Re: [PATCH] Add a page cache-backed balloon device driver.
From: Michael S. Tsirkin @ 2012-09-10 9:05 UTC (permalink / raw)
To: Frank Swiderski
Cc: Andrea Arcangeli, riel, kvm, linux-kernel, virtualization, mikew
In-Reply-To: <1340742778-11282-1-git-send-email-fes@google.com>
On Tue, Jun 26, 2012 at 01:32:58PM -0700, Frank Swiderski wrote:
> This implementation of a virtio balloon driver uses the page cache to
> "store" pages that have been released to the host. The communication
> (outside of target counts) is one way--the guest notifies the host when
> it adds a page to the page cache, allowing the host to madvise(2) with
> MADV_DONTNEED. Reclaim in the guest is therefore automatic and implicit
> (via the regular page reclaim). This means that inflating the balloon
> is similar to the existing balloon mechanism, but the deflate is
> different--it re-uses existing Linux kernel functionality to
> automatically reclaim.
>
> Signed-off-by: Frank Swiderski <fes@google.com>
I've been trying to understand this, and I have
a question: what exactly is the benefit
of this new device?
Note that users could not care less about how a driver
is implemented internally.
Is there some workload where you see VM working better with
this than regular balloon? Any numbers?
Also, can't we just replace existing balloon implementation
with this one? Why it is so important to deflate silently?
I guess filesystem does not currently get a callback
before page is reclaimed but this isan implementation detail -
maybe this can be fixed?
Also can you pls answer Avi's question?
How is overcommit managed?
> ---
> drivers/virtio/Kconfig | 13 +
> drivers/virtio/Makefile | 1 +
> drivers/virtio/virtio_fileballoon.c | 636 +++++++++++++++++++++++++++++++++++
> include/linux/virtio_balloon.h | 9 +
> include/linux/virtio_ids.h | 1 +
> 5 files changed, 660 insertions(+), 0 deletions(-)
> create mode 100644 drivers/virtio/virtio_fileballoon.c
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index f38b17a..cffa2a7 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -35,6 +35,19 @@ config VIRTIO_BALLOON
>
> If unsure, say M.
>
> +config VIRTIO_FILEBALLOON
> + tristate "Virtio page cache-backed balloon driver"
> + select VIRTIO
> + select VIRTIO_RING
> + ---help---
> + This driver supports decreasing and automatically reclaiming the
> + memory within a guest VM. Unlike VIRTIO_BALLOON, this driver instead
> + tries to maintain a specific target balloon size using the page cache.
> + This allows the guest to implicitly deflate the balloon by flushing
> + pages from the cache and touching the page.
> +
> + If unsure, say N.
> +
> config VIRTIO_MMIO
> tristate "Platform bus driver for memory mapped virtio devices (EXPERIMENTAL)"
> depends on HAS_IOMEM && EXPERIMENTAL
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 5a4c63c..7ca0a3f 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_VIRTIO_RING) += virtio_ring.o
> obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> +obj-$(CONFIG_VIRTIO_FILEBALLOON) += virtio_fileballoon.o
> diff --git a/drivers/virtio/virtio_fileballoon.c b/drivers/virtio/virtio_fileballoon.c
> new file mode 100644
> index 0000000..ff252ec
> --- /dev/null
> +++ b/drivers/virtio/virtio_fileballoon.c
> @@ -0,0 +1,636 @@
> +/* Virtio file (page cache-backed) balloon implementation, inspired by
> + * Dor Loar and Marcelo Tosatti's implementations, and based on Rusty Russel's
> + * implementation.
> + *
> + * This implementation of the virtio balloon driver re-uses the page cache to
> + * allow memory consumed by inflating the balloon to be reclaimed by linux. It
> + * creates and mounts a bare-bones filesystem containing a single inode. When
> + * the host requests the balloon to inflate, it does so by "reading" pages at
> + * offsets into the inode mapping's page_tree. The host is notified when the
> + * pages are added to the page_tree, allowing it (the host) to madvise(2) the
> + * corresponding host memory, reducing the RSS of the virtual machine. In this
> + * implementation, the host is only notified when a page is added to the
> + * balloon. Reclaim happens under the existing TTFP logic, which flushes unused
> + * pages in the page cache. If the host used MADV_DONTNEED, then when the guest
> + * uses the page, the zero page will be mapped in, allowing automatic (and fast,
> + * compared to requiring a host notification via a virtio queue to get memory
> + * back) reclaim.
> + *
> + * Copyright 2008 Rusty Russell IBM Corporation
> + * Copyright 2011 Frank Swiderski Google Inc
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +#include <linux/backing-dev.h>
> +#include <linux/delay.h>
> +#include <linux/file.h>
> +#include <linux/freezer.h>
> +#include <linux/fs.h>
> +#include <linux/jiffies.h>
> +#include <linux/kthread.h>
> +#include <linux/module.h>
> +#include <linux/mount.h>
> +#include <linux/pagemap.h>
> +#include <linux/slab.h>
> +#include <linux/swap.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_balloon.h>
> +#include <linux/writeback.h>
> +
> +#define VIRTBALLOON_PFN_ARRAY_SIZE 256
> +
> +struct virtio_balloon {
> + struct virtio_device *vdev;
> + struct virtqueue *inflate_vq;
> +
> + /* Where the ballooning thread waits for config to change. */
> + wait_queue_head_t config_change;
> +
> + /* The thread servicing the balloon. */
> + struct task_struct *thread;
> +
> + /* Waiting for host to ack the pages we released. */
> + struct completion acked;
> +
> + /* The array of pfns we tell the Host about. */
> + unsigned int num_pfns;
> + u32 pfns[VIRTBALLOON_PFN_ARRAY_SIZE];
> +
> + struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
> +
> + /* The last page offset read into the mapping's page_tree */
> + unsigned long last_scan_page_array;
> +
> + /* The last time a page was reclaimed */
> + unsigned long last_reclaim;
> +};
> +
> +/* Magic number used for the skeleton filesystem in the call to mount_pseudo */
> +#define BALLOONFS_MAGIC 0x42414c4c
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_FILE_BALLOON, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> +/*
> + * The skeleton filesystem contains a single inode, held by the structure below.
> + * Using the containing structure below allows easy access to the struct
> + * virtio_balloon.
> + */
> +static struct balloon_inode {
> + struct inode inode;
> + struct virtio_balloon *vb;
> +} the_inode;
> +
> +/*
> + * balloon_alloc_inode is called when the single inode for the skeleton
> + * filesystem is created in init() with the call to new_inode.
> + */
> +static struct inode *balloon_alloc_inode(struct super_block *sb)
> +{
> + static bool already_inited;
> + /* We should only ever be called once! */
> + BUG_ON(already_inited);
> + already_inited = true;
> + inode_init_once(&the_inode.inode);
> + return &the_inode.inode;
> +}
> +
> +/* Noop implementation of destroy_inode. */
> +static void balloon_destroy_inode(struct inode *inode)
> +{
> +}
> +
> +static int balloon_sync_fs(struct super_block *sb, int wait)
> +{
> + return filemap_write_and_wait(the_inode.inode.i_mapping);
> +}
> +
> +static const struct super_operations balloonfs_ops = {
> + .alloc_inode = balloon_alloc_inode,
> + .destroy_inode = balloon_destroy_inode,
> + .sync_fs = balloon_sync_fs,
> +};
> +
> +static const struct dentry_operations balloonfs_dentry_operations = {
> +};
> +
> +/*
> + * balloonfs_writepage is called when linux needs to reclaim memory held using
> + * the balloonfs' page cache.
> + */
> +static int balloonfs_writepage(struct page *page, struct writeback_control *wbc)
> +{
> + the_inode.vb->last_reclaim = jiffies;
> + SetPageUptodate(page);
> + ClearPageDirty(page);
> + /*
> + * If the page isn't being flushed from the page allocator, go ahead and
> + * drop it from the page cache anyway.
> + */
> + if (!wbc->for_reclaim)
> + delete_from_page_cache(page);
> + unlock_page(page);
> + return 0;
> +}
> +
> +/* Nearly no-op implementation of readpage */
> +static int balloonfs_readpage(struct file *file, struct page *page)
> +{
> + SetPageUptodate(page);
> + unlock_page(page);
> + return 0;
> +}
> +
> +static const struct address_space_operations balloonfs_aops = {
> + .writepage = balloonfs_writepage,
> + .readpage = balloonfs_readpage
> +};
> +
> +static struct backing_dev_info balloonfs_backing_dev_info = {
> + .name = "balloonfs",
> + .ra_pages = 0,
> + .capabilities = BDI_CAP_NO_ACCT_AND_WRITEBACK
> +};
> +
> +static struct dentry *balloonfs_mount(struct file_system_type *fs_type,
> + int flags, const char *dev_name, void *data)
> +{
> + struct dentry *root;
> + struct inode *inode;
> + root = mount_pseudo(fs_type, "balloon:", &balloonfs_ops,
> + &balloonfs_dentry_operations, BALLOONFS_MAGIC);
> + inode = root->d_inode;
> + inode->i_mapping->a_ops = &balloonfs_aops;
> + mapping_set_gfp_mask(inode->i_mapping,
> + (GFP_HIGHUSER | __GFP_NOMEMALLOC));
> + inode->i_mapping->backing_dev_info = &balloonfs_backing_dev_info;
> + return root;
> +}
> +
> +/* The single mounted skeleton filesystem */
> +static struct vfsmount *balloon_mnt __read_mostly;
> +
> +static struct file_system_type balloon_fs_type = {
> + .name = "balloonfs",
> + .mount = balloonfs_mount,
> + .kill_sb = kill_anon_super,
> +};
> +
> +/* Acknowledges a message from the specified virtqueue. */
> +static void balloon_ack(struct virtqueue *vq)
> +{
> + struct virtio_balloon *vb;
> + unsigned int len;
> +
> + vb = virtqueue_get_buf(vq, &len);
> + if (vb)
> + complete(&vb->acked);
> +}
> +
> +/*
> + * Scans the page_tree for the inode's mapping, looking for an offset that is
> + * currently empty, returning that index (or 0 if it could not fill the
> + * request).
> + */
> +static unsigned long find_available_inode_page(struct virtio_balloon *vb)
> +{
> + unsigned long radix_index, index, max_scan;
> + struct address_space *mapping = the_inode.inode.i_mapping;
> +
> + /*
> + * This function is a serialized call (only happens on the free-to-host
> + * thread), so no locking is necessary here.
> + */
> + index = vb->last_scan_page_array;
> + max_scan = totalram_pages - vb->last_scan_page_array;
> +
> + /*
> + * Scan starting at the last scanned offset, then wrap around if
> + * necessary.
> + */
> + if (index == 0)
> + index = 1;
> + rcu_read_lock();
> + radix_index = radix_tree_next_hole(&mapping->page_tree,
> + index, max_scan);
> + rcu_read_unlock();
> + /*
> + * If we hit the end of the tree, wrap and search up to the original
> + * index.
> + */
> + if (radix_index - index >= max_scan) {
> + if (index != 1) {
> + rcu_read_lock();
> + radix_index = radix_tree_next_hole(&mapping->page_tree,
> + 1, index);
> + rcu_read_unlock();
> + if (radix_index - 1 >= index)
> + radix_index = 0;
> + } else {
> + radix_index = 0;
> + }
> + }
> + vb->last_scan_page_array = radix_index;
> +
> + return radix_index;
> +}
> +
> +/* Notifies the host of pages in the specified virtqueue. */
> +static int tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
> +{
> + int err;
> + struct scatterlist sg;
> +
> + sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> +
> + init_completion(&vb->acked);
> +
> + /* We should always be able to add one buffer to an empty queue. */
> + err = virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL);
> + if (err < 0)
> + return err;
> + virtqueue_kick(vq);
> +
> + /* When host has read buffer, this completes via balloon_ack */
> + wait_for_completion(&vb->acked);
> + return err;
> +}
> +
> +static void fill_balloon(struct virtio_balloon *vb, size_t num)
> +{
> + int err;
> +
> + /* We can only do one array worth at a time. */
> + num = min(num, ARRAY_SIZE(vb->pfns));
> +
> + for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
> + struct page *page;
> + unsigned long inode_pfn = find_available_inode_page(vb);
> + /* Should always be able to find a page. */
> + BUG_ON(!inode_pfn);
> + page = read_mapping_page(the_inode.inode.i_mapping, inode_pfn,
> + NULL);
> + if (IS_ERR(page)) {
> + if (printk_ratelimit())
> + dev_printk(KERN_INFO, &vb->vdev->dev,
> + "Out of puff! Can't get %zu pages\n",
> + num);
> + break;
> + }
> +
> + /* Set the page to be dirty */
> + set_page_dirty(page);
> +
> + vb->pfns[vb->num_pfns] = page_to_pfn(page);
> + }
> +
> + /* Didn't get any? Oh well. */
> + if (vb->num_pfns == 0)
> + return;
> +
> + /* Notify the host of the pages we just added to the page_tree. */
> + err = tell_host(vb, vb->inflate_vq);
> +
> + for (; vb->num_pfns != 0; vb->num_pfns--) {
> + struct page *page = pfn_to_page(vb->pfns[vb->num_pfns - 1]);
> + /*
> + * Release our refcount on the page so that it can be reclaimed
> + * when necessary.
> + */
> + page_cache_release(page);
> + }
> + __mark_inode_dirty(&the_inode.inode, I_DIRTY_PAGES);
> +}
> +
> +static inline void update_stat(struct virtio_balloon *vb, int idx,
> + u64 val)
> +{
> + BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
> + vb->stats[idx].tag = idx;
> + vb->stats[idx].val = val;
> +}
> +
> +#define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
> +
> +static inline u32 config_pages(struct virtio_balloon *vb);
> +static void update_balloon_stats(struct virtio_balloon *vb)
> +{
> + unsigned long events[NR_VM_EVENT_ITEMS];
> + struct sysinfo i;
> +
> + all_vm_events(events);
> + si_meminfo(&i);
> +
> + update_stat(vb, VIRTIO_BALLOON_S_SWAP_IN,
> + pages_to_bytes(events[PSWPIN]));
> + update_stat(vb, VIRTIO_BALLOON_S_SWAP_OUT,
> + pages_to_bytes(events[PSWPOUT]));
> + update_stat(vb, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
> + update_stat(vb, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> +
> + /* Total and Free Mem */
> + update_stat(vb, VIRTIO_BALLOON_S_MEMFREE, pages_to_bytes(i.freeram));
> + update_stat(vb, VIRTIO_BALLOON_S_MEMTOT, pages_to_bytes(i.totalram));
> +}
> +
> +static void virtballoon_changed(struct virtio_device *vdev)
> +{
> + struct virtio_balloon *vb = vdev->priv;
> +
> + wake_up(&vb->config_change);
> +}
> +
> +static inline bool config_need_stats(struct virtio_balloon *vb)
> +{
> + u32 v = 0;
> +
> + vb->vdev->config->get(vb->vdev,
> + offsetof(struct virtio_balloon_config,
> + need_stats),
> + &v, sizeof(v));
> + return (v != 0);
> +}
> +
> +static inline u32 config_pages(struct virtio_balloon *vb)
> +{
> + u32 v = 0;
> +
> + vb->vdev->config->get(vb->vdev,
> + offsetof(struct virtio_balloon_config, num_pages),
> + &v, sizeof(v));
> + return v;
> +}
> +
> +static inline s64 towards_target(struct virtio_balloon *vb)
> +{
> + struct address_space *mapping = the_inode.inode.i_mapping;
> + u32 v = config_pages(vb);
> +
> + return (s64)v - (mapping ? mapping->nrpages : 0);
> +}
> +
> +static void update_balloon_size(struct virtio_balloon *vb)
> +{
> + struct address_space *mapping = the_inode.inode.i_mapping;
> + __le32 actual = cpu_to_le32((mapping ? mapping->nrpages : 0));
> +
> + vb->vdev->config->set(vb->vdev,
> + offsetof(struct virtio_balloon_config, actual),
> + &actual, sizeof(actual));
> +}
> +
> +static void update_free_and_total(struct virtio_balloon *vb)
> +{
> + struct sysinfo i;
> + u32 value;
> +
> + si_meminfo(&i);
> +
> + update_balloon_stats(vb);
> + value = i.totalram;
> + vb->vdev->config->set(vb->vdev,
> + offsetof(struct virtio_balloon_config,
> + pages_total),
> + &value, sizeof(value));
> + value = i.freeram;
> + vb->vdev->config->set(vb->vdev,
> + offsetof(struct virtio_balloon_config,
> + pages_free),
> + &value, sizeof(value));
> + value = 0;
> + vb->vdev->config->set(vb->vdev,
> + offsetof(struct virtio_balloon_config,
> + need_stats),
> + &value, sizeof(value));
> +}
> +
> +static int balloon(void *_vballoon)
> +{
> + struct virtio_balloon *vb = _vballoon;
> +
> + set_freezable();
> + while (!kthread_should_stop()) {
> + s64 diff;
> + try_to_freeze();
> + wait_event_interruptible(vb->config_change,
> + (diff = towards_target(vb)) > 0
> + || config_need_stats(vb)
> + || kthread_should_stop()
> + || freezing(current));
> + if (config_need_stats(vb))
> + update_free_and_total(vb);
> + if (diff > 0) {
> + unsigned long reclaim_time = vb->last_reclaim + 2 * HZ;
> + /*
> + * Don't fill the balloon if a page reclaim happened in
> + * the past 2 seconds.
> + */
> + if (time_after_eq(reclaim_time, jiffies)) {
> + /* Inflating too fast--sleep and skip. */
> + msleep(500);
> + } else {
> + fill_balloon(vb, diff);
> + }
> + } else if (diff < 0 && config_pages(vb) == 0) {
> + /*
> + * Here we are specifically looking to detect the case
> + * where there are pages in the page cache, but the
> + * device wants us to go to 0. This is used in save/
> + * restore since the host device doesn't keep track of
> + * PFNs, and must flush the page cache on restore
> + * (which loses the context of the original device
> + * instance). However, we still suggest syncing the
> + * diff so that we can get within the target range.
> + */
> + s64 nr_to_write =
> + (!config_pages(vb) ? LONG_MAX : -diff);
> + struct writeback_control wbc = {
> + .sync_mode = WB_SYNC_ALL,
> + .nr_to_write = nr_to_write,
> + .range_start = 0,
> + .range_end = LLONG_MAX,
> + };
> + sync_inode(&the_inode.inode, &wbc);
> + }
> + update_balloon_size(vb);
> + }
> + return 0;
> +}
> +
> +static ssize_t virtballoon_attr_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf);
> +
> +static DEVICE_ATTR(total_memory, 0644,
> + virtballoon_attr_show, NULL);
> +
> +static DEVICE_ATTR(free_memory, 0644,
> + virtballoon_attr_show, NULL);
> +
> +static DEVICE_ATTR(target_pages, 0644,
> + virtballoon_attr_show, NULL);
> +
> +static DEVICE_ATTR(actual_pages, 0644,
> + virtballoon_attr_show, NULL);
> +
> +static struct attribute *virtballoon_attrs[] = {
> + &dev_attr_total_memory.attr,
> + &dev_attr_free_memory.attr,
> + &dev_attr_target_pages.attr,
> + &dev_attr_actual_pages.attr,
> + NULL
> +};
> +static struct attribute_group virtballoon_attr_group = {
> + .name = "virtballoon",
> + .attrs = virtballoon_attrs,
> +};
> +
> +static ssize_t virtballoon_attr_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct address_space *mapping = the_inode.inode.i_mapping;
> + struct virtio_device *vdev = container_of(dev, struct virtio_device,
> + dev);
> + struct virtio_balloon *vb = vdev->priv;
> + unsigned long long value = 0;
> + if (attr == &dev_attr_total_memory)
> + value = vb->stats[VIRTIO_BALLOON_S_MEMTOT].val;
> + else if (attr == &dev_attr_free_memory)
> + value = vb->stats[VIRTIO_BALLOON_S_MEMFREE].val;
> + else if (attr == &dev_attr_target_pages)
> + value = config_pages(vb);
> + else if (attr == &dev_attr_actual_pages)
> + value = cpu_to_le32((mapping ? mapping->nrpages : 0));
> + return sprintf(buf, "%llu\n", value);
> +}
> +
> +static int virtballoon_probe(struct virtio_device *vdev)
> +{
> + struct virtio_balloon *vb;
> + struct virtqueue *vq[1];
> + vq_callback_t *callback = balloon_ack;
> + const char *name = "inflate";
> + int err;
> +
> + vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
> + if (!vb) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + init_waitqueue_head(&vb->config_change);
> + vb->vdev = vdev;
> +
> + /* We use one virtqueue: inflate */
> + err = vdev->config->find_vqs(vdev, 1, vq, &callback, &name);
> + if (err)
> + goto out_free_vb;
> +
> + vb->inflate_vq = vq[0];
> +
> + err = sysfs_create_group(&vdev->dev.kobj, &virtballoon_attr_group);
> + if (err) {
> + pr_err("Failed to create virtballoon sysfs node\n");
> + goto out_free_vb;
> + }
> +
> + vb->last_scan_page_array = 0;
> + vb->last_reclaim = 0;
> + the_inode.vb = vb;
> +
> + vb->thread = kthread_run(balloon, vb, "vballoon");
> + if (IS_ERR(vb->thread)) {
> + err = PTR_ERR(vb->thread);
> + goto out_del_vqs;
> + }
> +
> + return 0;
> +
> +out_del_vqs:
> + vdev->config->del_vqs(vdev);
> +out_free_vb:
> + kfree(vb);
> +out:
> + return err;
> +}
> +
> +static void __devexit virtballoon_remove(struct virtio_device *vdev)
> +{
> + struct virtio_balloon *vb = vdev->priv;
> +
> + kthread_stop(vb->thread);
> +
> + sysfs_remove_group(&vdev->dev.kobj, &virtballoon_attr_group);
> +
> + /* Now we reset the device so we can clean up the queues. */
> + vdev->config->reset(vdev);
> +
> + vdev->config->del_vqs(vdev);
> + kfree(vb);
> +}
> +
> +static struct virtio_driver virtio_balloon_driver = {
> + .feature_table = NULL,
> + .feature_table_size = 0,
> + .driver.name = KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .id_table = id_table,
> + .probe = virtballoon_probe,
> + .remove = __devexit_p(virtballoon_remove),
> + .config_changed = virtballoon_changed,
> +};
> +
> +static int __init init(void)
> +{
> + int err = register_filesystem(&balloon_fs_type);
> + if (err)
> + goto out;
> +
> + balloon_mnt = kern_mount(&balloon_fs_type);
> + if (IS_ERR(balloon_mnt)) {
> + err = PTR_ERR(balloon_mnt);
> + goto out_filesystem;
> + }
> +
> + err = register_virtio_driver(&virtio_balloon_driver);
> + if (err)
> + goto out_filesystem;
> +
> + goto out;
> +
> +out_filesystem:
> + unregister_filesystem(&balloon_fs_type);
> +
> +out:
> + return err;
> +}
> +
> +static void __exit fini(void)
> +{
> + if (balloon_mnt) {
> + unregister_filesystem(&balloon_fs_type);
> + balloon_mnt = NULL;
> + }
> + unregister_virtio_driver(&virtio_balloon_driver);
> +}
> +module_init(init);
> +module_exit(fini);
> +
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +MODULE_DESCRIPTION("Virtio file (page cache-backed) balloon driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
> index 652dc8b..2be9a02 100644
> --- a/include/linux/virtio_balloon.h
> +++ b/include/linux/virtio_balloon.h
> @@ -41,6 +41,15 @@ struct virtio_balloon_config
> __le32 num_pages;
> /* Number of pages we've actually got in balloon. */
> __le32 actual;
> +#if defined(CONFIG_VIRTIO_FILEBALLOON) ||\
> + defined(CONFIG_VIRTIO_FILEBALLOON_MODULE)
> + /* Total pages on this system. */
> + __le32 pages_total;
> + /* Free pages on this system. */
> + __le32 pages_free;
> + /* If the device needs pages_total/pages_free updated. */
> + __le32 need_stats;
> +#endif
> };
>
> #define VIRTIO_BALLOON_S_SWAP_IN 0 /* Amount of memory swapped in */
> diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
> index 7529b85..2f081d7 100644
> --- a/include/linux/virtio_ids.h
> +++ b/include/linux/virtio_ids.h
> @@ -37,5 +37,6 @@
> #define VIRTIO_ID_RPMSG 7 /* virtio remote processor messaging */
> #define VIRTIO_ID_SCSI 8 /* virtio scsi */
> #define VIRTIO_ID_9P 9 /* 9p virtio console */
> +#define VIRTIO_ID_FILE_BALLOON 10 /* virtio file-backed balloon */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
> --
> 1.7.7.3
^ permalink raw reply
* [patch 3/3 v2] xen/privcmd: add a __user annotation to a cast
From: Dan Carpenter @ 2012-09-10 10:21 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: kernel-janitors, Jeremy Fitzhardinge, xen-devel, virtualization,
Konrad Rzeszutek Wilk
In-Reply-To: <504DC8FE020000780009A190@nat28.tlf.novell.com>
Sparse complains that we lose the __user annotation in the cast here.
drivers/xen/privcmd.c:388:35: warning: cast removes address space of expression
drivers/xen/privcmd.c:388:32: warning: incorrect type in assignment (different address spaces)
drivers/xen/privcmd.c:388:32: expected unsigned long [noderef] [usertype] <asn:1>*[addressable] [assigned] user_mfn
drivers/xen/privcmd.c:388:32: got unsigned long [usertype] *<noident>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: In v1 I removed the const from the declaration but now I just removed it
from the cast. This data can either be a v1 or a v2 type struct. The m.arr
data is const in version 2 but not in version 1.
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 0ce006a..fceb83e 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
if (state.global_error && (version == 1)) {
/* Write back errors in second pass. */
- state.user_mfn = (xen_pfn_t *)m.arr;
+ state.user_mfn = (xen_pfn_t __user *)m.arr;
state.err = err_array;
ret = traverse_pages(m.num, sizeof(xen_pfn_t),
&pagelist, mmap_return_errors_v1, &state);
^ permalink raw reply related
* Re: [Xen-devel] [patch 1/3] xen/privcmd: check for integer overflow in ioctl
From: David Vrabel @ 2012-09-10 10:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
In-Reply-To: <20120908095208.GA608@elgon.mountain>
On 08/09/12 10:52, Dan Carpenter wrote:
> If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
> could overflow and the access_ok() check wouldn't test the right size.
m.num is range checked later on so it doesn't matter that the
access_ok() checks might be wrong. A bit subtle, perhaps.
David
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Only needed in linux-next.
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 215a3c0..fdff8f9 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -325,6 +325,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> return -EFAULT;
> /* Returns per-frame error in m.arr. */
> m.err = NULL;
> + if (m.num > SIZE_MAX / sizeof(*m.arr))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
> return -EFAULT;
> break;
> @@ -332,6 +334,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
> return -EFAULT;
> /* Returns per-frame error code in m.err. */
> + if (m.num > SIZE_MAX / sizeof(*m.err))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
> return -EFAULT;
> break;
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply
* Re: [Xen-devel] [patch 3/3] xen/privcmd: remove const modifier from declaration
From: David Vrabel @ 2012-09-10 10:43 UTC (permalink / raw)
To: Jan Beulich
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization,
Dan Carpenter
In-Reply-To: <504DC8FE020000780009A190@nat28.tlf.novell.com>
On 10/09/12 10:03, Jan Beulich wrote:
>>>> On 08.09.12 at 11:58, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> When we use this pointer, we cast away the const modifier and modify the
>> data. I think it was an accident to declare it as const.
>
> NAK - the const is very valid here, as the v2 interface (as opposed
> to the v1 one) does _not_ modify this array (or if it does, it's a
> bug). This is a guarantee made to user mode, so it should also be
> expressed that way in the interface.
>
> But of course the cast used before this patch isn't right either, as
> it indeed inappropriately discards the qualifier. Afaiu this was done
> to simplify the internal workings of the code, but I don't think it's
> desirable to sacrifice type safety for implementation simplicity.
m.arr here isn't const as m is really the V1 structure where m.arr is
non-const.
David
^ permalink raw reply
* Re: [PATCHv4] virtio-spec: virtio network device multiqueue support
From: Jason Wang @ 2012-09-10 11:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: rick.jones2, kvm, netdev, virtualization, levinsasha928, pbonzini,
Tom Herbert
In-Reply-To: <20120910063346.GA17338@redhat.com>
On 09/10/2012 02:33 PM, Michael S. Tsirkin wrote:
> On Mon, Sep 10, 2012 at 09:27:38AM +0300, Michael S. Tsirkin wrote:
>> On Mon, Sep 10, 2012 at 09:16:29AM +0300, Michael S. Tsirkin wrote:
>>> On Mon, Sep 10, 2012 at 11:42:25AM +0930, Rusty Russell wrote:
>>>> OK, I read the spec (pasted below for easy of reading), but I'm still
>>>> confused over how this will work.
>>>>
>>>> I thought normal net drivers have the hardware provide an rxhash for
>>>> each packet, and we map that to CPU to queue the packet on[1]. We hope
>>>> that the receiving process migrates to that CPU, so xmit queue
>>>> matches.
>>> This ony works sometimes. For example it's common to pin netperf to a
>>> cpu to get consistent performance. Proper hardware must obey what
>>> applications want it to do, not the other way around.
>>>
>>>> For virtio this would mean a new per-packet rxhash value, right?
>>>>
>>>> Why are we doing something different? What am I missing?
>>>>
>>>> Thanks,
>>>> Rusty.
>>>> [1] Everything I Know About Networking I Learned From LWN:
>>>> https://lwn.net/Articles/362339/
>>> I think you missed this:
>>>
>>> Some network interfaces can help with the distribution of incoming
>>> packets; they have multiple receive queues and multiple interrupt lines.
>>> Others, though, are equipped with a single queue, meaning that the
>>> driver for that hardware must deal with all incoming packets in a
>>> single, serialized stream. Parallelizing such a stream requires some
>>> intelligence on the part of the host operating system.
>>>
>>> In other words RPS is a hack to speed up networking on cheapo
>>> hardware, this is one of the reasons it is off by default.
>>> Good hardware has multiple receive queues.
>>> We can implement a good one so we do not need RPS.
>>>
>>> Also not all guest OS-es support RPS.
>>>
>>> Does this clarify?
>> I would like to add that on many processors, sending
>> IPCs between guest CPUs requires exits on sending *and*
>> receiving path, making it very expensive.
> A final addition: what you suggest above would be
> "TX follows RX", right?
> It is in anticipation of something like that, that I made
> steering programming so generic.
> I think TX follows RX is more immediately useful for reasons above
> but we can add both to spec and let drivers and devices
> decide what they want to support.
> Pls let me know.
AFAIK, ixgbe does "rx follows tx". The only differences between ixgbe
and virtio-net is that ixgbe driver programs the flow director during
packet transmission but we suggest to do it silently in the device for
simplicity. Even with this, more co-operation is still needed for the
driver ( e.g ixgbe try to use per-cpu queue by setting affinity hint and
using cpuid to choose the txq which could be reused in virtio-net driver).
>
>>>> ---
>>>> Transmit Packet Steering
>>>>
>>>> When VIRTIO_NET_F_MULTIQUEUE feature bit is negotiated, guest can use any of multiple configured transmit queues to transmit a given packet. To avoid packet reordering by device (which generally leads to performance degradation) driver should attempt to utilize the same transmit virtqueue for all packets of a given transmit flow. For bi-directional protocols (in practice, TCP), a given network connection can utilize both transmit and receive queues. For best performance, packets from a single connection should utilize the paired transmit and receive queues from the same virtqueue pair; for example both transmitqN and receiveqN. This rule makes it possible to optimize processing on the device side, but this is not a hard requirement: devices should function correctly even when this rul
e is not followed.
>>>>
>>>> Driver selects an active steering rule using VIRTIO_NET_CTRL_STEERING command (this controls both which virtqueue is selected for a given packet for receive and notifies the device which virtqueues are about to be used for transmit).
>>>>
>>>> This command accepts a single out argument in the following format:
>>>>
>>>> #define VIRTIO_NET_CTRL_STEERING 4
>>>>
>>>> The field rule specifies the function used to select transmit virtqueue for a given packet; the field param makes it possible to pass an extra parameter if appropriate. When rule is set to VIRTIO_NET_CTRL_STEERING_SINGLE (this is the default) all packets are steered to the default virtqueue transmitq (1); param is unused; this is the default. With any other rule, When rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by driver to the first N=(param+1) multiqueue virtqueues transmitq1...transmitqN; the default transmitq is unused. Driver must have configured all these (param+1) virtqueues beforehand.
>>>>
>>>> Supported steering rules can be added and removed in the future. Driver should check that the request to change the steering rule was successful by checking ack values of the command. As selecting a specific steering is an optimization feature, drivers should avoid hard failure and fall back on using a supported steering rule if this command fails. The default steering rule is VIRTIO_NET_CTRL_STEERING_SINGLE. It will not be removed.
>>>>
>>>> When the steering rule is modified, some packets can still be outstanding in one or more of the transmit virtqueues. Since drivers might choose to modify the current steering rule at a high rate (e.g. adaptively in response to changes in the workload) to avoid reordering packets, device is recommended to complete processing of the transmit queue(s) utilized by the original steering before processing any packets delivered by the modified steering rule.
>>>>
>>>> For debugging, the current steering rule can also be read from the configuration space.
>>>>
>>>> Receive Packet Steering
>>>>
>>>> When VIRTIO_NET_F_MULTIQUEUE feature bit is negotiated, device can use any of multiple configured receive queues to pass a given packet to driver. Driver controls which virtqueue is selected in practice by configuring packet steering rule using VIRTIO_NET_CTRL_STEERING command, as described above[sub:Transmit-Packet-Steering].
>>>>
>>>> The field rule specifies the function used to select receive virtqueue for a given packet; the field param makes it possible to pass an extra parameter if appropriate. When rule is set to VIRTIO_NET_CTRL_STEERING_SINGLE all packets are steered to the default virtqueue receiveq (0); param is unused; this is the default. When rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by host to the first N=(param+1) multiqueue virtqueues receiveq1...receiveqN; the default receiveq is unused. Driver must have configured all these (param+1) virtqueues beforehand. For best performance for bi-directional flows (such as TCP) device should detect the flow to virtqueue pair mapping on transmit and select the receive virtqueue from the same virtqueue pair. For uni-directional flo
ws, or when this mapping information is missing, a device-specific steering function is used.
>>>>
>>>> Supported steering rules can be added and removed in the future. Driver should probe for supported rules by checking ack values of the command.
>>>>
>>>> When the steering rule is modified, some packets can still be outstanding in one or more of the virtqueues. Device is not required to wait for these packets to be consumed before delivering packets using the new streering rule. Drivers modifying the steering rule at a high rate (e.g. adaptively in response to changes in the workload) are recommended to complete processing of the receive queue(s) utilized by the original steering before processing any packets delivered by the modified steering rule.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Xen-devel] [patch 1/3] xen/privcmd: check for integer overflow in ioctl
From: Dan Carpenter @ 2012-09-10 11:20 UTC (permalink / raw)
To: David Vrabel
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
In-Reply-To: <504DC25F.7000508@citrix.com>
On Mon, Sep 10, 2012 at 11:35:11AM +0100, David Vrabel wrote:
> On 08/09/12 10:52, Dan Carpenter wrote:
> > If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
> > could overflow and the access_ok() check wouldn't test the right size.
>
> m.num is range checked later on so it doesn't matter that the
> access_ok() checks might be wrong. A bit subtle, perhaps.
>
Yeah. It's too subtle for my static checker but not so subtle for
a human being. Laziness on my part.
Please drop this patch.
regards,
dan carpenter
^ permalink raw reply
* [RFCv3] virtio_console: Add support for virtio remoteproc serial
From: sjur.brandeland @ 2012-09-10 15:18 UTC (permalink / raw)
To: Rusty Russell, Michael S . Tsirkin
Cc: Sjur Brændeland, Linus Walleij, linux-kernel, virtualization,
Amit Shah, Sjur Brændeland
In-Reply-To: <87zk4ye2jz.fsf@rustcorp.com.au>
From: Sjur Brændeland <sjur.brandeland@stericsson.com>
Add a virtio remoteproc serial driver: VIRTIO_ID_RPROC_SERIAL (0xB)
for communicating with a remote processor in an asymmetric
multi-processing configuration.
The virtio remoteproc serial driver reuses the existing virtio_console
implementation, and adds support for DMA allocation of data buffers
but disables support for tty console, mutiple ports, and the control queue.
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Amit Shah <amit.shah@redhat.com>
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: Linus Walleij <linus.walleij@linaro.org>
cc: virtualization@lists.linux-foundation.org
cc: linux-kernel@vger.kernel.org
---
Hi Rusty,
...
>Sorry for the back and forth, I've been pondering MST's points.
>If we make a new dma-multiport device (eg. ID 11), how ugly is the code?
I don't think it's too bad.
It's a few more code lines - as I have added a new virtio_driver
definition and feature table. The rest is just replacing the check
on feature bits with driver type. I no longer need to check on
feature bits in the probe function, as the match on device type is
done automatically by the driver framework.
I actually like this new approach better.
It solves the issues Michael has pointed out, and we don't have to
think through side effects of weired combination of feature bits.
>It would be a virtio console with DMA buffers and no console, just the
>multiport stuff. This would have no impact on the current spec for
>virtio console.
Yes, and this way it will also be easier to explain what this new
feature does.
Regards,
Sjur
drivers/char/virtio_console.c | 133 +++++++++++++++++++++++++++++++++--------
include/linux/virtio_ids.h | 1 +
2 files changed, 109 insertions(+), 25 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cdf2f54..08593aa 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -35,6 +35,7 @@
#include <linux/wait.h>
#include <linux/workqueue.h>
#include <linux/module.h>
+#include <linux/dma-mapping.h>
#include "../tty/hvc/hvc_console.h"
/*
@@ -323,6 +324,52 @@ static bool is_console_port(struct port *port)
return false;
}
+#ifdef CONFIG_HAS_DMA
+static inline bool is_rproc_serial(struct virtio_device *vdev)
+{
+ return vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
+}
+#else
+static inline bool is_rproc_serial(struct virtio_device *vdev)
+{
+ return false;
+}
+#endif
+
+/* Allocate data buffer from DMA memory if requested */
+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, gfp_t flag)
+{
+ if (is_rproc_serial(vdev)) {
+ dma_addr_t dma;
+ struct device *dev = &vdev->dev;
+ /*
+ * Allocate DMA memory from ancestors. Finding the ancestor
+ * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
+ * implemented.
+ */
+ dev = dev->parent ? dev->parent : dev;
+ dev = dev->parent ? dev->parent : dev;
+ return dma_alloc_coherent(dev, size, &dma, flag);
+ }
+ return kzalloc(size, flag);
+}
+
+static inline void
+free_databuf(struct virtio_device *vdev, size_t size, void *cpu_addr)
+{
+
+ if (is_rproc_serial(vdev)) {
+ struct device *dev = &vdev->dev;
+ dma_addr_t dma_handle = virt_to_bus(cpu_addr);
+ dev = dev->parent ? dev->parent : dev;
+ dev = dev->parent ? dev->parent : dev;
+ dma_free_coherent(dev, size, cpu_addr, dma_handle);
+ return;
+ }
+ kfree(cpu_addr);
+}
+
static inline bool use_multiport(struct ports_device *portdev)
{
/*
@@ -334,20 +381,21 @@ static inline bool use_multiport(struct ports_device *portdev)
return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
}
-static void free_buf(struct port_buffer *buf)
+static void
+free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
{
- kfree(buf->buf);
+ free_databuf(vq->vdev, buf_size, buf->buf);
kfree(buf);
}
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size)
{
struct port_buffer *buf;
buf = kmalloc(sizeof(*buf), GFP_KERNEL);
if (!buf)
goto fail;
- buf->buf = kzalloc(buf_size, GFP_KERNEL);
+ buf->buf = alloc_databuf(vq->vdev, buf_size, GFP_KERNEL);
if (!buf->buf)
goto free_buf;
buf->len = 0;
@@ -414,7 +462,7 @@ static void discard_port_data(struct port *port)
port->stats.bytes_discarded += buf->len - buf->offset;
if (add_inbuf(port->in_vq, buf) < 0) {
err++;
- free_buf(buf);
+ free_buf(port->in_vq, buf, PAGE_SIZE);
}
port->inbuf = NULL;
buf = get_inbuf(port);
@@ -485,7 +533,7 @@ static void reclaim_consumed_buffers(struct port *port)
return;
}
while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
- kfree(buf);
+ free_databuf(port->portdev->vdev, len, buf);
port->outvq_full = false;
}
}
@@ -672,6 +720,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
char *buf;
ssize_t ret;
bool nonblock;
+ struct virtio_device *vdev;
/* Userspace could be out to fool us */
if (!count)
@@ -694,9 +743,10 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
if (!port->guest_connected)
return -ENODEV;
+ vdev = port->portdev->vdev;
count = min((size_t)(32 * 1024), count);
- buf = kmalloc(count, GFP_KERNEL);
+ buf = alloc_databuf(vdev, count, GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -720,7 +770,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
goto out;
free_buf:
- kfree(buf);
+ free_databuf(vdev, count, buf);
out:
return ret;
}
@@ -918,7 +968,8 @@ static void resize_console(struct port *port)
return;
vdev = port->portdev->vdev;
- if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+ if (!is_rproc_serial(vdev) &&
+ virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
hvc_resize(port->cons.hvc, port->cons.ws);
}
@@ -1102,7 +1153,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
nr_added_bufs = 0;
do {
- buf = alloc_buf(PAGE_SIZE);
+ buf = alloc_buf(vq, PAGE_SIZE);
if (!buf)
break;
@@ -1110,7 +1161,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
ret = add_inbuf(vq, buf);
if (ret < 0) {
spin_unlock_irq(lock);
- free_buf(buf);
+ free_buf(vq, buf, PAGE_SIZE);
break;
}
nr_added_bufs++;
@@ -1198,10 +1249,18 @@ static int add_port(struct ports_device *portdev, u32 id)
goto free_device;
}
- /*
- * If we're not using multiport support, this has to be a console port
- */
- if (!use_multiport(port->portdev)) {
+ if (is_rproc_serial(port->portdev->vdev))
+ /*
+ * For rproc_serial assume remote processor is connected.
+ * rproc_serial does not want the console port, but
+ * the generic port implementation.
+ */
+ port->host_connected = true;
+ else if (!use_multiport(port->portdev)) {
+ /*
+ * If we're not using multiport support,
+ * this has to be a console port.
+ */
err = init_port_console(port);
if (err)
goto free_inbufs;
@@ -1234,7 +1293,7 @@ static int add_port(struct ports_device *portdev, u32 id)
free_inbufs:
while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
- free_buf(buf);
+ free_buf(port->in_vq, buf, PAGE_SIZE);
free_device:
device_destroy(pdrvdata.class, port->dev->devt);
free_cdev:
@@ -1276,7 +1335,7 @@ static void remove_port_data(struct port *port)
/* Remove buffers we queued up for the Host to send us data in. */
while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
- free_buf(buf);
+ free_buf(port->in_vq, buf, PAGE_SIZE);
}
/*
@@ -1478,7 +1537,7 @@ static void control_work_handler(struct work_struct *work)
if (add_inbuf(portdev->c_ivq, buf) < 0) {
dev_warn(&portdev->vdev->dev,
"Error adding buffer to queue\n");
- free_buf(buf);
+ free_buf(portdev->c_ivq, buf, PAGE_SIZE);
}
}
spin_unlock(&portdev->cvq_lock);
@@ -1674,10 +1733,10 @@ static void remove_controlq_data(struct ports_device *portdev)
return;
while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
- free_buf(buf);
+ free_buf(portdev->c_ivq, buf, PAGE_SIZE);
while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
- free_buf(buf);
+ free_buf(portdev->c_ivq, buf, PAGE_SIZE);
}
/*
@@ -1724,10 +1783,12 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
multiport = false;
portdev->config.max_nr_ports = 1;
- if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
- offsetof(struct virtio_console_config,
- max_nr_ports),
- &portdev->config.max_nr_ports) == 0)
+ if (is_rproc_serial(vdev))
+ multiport = false;
+ else if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
+ offsetof(struct virtio_console_config,
+ max_nr_ports),
+ &portdev->config.max_nr_ports) == 0)
multiport = true;
err = init_vqs(portdev);
@@ -1838,6 +1899,16 @@ static unsigned int features[] = {
VIRTIO_CONSOLE_F_MULTIPORT,
};
+static struct virtio_device_id rproc_serial_id_table[] = {
+#ifdef CONFIG_HAS_DMA
+ { VIRTIO_ID_RPROC_SERIAL, VIRTIO_DEV_ANY_ID },
+#endif
+ { 0 },
+};
+
+static unsigned int rproc_serial_features[] = {
+};
+
#ifdef CONFIG_PM
static int virtcons_freeze(struct virtio_device *vdev)
{
@@ -1922,6 +1993,16 @@ static struct virtio_driver virtio_console = {
#endif
};
+static struct virtio_driver virtio_rproc_serial = {
+ .feature_table = rproc_serial_features,
+ .feature_table_size = ARRAY_SIZE(rproc_serial_features),
+ .driver.name = "virtio_rproc_serial",
+ .driver.owner = THIS_MODULE,
+ .id_table = rproc_serial_id_table,
+ .probe = virtcons_probe,
+ .remove = virtcons_remove,
+};
+
static int __init init(void)
{
int err;
@@ -1941,12 +2022,14 @@ static int __init init(void)
INIT_LIST_HEAD(&pdrvdata.consoles);
INIT_LIST_HEAD(&pdrvdata.portdevs);
- return register_virtio_driver(&virtio_console);
+ return register_virtio_driver(&virtio_console) &&
+ register_virtio_driver(&virtio_rproc_serial);
}
static void __exit fini(void)
{
unregister_virtio_driver(&virtio_console);
+ unregister_virtio_driver(&virtio_rproc_serial);
class_destroy(pdrvdata.class);
if (pdrvdata.debugfs_dir)
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 270fb22..07cf6f7 100644
--- a/include/linux/virtio_ids.h
+++ b/include/linux/virtio_ids.h
@@ -37,5 +37,6 @@
#define VIRTIO_ID_RPMSG 7 /* virtio remote processor messaging */
#define VIRTIO_ID_SCSI 8 /* virtio scsi */
#define VIRTIO_ID_9P 9 /* 9p virtio console */
+#define VIRTIO_ID_RPROC_SERIAL 0xB /* virtio remoteproc serial link */
#endif /* _LINUX_VIRTIO_IDS_H */
--
1.7.5.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Thomas Lendacky @ 2012-09-10 15:47 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, avi,
Sasha Levin
In-Reply-To: <87txvahfv3.fsf@rustcorp.com.au>
[-- Attachment #1.1: Type: text/plain, Size: 3937 bytes --]
On Friday, September 07, 2012 09:19:04 AM Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Thu, Sep 06, 2012 at 05:27:23PM +0930, Rusty Russell wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> > Yes without checksum net core always linearizes packets, so yes it is
> >> > screwed.
> >> > For -net, skb always allocates space for 17 frags + linear part so
> >> > it seems sane to do same in virtio core, and allocate, for -net,
> >> > up to max_frags + 1 from cache.
> >> > We can adjust it: no _SG -> 2 otherwise 18.
> >>
> >> But I thought it used individual buffers these days?
> >
> > Yes for receive, no for transmit. That's probably why
> > we should have the threshold per vq, not per device, BTW.
>
> Can someone actually run with my histogram patch and see what the real
> numbers are?
>
I ran some TCP_RR and TCP_STREAM sessions, both host-to-guest and
guest-to-host, with a form of the histogram patch applied against a
RHEL6.3 kernel. The histogram values were reset after each test.
Here are the results:
60 session TCP_RR from host-to-guest with 256 byte request and 256 byte
response for 60 seconds:
Queue histogram for virtio1:
Size distribution for input (max=7818456):
1: 7818456 ################################################################
Size distribution for output (max=7816698):
2: 149
3: 7816698 ################################################################
4: 2
5: 1
Size distribution for control (max=1):
0: 0
4 session TCP_STREAM from host-to-guest with 4K message size for 60 seconds:
Queue histogram for virtio1:
Size distribution for input (max=16050941):
1: 16050941 ################################################################
Size distribution for output (max=1877796):
2: 1877796 ################################################################
3: 5
Size distribution for control (max=1):
0: 0
4 session TCP_STREAM from host-to-guest with 16K message size for 60 seconds:
Queue histogram for virtio1:
Size distribution for input (max=16831151):
1: 16831151 ################################################################
Size distribution for output (max=1923965):
2: 1923965 ################################################################
3: 5
Size distribution for control (max=1):
0: 0
4 session TCP_STREAM from guest-to-host with 4K message size for 60 seconds:
Queue histogram for virtio1:
Size distribution for input (max=1316069):
1: 1316069 ################################################################
Size distribution for output (max=879213):
2: 24
3: 24097 #
4: 23176 #
5: 3412
6: 4446
7: 4663
8: 4195
9: 3772
10: 3388
11: 3666
12: 2885
13: 2759
14: 2997
15: 3060
16: 2651
17: 2235
18: 92721 ######
19: 879213 ################################################################
Size distribution for control (max=1):
0: 0
4 session TCP_STREAM from guest-to-host with 16K message size for 60 seconds:
Queue histogram for virtio1:
Size distribution for input (max=1428590):
1: 1428590 ################################################################
Size distribution for output (max=957774):
2: 20
3: 54955 ###
4: 34281 ##
5: 2967
6: 3394
7: 9400
8: 3061
9: 3397
10: 3258
11: 3275
12: 3147
13: 2876
14: 2747
15: 2832
16: 2013
17: 1670
18: 100369 ######
19: 957774 ################################################################
Size distribution for control (max=1):
0: 0
Thanks,
Tom
> I'm not convinced that the ideal 17-buffer case actually happens as much
> as we think. And if it's not happening with this netperf test, we're
> testing the wrong thing.
>
> Thanks,
> Rusty.
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #1.2: Type: text/html, Size: 23318 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Paolo Bonzini @ 2012-09-10 15:52 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, linux-kernel, virtualization, Sasha Levin, avi
In-Reply-To: <20120906050257.GA17656@redhat.com>
Il 06/09/2012 07:02, Michael S. Tsirkin ha scritto:
>> > It might be worth just unconditionally having a cache for the 2
>> > descriptor case. This is what I get with qemu tap, though for some
>> > reason the device features don't have guest or host CSUM, so my setup is
>> > probably screwed:
> Yes without checksum net core always linearizes packets, so yes it is
> screwed.
> For -net, skb always allocates space for 17 frags + linear part so
> it seems sane to do same in virtio core, and allocate, for -net,
> up to max_frags + 1 from cache.
> We can adjust it: no _SG -> 2 otherwise 18.
>
> Not sure about other drivers, maybe really use 2 there for now.
2 should also be good for virtio-blk and virtio-scsi 4KB random rw
workloads.
Paolo
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Thomas Lendacky @ 2012-09-10 16:01 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, avi,
Sasha Levin
In-Reply-To: <87txvahfv3.fsf@rustcorp.com.au>
On Friday, September 07, 2012 09:19:04 AM Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Thu, Sep 06, 2012 at 05:27:23PM +0930, Rusty Russell wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> > Yes without checksum net core always linearizes packets, so yes it is
> >> > screwed.
> >> > For -net, skb always allocates space for 17 frags + linear part so
> >> > it seems sane to do same in virtio core, and allocate, for -net,
> >> > up to max_frags + 1 from cache.
> >> > We can adjust it: no _SG -> 2 otherwise 18.
> >>
> >> But I thought it used individual buffers these days?
> >
> > Yes for receive, no for transmit. That's probably why
> > we should have the threshold per vq, not per device, BTW.
>
> Can someone actually run with my histogram patch and see what the real
> numbers are?
Somehow some HTML got in my first reply, resending...
I ran some TCP_RR and TCP_STREAM sessions, both host-to-guest and
guest-to-host, with a form of the histogram patch applied against a
RHEL6.3 kernel. The histogram values were reset after each test.
Here are the results:
60 session TCP_RR from host-to-guest with 256 byte request and 256 byte
response for 60 seconds:
Queue histogram for virtio1:
Size distribution for input (max=7818456):
1: 7818456 ################################################################
Size distribution for output (max=7816698):
2: 149
3: 7816698 ################################################################
4: 2
5: 1
Size distribution for control (max=1):
0: 0
4 session TCP_STREAM from host-to-guest with 4K message size for 60 seconds:
Queue histogram for virtio1:
Size distribution for input (max=16050941):
1: 16050941 ################################################################
Size distribution for output (max=1877796):
2: 1877796 ################################################################
3: 5
Size distribution for control (max=1):
0: 0
4 session TCP_STREAM from host-to-guest with 16K message size for 60 seconds:
Queue histogram for virtio1:
Size distribution for input (max=16831151):
1: 16831151 ################################################################
Size distribution for output (max=1923965):
2: 1923965 ################################################################
3: 5
Size distribution for control (max=1):
0: 0
4 session TCP_STREAM from guest-to-host with 4K message size for 60 seconds:
Queue histogram for virtio1:
Size distribution for input (max=1316069):
1: 1316069 ################################################################
Size distribution for output (max=879213):
2: 24
3: 24097 #
4: 23176 #
5: 3412
6: 4446
7: 4663
8: 4195
9: 3772
10: 3388
11: 3666
12: 2885
13: 2759
14: 2997
15: 3060
16: 2651
17: 2235
18: 92721 ######
19: 879213 ################################################################
Size distribution for control (max=1):
0: 0
4 session TCP_STREAM from guest-to-host with 16K message size for 60 seconds:
Queue histogram for virtio1:
Size distribution for input (max=1428590):
1: 1428590 ################################################################
Size distribution for output (max=957774):
2: 20
3: 54955 ###
4: 34281 ##
5: 2967
6: 3394
7: 9400
8: 3061
9: 3397
10: 3258
11: 3275
12: 3147
13: 2876
14: 2747
15: 2832
16: 2013
17: 1670
18: 100369 ######
19: 957774 ################################################################
Size distribution for control (max=1):
0: 0
Thanks,
Tom
>
> I'm not convinced that the ideal 17-buffer case actually happens as much
> as we think. And if it's not happening with this netperf test, we're
> testing the wrong thing.
>
> Thanks,
> Rusty.
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-09-10 16:08 UTC (permalink / raw)
To: Thomas Lendacky; +Cc: kvm, linux-kernel, virtualization, avi, Sasha Levin
In-Reply-To: <14037987.hiN6MSGY6z@tomlt1.ibmoffice.com>
On Mon, Sep 10, 2012 at 10:47:15AM -0500, Thomas Lendacky wrote:
> On Friday, September 07, 2012 09:19:04 AM Rusty Russell wrote:
>
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> > > On Thu, Sep 06, 2012 at 05:27:23PM +0930, Rusty Russell wrote:
>
> > >> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> > >> > Yes without checksum net core always linearizes packets, so yes it is
>
> > >> > screwed.
>
> > >> > For -net, skb always allocates space for 17 frags + linear part so
>
> > >> > it seems sane to do same in virtio core, and allocate, for -net,
>
> > >> > up to max_frags + 1 from cache.
>
> > >> > We can adjust it: no _SG -> 2 otherwise 18.
>
> > >>
>
> > >> But I thought it used individual buffers these days?
>
> > >
>
> > > Yes for receive, no for transmit. That's probably why
>
> > > we should have the threshold per vq, not per device, BTW.
>
> >
>
> > Can someone actually run with my histogram patch and see what the real
>
> > numbers are?
>
> >
>
>
>
> I ran some TCP_RR and TCP_STREAM sessions, both host-to-guest and
>
> guest-to-host, with a form of the histogram patch applied against a
>
> RHEL6.3 kernel. The histogram values were reset after each test.
>
>
>
> Here are the results:
>
>
>
> 60 session TCP_RR from host-to-guest with 256 byte request and 256 byte
>
> response for 60 seconds:
>
>
>
> Queue histogram for virtio1:
>
> Size distribution for input (max=7818456):
>
> 1: 7818456 ################################################################
>
> Size distribution for output (max=7816698):
>
> 2: 149
>
> 3: 7816698 ################################################################
Here, a threshold would help.
>
> 4: 2
>
> 5: 1
>
> Size distribution for control (max=1):
>
> 0: 0
>
>
>
>
>
> 4 session TCP_STREAM from host-to-guest with 4K message size for 60 seconds:
>
>
>
> Queue histogram for virtio1:
>
> Size distribution for input (max=16050941):
>
> 1: 16050941 ################################################################
>
> Size distribution for output (max=1877796):
>
> 2: 1877796 ################################################################
>
> 3: 5
>
> Size distribution for control (max=1):
>
> 0: 0
>
>
>
> 4 session TCP_STREAM from host-to-guest with 16K message size for 60 seconds:
>
>
>
> Queue histogram for virtio1:
>
> Size distribution for input (max=16831151):
>
> 1: 16831151 ################################################################
>
> Size distribution for output (max=1923965):
>
> 2: 1923965 ################################################################
>
> 3: 5
>
> Size distribution for control (max=1):
>
> 0: 0
>
Hmm for virtio net output we do always use 2 s/g, this is because of a
qemu bug. Maybe it's time we fixed this, added a feature bit?
This would fix above without threshold hacks.
>
> 4 session TCP_STREAM from guest-to-host with 4K message size for 60 seconds:
>
>
>
> Queue histogram for virtio1:
>
> Size distribution for input (max=1316069):
>
> 1: 1316069 ################################################################
>
> Size distribution for output (max=879213):
>
> 2: 24
>
> 3: 24097 #
>
> 4: 23176 #
>
> 5: 3412
>
> 6: 4446
>
> 7: 4663
>
> 8: 4195
>
> 9: 3772
>
> 10: 3388
>
> 11: 3666
>
> 12: 2885
>
> 13: 2759
>
> 14: 2997
>
> 15: 3060
>
> 16: 2651
>
> 17: 2235
>
> 18: 92721 ######
>
> 19: 879213 ################################################################
>
> Size distribution for control (max=1):
>
> 0: 0
>
>
>
> 4 session TCP_STREAM from guest-to-host with 16K message size for 60 seconds:
>
>
>
> Queue histogram for virtio1:
>
> Size distribution for input (max=1428590):
>
> 1: 1428590 ################################################################
>
> Size distribution for output (max=957774):
>
> 2: 20
>
> 3: 54955 ###
>
> 4: 34281 ##
>
> 5: 2967
>
> 6: 3394
>
> 7: 9400
>
> 8: 3061
>
> 9: 3397
>
> 10: 3258
>
> 11: 3275
>
> 12: 3147
>
> 13: 2876
>
> 14: 2747
>
> 15: 2832
>
> 16: 2013
>
> 17: 1670
>
> 18: 100369 ######
>
> 19: 957774 ################################################################
>
> Size distribution for control (max=1):
>
> 0: 0
>
>
>
> Thanks,
>
> Tom
In these tests we would have to set threshold pretty high.
I wonder whether the following makes any difference: the idea is to
A. get less false cache sharing by allocating full cache lines
B. better locality by using same cache for multiple sizes
So we get some of the wins of the threshold without bothering
with a cache.
Will try to test but not until later this week.
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5aa43c3..c184712 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -132,7 +132,8 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
unsigned head;
int i;
- desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
+ desc = kmalloc(L1_CACHE_ALIGN((out + in) * sizeof(struct vring_desc)),
+ gfp);
if (!desc)
return -ENOMEM;
--
MST
^ permalink raw reply related
* Re: [PATCH] Add a page cache-backed balloon device driver.
From: Mike Waychison @ 2012-09-10 17:37 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Frank Swiderski, Andrea Arcangeli, Rik van Riel, kvm,
linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <20120910090521.GB18544@redhat.com>
On Mon, Sep 10, 2012 at 5:05 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Jun 26, 2012 at 01:32:58PM -0700, Frank Swiderski wrote:
>> This implementation of a virtio balloon driver uses the page cache to
>> "store" pages that have been released to the host. The communication
>> (outside of target counts) is one way--the guest notifies the host when
>> it adds a page to the page cache, allowing the host to madvise(2) with
>> MADV_DONTNEED. Reclaim in the guest is therefore automatic and implicit
>> (via the regular page reclaim). This means that inflating the balloon
>> is similar to the existing balloon mechanism, but the deflate is
>> different--it re-uses existing Linux kernel functionality to
>> automatically reclaim.
>>
>> Signed-off-by: Frank Swiderski <fes@google.com>
Hi Michael,
I'm very sorry that Frank and I have been silent on these threads.
I've been out of the office and Frank has been been swamped :)
I'll take a stab at answering some of your questions below, and
hopefully we can end up on the same page.
> I've been trying to understand this, and I have
> a question: what exactly is the benefit
> of this new device?
The key difference between this device/driver and the pre-existing
virtio_balloon device/driver is in how the memory pressure loop is
controlled.
With the pre-existing balloon device/driver, the control loop for how
much memory a given VM is allowed to use is controlled completely by
the host. This is probably fine if the goal is to pack as much work
on a given host as possible, but it says nothing about the expected
performance that any given VM is expecting to have. Specifically, it
allows the host to set a target goal for the size of a VM, and the
driver in the guest does whatever is needed to get to that goal. This
is great for systems where one wants to "grow or shrink" a VM from the
outside.
This behaviour however doesn't match what applications actually expect
from a memory control loop however. In a native setup, an application
can usually expect to allocate memory from the kernel on an as-needed
basis, and can in turn return memory back to the system (using a heap
implementation that actually releases memory that is). The dynamic
size of an application is completely controlled by the application,
and there is very little that cluster management software can do to
ensure that the application fits some prescribed size.
We recognized this in the development of our cluster management
software long ago, so our systems are designed for managing tasks that
have a dynamic memory footprint. Overcommit is possible (as most
applications do not use the full reservation of memory they asked for
originally), letting us do things like schedule lower priority/lower
service-classification work using resources that are otherwise
available in stand-by for high-priority/low-latency workloads.
>
> Note that users could not care less about how a driver
> is implemented internally.
>
> Is there some workload where you see VM working better with
> this than regular balloon? Any numbers?
This device is less about performance as it is about getting the
memory size of a job (or in this case, a job in a VM) to grow and
shrink as the application workload sees fit, much like how processes
today can grow and shrink without external direction.
>
> Also, can't we just replace existing balloon implementation
> with this one?
Perhaps, but as described above, both devices have very different
characteristics.
> Why it is so important to deflate silently?
It may not be so important to deflate silently. I'm not sure why it
is important that we deflate "loudly" though either :) Doing so seems
like unnecessary guest/host communication IMO, especially if the guest
is expecting to be able to grow to totalram (and the host isn't able
to nack any pages reclaimed anyway...).
> I guess filesystem does not currently get a callback
> before page is reclaimed but this isan implementation detail -
> maybe this can be fixed?
I do not follow this question.
>
> Also can you pls answer Avi's question?
> How is overcommit managed?
Overcommit in our deployments is managed using memory cgroups on the
host. This allows us to have very directed policies as to how
competing VMs on a host may overcommit.
>
>
>> ---
>> drivers/virtio/Kconfig | 13 +
>> drivers/virtio/Makefile | 1 +
>> drivers/virtio/virtio_fileballoon.c | 636 +++++++++++++++++++++++++++++++++++
>> include/linux/virtio_balloon.h | 9 +
>> include/linux/virtio_ids.h | 1 +
>> 5 files changed, 660 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/virtio/virtio_fileballoon.c
>>
>> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
>> index f38b17a..cffa2a7 100644
>> --- a/drivers/virtio/Kconfig
>> +++ b/drivers/virtio/Kconfig
>> @@ -35,6 +35,19 @@ config VIRTIO_BALLOON
>>
>> If unsure, say M.
>>
>> +config VIRTIO_FILEBALLOON
>> + tristate "Virtio page cache-backed balloon driver"
>> + select VIRTIO
>> + select VIRTIO_RING
>> + ---help---
>> + This driver supports decreasing and automatically reclaiming the
>> + memory within a guest VM. Unlike VIRTIO_BALLOON, this driver instead
>> + tries to maintain a specific target balloon size using the page cache.
>> + This allows the guest to implicitly deflate the balloon by flushing
>> + pages from the cache and touching the page.
>> +
>> + If unsure, say N.
>> +
>> config VIRTIO_MMIO
>> tristate "Platform bus driver for memory mapped virtio devices (EXPERIMENTAL)"
>> depends on HAS_IOMEM && EXPERIMENTAL
>> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
>> index 5a4c63c..7ca0a3f 100644
>> --- a/drivers/virtio/Makefile
>> +++ b/drivers/virtio/Makefile
>> @@ -3,3 +3,4 @@ obj-$(CONFIG_VIRTIO_RING) += virtio_ring.o
>> obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
>> obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
>> obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
>> +obj-$(CONFIG_VIRTIO_FILEBALLOON) += virtio_fileballoon.o
>> diff --git a/drivers/virtio/virtio_fileballoon.c b/drivers/virtio/virtio_fileballoon.c
>> new file mode 100644
>> index 0000000..ff252ec
>> --- /dev/null
>> +++ b/drivers/virtio/virtio_fileballoon.c
>> @@ -0,0 +1,636 @@
>> +/* Virtio file (page cache-backed) balloon implementation, inspired by
>> + * Dor Loar and Marcelo Tosatti's implementations, and based on Rusty Russel's
>> + * implementation.
>> + *
>> + * This implementation of the virtio balloon driver re-uses the page cache to
>> + * allow memory consumed by inflating the balloon to be reclaimed by linux. It
>> + * creates and mounts a bare-bones filesystem containing a single inode. When
>> + * the host requests the balloon to inflate, it does so by "reading" pages at
>> + * offsets into the inode mapping's page_tree. The host is notified when the
>> + * pages are added to the page_tree, allowing it (the host) to madvise(2) the
>> + * corresponding host memory, reducing the RSS of the virtual machine. In this
>> + * implementation, the host is only notified when a page is added to the
>> + * balloon. Reclaim happens under the existing TTFP logic, which flushes unused
>> + * pages in the page cache. If the host used MADV_DONTNEED, then when the guest
>> + * uses the page, the zero page will be mapped in, allowing automatic (and fast,
>> + * compared to requiring a host notification via a virtio queue to get memory
>> + * back) reclaim.
>> + *
>> + * Copyright 2008 Rusty Russell IBM Corporation
>> + * Copyright 2011 Frank Swiderski Google Inc
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +#include <linux/backing-dev.h>
>> +#include <linux/delay.h>
>> +#include <linux/file.h>
>> +#include <linux/freezer.h>
>> +#include <linux/fs.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/kthread.h>
>> +#include <linux/module.h>
>> +#include <linux/mount.h>
>> +#include <linux/pagemap.h>
>> +#include <linux/slab.h>
>> +#include <linux/swap.h>
>> +#include <linux/virtio.h>
>> +#include <linux/virtio_balloon.h>
>> +#include <linux/writeback.h>
>> +
>> +#define VIRTBALLOON_PFN_ARRAY_SIZE 256
>> +
>> +struct virtio_balloon {
>> + struct virtio_device *vdev;
>> + struct virtqueue *inflate_vq;
>> +
>> + /* Where the ballooning thread waits for config to change. */
>> + wait_queue_head_t config_change;
>> +
>> + /* The thread servicing the balloon. */
>> + struct task_struct *thread;
>> +
>> + /* Waiting for host to ack the pages we released. */
>> + struct completion acked;
>> +
>> + /* The array of pfns we tell the Host about. */
>> + unsigned int num_pfns;
>> + u32 pfns[VIRTBALLOON_PFN_ARRAY_SIZE];
>> +
>> + struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
>> +
>> + /* The last page offset read into the mapping's page_tree */
>> + unsigned long last_scan_page_array;
>> +
>> + /* The last time a page was reclaimed */
>> + unsigned long last_reclaim;
>> +};
>> +
>> +/* Magic number used for the skeleton filesystem in the call to mount_pseudo */
>> +#define BALLOONFS_MAGIC 0x42414c4c
>> +
>> +static struct virtio_device_id id_table[] = {
>> + { VIRTIO_ID_FILE_BALLOON, VIRTIO_DEV_ANY_ID },
>> + { 0 },
>> +};
>> +
>> +/*
>> + * The skeleton filesystem contains a single inode, held by the structure below.
>> + * Using the containing structure below allows easy access to the struct
>> + * virtio_balloon.
>> + */
>> +static struct balloon_inode {
>> + struct inode inode;
>> + struct virtio_balloon *vb;
>> +} the_inode;
>> +
>> +/*
>> + * balloon_alloc_inode is called when the single inode for the skeleton
>> + * filesystem is created in init() with the call to new_inode.
>> + */
>> +static struct inode *balloon_alloc_inode(struct super_block *sb)
>> +{
>> + static bool already_inited;
>> + /* We should only ever be called once! */
>> + BUG_ON(already_inited);
>> + already_inited = true;
>> + inode_init_once(&the_inode.inode);
>> + return &the_inode.inode;
>> +}
>> +
>> +/* Noop implementation of destroy_inode. */
>> +static void balloon_destroy_inode(struct inode *inode)
>> +{
>> +}
>> +
>> +static int balloon_sync_fs(struct super_block *sb, int wait)
>> +{
>> + return filemap_write_and_wait(the_inode.inode.i_mapping);
>> +}
>> +
>> +static const struct super_operations balloonfs_ops = {
>> + .alloc_inode = balloon_alloc_inode,
>> + .destroy_inode = balloon_destroy_inode,
>> + .sync_fs = balloon_sync_fs,
>> +};
>> +
>> +static const struct dentry_operations balloonfs_dentry_operations = {
>> +};
>> +
>> +/*
>> + * balloonfs_writepage is called when linux needs to reclaim memory held using
>> + * the balloonfs' page cache.exactlyexactlyexactly
>> + */
>> +static int balloonfs_writepage(struct page *page, struct writeback_control *wbc)
>> +{
>> + the_inode.vb->last_reclaim = jiffies;
>> + SetPageUptodate(page);
>> + ClearPageDirty(page);
>> + /*
>> + * If the page isn't being flushed from the page allocator, go ahead and
>> + * drop it from the page cache anyway.
>> + */
>> + if (!wbc->for_reclaim)
>> + delete_from_page_cache(page);
>> + unlock_page(page);
>> + return 0;
>> +}
>> +
>> +/* Nearly no-op implementation of readpage */
>> +static int balloonfs_readpage(struct file *file, struct page *page)
>> +{
>> + SetPageUptodate(page);
>> + unlock_page(page);
>> + return 0;
>> +}
>> +
>> +static const struct address_space_operations balloonfs_aops = {
>> + .writepage = balloonfs_writepage,
>> + .readpage = balloonfs_readpage
>> +};
>> +
>> +static struct backing_dev_info balloonfs_backing_dev_info = {
>> + .name = "balloonfs",
>> + .ra_pages = 0,
>> + .capabilities = BDI_CAP_NO_ACCT_AND_WRITEBACK
>> +};
>> +
>> +static struct dentry *balloonfs_mount(struct file_system_type *fs_type,
>> + int flags, const char *dev_name, void *data)
>> +{
>> + struct dentry *root;
>> + struct inode *inode;
>> + root = mount_pseudo(fs_type, "balloon:", &balloonfs_ops,
>> + &balloonfs_dentry_operations, BALLOONFS_MAGIC);
>> + inode = root->d_inode;
>> + inode->i_mapping->a_ops = &balloonfs_aops;
>> + mapping_set_gfp_mask(inode->i_mapping,
>> + (GFP_HIGHUSER | __GFP_NOMEMALLOC));
>> + inode->i_mapping->backing_dev_info = &balloonfs_backing_dev_info;
>> + return root;
>> +}
>> +
>> +/* The single mounted skeleton filesystem */
>> +static struct vfsmount *balloon_mnt __read_mostly;
>> +
>> +static struct file_system_type balloon_fs_type = {
>> + .name = "balloonfs",
>> + .mount = balloonfs_mount,
>> + .kill_sb = kill_anon_super,
>> +};
>> +
>> +/* Acknowledges a message from the specified virtqueue. */
>> +static void balloon_ack(struct virtqueue *vq)
>> +{
>> + struct virtio_balloon *vb;
>> + unsigned int len;
>> +
>> + vb = virtqueue_get_buf(vq, &len);
>> + if (vb)
>> + complete(&vb->acked);
>> +}
>> +
>> +/*
>> + * Scans the page_tree for the inode's mapping, looking for an offset that is
>> + * currently empty, returning that index (or 0 if it could not fill the
>> + * request).
>> + */
>> +static unsigned long find_available_inode_page(struct virtio_balloon *vb)exactlyexactly
>> +{
>> + unsigned long radix_index, index, max_scan;
>> + struct address_space *mapping = the_inode.inode.i_mapping;
>> +
>> + /*
>> + * This function is a serialized call (only happens on the free-to-host
>> + * thread), so no locking is necessary here.
>> + */
>> + index = vb->last_scan_page_array;
>> + max_scan = totalram_pages - vb->last_scan_page_array;
>> +
>> + /*
>> + * Scan starting at the last scanned offset, then wrap around if
>> + * necessary.
>> + */
>> + if (index == 0)
>> + index = 1;
>> + rcu_read_lock();
>> + radix_index = radix_tree_next_hole(&mapping->page_tree,
>> + index, max_scan);
>> + rcu_read_unlock();
>> + /*
>> + * If we hit the end of the tree, wrap and search up to the original
>> + * index.
>> + */
>> + if (radix_index - index >= max_scan) {
>> + if (index != 1) {
>> + rcu_read_lock();
>> + radix_index = radix_tree_next_hole(&mapping->page_tree,
>> + 1, index);
>> + rcu_read_unlock();
>> + if (radix_index - 1 >= index)
>> + radix_index = 0;
>> + } else {
>> + radix_index = 0;
>> + }
>> + }
>> + vb->last_scan_page_array = radix_index;
>> +
>> + return radix_index;
>> +}
>> +
>> +/* Notifies the host of pages in the specified virtqueue. */
>> +static int tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
>> +{
>> + int err;
>> + struct scatterlist sg;
>> +
>> + sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
>> +
>> + init_completion(&vb->acked);
>> +
>> + /* We should always be able to add one buffer to an empty queue. */
>> + err = virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL);
>> + if (err < 0)
>> + return err;
>> + virtqueue_kick(vq);
>> +
>> + /* When host has read buffer, this completes via balloon_ack */
>> + wait_for_completion(&vb->acked);
>> + return err;
>> +}
>> +
>> +static void fill_balloon(struct virtio_balloon *vb, size_t num)
>> +{
>> + int err;
>> +
>> + /* We can only do one array worth at a time. */
>> + num = min(num, ARRAY_SIZE(vb->pfns));
>> +
>> + for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
>> + struct page *page;
>> + unsigned long inode_pfn = find_available_inode_page(vb);
>> + /* Should always be able to find a page. */
>> + BUG_ON(!inode_pfn);
>> + page = read_mapping_page(the_inode.inode.i_mapping, inode_pfn,
>> + NULL);
>> + if (IS_ERR(page)) {
>> + if (printk_ratelimit())
>> + dev_printk(KERN_INFO, &vb->vdev->dev,
>> + "Out of puff! Can't get %zu pages\n",
>> + num);
>> + break;
>> + }
>> +
>> + /* Set the page to be dirty */
>> + set_page_dirty(page);
>> +
>> + vb->pfns[vb->num_pfns] = page_to_pfn(page);
>> + }
>> +
>> + /* Didn't get any? Oh well. */
>> + if (vb->num_pfns == 0)
>> + return;
>> +
>> + /* Notify the host of the pages we just added to the page_tree. */
>> + err = tell_host(vb, vb->inflate_vq);
>> +
>> + for (; vb->num_pfns != 0; vb->num_pfns--) {
>> + struct page *page = pfn_to_page(vb->pfns[vb->num_pfns - 1]);
>> + /*
>> + * Release our refcount on the page so that it can be reclaimed
>> + * when necessary.
>> + */
>> + page_cache_release(page);
>> + }
>> + __mark_inode_dirty(&the_inode.inode, I_DIRTY_PAGES);
>> +}
>> +
>> +static inline void update_stat(struct virtio_balloon *vb, int idx,
>> + u64 val)
>> +{
>> + BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
>> + vb->stats[idx].tag = idx;
>> + vb->stats[idx].val = val;
>> +}
>> +
>> +#define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
>> +
>> +static inline u32 config_pages(struct virtio_balloon *vb);
>> +static void update_balloon_stats(struct virtio_balloon *vb)
>> +{
>> + unsigned long events[NR_VM_EVENT_ITEMS];
>> + struct sysinfo i;
>> +
>> + all_vm_events(events);
>> + si_meminfo(&i);
>> +
>> + update_stat(vb, VIRTIO_BALLOON_S_SWAP_IN,
>> + pages_to_bytes(events[PSWPIN]));
>> + update_stat(vb, VIRTIO_BALLOON_S_SWAP_OUT,
>> + pages_to_bytes(events[PSWPOUT]));
>> + update_stat(vb, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>> + update_stat(vb, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>> +
>> + /* Total and Free Mem */
>> + update_stat(vb, VIRTIO_BALLOON_S_MEMFREE, pages_to_bytes(i.freeram));
>> + update_stat(vb, VIRTIO_BALLOON_S_MEMTOT, pages_to_bytes(i.totalram));
>> +}
>> +
>> +static void virtballoon_changed(struct virtio_device *vdev)
>> +{
>> + struct virtio_balloon *vb = vdev->priv;
>> +
>> + wake_up(&vb->config_change);
>> +}
>> +
>> +static inline bool config_need_stats(struct virtio_balloon *vb)
>> +{
>> + u32 v = 0;
>> +
>> + vb->vdev->config->get(vb->vdev,
>> + offsetof(struct virtio_balloon_config,
>> + need_stats),
>> + &v, sizeof(v));
>> + return (v != 0);
>> +}
>> +
>> +static inline u32 config_pages(struct virtio_balloon *vb)
>> +{
>> + u32 v = 0;
>> +
>> + vb->vdev->config->get(vb->vdev,
>> + offsetof(struct virtio_balloon_config, num_pages),
>> + &v, sizeof(v));
>> + return v;
>> +}
>> +
>> +static inline s64 towards_target(struct virtio_balloon *vb)
>> +{
>> + struct address_space *mapping = the_inode.inode.i_mapping;
>> + u32 v = config_pages(vb);
>> +
>> + return (s64)v - (mapping ? mapping->nrpages : 0);
>> +}
>> +
>> +static void update_balloon_size(struct virtio_balloon *vb)
>> +{
>> + struct address_space *mapping = the_inode.inode.i_mapping;
>> + __le32 actual = cpu_to_le32((mapping ? mapping->nrpages : 0));
>> +
>> + vb->vdev->config->set(vb->vdev,
>> + offsetof(struct virtio_balloon_config, actual),
>> + &actual, sizeof(actual));
>> +}
>> +
>> +static void update_free_and_total(struct virtio_balloon *vb)
>> +{
>> + struct sysinfo i;
>> + u32 value;
>> +
>> + si_meminfo(&i);
>> +
>> + update_balloon_stats(vb);
>> + value = i.totalram;
>> + vb->vdev->config->set(vb->vdev,
>> + offsetof(struct virtio_balloon_config,
>> + pages_total),
>> + &value, sizeof(value));
>> + value = i.freeram;
>> + vb->vdev->config->set(vb->vdev,
>> + offsetof(struct virtio_balloon_config,
>> + pages_free),
>> + &value, sizeof(value));
>> + value = 0;
>> + vb->vdev->config->set(vb->vdev,
>> + offsetof(struct virtio_balloon_config,
>> + need_stats),
>> + &value, sizeof(value));
>> +}
>> +
>> +static int balloon(void *_vballoon)
>> +{
>> + struct virtio_balloon *vb = _vballoon;
>> +
>> + set_freezable();
>> + while (!kthread_should_stop()) {
>> + s64 diff;
>> + try_to_freeze();
>> + wait_event_interruptible(vb->config_change,
>> + (diff = towards_target(vb)) > 0
>> + || config_need_stats(vb)
>> + || kthread_should_stop()
>> + || freezing(current));
>> + if (config_need_stats(vb))
>> + update_free_and_total(vb);
>> + if (diff > 0) {
>> + unsigned long reclaim_time = vb->last_reclaim + 2 * HZ;
>> + /*
>> + * Don't fill the balloon if a page reclaim happened in
>> + * the past 2 seconds.
>> + */
>> + if (time_after_eq(reclaim_time, jiffies)) {
>> + /* Inflating too fast--sleep and skip. */
>> + msleep(500);
>> + } else {
>> + fill_balloon(vb, diff);
>> + }
>> + } else if (diff < 0 && config_pages(vb) == 0) {
>> + /*
>> + * Here we are specifically looking to detect the case
>> + * where there are pages in the page cache, but the
>> + * device wants us to go to 0. This is used in save/
>> + * restore since the host device doesn't keep track of
>> + * PFNs, and must flush the page cache on restore
>> + * (which loses the context of the original device
>> + * instance). However, we still suggest syncing the
>> + * diff so that we can get within the target range.
>> + */
>> + s64 nr_to_write =
>> + (!config_pages(vb) ? LONG_MAX : -diff);
>> + struct writeback_control wbc = {
>> + .sync_mode = WB_SYNC_ALL,
>> + .nr_to_write = nr_to_write,
>> + .range_start = 0,
>> + .range_end = LLONG_MAX,
>> + };
>> + sync_inode(&the_inode.inode, &wbc);
>> + }
>> + update_balloon_size(vb);
>> + }
>> + return 0;
>> +}
>> +
>> +static ssize_t virtballoon_attr_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf);
>> +
>> +static DEVICE_ATTR(total_memory, 0644,
>> + virtballoon_attr_show, NULL);
>> +
>> +static DEVICE_ATTR(free_memory, 0644,
>> + virtballoon_attr_show, NULL);
>> +
>> +static DEVICE_ATTR(target_pages, 0644,
>> + virtballoon_attr_show, NULL);
>> +
>> +static DEVICE_ATTR(actual_pages, 0644,
>> + virtballoon_attr_show, NULL);
>> +
>> +static struct attribute *virtballoon_attrs[] = {
>> + &dev_attr_total_memory.attr,
>> + &dev_attr_free_memory.attr,
>> + &dev_attr_target_pages.attr,
>> + &dev_attr_actual_pages.attr,
>> + NULL
>> +};
>> +static struct attribute_group virtballoon_attr_group = {
>> + .name = "virtballoon",
>> + .attrs = virtballoon_attrs,
>> +};
>> +
>> +static ssize_t virtballoon_attr_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct address_space *mapping = the_inode.inode.i_mapping;
>> + struct virtio_device *vdev = container_of(dev, struct virtio_device,
>> + dev);
>> + struct virtio_balloon *vb = vdev->priv;
>> + unsigned long long value = 0;
>> + if (attr == &dev_attr_total_memory)
>> + value = vb->stats[VIRTIO_BALLOON_S_MEMTOT].val;
>> + else if (attr == &dev_attr_free_memory)
>> + value = vb->stats[VIRTIO_BALLOON_S_MEMFREE].val;
>> + else if (attr == &dev_attr_target_pages)
>> + value = config_pages(vb);
>> + else if (attr == &dev_attr_actual_pages)
>> + value = cpu_to_le32((mapping ? mapping->nrpages : 0));
>> + return sprintf(buf, "%llu\n", value);
>> +}
>> +
>> +static int virtballoon_probe(struct virtio_device *vdev)
>> +{
>> + struct virtio_balloon *vb;
>> + struct virtqueue *vq[1];
>> + vq_callback_t *callback = balloon_ack;
>> + const char *name = "inflate";
>> + int err;
>> +
>> + vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
>> + if (!vb) {
>> + err = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + init_waitqueue_head(&vb->config_change);
>> + vb->vdev = vdev;
>> +
>> + /* We use one virtqueue: inflate */
>> + err = vdev->config->find_vqs(vdev, 1, vq, &callback, &name);
>> + if (err)
>> + goto out_free_vb;
>> +
>> + vb->inflate_vq = vq[0];
>> +
>> + err = sysfs_create_group(&vdev->dev.kobj, &virtballoon_attr_group);
>> + if (err) {
>> + pr_err("Failed to create virtballoon sysfs node\n");
>> + goto out_free_vb;
>> + }
>> +
>> + vb->last_scan_page_array = 0;
>> + vb->last_reclaim = 0;
>> + the_inode.vb = vb;
>> +
>> + vb->thread = kthread_run(balloon, vb, "vballoon");
>> + if (IS_ERR(vb->thread)) {
>> + err = PTR_ERR(vb->thread);
>> + goto out_del_vqs;
>> + }
>> +
>> + return 0;
>> +
>> +out_del_vqs:
>> + vdev->config->del_vqs(vdev);
>> +out_free_vb:
>> + kfree(vb);
>> +out:
>> + return err;
>> +}
>> +
>> +static void __devexit virtballoon_remove(struct virtio_device *vdev)
>> +{
>> + struct virtio_balloon *vb = vdev->priv;
>> +
>> + kthread_stop(vb->thread);
>> +
>> + sysfs_remove_group(&vdev->dev.kobj, &virtballoon_attr_group);
>> +
>> + /* Now we reset the device so we can clean up the queues. */
>> + vdev->config->reset(vdev);
>> +
>> + vdev->config->del_vqs(vdev);
>> + kfree(vb);
>> +}
>> +
>> +static struct virtio_driver virtio_balloon_driver = {
>> + .feature_table = NULL,
>> + .feature_table_size = 0,
>> + .driver.name = KBUILD_MODNAME,
>> + .driver.owner = THIS_MODULE,
>> + .id_table = id_table,
>> + .probe = virtballoon_probe,
>> + .remove = __devexit_p(virtballoon_remove),
>> + .config_changed = virtballoon_changed,
>> +};
>> +
>> +static int __init init(void)
>> +{
>> + int err = register_filesystem(&balloon_fs_type);
>> + if (err)
>> + goto out;
>> +
>> + balloon_mnt = kern_mount(&balloon_fs_type);
>> + if (IS_ERR(balloon_mnt)) {
>> + err = PTR_ERR(balloon_mnt);
>> + goto out_filesystem;
>> + }
>> +
>> + err = register_virtio_driver(&virtio_balloon_driver);
>> + if (err)
>> + goto out_filesystem;
>> +
>> + goto out;
>> +
>> +out_filesystem:
>> + unregister_filesystem(&balloon_fs_type);
>> +
>> +out:
>> + return err;
>> +}
>> +
>> +static void __exit fini(void)
>> +{
>> + if (balloon_mnt) {
>> + unregister_filesystem(&balloon_fs_type);
>> + balloon_mnt = NULL;
>> + }
>> + unregister_virtio_driver(&virtio_balloon_driver);
>> +}
>> +module_init(init);
>> +module_exit(fini);
>> +
>> +MODULE_DEVICE_TABLE(virtio, id_table);
>> +MODULE_DESCRIPTION("Virtio file (page cache-backed) balloon driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
>> index 652dc8b..2be9a02 100644
>> --- a/include/linux/virtio_balloon.h
>> +++ b/include/linux/virtio_balloon.h
>> @@ -41,6 +41,15 @@ struct virtio_balloon_config
>> __le32 num_pages;
>> /* Number of pages we've actually got in balloon. */
>> __le32 actual;
>> +#if defined(CONFIG_VIRTIO_FILEBALLOON) ||\
>> + defined(CONFIG_VIRTIO_FILEBALLOON_MODULE)
>> + /* Total pages on this system. */
>> + __le32 pages_total;
>> + /* Free pages on this system. */
>> + __le32 pages_free;
>> + /* If the device needs pages_total/pages_free updated. */
>> + __le32 need_stats;
>> +#endif
>> };
>>
>> #define VIRTIO_BALLOON_S_SWAP_IN 0 /* Amount of memory swapped in */
>> diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
>> index 7529b85..2f081d7 100644
>> --- a/include/linux/virtio_ids.h
>> +++ b/include/linux/virtio_ids.h
>> @@ -37,5 +37,6 @@
>> #define VIRTIO_ID_RPMSG 7 /* virtio remote processor messaging */
>> #define VIRTIO_ID_SCSI 8 /* virtio scsi */
>> #define VIRTIO_ID_9P 9 /* 9p virtio console */
>> +#define VIRTIO_ID_FILE_BALLOON 10 /* virtio file-backed balloon */
>>
>> #endif /* _LINUX_VIRTIO_IDS_H */
>> --
>> 1.7.7.3
^ permalink raw reply
* Re: [PATCH] Add a page cache-backed balloon device driver.
From: Rik van Riel @ 2012-09-10 18:04 UTC (permalink / raw)
To: Mike Waychison
Cc: Frank Swiderski, Andrea Arcangeli, kvm, Michael S. Tsirkin,
linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <CAGTjWtBa5jW6+y1k6zZmD9TUNHBgv8WRbO_iy7huY_dJSumStw@mail.gmail.com>
On 09/10/2012 01:37 PM, Mike Waychison wrote:
> On Mon, Sep 10, 2012 at 5:05 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> Also can you pls answer Avi's question?
>> How is overcommit managed?
>
> Overcommit in our deployments is managed using memory cgroups on the
> host. This allows us to have very directed policies as to how
> competing VMs on a host may overcommit.
How do your memory cgroups lead to guests inflating their balloons?
--
All rights reversed
^ permalink raw reply
* Re: [PATCH] Add a page cache-backed balloon device driver.
From: Mike Waychison @ 2012-09-10 18:29 UTC (permalink / raw)
To: Rik van Riel
Cc: Frank Swiderski, Andrea Arcangeli, kvm, Michael S. Tsirkin,
linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <504E2B98.3010007@redhat.com>
On Mon, Sep 10, 2012 at 2:04 PM, Rik van Riel <riel@redhat.com> wrote:
> On 09/10/2012 01:37 PM, Mike Waychison wrote:
>>
>> On Mon, Sep 10, 2012 at 5:05 AM, Michael S. Tsirkin <mst@redhat.com>
>> wrote:
>
>
>>> Also can you pls answer Avi's question?
>>> How is overcommit managed?
>>
>>
>> Overcommit in our deployments is managed using memory cgroups on the
>> host. This allows us to have very directed policies as to how
>> competing VMs on a host may overcommit.
>
>
> How do your memory cgroups lead to guests inflating their balloons?
The control loop that is driving the cgroup on the host can still move
the balloon target page count causing the balloon in the guest to try
and inflate. This allows the host to effectively slowly grow the
balloon in the guest, allowing reclaim of guest free memory, followed
by guest page cache (and memory on the host system). This can then be
compared with the subsequent growth (as this balloon setup allows the
guest to grow as it sees fit), which in effect gives us a memory
pressure indicator on the host, allowing it to back-off shrinking the
guest if the guest balloon quickly deflates.
The net effect is an opportunistic release of memory from the guest
back to the host, and the ability to quickly grow a VM's memory
footprint as the workload within it requires.
This dynamic memory sizing of VMs is much more in line with what we
can expect from native tasks today (and which is what our resource
management systems are designed to handle).
^ permalink raw reply
* Re: [PATCHv4] virtio-spec: virtio network device multiqueue support
From: Rick Jones @ 2012-09-10 18:39 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, netdev, virtualization, levinsasha928,
pbonzini, Tom Herbert
In-Reply-To: <878vcifwxi.fsf@rustcorp.com.au>
On 09/09/2012 07:12 PM, Rusty Russell wrote:
> OK, I read the spec (pasted below for easy of reading), but I'm still
> confused over how this will work.
>
> I thought normal net drivers have the hardware provide an rxhash for
> each packet, and we map that to CPU to queue the packet on[1]. We hope
> that the receiving process migrates to that CPU, so xmit queue
> matches.
>
> For virtio this would mean a new per-packet rxhash value, right?
>
> Why are we doing something different? What am I missing?
>
> Thanks,
> Rusty.
> [1] Everything I Know About Networking I Learned From LWN:
> https://lwn.net/Articles/362339/
In my taxonomy at least, "multi-queue" predates RPS and RFS and is
simply where the NIC via some means, perhaps a headers hash, separates
incoming frames to different queues.
RPS can be thought of as doing something similar inside the host. That
could be used to get a spread from an otherwise "dumb" NIC (certainly
that is what one of its predecessors - Inbound Packet Scheduling - used
it for in HP-UX 10.20), or it could be used to augment the multi-queue
support of a not-so-dump NIC - say if said NIC had a limit of queues
that was rather lower than the number of cores/threads in the host.
Indeed some driver/NIC combinations provide a hash value to the host for
the host to use as it sees fit.
However, there is still the matter of a single thread of an application
servicing multiple connections, each of which would hash to different
locations.
RFS (Receive Flow Steering) then goes one step further, and looks-up
where the flow endpoint was last accessed and steers the traffic there.
The idea being that a thread of execution servicing multiple flows
will have the traffic of those flows sent to the same place. It then
allows the scheduler to decide where things should be run rather than
the networking code.
rick jones
^ permalink raw reply
* Re: [PATCH] Add a page cache-backed balloon device driver.
From: Michael S. Tsirkin @ 2012-09-10 19:59 UTC (permalink / raw)
To: Mike Waychison
Cc: Frank Swiderski, Andrea Arcangeli, Rik van Riel, kvm,
linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <CAGTjWtBa5jW6+y1k6zZmD9TUNHBgv8WRbO_iy7huY_dJSumStw@mail.gmail.com>
On Mon, Sep 10, 2012 at 01:37:06PM -0400, Mike Waychison wrote:
> On Mon, Sep 10, 2012 at 5:05 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Jun 26, 2012 at 01:32:58PM -0700, Frank Swiderski wrote:
> >> This implementation of a virtio balloon driver uses the page cache to
> >> "store" pages that have been released to the host. The communication
> >> (outside of target counts) is one way--the guest notifies the host when
> >> it adds a page to the page cache, allowing the host to madvise(2) with
> >> MADV_DONTNEED. Reclaim in the guest is therefore automatic and implicit
> >> (via the regular page reclaim). This means that inflating the balloon
> >> is similar to the existing balloon mechanism, but the deflate is
> >> different--it re-uses existing Linux kernel functionality to
> >> automatically reclaim.
> >>
> >> Signed-off-by: Frank Swiderski <fes@google.com>
>
> Hi Michael,
>
> I'm very sorry that Frank and I have been silent on these threads.
> I've been out of the office and Frank has been been swamped :)
>
> I'll take a stab at answering some of your questions below, and
> hopefully we can end up on the same page.
>
> > I've been trying to understand this, and I have
> > a question: what exactly is the benefit
> > of this new device?
>
> The key difference between this device/driver and the pre-existing
> virtio_balloon device/driver is in how the memory pressure loop is
> controlled.
>
> With the pre-existing balloon device/driver, the control loop for how
> much memory a given VM is allowed to use is controlled completely by
> the host. This is probably fine if the goal is to pack as much work
> on a given host as possible, but it says nothing about the expected
> performance that any given VM is expecting to have. Specifically, it
> allows the host to set a target goal for the size of a VM, and the
> driver in the guest does whatever is needed to get to that goal. This
> is great for systems where one wants to "grow or shrink" a VM from the
> outside.
>
>
> This behaviour however doesn't match what applications actually expect
> from a memory control loop however. In a native setup, an application
> can usually expect to allocate memory from the kernel on an as-needed
> basis, and can in turn return memory back to the system (using a heap
> implementation that actually releases memory that is). The dynamic
> size of an application is completely controlled by the application,
> and there is very little that cluster management software can do to
> ensure that the application fits some prescribed size.
>
> We recognized this in the development of our cluster management
> software long ago, so our systems are designed for managing tasks that
> have a dynamic memory footprint. Overcommit is possible (as most
> applications do not use the full reservation of memory they asked for
> originally), letting us do things like schedule lower priority/lower
> service-classification work using resources that are otherwise
> available in stand-by for high-priority/low-latency workloads.
OK I am not sure I got this right so pls tell me if this summary is
correct (note: this does not talk about what guest does with memory,
ust what it is that device does):
- existing balloon is told lower limit on target size by host and pulls in at least
target size. Guest can inflate > target size if it likes
and then it is OK to deflate back to target size but not less.
- your balloon is told upper limit on target size by host and pulls at most
target size. Guest can deflate down to 0 at any point.
If so I think both approaches make sense and in fact they
can be useful at the same time for the same guest.
In that case, I see two ways how this can be done:
1. two devices: existing ballon + cache balloon
2. add "upper limit" to existing ballon
A single device looks a bit more natural in that we don't
really care in which balloon a page is as long as we
are between lower and upper limit. Right?
From implementation POV we could have it use
pagecache for pages above lower limit but that
is a separate question about driver design,
I would like to make sure I understand the high
level design first.
> >
> > Note that users could not care less about how a driver
> > is implemented internally.
> >
> > Is there some workload where you see VM working better with
> > this than regular balloon? Any numbers?
>
> This device is less about performance as it is about getting the
> memory size of a job (or in this case, a job in a VM) to grow and
> shrink as the application workload sees fit, much like how processes
> today can grow and shrink without external direction.
Still, e.g. swap in host achieves more or less the same functionality.
I am guessing balloon can work better by getting more cooperation
from guest but aren't there any tests showing this is true in practice?
> >
> > Also, can't we just replace existing balloon implementation
> > with this one?
>
> Perhaps, but as described above, both devices have very different
> characteristics.
>
> > Why it is so important to deflate silently?
>
> It may not be so important to deflate silently. I'm not sure why it
> is important that we deflate "loudly" though either :) Doing so seems
> like unnecessary guest/host communication IMO, especially if the guest
> is expecting to be able to grow to totalram (and the host isn't able
> to nack any pages reclaimed anyway...).
First, we could add nack easily enough :)
Second, access gets an exit anyway. If you tell
host first you can maybe batch these and actually speed things up.
It remains to be measured but historically we told host
so the onus of proof would be on whoever wants to remove this.
Third, see discussion on ML - we came up with
the idea of locking/unlocking balloon memory
which is useful for an assigned device.
Requires telling host first.
Also knowing how much memory there is in a balloon
would be useful for admin.
There could be other uses.
> > I guess filesystem does not currently get a callback
> > before page is reclaimed but this isan implementation detail -
> > maybe this can be fixed?
>
> I do not follow this question.
Assume we want to tell host before use.
Can you implement this on top of your patch?
> >
> > Also can you pls answer Avi's question?
> > How is overcommit managed?
>
> Overcommit in our deployments is managed using memory cgroups on the
> host. This allows us to have very directed policies as to how
> competing VMs on a host may overcommit.
So you push VM out to swap if it's over allowed memory?
Existing balloon does this better as it is cooperative,
it seems.
> >
> >
> >> ---
> >> drivers/virtio/Kconfig | 13 +
> >> drivers/virtio/Makefile | 1 +
> >> drivers/virtio/virtio_fileballoon.c | 636 +++++++++++++++++++++++++++++++++++
> >> include/linux/virtio_balloon.h | 9 +
> >> include/linux/virtio_ids.h | 1 +
> >> 5 files changed, 660 insertions(+), 0 deletions(-)
> >> create mode 100644 drivers/virtio/virtio_fileballoon.c
> >>
> >> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> >> index f38b17a..cffa2a7 100644
> >> --- a/drivers/virtio/Kconfig
> >> +++ b/drivers/virtio/Kconfig
> >> @@ -35,6 +35,19 @@ config VIRTIO_BALLOON
> >>
> >> If unsure, say M.
> >>
> >> +config VIRTIO_FILEBALLOON
> >> + tristate "Virtio page cache-backed balloon driver"
> >> + select VIRTIO
> >> + select VIRTIO_RING
> >> + ---help---
> >> + This driver supports decreasing and automatically reclaiming the
> >> + memory within a guest VM. Unlike VIRTIO_BALLOON, this driver instead
> >> + tries to maintain a specific target balloon size using the page cache.
> >> + This allows the guest to implicitly deflate the balloon by flushing
> >> + pages from the cache and touching the page.
> >> +
> >> + If unsure, say N.
> >> +
> >> config VIRTIO_MMIO
> >> tristate "Platform bus driver for memory mapped virtio devices (EXPERIMENTAL)"
> >> depends on HAS_IOMEM && EXPERIMENTAL
> >> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> >> index 5a4c63c..7ca0a3f 100644
> >> --- a/drivers/virtio/Makefile
> >> +++ b/drivers/virtio/Makefile
> >> @@ -3,3 +3,4 @@ obj-$(CONFIG_VIRTIO_RING) += virtio_ring.o
> >> obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> >> obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> >> obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> >> +obj-$(CONFIG_VIRTIO_FILEBALLOON) += virtio_fileballoon.o
> >> diff --git a/drivers/virtio/virtio_fileballoon.c b/drivers/virtio/virtio_fileballoon.c
> >> new file mode 100644
> >> index 0000000..ff252ec
> >> --- /dev/null
> >> +++ b/drivers/virtio/virtio_fileballoon.c
> >> @@ -0,0 +1,636 @@
> >> +/* Virtio file (page cache-backed) balloon implementation, inspired by
> >> + * Dor Loar and Marcelo Tosatti's implementations, and based on Rusty Russel's
> >> + * implementation.
> >> + *
> >> + * This implementation of the virtio balloon driver re-uses the page cache to
> >> + * allow memory consumed by inflating the balloon to be reclaimed by linux. It
> >> + * creates and mounts a bare-bones filesystem containing a single inode. When
> >> + * the host requests the balloon to inflate, it does so by "reading" pages at
> >> + * offsets into the inode mapping's page_tree. The host is notified when the
> >> + * pages are added to the page_tree, allowing it (the host) to madvise(2) the
> >> + * corresponding host memory, reducing the RSS of the virtual machine. In this
> >> + * implementation, the host is only notified when a page is added to the
> >> + * balloon. Reclaim happens under the existing TTFP logic, which flushes unused
> >> + * pages in the page cache. If the host used MADV_DONTNEED, then when the guest
> >> + * uses the page, the zero page will be mapped in, allowing automatic (and fast,
> >> + * compared to requiring a host notification via a virtio queue to get memory
> >> + * back) reclaim.
> >> + *
> >> + * Copyright 2008 Rusty Russell IBM Corporation
> >> + * Copyright 2011 Frank Swiderski Google Inc
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program; if not, write to the Free Software
> >> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> >> + */
> >> +#include <linux/backing-dev.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/file.h>
> >> +#include <linux/freezer.h>
> >> +#include <linux/fs.h>
> >> +#include <linux/jiffies.h>
> >> +#include <linux/kthread.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mount.h>
> >> +#include <linux/pagemap.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/swap.h>
> >> +#include <linux/virtio.h>
> >> +#include <linux/virtio_balloon.h>
> >> +#include <linux/writeback.h>
> >> +
> >> +#define VIRTBALLOON_PFN_ARRAY_SIZE 256
> >> +
> >> +struct virtio_balloon {
> >> + struct virtio_device *vdev;
> >> + struct virtqueue *inflate_vq;
> >> +
> >> + /* Where the ballooning thread waits for config to change. */
> >> + wait_queue_head_t config_change;
> >> +
> >> + /* The thread servicing the balloon. */
> >> + struct task_struct *thread;
> >> +
> >> + /* Waiting for host to ack the pages we released. */
> >> + struct completion acked;
> >> +
> >> + /* The array of pfns we tell the Host about. */
> >> + unsigned int num_pfns;
> >> + u32 pfns[VIRTBALLOON_PFN_ARRAY_SIZE];
> >> +
> >> + struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
> >> +
> >> + /* The last page offset read into the mapping's page_tree */
> >> + unsigned long last_scan_page_array;
> >> +
> >> + /* The last time a page was reclaimed */
> >> + unsigned long last_reclaim;
> >> +};
> >> +
> >> +/* Magic number used for the skeleton filesystem in the call to mount_pseudo */
> >> +#define BALLOONFS_MAGIC 0x42414c4c
> >> +
> >> +static struct virtio_device_id id_table[] = {
> >> + { VIRTIO_ID_FILE_BALLOON, VIRTIO_DEV_ANY_ID },
> >> + { 0 },
> >> +};
> >> +
> >> +/*
> >> + * The skeleton filesystem contains a single inode, held by the structure below.
> >> + * Using the containing structure below allows easy access to the struct
> >> + * virtio_balloon.
> >> + */
> >> +static struct balloon_inode {
> >> + struct inode inode;
> >> + struct virtio_balloon *vb;
> >> +} the_inode;
> >> +
> >> +/*
> >> + * balloon_alloc_inode is called when the single inode for the skeleton
> >> + * filesystem is created in init() with the call to new_inode.
> >> + */
> >> +static struct inode *balloon_alloc_inode(struct super_block *sb)
> >> +{
> >> + static bool already_inited;
> >> + /* We should only ever be called once! */
> >> + BUG_ON(already_inited);
> >> + already_inited = true;
> >> + inode_init_once(&the_inode.inode);
> >> + return &the_inode.inode;
> >> +}
> >> +
> >> +/* Noop implementation of destroy_inode. */
> >> +static void balloon_destroy_inode(struct inode *inode)
> >> +{
> >> +}
> >> +
> >> +static int balloon_sync_fs(struct super_block *sb, int wait)
> >> +{
> >> + return filemap_write_and_wait(the_inode.inode.i_mapping);
> >> +}
> >> +
> >> +static const struct super_operations balloonfs_ops = {
> >> + .alloc_inode = balloon_alloc_inode,
> >> + .destroy_inode = balloon_destroy_inode,
> >> + .sync_fs = balloon_sync_fs,
> >> +};
> >> +
> >> +static const struct dentry_operations balloonfs_dentry_operations = {
> >> +};
> >> +
> >> +/*
> >> + * balloonfs_writepage is called when linux needs to reclaim memory held using
> >> + * the balloonfs' page cache.exactlyexactlyexactly
> >> + */
> >> +static int balloonfs_writepage(struct page *page, struct writeback_control *wbc)
> >> +{
> >> + the_inode.vb->last_reclaim = jiffies;
> >> + SetPageUptodate(page);
> >> + ClearPageDirty(page);
> >> + /*
> >> + * If the page isn't being flushed from the page allocator, go ahead and
> >> + * drop it from the page cache anyway.
> >> + */
> >> + if (!wbc->for_reclaim)
> >> + delete_from_page_cache(page);
> >> + unlock_page(page);
> >> + return 0;
> >> +}
> >> +
> >> +/* Nearly no-op implementation of readpage */
> >> +static int balloonfs_readpage(struct file *file, struct page *page)
> >> +{
> >> + SetPageUptodate(page);
> >> + unlock_page(page);
> >> + return 0;
> >> +}
> >> +
> >> +static const struct address_space_operations balloonfs_aops = {
> >> + .writepage = balloonfs_writepage,
> >> + .readpage = balloonfs_readpage
> >> +};
> >> +
> >> +static struct backing_dev_info balloonfs_backing_dev_info = {
> >> + .name = "balloonfs",
> >> + .ra_pages = 0,
> >> + .capabilities = BDI_CAP_NO_ACCT_AND_WRITEBACK
> >> +};
> >> +
> >> +static struct dentry *balloonfs_mount(struct file_system_type *fs_type,
> >> + int flags, const char *dev_name, void *data)
> >> +{
> >> + struct dentry *root;
> >> + struct inode *inode;
> >> + root = mount_pseudo(fs_type, "balloon:", &balloonfs_ops,
> >> + &balloonfs_dentry_operations, BALLOONFS_MAGIC);
> >> + inode = root->d_inode;
> >> + inode->i_mapping->a_ops = &balloonfs_aops;
> >> + mapping_set_gfp_mask(inode->i_mapping,
> >> + (GFP_HIGHUSER | __GFP_NOMEMALLOC));
> >> + inode->i_mapping->backing_dev_info = &balloonfs_backing_dev_info;
> >> + return root;
> >> +}
> >> +
> >> +/* The single mounted skeleton filesystem */
> >> +static struct vfsmount *balloon_mnt __read_mostly;
> >> +
> >> +static struct file_system_type balloon_fs_type = {
> >> + .name = "balloonfs",
> >> + .mount = balloonfs_mount,
> >> + .kill_sb = kill_anon_super,
> >> +};
> >> +
> >> +/* Acknowledges a message from the specified virtqueue. */
> >> +static void balloon_ack(struct virtqueue *vq)
> >> +{
> >> + struct virtio_balloon *vb;
> >> + unsigned int len;
> >> +
> >> + vb = virtqueue_get_buf(vq, &len);
> >> + if (vb)
> >> + complete(&vb->acked);
> >> +}
> >> +
> >> +/*
> >> + * Scans the page_tree for the inode's mapping, looking for an offset that is
> >> + * currently empty, returning that index (or 0 if it could not fill the
> >> + * request).
> >> + */
> >> +static unsigned long find_available_inode_page(struct virtio_balloon *vb)exactlyexactly
> >> +{
> >> + unsigned long radix_index, index, max_scan;
> >> + struct address_space *mapping = the_inode.inode.i_mapping;
> >> +
> >> + /*
> >> + * This function is a serialized call (only happens on the free-to-host
> >> + * thread), so no locking is necessary here.
> >> + */
> >> + index = vb->last_scan_page_array;
> >> + max_scan = totalram_pages - vb->last_scan_page_array;
> >> +
> >> + /*
> >> + * Scan starting at the last scanned offset, then wrap around if
> >> + * necessary.
> >> + */
> >> + if (index == 0)
> >> + index = 1;
> >> + rcu_read_lock();
> >> + radix_index = radix_tree_next_hole(&mapping->page_tree,
> >> + index, max_scan);
> >> + rcu_read_unlock();
> >> + /*
> >> + * If we hit the end of the tree, wrap and search up to the original
> >> + * index.
> >> + */
> >> + if (radix_index - index >= max_scan) {
> >> + if (index != 1) {
> >> + rcu_read_lock();
> >> + radix_index = radix_tree_next_hole(&mapping->page_tree,
> >> + 1, index);
> >> + rcu_read_unlock();
> >> + if (radix_index - 1 >= index)
> >> + radix_index = 0;
> >> + } else {
> >> + radix_index = 0;
> >> + }
> >> + }
> >> + vb->last_scan_page_array = radix_index;
> >> +
> >> + return radix_index;
> >> +}
> >> +
> >> +/* Notifies the host of pages in the specified virtqueue. */
> >> +static int tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
> >> +{
> >> + int err;
> >> + struct scatterlist sg;
> >> +
> >> + sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> >> +
> >> + init_completion(&vb->acked);
> >> +
> >> + /* We should always be able to add one buffer to an empty queue. */
> >> + err = virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL);
> >> + if (err < 0)
> >> + return err;
> >> + virtqueue_kick(vq);
> >> +
> >> + /* When host has read buffer, this completes via balloon_ack */
> >> + wait_for_completion(&vb->acked);
> >> + return err;
> >> +}
> >> +
> >> +static void fill_balloon(struct virtio_balloon *vb, size_t num)
> >> +{
> >> + int err;
> >> +
> >> + /* We can only do one array worth at a time. */
> >> + num = min(num, ARRAY_SIZE(vb->pfns));
> >> +
> >> + for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
> >> + struct page *page;
> >> + unsigned long inode_pfn = find_available_inode_page(vb);
> >> + /* Should always be able to find a page. */
> >> + BUG_ON(!inode_pfn);
> >> + page = read_mapping_page(the_inode.inode.i_mapping, inode_pfn,
> >> + NULL);
> >> + if (IS_ERR(page)) {
> >> + if (printk_ratelimit())
> >> + dev_printk(KERN_INFO, &vb->vdev->dev,
> >> + "Out of puff! Can't get %zu pages\n",
> >> + num);
> >> + break;
> >> + }
> >> +
> >> + /* Set the page to be dirty */
> >> + set_page_dirty(page);
> >> +
> >> + vb->pfns[vb->num_pfns] = page_to_pfn(page);
> >> + }
> >> +
> >> + /* Didn't get any? Oh well. */
> >> + if (vb->num_pfns == 0)
> >> + return;
> >> +
> >> + /* Notify the host of the pages we just added to the page_tree. */
> >> + err = tell_host(vb, vb->inflate_vq);
> >> +
> >> + for (; vb->num_pfns != 0; vb->num_pfns--) {
> >> + struct page *page = pfn_to_page(vb->pfns[vb->num_pfns - 1]);
> >> + /*
> >> + * Release our refcount on the page so that it can be reclaimed
> >> + * when necessary.
> >> + */
> >> + page_cache_release(page);
> >> + }
> >> + __mark_inode_dirty(&the_inode.inode, I_DIRTY_PAGES);
> >> +}
> >> +
> >> +static inline void update_stat(struct virtio_balloon *vb, int idx,
> >> + u64 val)
> >> +{
> >> + BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
> >> + vb->stats[idx].tag = idx;
> >> + vb->stats[idx].val = val;
> >> +}
> >> +
> >> +#define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
> >> +
> >> +static inline u32 config_pages(struct virtio_balloon *vb);
> >> +static void update_balloon_stats(struct virtio_balloon *vb)
> >> +{
> >> + unsigned long events[NR_VM_EVENT_ITEMS];
> >> + struct sysinfo i;
> >> +
> >> + all_vm_events(events);
> >> + si_meminfo(&i);
> >> +
> >> + update_stat(vb, VIRTIO_BALLOON_S_SWAP_IN,
> >> + pages_to_bytes(events[PSWPIN]));
> >> + update_stat(vb, VIRTIO_BALLOON_S_SWAP_OUT,
> >> + pages_to_bytes(events[PSWPOUT]));
> >> + update_stat(vb, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
> >> + update_stat(vb, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> >> +
> >> + /* Total and Free Mem */
> >> + update_stat(vb, VIRTIO_BALLOON_S_MEMFREE, pages_to_bytes(i.freeram));
> >> + update_stat(vb, VIRTIO_BALLOON_S_MEMTOT, pages_to_bytes(i.totalram));
> >> +}
> >> +
> >> +static void virtballoon_changed(struct virtio_device *vdev)
> >> +{
> >> + struct virtio_balloon *vb = vdev->priv;
> >> +
> >> + wake_up(&vb->config_change);
> >> +}
> >> +
> >> +static inline bool config_need_stats(struct virtio_balloon *vb)
> >> +{
> >> + u32 v = 0;
> >> +
> >> + vb->vdev->config->get(vb->vdev,
> >> + offsetof(struct virtio_balloon_config,
> >> + need_stats),
> >> + &v, sizeof(v));
> >> + return (v != 0);
> >> +}
> >> +
> >> +static inline u32 config_pages(struct virtio_balloon *vb)
> >> +{
> >> + u32 v = 0;
> >> +
> >> + vb->vdev->config->get(vb->vdev,
> >> + offsetof(struct virtio_balloon_config, num_pages),
> >> + &v, sizeof(v));
> >> + return v;
> >> +}
> >> +
> >> +static inline s64 towards_target(struct virtio_balloon *vb)
> >> +{
> >> + struct address_space *mapping = the_inode.inode.i_mapping;
> >> + u32 v = config_pages(vb);
> >> +
> >> + return (s64)v - (mapping ? mapping->nrpages : 0);
> >> +}
> >> +
> >> +static void update_balloon_size(struct virtio_balloon *vb)
> >> +{
> >> + struct address_space *mapping = the_inode.inode.i_mapping;
> >> + __le32 actual = cpu_to_le32((mapping ? mapping->nrpages : 0));
> >> +
> >> + vb->vdev->config->set(vb->vdev,
> >> + offsetof(struct virtio_balloon_config, actual),
> >> + &actual, sizeof(actual));
> >> +}
> >> +
> >> +static void update_free_and_total(struct virtio_balloon *vb)
> >> +{
> >> + struct sysinfo i;
> >> + u32 value;
> >> +
> >> + si_meminfo(&i);
> >> +
> >> + update_balloon_stats(vb);
> >> + value = i.totalram;
> >> + vb->vdev->config->set(vb->vdev,
> >> + offsetof(struct virtio_balloon_config,
> >> + pages_total),
> >> + &value, sizeof(value));
> >> + value = i.freeram;
> >> + vb->vdev->config->set(vb->vdev,
> >> + offsetof(struct virtio_balloon_config,
> >> + pages_free),
> >> + &value, sizeof(value));
> >> + value = 0;
> >> + vb->vdev->config->set(vb->vdev,
> >> + offsetof(struct virtio_balloon_config,
> >> + need_stats),
> >> + &value, sizeof(value));
> >> +}
> >> +
> >> +static int balloon(void *_vballoon)
> >> +{
> >> + struct virtio_balloon *vb = _vballoon;
> >> +
> >> + set_freezable();
> >> + while (!kthread_should_stop()) {
> >> + s64 diff;
> >> + try_to_freeze();
> >> + wait_event_interruptible(vb->config_change,
> >> + (diff = towards_target(vb)) > 0
> >> + || config_need_stats(vb)
> >> + || kthread_should_stop()
> >> + || freezing(current));
> >> + if (config_need_stats(vb))
> >> + update_free_and_total(vb);
> >> + if (diff > 0) {
> >> + unsigned long reclaim_time = vb->last_reclaim + 2 * HZ;
> >> + /*
> >> + * Don't fill the balloon if a page reclaim happened in
> >> + * the past 2 seconds.
> >> + */
> >> + if (time_after_eq(reclaim_time, jiffies)) {
> >> + /* Inflating too fast--sleep and skip. */
> >> + msleep(500);
> >> + } else {
> >> + fill_balloon(vb, diff);
> >> + }
> >> + } else if (diff < 0 && config_pages(vb) == 0) {
> >> + /*
> >> + * Here we are specifically looking to detect the case
> >> + * where there are pages in the page cache, but the
> >> + * device wants us to go to 0. This is used in save/
> >> + * restore since the host device doesn't keep track of
> >> + * PFNs, and must flush the page cache on restore
> >> + * (which loses the context of the original device
> >> + * instance). However, we still suggest syncing the
> >> + * diff so that we can get within the target range.
> >> + */
> >> + s64 nr_to_write =
> >> + (!config_pages(vb) ? LONG_MAX : -diff);
> >> + struct writeback_control wbc = {
> >> + .sync_mode = WB_SYNC_ALL,
> >> + .nr_to_write = nr_to_write,
> >> + .range_start = 0,
> >> + .range_end = LLONG_MAX,
> >> + };
> >> + sync_inode(&the_inode.inode, &wbc);
> >> + }
> >> + update_balloon_size(vb);
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static ssize_t virtballoon_attr_show(struct device *dev,
> >> + struct device_attribute *attr,
> >> + char *buf);
> >> +
> >> +static DEVICE_ATTR(total_memory, 0644,
> >> + virtballoon_attr_show, NULL);
> >> +
> >> +static DEVICE_ATTR(free_memory, 0644,
> >> + virtballoon_attr_show, NULL);
> >> +
> >> +static DEVICE_ATTR(target_pages, 0644,
> >> + virtballoon_attr_show, NULL);
> >> +
> >> +static DEVICE_ATTR(actual_pages, 0644,
> >> + virtballoon_attr_show, NULL);
> >> +
> >> +static struct attribute *virtballoon_attrs[] = {
> >> + &dev_attr_total_memory.attr,
> >> + &dev_attr_free_memory.attr,
> >> + &dev_attr_target_pages.attr,
> >> + &dev_attr_actual_pages.attr,
> >> + NULL
> >> +};
> >> +static struct attribute_group virtballoon_attr_group = {
> >> + .name = "virtballoon",
> >> + .attrs = virtballoon_attrs,
> >> +};
> >> +
> >> +static ssize_t virtballoon_attr_show(struct device *dev,
> >> + struct device_attribute *attr,
> >> + char *buf)
> >> +{
> >> + struct address_space *mapping = the_inode.inode.i_mapping;
> >> + struct virtio_device *vdev = container_of(dev, struct virtio_device,
> >> + dev);
> >> + struct virtio_balloon *vb = vdev->priv;
> >> + unsigned long long value = 0;
> >> + if (attr == &dev_attr_total_memory)
> >> + value = vb->stats[VIRTIO_BALLOON_S_MEMTOT].val;
> >> + else if (attr == &dev_attr_free_memory)
> >> + value = vb->stats[VIRTIO_BALLOON_S_MEMFREE].val;
> >> + else if (attr == &dev_attr_target_pages)
> >> + value = config_pages(vb);
> >> + else if (attr == &dev_attr_actual_pages)
> >> + value = cpu_to_le32((mapping ? mapping->nrpages : 0));
> >> + return sprintf(buf, "%llu\n", value);
> >> +}
> >> +
> >> +static int virtballoon_probe(struct virtio_device *vdev)
> >> +{
> >> + struct virtio_balloon *vb;
> >> + struct virtqueue *vq[1];
> >> + vq_callback_t *callback = balloon_ack;
> >> + const char *name = "inflate";
> >> + int err;
> >> +
> >> + vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
> >> + if (!vb) {
> >> + err = -ENOMEM;
> >> + goto out;
> >> + }
> >> +
> >> + init_waitqueue_head(&vb->config_change);
> >> + vb->vdev = vdev;
> >> +
> >> + /* We use one virtqueue: inflate */
> >> + err = vdev->config->find_vqs(vdev, 1, vq, &callback, &name);
> >> + if (err)
> >> + goto out_free_vb;
> >> +
> >> + vb->inflate_vq = vq[0];
> >> +
> >> + err = sysfs_create_group(&vdev->dev.kobj, &virtballoon_attr_group);
> >> + if (err) {
> >> + pr_err("Failed to create virtballoon sysfs node\n");
> >> + goto out_free_vb;
> >> + }
> >> +
> >> + vb->last_scan_page_array = 0;
> >> + vb->last_reclaim = 0;
> >> + the_inode.vb = vb;
> >> +
> >> + vb->thread = kthread_run(balloon, vb, "vballoon");
> >> + if (IS_ERR(vb->thread)) {
> >> + err = PTR_ERR(vb->thread);
> >> + goto out_del_vqs;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +out_del_vqs:
> >> + vdev->config->del_vqs(vdev);
> >> +out_free_vb:
> >> + kfree(vb);
> >> +out:
> >> + return err;
> >> +}
> >> +
> >> +static void __devexit virtballoon_remove(struct virtio_device *vdev)
> >> +{
> >> + struct virtio_balloon *vb = vdev->priv;
> >> +
> >> + kthread_stop(vb->thread);
> >> +
> >> + sysfs_remove_group(&vdev->dev.kobj, &virtballoon_attr_group);
> >> +
> >> + /* Now we reset the device so we can clean up the queues. */
> >> + vdev->config->reset(vdev);
> >> +
> >> + vdev->config->del_vqs(vdev);
> >> + kfree(vb);
> >> +}
> >> +
> >> +static struct virtio_driver virtio_balloon_driver = {
> >> + .feature_table = NULL,
> >> + .feature_table_size = 0,
> >> + .driver.name = KBUILD_MODNAME,
> >> + .driver.owner = THIS_MODULE,
> >> + .id_table = id_table,
> >> + .probe = virtballoon_probe,
> >> + .remove = __devexit_p(virtballoon_remove),
> >> + .config_changed = virtballoon_changed,
> >> +};
> >> +
> >> +static int __init init(void)
> >> +{
> >> + int err = register_filesystem(&balloon_fs_type);
> >> + if (err)
> >> + goto out;
> >> +
> >> + balloon_mnt = kern_mount(&balloon_fs_type);
> >> + if (IS_ERR(balloon_mnt)) {
> >> + err = PTR_ERR(balloon_mnt);
> >> + goto out_filesystem;
> >> + }
> >> +
> >> + err = register_virtio_driver(&virtio_balloon_driver);
> >> + if (err)
> >> + goto out_filesystem;
> >> +
> >> + goto out;
> >> +
> >> +out_filesystem:
> >> + unregister_filesystem(&balloon_fs_type);
> >> +
> >> +out:
> >> + return err;
> >> +}
> >> +
> >> +static void __exit fini(void)
> >> +{
> >> + if (balloon_mnt) {
> >> + unregister_filesystem(&balloon_fs_type);
> >> + balloon_mnt = NULL;
> >> + }
> >> + unregister_virtio_driver(&virtio_balloon_driver);
> >> +}
> >> +module_init(init);
> >> +module_exit(fini);
> >> +
> >> +MODULE_DEVICE_TABLE(virtio, id_table);
> >> +MODULE_DESCRIPTION("Virtio file (page cache-backed) balloon driver");
> >> +MODULE_LICENSE("GPL");
> >> diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
> >> index 652dc8b..2be9a02 100644
> >> --- a/include/linux/virtio_balloon.h
> >> +++ b/include/linux/virtio_balloon.h
> >> @@ -41,6 +41,15 @@ struct virtio_balloon_config
> >> __le32 num_pages;
> >> /* Number of pages we've actually got in balloon. */
> >> __le32 actual;
> >> +#if defined(CONFIG_VIRTIO_FILEBALLOON) ||\
> >> + defined(CONFIG_VIRTIO_FILEBALLOON_MODULE)
> >> + /* Total pages on this system. */
> >> + __le32 pages_total;
> >> + /* Free pages on this system. */
> >> + __le32 pages_free;
> >> + /* If the device needs pages_total/pages_free updated. */
> >> + __le32 need_stats;
> >> +#endif
> >> };
> >>
> >> #define VIRTIO_BALLOON_S_SWAP_IN 0 /* Amount of memory swapped in */
> >> diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
> >> index 7529b85..2f081d7 100644
> >> --- a/include/linux/virtio_ids.h
> >> +++ b/include/linux/virtio_ids.h
> >> @@ -37,5 +37,6 @@
> >> #define VIRTIO_ID_RPMSG 7 /* virtio remote processor messaging */
> >> #define VIRTIO_ID_SCSI 8 /* virtio scsi */
> >> #define VIRTIO_ID_9P 9 /* 9p virtio console */
> >> +#define VIRTIO_ID_FILE_BALLOON 10 /* virtio file-backed balloon */
> >>
> >> #endif /* _LINUX_VIRTIO_IDS_H */
> >> --
> >> 1.7.7.3
^ permalink raw reply
* Re: [PATCH] Add a page cache-backed balloon device driver.
From: Mike Waychison @ 2012-09-10 20:49 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Frank Swiderski, Andrea Arcangeli, Rik van Riel, kvm,
linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <20120910195931.GD20721@redhat.com>
On Mon, Sep 10, 2012 at 3:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Sep 10, 2012 at 01:37:06PM -0400, Mike Waychison wrote:
>> On Mon, Sep 10, 2012 at 5:05 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Tue, Jun 26, 2012 at 01:32:58PM -0700, Frank Swiderski wrote:
>> >> This implementation of a virtio balloon driver uses the page cache to
>> >> "store" pages that have been released to the host. The communication
>> >> (outside of target counts) is one way--the guest notifies the host when
>> >> it adds a page to the page cache, allowing the host to madvise(2) with
>> >> MADV_DONTNEED. Reclaim in the guest is therefore automatic and implicit
>> >> (via the regular page reclaim). This means that inflating the balloon
>> >> is similar to the existing balloon mechanism, but the deflate is
>> >> different--it re-uses existing Linux kernel functionality to
>> >> automatically reclaim.
>> >>
>> >> Signed-off-by: Frank Swiderski <fes@google.com>
>>
>> Hi Michael,
>>
>> I'm very sorry that Frank and I have been silent on these threads.
>> I've been out of the office and Frank has been been swamped :)
>>
>> I'll take a stab at answering some of your questions below, and
>> hopefully we can end up on the same page.
>>
>> > I've been trying to understand this, and I have
>> > a question: what exactly is the benefit
>> > of this new device?r balloon is told upper limit on target size by host and pulls
>>
>> The key difference between this device/driver and the pre-existing
>> virtio_balloon device/driver is in how the memory pressure loop is
>> controlled.
>>
>> With the pre-existing balloon device/driver, the control loop for how
>> much memory a given VM is allowed to use is controlled completely by
>> the host. This is probably fine if the goal is to pack as much work
>> on a given host as possible, but it says nothing about the expected
>> performance that any given VM is expecting to have. Specifically, it
>> allows the host to set a target goal for the size of a VM, and the
>> driver in the guest does whatever is needed to get to that goal. This
>> is great for systems where one wants to "grow or shrink" a VM from the
>> outside.
>>
>>
>> This behaviour however doesn't match what applications actually expectr balloon is told upper limit on target size by host and pulls
>> from a memory control loop however. In a native setup, an application
>> can usually expect to allocate memory from the kernel on an as-needed
>> basis, and can in turn return memory back to the system (using a heap
>> implementation that actually releases memory that is). The dynamic
>> size of an application is completely controlled by the application,
>> and there is very little that cluster management software can do to
>> ensure that the application fits some prescribed size.
>>
>> We recognized this in the development of our cluster management
>> software long ago, so our systems are designed for managing tasks that
>> have a dynamic memory footprint. Overcommit is possible (as most
>> applications do not use the full reservation of memory they asked for
>> originally), letting us do things like schedule lower priority/lower
>> service-classification work using resources that are otherwise
>> available in stand-by for high-priority/low-latency workloads.
>
> OK I am not sure I got this right so pls tell me if this summary is
> correct (note: this does not talk about what guest does with memory,
> ust what it is that device does):
>
> - existing balloon is told lower limit on target size by host and pulls in at least
> target size. Guest can inflate > target size if it likes
> and then it is OK to deflate back to target size but not less.
Is this true? I take it nothing is keeping the existing balloon
driver from going over the target, but the same can be said about
either balloon implementation.
> - your balloon is told upper limit on target size by host and pulls at most
> target size. Guest can deflate down to 0 at any point.
>
> If so I think both approaches make sense and in fact they
> can be useful at the same time for the same guest.
> In that case, I see two ways how this can be done:
>
> 1. two devices: existing ballon + cache balloon the
> 2. add "upper limit" to existing ballon
>
> A single device looks a bit more natural in that we don't
> really care in which balloon a page is as long as we
> are between lower and upper limit. Right?
I agree that this may be better done using a single device if possible.
> From implementation POV we could have it use
> pagecache for pages above lower limit but that
> is a separate question about driver design,
> I would like to make sure I understand the highr balloon is told upper limit on tr balloon is told upper limit on target size by host and pullsarget size by host and pulls
> level design first.
I agree that this is an implementation detail that is separate from
discussions of high and low limits. That said, there are several
advantages to pushing these pages to the page cache (memory defrag
still works for one).
>> > Note that users could not care less about how a driver
>> > is implemented internally.
>> >
>> > Is there some workload where you see VM working better with
>> > this than regular balloon? Any numbers?
>>
>> This device is less about performance as it is about getting the
>> memory size of a job (or in this case, a job in a VM) to grow and
>> shrink as the application workload sees fit, much like how processes
>> today can grow and shrink without external direction.
>
> Still, e.g. swap in host achieves more or less the same functionality.
Swap comes at the extremely prejudiced cost of latency. Swap is very
very rarely used in our production environment for this reason.
> I am guessing balloon can work better by getting more cooperation
> from guest but aren't there any tests showing this is true in practice?
There aren't any meaningful test-specific numbers that I can readily
share unfortunately :( If you have suggestions for specific things we
should try, that may be useful.
The way this change is validated on our end is to ensure that VM
processes on the host "shrink" to a reasonable working set in size
that is near-linear with the expected working set size for the
embedded tasks as if they were running native on the host. Making
this happen with the current balloon just isn't possible as there
isn't enough visibility on the host as to how much pressure there is
in the guest.
>
>
>> >
>> > Also, can't we just replace existing balloon implementation
>> > with this one?
>>
>> Perhaps, but as described above, both devices have very different
>> characteristics.
>>
>> > Why it is so important to deflate silently?
>>
>> It may not be so important to deflate silently. I'm not sure why it
>> is important that we deflate "loudly" though either :) Doing so seems
>> like unnecessary guest/host communication IMO, especially if the guest
>> is expecting to be able to grow to totalram (and the host isn't able
>> to nack any pages reclaimed anyway...).
>
> First, we could add nack easily enough :)
:) Sure. Not sure how the driver is going to expect to handle that though ! :D
> Second, access gets an exit anyway. If you tell
> host first you can maybe batch these and actually speed things up.
> It remains to be measured but historically we told host
> so the onus of proof would be on whoever wants to remove this.
I'll concede that there isn't a very compelling argument as to why the
balloon should deflate silently. You are right that it may be better
to deflate in batches (amortizing exit costs). That said, it isn't
totally obvious that queue'ing pfns to the virtio queue is the right
thing to do algorithmically either. Currently, the file balloon
driver can reclaim memory inline with memory reclaim (via the
->writepage callback). Doing otherwise may cause the LRU shrinking to
queue large numbers of pages to the virtio queue, without any
immediate progress made with regards to actually freeing memory. I'm
worried that such an enqueue scheme will cause large bursts of pages
to be deflated unnecessarily when we go into reclaim.
On the plus side, having an exit taken here on each page turns out to
be relatively cheap, as the vmexit from the page fault should be
faster to process as it is fully handled within the host kernel.
Perhaps some combination of both methods is required? I'm not sure :\
>
> Third, see discussion on ML - we came up with
> the idea of locking/unlocking balloon memory
> which is useful for an assigned device.
> Requires telling host first.
I just skimmed the other thread (sorry, I'm very much backlogged on
email). By "locking", does this mean pinning the pages so that they
are not changed?
I'll admit that I'm not familiar with the details for device
assignment. If a page for a given bus address isn't present in the
IOMMU, does this not result in a serviceable fault?
>
> Also knowing how much memory there is in a balloon
> would be useful for admin.
This is just another counter and should already be exposed.
>
> There could be other uses.
>
>> > I guess filesystem does not currently get a callback
>> > before page is reclaimed but this isan implementation detail -
>> > maybe this can be fixed?
>>
>> I do not follow this question.
>
> Assume we want to tell host before use.
> Can you implement this on top of your patch?
Potentially, yes. Both drivers are bare-bones at the moment IIRC and
don't support sending multiple outstanding commands to the host, but
this could be conceivably fixed (although one would have to work out
what happens when virtio_add_buf() returns -ENOBUFS).
>
>> >
>> > Also can you pls answer Avi's question?
>> > How is overcommit managed?
>>
>> Overcommit in our deployments is managed using memory cgroups on the
>> host. This allows us to have very directed policies as to how
>> competing VMs on a host may overcommit.
>
> So you push VM out to swap if it's over allowed memory?
As mentioned above, we don't use swap. If the task is of a lower
service band, it may end up blocking a lot more waiting for host
memory to become available, or may even be killed by the system and
restarted elsewhere. Tasks that are of the higher service bands will
cause other tasks of lower service band to give up the ram (by will or
by force).
> Existing balloon does this better as it is cooperative,
> it seems.
^ permalink raw reply
* Re: [PATCH] Add a page cache-backed balloon device driver.
From: Michael S. Tsirkin @ 2012-09-10 21:10 UTC (permalink / raw)
To: Mike Waychison
Cc: Frank Swiderski, Andrea Arcangeli, Rik van Riel, kvm,
linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <CAGTjWtCiqjkR_Ze=0-ebO08+eziz6c8TXLb+O1iiArRQeSE6zw@mail.gmail.com>
On Mon, Sep 10, 2012 at 04:49:40PM -0400, Mike Waychison wrote:
> On Mon, Sep 10, 2012 at 3:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Sep 10, 2012 at 01:37:06PM -0400, Mike Waychison wrote:
> >> On Mon, Sep 10, 2012 at 5:05 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Tue, Jun 26, 2012 at 01:32:58PM -0700, Frank Swiderski wrote:
> >> >> This implementation of a virtio balloon driver uses the page cache to
> >> >> "store" pages that have been released to the host. The communication
> >> >> (outside of target counts) is one way--the guest notifies the host when
> >> >> it adds a page to the page cache, allowing the host to madvise(2) with
> >> >> MADV_DONTNEED. Reclaim in the guest is therefore automatic and implicit
> >> >> (via the regular page reclaim). This means that inflating the balloon
> >> >> is similar to the existing balloon mechanism, but the deflate is
> >> >> different--it re-uses existing Linux kernel functionality to
> >> >> automatically reclaim.
> >> >>
> >> >> Signed-off-by: Frank Swiderski <fes@google.com>
> >>
> >> Hi Michael,
> >>
> >> I'm very sorry that Frank and I have been silent on these threads.
> >> I've been out of the office and Frank has been been swamped :)
> >>
> >> I'll take a stab at answering some of your questions below, and
> >> hopefully we can end up on the same page.
> >>
> >> > I've been trying to understand this, and I have
> >> > a question: what exactly is the benefit
> >> > of this new device?r balloon is told upper limit on target size by host and pulls
> >>
> >> The key difference between this device/driver and the pre-existing
> >> virtio_balloon device/driver is in how the memory pressure loop is
> >> controlled.
> >>
> >> With the pre-existing balloon device/driver, the control loop for how
> >> much memory a given VM is allowed to use is controlled completely by
> >> the host. This is probably fine if the goal is to pack as much work
> >> on a given host as possible, but it says nothing about the expected
> >> performance that any given VM is expecting to have. Specifically, it
> >> allows the host to set a target goal for the size of a VM, and the
> >> driver in the guest does whatever is needed to get to that goal. This
> >> is great for systems where one wants to "grow or shrink" a VM from the
> >> outside.
> >>
> >>
> >> This behaviour however doesn't match what applications actually expectr balloon is told upper limit on target size by host and pulls
> >> from a memory control loop however. In a native setup, an application
> >> can usually expect to allocate memory from the kernel on an as-needed
> >> basis, and can in turn return memory back to the system (using a heap
> >> implementation that actually releases memory that is). The dynamic
> >> size of an application is completely controlled by the application,
> >> and there is very little that cluster management software can do to
> >> ensure that the application fits some prescribed size.
> >>
> >> We recognized this in the development of our cluster management
> >> software long ago, so our systems are designed for managing tasks that
> >> have a dynamic memory footprint. Overcommit is possible (as most
> >> applications do not use the full reservation of memory they asked for
> >> originally), letting us do things like schedule lower priority/lower
> >> service-classification work using resources that are otherwise
> >> available in stand-by for high-priority/low-latency workloads.
> >
> > OK I am not sure I got this right so pls tell me if this summary is
> > correct (note: this does not talk about what guest does with memory,
> > ust what it is that device does):
> >
> > - existing balloon is told lower limit on target size by host and pulls in at least
> > target size. Guest can inflate > target size if it likes
> > and then it is OK to deflate back to target size but not less.
>
> Is this true? I take it nothing is keeping the existing balloon
> driver from going over the target, but the same can be said about
> either balloon implementation.
>
> > - your balloon is told upper limit on target size by host and pulls at most
> > target size. Guest can deflate down to 0 at any point.
> >
> > If so I think both approaches make sense and in fact they
> > can be useful at the same time for the same guest.
> > In that case, I see two ways how this can be done:
> >
> > 1. two devices: existing ballon + cache balloon the
> > 2. add "upper limit" to existing ballon
> >
> > A single device looks a bit more natural in that we don't
> > really care in which balloon a page is as long as we
> > are between lower and upper limit. Right?
>
> I agree that this may be better done using a single device if possible.
I am not sure myself, just asking.
> > From implementation POV we could have it use
> > pagecache for pages above lower limit but that
> > is a separate question about driver design,
> > I would like to make sure I understand the highr balloon is told upper limit on tr balloon is told upper limit on target size by host and pullsarget size by host and pulls
> > level design first.
>
> I agree that this is an implementation detail that is separate from
> discussions of high and low limits. That said, there are several
> advantages to pushing these pages to the page cache (memory defrag
> still works for one).
I'm not arguing against it at all.
> >> > Note that users could not care less about how a driver
> >> > is implemented internally.
> >> >
> >> > Is there some workload where you see VM working better with
> >> > this than regular balloon? Any numbers?
> >>
> >> This device is less about performance as it is about getting the
> >> memory size of a job (or in this case, a job in a VM) to grow and
> >> shrink as the application workload sees fit, much like how processes
> >> today can grow and shrink without external direction.
> >
> > Still, e.g. swap in host achieves more or less the same functionality.
>
> Swap comes at the extremely prejudiced cost of latency. Swap is very
> very rarely used in our production environment for this reason.
>
> > I am guessing balloon can work better by getting more cooperation
> > from guest but aren't there any tests showing this is true in practice?
>
> There aren't any meaningful test-specific numbers that I can readily
> share unfortunately :( If you have suggestions for specific things we
> should try, that may be useful.
>
> The way this change is validated on our end is to ensure that VM
> processes on the host "shrink" to a reasonable working set in size
> that is near-linear with the expected working set size for the
> embedded tasks as if they were running native on the host. Making
> this happen with the current balloon just isn't possible as there
> isn't enough visibility on the host as to how much pressure there is
> in the guest.
>
> >
> >
> >> >
> >> > Also, can't we just replace existing balloon implementation
> >> > with this one?
> >>
> >> Perhaps, but as described above, both devices have very different
> >> characteristics.
> >>
> >> > Why it is so important to deflate silently?
> >>
> >> It may not be so important to deflate silently. I'm not sure why it
> >> is important that we deflate "loudly" though either :) Doing so seems
> >> like unnecessary guest/host communication IMO, especially if the guest
> >> is expecting to be able to grow to totalram (and the host isn't able
> >> to nack any pages reclaimed anyway...).
> >
> > First, we could add nack easily enough :)
>
> :) Sure. Not sure how the driver is going to expect to handle that though ! :D
Not sure about pagecache backed - regular one can just hang on
to the page for a while more and try later or with another page.
> > Second, access gets an exit anyway. If you tell
> > host first you can maybe batch these and actually speed things up.
> > It remains to be measured but historically we told host
> > so the onus of proof would be on whoever wants to remove this.
>
> I'll concede that there isn't a very compelling argument as to why the
> balloon should deflate silently. You are right that it may be better
> to deflate in batches (amortizing exit costs). That said, it isn't
> totally obvious that queue'ing pfns to the virtio queue is the right
> thing to do algorithmically either. Currently, the file balloon
> driver can reclaim memory inline with memory reclaim (via the
> ->writepage callback). Doing otherwise may cause the LRU shrinking to
> queue large numbers of pages to the virtio queue, without any
> immediate progress made with regards to actually freeing memory. I'm
> worried that such an enqueue scheme will cause large bursts of pages
> to be deflated unnecessarily when we go into reclaim.
Yes it would seem writepage is not a good mechanism since
it can try to write pages speculatively.
Maybe add a flag to tell LRU to only write pages when
we really need the memory?
> On the plus side, having an exit taken here on each page turns out to
> be relatively cheap, as the vmexit from the page fault should be
> faster to process as it is fully handled within the host kernel.
>
> Perhaps some combination of both methods is required? I'm not sure :\
Perhaps some benchmarking is in order :)
Can you try telling host, potentially MADV_WILL_NEED
in that case like qemu does, then run your proprietary test
and see if things work well enough?
> >
> > Third, see discussion on ML - we came up with
> > the idea of locking/unlocking balloon memory
> > which is useful for an assigned device.
> > Requires telling host first.
>
> I just skimmed the other thread (sorry, I'm very much backlogged on
> email). By "locking", does this mean pinning the pages so that they
> are not changed?
Yes by get user pages.
> I'll admit that I'm not familiar with the details for device
> assignment. If a page for a given bus address isn't present in the
> IOMMU, does this not result in a serviceable fault?
Yes.
> >
> > Also knowing how much memory there is in a balloon
> > would be useful for admin.
>
> This is just another counter and should already be exposed.
>
> >
> > There could be other uses.
> >
> >> > I guess filesystem does not currently get a callback
> >> > before page is reclaimed but this isan implementation detail -
> >> > maybe this can be fixed?
> >>
> >> I do not follow this question.
> >
> > Assume we want to tell host before use.
> > Can you implement this on top of your patch?
>
> Potentially, yes. Both drivers are bare-bones at the moment IIRC and
> don't support sending multiple outstanding commands to the host, but
> this could be conceivably fixed (although one would have to work out
> what happens when virtio_add_buf() returns -ENOBUFS).
It's not enough to add buf. You need to wait for host ack.
Once you got ack you know you can add another buf.
> >
> >> >
> >> > Also can you pls answer Avi's question?
> >> > How is overcommit managed?
> >>
> >> Overcommit in our deployments is managed using memory cgroups on the
> >> host. This allows us to have very directed policies as to how
> >> competing VMs on a host may overcommit.
> >
> > So you push VM out to swap if it's over allowed memory?
>
> As mentioned above, we don't use swap. If the task is of a lower
> service band, it may end up blocking a lot more waiting for host
> memory to become available, or may even be killed by the system and
> restarted elsewhere. Tasks that are of the higher service bands will
> cause other tasks of lower service band to give up the ram (by will or
> by force).
Right. I think the comment below applies.
> > Existing balloon does this better as it is cooperative,
> > it seems.
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Michael S. Tsirkin @ 2012-09-10 22:11 UTC (permalink / raw)
To: Rusty Russell
Cc: Sjur Brændeland, Linus Walleij, linux-kernel, virtualization,
Amit Shah
In-Reply-To: <20120906052735.GB17656@redhat.com>
On Thu, Sep 06, 2012 at 08:27:35AM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
> > >> Hi Michael,
> > >>
> > >> > Exactly. Though if we just fail load it will be much less code.
> > >> >
> > >> > Generally, using a feature bit for this is a bit of a problem though:
> > >> > normally driver is expected to be able to simply ignore
> > >> > a feature bit. In this case driver is required to
> > >> > do something so a feature bit is not a good fit.
> > >> > I am not sure what the right thing to do is.
> > >>
> > >> I see - so in order to avoid the binding between driver and device
> > >> there are two options I guess. Either make virtio_dev_match() or
> > >> virtcons_probe() fail. Neither of them seems like the obvious choice.
> > >>
> > >> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match
> > >> between device and driver in virtcons_probe() is the lesser evil?
> > >>
> > >> Regards,
> > >> Sjur
> > >
> > > A simplest thing to do is change dev id. rusty?
> >
> > For generic usage, this is correct. But my opinion is that fallback on
> > feature non-ack is quality-of-implementation issue: great to have, but
> > there are cases where you just want to fail with "you're too old".
> >
> > And in this case, an old system simply will never work. So it's a
> > question of how graceful the failure is.
> >
> > Can your userspace loader can refuse to proceed if the driver doesn't
> > ack the bits? If so, it's simpler than a whole new ID.
> >
> > Cheers,
> > Rusty.
>
> Yes but how can it signal guest that it will never proceed?
>
> Also grep for BUG_ON in core found this:
>
> drv->remove(dev);
>
> /* Driver should have reset device. */
> BUG_ON(dev->config->get_status(dev));
>
> I think below is what Sjur refers to.
> I think below is a good idea for 3.6. Thoughts?
>
> --->
>
> virtio: don't crash when device is buggy
>
> Because of a sanity check in virtio_dev_remove, a buggy device can crash
> kernel. And in case of rproc it's userspace so it's not a good idea.
> We are unloading a driver so how bad can it be?
> Be less aggressive in handling this error: if it's a driver bug,
> warning once should be enough.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> --
Rusty?
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index c3b3f7f..1e8659c 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -159,7 +159,7 @@ static int virtio_dev_remove(struct device *_d)
> drv->remove(dev);
>
> /* Driver should have reset device. */
> - BUG_ON(dev->config->get_status(dev));
> + WARN_ON_ONCE(dev->config->get_status(dev));
>
> /* Acknowledge the device's existence again. */
> add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>
> --
> MST
^ permalink raw reply
* Re: [PATCH 0/5] vhost-scsi: Add support for host virtualized target
From: Asias He @ 2012-09-11 4:36 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin, Jan Kiszka,
qemu-devel, Zhi Yong Wu, Anthony Liguori, target-devel,
Paolo Bonzini, lf-virt, Christoph Hellwig
In-Reply-To: <1347000499-28701-1-git-send-email-nab@linux-iscsi.org>
Hello Nicholas,
On 09/07/2012 02:48 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Hello Anthony & Co,
>
> This is the fourth installment to add host virtualized target support for
> the mainline tcm_vhost fabric driver using Linux v3.6-rc into QEMU 1.3.0-rc.
>
> The series is available directly from the following git branch:
>
> git://git.kernel.org/pub/scm/virt/kvm/nab/qemu-kvm.git vhost-scsi-for-1.3
>
> Note the code is cut against yesterday's QEMU head, and dispite the name
> of the tree is based upon mainline qemu.org git code + has thus far been
> running overnight with > 100K IOPs small block 4k workloads using v3.6-rc2+
> based target code with RAMDISK_DR backstores.
Are you still seeing the performance degradation discussed in the thread
"vhost-scsi port to v1.1.0 + MSI-X performance regression"
?
> Other than some minor fuzz between jumping from QEMU 1.2.0 -> 1.2.50, this
> series is functionally identical to what's been posted for vhost-scsi RFC-v3
> to qemu-devel.
>
> Please consider applying these patches for an initial vhost-scsi merge into
> QEMU 1.3.0-rc code, or let us know what else you'd like to see addressed for
> this series to in order to merge.
>
> Thank you!
>
> --nab
>
> Nicholas Bellinger (2):
> monitor: Rename+move net_handle_fd_param -> monitor_handle_fd_param
> virtio-scsi: Set max_target=0 during vhost-scsi operation
>
> Stefan Hajnoczi (3):
> vhost: Pass device path to vhost_dev_init()
> vhost-scsi: add -vhost-scsi host device for use with tcm-vhost
> virtio-scsi: Add start/stop functionality for vhost-scsi
>
> configure | 10 +++
> hw/Makefile.objs | 1 +
> hw/qdev-properties.c | 41 +++++++++++
> hw/vhost-scsi.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/vhost-scsi.h | 62 ++++++++++++++++
> hw/vhost.c | 5 +-
> hw/vhost.h | 3 +-
> hw/vhost_net.c | 2 +-
> hw/virtio-pci.c | 2 +
> hw/virtio-scsi.c | 55 ++++++++++++++-
> hw/virtio-scsi.h | 1 +
> monitor.c | 18 +++++
> monitor.h | 1 +
> net.c | 18 -----
> net.h | 2 -
> net/socket.c | 2 +-
> net/tap.c | 4 +-
> qemu-common.h | 1 +
> qemu-config.c | 19 +++++
> qemu-options.hx | 4 +
> vl.c | 18 +++++
> 21 files changed, 431 insertions(+), 28 deletions(-)
> create mode 100644 hw/vhost-scsi.c
> create mode 100644 hw/vhost-scsi.h
>
--
Asias
^ permalink raw reply
* Re: [PATCH 4/5] virtio-scsi: Add start/stop functionality for vhost-scsi
From: Anthony Liguori @ 2012-09-11 13:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, kvm-devel, Jan Kiszka, qemu-devel, lf-virt,
Anthony Liguori, target-devel, Paolo Bonzini, Zhi Yong Wu,
Christoph Hellwig
In-Reply-To: <20120910062437.GD16819@redhat.com>
On 09/10/2012 01:24 AM, Michael S. Tsirkin wrote:
> On Mon, Sep 10, 2012 at 08:16:54AM +0200, Paolo Bonzini wrote:
>> Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto:
>>> On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote:
>>>> Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto:
>>>>> Cc: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>
>>>>> Cc: Zhi Yong Wu<wuzhy@linux.vnet.ibm.com>
>>>>> Cc: Michael S. Tsirkin<mst@redhat.com>
>>>>> Cc: Paolo Bonzini<pbonzini@redhat.com>
>>>>> Signed-off-by: Nicholas Bellinger<nab@linux-iscsi.org>
>>>>> ---
>>>>> hw/virtio-pci.c | 2 ++
>>>>> hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> hw/virtio-scsi.h | 1 +
>>>>> 3 files changed, 52 insertions(+), 0 deletions(-)
>>>>
>>>> Please create a completely separate device vhost-scsi-pci instead (or
>>>> virtio-scsi-tcm-pci, or something like that). It is used completely
>>>> differently from virtio-scsi-pci, it does not make sense to conflate the
>>>> two.
>>>
>>> Ideally the name would say how it is different, not what backend it
>>> uses. Any good suggestions?
>>
>> I chose the backend name because, ideally, there would be no other
>> difference. QEMU _could_ implement all the goodies in vhost-scsi (such
>> as reservations or ALUA), it just doesn't do that yet.
>>
>> Paolo
>
> Then why do you say "It is used completely differently from
> virtio-scsi-pci"? Isn't it just a different backend?
>
> If yes then it should be a backend option, like it is
> for virtio-net.
I don't mean to bike shed here so don't take this as a nack on making it a
backend option, but in retrospect, the way we did vhost-net was a mistake even
though I strongly advocated for it to be a backend option.
The code to do it is really, really ugly. I think it would have made a lot more
sense to just make it a device and then have it not use a netdev backend or any
other kind of backend split.
For instance:
qemu -device vhost-net-pci,tapfd=X
I know this breaks the model of separate backends and frontends but since
vhost-net absolutely requires a tap fd, I think it's better in the long run to
not abuse the netdev backend to prevent user confusion. Having a dedicated
backend type that only has one possible option and can only be used by one
device is a bit silly too.
So I would be in favor of dropping/squashing 3/5 and radically simplifying how
this was exposed to the user.
I would just take qemu_vhost_scsi_opts and make them device properties.
Regards,
Anthony Liguori
>
^ permalink raw reply
* Re: [PATCH 4/5] virtio-scsi: Add start/stop functionality for vhost-scsi
From: Michael S. Tsirkin @ 2012-09-11 15:07 UTC (permalink / raw)
To: Anthony Liguori
Cc: Stefan Hajnoczi, kvm-devel, Jan Kiszka, qemu-devel, lf-virt,
Anthony Liguori, target-devel, Paolo Bonzini, Zhi Yong Wu,
Christoph Hellwig
In-Reply-To: <504F40BA.7020800@us.ibm.com>
On Tue, Sep 11, 2012 at 08:46:34AM -0500, Anthony Liguori wrote:
> On 09/10/2012 01:24 AM, Michael S. Tsirkin wrote:
> >On Mon, Sep 10, 2012 at 08:16:54AM +0200, Paolo Bonzini wrote:
> >>Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto:
> >>>On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote:
> >>>>Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto:
> >>>>>Cc: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>
> >>>>>Cc: Zhi Yong Wu<wuzhy@linux.vnet.ibm.com>
> >>>>>Cc: Michael S. Tsirkin<mst@redhat.com>
> >>>>>Cc: Paolo Bonzini<pbonzini@redhat.com>
> >>>>>Signed-off-by: Nicholas Bellinger<nab@linux-iscsi.org>
> >>>>>---
> >>>>> hw/virtio-pci.c | 2 ++
> >>>>> hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>> hw/virtio-scsi.h | 1 +
> >>>>> 3 files changed, 52 insertions(+), 0 deletions(-)
> >>>>
> >>>>Please create a completely separate device vhost-scsi-pci instead (or
> >>>>virtio-scsi-tcm-pci, or something like that). It is used completely
> >>>>differently from virtio-scsi-pci, it does not make sense to conflate the
> >>>>two.
> >>>
> >>>Ideally the name would say how it is different, not what backend it
> >>>uses. Any good suggestions?
> >>
> >>I chose the backend name because, ideally, there would be no other
> >>difference. QEMU _could_ implement all the goodies in vhost-scsi (such
> >>as reservations or ALUA), it just doesn't do that yet.
> >>
> >>Paolo
> >
> >Then why do you say "It is used completely differently from
> >virtio-scsi-pci"? Isn't it just a different backend?
> >
> >If yes then it should be a backend option, like it is
> >for virtio-net.
>
> I don't mean to bike shed here so don't take this as a nack on
> making it a backend option, but in retrospect, the way we did
> vhost-net was a mistake even though I strongly advocated for it to
> be a backend option.
>
> The code to do it is really, really ugly. I think it would have
> made a lot more sense to just make it a device and then have it not
> use a netdev backend or any other kind of backend split.
>
> For instance:
>
> qemu -device vhost-net-pci,tapfd=X
>
> I know this breaks the model of separate backends and frontends but
> since vhost-net absolutely requires a tap fd, I think it's better in
> the long run to not abuse the netdev backend to prevent user
> confusion. Having a dedicated backend type that only has one
> possible option and can only be used by one device is a bit silly
> too.
>
> So I would be in favor of dropping/squashing 3/5 and radically
> simplifying how this was exposed to the user.
>
> I would just take qemu_vhost_scsi_opts and make them device properties.
>
> Regards,
>
> Anthony Liguori
I'd like to clarify that I'm fine with either approach.
Even a separate device is OK if this is what others want
though I like it the least.
> >
^ permalink raw reply
* Re: [PATCHv4] virtio-spec: virtio network device multiqueue support
From: Rusty Russell @ 2012-09-12 0:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, rick.jones2, virtualization, levinsasha928, pbonzini,
Tom Herbert
In-Reply-To: <20120910061629.GC16819@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> In other words RPS is a hack to speed up networking on cheapo
> hardware, this is one of the reasons it is off by default.
> Good hardware has multiple receive queues.
> We can implement a good one so we do not need RPS.
>
> Also not all guest OS-es support RPS.
>
> Does this clarify?
Ok, thanks.
BTW, I found a better description by Tom Herbert, BTW:
https://code.google.com/p/kernel/wiki/NetScalingGuide
Now, I find the description of VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX
confusing:
1) AFAICT it turns on multiqueue rx, with no semantics attached.
I have no idea why it's called what it is. Why?
2) We've said we can remove steering methods, but we haven't actually
defined any, as we've left it completely open.
If I were a driver author, it leaves me completely baffled on how to
implement the spec :(
What are we actually planning to implement at the moment?
>For best performance, packets from a single connection should utilize
>the paired transmit and receive queues from the same virtqueue pair;
>for example both transmitqN and receiveqN. This rule makes it possible
>to optimize processing on the device side, but this is not a hard
>requirement: devices should function correctly even when this rule is
>not followed.
Why is this true? I don't actually see why the queues are in pairs at
all; are tx and rx not completely independent? So why does it matter?
>> When the steering rule is modified, some packets can still be
>> outstanding in one or more of the virtqueues. Device is not required
>> to wait for these packets to be consumed before delivering packets
>> using the new streering rule. Drivers modifying the steering rule at
>> a high rate (e.g. adaptively in response to changes in the workload)
>> are recommended to complete processing of the receive queue(s)
>> utilized by the original steering before processing any packets
>> delivered by the modified steering rule.
How can this be done? This isn't actually possible without taking the
queue down, since more packets are incoming.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH] Add a page cache-backed balloon device driver.
From: Rusty Russell @ 2012-09-12 5:25 UTC (permalink / raw)
To: Mike Waychison, Michael S. Tsirkin
Cc: Frank Swiderski, Andrea Arcangeli, Rik van Riel, kvm,
linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <CAGTjWtCiqjkR_Ze=0-ebO08+eziz6c8TXLb+O1iiArRQeSE6zw@mail.gmail.com>
Mike Waychison <mikew@google.com> writes:
> On Mon, Sep 10, 2012 at 3:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Mon, Sep 10, 2012 at 01:37:06PM -0400, Mike Waychison wrote:
>>> On Mon, Sep 10, 2012 at 5:05 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> > On Tue, Jun 26, 2012 at 01:32:58PM -0700, Frank Swiderski wrote:
>>> >> This implementation of a virtio balloon driver uses the page cache to
>>> >> "store" pages that have been released to the host. The communication
>>> >> (outside of target counts) is one way--the guest notifies the host when
>>> >> it adds a page to the page cache, allowing the host to madvise(2) with
>>> >> MADV_DONTNEED. Reclaim in the guest is therefore automatic and implicit
>>> >> (via the regular page reclaim). This means that inflating the balloon
>>> >> is similar to the existing balloon mechanism, but the deflate is
>>> >> different--it re-uses existing Linux kernel functionality to
>>> >> automatically reclaim.
>>> >>
>>> >> Signed-off-by: Frank Swiderski <fes@google.com>
>>>
>>> Hi Michael,
>>>
>>> I'm very sorry that Frank and I have been silent on these threads.
>>> I've been out of the office and Frank has been been swamped :)
>>>
>>> I'll take a stab at answering some of your questions below, and
>>> hopefully we can end up on the same page.
Hi Mike, Frank, Michael,
Thanks for the explanation and discussion. I like that this
implementation is more dynamic: the guest can use more pages for a while
(and the balloon kthread will furiously start trying to grab more pages
to give back). This part is a completely reasonable implementation, and
more sophisticated that what we have.
It doesn't *quite* meet the spec, because we don't notify the host when
we pull a page from the balloon, but I think that is quite possible. If
this is a performance waster, we should add a "SILENT_DEFLATE" feature
to tell the driver that it doesn't need to, though we should stll
support the !SILENT_DEFLATE case.
And Michael: thanks again for doing the heavy lifting on this!
Cheers,
Rusty.
^ 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