* [Qemu-devel] [PATCH 01/10] migration: Fix migrate_test_apply for multifd parameters
2017-10-10 18:15 [Qemu-devel] [PATCH 00/10] Make xbzrle_cache_size a migration parameter Juan Quintela
@ 2017-10-10 18:15 ` Juan Quintela
2017-10-12 5:46 ` Peter Xu
2017-10-10 18:15 ` [Qemu-devel] [PATCH 02/10] migratiom: Remove max_item_age parameter Juan Quintela
` (8 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Juan Quintela @ 2017-10-10 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
They were missing when introduced on the tree
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/migration.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 98429dc843..fb62a639d8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -865,6 +865,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
if (params->has_block_incremental) {
dest->block_incremental = params->block_incremental;
}
+ if (params->has_x_multifd_channels) {
+ dest->x_multifd_channels = params->x_multifd_channels;
+ }
+ if (params->has_x_multifd_page_count) {
+ dest->x_multifd_page_count = params->x_multifd_page_count;
+ }
}
static void migrate_params_apply(MigrateSetParameters *params)
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 01/10] migration: Fix migrate_test_apply for multifd parameters
2017-10-10 18:15 ` [Qemu-devel] [PATCH 01/10] migration: Fix migrate_test_apply for multifd parameters Juan Quintela
@ 2017-10-12 5:46 ` Peter Xu
2017-10-18 8:37 ` Juan Quintela
0 siblings, 1 reply; 26+ messages in thread
From: Peter Xu @ 2017-10-12 5:46 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, dgilbert, lvivier
On Tue, Oct 10, 2017 at 08:15:33PM +0200, Juan Quintela wrote:
> They were missing when introduced on the tree
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/migration.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 98429dc843..fb62a639d8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -865,6 +865,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
> if (params->has_block_incremental) {
> dest->block_incremental = params->block_incremental;
> }
(newline?)
> + if (params->has_x_multifd_channels) {
> + dest->x_multifd_channels = params->x_multifd_channels;
> + }
(newline?)
> + if (params->has_x_multifd_page_count) {
> + dest->x_multifd_page_count = params->x_multifd_page_count;
> + }
> }
>
> static void migrate_params_apply(MigrateSetParameters *params)
> --
> 2.13.6
>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 01/10] migration: Fix migrate_test_apply for multifd parameters
2017-10-12 5:46 ` Peter Xu
@ 2017-10-18 8:37 ` Juan Quintela
0 siblings, 0 replies; 26+ messages in thread
From: Juan Quintela @ 2017-10-18 8:37 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, dgilbert, lvivier
Peter Xu <peterx@redhat.com> wrote:
> On Tue, Oct 10, 2017 at 08:15:33PM +0200, Juan Quintela wrote:
>> They were missing when introduced on the tree
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> migration/migration.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 98429dc843..fb62a639d8 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -865,6 +865,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>> if (params->has_block_incremental) {
>> dest->block_incremental = params->block_incremental;
>> }
>
> (newline?)
I preffer without it.
At least in migration we have code with both approachs.
>
>> + if (params->has_x_multifd_channels) {
>> + dest->x_multifd_channels = params->x_multifd_channels;
>> + }
>
> (newline?)
>
>> + if (params->has_x_multifd_page_count) {
>> + dest->x_multifd_page_count = params->x_multifd_page_count;
>> + }
>> }
>>
>> static void migrate_params_apply(MigrateSetParameters *params)
>> --
>> 2.13.6
>>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 02/10] migratiom: Remove max_item_age parameter
2017-10-10 18:15 [Qemu-devel] [PATCH 00/10] Make xbzrle_cache_size a migration parameter Juan Quintela
2017-10-10 18:15 ` [Qemu-devel] [PATCH 01/10] migration: Fix migrate_test_apply for multifd parameters Juan Quintela
@ 2017-10-10 18:15 ` Juan Quintela
2017-10-12 5:46 ` Peter Xu
2017-10-10 18:15 ` [Qemu-devel] [PATCH 03/10] migration: Make cache size elements use the right types Juan Quintela
` (7 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Juan Quintela @ 2017-10-10 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
It was not used at all since commit:
27af7d6ea5015e5ef1f7985eab94a8a218267a2b
which replaced its use by the dirty sync count.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/page_cache.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/migration/page_cache.c b/migration/page_cache.c
index ba984c4858..381e555ddb 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -41,7 +41,6 @@ struct PageCache {
CacheItem *page_cache;
unsigned int page_size;
int64_t max_num_items;
- uint64_t max_item_age;
int64_t num_items;
};
@@ -69,7 +68,6 @@ PageCache *cache_init(int64_t num_pages, unsigned int page_size)
}
cache->page_size = page_size;
cache->num_items = 0;
- cache->max_item_age = 0;
cache->max_num_items = num_pages;
DPRINTF("Setting cache buckets to %" PRId64 "\n", cache->max_num_items);
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 02/10] migratiom: Remove max_item_age parameter
2017-10-10 18:15 ` [Qemu-devel] [PATCH 02/10] migratiom: Remove max_item_age parameter Juan Quintela
@ 2017-10-12 5:46 ` Peter Xu
0 siblings, 0 replies; 26+ messages in thread
From: Peter Xu @ 2017-10-12 5:46 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, dgilbert, lvivier
On Tue, Oct 10, 2017 at 08:15:34PM +0200, Juan Quintela wrote:
> It was not used at all since commit:
>
> 27af7d6ea5015e5ef1f7985eab94a8a218267a2b
>
> which replaced its use by the dirty sync count.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
> ---
> migration/page_cache.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index ba984c4858..381e555ddb 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -41,7 +41,6 @@ struct PageCache {
> CacheItem *page_cache;
> unsigned int page_size;
> int64_t max_num_items;
> - uint64_t max_item_age;
> int64_t num_items;
> };
>
> @@ -69,7 +68,6 @@ PageCache *cache_init(int64_t num_pages, unsigned int page_size)
> }
> cache->page_size = page_size;
> cache->num_items = 0;
> - cache->max_item_age = 0;
> cache->max_num_items = num_pages;
>
> DPRINTF("Setting cache buckets to %" PRId64 "\n", cache->max_num_items);
> --
> 2.13.6
>
--
Peter Xu
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 03/10] migration: Make cache size elements use the right types
2017-10-10 18:15 [Qemu-devel] [PATCH 00/10] Make xbzrle_cache_size a migration parameter Juan Quintela
2017-10-10 18:15 ` [Qemu-devel] [PATCH 01/10] migration: Fix migrate_test_apply for multifd parameters Juan Quintela
2017-10-10 18:15 ` [Qemu-devel] [PATCH 02/10] migratiom: Remove max_item_age parameter Juan Quintela
@ 2017-10-10 18:15 ` Juan Quintela
2017-10-12 10:01 ` Dr. David Alan Gilbert
2017-10-10 18:15 ` [Qemu-devel] [PATCH 04/10] migration: Move xbzrle cache resize error handling to xbzrle_cache_resize Juan Quintela
` (6 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Juan Quintela @ 2017-10-10 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/page_cache.c | 8 ++++----
migration/page_cache.h | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/migration/page_cache.c b/migration/page_cache.c
index 381e555ddb..6b2dd77cf0 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -39,12 +39,12 @@ struct CacheItem {
struct PageCache {
CacheItem *page_cache;
- unsigned int page_size;
- int64_t max_num_items;
- int64_t num_items;
+ size_t page_size;
+ size_t max_num_items;
+ size_t num_items;
};
-PageCache *cache_init(int64_t num_pages, unsigned int page_size)
+PageCache *cache_init(size_t num_pages, size_t page_size)
{
int64_t i;
diff --git a/migration/page_cache.h b/migration/page_cache.h
index 4fadd0c501..931868b857 100644
--- a/migration/page_cache.h
+++ b/migration/page_cache.h
@@ -28,7 +28,7 @@ typedef struct PageCache PageCache;
* @num_pages: cache maximal number of cached pages
* @page_size: cache page size
*/
-PageCache *cache_init(int64_t num_pages, unsigned int page_size);
+PageCache *cache_init(size_t num_pages, size_t page_size);
/**
* cache_fini: free all cache resources
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 03/10] migration: Make cache size elements use the right types
2017-10-10 18:15 ` [Qemu-devel] [PATCH 03/10] migration: Make cache size elements use the right types Juan Quintela
@ 2017-10-12 10:01 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 26+ messages in thread
From: Dr. David Alan Gilbert @ 2017-10-12 10:01 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, lvivier, peterx
* Juan Quintela (quintela@redhat.com) wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/page_cache.c | 8 ++++----
> migration/page_cache.h | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index 381e555ddb..6b2dd77cf0 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -39,12 +39,12 @@ struct CacheItem {
>
> struct PageCache {
> CacheItem *page_cache;
> - unsigned int page_size;
> - int64_t max_num_items;
> - int64_t num_items;
> + size_t page_size;
> + size_t max_num_items;
> + size_t num_items;
> };
>
> -PageCache *cache_init(int64_t num_pages, unsigned int page_size)
> +PageCache *cache_init(size_t num_pages, size_t page_size)
> {
> int64_t i;
>
> diff --git a/migration/page_cache.h b/migration/page_cache.h
> index 4fadd0c501..931868b857 100644
> --- a/migration/page_cache.h
> +++ b/migration/page_cache.h
> @@ -28,7 +28,7 @@ typedef struct PageCache PageCache;
> * @num_pages: cache maximal number of cached pages
> * @page_size: cache page size
> */
> -PageCache *cache_init(int64_t num_pages, unsigned int page_size);
> +PageCache *cache_init(size_t num_pages, size_t page_size);
Makes sense; have you checked that the build is happy on 32bit
machines with xbzrle_cache_resize still passing an int64_t at this
point?
However, it's the right thing to move to, so:
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> /**
> * cache_fini: free all cache resources
> --
> 2.13.6
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 04/10] migration: Move xbzrle cache resize error handling to xbzrle_cache_resize
2017-10-10 18:15 [Qemu-devel] [PATCH 00/10] Make xbzrle_cache_size a migration parameter Juan Quintela
` (2 preceding siblings ...)
2017-10-10 18:15 ` [Qemu-devel] [PATCH 03/10] migration: Make cache size elements use the right types Juan Quintela
@ 2017-10-10 18:15 ` Juan Quintela
2017-10-12 5:48 ` Peter Xu
2017-10-10 18:15 ` [Qemu-devel] [PATCH 05/10] migration: Make cache_init() take an error parameter Juan Quintela
` (5 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Juan Quintela @ 2017-10-10 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/migration.c | 18 +-----------------
migration/ram.c | 22 ++++++++++++++++++++--
migration/ram.h | 2 +-
3 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index fb62a639d8..3feffb5e26 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1373,24 +1373,8 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
MigrationState *s = migrate_get_current();
int64_t new_size;
- /* Check for truncation */
- if (value != (size_t)value) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
- "exceeding address space");
- return;
- }
-
- /* Cache should not be larger than guest ram size */
- if (value > ram_bytes_total()) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
- "exceeds guest ram size ");
- return;
- }
-
- new_size = xbzrle_cache_resize(value);
+ new_size = xbzrle_cache_resize(value, errp);
if (new_size < 0) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
- "is smaller than page size");
return;
}
diff --git a/migration/ram.c b/migration/ram.c
index b83f8977c5..7c3acad029 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -42,6 +42,7 @@
#include "postcopy-ram.h"
#include "migration/page_cache.h"
#include "qemu/error-report.h"
+#include "qapi/qmp/qerror.h"
#include "trace.h"
#include "exec/ram_addr.h"
#include "qemu/rcu_queue.h"
@@ -113,13 +114,30 @@ static void XBZRLE_cache_unlock(void)
* Returns the new_size or negative in case of error.
*
* @new_size: new cache size
+ * @errp: set *errp if the check failed, with reason
*/
-int64_t xbzrle_cache_resize(int64_t new_size)
+int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
{
PageCache *new_cache;
int64_t ret;
+ /* Check for truncation */
+ if (new_size != (size_t)new_size) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+ "exceeding address space");
+ return -1;
+ }
+
+ /* Cache should not be larger than guest ram size */
+ if (new_size > ram_bytes_total()) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+ "exceeds guest ram size");
+ return -1;
+ }
+
if (new_size < TARGET_PAGE_SIZE) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+ "is smaller than one target page size");
return -1;
}
@@ -132,7 +150,7 @@ int64_t xbzrle_cache_resize(int64_t new_size)
new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
TARGET_PAGE_SIZE);
if (!new_cache) {
- error_report("Error creating cache");
+ error_setg(errp, "Error creating cache");
ret = -1;
goto out;
}
diff --git a/migration/ram.h b/migration/ram.h
index 4a72d66503..511b3dc582 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -35,7 +35,7 @@
extern MigrationStats ram_counters;
extern XBZRLECacheStats xbzrle_counters;
-int64_t xbzrle_cache_resize(int64_t new_size);
+int64_t xbzrle_cache_resize(int64_t new_size, Error **errp);
uint64_t ram_bytes_remaining(void);
uint64_t ram_bytes_total(void);
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* [Qemu-devel] [PATCH 05/10] migration: Make cache_init() take an error parameter
2017-10-10 18:15 [Qemu-devel] [PATCH 00/10] Make xbzrle_cache_size a migration parameter Juan Quintela
` (3 preceding siblings ...)
2017-10-10 18:15 ` [Qemu-devel] [PATCH 04/10] migration: Move xbzrle cache resize error handling to xbzrle_cache_resize Juan Quintela
@ 2017-10-10 18:15 ` Juan Quintela
2017-10-12 5:55 ` Peter Xu
2017-10-10 18:15 ` [Qemu-devel] [PATCH 06/10] migration: Make sure that we pass the right cache size Juan Quintela
` (4 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Juan Quintela @ 2017-10-10 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
Once there, take a total size instead of the size of the pages. We
move the check that the new_size is bigger than one page from
xbzrle_cache_resize().
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/page_cache.c | 17 +++++++++++------
migration/page_cache.h | 7 +++----
migration/ram.c | 18 +++++-------------
3 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/migration/page_cache.c b/migration/page_cache.c
index 6b2dd77cf0..9a9d13d6a2 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -14,6 +14,8 @@
#include "qemu/osdep.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/error.h"
#include "qemu-common.h"
#include "qemu/host-utils.h"
#include "migration/page_cache.h"
@@ -44,21 +46,23 @@ struct PageCache {
size_t num_items;
};
-PageCache *cache_init(size_t num_pages, size_t page_size)
+PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
{
int64_t i;
-
+ size_t num_pages = new_size / page_size;
PageCache *cache;
- if (num_pages <= 0) {
- DPRINTF("invalid number of pages\n");
+ if (new_size < page_size) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+ "is smaller than one target page size");
return NULL;
}
/* We prefer not to abort if there is no memory */
cache = g_try_malloc(sizeof(*cache));
if (!cache) {
- DPRINTF("Failed to allocate cache\n");
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+ "Failed to allocate cache");
return NULL;
}
/* round down to the nearest power of 2 */
@@ -76,7 +80,8 @@ PageCache *cache_init(size_t num_pages, size_t page_size)
cache->page_cache = g_try_malloc((cache->max_num_items) *
sizeof(*cache->page_cache));
if (!cache->page_cache) {
- DPRINTF("Failed to allocate cache->page_cache\n");
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+ "Failed to allocate page cache");
g_free(cache);
return NULL;
}
diff --git a/migration/page_cache.h b/migration/page_cache.h
index 931868b857..4596496416 100644
--- a/migration/page_cache.h
+++ b/migration/page_cache.h
@@ -24,12 +24,11 @@ typedef struct PageCache PageCache;
*
* Returns new allocated cache or NULL on error
*
- * @cache pointer to the PageCache struct
- * @num_pages: cache maximal number of cached pages
+ * @cache_size: cache size in byets
* @page_size: cache page size
+ * @errp: set *errp if the check failed, with reason
*/
-PageCache *cache_init(size_t num_pages, size_t page_size);
-
+PageCache *cache_init(int64_t cache_size, size_t page_size, Error **errp);
/**
* cache_fini: free all cache resources
* @cache pointer to the PageCache struct
diff --git a/migration/ram.c b/migration/ram.c
index 7c3acad029..47501460c8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -135,22 +135,14 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
return -1;
}
- if (new_size < TARGET_PAGE_SIZE) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
- "is smaller than one target page size");
- return -1;
- }
-
XBZRLE_cache_lock();
if (XBZRLE.cache != NULL) {
if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
goto out_new_size;
}
- new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
- TARGET_PAGE_SIZE);
+ new_cache = cache_init(new_size, TARGET_PAGE_SIZE, errp);
if (!new_cache) {
- error_setg(errp, "Error creating cache");
ret = -1;
goto out;
}
@@ -2028,6 +2020,7 @@ err:
static int ram_state_init(RAMState **rsp)
{
*rsp = g_new0(RAMState, 1);
+ Error *local_err = NULL;
qemu_mutex_init(&(*rsp)->bitmap_mutex);
qemu_mutex_init(&(*rsp)->src_page_req_mutex);
@@ -2036,12 +2029,11 @@ static int ram_state_init(RAMState **rsp)
if (migrate_use_xbzrle()) {
XBZRLE_cache_lock();
XBZRLE.zero_target_page = g_malloc0(TARGET_PAGE_SIZE);
- XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
- TARGET_PAGE_SIZE,
- TARGET_PAGE_SIZE);
+ XBZRLE.cache = cache_init(migrate_xbzrle_cache_size(),
+ TARGET_PAGE_SIZE, &local_err);
if (!XBZRLE.cache) {
XBZRLE_cache_unlock();
- error_report("Error creating cache");
+ error_report_err(local_err);
g_free(*rsp);
*rsp = NULL;
return -1;
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 05/10] migration: Make cache_init() take an error parameter
2017-10-10 18:15 ` [Qemu-devel] [PATCH 05/10] migration: Make cache_init() take an error parameter Juan Quintela
@ 2017-10-12 5:55 ` Peter Xu
0 siblings, 0 replies; 26+ messages in thread
From: Peter Xu @ 2017-10-12 5:55 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, dgilbert, lvivier
On Tue, Oct 10, 2017 at 08:15:37PM +0200, Juan Quintela wrote:
[...]
> diff --git a/migration/page_cache.h b/migration/page_cache.h
> index 931868b857..4596496416 100644
> --- a/migration/page_cache.h
> +++ b/migration/page_cache.h
> @@ -24,12 +24,11 @@ typedef struct PageCache PageCache;
> *
> * Returns new allocated cache or NULL on error
> *
> - * @cache pointer to the PageCache struct
> - * @num_pages: cache maximal number of cached pages
> + * @cache_size: cache size in byets
^^^^^
(typo)
> * @page_size: cache page size
> + * @errp: set *errp if the check failed, with reason
> */
> -PageCache *cache_init(size_t num_pages, size_t page_size);
> -
> +PageCache *cache_init(int64_t cache_size, size_t page_size, Error **errp);
Besides:
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 06/10] migration: Make sure that we pass the right cache size
2017-10-10 18:15 [Qemu-devel] [PATCH 00/10] Make xbzrle_cache_size a migration parameter Juan Quintela
` (4 preceding siblings ...)
2017-10-10 18:15 ` [Qemu-devel] [PATCH 05/10] migration: Make cache_init() take an error parameter Juan Quintela
@ 2017-10-10 18:15 ` Juan Quintela
2017-10-12 5:57 ` Peter Xu
2017-10-10 18:15 ` [Qemu-devel] [PATCH 07/10] migration: Don't play games with the requested " Juan Quintela
` (3 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Juan Quintela @ 2017-10-10 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
Instead of passing silently round down the number of pages, make it an
error that the cache size is not a multiple of 2.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/page_cache.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/migration/page_cache.c b/migration/page_cache.c
index 9a9d13d6a2..96268c3aea 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -58,6 +58,13 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
return NULL;
}
+ /* round down to the nearest power of 2 */
+ if (!is_power_of_2(num_pages)) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+ "is not a power of two number of pages");
+ return NULL;
+ }
+
/* We prefer not to abort if there is no memory */
cache = g_try_malloc(sizeof(*cache));
if (!cache) {
@@ -65,11 +72,6 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
"Failed to allocate cache");
return NULL;
}
- /* round down to the nearest power of 2 */
- if (!is_power_of_2(num_pages)) {
- num_pages = pow2floor(num_pages);
- DPRINTF("rounding down to %" PRId64 "\n", num_pages);
- }
cache->page_size = page_size;
cache->num_items = 0;
cache->max_num_items = num_pages;
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 06/10] migration: Make sure that we pass the right cache size
2017-10-10 18:15 ` [Qemu-devel] [PATCH 06/10] migration: Make sure that we pass the right cache size Juan Quintela
@ 2017-10-12 5:57 ` Peter Xu
2017-10-18 8:45 ` Juan Quintela
0 siblings, 1 reply; 26+ messages in thread
From: Peter Xu @ 2017-10-12 5:57 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, dgilbert, lvivier
On Tue, Oct 10, 2017 at 08:15:38PM +0200, Juan Quintela wrote:
> Instead of passing silently round down the number of pages, make it an
> error that the cache size is not a multiple of 2.
s/multiple/power/?
Would this patch break existing users?
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/page_cache.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index 9a9d13d6a2..96268c3aea 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -58,6 +58,13 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
> return NULL;
> }
>
> + /* round down to the nearest power of 2 */
> + if (!is_power_of_2(num_pages)) {
> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> + "is not a power of two number of pages");
> + return NULL;
> + }
> +
> /* We prefer not to abort if there is no memory */
> cache = g_try_malloc(sizeof(*cache));
> if (!cache) {
> @@ -65,11 +72,6 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
> "Failed to allocate cache");
> return NULL;
> }
> - /* round down to the nearest power of 2 */
> - if (!is_power_of_2(num_pages)) {
> - num_pages = pow2floor(num_pages);
> - DPRINTF("rounding down to %" PRId64 "\n", num_pages);
> - }
> cache->page_size = page_size;
> cache->num_items = 0;
> cache->max_num_items = num_pages;
> --
> 2.13.6
>
--
Peter Xu
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 06/10] migration: Make sure that we pass the right cache size
2017-10-12 5:57 ` Peter Xu
@ 2017-10-18 8:45 ` Juan Quintela
0 siblings, 0 replies; 26+ messages in thread
From: Juan Quintela @ 2017-10-18 8:45 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, dgilbert, lvivier
Peter Xu <peterx@redhat.com> wrote:
> On Tue, Oct 10, 2017 at 08:15:38PM +0200, Juan Quintela wrote:
>> Instead of passing silently round down the number of pages, make it an
>> error that the cache size is not a multiple of 2.
>
> s/multiple/power/?
Thanks.
> Would this patch break existing users?
I have a problem here. Current code:
- "silently" truncate the value that we pass
- "silently" uses a different value that the one that it shows as used
- "shows" a value that is different of what has been set to
So, I have to break one of them:
- only allow valid values (and then we output the same value that user
input)
- store "setup" value, and use something different "internaly"
No clear winner. This way we are at least consistent with everything
else. Documentation already required a power of 2 value. And we give a
good error message.
Later, Juan.
>
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> migration/page_cache.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/migration/page_cache.c b/migration/page_cache.c
>> index 9a9d13d6a2..96268c3aea 100644
>> --- a/migration/page_cache.c
>> +++ b/migration/page_cache.c
>> @@ -58,6 +58,13 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
>> return NULL;
>> }
>>
>> + /* round down to the nearest power of 2 */
>> + if (!is_power_of_2(num_pages)) {
>> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> + "is not a power of two number of pages");
>> + return NULL;
>> + }
>> +
>> /* We prefer not to abort if there is no memory */
>> cache = g_try_malloc(sizeof(*cache));
>> if (!cache) {
>> @@ -65,11 +72,6 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
>> "Failed to allocate cache");
>> return NULL;
>> }
>> - /* round down to the nearest power of 2 */
>> - if (!is_power_of_2(num_pages)) {
>> - num_pages = pow2floor(num_pages);
>> - DPRINTF("rounding down to %" PRId64 "\n", num_pages);
>> - }
>> cache->page_size = page_size;
>> cache->num_items = 0;
>> cache->max_num_items = num_pages;
>> --
>> 2.13.6
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 07/10] migration: Don't play games with the requested cache size
2017-10-10 18:15 [Qemu-devel] [PATCH 00/10] Make xbzrle_cache_size a migration parameter Juan Quintela
` (5 preceding siblings ...)
2017-10-10 18:15 ` [Qemu-devel] [PATCH 06/10] migration: Make sure that we pass the right cache size Juan Quintela
@ 2017-10-10 18:15 ` Juan Quintela
2017-10-10 18:15 ` [Qemu-devel] [PATCH 08/10] migration: No need to return the size of the cache Juan Quintela
` (2 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Juan Quintela @ 2017-10-10 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
Now that we check that the value passed is a power of 2, we don't need
to play games when comparing what is the size that is going to take
the cache.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 47501460c8..c84f22d759 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -135,12 +135,14 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
return -1;
}
+ if (new_size == migrate_xbzrle_cache_size()) {
+ /* nothing to do */
+ return new_size;
+ }
+
XBZRLE_cache_lock();
if (XBZRLE.cache != NULL) {
- if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
- goto out_new_size;
- }
new_cache = cache_init(new_size, TARGET_PAGE_SIZE, errp);
if (!new_cache) {
ret = -1;
@@ -151,8 +153,7 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
XBZRLE.cache = new_cache;
}
-out_new_size:
- ret = pow2floor(new_size);
+ ret = new_size;
out:
XBZRLE_cache_unlock();
return ret;
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* [Qemu-devel] [PATCH 08/10] migration: No need to return the size of the cache
2017-10-10 18:15 [Qemu-devel] [PATCH 00/10] Make xbzrle_cache_size a migration parameter Juan Quintela
` (6 preceding siblings ...)
2017-10-10 18:15 ` [Qemu-devel] [PATCH 07/10] migration: Don't play games with the requested " Juan Quintela
@ 2017-10-10 18:15 ` Juan Quintela
2017-10-10 18:15 ` [Qemu-devel] [PATCH 09/10] migration: Make xbzrle_cache_size a migration parameter Juan Quintela
2017-10-10 18:15 ` [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json Juan Quintela
9 siblings, 0 replies; 26+ messages in thread
From: Juan Quintela @ 2017-10-10 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
After the previous commits, we make sure that the value passed is
right, or we just drop an error. So now we return if there is one
error or we have setup correctly the value passed.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/migration.c | 6 ++----
migration/ram.c | 8 +++-----
migration/ram.h | 2 +-
3 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 3feffb5e26..f3d4503ce2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1371,14 +1371,12 @@ void qmp_migrate_cancel(Error **errp)
void qmp_migrate_set_cache_size(int64_t value, Error **errp)
{
MigrationState *s = migrate_get_current();
- int64_t new_size;
- new_size = xbzrle_cache_resize(value, errp);
- if (new_size < 0) {
+ if (xbzrle_cache_resize(value, errp) < 0) {
return;
}
- s->xbzrle_cache_size = new_size;
+ s->xbzrle_cache_size = value;
}
int64_t qmp_query_migrate_cache_size(Error **errp)
diff --git a/migration/ram.c b/migration/ram.c
index c84f22d759..ed4d3c6295 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -111,15 +111,15 @@ static void XBZRLE_cache_unlock(void)
* migration may be using the cache and might finish during this call,
* hence changes to the cache are protected by XBZRLE.lock().
*
- * Returns the new_size or negative in case of error.
+ * Returns the 0 or negative in case of error.
*
* @new_size: new cache size
* @errp: set *errp if the check failed, with reason
*/
-int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
+int xbzrle_cache_resize(int64_t new_size, Error **errp)
{
PageCache *new_cache;
- int64_t ret;
+ int64_t ret = 0;
/* Check for truncation */
if (new_size != (size_t)new_size) {
@@ -152,8 +152,6 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
cache_fini(XBZRLE.cache);
XBZRLE.cache = new_cache;
}
-
- ret = new_size;
out:
XBZRLE_cache_unlock();
return ret;
diff --git a/migration/ram.h b/migration/ram.h
index 511b3dc582..c8ae382b5b 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -35,7 +35,7 @@
extern MigrationStats ram_counters;
extern XBZRLECacheStats xbzrle_counters;
-int64_t xbzrle_cache_resize(int64_t new_size, Error **errp);
+int xbzrle_cache_resize(int64_t new_size, Error **errp);
uint64_t ram_bytes_remaining(void);
uint64_t ram_bytes_total(void);
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* [Qemu-devel] [PATCH 09/10] migration: Make xbzrle_cache_size a migration parameter
2017-10-10 18:15 [Qemu-devel] [PATCH 00/10] Make xbzrle_cache_size a migration parameter Juan Quintela
` (7 preceding siblings ...)
2017-10-10 18:15 ` [Qemu-devel] [PATCH 08/10] migration: No need to return the size of the cache Juan Quintela
@ 2017-10-10 18:15 ` Juan Quintela
2017-10-12 12:04 ` Dr. David Alan Gilbert
2017-10-10 18:15 ` [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json Juan Quintela
9 siblings, 1 reply; 26+ messages in thread
From: Juan Quintela @ 2017-10-10 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
Right now it is a variable in MigrationState instead of a
MigrationParameter. The change allows to set it as the rest of the
Migration parameters, from the command line, with
query_migration_paramters, set_migrate_parameters, etc.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
hmp.c | 14 ++++++++++++++
migration/migration.c | 39 ++++++++++++++++++++++++++++++---------
migration/migration.h | 1 -
migration/ram.c | 7 -------
qapi/migration.json | 23 ++++++++++++++++++++---
5 files changed, 64 insertions(+), 20 deletions(-)
diff --git a/hmp.c b/hmp.c
index 739d330f4e..f73232399a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -342,6 +342,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "%s: %" PRId64 "\n",
MigrationParameter_str(MIGRATION_PARAMETER_X_MULTIFD_PAGE_COUNT),
params->x_multifd_page_count);
+ monitor_printf(mon, "%s: %" PRId64 "\n",
+ MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
+ params->xbzrle_cache_size);
}
qapi_free_MigrationParameters(params);
@@ -1560,6 +1563,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
Visitor *v = string_input_visitor_new(valuestr);
MigrateSetParameters *p = g_new0(MigrateSetParameters, 1);
uint64_t valuebw = 0;
+ uint64_t cache_size;
Error *err = NULL;
int val, ret;
@@ -1635,6 +1639,16 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
p->has_x_multifd_page_count = true;
visit_type_int(v, param, &p->x_multifd_page_count, &err);
break;
+ case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
+ p->has_xbzrle_cache_size = true;
+ visit_type_size(v, param, &cache_size, &err);
+ if (err || cache_size > INT64_MAX
+ || (size_t)cache_size != cache_size) {
+ error_setg(&err, "Invalid size %s", valuestr);
+ break;
+ }
+ p->xbzrle_cache_size = cache_size;
+ break;
default:
assert(0);
}
diff --git a/migration/migration.c b/migration/migration.c
index f3d4503ce2..fcc0d64625 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -71,7 +71,7 @@
#define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10
/* Migration XBZRLE default cache size */
-#define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
+#define DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE (64 * 1024 * 1024)
/* The delay time (in ms) between two COLO checkpoints
* Note: Please change this default value to 10000 when we support hybrid mode.
@@ -512,6 +512,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
params->x_multifd_channels = s->parameters.x_multifd_channels;
params->has_x_multifd_page_count = true;
params->x_multifd_page_count = s->parameters.x_multifd_page_count;
+ params->has_xbzrle_cache_size = true;
+ params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
return params;
}
@@ -810,6 +812,16 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
return false;
}
+ if (params->has_xbzrle_cache_size &&
+ (params->xbzrle_cache_size < qemu_target_page_size() ||
+ !is_power_of_2(params->xbzrle_cache_size))) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ "xbzrle_cache_size",
+ "is invalid, it should be bigger than target page size"
+ " and a power of two");
+ return false;
+ }
+
return true;
}
@@ -871,6 +883,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
if (params->has_x_multifd_page_count) {
dest->x_multifd_page_count = params->x_multifd_page_count;
}
+ if (params->has_xbzrle_cache_size) {
+ dest->xbzrle_cache_size = params->xbzrle_cache_size;
+ }
}
static void migrate_params_apply(MigrateSetParameters *params)
@@ -939,6 +954,10 @@ static void migrate_params_apply(MigrateSetParameters *params)
if (params->has_x_multifd_page_count) {
s->parameters.x_multifd_page_count = params->x_multifd_page_count;
}
+ if (params->has_xbzrle_cache_size) {
+ s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
+ xbzrle_cache_resize(params->xbzrle_cache_size, NULL);
+ }
}
void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
@@ -1370,13 +1389,12 @@ void qmp_migrate_cancel(Error **errp)
void qmp_migrate_set_cache_size(int64_t value, Error **errp)
{
- MigrationState *s = migrate_get_current();
+ MigrateSetParameters p = {
+ .has_xbzrle_cache_size = true,
+ .xbzrle_cache_size = value,
+ };
- if (xbzrle_cache_resize(value, errp) < 0) {
- return;
- }
-
- s->xbzrle_cache_size = value;
+ qmp_migrate_set_parameters(&p, errp);
}
int64_t qmp_query_migrate_cache_size(Error **errp)
@@ -1542,7 +1560,7 @@ int64_t migrate_xbzrle_cache_size(void)
s = migrate_get_current();
- return s->xbzrle_cache_size;
+ return s->parameters.xbzrle_cache_size;
}
bool migrate_use_block(void)
@@ -2312,6 +2330,9 @@ static Property migration_properties[] = {
DEFINE_PROP_INT64("x-multifd-page-count", MigrationState,
parameters.x_multifd_page_count,
DEFAULT_MIGRATE_MULTIFD_PAGE_COUNT),
+ DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
+ parameters.xbzrle_cache_size,
+ DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
/* Migration capabilities */
DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
@@ -2353,7 +2374,6 @@ static void migration_instance_init(Object *obj)
MigrationParameters *params = &ms->parameters;
ms->state = MIGRATION_STATUS_NONE;
- ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
ms->mbps = -1;
params->tls_hostname = g_strdup("");
@@ -2371,6 +2391,7 @@ static void migration_instance_init(Object *obj)
params->has_block_incremental = true;
params->has_x_multifd_channels = true;
params->has_x_multifd_page_count = true;
+ params->has_xbzrle_cache_size = true;
}
/*
diff --git a/migration/migration.h b/migration/migration.h
index b83cceadc4..d2a8d319f1 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -107,7 +107,6 @@ struct MigrationState
int64_t downtime;
int64_t expected_downtime;
bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
- int64_t xbzrle_cache_size;
int64_t setup_time;
/* Flag set once the migration has been asked to enter postcopy */
diff --git a/migration/ram.c b/migration/ram.c
index ed4d3c6295..78e3b80e39 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -128,13 +128,6 @@ int xbzrle_cache_resize(int64_t new_size, Error **errp)
return -1;
}
- /* Cache should not be larger than guest ram size */
- if (new_size > ram_bytes_total()) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
- "exceeds guest ram size");
- return -1;
- }
-
if (new_size == migrate_xbzrle_cache_size()) {
/* nothing to do */
return new_size;
diff --git a/qapi/migration.json b/qapi/migration.json
index f8b365e3f5..830b2e4480 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -474,6 +474,10 @@
# @x-multifd-page-count: Number of pages sent together to a thread
# The default value is 16 (since 2.11)
#
+# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
+# needs to be a multiple of the target page size
+# (Since 2.11)
+#
# Since: 2.4
##
{ 'enum': 'MigrationParameter',
@@ -481,7 +485,8 @@
'cpu-throttle-initial', 'cpu-throttle-increment',
'tls-creds', 'tls-hostname', 'max-bandwidth',
'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
- 'x-multifd-channels', 'x-multifd-page-count' ] }
+ 'x-multifd-channels', 'x-multifd-page-count',
+ 'xbzrle-cache-size' ] }
##
# @MigrateSetParameters:
@@ -545,6 +550,9 @@
# @x-multifd-page-count: Number of pages sent together to a thread
# The default value is 16 (since 2.11)
#
+# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
+# needs to be a multiple of the target page size
+# (Since 2.11)
# Since: 2.4
##
# TODO either fuse back into MigrationParameters, or make
@@ -562,7 +570,8 @@
'*x-checkpoint-delay': 'int',
'*block-incremental': 'bool',
'*x-multifd-channels': 'int',
- '*x-multifd-page-count': 'int' } }
+ '*x-multifd-page-count': 'int',
+ '*xbzrle-cache-size': 'size' } }
##
# @migrate-set-parameters:
@@ -641,6 +650,9 @@
# @x-multifd-page-count: Number of pages sent together to a thread
# The default value is 16 (since 2.11)
#
+# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
+# needs to be a multiple of the target page size
+# (Since 2.11)
# Since: 2.4
##
{ 'struct': 'MigrationParameters',
@@ -656,7 +668,8 @@
'*x-checkpoint-delay': 'int',
'*block-incremental': 'bool' ,
'*x-multifd-channels': 'int',
- '*x-multifd-page-count': 'int' } }
+ '*x-multifd-page-count': 'int',
+ '*xbzrle-cache-size': 'size' } }
##
# @query-migrate-parameters:
@@ -921,6 +934,8 @@
#
# Returns: nothing on success
#
+# Notes: This command is deprecated in favor of 'migrate-set-parameters'
+#
# Since: 1.2
#
# Example:
@@ -939,6 +954,8 @@
#
# Returns: XBZRLE cache size in bytes
#
+# Notes: This command is deprecated in favor of 'query-migrate-parameters'
+#
# Since: 1.2
#
# Example:
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 09/10] migration: Make xbzrle_cache_size a migration parameter
2017-10-10 18:15 ` [Qemu-devel] [PATCH 09/10] migration: Make xbzrle_cache_size a migration parameter Juan Quintela
@ 2017-10-12 12:04 ` Dr. David Alan Gilbert
2017-10-18 8:50 ` Juan Quintela
0 siblings, 1 reply; 26+ messages in thread
From: Dr. David Alan Gilbert @ 2017-10-12 12:04 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, lvivier, peterx
* Juan Quintela (quintela@redhat.com) wrote:
> Right now it is a variable in MigrationState instead of a
> MigrationParameter. The change allows to set it as the rest of the
> Migration parameters, from the command line, with
> query_migration_paramters, set_migrate_parameters, etc.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> hmp.c | 14 ++++++++++++++
> migration/migration.c | 39 ++++++++++++++++++++++++++++++---------
> migration/migration.h | 1 -
> migration/ram.c | 7 -------
> qapi/migration.json | 23 ++++++++++++++++++++---
> 5 files changed, 64 insertions(+), 20 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 739d330f4e..f73232399a 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -342,6 +342,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
> monitor_printf(mon, "%s: %" PRId64 "\n",
> MigrationParameter_str(MIGRATION_PARAMETER_X_MULTIFD_PAGE_COUNT),
> params->x_multifd_page_count);
> + monitor_printf(mon, "%s: %" PRId64 "\n",
> + MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
> + params->xbzrle_cache_size);
> }
>
> qapi_free_MigrationParameters(params);
> @@ -1560,6 +1563,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
> Visitor *v = string_input_visitor_new(valuestr);
> MigrateSetParameters *p = g_new0(MigrateSetParameters, 1);
> uint64_t valuebw = 0;
> + uint64_t cache_size;
> Error *err = NULL;
> int val, ret;
>
> @@ -1635,6 +1639,16 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
> p->has_x_multifd_page_count = true;
> visit_type_int(v, param, &p->x_multifd_page_count, &err);
> break;
> + case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
> + p->has_xbzrle_cache_size = true;
> + visit_type_size(v, param, &cache_size, &err);
> + if (err || cache_size > INT64_MAX
> + || (size_t)cache_size != cache_size) {
> + error_setg(&err, "Invalid size %s", valuestr);
> + break;
> + }
> + p->xbzrle_cache_size = cache_size;
> + break;
> default:
> assert(0);
> }
> diff --git a/migration/migration.c b/migration/migration.c
> index f3d4503ce2..fcc0d64625 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -71,7 +71,7 @@
> #define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10
>
> /* Migration XBZRLE default cache size */
> -#define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
> +#define DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE (64 * 1024 * 1024)
>
> /* The delay time (in ms) between two COLO checkpoints
> * Note: Please change this default value to 10000 when we support hybrid mode.
> @@ -512,6 +512,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
> params->x_multifd_channels = s->parameters.x_multifd_channels;
> params->has_x_multifd_page_count = true;
> params->x_multifd_page_count = s->parameters.x_multifd_page_count;
> + params->has_xbzrle_cache_size = true;
> + params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
>
> return params;
> }
> @@ -810,6 +812,16 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
> return false;
> }
>
> + if (params->has_xbzrle_cache_size &&
> + (params->xbzrle_cache_size < qemu_target_page_size() ||
> + !is_power_of_2(params->xbzrle_cache_size))) {
> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> + "xbzrle_cache_size",
> + "is invalid, it should be bigger than target page size"
> + " and a power of two");
> + return false;
> + }
> +
> return true;
> }
>
> @@ -871,6 +883,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
> if (params->has_x_multifd_page_count) {
> dest->x_multifd_page_count = params->x_multifd_page_count;
> }
> + if (params->has_xbzrle_cache_size) {
> + dest->xbzrle_cache_size = params->xbzrle_cache_size;
> + }
> }
>
> static void migrate_params_apply(MigrateSetParameters *params)
> @@ -939,6 +954,10 @@ static void migrate_params_apply(MigrateSetParameters *params)
> if (params->has_x_multifd_page_count) {
> s->parameters.x_multifd_page_count = params->x_multifd_page_count;
> }
> + if (params->has_xbzrle_cache_size) {
> + s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
> + xbzrle_cache_resize(params->xbzrle_cache_size, NULL);
Not having the errp is a pain; you've just moved all the error
checking into xbzrle_cache_resize, so I think we really need an error
pointer and to do something with it (even if it's just a local report).
> + }
> }
>
> void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> @@ -1370,13 +1389,12 @@ void qmp_migrate_cancel(Error **errp)
>
> void qmp_migrate_set_cache_size(int64_t value, Error **errp)
> {
> - MigrationState *s = migrate_get_current();
> + MigrateSetParameters p = {
> + .has_xbzrle_cache_size = true,
> + .xbzrle_cache_size = value,
> + };
>
> - if (xbzrle_cache_resize(value, errp) < 0) {
> - return;
> - }
> -
> - s->xbzrle_cache_size = value;
> + qmp_migrate_set_parameters(&p, errp);
> }
>
> int64_t qmp_query_migrate_cache_size(Error **errp)
> @@ -1542,7 +1560,7 @@ int64_t migrate_xbzrle_cache_size(void)
>
> s = migrate_get_current();
>
> - return s->xbzrle_cache_size;
> + return s->parameters.xbzrle_cache_size;
> }
>
> bool migrate_use_block(void)
> @@ -2312,6 +2330,9 @@ static Property migration_properties[] = {
> DEFINE_PROP_INT64("x-multifd-page-count", MigrationState,
> parameters.x_multifd_page_count,
> DEFAULT_MIGRATE_MULTIFD_PAGE_COUNT),
> + DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
> + parameters.xbzrle_cache_size,
> + DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
>
> /* Migration capabilities */
> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> @@ -2353,7 +2374,6 @@ static void migration_instance_init(Object *obj)
> MigrationParameters *params = &ms->parameters;
>
> ms->state = MIGRATION_STATUS_NONE;
> - ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
> ms->mbps = -1;
>
> params->tls_hostname = g_strdup("");
> @@ -2371,6 +2391,7 @@ static void migration_instance_init(Object *obj)
> params->has_block_incremental = true;
> params->has_x_multifd_channels = true;
> params->has_x_multifd_page_count = true;
> + params->has_xbzrle_cache_size = true;
> }
>
> /*
> diff --git a/migration/migration.h b/migration/migration.h
> index b83cceadc4..d2a8d319f1 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -107,7 +107,6 @@ struct MigrationState
> int64_t downtime;
> int64_t expected_downtime;
> bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
> - int64_t xbzrle_cache_size;
> int64_t setup_time;
>
> /* Flag set once the migration has been asked to enter postcopy */
> diff --git a/migration/ram.c b/migration/ram.c
> index ed4d3c6295..78e3b80e39 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -128,13 +128,6 @@ int xbzrle_cache_resize(int64_t new_size, Error **errp)
> return -1;
> }
>
> - /* Cache should not be larger than guest ram size */
> - if (new_size > ram_bytes_total()) {
> - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> - "exceeds guest ram size");
> - return -1;
> - }
> -
> if (new_size == migrate_xbzrle_cache_size()) {
> /* nothing to do */
> return new_size;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index f8b365e3f5..830b2e4480 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -474,6 +474,10 @@
> # @x-multifd-page-count: Number of pages sent together to a thread
> # The default value is 16 (since 2.11)
> #
> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
> +# needs to be a multiple of the target page size
and power of 2
> +# (Since 2.11)
> +#
> # Since: 2.4
> ##
> { 'enum': 'MigrationParameter',
> @@ -481,7 +485,8 @@
> 'cpu-throttle-initial', 'cpu-throttle-increment',
> 'tls-creds', 'tls-hostname', 'max-bandwidth',
> 'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
> - 'x-multifd-channels', 'x-multifd-page-count' ] }
> + 'x-multifd-channels', 'x-multifd-page-count',
> + 'xbzrle-cache-size' ] }
>
> ##
> # @MigrateSetParameters:
> @@ -545,6 +550,9 @@
> # @x-multifd-page-count: Number of pages sent together to a thread
> # The default value is 16 (since 2.11)
> #
> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
> +# needs to be a multiple of the target page size
> +# (Since 2.11)
and power of 2
> # Since: 2.4
> ##
> # TODO either fuse back into MigrationParameters, or make
> @@ -562,7 +570,8 @@
> '*x-checkpoint-delay': 'int',
> '*block-incremental': 'bool',
> '*x-multifd-channels': 'int',
> - '*x-multifd-page-count': 'int' } }
> + '*x-multifd-page-count': 'int',
> + '*xbzrle-cache-size': 'size' } }
>
> ##
> # @migrate-set-parameters:
> @@ -641,6 +650,9 @@
> # @x-multifd-page-count: Number of pages sent together to a thread
> # The default value is 16 (since 2.11)
> #
> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
> +# needs to be a multiple of the target page size
> +# (Since 2.11)
and power of 2 (wow this text is repeated a lot)
> # Since: 2.4
> ##
> { 'struct': 'MigrationParameters',
> @@ -656,7 +668,8 @@
> '*x-checkpoint-delay': 'int',
> '*block-incremental': 'bool' ,
> '*x-multifd-channels': 'int',
> - '*x-multifd-page-count': 'int' } }
> + '*x-multifd-page-count': 'int',
> + '*xbzrle-cache-size': 'size' } }
>
> ##
> # @query-migrate-parameters:
> @@ -921,6 +934,8 @@
> #
> # Returns: nothing on success
> #
> +# Notes: This command is deprecated in favor of 'migrate-set-parameters'
> +#
> # Since: 1.2
> #
> # Example:
> @@ -939,6 +954,8 @@
> #
> # Returns: XBZRLE cache size in bytes
> #
> +# Notes: This command is deprecated in favor of 'query-migrate-parameters'
> +#
> # Since: 1.2
> #
> # Example:
> --
> 2.13.6
Dave
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 09/10] migration: Make xbzrle_cache_size a migration parameter
2017-10-12 12:04 ` Dr. David Alan Gilbert
@ 2017-10-18 8:50 ` Juan Quintela
0 siblings, 0 replies; 26+ messages in thread
From: Juan Quintela @ 2017-10-18 8:50 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: qemu-devel, lvivier, peterx, Daniel P. Berrange
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Right now it is a variable in MigrationState instead of a
>> MigrationParameter. The change allows to set it as the rest of the
>> Migration parameters, from the command line, with
>> query_migration_paramters, set_migrate_parameters, etc.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> static void migrate_params_apply(MigrateSetParameters *params)
>> @@ -939,6 +954,10 @@ static void migrate_params_apply(MigrateSetParameters *params)
>> if (params->has_x_multifd_page_count) {
>> s->parameters.x_multifd_page_count = params->x_multifd_page_count;
>> }
>> + if (params->has_xbzrle_cache_size) {
>> + s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
>> + xbzrle_cache_resize(params->xbzrle_cache_size, NULL);
>
> Not having the errp is a pain; you've just moved all the error
> checking into xbzrle_cache_resize, so I think we really need an error
> pointer and to do something with it (even if it's just a local report).
ok.
>> @@ -474,6 +474,10 @@
>> # @x-multifd-page-count: Number of pages sent together to a thread
>> # The default value is 16 (since 2.11)
>> #
>> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
>> +# needs to be a multiple of the target page size
>
> and power of 2
>
>> +# (Since 2.11)
>> +#
>> # Since: 2.4
>> ##
>> { 'enum': 'MigrationParameter',
>> @@ -481,7 +485,8 @@
>> 'cpu-throttle-initial', 'cpu-throttle-increment',
>> 'tls-creds', 'tls-hostname', 'max-bandwidth',
>> 'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
>> - 'x-multifd-channels', 'x-multifd-page-count' ] }
>> + 'x-multifd-channels', 'x-multifd-page-count',
>> + 'xbzrle-cache-size' ] }
>>
>> ##
>> # @MigrateSetParameters:
>> @@ -545,6 +550,9 @@
>> # @x-multifd-page-count: Number of pages sent together to a thread
>> # The default value is 16 (since 2.11)
>> #
>> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
>> +# needs to be a multiple of the target page size
>> +# (Since 2.11)
>
> and power of 2
>
>> # Since: 2.4
>> ##
>> # TODO either fuse back into MigrationParameters, or make
>> @@ -562,7 +570,8 @@
>> '*x-checkpoint-delay': 'int',
>> '*block-incremental': 'bool',
>> '*x-multifd-channels': 'int',
>> - '*x-multifd-page-count': 'int' } }
>> + '*x-multifd-page-count': 'int',
>> + '*xbzrle-cache-size': 'size' } }
>>
>> ##
>> # @migrate-set-parameters:
>> @@ -641,6 +650,9 @@
>> # @x-multifd-page-count: Number of pages sent together to a thread
>> # The default value is 16 (since 2.11)
>> #
>> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
>> +# needs to be a multiple of the target page size
>> +# (Since 2.11)
>
> and power of 2 (wow this text is repeated a lot)
We repeat each parameter three times, and the text another three times.
Could we just put a pointer telling that documentation is in a single
place and call it a day?
I know that we need to declare the name in three places for historical
reasons, but at least the help can be done in a single place, or is the
text taken automagically?
Later, Juan.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json
2017-10-10 18:15 [Qemu-devel] [PATCH 00/10] Make xbzrle_cache_size a migration parameter Juan Quintela
` (8 preceding siblings ...)
2017-10-10 18:15 ` [Qemu-devel] [PATCH 09/10] migration: Make xbzrle_cache_size a migration parameter Juan Quintela
@ 2017-10-10 18:15 ` Juan Quintela
2017-10-11 9:28 ` Daniel P. Berrange
9 siblings, 1 reply; 26+ messages in thread
From: Juan Quintela @ 2017-10-10 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx
We use int for everything (int64_t), and then we check that value is
between 0 and 255. Change it to the valid types.
For qmp, the only real change is that now max_bandwidth allows to use
the k/M/G suffixes.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
hmp.c | 22 +++++++++++-----------
migration/migration.c | 49 ++++++++++++++++++++-----------------------------
qapi/migration.json | 20 ++++++++++----------
3 files changed, 41 insertions(+), 50 deletions(-)
diff --git a/hmp.c b/hmp.c
index f73232399a..905dc93aef 100644
--- a/hmp.c
+++ b/hmp.c
@@ -293,23 +293,23 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
if (params) {
assert(params->has_compress_level);
- monitor_printf(mon, "%s: %" PRId64 "\n",
+ monitor_printf(mon, "%s: %u\n",
MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_LEVEL),
params->compress_level);
assert(params->has_compress_threads);
- monitor_printf(mon, "%s: %" PRId64 "\n",
+ monitor_printf(mon, "%s: %u\n",
MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_THREADS),
params->compress_threads);
assert(params->has_decompress_threads);
- monitor_printf(mon, "%s: %" PRId64 "\n",
+ monitor_printf(mon, "%s: %u\n",
MigrationParameter_str(MIGRATION_PARAMETER_DECOMPRESS_THREADS),
params->decompress_threads);
assert(params->has_cpu_throttle_initial);
- monitor_printf(mon, "%s: %" PRId64 "\n",
+ monitor_printf(mon, "%s: %u\n",
MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL),
params->cpu_throttle_initial);
assert(params->has_cpu_throttle_increment);
- monitor_printf(mon, "%s: %" PRId64 "\n",
+ monitor_printf(mon, "%s: %u\n",
MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT),
params->cpu_throttle_increment);
assert(params->has_tls_creds);
@@ -321,28 +321,28 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
params->tls_hostname);
assert(params->has_max_bandwidth);
- monitor_printf(mon, "%s: %" PRId64 " bytes/second\n",
+ monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
params->max_bandwidth);
assert(params->has_downtime_limit);
- monitor_printf(mon, "%s: %" PRId64 " milliseconds\n",
+ monitor_printf(mon, "%s: %" PRIu64 " milliseconds\n",
MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT),
params->downtime_limit);
assert(params->has_x_checkpoint_delay);
- monitor_printf(mon, "%s: %" PRId64 "\n",
+ monitor_printf(mon, "%s: %u\n",
MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY),
params->x_checkpoint_delay);
assert(params->has_block_incremental);
monitor_printf(mon, "%s: %s\n",
MigrationParameter_str(MIGRATION_PARAMETER_BLOCK_INCREMENTAL),
params->block_incremental ? "on" : "off");
- monitor_printf(mon, "%s: %" PRId64 "\n",
+ monitor_printf(mon, "%s: %u\n",
MigrationParameter_str(MIGRATION_PARAMETER_X_MULTIFD_CHANNELS),
params->x_multifd_channels);
- monitor_printf(mon, "%s: %" PRId64 "\n",
+ monitor_printf(mon, "%s: %u\n",
MigrationParameter_str(MIGRATION_PARAMETER_X_MULTIFD_PAGE_COUNT),
params->x_multifd_page_count);
- monitor_printf(mon, "%s: %" PRId64 "\n",
+ monitor_printf(mon, "%s: %" PRIu64 "\n",
MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
params->xbzrle_cache_size);
}
diff --git a/migration/migration.c b/migration/migration.c
index fcc0d64625..6d6dcc4e42 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -734,22 +734,20 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
static bool migrate_params_check(MigrationParameters *params, Error **errp)
{
if (params->has_compress_level &&
- (params->compress_level < 0 || params->compress_level > 9)) {
+ (params->compress_level > 9)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
"is invalid, it should be in the range of 0 to 9");
return false;
}
- if (params->has_compress_threads &&
- (params->compress_threads < 1 || params->compress_threads > 255)) {
+ if (params->has_compress_threads && (params->compress_threads < 1)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
"compress_threads",
"is invalid, it should be in the range of 1 to 255");
return false;
}
- if (params->has_decompress_threads &&
- (params->decompress_threads < 1 || params->decompress_threads > 255)) {
+ if (params->has_decompress_threads && (params->decompress_threads < 1)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
"decompress_threads",
"is invalid, it should be in the range of 1 to 255");
@@ -774,38 +772,31 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
return false;
}
- if (params->has_max_bandwidth &&
- (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
+ if (params->has_max_bandwidth && (params->max_bandwidth > SIZE_MAX)) {
error_setg(errp, "Parameter 'max_bandwidth' expects an integer in the"
" range of 0 to %zu bytes/second", SIZE_MAX);
return false;
}
if (params->has_downtime_limit &&
- (params->downtime_limit < 0 ||
- params->downtime_limit > MAX_MIGRATE_DOWNTIME)) {
+ (params->downtime_limit > MAX_MIGRATE_DOWNTIME)) {
error_setg(errp, "Parameter 'downtime_limit' expects an integer in "
"the range of 0 to %d milliseconds",
MAX_MIGRATE_DOWNTIME);
return false;
}
- if (params->has_x_checkpoint_delay && (params->x_checkpoint_delay < 0)) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- "x_checkpoint_delay",
- "is invalid, it should be positive");
- return false;
- }
- if (params->has_x_multifd_channels &&
- (params->x_multifd_channels < 1 || params->x_multifd_channels > 255)) {
+ /* x_checkpoint_delay is now always positive */
+
+ if (params->has_x_multifd_channels && (params->x_multifd_channels < 1)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
"multifd_channels",
"is invalid, it should be in the range of 1 to 255");
return false;
}
if (params->has_x_multifd_page_count &&
- (params->x_multifd_page_count < 1 ||
- params->x_multifd_page_count > 10000)) {
+ (params->x_multifd_page_count < 1 ||
+ params->x_multifd_page_count > 10000)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
"multifd_page_count",
"is invalid, it should be in the range of 1 to 10000");
@@ -2301,33 +2292,33 @@ static Property migration_properties[] = {
send_section_footer, true),
/* Migration parameters */
- DEFINE_PROP_INT64("x-compress-level", MigrationState,
+ DEFINE_PROP_UINT8("x-compress-level", MigrationState,
parameters.compress_level,
DEFAULT_MIGRATE_COMPRESS_LEVEL),
- DEFINE_PROP_INT64("x-compress-threads", MigrationState,
+ DEFINE_PROP_UINT8("x-compress-threads", MigrationState,
parameters.compress_threads,
DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT),
- DEFINE_PROP_INT64("x-decompress-threads", MigrationState,
+ DEFINE_PROP_UINT8("x-decompress-threads", MigrationState,
parameters.decompress_threads,
DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
- DEFINE_PROP_INT64("x-cpu-throttle-initial", MigrationState,
+ DEFINE_PROP_UINT8("x-cpu-throttle-initial", MigrationState,
parameters.cpu_throttle_initial,
DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL),
- DEFINE_PROP_INT64("x-cpu-throttle-increment", MigrationState,
+ DEFINE_PROP_UINT8("x-cpu-throttle-increment", MigrationState,
parameters.cpu_throttle_increment,
DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT),
- DEFINE_PROP_INT64("x-max-bandwidth", MigrationState,
+ DEFINE_PROP_SIZE("x-max-bandwidth", MigrationState,
parameters.max_bandwidth, MAX_THROTTLE),
- DEFINE_PROP_INT64("x-downtime-limit", MigrationState,
+ DEFINE_PROP_UINT64("x-downtime-limit", MigrationState,
parameters.downtime_limit,
DEFAULT_MIGRATE_SET_DOWNTIME),
- DEFINE_PROP_INT64("x-checkpoint-delay", MigrationState,
+ DEFINE_PROP_UINT32("x-checkpoint-delay", MigrationState,
parameters.x_checkpoint_delay,
DEFAULT_MIGRATE_X_CHECKPOINT_DELAY),
- DEFINE_PROP_INT64("x-multifd-channels", MigrationState,
+ DEFINE_PROP_UINT8("x-multifd-channels", MigrationState,
parameters.x_multifd_channels,
DEFAULT_MIGRATE_MULTIFD_CHANNELS),
- DEFINE_PROP_INT64("x-multifd-page-count", MigrationState,
+ DEFINE_PROP_UINT32("x-multifd-page-count", MigrationState,
parameters.x_multifd_page_count,
DEFAULT_MIGRATE_MULTIFD_PAGE_COUNT),
DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
diff --git a/qapi/migration.json b/qapi/migration.json
index 830b2e4480..81bd979912 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -656,19 +656,19 @@
# Since: 2.4
##
{ 'struct': 'MigrationParameters',
- 'data': { '*compress-level': 'int',
- '*compress-threads': 'int',
- '*decompress-threads': 'int',
- '*cpu-throttle-initial': 'int',
- '*cpu-throttle-increment': 'int',
+ 'data': { '*compress-level': 'uint8',
+ '*compress-threads': 'uint8',
+ '*decompress-threads': 'uint8',
+ '*cpu-throttle-initial': 'uint8',
+ '*cpu-throttle-increment': 'uint8',
'*tls-creds': 'str',
'*tls-hostname': 'str',
- '*max-bandwidth': 'int',
- '*downtime-limit': 'int',
- '*x-checkpoint-delay': 'int',
+ '*max-bandwidth': 'size',
+ '*downtime-limit': 'uint64',
+ '*x-checkpoint-delay': 'uint32',
'*block-incremental': 'bool' ,
- '*x-multifd-channels': 'int',
- '*x-multifd-page-count': 'int',
+ '*x-multifd-channels': 'uint8',
+ '*x-multifd-page-count': 'uint32',
'*xbzrle-cache-size': 'size' } }
##
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json
2017-10-10 18:15 ` [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json Juan Quintela
@ 2017-10-11 9:28 ` Daniel P. Berrange
2017-10-11 9:41 ` Juan Quintela
0 siblings, 1 reply; 26+ messages in thread
From: Daniel P. Berrange @ 2017-10-11 9:28 UTC (permalink / raw)
To: Juan Quintela, Markus Armbruster; +Cc: qemu-devel, lvivier, dgilbert, peterx
On Tue, Oct 10, 2017 at 08:15:42PM +0200, Juan Quintela wrote:
> We use int for everything (int64_t), and then we check that value is
> between 0 and 255. Change it to the valid types.
>
> For qmp, the only real change is that now max_bandwidth allows to use
> the k/M/G suffixes.
So on input apps previous would use:
"max-bandwidth": 1024
ie json numeric field, and would expect to see the same when reading
data back from QEMU.
With this change they could use a string field
"max-bandwidth": "1k"
As long as QEMU's JSON parser accepts both number & string values
for the 'size' type it is still backwards compatible if an app
continues to use 1024 instead of "1k"
On *output* though (ie 'info migrate-parameters') this is not
compatible for applications, unless QEMU *always* uses the
numeric format when generating values. ie it must always
report 1024, and never "1k", as apps won't be expecting a string
with suffix. I can't 100% tell whether this is the case or not,
so CC'ing Markus to confirm if changing "int" to "size" is
guaranteed back-compat in both directions
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> hmp.c | 22 +++++++++++-----------
> migration/migration.c | 49 ++++++++++++++++++++-----------------------------
> qapi/migration.json | 20 ++++++++++----------
> 3 files changed, 41 insertions(+), 50 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index f73232399a..905dc93aef 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -293,23 +293,23 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>
> if (params) {
> assert(params->has_compress_level);
> - monitor_printf(mon, "%s: %" PRId64 "\n",
> + monitor_printf(mon, "%s: %u\n",
> MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_LEVEL),
> params->compress_level);
> assert(params->has_compress_threads);
> - monitor_printf(mon, "%s: %" PRId64 "\n",
> + monitor_printf(mon, "%s: %u\n",
> MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_THREADS),
> params->compress_threads);
> assert(params->has_decompress_threads);
> - monitor_printf(mon, "%s: %" PRId64 "\n",
> + monitor_printf(mon, "%s: %u\n",
> MigrationParameter_str(MIGRATION_PARAMETER_DECOMPRESS_THREADS),
> params->decompress_threads);
> assert(params->has_cpu_throttle_initial);
> - monitor_printf(mon, "%s: %" PRId64 "\n",
> + monitor_printf(mon, "%s: %u\n",
> MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL),
> params->cpu_throttle_initial);
> assert(params->has_cpu_throttle_increment);
> - monitor_printf(mon, "%s: %" PRId64 "\n",
> + monitor_printf(mon, "%s: %u\n",
> MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT),
> params->cpu_throttle_increment);
> assert(params->has_tls_creds);
> @@ -321,28 +321,28 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
> MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
> params->tls_hostname);
> assert(params->has_max_bandwidth);
> - monitor_printf(mon, "%s: %" PRId64 " bytes/second\n",
> + monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
> MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
> params->max_bandwidth);
> assert(params->has_downtime_limit);
> - monitor_printf(mon, "%s: %" PRId64 " milliseconds\n",
> + monitor_printf(mon, "%s: %" PRIu64 " milliseconds\n",
> MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT),
> params->downtime_limit);
> assert(params->has_x_checkpoint_delay);
> - monitor_printf(mon, "%s: %" PRId64 "\n",
> + monitor_printf(mon, "%s: %u\n",
> MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY),
> params->x_checkpoint_delay);
> assert(params->has_block_incremental);
> monitor_printf(mon, "%s: %s\n",
> MigrationParameter_str(MIGRATION_PARAMETER_BLOCK_INCREMENTAL),
> params->block_incremental ? "on" : "off");
> - monitor_printf(mon, "%s: %" PRId64 "\n",
> + monitor_printf(mon, "%s: %u\n",
> MigrationParameter_str(MIGRATION_PARAMETER_X_MULTIFD_CHANNELS),
> params->x_multifd_channels);
> - monitor_printf(mon, "%s: %" PRId64 "\n",
> + monitor_printf(mon, "%s: %u\n",
> MigrationParameter_str(MIGRATION_PARAMETER_X_MULTIFD_PAGE_COUNT),
> params->x_multifd_page_count);
> - monitor_printf(mon, "%s: %" PRId64 "\n",
> + monitor_printf(mon, "%s: %" PRIu64 "\n",
> MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
> params->xbzrle_cache_size);
> }
> diff --git a/migration/migration.c b/migration/migration.c
> index fcc0d64625..6d6dcc4e42 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -734,22 +734,20 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
> static bool migrate_params_check(MigrationParameters *params, Error **errp)
> {
> if (params->has_compress_level &&
> - (params->compress_level < 0 || params->compress_level > 9)) {
> + (params->compress_level > 9)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
> "is invalid, it should be in the range of 0 to 9");
> return false;
> }
>
> - if (params->has_compress_threads &&
> - (params->compress_threads < 1 || params->compress_threads > 255)) {
> + if (params->has_compress_threads && (params->compress_threads < 1)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "compress_threads",
> "is invalid, it should be in the range of 1 to 255");
> return false;
> }
>
> - if (params->has_decompress_threads &&
> - (params->decompress_threads < 1 || params->decompress_threads > 255)) {
> + if (params->has_decompress_threads && (params->decompress_threads < 1)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "decompress_threads",
> "is invalid, it should be in the range of 1 to 255");
> @@ -774,38 +772,31 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
> return false;
> }
>
> - if (params->has_max_bandwidth &&
> - (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
> + if (params->has_max_bandwidth && (params->max_bandwidth > SIZE_MAX)) {
> error_setg(errp, "Parameter 'max_bandwidth' expects an integer in the"
> " range of 0 to %zu bytes/second", SIZE_MAX);
> return false;
> }
>
> if (params->has_downtime_limit &&
> - (params->downtime_limit < 0 ||
> - params->downtime_limit > MAX_MIGRATE_DOWNTIME)) {
> + (params->downtime_limit > MAX_MIGRATE_DOWNTIME)) {
> error_setg(errp, "Parameter 'downtime_limit' expects an integer in "
> "the range of 0 to %d milliseconds",
> MAX_MIGRATE_DOWNTIME);
> return false;
> }
>
> - if (params->has_x_checkpoint_delay && (params->x_checkpoint_delay < 0)) {
> - error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> - "x_checkpoint_delay",
> - "is invalid, it should be positive");
> - return false;
> - }
> - if (params->has_x_multifd_channels &&
> - (params->x_multifd_channels < 1 || params->x_multifd_channels > 255)) {
> + /* x_checkpoint_delay is now always positive */
> +
> + if (params->has_x_multifd_channels && (params->x_multifd_channels < 1)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "multifd_channels",
> "is invalid, it should be in the range of 1 to 255");
> return false;
> }
> if (params->has_x_multifd_page_count &&
> - (params->x_multifd_page_count < 1 ||
> - params->x_multifd_page_count > 10000)) {
> + (params->x_multifd_page_count < 1 ||
> + params->x_multifd_page_count > 10000)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "multifd_page_count",
> "is invalid, it should be in the range of 1 to 10000");
> @@ -2301,33 +2292,33 @@ static Property migration_properties[] = {
> send_section_footer, true),
>
> /* Migration parameters */
> - DEFINE_PROP_INT64("x-compress-level", MigrationState,
> + DEFINE_PROP_UINT8("x-compress-level", MigrationState,
> parameters.compress_level,
> DEFAULT_MIGRATE_COMPRESS_LEVEL),
> - DEFINE_PROP_INT64("x-compress-threads", MigrationState,
> + DEFINE_PROP_UINT8("x-compress-threads", MigrationState,
> parameters.compress_threads,
> DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT),
> - DEFINE_PROP_INT64("x-decompress-threads", MigrationState,
> + DEFINE_PROP_UINT8("x-decompress-threads", MigrationState,
> parameters.decompress_threads,
> DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
> - DEFINE_PROP_INT64("x-cpu-throttle-initial", MigrationState,
> + DEFINE_PROP_UINT8("x-cpu-throttle-initial", MigrationState,
> parameters.cpu_throttle_initial,
> DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL),
> - DEFINE_PROP_INT64("x-cpu-throttle-increment", MigrationState,
> + DEFINE_PROP_UINT8("x-cpu-throttle-increment", MigrationState,
> parameters.cpu_throttle_increment,
> DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT),
> - DEFINE_PROP_INT64("x-max-bandwidth", MigrationState,
> + DEFINE_PROP_SIZE("x-max-bandwidth", MigrationState,
> parameters.max_bandwidth, MAX_THROTTLE),
> - DEFINE_PROP_INT64("x-downtime-limit", MigrationState,
> + DEFINE_PROP_UINT64("x-downtime-limit", MigrationState,
> parameters.downtime_limit,
> DEFAULT_MIGRATE_SET_DOWNTIME),
> - DEFINE_PROP_INT64("x-checkpoint-delay", MigrationState,
> + DEFINE_PROP_UINT32("x-checkpoint-delay", MigrationState,
> parameters.x_checkpoint_delay,
> DEFAULT_MIGRATE_X_CHECKPOINT_DELAY),
> - DEFINE_PROP_INT64("x-multifd-channels", MigrationState,
> + DEFINE_PROP_UINT8("x-multifd-channels", MigrationState,
> parameters.x_multifd_channels,
> DEFAULT_MIGRATE_MULTIFD_CHANNELS),
> - DEFINE_PROP_INT64("x-multifd-page-count", MigrationState,
> + DEFINE_PROP_UINT32("x-multifd-page-count", MigrationState,
> parameters.x_multifd_page_count,
> DEFAULT_MIGRATE_MULTIFD_PAGE_COUNT),
> DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 830b2e4480..81bd979912 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -656,19 +656,19 @@
> # Since: 2.4
> ##
> { 'struct': 'MigrationParameters',
> - 'data': { '*compress-level': 'int',
> - '*compress-threads': 'int',
> - '*decompress-threads': 'int',
> - '*cpu-throttle-initial': 'int',
> - '*cpu-throttle-increment': 'int',
> + 'data': { '*compress-level': 'uint8',
> + '*compress-threads': 'uint8',
> + '*decompress-threads': 'uint8',
> + '*cpu-throttle-initial': 'uint8',
> + '*cpu-throttle-increment': 'uint8',
> '*tls-creds': 'str',
> '*tls-hostname': 'str',
> - '*max-bandwidth': 'int',
> - '*downtime-limit': 'int',
> - '*x-checkpoint-delay': 'int',
> + '*max-bandwidth': 'size',
> + '*downtime-limit': 'uint64',
> + '*x-checkpoint-delay': 'uint32',
> '*block-incremental': 'bool' ,
> - '*x-multifd-channels': 'int',
> - '*x-multifd-page-count': 'int',
> + '*x-multifd-channels': 'uint8',
> + '*x-multifd-page-count': 'uint32',
> '*xbzrle-cache-size': 'size' } }
>
> ##
> --
> 2.13.6
>
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json
2017-10-11 9:28 ` Daniel P. Berrange
@ 2017-10-11 9:41 ` Juan Quintela
2017-11-06 14:26 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Juan Quintela @ 2017-10-11 9:41 UTC (permalink / raw)
To: Daniel P. Berrange
Cc: Markus Armbruster, qemu-devel, lvivier, dgilbert, peterx
"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Tue, Oct 10, 2017 at 08:15:42PM +0200, Juan Quintela wrote:
>> We use int for everything (int64_t), and then we check that value is
>> between 0 and 255. Change it to the valid types.
>>
>> For qmp, the only real change is that now max_bandwidth allows to use
>> the k/M/G suffixes.
>
> So on input apps previous would use:
>
> "max-bandwidth": 1024
>
> ie json numeric field, and would expect to see the same when reading
> data back from QEMU.
>
> With this change they could use a string field
>
> "max-bandwidth": "1k"
Actually it is worse than that
if you set:
"max-bandwidth": 1024
it understand 1024M, and it outputs that big number
"max-bandwidth": $((1024*1024*1024))
(no, I don't know the value from memory)
And yes, for migrate_set_parameter, we introduced it on 2.10. We should
have done the right thing, but I didn't catch the error (Markus did,
but too late, release were already done)
> As long as QEMU's JSON parser accepts both number & string values
> for the 'size' type it is still backwards compatible if an app
> continues to use 1024 instead of "1k"
>
> On *output* though (ie 'info migrate-parameters') this is not
> compatible for applications, unless QEMU *always* uses the
> numeric format when generating values. ie it must always
> report 1024, and never "1k", as apps won't be expecting a string
> with suffix. I can't 100% tell whether this is the case or not,
> so CC'ing Markus to confirm if changing "int" to "size" is
> guaranteed back-compat in both directions
This is why I asked. My *understanding* was that my changes are NOP
if you use the old interface, but I don't claim to be an expert on QAPI.
(qemu) migrate_set_parameter 100
(qemu) info migrate_parameters
...
max-bandwidth: 104857600 bytes/second
...
(qemu) migrate_set_parameter max-bandwidth 1M
(qemu) info migrate_parameters
...
max-bandwidth: 1048576 bytes/second
...
(qemu)
This is the output with my changes applied, so I think that it works
correctly on your example.
Thanks, Juan.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json
2017-10-11 9:41 ` Juan Quintela
@ 2017-11-06 14:26 ` Markus Armbruster
2017-11-08 9:41 ` Juan Quintela
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2017-11-06 14:26 UTC (permalink / raw)
To: Juan Quintela; +Cc: Daniel P. Berrange, lvivier, peterx, dgilbert, qemu-devel
Juan Quintela <quintela@redhat.com> writes:
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
>> On Tue, Oct 10, 2017 at 08:15:42PM +0200, Juan Quintela wrote:
>>> We use int for everything (int64_t), and then we check that value is
>>> between 0 and 255. Change it to the valid types.
>>>
>>> For qmp, the only real change is that now max_bandwidth allows to use
>>> the k/M/G suffixes.
Are you sure?
QAPI type 'size' is integer in JSON. No suffixes. The QObject input
visitor does suffixes only when visiting v->keyval is true.
>> So on input apps previous would use:
>>
>> "max-bandwidth": 1024
>>
>> ie json numeric field, and would expect to see the same when reading
>> data back from QEMU.
>>
>> With this change they could use a string field
>>
>> "max-bandwidth": "1k"
>
> Actually it is worse than that
>
>
> if you set:
>
> "max-bandwidth": 1024
>
> it understand 1024M, and it outputs that big number
>
> "max-bandwidth": $((1024*1024*1024))
>
> (no, I don't know the value from memory)
>
> And yes, for migrate_set_parameter, we introduced it on 2.10. We should
> have done the right thing, but I didn't catch the error (Markus did,
> but too late, release were already done)
I suspect you're talking about *HMP*. hmp_migrate_set_parameter(), to
be precise:
case MIGRATION_PARAMETER_MAX_BANDWIDTH:
p->has_max_bandwidth = true;
/*
* Can't use visit_type_size() here, because it
* defaults to Bytes rather than Mebibytes.
*/
ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);
if (ret < 0 || valuebw > INT64_MAX
|| (size_t)valuebw != valuebw) {
error_setg(&err, "Invalid size %s", valuestr);
break;
}
p->max_bandwidth = valuebw;
break;
>> As long as QEMU's JSON parser accepts both number & string values
>> for the 'size' type it is still backwards compatible if an app
>> continues to use 1024 instead of "1k"
>>
>> On *output* though (ie 'info migrate-parameters') this is not
>> compatible for applications, unless QEMU *always* uses the
>> numeric format when generating values. ie it must always
>> report 1024, and never "1k", as apps won't be expecting a string
>> with suffix. I can't 100% tell whether this is the case or not,
>> so CC'ing Markus to confirm if changing "int" to "size" is
>> guaranteed back-compat in both directions
>
> This is why I asked. My *understanding* was that my changes are NOP
> if you use the old interface, but I don't claim to be an expert on QAPI.
>
> (qemu) migrate_set_parameter 100
> (qemu) info migrate_parameters
> ...
> max-bandwidth: 104857600 bytes/second
> ...
> (qemu) migrate_set_parameter max-bandwidth 1M
> (qemu) info migrate_parameters
> ...
> max-bandwidth: 1048576 bytes/second
> ...
> (qemu)
>
> This is the output with my changes applied, so I think that it works
> correctly on your example.
This is HMP. Not a stable interface. QMP is a stable interface, but it
should not be affected by this patch. Is your commit message
misleading?
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json
2017-11-06 14:26 ` Markus Armbruster
@ 2017-11-08 9:41 ` Juan Quintela
2017-11-08 10:25 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Juan Quintela @ 2017-11-08 9:41 UTC (permalink / raw)
To: Markus Armbruster
Cc: Daniel P. Berrange, lvivier, peterx, dgilbert, qemu-devel
Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> "Daniel P. Berrange" <berrange@redhat.com> wrote:
>>> On Tue, Oct 10, 2017 at 08:15:42PM +0200, Juan Quintela wrote:
>>>> We use int for everything (int64_t), and then we check that value is
>>>> between 0 and 255. Change it to the valid types.
>>>>
>>>> For qmp, the only real change is that now max_bandwidth allows to use
>>>> the k/M/G suffixes.
>
> Are you sure?
No. That is why I ask O:-)
> QAPI type 'size' is integer in JSON. No suffixes. The QObject input
> visitor does suffixes only when visiting v->keyval is true.
Aha. So patch can go in.
>> if you set:
>>
>> "max-bandwidth": 1024
>>
>> it understand 1024M, and it outputs that big number
>>
>> "max-bandwidth": $((1024*1024*1024))
>>
>> (no, I don't know the value from memory)
>>
>> And yes, for migrate_set_parameter, we introduced it on 2.10. We should
>> have done the right thing, but I didn't catch the error (Markus did,
>> but too late, release were already done)
>
> I suspect you're talking about *HMP*. hmp_migrate_set_parameter(), to
> be precise:
>
> case MIGRATION_PARAMETER_MAX_BANDWIDTH:
> p->has_max_bandwidth = true;
> /*
> * Can't use visit_type_size() here, because it
> * defaults to Bytes rather than Mebibytes.
> */
> ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);
> if (ret < 0 || valuebw > INT64_MAX
> || (size_t)valuebw != valuebw) {
> error_setg(&err, "Invalid size %s", valuestr);
> break;
> }
> p->max_bandwidth = valuebw;
> break;
Ok. Understood. Thanks.
>>> As long as QEMU's JSON parser accepts both number & string values
>>> for the 'size' type it is still backwards compatible if an app
>>> continues to use 1024 instead of "1k"
>>>
>>> On *output* though (ie 'info migrate-parameters') this is not
>>> compatible for applications, unless QEMU *always* uses the
>>> numeric format when generating values. ie it must always
>>> report 1024, and never "1k", as apps won't be expecting a string
>>> with suffix. I can't 100% tell whether this is the case or not,
>>> so CC'ing Markus to confirm if changing "int" to "size" is
>>> guaranteed back-compat in both directions
>>
>> This is why I asked. My *understanding* was that my changes are NOP
>> if you use the old interface, but I don't claim to be an expert on QAPI.
>>
>> (qemu) migrate_set_parameter 100
>> (qemu) info migrate_parameters
>> ...
>> max-bandwidth: 104857600 bytes/second
>> ...
>> (qemu) migrate_set_parameter max-bandwidth 1M
>> (qemu) info migrate_parameters
>> ...
>> max-bandwidth: 1048576 bytes/second
>> ...
>> (qemu)
>>
>> This is the output with my changes applied, so I think that it works
>> correctly on your example.
>
> This is HMP. Not a stable interface. QMP is a stable interface, but it
> should not be affected by this patch. Is your commit message
> misleading?
Yeap. The part about RFC was because I didn't really understood what
was happening here, and wanted "adult supervision".
My reading from your answer is that patch can go in, right?
Thanks, Juan.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json
2017-11-08 9:41 ` Juan Quintela
@ 2017-11-08 10:25 ` Markus Armbruster
0 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2017-11-08 10:25 UTC (permalink / raw)
To: Juan Quintela; +Cc: lvivier, dgilbert, peterx, qemu-devel
Juan Quintela <quintela@redhat.com> writes:
> Markus Armbruster <armbru@redhat.com> wrote:
>> Juan Quintela <quintela@redhat.com> writes:
>>
>>> "Daniel P. Berrange" <berrange@redhat.com> wrote:
>>>> On Tue, Oct 10, 2017 at 08:15:42PM +0200, Juan Quintela wrote:
>>>>> We use int for everything (int64_t), and then we check that value is
>>>>> between 0 and 255. Change it to the valid types.
>>>>>
>>>>> For qmp, the only real change is that now max_bandwidth allows to use
>>>>> the k/M/G suffixes.
>>
>> Are you sure?
>
> No. That is why I ask O:-)
Fair enough :)
>> QAPI type 'size' is integer in JSON. No suffixes. The QObject input
>> visitor does suffixes only when visiting v->keyval is true.
>
> Aha. So patch can go in.
>
>>> if you set:
>>>
>>> "max-bandwidth": 1024
>>>
>>> it understand 1024M, and it outputs that big number
>>>
>>> "max-bandwidth": $((1024*1024*1024))
>>>
>>> (no, I don't know the value from memory)
>>>
>>> And yes, for migrate_set_parameter, we introduced it on 2.10. We should
>>> have done the right thing, but I didn't catch the error (Markus did,
>>> but too late, release were already done)
>>
>> I suspect you're talking about *HMP*. hmp_migrate_set_parameter(), to
>> be precise:
>>
>> case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>> p->has_max_bandwidth = true;
>> /*
>> * Can't use visit_type_size() here, because it
>> * defaults to Bytes rather than Mebibytes.
>> */
>> ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);
>> if (ret < 0 || valuebw > INT64_MAX
>> || (size_t)valuebw != valuebw) {
>> error_setg(&err, "Invalid size %s", valuestr);
>> break;
>> }
>> p->max_bandwidth = valuebw;
>> break;
>
> Ok. Understood. Thanks.
>
>
>>>> As long as QEMU's JSON parser accepts both number & string values
>>>> for the 'size' type it is still backwards compatible if an app
>>>> continues to use 1024 instead of "1k"
>>>>
>>>> On *output* though (ie 'info migrate-parameters') this is not
>>>> compatible for applications, unless QEMU *always* uses the
>>>> numeric format when generating values. ie it must always
>>>> report 1024, and never "1k", as apps won't be expecting a string
>>>> with suffix. I can't 100% tell whether this is the case or not,
>>>> so CC'ing Markus to confirm if changing "int" to "size" is
>>>> guaranteed back-compat in both directions
>>>
>>> This is why I asked. My *understanding* was that my changes are NOP
>>> if you use the old interface, but I don't claim to be an expert on QAPI.
>>>
>>> (qemu) migrate_set_parameter 100
>>> (qemu) info migrate_parameters
>>> ...
>>> max-bandwidth: 104857600 bytes/second
>>> ...
>>> (qemu) migrate_set_parameter max-bandwidth 1M
>>> (qemu) info migrate_parameters
>>> ...
>>> max-bandwidth: 1048576 bytes/second
>>> ...
>>> (qemu)
>>>
>>> This is the output with my changes applied, so I think that it works
>>> correctly on your example.
>>
>> This is HMP. Not a stable interface. QMP is a stable interface, but it
>> should not be affected by this patch. Is your commit message
>> misleading?
>
> Yeap. The part about RFC was because I didn't really understood what
> was happening here, and wanted "adult supervision".
>
> My reading from your answer is that patch can go in, right?
I think so.
^ permalink raw reply [flat|nested] 26+ messages in thread