* Re: [PATCH v11 4/7] mm: introduce compaction and migration for ballooned pages
From: Mel Gorman @ 2012-11-09 12:16 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
Peter Zijlstra, linux-kernel, virtualization, linux-mm,
Andi Kleen, Minchan Kim, Andrew Morton, Paul E. McKenney
In-Reply-To: <08be4346b620ae9344691cc6c2ad0bc51f492e01.1352256088.git.aquini@redhat.com>
On Wed, Nov 07, 2012 at 01:05:51AM -0200, Rafael Aquini wrote:
> Memory fragmentation introduced by ballooning might reduce significantly
> the number of 2MB contiguous memory blocks that can be used within a guest,
> thus imposing performance penalties associated with the reduced number of
> transparent huge pages that could be used by the guest workload.
>
> This patch introduces the helper functions as well as the necessary changes
> to teach compaction and migration bits how to cope with pages which are
> part of a guest memory balloon, in order to make them movable by memory
> compaction procedures.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> mm/compaction.c | 21 +++++++++++++++++++--
> mm/migrate.c | 36 ++++++++++++++++++++++++++++++++++--
> 2 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 9eef558..76abd84 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -14,6 +14,7 @@
> #include <linux/backing-dev.h>
> #include <linux/sysctl.h>
> #include <linux/sysfs.h>
> +#include <linux/balloon_compaction.h>
> #include "internal.h"
>
> #if defined CONFIG_COMPACTION || defined CONFIG_CMA
> @@ -565,9 +566,24 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> goto next_pageblock;
> }
>
> - /* Check may be lockless but that's ok as we recheck later */
> - if (!PageLRU(page))
> + /*
> + * Check may be lockless but that's ok as we recheck later.
> + * It's possible to migrate LRU pages and balloon pages
> + * Skip any other type of page
> + */
> + if (!PageLRU(page)) {
> + if (unlikely(balloon_page_movable(page))) {
Because it's lockless, it really seems that the barrier stuck down there
is unnecessary. At worst you get a temporarily incorrect answer that you
recheck later under page lock in balloon_page_isolate.
> + if (locked && balloon_page_isolate(page)) {
> + /* Successfully isolated */
> + cc->finished_update_migrate = true;
> + list_add(&page->lru, migratelist);
> + cc->nr_migratepages++;
> + nr_isolated++;
> + goto check_compact_cluster;
> + }
> + }
> continue;
> + }
>
> /*
> * PageLRU is set. lru_lock normally excludes isolation
> @@ -621,6 +637,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> cc->nr_migratepages++;
> nr_isolated++;
>
> +check_compact_cluster:
> /* Avoid isolating too much */
> if (cc->nr_migratepages == COMPACT_CLUSTER_MAX) {
> ++low_pfn;
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 98c7a89..87ffe54 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -35,6 +35,7 @@
> #include <linux/hugetlb.h>
> #include <linux/hugetlb_cgroup.h>
> #include <linux/gfp.h>
> +#include <linux/balloon_compaction.h>
>
> #include <asm/tlbflush.h>
>
> @@ -79,7 +80,10 @@ void putback_lru_pages(struct list_head *l)
> list_del(&page->lru);
> dec_zone_page_state(page, NR_ISOLATED_ANON +
> page_is_file_cache(page));
> - putback_lru_page(page);
> + if (unlikely(balloon_page_movable(page)))
> + balloon_page_putback(page);
> + else
> + putback_lru_page(page);
> }
> }
>
> @@ -778,6 +782,18 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> }
> }
>
> + if (unlikely(balloon_page_movable(page))) {
> + /*
> + * A ballooned page does not need any special attention from
> + * physical to virtual reverse mapping procedures.
> + * Skip any attempt to unmap PTEs or to remap swap cache,
> + * in order to avoid burning cycles at rmap level, and perform
> + * the page migration right away (proteced by page lock).
> + */
> + rc = balloon_page_migrate(newpage, page, mode);
> + goto uncharge;
> + }
> +
> /*
> * Corner case handling:
> * 1. When a new swap-cache page is read into, it is added to the LRU
> @@ -814,7 +830,9 @@ skip_unmap:
> put_anon_vma(anon_vma);
>
> uncharge:
> - mem_cgroup_end_migration(mem, page, newpage, rc == MIGRATEPAGE_SUCCESS);
> + mem_cgroup_end_migration(mem, page, newpage,
> + (rc == MIGRATEPAGE_SUCCESS ||
> + rc == MIGRATEPAGE_BALLOON_SUCCESS));
> unlock:
> unlock_page(page);
> out:
> @@ -846,6 +864,20 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
> goto out;
>
> rc = __unmap_and_move(page, newpage, force, offlining, mode);
> +
> + if (unlikely(rc == MIGRATEPAGE_BALLOON_SUCCESS)) {
> + /*
> + * A ballooned page has been migrated already.
> + * Now, it's the time to remove the old page from the isolated
> + * pageset list and handle it back to Buddy, wrap-up counters
> + * and return.
> + */
> + dec_zone_page_state(page, NR_ISOLATED_ANON +
> + page_is_file_cache(page));
> + put_page(page);
> + __free_page(page);
> + return 0;
> + }
> out:
> if (rc != -EAGAIN) {
> /*
It may be necessary to make this more generic for migration-related
callbacks but I see nothing incompatible in your patch with doing that.
Doing the abstraction now would be overkill so
Acked-by: Mel Gorman <mel@csn.ul.ie>
--
Mel Gorman
SUSE Labs
^ permalink raw reply
* Re: [PATCH v11 3/7] mm: introduce a common interface for balloon pages mobility
From: Mel Gorman @ 2012-11-09 12:11 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
Peter Zijlstra, linux-kernel, virtualization, linux-mm,
Andi Kleen, Minchan Kim, Andrew Morton, Paul E. McKenney
In-Reply-To: <4ea10ef1eb1544e12524c8ca7df20cf621395463.1352256087.git.aquini@redhat.com>
On Wed, Nov 07, 2012 at 01:05:50AM -0200, Rafael Aquini wrote:
> Memory fragmentation introduced by ballooning might reduce significantly
> the number of 2MB contiguous memory blocks that can be used within a guest,
> thus imposing performance penalties associated with the reduced number of
> transparent huge pages that could be used by the guest workload.
>
> This patch introduces a common interface to help a balloon driver on
> making its page set movable to compaction, and thus allowing the system
> to better leverage the compation efforts on memory defragmentation.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> include/linux/balloon_compaction.h | 220 ++++++++++++++++++++++++++++++
> include/linux/migrate.h | 10 ++
> include/linux/pagemap.h | 16 +++
> mm/Kconfig | 15 +++
> mm/Makefile | 1 +
> mm/balloon_compaction.c | 269 +++++++++++++++++++++++++++++++++++++
> 6 files changed, 531 insertions(+)
> create mode 100644 include/linux/balloon_compaction.h
> create mode 100644 mm/balloon_compaction.c
>
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> new file mode 100644
> index 0000000..1865bd5
> --- /dev/null
> +++ b/include/linux/balloon_compaction.h
> @@ -0,0 +1,220 @@
> +/*
> + * include/linux/balloon_compaction.h
> + *
> + * Common interface definitions for making balloon pages movable to compaction.
> + *
s/to/by/
And the record for nit-picking goes to....
> + * Copyright (C) 2012, Red Hat, Inc. Rafael Aquini <aquini@redhat.com>
> + */
> +#ifndef _LINUX_BALLOON_COMPACTION_H
> +#define _LINUX_BALLOON_COMPACTION_H
> +#include <linux/pagemap.h>
> +#include <linux/migrate.h>
> +#include <linux/gfp.h>
> +#include <linux/err.h>
> +
> +/*
> + * Balloon device information descriptor.
> + * This struct is used to allow the common balloon compaction interface
> + * procedures to find the proper balloon device holding memory pages they'll
> + * have to cope for page compaction / migration, as well as it serves the
> + * balloon driver as a page book-keeper for its registered balloon devices.
> + */
> +struct balloon_dev_info {
> + void *balloon_device; /* balloon device descriptor */
> + struct address_space *mapping; /* balloon special page->mapping */
> + unsigned long isolated_pages; /* # of isolated pages for migration */
> + spinlock_t pages_lock; /* Protection to pages list */
> + struct list_head pages; /* Pages enqueued & handled to Host */
> +};
> +
> +extern struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info);
> +extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info);
> +extern struct balloon_dev_info *balloon_devinfo_alloc(
> + void *balloon_dev_descriptor);
> +
> +static inline void balloon_devinfo_free(struct balloon_dev_info *b_dev_info)
> +{
> + kfree(b_dev_info);
> +}
> +
More stupid nit-picking but in terms of defensive programming it would
be preferred if the container struct of balloon_dev_info was passed in
and then
kfree(something->b_dev_info);
something->b_dev_info = NULL;
This looks like serious overkill but if the lifetime of the balloon driver
is complex and relatively rarely used (which I bet is the case) then we
want use-after-free bugs to blow up quickly instead of corrupt.
It is not mandatory to address this before merging but if you do another
revision it might be a nice idea. I'm only bringing this up as I got bitten
by this recently when I screwed up the lifetime of a mm-associated struct
and it took a while to pin down.
The suggestion is irrelevant if the containing structure is freed at the
same time of course.
> +#ifdef CONFIG_BALLOON_COMPACTION
> +extern bool balloon_page_isolate(struct page *page);
> +extern void balloon_page_putback(struct page *page);
> +extern int balloon_page_migrate(struct page *newpage,
> + struct page *page, enum migrate_mode mode);
> +extern struct address_space
> +*balloon_mapping_alloc(struct balloon_dev_info *b_dev_info,
> + const struct address_space_operations *a_ops);
> +
It should not be part of your patch but one day we might want to abstract
even this and put users with different migration requirements behind some
sort of migration_operations callback strucr. Balloon would be the first
user of course but CMA also is and but if placement constraints for
memory usage ever happens, it'd be another user.
> +static inline void balloon_mapping_free(struct address_space *balloon_mapping)
> +{
> + kfree(balloon_mapping);
> +}
> +
> +/*
> + * balloon_page_insert - insert a page into the balloon's page list and make
> + * the page->mapping assignment accordingly.
> + * @page : page to be assigned as a 'balloon page'
> + * @mapping : allocated special 'balloon_mapping'
> + * @head : balloon's device page list head
> + */
> +static inline void balloon_page_insert(struct page *page,
> + struct address_space *mapping,
> + struct list_head *head)
> +{
> + list_add(&page->lru, head);
> + /*
> + * Make sure the page is already inserted on balloon's page list
> + * before assigning its ->mapping.
> + */
> + smp_wmb();
> + page->mapping = mapping;
> +}
> +
Elsewhere we have;
spin_lock_irqsave(&b_dev_info->pages_lock, flags);
balloon_page_insert(page, b_dev_info->mapping, &b_dev_info->pages);
spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
So this happens under an irq-safe lock. Why is a smp_wmb necessary?
> +
> +/*
> + * balloon_page_delete - clear the page->mapping and delete the page from
> + * balloon's page list accordingly.
> + * @page : page to be released from balloon's page list
> + */
> +static inline void balloon_page_delete(struct page *page)
> +{
> + page->mapping = NULL;
> + /*
> + * Make sure page->mapping is cleared before we proceed with
> + * balloon's page list deletion.
> + */
> + smp_wmb();
> + list_del(&page->lru);
> +}
> +
Same thing on locking except this also appears to be under the page lock
making the barrier seem even more unnecessary.
> +/*
> + * __is_movable_balloon_page - helper to perform @page mapping->flags tests
> + */
> +static inline bool __is_movable_balloon_page(struct page *page)
> +{
> + /*
> + * we might attempt to read ->mapping concurrently to other
> + * threads trying to write to it.
> + */
> + struct address_space *mapping = ACCESS_ONCE(page->mapping);
> + smp_read_barrier_depends();
> + return mapping_balloon(mapping);
> +}
> +
What happens if this race occurs? I assume it's a racy check before you
isolate the balloon in which case the barrier may be overkill.
> +/*
> + * balloon_page_movable - test page->mapping->flags to identify balloon pages
> + * that can be moved by compaction/migration.
> + *
> + * This function is used at core compaction's page isolation scheme, therefore
> + * most pages exposed to it are not enlisted as balloon pages and so, to avoid
> + * undesired side effects like racing against __free_pages(), we cannot afford
> + * holding the page locked while testing page->mapping->flags here.
> + *
> + * As we might return false positives in the case of a balloon page being just
> + * released under us, the page->mapping->flags need to be re-tested later,
> + * under the proper page lock, at the functions that will be coping with the
> + * balloon page case.
> + */
> +static inline bool balloon_page_movable(struct page *page)
> +{
> + /*
> + * Before dereferencing and testing mapping->flags, lets make sure
> + * this is not a page that uses ->mapping in a different way
> + */
> + if (!PageSlab(page) && !PageSwapCache(page) && !PageAnon(page) &&
> + !page_mapped(page))
> + return __is_movable_balloon_page(page);
> +
> + return false;
> +}
> +
Ok.
> +/*
> + * balloon_page_device - get the b_dev_info descriptor for the balloon device
> + * that enqueues the given page.
> + */
> +static inline struct balloon_dev_info *balloon_page_device(struct page *page)
> +{
> + struct address_space *mapping = page->mapping;
> + if (likely(mapping))
> + return mapping->private_data;
> +
> + return NULL;
> +}
> +
> +static inline gfp_t balloon_mapping_gfp_mask(void)
> +{
> + return GFP_HIGHUSER_MOVABLE;
> +}
> +
> +static inline void balloon_event_count(enum vm_event_item item)
> +{
> + count_vm_event(item);
> +}
> +
> +static inline bool balloon_compaction_check(void)
> +{
> + return true;
> +}
> +
> +#else /* !CONFIG_BALLOON_COMPACTION */
> +
> +static inline void *balloon_mapping_alloc(void *balloon_device,
> + const struct address_space_operations *a_ops)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline void balloon_mapping_free(struct address_space *balloon_mapping)
> +{
> + return;
> +}
> +
> +static inline void balloon_page_insert(struct page *page,
> + struct address_space *mapping,
> + struct list_head *head)
> +{
> + list_add(&page->lru, head);
> +}
> +
> +static inline void balloon_page_delete(struct page *page)
> +{
> + list_del(&page->lru);
> +}
> +
> +static inline bool balloon_page_movable(struct page *page)
> +{
> + return false;
> +}
> +
> +static inline bool balloon_page_isolate(struct page *page)
> +{
> + return false;
> +}
> +
> +static inline void balloon_page_putback(struct page *page)
> +{
> + return;
> +}
> +
> +static inline int balloon_page_migrate(struct page *newpage,
> + struct page *page, enum migrate_mode mode)
> +{
> + return 0;
> +}
> +
> +static inline gfp_t balloon_mapping_gfp_mask(void)
> +{
> + return GFP_HIGHUSER;
> +}
> +
> +static inline void balloon_event_count(enum vm_event_item item)
> +{
> + return;
> +}
> +
> +static inline bool balloon_compaction_check(void)
> +{
> + return false;
> +}
> +#endif /* CONFIG_BALLOON_COMPACTION */
> +#endif /* _LINUX_BALLOON_COMPACTION_H */
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index a4e886d..e570c3c 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -11,8 +11,18 @@ typedef struct page *new_page_t(struct page *, unsigned long private, int **);
> * Return values from addresss_space_operations.migratepage():
> * - negative errno on page migration failure;
> * - zero on page migration success;
> + *
> + * The balloon page migration introduces this special case where a 'distinct'
> + * return code is used to flag a successful page migration to unmap_and_move().
> + * This approach is necessary because page migration can race against balloon
> + * deflation procedure, and for such case we could introduce a nasty page leak
> + * if a successfully migrated balloon page gets released concurrently with
> + * migration's unmap_and_move() wrap-up steps.
> */
> #define MIGRATEPAGE_SUCCESS 0
> +#define MIGRATEPAGE_BALLOON_SUCCESS 1 /* special ret code for balloon page
> + * sucessfull migration case.
> + */
s/sucessfull/successful/
>
> #ifdef CONFIG_MIGRATION
>
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index e42c762..6da609d 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -24,6 +24,7 @@ enum mapping_flags {
> AS_ENOSPC = __GFP_BITS_SHIFT + 1, /* ENOSPC on async write */
> AS_MM_ALL_LOCKS = __GFP_BITS_SHIFT + 2, /* under mm_take_all_locks() */
> AS_UNEVICTABLE = __GFP_BITS_SHIFT + 3, /* e.g., ramdisk, SHM_LOCK */
> + AS_BALLOON_MAP = __GFP_BITS_SHIFT + 4, /* balloon page special map */
> };
>
> static inline void mapping_set_error(struct address_space *mapping, int error)
> @@ -53,6 +54,21 @@ static inline int mapping_unevictable(struct address_space *mapping)
> return !!mapping;
> }
>
> +static inline void mapping_set_balloon(struct address_space *mapping)
> +{
> + set_bit(AS_BALLOON_MAP, &mapping->flags);
> +}
> +
> +static inline void mapping_clear_balloon(struct address_space *mapping)
> +{
> + clear_bit(AS_BALLOON_MAP, &mapping->flags);
> +}
> +
> +static inline int mapping_balloon(struct address_space *mapping)
> +{
> + return mapping && test_bit(AS_BALLOON_MAP, &mapping->flags);
> +}
> +
> static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
> {
> return (__force gfp_t)mapping->flags & __GFP_BITS_MASK;
> diff --git a/mm/Kconfig b/mm/Kconfig
> index a3f8ddd..b119172 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -188,6 +188,21 @@ config SPLIT_PTLOCK_CPUS
> default "4"
>
> #
> +# support for memory balloon compaction
> +config BALLOON_COMPACTION
> + bool "Allow for balloon memory compaction/migration"
> + select COMPACTION
> + depends on VIRTIO_BALLOON
> + help
> + Memory fragmentation introduced by ballooning might reduce
> + significantly the number of 2MB contiguous memory blocks that can be
> + used within a guest, thus imposing performance penalties associated
> + with the reduced number of transparent huge pages that could be used
> + by the guest workload. Allowing the compaction & migration for memory
> + pages enlisted as being part of memory balloon devices avoids the
> + scenario aforementioned and helps improving memory defragmentation.
> +
Rather than select COMPACTION, should it depend on it? Similarly as THP
is the primary motivation, would it make more sense to depend on
TRANSPARENT_HUGEPAGE?
Should it default y? It seems useful, why would someone support
VIRTIO_BALLOON and *not* use this?
> +#
> # support for memory compaction
> config COMPACTION
> bool "Allow for memory compaction"
> diff --git a/mm/Makefile b/mm/Makefile
> index 6b025f8..21e10ee 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_DEBUG_KMEMLEAK) += kmemleak.o
> obj-$(CONFIG_DEBUG_KMEMLEAK_TEST) += kmemleak-test.o
> obj-$(CONFIG_CLEANCACHE) += cleancache.o
> obj-$(CONFIG_MEMORY_ISOLATION) += page_isolation.o
> +obj-$(CONFIG_BALLOON_COMPACTION) += balloon_compaction.o
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> new file mode 100644
> index 0000000..90935aa
> --- /dev/null
> +++ b/mm/balloon_compaction.c
> @@ -0,0 +1,269 @@
> +/*
> + * mm/balloon_compaction.c
> + *
> + * Common interface for making balloon pages movable to compaction.
> + *
> + * Copyright (C) 2012, Red Hat, Inc. Rafael Aquini <aquini@redhat.com>
> + */
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/balloon_compaction.h>
> +
> +/*
> + * balloon_devinfo_alloc - allocates a balloon device information descriptor.
> + * @balloon_dev_descriptor: pointer to reference the balloon device which
> + * this struct balloon_dev_info will be servicing.
> + *
> + * Driver must call it to properly allocate and initialize an instance of
> + * struct balloon_dev_info which will be used to reference a balloon device
> + * as well as to keep track of the balloon device page list.
> + */
> +struct balloon_dev_info *balloon_devinfo_alloc(void *balloon_dev_descriptor)
> +{
> + struct balloon_dev_info *b_dev_info;
> + b_dev_info = kmalloc(sizeof(*b_dev_info), GFP_KERNEL);
> + if (!b_dev_info)
> + return ERR_PTR(-ENOMEM);
> +
> + b_dev_info->balloon_device = balloon_dev_descriptor;
> + b_dev_info->mapping = NULL;
> + b_dev_info->isolated_pages = 0;
> + spin_lock_init(&b_dev_info->pages_lock);
> + INIT_LIST_HEAD(&b_dev_info->pages);
> +
> + return b_dev_info;
> +}
> +EXPORT_SYMBOL_GPL(balloon_devinfo_alloc);
> +
> +/*
> + * balloon_page_enqueue - allocates a new page and inserts it into the balloon
> + * page list.
> + * @b_dev_info: balloon device decriptor where we will insert a new page to
> + *
> + * Driver must call it to properly allocate a new enlisted balloon page
> + * before definetively removing it from the guest system.
> + * This function returns the page address for the recently enqueued page or
> + * NULL in the case we fail to allocate a new page this turn.
> + */
> +struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info)
> +{
> + unsigned long flags;
> + struct page *page = alloc_page(balloon_mapping_gfp_mask() |
> + __GFP_NOMEMALLOC | __GFP_NORETRY);
> + if (!page)
> + return NULL;
> +
> + BUG_ON(!trylock_page(page));
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> + balloon_page_insert(page, b_dev_info->mapping, &b_dev_info->pages);
> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> + unlock_page(page);
> + return page;
> +}
> +EXPORT_SYMBOL_GPL(balloon_page_enqueue);
> +
> +/*
> + * balloon_page_dequeue - removes a page from balloon's page list and returns
> + * the its address to allow the driver release the page.
> + * @b_dev_info: balloon device decriptor where we will grab a page from.
> + *
> + * Driver must call it to properly de-allocate a previous enlisted balloon page
> + * before definetively releasing it back to the guest system.
> + * This function returns the page address for the recently dequeued page or
> + * NULL in the case we find balloon's page list temporarely empty due to
> + * compaction isolated pages.
> + */
> +struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info)
> +{
> + struct page *page, *tmp;
> + unsigned long flags;
> + bool dequeued_page;
> +
> + dequeued_page = false;
> + list_for_each_entry_safe(page, tmp, &b_dev_info->pages, lru) {
> + if (trylock_page(page)) {
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> + balloon_page_delete(page);
> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> + unlock_page(page);
> + dequeued_page = true;
> + break;
> + }
> + }
> +
> + if (!dequeued_page) {
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> + if (unlikely(list_empty(&b_dev_info->pages) &&
> + !b_dev_info->isolated_pages))
> + BUG();
> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> + page = NULL;
> + }
> + return page;
> +}
> +EXPORT_SYMBOL_GPL(balloon_page_dequeue);
> +
> +#ifdef CONFIG_BALLOON_COMPACTION
> +/*
> + * balloon_mapping_alloc - allocates a special ->mapping for ballooned pages.
> + * @b_dev_info: holds the balloon device information descriptor.
> + * @a_ops: balloon_mapping address_space_operations descriptor.
> + *
> + * Driver must call it to properly allocate and initialize an instance of
> + * struct address_space which will be used as the special page->mapping for
> + * balloon device enlisted page instances.
> + */
> +struct address_space *balloon_mapping_alloc(struct balloon_dev_info *b_dev_info,
> + const struct address_space_operations *a_ops)
> +{
> + struct address_space *mapping;
> +
> + mapping = kmalloc(sizeof(*mapping), GFP_KERNEL);
> + if (!mapping)
> + return ERR_PTR(-ENOMEM);
> +
> + /*
> + * Give a clean 'zeroed' status to all elements of this special
> + * balloon page->mapping struct address_space instance.
> + */
> + address_space_init_once(mapping);
> +
> + /*
> + * Set mapping->flags appropriately, to allow balloon pages
> + * ->mapping identification.
> + */
> + mapping_set_balloon(mapping);
> + mapping_set_gfp_mask(mapping, balloon_mapping_gfp_mask());
> +
> + /* balloon's page->mapping->a_ops callback descriptor */
> + mapping->a_ops = a_ops;
> +
> + /*
> + * Establish a pointer reference back to the balloon device descriptor
> + * this particular page->mapping will be servicing.
> + * This is used by compaction / migration procedures to identify and
> + * access the balloon device pageset while isolating / migrating pages.
> + *
> + * As some balloon drivers can register multiple balloon devices
> + * for a single guest, this also helps compaction / migration to
> + * properly deal with multiple balloon pagesets, when required.
> + */
> + mapping->private_data = b_dev_info;
> + b_dev_info->mapping = mapping;
> +
> + return mapping;
> +}
> +EXPORT_SYMBOL_GPL(balloon_mapping_alloc);
> +
> +static inline void __isolate_balloon_page(struct page *page)
> +{
> + struct balloon_dev_info *b_dev_info = page->mapping->private_data;
> + unsigned long flags;
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> + list_del(&page->lru);
> + b_dev_info->isolated_pages++;
> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> +}
> +
> +static inline void __putback_balloon_page(struct page *page)
> +{
> + struct balloon_dev_info *b_dev_info = page->mapping->private_data;
> + unsigned long flags;
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> + list_add(&page->lru, &b_dev_info->pages);
> + b_dev_info->isolated_pages--;
> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> +}
> +
> +static inline int __migrate_balloon_page(struct address_space *mapping,
> + struct page *newpage, struct page *page, enum migrate_mode mode)
> +{
> + return page->mapping->a_ops->migratepage(mapping, newpage, page, mode);
> +}
> +
> +/* __isolate_lru_page() counterpart for a ballooned page */
> +bool balloon_page_isolate(struct page *page)
> +{
> + /*
> + * Avoid burning cycles with pages that are yet under __free_pages(),
> + * or just got freed under us.
> + *
> + * In case we 'win' a race for a balloon page being freed under us and
> + * raise its refcount preventing __free_pages() from doing its job
> + * the put_page() at the end of this block will take care of
> + * release this page, thus avoiding a nasty leakage.
> + */
> + if (likely(get_page_unless_zero(page))) {
> + /*
> + * As balloon pages are not isolated from LRU lists, concurrent
> + * compaction threads can race against page migration functions
> + * as well as race against the balloon driver releasing a page.
> + *
> + * In order to avoid having an already isolated balloon page
> + * being (wrongly) re-isolated while it is under migration,
> + * or to avoid attempting to isolate pages being released by
> + * the balloon driver, lets be sure we have the page lock
> + * before proceeding with the balloon page isolation steps.
> + */
> + if (likely(trylock_page(page))) {
> + /*
> + * A ballooned page, by default, has just one refcount.
> + * Prevent concurrent compaction threads from isolating
> + * an already isolated balloon page by refcount check.
> + */
> + if (__is_movable_balloon_page(page) &&
> + page_count(page) == 2) {
> + __isolate_balloon_page(page);
> + unlock_page(page);
> + return true;
> + }
> + unlock_page(page);
> + }
> + put_page(page);
> + }
> + return false;
> +}
> +
> +/* putback_lru_page() counterpart for a ballooned page */
> +void balloon_page_putback(struct page *page)
> +{
> + /*
> + * 'lock_page()' stabilizes the page and prevents races against
> + * concurrent isolation threads attempting to re-isolate it.
> + */
> + lock_page(page);
> +
> + if (__is_movable_balloon_page(page)) {
> + __putback_balloon_page(page);
> + put_page(page);
> + } else {
> + __WARN();
> + dump_page(page);
> + }
> + unlock_page(page);
> +}
> +
> +/* move_to_new_page() counterpart for a ballooned page */
> +int balloon_page_migrate(struct page *newpage,
> + struct page *page, enum migrate_mode mode)
> +{
> + struct address_space *mapping;
> + int rc = -EAGAIN;
> +
> + BUG_ON(!trylock_page(newpage));
> +
> + if (WARN_ON(!__is_movable_balloon_page(page))) {
> + dump_page(page);
> + unlock_page(newpage);
> + return rc;
> + }
> +
> + mapping = page->mapping;
> + if (mapping)
> + rc = __migrate_balloon_page(mapping, newpage, page, mode);
> +
> + unlock_page(newpage);
> + return rc;
> +}
> +#endif /* CONFIG_BALLOON_COMPACTION */
Ok, I did not spot any obvious problems in this. The barriers were the
big issue for me really - they seem overkill. I think we've discussed
this already but even though it was recent I cannot remember the
conclusion. In a sense, it doesn't matter because it should have been
described in the code anyway.
If you get the barrier issue sorted out then feel free to add
Acked-by: Mel Gorman <mel@csn.ul.ie>
--
Mel Gorman
SUSE Labs
^ permalink raw reply
* Re: [PATCH -next] tcm_vhost: remove unused variable in vhost_scsi_allocate_cmd()
From: Nicholas A. Bellinger @ 2012-11-09 5:24 UTC (permalink / raw)
To: Wei Yongjun; +Cc: kvm, mst, netdev, virtualization, yongjun_wei, target-devel
In-Reply-To: <CAPgLHd81=dTwwSTgUtd9YbgWf8YLMfi3VJQ-76h71+0W_mx_2g@mail.gmail.com>
Hello Wei,
On Wed, 2012-11-07 at 20:53 +0800, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> The variable se_sess is initialized but never used
> otherwise, so remove the unused variable.
>
> dpatch engine is used to auto generate this patch.
> (https://github.com/weiyj/dpatch)
>
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> ---
> drivers/vhost/tcm_vhost.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index 23c138f..551fff0 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -415,14 +415,12 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
> {
> struct tcm_vhost_cmd *tv_cmd;
> struct tcm_vhost_nexus *tv_nexus;
> - struct se_session *se_sess;
>
> tv_nexus = tv_tpg->tpg_nexus;
> if (!tv_nexus) {
> pr_err("Unable to locate active struct tcm_vhost_nexus\n");
> return ERR_PTR(-EIO);
> }
> - se_sess = tv_nexus->tvn_se_sess;
>
> tv_cmd = kzalloc(sizeof(struct tcm_vhost_cmd), GFP_ATOMIC);
> if (!tv_cmd) {
>
> --
Looks fine to me.
Reviewed-by + Acked-by: Nicholas Bellinger <nab@linux-iscsi.org>
Thanks!
^ permalink raw reply
* Re: [PATCH] virtio: Don't access index after unregister.
From: Michael S. Tsirkin @ 2012-11-09 5:14 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-kernel, virtualization
In-Reply-To: <1352371427-32363-1-git-send-email-cornelia.huck@de.ibm.com>
On Thu, Nov 08, 2012 at 11:43:47AM +0100, Cornelia Huck wrote:
> Virtio wants to release used indices after the corresponding
> virtio device has been unregistered. However, virtio does not
> hold an extra reference, giving up its last reference with
> device_unregister(), making accessing dev->index afterwards
> invalid.
>
> I actually saw problems when testing my (not-yet-merged)
> virtio-ccw code:
>
> - device_add virtio-net,id=xxx
> -> creates device virtio<n> with n>0
>
> - device_del xxx
> -> deletes virtio<n>, but calls ida_simple_remove with an
> index of 0
>
> - device_add virtio-net,id=xxx
> -> tries to add virtio0, which is still in use...
>
> So let's save the index we want to release before calling
> device_unregister().
>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> drivers/virtio/virtio.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 1e8659c..809b0de 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -225,8 +225,10 @@ EXPORT_SYMBOL_GPL(register_virtio_device);
>
> void unregister_virtio_device(struct virtio_device *dev)
> {
> + int index = dev->index; /* save for after device release */
It's obvious from code that we safe for after release,
I think a better comment would explain *why* we do this.
Something like
/*
device_unregister drops reference to device so put_device could
invoke release callback. In case that callback will free the device,
make sure we don't access device after this call.
*/
int index = dev->index;
?
> +
> device_unregister(&dev->dev);
> - ida_simple_remove(&virtio_index_ida, dev->index);
> + ida_simple_remove(&virtio_index_ida, index);
> }
> EXPORT_SYMBOL_GPL(unregister_virtio_device);
>
> --
> 1.7.12.4
^ permalink raw reply
* Re: [PATCH] virtio: Don't access index after unregister.
From: Rusty Russell @ 2012-11-09 4:24 UTC (permalink / raw)
To: Cornelia Huck, Michael S. Tsirkin; +Cc: linux-kernel, virtualization
In-Reply-To: <1352371427-32363-1-git-send-email-cornelia.huck@de.ibm.com>
Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> Virtio wants to release used indices after the corresponding
> virtio device has been unregistered. However, virtio does not
> hold an extra reference, giving up its last reference with
> device_unregister(), making accessing dev->index afterwards
> invalid.
>
> I actually saw problems when testing my (not-yet-merged)
> virtio-ccw code:
>
> - device_add virtio-net,id=xxx
> -> creates device virtio<n> with n>0
>
> - device_del xxx
> -> deletes virtio<n>, but calls ida_simple_remove with an
> index of 0
>
> - device_add virtio-net,id=xxx
> -> tries to add virtio0, which is still in use...
>
> So let's save the index we want to release before calling
> device_unregister().
Great catch! I've add a CC:stable.
Applied,
Rusty.
^ permalink raw reply
* Re: [PATCH v3] virtio-mmio: Fix irq parsing in command line parameter
From: Rusty Russell @ 2012-11-09 4:11 UTC (permalink / raw)
Cc: Pawel Moll, virtualization
In-Reply-To: <1352401544-8218-1-git-send-email-pawel.moll@arm.com>
Pawel Moll <pawel.moll@arm.com> writes:
> When the resource_size_t is 64-bit long, the sscanf() on
> the virtio device command line paramter string may return
> wrong value because its format was defined as "%u". Fixed
> by using an intermediate local value of a known length.
>
> Also added cleaned up the resource creation and added extra
> comments to make the parameters parsing easier to follow.
Applied.
Thanks,
Rusty.
^ permalink raw reply
* Re: [rfc net-next v6 3/3] virtio-net: change the number of queues through ethtool
From: Ben Hutchings @ 2012-11-08 21:13 UTC (permalink / raw)
To: Jason Wang
Cc: krkumar2, kvm, mst, netdev, linux-kernel, virtualization, davem
In-Reply-To: <1351591403-23065-4-git-send-email-jasowang@redhat.com>
On Tue, 2012-10-30 at 18:03 +0800, Jason Wang wrote:
> This patch implement the {set|get}_channels method of ethool to allow user to
> change the number of queues dymaically when the device is running. This would
> let the user to tune the device for specific applications.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/net/virtio_net.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 43 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 8cc43e5..66fc129 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1616,10 +1616,53 @@ static struct virtio_driver virtio_net_driver = {
> #endif
> };
>
> +/* TODO: Eliminate OOO packets during switching */
> +static int virtnet_set_channels(struct net_device *dev,
> + struct ethtool_channels *channels)
> +{
> + struct virtnet_info *vi = netdev_priv(dev);
> + u16 queue_pairs = channels->combined_count;
> +
> + /* Only two modes were support currently */
> + if (queue_pairs == 0)
> + return -EINVAL;
> + if (queue_pairs != vi->total_queue_pairs - 1 && queue_pairs != 1)
> + return -EINVAL;
You should also check that all of the other counts are == 0.
> + vi->num_queue_pairs = queue_pairs > 1 ? queue_pairs + 1 : 1;
This doesn't make sense. You're setting vi->num_queue_pairs to be
combined_count + 1...
> + BUG_ON(virtnet_set_queues(vi));
> +
> + netif_set_real_num_tx_queues(dev, vi->num_queue_pairs);
> + netif_set_real_num_rx_queues(dev, vi->num_queue_pairs);
> +
> + return 0;
> +}
> +
> +static void virtnet_get_channels(struct net_device *dev,
> + struct ethtool_channels *channels)
> +{
> + struct virtnet_info *vi = netdev_priv(dev);
> +
> + if (vi->total_queue_pairs != 1) {
> + channels->combined_count = vi->num_queue_pairs;
but here you report combined_count as being vi->num_queue_pairs. Do you
need to subtract 1 here?
Ben.
> + channels->max_combined = vi->total_queue_pairs - 1;
> + } else {
> + channels->combined_count = 1;
> + channels->max_combined = 1;
> + }
> +
> + channels->max_other = 0;
> + channels->rx_count = 0;
> + channels->tx_count = 0;
> + channels->other_count = 0;
> +}
> +
> static const struct ethtool_ops virtnet_ethtool_ops = {
> .get_drvinfo = virtnet_get_drvinfo,
> .get_link = ethtool_op_get_link,
> .get_ringparam = virtnet_get_ringparam,
> + .set_channels = virtnet_set_channels,
> + .get_channels = virtnet_get_channels,
> };
>
> static int __init init(void)
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH v3] virtio-mmio: Fix irq parsing in command line parameter
From: Pawel Moll @ 2012-11-08 19:05 UTC (permalink / raw)
To: Rusty Russell; +Cc: Pawel Moll, virtualization
In-Reply-To: <878vad3u9r.fsf@rustcorp.com.au>
When the resource_size_t is 64-bit long, the sscanf() on
the virtio device command line paramter string may return
wrong value because its format was defined as "%u". Fixed
by using an intermediate local value of a known length.
Also added cleaned up the resource creation and added extra
comments to make the parameters parsing easier to follow.
Reported-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
drivers/virtio/virtio_mmio.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..9df4578 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -521,25 +521,33 @@ static int vm_cmdline_set(const char *device,
int err;
struct resource resources[2] = {};
char *str;
- long long int base;
+ long long int base, size;
+ unsigned int irq;
int processed, consumed = 0;
struct platform_device *pdev;
- resources[0].flags = IORESOURCE_MEM;
- resources[1].flags = IORESOURCE_IRQ;
-
- resources[0].end = memparse(device, &str) - 1;
+ /* Consume "size" part of the command line parameter */
+ size = memparse(device, &str);
+ /* Get "@<base>:<irq>[:<id>]" chunks */
processed = sscanf(str, "@%lli:%u%n:%d%n",
- &base, &resources[1].start, &consumed,
+ &base, &irq, &consumed,
&vm_cmdline_id, &consumed);
- if (processed < 2 || processed > 3 || str[consumed])
+ /*
+ * sscanf() must processes at least 2 chunks; also there
+ * must be no extra characters after the last chunk, so
+ * str[consumed] must be '\0'
+ */
+ if (processed < 2 || str[consumed])
return -EINVAL;
+ resources[0].flags = IORESOURCE_MEM;
resources[0].start = base;
- resources[0].end += base;
- resources[1].end = resources[1].start;
+ resources[0].end = base + size - 1;
+
+ resources[1].flags = IORESOURCE_IRQ;
+ resources[1].start = resources[1].end = irq;
if (!vm_cmdline_parent_registered) {
err = device_register(&vm_cmdline_parent);
--
1.7.10.4
^ permalink raw reply related
* Re: [Pv-drivers] [PATCH 0/6] VSOCK for Linux upstreaming
From: Andy King @ 2012-11-08 15:47 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: pv-drivers, netdev, linux-kernel, virtualization, gregkh,
David Miller, georgezhang
In-Reply-To: <509A06AB.2020700@redhat.com>
Hi Gerd,
>> Also, there was some interest from RedHat into using vSockets as
>> a unified interface, routed over a hypervisor-specific transport
>> (virtio or otherwise, although for now VMCI is the only one
>> implemented).
>
> Can you outline how this can be done? From a quick look over the
> code it seems like vsock has a hard dependency on vmci, is that
> correct?
That's correct, VMCI is wired into vSockets and we don't currently
provide any way to insert another transport.
> When making vsock a generic, reusable kernel service it should be
> the other way around: vsock should provide the core implementation
> and an interface where hypervisor-specific transports (vmci,
> virtio, xenbus, ...) can register themself.
Sorry, that was a bad explanation on my part. You're absolutely
correct as to how it _should_ work. But it's up to Red Hat or others
to get the ball rolling and motivate the necessary work on vSockets
to make this happen. As Greg says, "everyone is lazy and just wants
their code accepted" ;)
Thanks!
- Andy
^ permalink raw reply
* Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter
From: Rusty Russell @ 2012-11-08 12:23 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-kernel, Pawel Moll, virtualization
In-Reply-To: <CAF2Aj3gLLTw5ng0iZ6ecEkjGnJ-XGi3_RBrjCroW-=v=pfcFAw@mail.gmail.com>
Lee Jones <lee.jones@linaro.org> writes:
>> > Resorted to poaching now have we Pawel?
>
>> I hope you were joking!
>
> Yes, of course. I thought that was clearly indicated by the jovial winking
> smiley. :)
>
> I realise it wasn't obvious soley by this exchange, but Pawel and I are
> actually ol' friends.
>
>> Doing your work for you isn't poaching.
>
> This isn't related to my work, as I have no professional interest in
> virtio. In this particular instance I was trying to help out by fixing bugs
> uncovered by the use of randconfig, which I'm doing in my own time, as a
> hobby. I guess the use my work address clouds this fact, so I apologise for
> that.
>
> Sorry for any confusion or offence caused though Rusty. :)
>
> Kind regards,
> Lee
No offence for me. I just don't want bystanders to believe that there
is some etiquette other than "best patch wins".
Thanks for the clarification!
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio: Don't access index after unregister.
From: Sjur Brændeland @ 2012-11-08 11:50 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <1352371427-32363-1-git-send-email-cornelia.huck@de.ibm.com>
On Thu, Nov 8, 2012 at 11:43 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> Virtio wants to release used indices after the corresponding
> virtio device has been unregistered. However, virtio does not
> hold an extra reference, giving up its last reference with
> device_unregister(), making accessing dev->index afterwards
> invalid.
>
> I actually saw problems when testing my (not-yet-merged)
> virtio-ccw code:
>
> - device_add virtio-net,id=xxx
> -> creates device virtio<n> with n>0
>
> - device_del xxx
> -> deletes virtio<n>, but calls ida_simple_remove with an
> index of 0
>
> - device_add virtio-net,id=xxx
> -> tries to add virtio0, which is still in use...
>
> So let's save the index we want to release before calling
> device_unregister().
>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> drivers/virtio/virtio.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 1e8659c..809b0de 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -225,8 +225,10 @@ EXPORT_SYMBOL_GPL(register_virtio_device);
>
> void unregister_virtio_device(struct virtio_device *dev)
> {
> + int index = dev->index; /* save for after device release */
> +
> device_unregister(&dev->dev);
> - ida_simple_remove(&virtio_index_ida, dev->index);
> + ida_simple_remove(&virtio_index_ida, index);
> }
> EXPORT_SYMBOL_GPL(unregister_virtio_device);
Acked-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
Great minds think alike! I discovered issues with this implementation
a while back and Michael suggested an identical patch:
https://lkml.org/lkml/2012/9/4/173
https://lkml.org/lkml/2012/9/7/105
The issue I ran into was that when virtio devices are created by remoteproc
the device memory might be freed when calling device_unregister(), and
the value of dev->index is then undefined. So this bug bites when
unregistering a Virtio devices from remoteproc with CONFIG_DEBUG_SLAB
enabled. However this bug is not triggered by virtio_pci as it implements a
non-standard device release-function that does not free the device memory.
Thanks,
Sjur
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] virtio: Don't access index after unregister.
From: Cornelia Huck @ 2012-11-08 10:43 UTC (permalink / raw)
To: Rusty Russell, Michael S. Tsirkin; +Cc: linux-kernel, virtualization
Virtio wants to release used indices after the corresponding
virtio device has been unregistered. However, virtio does not
hold an extra reference, giving up its last reference with
device_unregister(), making accessing dev->index afterwards
invalid.
I actually saw problems when testing my (not-yet-merged)
virtio-ccw code:
- device_add virtio-net,id=xxx
-> creates device virtio<n> with n>0
- device_del xxx
-> deletes virtio<n>, but calls ida_simple_remove with an
index of 0
- device_add virtio-net,id=xxx
-> tries to add virtio0, which is still in use...
So let's save the index we want to release before calling
device_unregister().
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
drivers/virtio/virtio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 1e8659c..809b0de 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -225,8 +225,10 @@ EXPORT_SYMBOL_GPL(register_virtio_device);
void unregister_virtio_device(struct virtio_device *dev)
{
+ int index = dev->index; /* save for after device release */
+
device_unregister(&dev->dev);
- ida_simple_remove(&virtio_index_ida, dev->index);
+ ida_simple_remove(&virtio_index_ida, index);
}
EXPORT_SYMBOL_GPL(unregister_virtio_device);
--
1.7.12.4
^ permalink raw reply related
* Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter
From: Lee Jones @ 2012-11-08 9:48 UTC (permalink / raw)
To: Rusty Russell; +Cc: linux-kernel, Pawel Moll, virtualization
In-Reply-To: <87bof93vf2.fsf@rustcorp.com.au>
[-- Attachment #1.1: Type: text/plain, Size: 676 bytes --]
> > Resorted to poaching now have we Pawel?
> I hope you were joking!
Yes, of course. I thought that was clearly indicated by the jovial winking
smiley. :)
I realise it wasn't obvious soley by this exchange, but Pawel and I are
actually ol' friends.
> Doing your work for you isn't poaching.
This isn't related to my work, as I have no professional interest in
virtio. In this particular instance I was trying to help out by fixing bugs
uncovered by the use of randconfig, which I'm doing in my own time, as a
hobby. I guess the use my work address clouds this fact, so I apologise for
that.
Sorry for any confusion or offence caused though Rusty. :)
Kind regards,
Lee
[-- Attachment #1.2: Type: text/html, Size: 780 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 resend] virtio_console: Free buffers from out-queue upon close
From: Sjur Brændeland @ 2012-11-08 9:25 UTC (permalink / raw)
To: Amit Shah
Cc: Masami Hiramatsu, virtualization, linux-kernel,
Michael S. Tsirkin
In-Reply-To: <20121108085913.GA31962@amit.redhat.com>
>> > Note: This patch is compile tested only. I have done the removal
>> > of buffers from out-queue in handle_control_message()
>> > when host has acked the close request. This seems less
>> > racy than doing it in the release function.
>>
>> This confuses me... why are we doing this in case
>> VIRTIO_CONSOLE_PORT_OPEN:?
>>
>> We can't pull unconsumed buffers out of the ring when the other side may
>> still access it, and this seems to be doing that.
>
> Yes -- and it's my fault; I asked Sjur to do that in the close fops
> function.
Thanks Amit :-), but this was really my bad.
> We should only do this in the port remove case (unplug or device
> remove) -- so the original patch, with just the WARN_ON removed is the
> right way.
>
> I'll send the revised 3/3 patch for you.
Thank you.
Regards,
Sjur
^ permalink raw reply
* [[PATCH v9 3/3] 1/1] virtio_console: Remove buffers from out_vq at port removal
From: Amit Shah @ 2012-11-08 9:17 UTC (permalink / raw)
To: Rusty Russell
Cc: Michael S. Tsirkin, sjur, linus.walleij, linux-kernel,
virtualization, Amit Shah, masami.hiramatsu.pt,
Sjur Brændeland
From: Sjur Brændeland <sjur.brandeland@stericsson.com>
Remove buffers from the out-queue when a port is removed. Rproc_serial
communicates with remote processors that may crash and leave buffers in
the out-queue. The virtio serial ports may have buffers in the out-queue
as well, e.g. for non-blocking ports and the host didn't consume them
yet.
[Amit: Remove WARN_ON for generic ports case.]
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9ebadcb..5ff3b3e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1521,6 +1521,10 @@ 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, true);
+
+ /* Remove buffers we queued up for the Host to consume */
+ while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
+ free_buf(buf, true);
}
/*
--
1.7.7.6
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCHv8 0/3]virtio_console: Add rproc_serial driver
From: Amit Shah @ 2012-11-08 9:06 UTC (permalink / raw)
To: Sjur Brændeland
Cc: Michael S. Tsirkin, Linus Walleij, linux-kernel, virtualization,
Masami Hiramatsu, Sjur Brændeland
In-Reply-To: <1351587113-2566-1-git-send-email-sjur@brendeland.net>
On (Tue) 30 Oct 2012 [09:51:50], Sjur Brændeland wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> This patch-set introduces a new virtio type "rproc_serial" for communicating
> with remote processors over shared memory. The driver depends on the
> the remoteproc framework. As preparation for introducing "rproc_serial"
> I've done a refactoring of the transmit buffer handling.
Thanks, Sjur.
Please pick the virtio spec from
https://github.com/rustyrussell/virtio-spec
and update the spec with info for remote-proc.
Thanks,
Amit
^ permalink raw reply
* Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close
From: Amit Shah @ 2012-11-08 8:59 UTC (permalink / raw)
To: Rusty Russell
Cc: Michael S. Tsirkin, sjur, linux-kernel, virtualization,
Masami Hiramatsu, sjur.brandeland
In-Reply-To: <87sj8l2cea.fsf@rustcorp.com.au>
On (Thu) 08 Nov 2012 [10:28:53], Rusty Russell wrote:
> sjur.brandeland@stericsson.com writes:
>
> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> >
> > Free pending output buffers from the virtio out-queue when
> > host has acknowledged port_close. Also removed WARN_ON()
> > in remove_port_data().
> >
> > Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> > ---
> >
> > Resending, this time including a proper "Subject"...
> > --
> >
> > Hi Amit,
> >
> > Note: This patch is compile tested only. I have done the removal
> > of buffers from out-queue in handle_control_message()
> > when host has acked the close request. This seems less
> > racy than doing it in the release function.
>
> This confuses me... why are we doing this in case
> VIRTIO_CONSOLE_PORT_OPEN:?
>
> We can't pull unconsumed buffers out of the ring when the other side may
> still access it, and this seems to be doing that.
Yes -- and it's my fault; I asked Sjur to do that in the close fops
function.
We should only do this in the port remove case (unplug or device
remove) -- so the original patch, with just the WARN_ON removed is the
right way.
I'll send the revised 3/3 patch for you.
Amit
^ permalink raw reply
* Re: [PATCH 1/1] vhost-blk: Add vhost-blk support v4
From: Asias He @ 2012-11-08 7:16 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <87wqxx2cxt.fsf@rustcorp.com.au>
Hello Rusty,
On Thu, Nov 8, 2012 at 7:47 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Asias He <asias@redhat.com> writes:
>> vhost-blk is an in-kernel virito-blk device accelerator.
>>
>> Due to lack of proper in-kernel AIO interface, this version converts
>> guest's I/O request to bio and use submit_bio() to submit I/O directly.
>> So this version any supports raw block device as guest's disk image,
>> e.g. /dev/sda, /dev/ram0. We can add file based image support to
>> vhost-blk once we have in-kernel AIO interface. There are some work in
>> progress for in-kernel AIO interface from Dave Kleikamp and Zach Brown:
>>
>> http://marc.info/?l=linux-fsdevel&m=133312234313122
>
> OK, this generally looks quite neat. There is one significant bug,
> however:
>
>> +/* The block header is in the first and separate buffer. */
>> +#define BLK_HDR 0
>
> You need to do a proper pull off the iovec; you can't simply assume
> this. I'm working on fixing qemu, too.
Yes. I have changed the code to handle the buffer without assumption
about the layout already.
Just haven't sent out the new version. I will send it out after the kvm forum.
Cheers.
> linux/drivers/vhost/net.c simply skips the header, you want something
> which actually copies it from userspace:
>
> /* Returns 0, -EFAULT or -EINVAL (too short) */
> int copy_from_iovec_user(void *dst, size_t len, struct iovec *iov, int iov_nr);
> int copy_to_iovec_user(struct iovec *iov, int iov_nr, const void *src, size_t len);
>
> These consume the iov in place. You could pass struct iovec **iov and
> int * if you wanted to be really efficient (otherwise you have
> zero-length iov entries at the front after you've pulled things off).
>
> This goes away:
>> + if (hdr->type == VIRTIO_BLK_T_IN || hdr->type == VIRTIO_BLK_T_GET_ID)
>> + iov_nr = in - 1;
>> + else
>> + iov_nr = out - 1;
>
> This becomes a simple assignment:
>
>> + /* The block data buffer follows block header buffer */
>> + req->iov = &vq->iov[BLK_HDR + 1];
>
> This one actually requires iteration, since you should handle the case
> where the last iov is zero length:
>
>> + /* The block status buffer follows block data buffer */
>> + req->status = vq->iov[iov_nr + 1].iov_base;
>
> This becomes copy_to_iovec_user:
>
>> + case VIRTIO_BLK_T_GET_ID: {
>> + char id[VIRTIO_BLK_ID_BYTES];
>> + int len;
>> +
>> + ret = snprintf(id, VIRTIO_BLK_ID_BYTES,
>> + "vhost-blk%d", blk->index);
>> + if (ret < 0)
>> + break;
>> + len = ret;
>> + ret = __copy_to_user(req->iov[0].iov_base, id, len);
>
> This becomes copy_from_iovec_user:
>
>> + if (unlikely(copy_from_user(&hdr, vq->iov[BLK_HDR].iov_base,
>> + sizeof(hdr)))) {
>> + vq_err(vq, "Failed to get block header!\n");
>> + vhost_discard_vq_desc(vq, 1);
>> + break;
>> + }
>
> The rest looks OK, at a glance.
>
> 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
--
Asias He
^ permalink raw reply
* Re: [PATCH v11 5/7] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-11-08 0:34 UTC (permalink / raw)
To: Rusty Russell
Cc: Rik van Riel, Michael S. Tsirkin, Konrad Rzeszutek Wilk,
linux-kernel, virtualization, linux-mm, Peter Zijlstra,
Andi Kleen, Minchan Kim, Andrew Morton, Paul E. McKenney
In-Reply-To: <87625h3tl1.fsf@rustcorp.com.au>
On Thu, Nov 08, 2012 at 09:32:18AM +1030, Rusty Russell wrote:
> The first one can be delayed, the second one can be delayed if the host
> didn't ask for VIRTIO_BALLOON_F_MUST_TELL_HOST (qemu doesn't).
>
> We could implement a proper request queue for these, and return -EAGAIN
> if the queue fills. Though in practice, it's not important (it might
> help performance).
I liked the idea. Give me the directions to accomplish it and I'll give it a try
for sure.
^ permalink raw reply
* Re: [PATCH v11 5/7] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-11-08 0:32 UTC (permalink / raw)
To: Andrew Morton
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Peter Zijlstra,
Andi Kleen, Minchan Kim, Paul E. McKenney
In-Reply-To: <20121107161146.b99dc4a8.akpm@linux-foundation.org>
On Wed, Nov 07, 2012 at 04:11:46PM -0800, Andrew Morton wrote:
> On Thu, 08 Nov 2012 09:32:18 +1030
> Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> > Rafael Aquini <aquini@redhat.com> writes:
> > > + * virtballoon_migratepage - perform the balloon page migration on behalf of
> > > + * a compation thread. (called under page lock)
> >
> > > + if (!mutex_trylock(&vb->balloon_lock))
> > > + return -EAGAIN;
> >
> > Erk, OK...
>
> Not really. As is almost always the case with a trylock, it needs a
> comment explaining why we couldn't use the far superior mutex_lock().
> Data: this reader doesn't know!
>
That was just to alleviate balloon_lock contention if we're migrating pages
concurrently with balloon_fill() or balloon_leak(), as it's easier to retry
the page migration later (in the contended case).
> > > + /* balloon's page migration 1st step -- inflate "newpage" */
> > > + spin_lock_irqsave(&vb_dev_info->pages_lock, flags);
> > > + balloon_page_insert(newpage, mapping, &vb_dev_info->pages);
> > > + vb_dev_info->isolated_pages--;
> > > + spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
> > > + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> > > + set_page_pfns(vb->pfns, newpage);
> > > + tell_host(vb, vb->inflate_vq);
> >
> > tell_host does wait_event(), so you can't call it under the page_lock.
> > Right?
>
> Sleeping inside lock_page() is OK. More problematic is that GFP_KERNEL
> allocation. iirc it _should_ be OK. Core VM uses trylock_page() and
> the filesystems shouldn't be doing a synchronous lock_page() in the
> pageout path. But I suspect it isn't a well-tested area.
The locked page under migration is not contended by any other FS / core VM path,
as it is already isolated by compaction to a private migration page list.
OTOH, there's a chance of another parallel compaction thread hitting this page
while scanning page blocks for isolation, but that path can be considered safe
as it uses trylock_page()
>
> > You probably get away with it because current qemu will service you
> > immediately. You could spin here in this case for the moment.
> >
> > There's a second call to tell_host():
> >
> > > + /*
> > > + * balloon's page migration 2nd step -- deflate "page"
> > > + *
> > > + * It's safe to delete page->lru here because this page is at
> > > + * an isolated migration list, and this step is expected to happen here
> > > + */
> > > + balloon_page_delete(page);
> > > + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> > > + set_page_pfns(vb->pfns, page);
> > > + tell_host(vb, vb->deflate_vq);
> >
> > The first one can be delayed, the second one can be delayed if the host
> > didn't ask for VIRTIO_BALLOON_F_MUST_TELL_HOST (qemu doesn't).
> >
> > We could implement a proper request queue for these, and return -EAGAIN
> > if the queue fills. Though in practice, it's not important (it might
> > help performance).
>
^ permalink raw reply
* Re: [PATCH v11 5/7] virtio_balloon: introduce migration primitives to balloon pages
From: Andrew Morton @ 2012-11-08 0:11 UTC (permalink / raw)
To: Rusty Russell
Cc: Rik van Riel, Rafael Aquini, Michael S. Tsirkin,
Konrad Rzeszutek Wilk, linux-kernel, virtualization, linux-mm,
Peter Zijlstra, Andi Kleen, Minchan Kim, Paul E. McKenney
In-Reply-To: <87625h3tl1.fsf@rustcorp.com.au>
On Thu, 08 Nov 2012 09:32:18 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:
> Rafael Aquini <aquini@redhat.com> writes:
> > + * virtballoon_migratepage - perform the balloon page migration on behalf of
> > + * a compation thread. (called under page lock)
>
> > + if (!mutex_trylock(&vb->balloon_lock))
> > + return -EAGAIN;
>
> Erk, OK...
Not really. As is almost always the case with a trylock, it needs a
comment explaining why we couldn't use the far superior mutex_lock().
Data: this reader doesn't know!
> > + /* balloon's page migration 1st step -- inflate "newpage" */
> > + spin_lock_irqsave(&vb_dev_info->pages_lock, flags);
> > + balloon_page_insert(newpage, mapping, &vb_dev_info->pages);
> > + vb_dev_info->isolated_pages--;
> > + spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
> > + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> > + set_page_pfns(vb->pfns, newpage);
> > + tell_host(vb, vb->inflate_vq);
>
> tell_host does wait_event(), so you can't call it under the page_lock.
> Right?
Sleeping inside lock_page() is OK. More problematic is that GFP_KERNEL
allocation. iirc it _should_ be OK. Core VM uses trylock_page() and
the filesystems shouldn't be doing a synchronous lock_page() in the
pageout path. But I suspect it isn't a well-tested area.
> You probably get away with it because current qemu will service you
> immediately. You could spin here in this case for the moment.
>
> There's a second call to tell_host():
>
> > + /*
> > + * balloon's page migration 2nd step -- deflate "page"
> > + *
> > + * It's safe to delete page->lru here because this page is at
> > + * an isolated migration list, and this step is expected to happen here
> > + */
> > + balloon_page_delete(page);
> > + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> > + set_page_pfns(vb->pfns, page);
> > + tell_host(vb, vb->deflate_vq);
>
> The first one can be delayed, the second one can be delayed if the host
> didn't ask for VIRTIO_BALLOON_F_MUST_TELL_HOST (qemu doesn't).
>
> We could implement a proper request queue for these, and return -EAGAIN
> if the queue fills. Though in practice, it's not important (it might
> help performance).
^ permalink raw reply
* Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close
From: Rusty Russell @ 2012-11-07 23:58 UTC (permalink / raw)
To: Amit Shah
Cc: Michael S. Tsirkin, sjur, linux-kernel, virtualization,
Masami Hiramatsu, Sjur Brændeland
In-Reply-To: <1352295822-22906-1-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Free pending output buffers from the virtio out-queue when
> host has acknowledged port_close. Also removed WARN_ON()
> in remove_port_data().
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>
> Resending, this time including a proper "Subject"...
> --
>
> Hi Amit,
>
> Note: This patch is compile tested only. I have done the removal
> of buffers from out-queue in handle_control_message()
> when host has acked the close request. This seems less
> racy than doing it in the release function.
This confuses me... why are we doing this in case
VIRTIO_CONSOLE_PORT_OPEN:?
We can't pull unconsumed buffers out of the ring when the other side may
still access it, and this seems to be doing that.
Thanks,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 1/1] vhost-blk: Add vhost-blk support v4
From: Rusty Russell @ 2012-11-07 23:47 UTC (permalink / raw)
To: Asias He, Michael S. Tsirkin; +Cc: kvm, virtualization
In-Reply-To: <1350295023-9179-1-git-send-email-asias@redhat.com>
Asias He <asias@redhat.com> writes:
> vhost-blk is an in-kernel virito-blk device accelerator.
>
> Due to lack of proper in-kernel AIO interface, this version converts
> guest's I/O request to bio and use submit_bio() to submit I/O directly.
> So this version any supports raw block device as guest's disk image,
> e.g. /dev/sda, /dev/ram0. We can add file based image support to
> vhost-blk once we have in-kernel AIO interface. There are some work in
> progress for in-kernel AIO interface from Dave Kleikamp and Zach Brown:
>
> http://marc.info/?l=linux-fsdevel&m=133312234313122
OK, this generally looks quite neat. There is one significant bug,
however:
> +/* The block header is in the first and separate buffer. */
> +#define BLK_HDR 0
You need to do a proper pull off the iovec; you can't simply assume
this. I'm working on fixing qemu, too.
linux/drivers/vhost/net.c simply skips the header, you want something
which actually copies it from userspace:
/* Returns 0, -EFAULT or -EINVAL (too short) */
int copy_from_iovec_user(void *dst, size_t len, struct iovec *iov, int iov_nr);
int copy_to_iovec_user(struct iovec *iov, int iov_nr, const void *src, size_t len);
These consume the iov in place. You could pass struct iovec **iov and
int * if you wanted to be really efficient (otherwise you have
zero-length iov entries at the front after you've pulled things off).
This goes away:
> + if (hdr->type == VIRTIO_BLK_T_IN || hdr->type == VIRTIO_BLK_T_GET_ID)
> + iov_nr = in - 1;
> + else
> + iov_nr = out - 1;
This becomes a simple assignment:
> + /* The block data buffer follows block header buffer */
> + req->iov = &vq->iov[BLK_HDR + 1];
This one actually requires iteration, since you should handle the case
where the last iov is zero length:
> + /* The block status buffer follows block data buffer */
> + req->status = vq->iov[iov_nr + 1].iov_base;
This becomes copy_to_iovec_user:
> + case VIRTIO_BLK_T_GET_ID: {
> + char id[VIRTIO_BLK_ID_BYTES];
> + int len;
> +
> + ret = snprintf(id, VIRTIO_BLK_ID_BYTES,
> + "vhost-blk%d", blk->index);
> + if (ret < 0)
> + break;
> + len = ret;
> + ret = __copy_to_user(req->iov[0].iov_base, id, len);
This becomes copy_from_iovec_user:
> + if (unlikely(copy_from_user(&hdr, vq->iov[BLK_HDR].iov_base,
> + sizeof(hdr)))) {
> + vq_err(vq, "Failed to get block header!\n");
> + vhost_discard_vq_desc(vq, 1);
> + break;
> + }
The rest looks OK, at a glance.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH v11 5/7] virtio_balloon: introduce migration primitives to balloon pages
From: Rusty Russell @ 2012-11-07 23:02 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, aquini, Michael S. Tsirkin, Konrad Rzeszutek Wilk,
linux-kernel, virtualization, Minchan Kim, Peter Zijlstra,
Andi Kleen, Andrew Morton, Paul E. McKenney
In-Reply-To: <265aaff9a79f503672f0cdcdff204114b5b5ba5b.1352256088.git.aquini@redhat.com>
Rafael Aquini <aquini@redhat.com> writes:
> + * virtballoon_migratepage - perform the balloon page migration on behalf of
> + * a compation thread. (called under page lock)
> + if (!mutex_trylock(&vb->balloon_lock))
> + return -EAGAIN;
Erk, OK...
> + /* balloon's page migration 1st step -- inflate "newpage" */
> + spin_lock_irqsave(&vb_dev_info->pages_lock, flags);
> + balloon_page_insert(newpage, mapping, &vb_dev_info->pages);
> + vb_dev_info->isolated_pages--;
> + spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb->pfns, newpage);
> + tell_host(vb, vb->inflate_vq);
tell_host does wait_event(), so you can't call it under the page_lock.
Right?
You probably get away with it because current qemu will service you
immediately. You could spin here in this case for the moment.
There's a second call to tell_host():
> + /*
> + * balloon's page migration 2nd step -- deflate "page"
> + *
> + * It's safe to delete page->lru here because this page is at
> + * an isolated migration list, and this step is expected to happen here
> + */
> + balloon_page_delete(page);
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb->pfns, page);
> + tell_host(vb, vb->deflate_vq);
The first one can be delayed, the second one can be delayed if the host
didn't ask for VIRTIO_BALLOON_F_MUST_TELL_HOST (qemu doesn't).
We could implement a proper request queue for these, and return -EAGAIN
if the queue fills. Though in practice, it's not important (it might
help performance).
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH v2] virtio-mmio: Fix irq parsing in the command line
From: Rusty Russell @ 2012-11-07 22:47 UTC (permalink / raw)
Cc: Pawel Moll, virtualization
In-Reply-To: <1352297892-27838-1-git-send-email-pawel.moll@arm.com>
Pawel Moll <pawel.moll@arm.com> writes:
> On 64-bit machines resource_size_t is a 64-bit value, while
> sscanf() format for this argument was defined as "%u". Fixed
> by using an intermediate local value of a known length.
Actually, on 32-bit machines, too (eg. x86 with PAE). Otherwise we'd
just change it to %lu.
> + /* Get "@<base>:<irq>[:<id>]" chunks */
> processed = sscanf(str, "@%lli:%u%n:%d%n",
> - &base, &resources[1].start, &consumed,
> + &base, &irq, &consumed,
> &vm_cmdline_id, &consumed);
>
> + /*
> + * sscanf() processes 3 chunks if "<id>" is given, 2 if not;
> + * also there must be no extra characters after the last
> + * chunk, so str[consumed] should be '\0'
> + */
> if (processed < 2 || processed > 3 || str[consumed])
> return -EINVAL;
I would drop the > 3 case. It's unnecessary, and you're assuming
consumed doesn't add to the count, which may be true but is a documented
sscanf weirdness so I don't like to rely on it.
Thanks,
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