[Qemu-block] [PATCH 0/8] Fix several unbounded stack usage
Suggested by Paolo. Ths patchset fixes several of the warnings generated by "-Wstack-usage=10". There are about 20-30 unbound stack cases during my build, and this patch is only fixing several of them, those which are obvious and easy. For the rest, most of them need some knowledge on specific area (e.g., USB, net, block) to have a better assessment on the limitiation values, and are not covered in this patchset. One thing to mention about patch 4: I still cannot figure out why the function xhci_dma_write_u32s() cannot be inlined in short time... However, the current fix can at least keep the code behavior not changed while making it stack bounded. Please let me know if anyone knows. Thanks. Peter CC: Markus Armbruster CC: Kevin Wolf CC: "Michael S. Tsirkin" CC: Paolo Bonzini CC: Richard Henderson CC: Eduardo Habkost CC: Gerd Hoffmann CC: Juan Quintela CC: Amit Shah CC: Luiz Capitulino CC: qemu-block@nongnu.org Peter Xu (8): qdict: fix unbounded stack for qdict_array_entries block: fix unbounded stack for dump_qdict usb: fix unbounded stack for ohci_td_pkt usb: fix unbounded stack for xhci_dma_write_u32s usb: fix unbounded stack for inotify_watchfn usb: fix unbounded stack for usb_mtp_add_str migration: fix unbounded stack for source_return_path_thread hw/i386: fix unbounded stack for load_multiboot block/qapi.c | 5 - hw/i386/multiboot.c | 10 +- hw/usb/dev-mtp.c | 13 + hw/usb/hcd-ohci.c | 7 --- hw/usb/hcd-xhci.c | 12 migration/migration.c | 7 --- qobject/qdict.c | 15 +-- 7 files changed, 47 insertions(+), 22 deletions(-) -- 2.4.3
Re: [Qemu-block] [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict
On Tue, Mar 08, 2016 at 09:12:45AM +0100, Markus Armbruster wrote: > Peter Xu writes: > > const char *format = composite ? "%*s%s:\n" : "%*s%s: "; > > Unrelated to your patch: ugh! > > Printf formats should be literals whenever possible, to make it easy for > the compiler to warn you when you screw up. It's trivially possible > here! Instead of > >func_fprintf(f, format, indentation * 4, "", key); > > do > >func_fprintf(f, "%*s%s:%c", indentation * 4, "", key, > composite ? '\n', ' ');; Yes, I can do that too. Do you think I should split the patchset into several smaller ones possibly? So that I can use a 2/2 for this specific function, one for the printf() issue, one for the stack allocation issue. > > > -char key[strlen(entry->key) + 1]; > > +#define __KEY_LEN (256) > > +char key[__KEY_LEN]; > > int i; > > > > +assert(strlen(entry->key) + 1 <= __KEY_LEN); > > +#undef __KEY_LEN > > /* replace dashes with spaces in key (variable) names */ > > for (i = 0; entry->key[i]; i++) { > > key[i] = entry->key[i] == '-' ? ' ' : entry->key[i]; > > I'm afraid this isn't a good idea. It relies on the non-local argument > that nobody will ever put a key longer than 255 into a qdict that gets > dumped. That may even be the case, but you need to *prove* it, not just > assert it. The weakest acceptable proof might be assertions in every > place that put keys into a dict that might get dumped. I suspect that's > practical and maintainable only if there's a single place that does it. Yes. Actually I doubted whether I should do this change... since I am not experienced enough to estimate a value which will be 100% working all the time. Here I just assumed 256 is big enough for qdict keys, which indeed is lack of proof. > > If this was a good idea, I'd recommend to avoid the awkward macro: > >char key[256]; >int i; > >assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key)); Yes, possibly! I forgot ARRAY_SIZE() macro. Thanks to point out. > > There are several other ways to limit the stack usage: > > 1. Move the array from stack to heap. Fine unless it's on a hot path. >As far as I can tell, this dumping business is for HMP and qemu-io, >i.e. not hot. > > 2. Use a stack array for short strings, switch to the heap for large >strings. More complex, but may be worth it on a hot path where most >strings are short. > > 3. Instead of creating a new string with '-' replaced for printing, >print characters. Can be okay with buffered I/O, but obviously not >on a hot path. > > 4. Like 3., but print substrings not containing '-'. Thanks for all the suggestions and ideas. To avoid the limitation of 256 (and also consider this path is not hot path), I'd like to choose (1) if you would not mind, in a split patch maybe. Thanks. Peter
Re: [Qemu-block] [Qemu-devel] [PATCH] vmdk: Switch to heap arrays for vmdk_write_cid
On Tue, Mar 08, 2016 at 02:18:35PM +0800, Fam Zheng wrote: > It is only called once for each opened image, so we can do it the easy > way. > > Signed-off-by: Fam Zheng > --- > block/vmdk.c | 25 ++--- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/block/vmdk.c b/block/vmdk.c > index a8db5d9..1ec2452 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -274,36 +274,39 @@ static uint32_t vmdk_read_cid(BlockDriverState *bs, int > parent) > > static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid) If to use heap in write, do we need to do the same for e.g., vmdk_parent_open() and vmdk_read_cid(), to make all things at least aligned? > { > -char desc[DESC_SIZE], tmp_desc[DESC_SIZE]; > +char *desc, *tmp_desc; > char *p_name, *tmp_str; > BDRVVmdkState *s = bs->opaque; > -int ret; > +int ret = 0; > > +desc = g_malloc0(DESC_SIZE); > +tmp_desc = g_malloc0(DESC_SIZE); > ret = bdrv_pread(bs->file->bs, s->desc_offset, desc, DESC_SIZE); > if (ret < 0) { > -return ret; > +goto out; > } > > desc[DESC_SIZE - 1] = '\0'; > tmp_str = strstr(desc, "parentCID"); > if (tmp_str == NULL) { > -return -EINVAL; > +ret = -EINVAL; > +goto out; > } > > -pstrcpy(tmp_desc, sizeof(tmp_desc), tmp_str); > +pstrcpy(tmp_desc, DESC_SIZE, tmp_str); > p_name = strstr(desc, "CID"); > if (p_name != NULL) { > p_name += sizeof("CID"); > -snprintf(p_name, sizeof(desc) - (p_name - desc), "%" PRIx32 "\n", > cid); > -pstrcat(desc, sizeof(desc), tmp_desc); > +snprintf(p_name, DESC_SIZE - (p_name - desc), "%" PRIx32 "\n", cid); > +pstrcat(desc, DESC_SIZE, tmp_desc); > } > > ret = bdrv_pwrite_sync(bs->file->bs, s->desc_offset, desc, DESC_SIZE); > -if (ret < 0) { > -return ret; > -} > > -return 0; > +out: > +g_free(desc); > +g_free(tmp_desc); > +return ret; > } > > static int vmdk_is_cid_valid(BlockDriverState *bs) > -- > 2.4.3 > > Besides the above nit-pick: Reviewed-by: Peter Xu
[Qemu-block] [PATCH 2/8] block: fix unbounded stack for dump_qdict
Suggested-by: Paolo Bonzini CC: Markus Armbruster CC: Kevin Wolf CC: qemu-block@nongnu.org Signed-off-by: Peter Xu --- block/qapi.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/qapi.c b/block/qapi.c index db2d3fb..687e577 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -638,9 +638,12 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation, QType type = qobject_type(entry->value); bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); const char *format = composite ? "%*s%s:\n" : "%*s%s: "; -char key[strlen(entry->key) + 1]; +#define __KEY_LEN (256) +char key[__KEY_LEN]; int i; +assert(strlen(entry->key) + 1 <= __KEY_LEN); +#undef __KEY_LEN /* replace dashes with spaces in key (variable) names */ for (i = 0; entry->key[i]; i++) { key[i] = entry->key[i] == '-' ? ' ' : entry->key[i]; -- 2.4.3
Re: [Qemu-block] [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict
On Tue, Mar 08, 2016 at 02:47:31PM +0100, Markus Armbruster wrote: > Fam Zheng writes: > > Also I think the double underscore identifiers are considered reserved in C, > > no? > > Correct. C99 7.1.3 Reserved identifiers: All identifiers that begin > with an underscore and either an uppercase letter or another underscore > are always reserved for any use. > > [...] Fam, Markus, Thanks to point out. Will fix. Peter
Re: [Qemu-block] [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict
On Tue, Mar 08, 2016 at 01:17:03PM +0100, Paolo Bonzini wrote: > > > On 08/03/2016 09:12, Markus Armbruster wrote: > > I'm afraid this isn't a good idea. It relies on the non-local argument > > that nobody will ever put a key longer than 255 into a qdict that gets > > dumped. That may even be the case, but you need to *prove* it, not just > > assert it. The weakest acceptable proof might be assertions in every > > place that put keys into a dict that might get dumped. I suspect that's > > practical and maintainable only if there's a single place that does it. > > > > If this was a good idea, I'd recommend to avoid the awkward macro: > > > >char key[256]; > >int i; > > > >assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key)); > > > > There are several other ways to limit the stack usage: > > > > 1. Move the array from stack to heap. Fine unless it's on a hot path. > >As far as I can tell, this dumping business is for HMP and qemu-io, > >i.e. not hot. > > I think this is the best. You can just g_strdup, modify in place, print > and free. g_strdup() will bring one more loop? One to copy the strings, one for replacing "-" to " ". Though I will first need to replace g_malloc0() with g_malloc(), which seems more suitable here. :) Thanks! Peter
[Qemu-block] [PATCH 0/2] block/qapi: trivial fixes
One is to use literal printf format when possible. One is to fix an unbounded usage of stack. Peter Xu (2): block/qapi: make two printf() formats literal block/qapi: fix unbounded stack for dump_qdict block/qapi.c | 14 ++ 1 file changed, 6 insertions(+), 8 deletions(-) -- 2.4.3
[Qemu-block] [PATCH 1/2] block/qapi: make two printf() formats literal
Fix two places to use literal printf format when possible. Signed-off-by: Peter Xu --- block/qapi.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/block/qapi.c b/block/qapi.c index db2d3fb..c4c2115 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -619,9 +619,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation, for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) { QType type = qobject_type(entry->value); bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); -const char *format = composite ? "%*s[%i]:\n" : "%*s[%i]: "; - -func_fprintf(f, format, indentation * 4, "", i); +func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i, + composite ? '\n' : ' '); dump_qobject(func_fprintf, f, indentation + 1, entry->value); if (!composite) { func_fprintf(f, "\n"); @@ -637,7 +636,6 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation, for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) { QType type = qobject_type(entry->value); bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); -const char *format = composite ? "%*s%s:\n" : "%*s%s: "; char key[strlen(entry->key) + 1]; int i; @@ -646,8 +644,8 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation, key[i] = entry->key[i] == '-' ? ' ' : entry->key[i]; } key[i] = 0; - -func_fprintf(f, format, indentation * 4, "", key); +func_fprintf(f, "%*s%s:%c", indentation * 4, "", key, + composite ? '\n' : ' '); dump_qobject(func_fprintf, f, indentation + 1, entry->value); if (!composite) { func_fprintf(f, "\n"); -- 2.4.3
[Qemu-block] [PATCH 2/2] block/qapi: fix unbounded stack for dump_qdict
Using heap instead of stack for better safety. Signed-off-by: Peter Xu --- block/qapi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/qapi.c b/block/qapi.c index c4c2115..b798e35 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -636,9 +636,8 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation, for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) { QType type = qobject_type(entry->value); bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); -char key[strlen(entry->key) + 1]; +char *key = g_malloc(strlen(entry->key) + 1); int i; - /* replace dashes with spaces in key (variable) names */ for (i = 0; entry->key[i]; i++) { key[i] = entry->key[i] == '-' ? ' ' : entry->key[i]; @@ -650,6 +649,7 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation, if (!composite) { func_fprintf(f, "\n"); } +g_free(key); } } -- 2.4.3
Re: [Qemu-block] [PATCH 1/2] block/qapi: make two printf() formats literal
On Wed, Mar 09, 2016 at 03:14:03PM -0700, Eric Blake wrote: > > +func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i, > > + composite ? '\n' : ' '); > > [The nerd in me wants to point out that you could avoid the ternary by > writing '"\n "[composite]', but that's too ugly to use outside of IOCCC > submissions, and I wouldn't be surprised if it (rightfully) triggers > clang warnings] Do you mean something like: int i = 0; printf("%c", '"\n "[i]'); Is this a grammar btw? Peter
Re: [Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote: > In load_vmstate, mis->from_src_file is freed twice, the first free is by > qemu_fclose, the second is by migration_incoming_state_destroy and > it causes Illegal instruction exception. The fix is just to remove the > first free. > > This problem is found by qemu-iotests case 068 since commit > "660819b migration: shut src return path unconditionally". The error is: > 068 1s ... - output mismatch (see 068.out.bad) > --- tests/qemu-iotests/068.out2017-05-06 01:00:26.417270437 +0200 > +++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200 > @@ -6,6 +6,8 @@ > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) savevm 0 > (qemu) quit > +./common.config: line 107: 242472 Illegal instruction (core dumped) > ( if [ -n "${QEMU_NEED_PID}" ]; then > +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; > +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) > QEMU X.Y.Z monitor - type 'help' for more information > -(qemu) quit > -*** done > +(qemu) *** done > > Signed-off-by: QingFeng Hao > --- > migration/savevm.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/migration/savevm.c b/migration/savevm.c > index 9c320f59d0..853e14e34e 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp) > > aio_context_acquire(aio_context); > ret = qemu_loadvm_state(f); > -qemu_fclose(f); > aio_context_release(aio_context); > > migration_incoming_state_destroy(); Thanks QingFeng for the fix! Reviewed-by: Peter Xu Though here instead of removing the fclose(), not sure whether this is nicer: diff --git a/migration/savevm.c b/migration/savevm.c index 9c320f5..1feb519 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp) QEMUFile *f; int ret; AioContext *aio_context; -MigrationIncomingState *mis = migration_incoming_get_current(); if (!bdrv_all_can_snapshot(&bs)) { error_setg(errp, @@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp) } qemu_system_reset(SHUTDOWN_CAUSE_NONE); -mis->from_src_file = f; aio_context_acquire(aio_context); ret = qemu_loadvm_state(f); Since I see we setup from_src_file but we don't really use it. If we remove that line, we can also remove the migration_incoming_get_current() call altogether. Either way work for me. So my r-b stands always. :-) -- Peter Xu
Re: [Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
On Tue, Jun 06, 2017 at 11:38:05AM +0800, QingFeng Hao wrote: > > > 在 2017/6/6 11:03, Peter Xu 写道: > >On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote: > >>In load_vmstate, mis->from_src_file is freed twice, the first free is by > >>qemu_fclose, the second is by migration_incoming_state_destroy and > >>it causes Illegal instruction exception. The fix is just to remove the > >>first free. > >> > >>This problem is found by qemu-iotests case 068 since commit > >>"660819b migration: shut src return path unconditionally". The error is: > >>068 1s ... - output mismatch (see 068.out.bad) > >> --- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200 > >> +++ 068.out.bad2017-06-03 13:59:55.360274640 +0200 > >> @@ -6,6 +6,8 @@ > >> QEMU X.Y.Z monitor - type 'help' for more information > >> (qemu) savevm 0 > >> (qemu) quit > >> +./common.config: line 107: 242472 Illegal instruction (core > >> dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then > >> +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; > >> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) > >> QEMU X.Y.Z monitor - type 'help' for more information > >> -(qemu) quit > >> -*** done > >> +(qemu) *** done > >> > >>Signed-off-by: QingFeng Hao > >>--- > >> migration/savevm.c | 1 - > >> 1 file changed, 1 deletion(-) > >> > >>diff --git a/migration/savevm.c b/migration/savevm.c > >>index 9c320f59d0..853e14e34e 100644 > >>--- a/migration/savevm.c > >>+++ b/migration/savevm.c > >>@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp) > >> aio_context_acquire(aio_context); > >> ret = qemu_loadvm_state(f); > >>-qemu_fclose(f); > >> aio_context_release(aio_context); > >> migration_incoming_state_destroy(); > >Thanks QingFeng for the fix! > > > >Reviewed-by: Peter Xu > > > >Though here instead of removing the fclose(), not sure whether this is > >nicer: > > > >diff --git a/migration/savevm.c b/migration/savevm.c > >index 9c320f5..1feb519 100644 > >--- a/migration/savevm.c > >+++ b/migration/savevm.c > >@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp) > > QEMUFile *f; > > int ret; > > AioContext *aio_context; > >-MigrationIncomingState *mis = migration_incoming_get_current(); > > > > if (!bdrv_all_can_snapshot(&bs)) { > > error_setg(errp, > >@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp) > > } > > > > qemu_system_reset(SHUTDOWN_CAUSE_NONE); > >-mis->from_src_file = f; > > > > aio_context_acquire(aio_context); > > ret = qemu_loadvm_state(f); > Thanks Peter. I have a doubt on this change because I see there are several > places > referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and it > seems to > get byte from the file and it's called by qemu_loadvm_state. > Anyway, you remind me for the description that is "In load_snapshot" rather > than > "In load_vmstate". thanks Oh I didn't really notice that. :) It shouldn't affect imho since load snapshot won't trigger postcopy codes. But sure current fix is good enough for me at least. Thanks, -- Peter Xu
Re: [Qemu-block] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file
On Tue, Jun 06, 2017 at 06:42:18PM +0100, Dr. David Alan Gilbert wrote: > * Kevin Wolf (kw...@redhat.com) wrote: > > Am 06.06.2017 um 07:24 hat QingFeng Hao geschrieben: > > > In load_snapshot, mis->from_src_file is freed twice, the first free is by > > > qemu_fclose, the second is by migration_incoming_state_destroy and > > > it causes Illegal instruction exception. The fix is just to remove the > > > first free. > > > > > > This problem is found by qemu-iotests case 068 since commit > > > "660819b migration: shut src return path unconditionally". The error is: > > > 068 1s ... - output mismatch (see 068.out.bad) > > > --- tests/qemu-iotests/068.out2017-05-06 01:00:26.417270437 > > > +0200 > > > +++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200 > > > @@ -6,6 +6,8 @@ > > > QEMU X.Y.Z monitor - type 'help' for more information > > > (qemu) savevm 0 > > > (qemu) quit > > > +./common.config: line 107: 242472 Illegal instruction (core > > > dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then > > > +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; > > > +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) > > > QEMU X.Y.Z monitor - type 'help' for more information > > > -(qemu) quit > > > -*** done > > > +(qemu) *** done > > > > > > Signed-off-by: QingFeng Hao > > > Reviewed-by: Dr. David Alan Gilbert > > > Reviewed-by: Peter Xu > > > > Dave, as you only gave R-b rather than merging the patch, should this be > > merged through the block tree? > > I'm happy for it to go via block but also happy for it to go via > migration; Juan is mostly doing the migration set at the moment since > they're dominated by his cleanups. > > > > diff --git a/migration/savevm.c b/migration/savevm.c > > > index 9c320f59d0..853e14e34e 100644 > > > --- a/migration/savevm.c > > > +++ b/migration/savevm.c > > > @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp) > > > > > > aio_context_acquire(aio_context); > > > ret = qemu_loadvm_state(f); > > > -qemu_fclose(f); > > > aio_context_release(aio_context); > > > > > > migration_incoming_state_destroy(); > > > > Did we check other callers of migration_incoming_state_destroy()? > > > > For example, qmp_xen_load_devices_state() looks suspicious, too. > > Hmm, it looks suspicious in the opposite direction; it never sets > mis->from_src_file as was added by b4b076da into the load_snapshot path. Agree. Does qmp_xen_load_devices_state() needs to call migration_incoming_state_destroy() after all? Since the latter function only cleanups MigrationIncomingState and looks like the former xen code didn't really use it at all. > > > I can't tell for postcopy_ram_listen_thread() - commit 660819b didn't > > seem to remove a qemu_fclose() call there, but I can't see one left > > behind either. Was the file leaked before commit 660819b or am I > > missing something? > > I don't think there's a problem in the postcopy path, although hmm was > I missing a close before? > > Dave > > > > Kevin > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK -- Peter Xu
Re: [Qemu-block] [PATCH 1/2] block/qapi: make two printf() formats literal
On Mon, Mar 21, 2016 at 03:14:48PM -0600, Eric Blake wrote: > On 03/09/2016 06:46 PM, Peter Xu wrote: > > > > Is this a grammar btw? > > Yes, C has an ugly grammar, because [] is just syntactic sugar for > deferencing pointer addition with nicer operator precedence. Quoting > C99 6.5.2.1: > > "The definition of the subscript operator [] is that E1[E2] is identical > to (*((E1)+(E2))). Because of the conversion rules that apply to the > binary + operator, if E1 is an array object (equivalently, a pointer to > the initial element of an array object) and E2 is an integer, E1[E2] > designates the E2-th element of E1 (counting from zero)." > > And a string literal is just a fancy way of writing the address of an > array of characters (where the address is chosen by the compiler). > > Thus, it IS valid to dereference the addition of an integer offset with > the address implied by a string literal in order to obtain a character > within the string. And since the [] operator is commutative (even > though no one in their right mind commutes the operands), you can also > write the even-uglier: > > composite["\n "] > > But now we've gone far astray from the original patch review :) Interesting thing to know. Thanks. :) -- peterx
Re: [Qemu-block] raw iotest regressions in 2.12.0-rc0
On Wed, Mar 21, 2018 at 05:58:48PM -0400, John Snow wrote: > ./check -v -raw > Failures: 109 132 136 148 152 183 > > 3fd2457d18edf5736f713dfe1ada9c87a9badab1 is the first bad commit > commit 3fd2457d18edf5736f713dfe1ada9c87a9badab1 > Author: Peter Xu > Date: Fri Mar 9 17:00:03 2018 +0800 > > monitor: enable IO thread for (qmp & !mux) typed > > Start to use dedicate IO thread for QMP monitors that are not using > MUXed chardev. > > Reviewed-by: Fam Zheng > Reviewed-by: Stefan Hajnoczi > Signed-off-by: Peter Xu > Message-Id: <20180309090006.10018-21-pet...@redhat.com> > Signed-off-by: Eric Blake > > > The symptom appears to be extra "RESUME" events in the stream that > weren't expected by the original output for tests 109 and 183; the rest > are python and I didn't dig yet. > > ./check -v raw > Failures: 055 > Failed 5 of 5 tests > > 91ad45061af0fe44ac5dadb5bedaf4d7a08077c8 is the first bad commit > commit 91ad45061af0fe44ac5dadb5bedaf4d7a08077c8 > Author: Peter Xu > Date: Fri Mar 9 17:00:05 2018 +0800 > > tests: qmp-test: verify command batching > > OOB introduced DROP event for flow control. This should not affect old > QMP clients. Add a command batching check to make sure of it. > > Reviewed-by: Stefan Hajnoczi > Signed-off-by: Peter Xu > Message-Id: <20180309090006.10018-23-pet...@redhat.com> > Reviewed-by: Eric Blake > Signed-off-by: Eric Blake > > > > Maybe these are known, but I wanted to consolidate them for rc0 for > something easy to search for. There are others for qcow2 which I'll post > in a bit...! I'll have a look on this today and see whether this is the same problem as reported by Max. Sorry for that, and thanks for reporting! -- Peter Xu
Re: [Qemu-block] raw iotest regressions in 2.12.0-rc0
On Wed, Mar 21, 2018 at 05:58:48PM -0400, John Snow wrote: > ./check -v -raw > Failures: 109 132 136 148 152 183 > > 3fd2457d18edf5736f713dfe1ada9c87a9badab1 is the first bad commit > commit 3fd2457d18edf5736f713dfe1ada9c87a9badab1 > Author: Peter Xu > Date: Fri Mar 9 17:00:03 2018 +0800 > > monitor: enable IO thread for (qmp & !mux) typed > > Start to use dedicate IO thread for QMP monitors that are not using > MUXed chardev. > > Reviewed-by: Fam Zheng > Reviewed-by: Stefan Hajnoczi > Signed-off-by: Peter Xu > Message-Id: <20180309090006.10018-21-pet...@redhat.com> > Signed-off-by: Eric Blake > > > The symptom appears to be extra "RESUME" events in the stream that > weren't expected by the original output for tests 109 and 183; the rest > are python and I didn't dig yet. > > ./check -v raw > Failures: 055 > Failed 5 of 5 tests > > 91ad45061af0fe44ac5dadb5bedaf4d7a08077c8 is the first bad commit > commit 91ad45061af0fe44ac5dadb5bedaf4d7a08077c8 > Author: Peter Xu > Date: Fri Mar 9 17:00:05 2018 +0800 > > tests: qmp-test: verify command batching > > OOB introduced DROP event for flow control. This should not affect old > QMP clients. Add a command batching check to make sure of it. > > Reviewed-by: Stefan Hajnoczi > Signed-off-by: Peter Xu > Message-Id: <20180309090006.10018-23-pet...@redhat.com> > Reviewed-by: Eric Blake > Signed-off-by: Eric Blake > > > > Maybe these are known, but I wanted to consolidate them for rc0 for > something easy to search for. There are others for qcow2 which I'll post > in a bit...! > > > Thanks, > --js CCing Max, Fam. Now I think I know how to solve some of the tests already (109, 132, 148, 152, 183). While I am still working (or, not yet started to work) on some others (055, 136, 205). 205 is interesting - it won't fail every time, but randomly: 205 1s ... [failed, exit status 1] - output mismatch (see 205.out.bad) --- /home/peterx/git/qemu/tests/qemu-iotests/205.out2018-03-08 19:36:27.452220803 +0800 +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/205.out.bad 2018-03-22 21:16:52.727152006 +0800 @@ -1,5 +1,19 @@ -... +F.. +== +FAIL: test_connect_after_remove_default (__main__.TestNbdServerRemove) +-- +Traceback (most recent call last): + File "205", line 96, in test_connect_after_remove_default +self.do_test_connect_after_remove() + File "205", line 90, in do_test_connect_after_remove +self.assert_qmp(result, 'return', {}) + File "/home/peterx/git/qemu/tests/qemu-iotests/iotests.py", line 422, in assert_qmp +result = self.dictpath(d, path) + File "/home/peterx/git/qemu/tests/qemu-iotests/iotests.py", line 381, in dictpath +self.fail('failed path traversal for "%s" in "%s"' % (path, str(d))) +AssertionError: failed path traversal for "return" in "{u'error': {u'class': u'GenericError', u'desc': u"export 'exp' still in use"}}" + -- Ran 7 tests Not digged yet. For 136, it happens always, this is the error: 136 4s ... [failed, exit status 1] - output mismatch (see 136.out.bad) --- /home/peterx/git/qemu/tests/qemu-iotests/136.out2018-01-12 12:46:42.069915434 +0800 +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/136.out.bad 2018-03-22 21:16:13.981116000 +0800 @@ -1,5 +1,125 @@ -... +.EE.EE.EE.EE.EE +== +ERROR: test_read_only (__main__.BlockDeviceStatsTestAccountBoth) +-- +Traceback (most recent call last): + File "136", line 286, in test_read_only +self.do_test_stats(rd_size = i[0], rd_ops = i[1]) + File "136", line 278, in do_test_stats +self.check_values() + File "136", line 204, in check_values +self.assertLess(0, stats['idle_time_ns']) +KeyError: 'idle_time_ns' + +== +ERROR: test_write_only (__main__.BlockDeviceStatsTes
Re: [PATCH v2 8/9] hw/dma: Use dma_addr_t type definition when relevant
On Tue, Jan 04, 2022 at 09:54:30AM +0100, Philippe Mathieu-Daudé wrote: > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > index 462f79a1f60..c3c49176110 100644 > --- a/hw/nvme/ctrl.c > +++ b/hw/nvme/ctrl.c > @@ -1147,7 +1147,7 @@ static uint16_t nvme_tx(NvmeCtrl *n, NvmeSg *sg, > uint8_t *ptr, uint32_t len, > > if (sg->flags & NVME_SG_DMA) { > const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; > -uint64_t residual; > +dma_addr_t residual; > > if (dir == NVME_TX_DIRECTION_TO_DEVICE) { > residual = dma_buf_write(ptr, len, &sg->qsg, attrs); If there's a new version: Maybe also change the return value types of dma_buf_write|read() to dma_addr_t? It'll be changed anyway in the next patch, so not a big deal. The rest patches looks good to me. Thanks. -- Peter Xu
Re: [PATCH v3 00/10] hw/dma: Use dma_addr_t type definition when relevant
On Tue, Jan 11, 2022 at 07:42:59PM +0100, Philippe Mathieu-Daudé wrote: > Since v2: > - Split meson patch restricting fw_cfg (Richard) > - Reorder pci_dma_map() docstring (Peter, Richard) > - Move QEMUSGList in previous patch (David) > - Have dma_buf_read/dma_buf_write return dma_addr_t (Peter) > - Drop 'propagate MemTxResult' patch (David) > - Added R-b tags Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH v3] hw/dma: Let dma_buf_read() / dma_buf_write() propagate MemTxResult
On Mon, Jan 17, 2022 at 01:51:30PM +0100, Philippe Mathieu-Daudé wrote: > From: Philippe Mathieu-Daudé > > Since commit 292e13142d2, dma_buf_rw() returns a MemTxResult type. > Do not discard it, return it to the caller. Pass the previously > returned value (the QEMUSGList residual size, which was rarely used) > as an optional argument. > > With this new API, SCSIRequest::residual might now be accessed via > a pointer. Since the size_t type does not have the same size on > 32 and 64-bit host architectures, convert it to a uint64_t, which > is big enough to hold the residual size, and the type is constant > on both 32/64-bit hosts. > > Update the few dma_buf_read() / dma_buf_write() callers to the new > API. > > Reviewed-by: Klaus Jensen > Signed-off-by: Philippe Mathieu-Daudé > Signed-off-by: Philippe Mathieu-Daudé Acked-by: Peter Xu -- Peter Xu
Re: [PATCH v8 1/5] QIOChannel: Add flags on io_writev and introduce io_flush callback
On Tue, Feb 01, 2022 at 03:28:59AM -0300, Leonardo Bras wrote: > Add flags to io_writev and introduce io_flush as optional callback to > QIOChannelClass, allowing the implementation of zero copy writes by > subclasses. > > How to use them: > - Write data using qio_channel_writev*(...,QIO_CHANNEL_WRITE_FLAG_ZERO_COPY), > - Wait write completion with qio_channel_flush(). > > Notes: > As some zero copy write implementations work asynchronously, it's > recommended to keep the write buffer untouched until the return of > qio_channel_flush(), to avoid the risk of sending an updated buffer > instead of the buffer state during write. > > As io_flush callback is optional, if a subclass does not implement it, then: > - io_flush will return 0 without changing anything. > > Also, some functions like qio_channel_writev_full_all() were adapted to > receive a flag parameter. That allows shared code between zero copy and > non-zero copy writev, and also an easier implementation on new flags. > > Signed-off-by: Leonardo Bras With Dan's comment addressed on removing the redundant assertion: Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH v8 5/5] multifd: Implement zero copy write in multifd migration (multifd-zero-copy)
On Tue, Feb 01, 2022 at 03:29:03AM -0300, Leonardo Bras wrote: > -void multifd_send_sync_main(QEMUFile *f) > +int multifd_send_sync_main(QEMUFile *f) > { > int i; > +bool flush_zero_copy; > > if (!migrate_use_multifd()) { > -return; > +return 0; > } > if (multifd_send_state->pages->num) { > if (multifd_send_pages(f) < 0) { > error_report("%s: multifd_send_pages fail", __func__); > -return; > +return 0; I've not checked how it used to do if multifd_send_pages() failed, but.. should it returns -1 rather than 0 when there will be a return code? > } > } > + > +/* > + * When using zero-copy, it's necessary to flush after each iteration to > + * make sure pages from earlier iterations don't end up replacing newer > + * pages. > + */ > +flush_zero_copy = migrate_use_zero_copy_send(); > + > for (i = 0; i < migrate_multifd_channels(); i++) { > MultiFDSendParams *p = &multifd_send_state->params[i]; > > @@ -591,7 +600,7 @@ void multifd_send_sync_main(QEMUFile *f) > if (p->quit) { > error_report("%s: channel %d has already quit", __func__, i); > qemu_mutex_unlock(&p->mutex); > -return; > +return 0; Same question here. > } The rest looks good. Thanks, -- Peter Xu
Re: [PATCH v8 5/5] multifd: Implement zero copy write in multifd migration (multifd-zero-copy)
On Mon, Feb 07, 2022 at 11:49:38PM -0300, Leonardo Bras Soares Passos wrote: > Hello Peter, thanks for reviewing! > > On Mon, Feb 7, 2022 at 11:22 PM Peter Xu wrote: > > > > On Tue, Feb 01, 2022 at 03:29:03AM -0300, Leonardo Bras wrote: > > > -void multifd_send_sync_main(QEMUFile *f) > > > +int multifd_send_sync_main(QEMUFile *f) > > > { > > > int i; > > > +bool flush_zero_copy; > > > > > > if (!migrate_use_multifd()) { > > > -return; > > > +return 0; > > > } > > > if (multifd_send_state->pages->num) { > > > if (multifd_send_pages(f) < 0) { > > > error_report("%s: multifd_send_pages fail", __func__); > > > -return; > > > +return 0; > > > > I've not checked how it used to do if multifd_send_pages() failed, but.. > > should > > it returns -1 rather than 0 when there will be a return code? > > Yeah, that makes sense. > The point here is that I was trying not to modify much of the current > behavior. > > I mean, multifd_send_sync_main() would previously return void, so any > other errors would not matter to the caller of this function, which > will continue to run as if nothing happened. > > Now, if it fails with flush_zero_copy, the operation needs to be aborted. Right, so how I understand is we'll fail anyway, and this allows us to fail (probably) sooner. > > Maybe, I should make it different: > - In any error, return -1. > - Create/use a specific error code in the case of a failing > flush_zero_copy, so I can test the return value for it on the caller > function and return early. > > Or alternatively, the other errors could also return early, but since > this will change how the code currently works, I would probably need > another patch for that change. (so it can be easily reverted if > needed) Yeah, should work too to add a patch before this one. > > What do you think is better? I just don't see how it could continue if e.g. multifd_send_pages() failed. The other thing is returning zero looks weird itself when there's obviously an error. Normally we could allow that but better with a comment showing why. For this case it's more natural to me if we could just fail early. Juan? -- Peter Xu
Re: [Qemu-block] [PATCH] cutils: Move size_to_str() from "qemu-common.h" to "qemu/cutils.h"
On Tue, Sep 03, 2019 at 02:57:31PM -0400, John Snow wrote: [...] > Seems proper. It must be an oversight to begin with that we declared it > in qemu-common but defined it in cutils. Porbably true.. > > Reviewed-by: John Snow Reviewed-by: Peter Xu Thanks, -- Peter Xu
Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler
On Thu, Sep 13, 2018 at 10:00:43AM -0600, Alex Williamson wrote: > On Thu, 13 Sep 2018 12:04:34 +0200 > Paolo Bonzini wrote: > > > On 13/09/2018 11:11, Paolo Bonzini wrote: > > > On 13/09/2018 08:03, Fam Zheng wrote: > > >> On Wed, 09/12 14:42, Paolo Bonzini wrote: > > >>> On 12/09/2018 13:50, Fam Zheng wrote: > > >>>>> I think it's okay if it is invoked. The sequence is first you stop > > >>>>> the > > >>>>> vq, then you drain the BlockBackends, then you switch AioContext. All > > >>>>> that matters is the outcome when virtio_scsi_dataplane_stop returns. > > >>>> Yes, but together with vIOMMU, it also effectively leads to a > > >>>> virtio_error(), > > >>>> which is not clean. QEMU stderr when this call happens (with patch 1 > > >>>> but not > > >>>> this patch): > > >>>> > > >>>> 2018-09-12T11:48:10.193023Z qemu-system-x86_64: vtd_iommu_translate: > > >>>> detected translation failure (dev=02:00:00, iova=0x0) > > >>>> 2018-09-12T11:48:10.193044Z qemu-system-x86_64: New fault is not > > >>>> recorded due to compression of faults > > >>>> 2018-09-12T11:48:10.193061Z qemu-system-x86_64: virtio: zero sized > > >>>> buffers are not allowed > > >>> > > >>> But with iothread, virtio_scsi_dataplane_stop runs in another thread > > >>> than the iothread; in that case you still have a race where the iothread > > >>> can process the vq before aio_disable_external and print the error. > > >>> > > >>> IIUC the guest has cleared the IOMMU page tables _before_ clearing the > > >>> DRIVER_OK bit in the status field. Could this be a guest bug? > > >> > > >> I'm not sure if it is a bug or not. I think what happens is the device > > >> is left > > >> enabled by Seabios, and then reset by kernel. > > > > > > That makes sense, though I'm not sure why QEMU needs to process a > > > request long after SeaBIOS has left control to Linux. Maybe it's just > > > that the messages should not go on QEMU stderr, and rather trace-point > > > should be enough. > > > > Aha, it's not that QEMU needs to poll, it's just that polling mode is > > enabled, and it decides to do one last iteration. In general the virtio > > spec allows the hardware to poll whenever it wants, hence: > > > > 1) I'm not sure that translation failures should mark the device as > > broken---definitely not when doing polling, possibly not even in > > response to the guest "kicking" the virtqueue. Alex, does the PCI spec > > say anything about this? > > AFAIK the PCI spec doesn't define anything about the IOMMU or response > to translation failures. Depending on whether it's a read or write, > the device might see an unsupported request or not even be aware of the > error. It's really a platform RAS question whether to have any more > significant response, most don't, but at least one tends to consider > IOMMU faults to be a data integrity issue worth bring the system down. > We've struggled with handling ongoing DMA generating IOMMU faults > during kexec for a long time, so any sort of marking a device broken > for a fault should be thoroughly considered, especially when a device > could be assigned to a user who can trivially trigger a fault. > > > 2) translation faliures should definitely not print messages to stderr. > > Yep, easy DoS vector for a malicious guest, or malicious userspace > driver within the guest. Thanks, Note that it's using error_report_once() upstream so it'll only print once for the whole lifecycle of QEMU process, and it's still a tracepoint downstream so no error will be dumped by default. So AFAIU it's not a DoS target for either. I would consider it a good hint for strange bugs since AFAIU DMA error should never exist on well-behaved guests. However I'll be fine too to post a patch to make it an explicit tracepoint again if any of us would still like it to go away. Thanks, -- Peter Xu
Re: [PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus
On Sat, Oct 19, 2019 at 11:41:53AM +0800, Peter Xu wrote: > On Wed, Oct 16, 2019 at 11:40:01AM -0300, Eduardo Habkost wrote: > > On Wed, Oct 16, 2019 at 10:29:29AM +0800, Peter Xu wrote: > > > v2: > > > - use uint32_t rather than int64_t [Juan] > > > - one more patch (patch 4) to check dup SaveStateEntry [Dave] > > > - one more patch to define a macro (patch 1) to simplify patch 2 > > > > > > Please review, thanks. > > > > I wonder how hard it is to write a simple test case to reproduce > > the original bug. We can extend tests/migration-test.c or > > tests/acceptance/migration.py. If using -device with explicit > > apic-id, we probably don't even need to create >255 VCPUs. > > I can give it a shot next week. :) When trying this, I probably noticed a block layer issue: q35 seems to have problem on booting from a very small block device (like 512B, which is the image size that currently used for migration-test.c). For example, this cmdline can boot successfully into the test image: $qemu -M pc -m 200m -accel kvm -nographic \ -drive file=$image,id=drive0,index=0,format=raw \ -device ide-hd,drive=drive0 While this cannot: $qemu -M q35 -m 200m -accel kvm -nographic \ -drive file=$image,id=drive0,index=0,format=raw \ -device ide-hd,drive=drive0 With error (BIOS debug messages on): Booting from Hard Disk..invalid basic_access:143: a=0201 b= c=0001 d=0080 ds= es=07c0 ss=d980 si= di= bp= sp=fd8e cs=f000 ip=cb81 f=0202 invalid basic_access:144: a=0201 b= c=0001 d=0080 ds= es=07c0 ss=d980 si= di= bp= sp=fd8e cs=f000 ip=cb81 f=0202 . Boot failed: could not read the boot disenter handle_18: NULL k This corresponds to this SeaBIOS check error: static void noinline basic_access(struct bregs *regs, struct drive_s *drive_fl, u16 command) { ... // sanity check on cyl heads, sec if (cylinder >= nlc || head >= nlh || sector > nls) { warn_invalid(regs); disk_ret(regs, DISK_RET_EPARAM); return; } ... } And... below cmdline will work even for q35 (as suggested by Fam when we talked offline): $qemu -M q35 -m 200m -accel kvm -nographic \ -drive file=$image,id=drive0,index=0,format=raw \ -device ide-hd,drive=drive0,secs=1,cyls=1,heads=1 I think for migration test we can workaround like above, but I'm also curious whether this is a real bug somewhere because I don't see a reason for q35 to refuse to boot on a one-sector image. Thanks, -- Peter Xu
Re: [PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus
On Thu, Oct 24, 2019 at 01:49:11PM -0400, John Snow wrote: > > > On 10/23/19 4:17 AM, Kevin Wolf wrote: > > The important difference here is legacy IDE (which works) vs. AHCI > > (which doesn't work). If you add a -device ahci to the -M pc case, it > > starts failing, too. > > > > Not sure why AHCI fails, but I'll just CC John who is the lucky > > maintainer of this device. :-) > > Hm... It looks like SeaBIOS is identifying the drive correctly and > perfectly well, but we're failing at boot_disk(u8 bootdrv, int > checksig), about here: > > call16_int(0x13, &br); > > if (br.flags & F_CF) { > printf("Boot failed: could not read the boot disk\n\n"); > return; > } > > Looking at AHCI tracing (From the QEMU side), it looks like we set up > the drive correctly, and then never touch the port ever again -- I don't > see an attempted read on QEMU's end. > > I'll need to look through SeaBIOS source for hints, I'm not sure right > yet. If anyone is more familiar with the SeaBIOS boot code, maybe they > can give a pointer faster than I'll figure it out myself. Hi, John, I don't know seabios well, but I did have a pointer in my previous email on where it faulted. It seems to me that the issue is that SeaBIOS may have got incorrect secs/cyls/heads information (and explicitly setting secs=1,cyls=1,heads=1 on the block device fixes the issue). Thanks, -- Peter Xu
Re: [PATCH v1 2/3] io: Add zerocopy and errqueue
On Tue, Aug 31, 2021 at 01:57:33PM +0100, Daniel P. Berrangé wrote: > On Tue, Aug 31, 2021 at 08:02:38AM -0300, Leonardo Bras wrote: > > MSG_ZEROCOPY is a feature that enables copy avoidance in TCP/UDP socket > > send calls. It does so by avoiding copying user data into kernel buffers. > > > > To make it work, three steps are needed: > > 1 - A setsockopt() system call, enabling SO_ZEROCOPY > > 2 - Passing down the MSG_ZEROCOPY flag for each send*() syscall > > 3 - Process the socket's error queue, dealing with any error > > AFAICT, this is missing the single most critical aspect of MSG_ZEROCOPY. > > It is non-obvious, but setting the MSG_ZEROCOPY flag turns sendmsg() > from a synchronous call to an asynchronous call. > > It is forbidden to overwrite/reuse/free the buffer passed to sendmsg > until an asynchronous completion notification has been received from > the socket error queue. These notifications are not required to > arrive in-order, even for a TCP stream, because the kernel hangs on > to the buffer if a re-transmit is needed. > > https://www.kernel.org/doc/html/v5.4/networking/msg_zerocopy.html > > "Page pinning also changes system call semantics. It temporarily >shares the buffer between process and network stack. Unlike with >copying, the process cannot immediately overwrite the buffer >after system call return without possibly modifying the data in >flight. Kernel integrity is not affected, but a buggy program >can possibly corrupt its own data stream." > > AFAICT, the design added in this patch does not provide any way > to honour these requirements around buffer lifetime. > > I can't see how we can introduce MSG_ZEROCOPY in any seemless > way. The buffer lifetime requirements imply need for an API > design that is fundamentally different for asynchronous usage, > with a callback to notify when the write has finished/failed. Regarding buffer reuse - it indeed has a very deep implication on the buffer being available and it's not obvious at all. Just to mention that the initial user of this work will make sure all zero copy buffers will be guest pages only (as it's only used in multi-fd), so they should always be there during the process. I think asking for a complete design still makes sense. E.g., for precopy before we flush device states and completes the migration, we may want to at least have a final ack on all the zero-copies of guest pages to guarantee they are flushed. IOW, we need to make sure the last piece of migration stream lands after the guest pages so that the dest VM will always contain the latest page data when dest VM starts. So far I don't see how current code guaranteed that. In short, we may just want to at least having a way to make sure all zero copied buffers are finished using and they're sent after some function returns (e.g., qio_channel_flush()). That may require us to do some accounting on when we called sendmsg(MSG_ZEROCOPY), meanwhile we should need to read out the ee_data field within SO_EE_ORIGIN_ZEROCOPY msg when we do recvmsg() for the error queue and keep those information somewhere too. Some other side notes that reached my mind.. The qio_channel_writev_full() may not be suitable for async operations, as the name "full" implies synchronous to me. So maybe we can add a new helper for zero copy on the channel? We may also want a new QIOChannelFeature as QIO_CHANNEL_FEATURE_ZEROCOPY, then we fail qio_channel_writv_zerocopy() (or whatever name we come up with) if that bit is not set in qio channel features. Thanks, -- Peter Xu
Re: [PATCH v1 3/3] migration: multifd: Enable zerocopy
On Tue, Aug 31, 2021 at 02:16:42PM +0100, Daniel P. Berrangé wrote: > On Tue, Aug 31, 2021 at 08:02:39AM -0300, Leonardo Bras wrote: > > Call qio_channel_set_zerocopy(true) in the start of every multifd thread. > > > > Change the send_write() interface of multifd, allowing it to pass down > > flags for qio_channel_write*(). > > > > Pass down MSG_ZEROCOPY flag for sending memory pages, while keeping the > > other data being sent at the default copying approach. > > > > Signed-off-by: Leonardo Bras > > --- > > migration/multifd-zlib.c | 7 --- > > migration/multifd-zstd.c | 7 --- > > migration/multifd.c | 9 ++--- > > migration/multifd.h | 3 ++- > > 4 files changed, 16 insertions(+), 10 deletions(-) > > > @@ -675,7 +676,8 @@ static void *multifd_send_thread(void *opaque) > > } > > > > if (used) { > > -ret = multifd_send_state->ops->send_write(p, used, > > &local_err); > > +ret = multifd_send_state->ops->send_write(p, used, > > MSG_ZEROCOPY, > > + &local_err); > > I don't think it is valid to unconditionally enable this feature due to the > resource usage implications > > https://www.kernel.org/doc/html/v5.4/networking/msg_zerocopy.html > > "A zerocopy failure will return -1 with errno ENOBUFS. This happens >if the socket option was not set, the socket exceeds its optmem >limit or the user exceeds its ulimit on locked pages." > > The limit on locked pages is something that looks very likely to be > exceeded unless you happen to be running a QEMU config that already > implies locked memory (eg PCI assignment) Yes it would be great to be a migration capability in parallel to multifd. At initial phase if it's easy to be implemented on multi-fd only, we can add a dependency between the caps. In the future we can remove that dependency when the code is ready to go without multifd. Thanks, -- Peter Xu
Re: [PATCH v1 0/3] QIOChannel flags + multifd zerocopy
On Tue, Aug 31, 2021 at 08:02:36AM -0300, Leonardo Bras wrote: > Results: > So far, the resource usage of __sys_sendmsg() reduced 15 times, and the > overall migration took 13-18% less time, based in synthetic workload. Leo, Could you share some of the details of your tests? E.g., what's the configuration of your VM for testing? What's the migration time before/after the patchset applied? What is the network you're using? Thanks, -- Peter Xu
Re: [PATCH v1 3/3] migration: multifd: Enable zerocopy
On Wed, Sep 01, 2021 at 09:53:07AM +0100, Daniel P. Berrangé wrote: > On Tue, Aug 31, 2021 at 04:29:09PM -0400, Peter Xu wrote: > > On Tue, Aug 31, 2021 at 02:16:42PM +0100, Daniel P. Berrangé wrote: > > > On Tue, Aug 31, 2021 at 08:02:39AM -0300, Leonardo Bras wrote: > > > > Call qio_channel_set_zerocopy(true) in the start of every multifd > > > > thread. > > > > > > > > Change the send_write() interface of multifd, allowing it to pass down > > > > flags for qio_channel_write*(). > > > > > > > > Pass down MSG_ZEROCOPY flag for sending memory pages, while keeping the > > > > other data being sent at the default copying approach. > > > > > > > > Signed-off-by: Leonardo Bras > > > > --- > > > > migration/multifd-zlib.c | 7 --- > > > > migration/multifd-zstd.c | 7 --- > > > > migration/multifd.c | 9 ++--- > > > > migration/multifd.h | 3 ++- > > > > 4 files changed, 16 insertions(+), 10 deletions(-) > > > > > > > @@ -675,7 +676,8 @@ static void *multifd_send_thread(void *opaque) > > > > } > > > > > > > > if (used) { > > > > -ret = multifd_send_state->ops->send_write(p, used, > > > > &local_err); > > > > +ret = multifd_send_state->ops->send_write(p, used, > > > > MSG_ZEROCOPY, > > > > + &local_err); > > > > > > I don't think it is valid to unconditionally enable this feature due to > > > the > > > resource usage implications > > > > > > https://www.kernel.org/doc/html/v5.4/networking/msg_zerocopy.html > > > > > > "A zerocopy failure will return -1 with errno ENOBUFS. This happens > > >if the socket option was not set, the socket exceeds its optmem > > >limit or the user exceeds its ulimit on locked pages." > > > > > > The limit on locked pages is something that looks very likely to be > > > exceeded unless you happen to be running a QEMU config that already > > > implies locked memory (eg PCI assignment) > > > > Yes it would be great to be a migration capability in parallel to multifd. > > At > > initial phase if it's easy to be implemented on multi-fd only, we can add a > > dependency between the caps. In the future we can remove that dependency > > when > > the code is ready to go without multifd. Thanks, > > Also, I'm wondering how zerocopy support interacts with kernel support > for kTLS and multipath-TCP, both of which we want to be able to use > with migration. Copying Jason Wang for net implications between these features on kernel side and whether they can be enabled together (MSG_ZEROCOPY, mptcp, kTLS). >From the safe side we may want to only enable one of them until we prove they'll work together I guess.. Not a immediate concern as I don't really think any of them is really explicitly supported in qemu. KTLS may be implicitly included by a new gnutls, but we need to mark TLS and ZEROCOPY mutual exclusive anyway because at least the userspace TLS code of gnutls won't has a way to maintain the tls buffers used by zerocopy. So at least we need some knob to detect whether kTLS is enabled in gnutls. -- Peter Xu
Re: [PATCH v1 3/3] migration: multifd: Enable zerocopy
On Wed, Sep 01, 2021 at 04:44:30PM +0100, Daniel P. Berrangé wrote: > QEMU has mptcp support already: > > commit 8bd1078aebcec5eac196a83ef1a7e74be0ba67b7 > Author: Dr. David Alan Gilbert > Date: Wed Apr 21 12:28:34 2021 +0100 > > sockets: Support multipath TCP > > Multipath TCP allows combining multiple interfaces/routes into a single > socket, with very little work for the user/admin. > > It's enabled by 'mptcp' on most socket addresses: > >./qemu-system-x86_64 -nographic -incoming tcp:0:,mptcp Oops, I totally forgot about that, sorry! > > > KTLS may be implicitly included by a new gnutls, but we need to mark TLS and > > ZEROCOPY mutual exclusive anyway because at least the userspace TLS code of > > gnutls won't has a way to maintain the tls buffers used by zerocopy. So at > > least we need some knob to detect whether kTLS is enabled in gnutls. > > It isn't possible for gnutls to transparently enable KTLS, because > GNUTLS doesn't get to see the actual socket directly - it'll need > some work in QEMU to enable it. We know MPTCP and KTLS are currently > mutually exclusive as they both use the same kernel network hooks > framework. Then we may need to at least figure out whether zerocopy needs to mask out mptcp. -- Peter Xu
Re: [PATCH v1 2/3] io: Add zerocopy and errqueue
On Wed, Sep 01, 2021 at 09:50:56AM +0100, Daniel P. Berrangé wrote: > On Tue, Aug 31, 2021 at 04:27:04PM -0400, Peter Xu wrote: > > On Tue, Aug 31, 2021 at 01:57:33PM +0100, Daniel P. Berrangé wrote: > > > On Tue, Aug 31, 2021 at 08:02:38AM -0300, Leonardo Bras wrote: > > > > MSG_ZEROCOPY is a feature that enables copy avoidance in TCP/UDP socket > > > > send calls. It does so by avoiding copying user data into kernel > > > > buffers. > > > > > > > > To make it work, three steps are needed: > > > > 1 - A setsockopt() system call, enabling SO_ZEROCOPY > > > > 2 - Passing down the MSG_ZEROCOPY flag for each send*() syscall > > > > 3 - Process the socket's error queue, dealing with any error > > > > > > AFAICT, this is missing the single most critical aspect of MSG_ZEROCOPY. > > > > > > It is non-obvious, but setting the MSG_ZEROCOPY flag turns sendmsg() > > > from a synchronous call to an asynchronous call. > > > > > > It is forbidden to overwrite/reuse/free the buffer passed to sendmsg > > > until an asynchronous completion notification has been received from > > > the socket error queue. These notifications are not required to > > > arrive in-order, even for a TCP stream, because the kernel hangs on > > > to the buffer if a re-transmit is needed. > > > > > > https://www.kernel.org/doc/html/v5.4/networking/msg_zerocopy.html > > > > > > "Page pinning also changes system call semantics. It temporarily > > >shares the buffer between process and network stack. Unlike with > > >copying, the process cannot immediately overwrite the buffer > > >after system call return without possibly modifying the data in > > >flight. Kernel integrity is not affected, but a buggy program > > >can possibly corrupt its own data stream." > > > > > > AFAICT, the design added in this patch does not provide any way > > > to honour these requirements around buffer lifetime. > > > > > > I can't see how we can introduce MSG_ZEROCOPY in any seemless > > > way. The buffer lifetime requirements imply need for an API > > > design that is fundamentally different for asynchronous usage, > > > with a callback to notify when the write has finished/failed. > > > > Regarding buffer reuse - it indeed has a very deep implication on the buffer > > being available and it's not obvious at all. Just to mention that the > > initial > > user of this work will make sure all zero copy buffers will be guest pages > > only > > (as it's only used in multi-fd), so they should always be there during the > > process. > > That is not the case when migration is using TLS, because the buffers > transmitted are the ciphertext buffer, not the plaintext guest page. Agreed. > > > In short, we may just want to at least having a way to make sure all zero > > copied buffers are finished using and they're sent after some function > > returns > > (e.g., qio_channel_flush()). That may require us to do some accounting on > > when > > we called sendmsg(MSG_ZEROCOPY), meanwhile we should need to read out the > > ee_data field within SO_EE_ORIGIN_ZEROCOPY msg when we do recvmsg() for the > > error queue and keep those information somewhere too. > > > > Some other side notes that reached my mind.. > > > > The qio_channel_writev_full() may not be suitable for async operations, as > > the > > name "full" implies synchronous to me. So maybe we can add a new helper for > > zero copy on the channel? > > All the APIs that exist today are fundamentally only suitable for sync > operations. Supporting async correctly will definitely a brand new APIs > separate from what exists today. Yes. What I wanted to say is maybe we can still keep the io_writev() interface untouched, but just add a new interface at qio_channel_writev_full() level. IOW, we could comment on io_writev() that it can be either sync or async now, just like sendmsg() has that implication too with different flag passed in. When calling io_writev() with different upper helpers, QIO channel could identify what flag to pass over to io_writev(). -- Peter Xu
Re: [PATCH v1 2/3] io: Add zerocopy and errqueue
On Thu, Sep 02, 2021 at 03:59:25AM -0300, Leonardo Bras Soares Passos wrote: > I also suggested something like that, but I thought it could be good if we > could > fall back to io_writev() if we didn't have the zerocopy feature (or > the async feature). > > What do you think? That fallback looks safe and ok, I'm just not sure whether it'll be of great help. E.g. if we provide an QIO api that allows both sync write and zero-copy write (then we do the fallback when necessary), it means the buffer implication applies too to this api, so it's easier to me to just detect the zero copy capability and use one alternative. Thanks, -- Peter Xu
Re: [PATCH v1 2/3] io: Add zerocopy and errqueue
On Tue, Sep 07, 2021 at 12:06:15PM +0100, Dr. David Alan Gilbert wrote: > > > What if we do the 'flush()' before we start post-copy, instead of after > > > each > > > iteration? would that be enough? > > > > Possibly, yes. This really need David G's input since he understands > > the code in way more detail than me. > > Hmm I'm not entirely sure why we have the sync after each iteration; We don't have that yet, do we? > the case I can think of is if we're doing async sending, we could have > two versions of the same page in flight (one from each iteration) - > you'd want those to get there in the right order. >From MSG_ZEROCOPY document: For protocols that acknowledge data in-order, like TCP, each notification can be squashed into the previous one, so that no more than one notification is outstanding at any one point. Ordered delivery is the common case, but not guaranteed. Notifications may arrive out of order on retransmission and socket teardown. So no matter whether we have a flush() per iteration before, it seems we may want one when zero copy enabled? Thanks, -- Peter Xu
Re: [PATCH v1 3/3] migration: multifd: Enable zerocopy
On Thu, Sep 02, 2021 at 04:22:55AM -0300, Leonardo Bras Soares Passos wrote: > > I don't think it is valid to unconditionally enable this feature due to the > > resource usage implications > > > > https://www.kernel.org/doc/html/v5.4/networking/msg_zerocopy.html > > > > "A zerocopy failure will return -1 with errno ENOBUFS. This happens > >if the socket option was not set, the socket exceeds its optmem > >limit or the user exceeds its ulimit on locked pages." > > You are correct, I unfortunately missed this part in the docs :( > > > The limit on locked pages is something that looks very likely to be > > exceeded unless you happen to be running a QEMU config that already > > implies locked memory (eg PCI assignment) > > Do you mean the limit an user has on locking memory? > > If so, that makes sense. I remember I needed to set the upper limit of locked > memory for the user before using it, or adding a capability to qemu before. So I'm a bit confused on why MSG_ZEROCOPY requires checking RLIMIT_MEMLOCK. The thing is IIUC that's accounting for pinned pages only with either mlock() (FOLL_MLOCK) or vfio (FOLL_PIN). I don't really think MSG_ZEROCOPY is doing that at all... I'm looking at __zerocopy_sg_from_iter() -> iov_iter_get_pages(). Say, I think there're plenty of iov_iter_get_pages() callers and normal GUPs, I think most of them do no need such accountings. -- Peter Xu
Re: [PATCH v1 3/3] migration: multifd: Enable zerocopy
On Wed, Sep 08, 2021 at 10:59:57AM +0800, Jason Wang wrote: > On Wed, Sep 8, 2021 at 2:32 AM Peter Xu wrote: > > > > On Thu, Sep 02, 2021 at 04:22:55AM -0300, Leonardo Bras Soares Passos wrote: > > > > I don't think it is valid to unconditionally enable this feature due to > > > > the > > > > resource usage implications > > > > > > > > https://www.kernel.org/doc/html/v5.4/networking/msg_zerocopy.html > > > > > > > > "A zerocopy failure will return -1 with errno ENOBUFS. This happens > > > >if the socket option was not set, the socket exceeds its optmem > > > >limit or the user exceeds its ulimit on locked pages." > > > > > > You are correct, I unfortunately missed this part in the docs :( > > > > > > > The limit on locked pages is something that looks very likely to be > > > > exceeded unless you happen to be running a QEMU config that already > > > > implies locked memory (eg PCI assignment) > > > > > > Do you mean the limit an user has on locking memory? > > > > > > If so, that makes sense. I remember I needed to set the upper limit of > > > locked > > > memory for the user before using it, or adding a capability to qemu > > > before. > > > > So I'm a bit confused on why MSG_ZEROCOPY requires checking RLIMIT_MEMLOCK. > > > > The thing is IIUC that's accounting for pinned pages only with either > > mlock() > > (FOLL_MLOCK) or vfio (FOLL_PIN). > > > > I don't really think MSG_ZEROCOPY is doing that at all... I'm looking at > > __zerocopy_sg_from_iter() -> iov_iter_get_pages(). > > It happens probably here: > > E.g > > __ip_append_data() > msg_zerocopy_realloc() > mm_account_pinned_pages() Right. :) But my previous question is more about the reason behind it - I thought that's a common GUP and it shouldn't rely on locked_vm because it should still be temporary GUP rather than a long time GUP, IMHO that's how we use locked_vm (we don't account for temp GUPs but only longterm ones). IOW, I'm wondering whether all the rest of iov_iter_get_pages() callers should check locked_vm too, and AFAIU they're not doing that right now.. Thanks, -- Peter Xu
Re: [PATCH v1 3/3] migration: multifd: Enable zerocopy
On Wed, Sep 08, 2021 at 09:19:27AM +0100, Dr. David Alan Gilbert wrote: > * Jason Wang (jasow...@redhat.com) wrote: > > On Wed, Sep 8, 2021 at 2:32 AM Peter Xu wrote: > > > > > > On Thu, Sep 02, 2021 at 04:22:55AM -0300, Leonardo Bras Soares Passos > > > wrote: > > > > > I don't think it is valid to unconditionally enable this feature due > > > > > to the > > > > > resource usage implications > > > > > > > > > > https://www.kernel.org/doc/html/v5.4/networking/msg_zerocopy.html > > > > > > > > > > "A zerocopy failure will return -1 with errno ENOBUFS. This happens > > > > >if the socket option was not set, the socket exceeds its optmem > > > > >limit or the user exceeds its ulimit on locked pages." > > > > > > > > You are correct, I unfortunately missed this part in the docs :( > > > > > > > > > The limit on locked pages is something that looks very likely to be > > > > > exceeded unless you happen to be running a QEMU config that already > > > > > implies locked memory (eg PCI assignment) > > > > > > > > Do you mean the limit an user has on locking memory? > > > > > > > > If so, that makes sense. I remember I needed to set the upper limit of > > > > locked > > > > memory for the user before using it, or adding a capability to qemu > > > > before. > > > > > > So I'm a bit confused on why MSG_ZEROCOPY requires checking > > > RLIMIT_MEMLOCK. > > > > > > The thing is IIUC that's accounting for pinned pages only with either > > > mlock() > > > (FOLL_MLOCK) or vfio (FOLL_PIN). > > > > > > I don't really think MSG_ZEROCOPY is doing that at all... I'm looking at > > > __zerocopy_sg_from_iter() -> iov_iter_get_pages(). > > > > It happens probably here: > > > > E.g > > > > __ip_append_data() > > msg_zerocopy_realloc() > > mm_account_pinned_pages() > > When do they get uncounted? i.e. is it just the data that's in flight > that's marked as pinned? I think so - there's __msg_zerocopy_callback() -> mm_unaccount_pinned_pages() correspondingly. Thanks, -- Peter Xu
Re: [PATCH v1 2/3] io: Add zerocopy and errqueue
On Wed, Sep 08, 2021 at 09:30:58AM +0100, Dr. David Alan Gilbert wrote: > * Peter Xu (pet...@redhat.com) wrote: > > On Tue, Sep 07, 2021 at 12:06:15PM +0100, Dr. David Alan Gilbert wrote: > > > > > What if we do the 'flush()' before we start post-copy, instead of > > > > > after each > > > > > iteration? would that be enough? > > > > > > > > Possibly, yes. This really need David G's input since he understands > > > > the code in way more detail than me. > > > > > > Hmm I'm not entirely sure why we have the sync after each iteration; > > > > We don't have that yet, do we? > > I think multifd does; I think it calls multifd_send_sync_main sometimes, > which I *think* corresponds to iterations. Oh.. Then we may need to: (1) Make that sync work for zero-copy too; say, semaphores may not be enough with it - we need to make sure all async buffers are consumed by checking the error queue of the sockets, (2) When we make zero-copy work without multi-fd, we may want some similar sync on the main stream too Thanks, > > Dave > > > > the case I can think of is if we're doing async sending, we could have > > > two versions of the same page in flight (one from each iteration) - > > > you'd want those to get there in the right order. > > > > From MSG_ZEROCOPY document: > > > > For protocols that acknowledge data in-order, like TCP, each > > notification can be squashed into the previous one, so that no more > > than one notification is outstanding at any one point. > > > > Ordered delivery is the common case, but not guaranteed. > > Notifications > > may arrive out of order on retransmission and socket teardown. > > > > So no matter whether we have a flush() per iteration before, it seems we may > > want one when zero copy enabled? > > > > Thanks, > > > > -- > > Peter Xu > > > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > -- Peter Xu
Re: [PATCH v1 2/3] io: Add zerocopy and errqueue
On Wed, Sep 08, 2021 at 05:13:40PM -0300, Leonardo Bras Soares Passos wrote: > On Tue, Sep 7, 2021 at 1:44 PM Peter Xu wrote: > > > > On Thu, Sep 02, 2021 at 03:59:25AM -0300, Leonardo Bras Soares Passos wrote: > > > I also suggested something like that, but I thought it could be good if > > > we could > > > fall back to io_writev() if we didn't have the zerocopy feature (or > > > the async feature). > > > > > > What do you think? > > > > That fallback looks safe and ok, I'm just not sure whether it'll be of great > > help. E.g. if we provide an QIO api that allows both sync write and > > zero-copy > > write (then we do the fallback when necessary), it means the buffer > > implication > > applies too to this api, so it's easier to me to just detect the zero copy > > capability and use one alternative. Thanks, > > > > -- > > Peter Xu > > > > I was thinking this way (async method with fallback) we would allow code using > the QIO api to just try to use the io_async_writev() whenever the code > seems fit, and > let the QIO channel layer to decide when it can be used > (implementation provides, > features available), and just fallback to io_writev() when it can't be used. Sure, I think it depends on whether the whole sync/async interface will be the same, e.g., when async api needs some callback registered then the interface won't be the same, and the caller will be aware of that anyways. Otherwise it looks fine indeed. Thanks, -- Peter Xu
Re: [PATCH v1 2/3] io: Add zerocopy and errqueue
On Wed, Sep 08, 2021 at 05:25:50PM -0300, Leonardo Bras Soares Passos wrote: > On Tue, Sep 7, 2021 at 8:06 AM Dr. David Alan Gilbert > wrote: > > > Possibly, yes. This really need David G's input since he understands > > > the code in way more detail than me. > > > > Hmm I'm not entirely sure why we have the sync after each iteration; > > the case I can think of is if we're doing async sending, we could have > > two versions of the same page in flight (one from each iteration) - > > you'd want those to get there in the right order. > > > > Dave > > Well, that's the thing: as we don't copy the buffer in MSG_ZEROCOPY, we will > in > fact have the same page in flight twice, instead of two versions, > given the buffer is > sent as it is during transmission. That's an interesting point, which looks even valid... :) There can still be two versions depending on when the page is read and feed to the NICs as the page can be changing during the window, but as long as the latter sent page always lands later than the former page on dest node then it looks ok. -- Peter Xu
Re: [PATCH v1 2/3] io: Add zerocopy and errqueue
On Wed, Sep 08, 2021 at 10:57:06PM +0100, Daniel P. Berrangé wrote: > We think we're probably ok with migration as we are going to rely on the > face that we eventually pause the guest to stop page changes during the > final switchover. None the less I really strongly dislike the idea of > not honouring the kernel API contract, despite the potential performance > benefits it brings. Yes understandable, and it does looks tricky. But it's guest page and it's just by nature how it works to me (sending page happening in parallel with page being modified). I think the MSG_ZEROCOPY doc page mentioned that and it's userspace program's own choice if that happens. So even if it's not by design and not suggested, it seems not forbidden either. We can wr-protect the page (using things like userfaultfd-wp) during sending and unprotect them when it's done with a qio callback, that'll guarantee the buffer not changing during sending, however we gain nothing besides the "api cleaness" then.. Thanks, -- Peter Xu
Re: [PATCH v1 2/3] io: Add zerocopy and errqueue
On Thu, Sep 09, 2021 at 01:58:39AM -0300, Leonardo Bras Soares Passos wrote: > FWIW, what I had in mind for a (theoretical) migration setup with > io_async_writev() + io_async_flush(): One trivial concern is it's not strictly just "async" because "async" can happen on any nonblocking fd; here it's more "zerocopy" specific. But as long as Dan is okay with it I'm fine too to start with "async", as long as we do proper documentation on the requirement of lifecycle of the buffers. > - For guest RAM we can decide not to rw_lock memory on zerocopy, > because there is no need, > - For headers, we can decide to not use async (use io_writev() instead), > - flush() can happen each iteration of migration, or at each N > seconds, or at the end. I think we should only need the flush at the end of precopy, and that should also cover when switching to postcopy. We could merge this flush() into the existing per-iteration sync of multi-fd, but it's not strictly necessary, imho. We can see which is easier. The rest looks good to me. Thanks, -- Peter Xu
Re: [Qemu-block] [Qemu-devel] [PATCH v2 04/11] monitor: Create MonitorHMP with readline state
On Wed, Jun 12, 2019 at 11:07:01AM +0200, Markus Armbruster wrote: [...] > > +struct MonitorHMP { > > +Monitor common; > > +/* > > + * State used only in the thread "owning" the monitor. > > + * If @use_io_thread, this is @mon_iothread. > > + * Else, it's the main thread. > > + * These members can be safely accessed without locks. > > + */ > > +ReadLineState *rs; > > +}; > > + > > Hmm. > > The monitor I/O thread code makes an effort not to restrict I/O thread > use to QMP, even though we only use it there. Whether the code would > actually work for HMP as well we don't know. > > Readline was similar until your PATCH 02: the code made an effort not to > restrict it to HMP, even though we only use it there. Whether the code > would actually work for QMP as well we don't know. > > Should we stop pretending and hard-code "I/O thread only for QMP"? > > If yes, the comment above gets simplified by the patch that hard-codes > "I/O thread only for QMP". > > If no, we should perhaps point out that we currently don't use an I/O > thread with HMP. The comment above seems like a good place for that. Yes I agree on that if we're refactoring the comment then we can make it more explicit here. For my own preference, I would prefer the latter one, even we can have a bigger comment above MonitorHMP mentioning that it's only used in main thread so no lock is needed for all the HMP only structs (until someone wants to hammer on HMP again). Thanks, -- Peter Xu
Re: [Qemu-block] [PATCH] iothread: fix epollfd leak in the process of delIOThread
On Wed, May 16, 2018 at 09:38:31AM +0800, Fam Zheng wrote: > On Tue, 05/15 20:00, Jie Wang wrote: > > When we call addIOThread, the epollfd created in aio_context_setup, > > but not close it in the process of delIOThread, so the epollfd will leak. > > > > Signed-off-by: Jie Wang > > --- > > iothread.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/iothread.c b/iothread.c > > index aff1281..23ac0a8 100644 > > --- a/iothread.c > > +++ b/iothread.c > > @@ -128,6 +128,7 @@ static void iothread_instance_finalize(Object *obj) > > * GSources first before destroying any GMainContext. > > */ > > if (iothread->ctx) { > > +close(iothread->ctx->epollfd); > > aio_context_unref(iothread->ctx); > > iothread->ctx = NULL; > > } > > -- > > 1.8.3.1 > > > > Please add an aio_context_destroy() function in aio-posix.c and call it from > aio_context_finalize(). IOThread code should not touch AioContext internals. I believe Fam means aio_ctx_finalize(). > Also please remember to wrap the close() code in CONFIG_EPOLL_CREATE1. An > empty > function may need to be added to aio-win32.c, too. Agreed. -- Peter Xu
Re: [Qemu-block] [PATCH v2] iothread: fix epollfd leak in the process of delIOThread
On Wed, May 16, 2018 at 02:39:44PM +0800, Jie Wang wrote: > From: w00251574 (Maybe you'd prefer to still use "Jie Wang" here? :) > > When we call addIOThread, the epollfd created in aio_context_setup, > but not close it in the process of delIOThread, so the epollfd will leak. > > Signed-off-by: Jie Wang [...] > diff --git a/util/aio-posix.c b/util/aio-posix.c > index d8f0cb4af8..bd81455851 100644 > --- a/util/aio-posix.c > +++ b/util/aio-posix.c > @@ -713,6 +713,13 @@ void aio_context_setup(AioContext *ctx) > #endif > } > > +void aio_context_destroy(AioContext *ctx) > +{ > +#ifdef CONFIG_EPOLL_CREATE1 > +close(ctx->epollfd); Would it be better to call aio_epoll_disable() here? Otherwise it looks good to me. > +#endif > +} > + Regards, -- Peter Xu
Re: [Qemu-block] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
On Wed, May 16, 2018 at 07:14:53PM +0800, WangJie (Pluto) wrote: > Hi, Peter Xu: > If call aio_epoll_disable() here, aio_epoll_disable() will return > before close ctx->epollfd, > Because the ctx->epoll_enabled is false in the moment. > In the process of addIOThread, aio_context_setup created epoll without > call aio_epoll_try_enable, > so ctx->epoll_enabled have no chance to set true. I see that epoll_available will only be set if epollfd != -1, so it seems to me to make more sense if we swap the two variables in aio_epoll_disable(), from current version: static void aio_epoll_disable(AioContext *ctx) { ctx->epoll_available = false; if (!ctx->epoll_enabled) { return; } ctx->epoll_enabled = false; close(ctx->epollfd); } To: static void aio_epoll_disable(AioContext *ctx) { ctx->epoll_enabled = false; if (!ctx->epoll_available) { return; } ctx->epoll_available = false; close(ctx->epollfd); } What do you think? And Fam? > > On 2018/5/16 16:36, Jie Wang wrote: > > +void aio_context_destroy(AioContext *ctx) > > +{ > > +#ifdef CONFIG_EPOLL_CREATE1 > > +if (ctx->epollfd >= 0) { > > +close(ctx->epollfd); > > +} > > +#endif > > +} > > + > > void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns, > > int64_t grow, int64_t shrink, Error > > **errp) > -- Peter Xu
Re: [Qemu-block] [PATCH v5] iothread: fix epollfd leak in the process of delIOThread
On Thu, May 17, 2018 at 08:42:43AM +0800, Jie Wang wrote: > When we call addIOThread, the epollfd created in aio_context_setup, > but not close it in the process of delIOThread, so the epollfd will leak. (maybe also mention about the aio_epoll_disable change, or split into two patches?) > > Signed-off-by: Jie Wang Reviewed-by: Peter Xu > --- > include/block/aio.h | 8 > util/aio-posix.c| 13 ++--- > util/aio-win32.c| 4 > util/async.c| 1 + > 4 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/include/block/aio.h b/include/block/aio.h > index a1d6b9e..ae6f354 100644 > --- a/include/block/aio.h > +++ b/include/block/aio.h > @@ -555,6 +555,14 @@ static inline bool in_aio_context_home_thread(AioContext > *ctx) > void aio_context_setup(AioContext *ctx); > > /** > + * aio_context_destroy: > + * @ctx: the aio context > + * > + * Destroy the aio context. > + */ > +void aio_context_destroy(AioContext *ctx); > + > +/** > * aio_context_set_poll_params: > * @ctx: the aio context > * @max_ns: how long to busy poll for, in nanoseconds > diff --git a/util/aio-posix.c b/util/aio-posix.c > index d8f0cb4..118bf57 100644 > --- a/util/aio-posix.c > +++ b/util/aio-posix.c > @@ -45,11 +45,11 @@ struct AioHandler > > static void aio_epoll_disable(AioContext *ctx) > { > -ctx->epoll_available = false; > -if (!ctx->epoll_enabled) { > +ctx->epoll_enabled = false; > +if (!ctx->epoll_available) { > return; > } > -ctx->epoll_enabled = false; > +ctx->epoll_available = false; > close(ctx->epollfd); > } > > @@ -713,6 +713,13 @@ void aio_context_setup(AioContext *ctx) > #endif > } > > +void aio_context_destroy(AioContext *ctx) > +{ > +#ifdef CONFIG_EPOLL_CREATE1 > +aio_epoll_disable(ctx); > +#endif > +} > + > void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns, > int64_t grow, int64_t shrink, Error **errp) > { > diff --git a/util/aio-win32.c b/util/aio-win32.c > index a67b00c..e676a8d 100644 > --- a/util/aio-win32.c > +++ b/util/aio-win32.c > @@ -407,6 +407,10 @@ void aio_context_setup(AioContext *ctx) > { > } > > +void aio_context_destroy(AioContext *ctx) > +{ > +} > + > void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns, > int64_t grow, int64_t shrink, Error **errp) > { > diff --git a/util/async.c b/util/async.c > index 4dd9d95..03f6278 100644 > --- a/util/async.c > +++ b/util/async.c > @@ -298,6 +298,7 @@ aio_ctx_finalize(GSource *source) > qemu_rec_mutex_destroy(&ctx->lock); > qemu_lockcnt_destroy(&ctx->list_lock); > timerlistgroup_deinit(&ctx->tlg); > +aio_context_destroy(ctx); > } > > static GSourceFuncs aio_source_funcs = { > -- > 1.8.3.1 > -- Peter Xu
Re: [Qemu-block] [PATCH v6 2/2] iothread: let aio_epoll_disable fit to aio_context_destroy
On Thu, May 17, 2018 at 10:26:17AM +0800, Jie Wang wrote: > epoll_available will only be set if epollfd != -1, os we > can swap the two variables in aio_epoll_disable, and > aio_context_destroy can call aio_epoll_disable directly. > > Signed-off-by: Jie Wang > --- > util/aio-posix.c | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/util/aio-posix.c b/util/aio-posix.c > index 0ade2c7..118bf57 100644 > --- a/util/aio-posix.c > +++ b/util/aio-posix.c > @@ -45,11 +45,11 @@ struct AioHandler > > static void aio_epoll_disable(AioContext *ctx) > { > -ctx->epoll_available = false; > -if (!ctx->epoll_enabled) { > +ctx->epoll_enabled = false; > +if (!ctx->epoll_available) { > return; > } > -ctx->epoll_enabled = false; > +ctx->epoll_available = false; > close(ctx->epollfd); > } > > @@ -716,9 +716,7 @@ void aio_context_setup(AioContext *ctx) > void aio_context_destroy(AioContext *ctx) > { > #ifdef CONFIG_EPOLL_CREATE1 > -if (ctx->epollfd >= 0) { > -close(ctx->epollfd); > -} > +aio_epoll_disable(ctx); Hmm... I think this patch should be the first if to split. Anyway, IMHO version 5 is already good enough and has got r-bs, no need to bother reposting a version 7. Maintainer could possibly touch up the commit message if necessary. Thanks, > #endif > } > > -- > 1.8.3.1 > -- Peter Xu
Re: [PATCH v8 5/5] multifd: Implement zero copy write in multifd migration (multifd-zero-copy)
On Fri, Feb 18, 2022 at 05:57:13PM +0100, Juan Quintela wrote: > I did a change on: > > commit d48c3a044537689866fe44e65d24c7d39a68868a > Author: Juan Quintela > Date: Fri Nov 19 15:35:58 2021 +0100 > > multifd: Use a single writev on the send side > > Until now, we wrote the packet header with write(), and the rest of the > pages with writev(). Just increase the size of the iovec and do a > single writev(). > > Signed-off-by: Juan Quintela > Reviewed-by: Dr. David Alan Gilbert > > And now we need to "perserve" this header until we do the sync, > otherwise we are overwritting it with other things. > > What testing have you done after this commit? > > Notice that it is not _complicated_ to fix it, I will try to come with > some idea on monday, but basically is having an array of buffers for > each thread, and store them until we call a sync(). Or can we conditionally merge the two write()s? IMHO the array of buffers idea sounds too complicated, and I'm not extremely sure whether it'll pay off at last. We could keep the two write()s with ZEROCOPY enabled, and use the merged version otherwise. Btw, is there any performance measurements for above commit d48c3a044537? I had a feeling that the single write() may not help that much, because for multifd the bottleneck should be on the nic not on the processor. IOW, we could find that the major time used does not fall into the user<->kernel switches (which is where the extra overhead of write() syscall, iiuc), but we simply blocked on any of the write()s because the socket write buffer is full... So we could have saved some cpu cycles by merging the calls, but performance-wise we may not get much. Thanks, -- Peter Xu
Re: introducing vrc :)
On Tue, Apr 19, 2022 at 04:39:13PM +0200, Paolo Bonzini wrote: > Hi all, > > a while ago I looked at tools that could be used too build a call graph. > The simplest but most effective that I found was a small Perl program > (called "egypt", which is rot13 for "rtlcg" aka RTL call graph) that used > the GCC dumps to build the graph. Paolo, Do you have any plan to put it into some git repository? Thanks! -- Peter Xu
Re: introducing vrc :)
On Thu, Apr 21, 2022 at 05:04:52PM +0200, Paolo Bonzini wrote: > On 4/20/22 20:12, Peter Xu wrote: > > > a while ago I looked at tools that could be used too build a call graph. > > > The simplest but most effective that I found was a small Perl program > > > (called "egypt", which is rot13 for "rtlcg" aka RTL call graph) that used > > > the GCC dumps to build the graph. > > > > Do you have any plan to put it into some git repository? > > Good idea, it's now at https://github.com/bonzini/vrc. > > It can be installed using "pip install --user ." after checking out the > repository. Looks great. Another trivial request is whether you'd like to add a sentence to the 1st paragraph of README.md on showing why it's named "vrc"? I'm just curious :-D Thanks again! -- Peter Xu
Re: [PATCH v9 5/7] multifd: multifd_send_sync_main now returns negative on error
On Mon, Apr 25, 2022 at 06:50:54PM -0300, Leonardo Bras wrote: > Even though multifd_send_sync_main() currently emits error_reports, it's > callers don't really check it before continuing. > > Change multifd_send_sync_main() to return -1 on error and 0 on success. > Also change all it's callers to make use of this change and possibly fail > earlier. > > (This change is important to next patch on multifd zero copy > implementation, to make it sure an error in zero-copy flush does not go > unnoticed. > > Signed-off-by: Leonardo Bras Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH v9 7/7] multifd: Implement zero copy write in multifd migration (multifd-zero-copy)
Error *err = NULL; > + > +ret = qio_channel_flush(p->c, &err); > +if (ret < 0) { > +error_report_err(err); > +return -1; > +} > +} > } > for (i = 0; i < migrate_multifd_channels(); i++) { > MultiFDSendParams *p = &multifd_send_state->params[i]; > @@ -688,10 +708,9 @@ static void *multifd_send_thread(void *opaque) > p->iov[0].iov_base = p->packet; > } > > -ret = qio_channel_writev_all(p->c, p->iov + iov_offset, > - p->iovs_num - iov_offset, > - &local_err); > - > +ret = qio_channel_writev_full_all(p->c, p->iov + iov_offset, > + p->iovs_num - iov_offset, NULL, > + 0, p->write_flags, &local_err); I kind of agree with Dan in previous patch - this iov_offset is confusing, better drop it. > if (ret != 0) { > break; > } > @@ -920,6 +939,13 @@ int multifd_save_setup(Error **errp) > /* We need one extra place for the packet header */ > p->iov = g_new0(struct iovec, page_count + 1); > p->normal = g_new0(ram_addr_t, page_count); > + > +if (migrate_use_zero_copy_send()) { > +p->write_flags = QIO_CHANNEL_WRITE_FLAG_ZERO_COPY; > +} else { > +p->write_flags = 0; > +} > + > socket_send_channel_create(multifd_new_send_channel_async, p); > } > > diff --git a/migration/socket.c b/migration/socket.c > index 3754d8f72c..4fd5e85f50 100644 > --- a/migration/socket.c > +++ b/migration/socket.c > @@ -79,8 +79,9 @@ static void socket_outgoing_migration(QIOTask *task, > > trace_migration_socket_outgoing_connected(data->hostname); > > -if (migrate_use_zero_copy_send()) { > -error_setg(&err, "Zero copy send not available in migration"); > +if (migrate_use_zero_copy_send() && > +!qio_channel_has_feature(sioc, QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY)) > { > +error_setg(&err, "Zero copy send feature not detected in host > kernel"); > } > > out: > -- > 2.36.0 > -- Peter Xu
Re: [PATCH v10 6/7] multifd: Send header packet without flags if zero-copy-send is enabled
On Tue, Apr 26, 2022 at 08:06:55PM -0300, Leonardo Bras wrote: > Since d48c3a0445 ("multifd: Use a single writev on the send side"), > sending the header packet and the memory pages happens in the same > writev, which can potentially make the migration faster. > > Using channel-socket as example, this works well with the default copying > mechanism of sendmsg(), but with zero-copy-send=true, it will cause > the migration to often break. > > This happens because the header packet buffer gets reused quite often, > and there is a high chance that by the time the MSG_ZEROCOPY mechanism get > to send the buffer, it has already changed, sending the wrong data and > causing the migration to abort. > > It means that, as it is, the buffer for the header packet is not suitable > for sending with MSG_ZEROCOPY. > > In order to enable zero copy for multifd, send the header packet on an > individual write(), without any flags, and the remanining pages with a > writev(), as it was happening before. This only changes how a migration > with zero-copy-send=true works, not changing any current behavior for > migrations with zero-copy-send=false. > > Signed-off-by: Leonardo Bras > --- > migration/multifd.c | 23 --- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/migration/multifd.c b/migration/multifd.c > index 15fb668e64..07b2e92d8d 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -617,6 +617,7 @@ static void *multifd_send_thread(void *opaque) > MultiFDSendParams *p = opaque; > Error *local_err = NULL; > int ret = 0; > +bool use_zero_copy_send = migrate_use_zero_copy_send(); > > trace_multifd_send_thread_start(p->id); > rcu_register_thread(); > @@ -639,9 +640,14 @@ static void *multifd_send_thread(void *opaque) > if (p->pending_job) { > uint64_t packet_num = p->packet_num; > uint32_t flags = p->flags; > -p->iovs_num = 1; > p->normal_num = 0; > > +if (use_zero_copy_send) { > +p->iovs_num = 0; > +} else { > +p->iovs_num = 1; > +} > + > for (int i = 0; i < p->pages->num; i++) { > p->normal[p->normal_num] = p->pages->offset[i]; > p->normal_num++; > @@ -665,8 +671,19 @@ static void *multifd_send_thread(void *opaque) > trace_multifd_send(p->id, packet_num, p->normal_num, flags, > p->next_packet_size); > > -p->iov[0].iov_len = p->packet_len; > -p->iov[0].iov_base = p->packet; > +if (use_zero_copy_send) { > +/* Send header first, without zerocopy */ > +ret = qio_channel_write_all(p->c, (void *)p->packet, > +p->packet_len, &local_err); > +if (ret != 0) { > +break; > +} > + Extra but useless newline.. but not worth a repost. Looks good here: Reviewed-by: Peter Xu Thanks, > +} else { > +/* Send header using the same writev call */ > +p->iov[0].iov_len = p->packet_len; > +p->iov[0].iov_base = p->packet; > +} > > ret = qio_channel_writev_all(p->c, p->iov, p->iovs_num, > &local_err); > -- > 2.36.0 > -- Peter Xu
Re: [PATCH v10 7/7] multifd: Implement zero copy write in multifd migration (multifd-zero-copy)
On Tue, Apr 26, 2022 at 08:06:56PM -0300, Leonardo Bras wrote: > Implement zero copy send on nocomp_send_write(), by making use of QIOChannel > writev + flags & flush interface. > > Change multifd_send_sync_main() so flush_zero_copy() can be called > after each iteration in order to make sure all dirty pages are sent before > a new iteration is started. It will also flush at the beginning and at the > end of migration. > > Also make it return -1 if flush_zero_copy() fails, in order to cancel > the migration process, and avoid resuming the guest in the target host > without receiving all current RAM. > > This will work fine on RAM migration because the RAM pages are not usually > freed, > and there is no problem on changing the pages content between > writev_zero_copy() and > the actual sending of the buffer, because this change will dirty the page and > cause it to be re-sent on a next iteration anyway. > > A lot of locked memory may be needed in order to use multifd migration > with zero-copy enabled, so disabling the feature should be necessary for > low-privileged users trying to perform multifd migrations. > > Signed-off-by: Leonardo Bras Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH v11 2/7] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX
On Wed, May 04, 2022 at 04:18:31PM -0300, Leonardo Bras wrote: > +/* > + * Zero-copy defines bellow are included to avoid breaking builds on systems > + * that don't support MSG_ZEROCOPY, while keeping the functions more readable > + * (without a lot of ifdefs). > + */ > +#ifndef MSG_ZEROCOPY > +#define MSG_ZEROCOPY 0x400 > +#endif > +#ifndef SO_ZEROCOPY > +#define SO_ZEROCOPY 60 > +#endif So this will define these two values on e.g. FreeBSD, while they do not make sense at all there because these numbers are pure magics and meaningless outside Linux.. I don't think it's anything dangerous, but IMHO it's another way of being not clean comparing of using some "#ifdef"s. Comparing to this approach the "use #ifdef" approach is actually slightly more cleaner to me. :) Let's wait for some other inputs. -- Peter Xu
Re: [PATCH v11 2/7] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX
On Thu, May 05, 2022 at 01:20:04AM -0300, Leonardo Bras Soares Passos wrote: > (2) is already implemented in v11, but I have no issue implementing > (1) for v12 if it's ok to create this 'global' define. Dan's suggestion in the other thread sounds good to me with current approach, on having CONFIG_LINUX to wrap the defines. But note that it's still prone to change in the future, e.g., when other qemu modules start to use MSG_ZEROCOPY, we'll probably at least move the defines into some other headers. We could probably need to clean things up when it happens. But I won't strongly ask for that - we can leave that for later. Thanks, -- Peter Xu
Re: [PATCH v12 2/7] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX
On Fri, May 06, 2022 at 10:57:54PM -0300, Leonardo Bras wrote: > diff --git a/io/channel-socket.c b/io/channel-socket.c > index 05c425abb8..f03a068f25 100644 > --- a/io/channel-socket.c > +++ b/io/channel-socket.c > @@ -25,9 +25,18 @@ > #include "io/channel-watch.h" > #include "trace.h" > #include "qapi/clone-visitor.h" > +#ifdef CONFIG_LINUX > +#include > +#include > + > +#if (defined(MSG_ZEROCOPY) && defined(SO_ZEROCOPY)) > +#define QEMU_MSG_ZEROCOPY > +#endif > +#endif > > #define SOCKET_MAX_FDS 16 > > + This line can be dropped when merge. > SocketAddress * > qio_channel_socket_get_local_address(QIOChannelSocket *ioc, > Error **errp) This does look nicer, imho. :) Thanks! -- Peter Xu
Re: [External] [PATCH v13 3/8] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX
On Wed, Jun 01, 2022 at 05:37:10PM +0800, 徐闯 wrote: > > On 2022/5/13 下午2:28, Leonardo Bras wrote: > > For CONFIG_LINUX, implement the new zero copy flag and the optional callback > > io_flush on QIOChannelSocket, but enables it only when MSG_ZEROCOPY > > feature is available in the host kernel, which is checked on > > qio_channel_socket_connect_sync() > > > > qio_channel_socket_flush() was implemented by counting how many times > > sendmsg(...,MSG_ZEROCOPY) was successfully called, and then reading the > > socket's error queue, in order to find how many of them finished sending. > > Flush will loop until those counters are the same, or until some error > > occurs. > > > > Notes on using writev() with QIO_CHANNEL_WRITE_FLAG_ZERO_COPY: > > 1: Buffer > > - As MSG_ZEROCOPY tells the kernel to use the same user buffer to avoid > > copying, > > some caution is necessary to avoid overwriting any buffer before it's sent. > > If something like this happen, a newer version of the buffer may be sent > > instead. > > - If this is a problem, it's recommended to call qio_channel_flush() before > > freeing > > or re-using the buffer. > > > > 2: Locked memory > > - When using MSG_ZERCOCOPY, the buffer memory will be locked after queued, > > and > > unlocked after it's sent. > > - Depending on the size of each buffer, and how often it's sent, it may > > require > > a larger amount of locked memory than usually available to non-root user. > > - If the required amount of locked memory is not available, writev_zero_copy > > will return an error, which can abort an operation like migration, > > - Because of this, when an user code wants to add zero copy as a feature, it > > requires a mechanism to disable it, so it can still be accessible to less > > privileged users. > > > > Signed-off-by: Leonardo Bras > > Reviewed-by: Peter Xu > > Reviewed-by: Daniel P. Berrangé > > Reviewed-by: Juan Quintela > > --- > > include/io/channel-socket.h | 2 + > > io/channel-socket.c | 116 ++-- > > 2 files changed, 114 insertions(+), 4 deletions(-) > > > > diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h > > index e747e63514..513c428fe4 100644 > > --- a/include/io/channel-socket.h > > +++ b/include/io/channel-socket.h > > @@ -47,6 +47,8 @@ struct QIOChannelSocket { > > socklen_t localAddrLen; > > struct sockaddr_storage remoteAddr; > > socklen_t remoteAddrLen; > > +ssize_t zero_copy_queued; > > +ssize_t zero_copy_sent; > > }; > Hi, Leonardo. I'm also paying attention to the application of MSG_ZEROCOPY > in live migration recently. I noticed that you defined a member > `zero_copy_queued` in the struct QIOChannelSocket, but I can't find out > where the value of this member has been changed in your patch. Can you > answer it for me? > Good point.. it should probably be increased when queuing the pages. We'd better fix it up or it seems the flush() will be literally an no-op.. Two things in qio_channel_socket_flush() we can do to make sure it'll work as expected, imo: 1) make ret=-1 as initial value, rather than 1 - we only check negative errors in the caller so we could have missed a positive "1" 2) add a tracepoint into the loop of updating zero_copy_sent Leo, what's your take? Thanks, -- Peter Xu
Re: [External] [PATCH v13 3/8] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX
On Wed, Jun 08, 2022 at 02:37:28AM -0300, Leonardo Bras Soares Passos wrote: > (1) is not an option, as the interface currently uses ret=1 to make > sure MSG_ZEROCOPY is getting used, > I added that so the user of qio_channel can switch off zero-copy if > it's not getting used, and save some cpu. Yes (1) is not, but could you explain what do you mean by making sure MSG_ZEROCOPY being used? Why is it relevant to the retval here? I just figured it's a bit weird to return >0 here in flush(). > > (2) is not a problem, but I fail to see how useful that would be. Is > the idea manually keeping track of flush happening? Yes if we can check this up it'll be good enough to me. The trace point could help in some case in the future too to monitor the behavior of kernel MSG_ERRQUEUE but if you don't like it then it's okay. -- Peter Xu
Re: [External] [PATCH v13 3/8] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX
On Wed, Jun 08, 2022 at 03:14:36PM -0300, Leonardo Bras Soares Passos wrote: > On Wed, Jun 8, 2022 at 8:41 AM Peter Xu wrote: > > > > On Wed, Jun 08, 2022 at 02:37:28AM -0300, Leonardo Bras Soares Passos wrote: > > > (1) is not an option, as the interface currently uses ret=1 to make > > > sure MSG_ZEROCOPY is getting used, > > > I added that so the user of qio_channel can switch off zero-copy if > > > it's not getting used, and save some cpu. > > > > Yes (1) is not, but could you explain what do you mean by making sure > > MSG_ZEROCOPY being used? Why is it relevant to the retval here? > > If sendmsg() is called with MSG_ZEROCOPY, and everything is configured > correctly, the kernel will attempt to send the buffer using zero-copy. > > Even with the right configuration on a recent enough kernel, there are > factors that can prevent zero-copy from happening, and the kernel will > fall back to the copying mechanism. > An example being the net device not supporting 'Scatter-Gather' > feature (NETIF_F_SG). > > When this happens, there is an overhead for 'trying zero-copy first', > instead of just opting for the copying mechanism. > > In a previous iteration of the patchset, it was made clear that it's > desirable to detect when the kernel falls back to copying mechanism, > so the user of 'QIOChannelSocket' can switch to copying and avoid the > overhead. This was done by the return value of flush(), which is 1 if > that occurs. Two questions.. 1) When that happens, will MSG_ERRQUEUE keeps working just like zerocopy is functional? If the answer is yes, I don't see how ret=1 will ever be returned.. because we'll also go into the same loop in qio_channel_socket_flush() anyway. If the answer is no, then since we'll have non-zero zero_copy_queued, will the loop in qio_channel_socket_flush() go into a dead one? How could it return? 2) Even if we have the correct ret=1 returned when that happens, which caller is detecting that ret==1 and warn the admin? Thanks, -- Peter Xu
Re: [External] [PATCH v13 3/8] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX
On Mon, Jun 13, 2022 at 05:58:44PM -0300, Leonardo Bras Soares Passos wrote: > Hello Peter, > > On Wed, Jun 8, 2022 at 5:23 PM Peter Xu wrote: > [...] > > > In a previous iteration of the patchset, it was made clear that it's > > > desirable to detect when the kernel falls back to copying mechanism, > > > so the user of 'QIOChannelSocket' can switch to copying and avoid the > > > overhead. This was done by the return value of flush(), which is 1 if > > > that occurs. > > > > Two questions.. > > > > 1) When that happens, will MSG_ERRQUEUE keeps working just like zerocopy > > is functional? > > I am not sure about what exactly you meant by 'like zerocopy is > funcional', but the > idea is that reading from MSG_ERRQUEUE should return a msg for each sendmsg > syscall with MSG_ZEROCOPY that previously happened. This does not depend on > the outcome (like falling back to the copying mechanism). > btw, most of those messages may be batched to reduce overhead. > > At some point, zero-copy may fail, and fall back to copying, so in > those messages > an error code SO_EE_CODE_ZEROCOPY_COPIED can be seen. Having only > those messages in a flush will trigger the returning of 1 from the > flush function. Ah I think I missed the "reset ret==0 when !SO_EE_CODE_ZEROCOPY_COPIED" path.. Sorry. > > > > > If the answer is yes, I don't see how ret=1 will ever be > > returned.. because we'll also go into the same loop in > > qio_channel_socket_flush() anyway. > > > We set ret to 1 at function entry and then for each message in the > MSG_ERRQUEUE, > we test if it has error code different than SO_EE_CODE_ZEROCOPY_COPIED. > If it ever have a different error code, we set ret=0. > > So, in our previous example, if we have a net device not supporting > the 'Scatter-Gather' > feature (NETIF_F_SG), every error message will be > SO_EE_CODE_ZEROCOPY_COPIED, and it will return 1. > > > > > > If the answer is no, then since we'll have non-zero zero_copy_queued, > > will the loop in qio_channel_socket_flush() go into a dead one? How > > could it return? > > No, because it will go through all packets sent with MSG_ZEROCOPY, including > the > ones that fell back to copying, so the counter should be fine. If any > code disables > zero-copy, it will both stop sending stuff wil MSG_ZEROCOPY and flushing, so > it > should be fine. > > > > > 2) Even if we have the correct ret=1 returned when that happens, which > > caller is detecting that ret==1 and warn the admin? > > > > No caller is using that right now. > It's supposed to be a QIOChannel interface feature, and any > user/implementation > could use that information to warn if zero-copy is not being used, fall back > to > copying directly (to avoid overhead of testing zero-copy) or even use > it to cancel the > sending if wanted. > > It was a suggestion of Daniel on top of [PATCH v5 1/6] IIRC. OK the detection makes sense, thanks for the details. Then now I'm wondering whether we should have warned the admin already if zero-copy send is not fully enabled in live migration. Should we add a error_report_once() somewhere for the ret==1 already? After all the user specify zero_copy_send=true explicitly. Did I miss something again? Thanks, -- Peter Xu
Re: QMP (without OOB) function running in thread different from the main thread as part of aio_poll
../migration/savevm.c:2954 > >> #15 0x558ed3e17fb1 in snapshot_save_job_bh (opaque=0x558ed68c5170) at > >> ../migration/savevm.c:3253 > >> #16 0x558ed42f050a in aio_bh_call (bh=0x558ed671ae00) at > >> ../util/async.c:155 > >> #17 0x558ed42f0615 in aio_bh_poll (ctx=0x558ed5c62910) at > >> ../util/async.c:184 > >> #18 0x558ed42d47b8 in aio_poll (ctx=0x558ed5c62910, blocking=true) at > >> ../util/aio-posix.c:721 > >> #19 0x558ed412df1c in bdrv_poll_co (s=0x7f8ffcff3eb0) at > >> /home/febner/repos/qemu/block/block-gen.h:43 > >> #20 0x558ed4130c3a in blk_pwrite (blk=0x558ed5ed4f60, > >> offset=230912, bytes=512, buf=0x7f8ffc438600, flags=0) at > >> block/block-gen.c:1650 > >> #21 0x558ed3ba9078 in pflash_update (pfl=0x558ed5eb7b30, > >> offset=230912, size=1) at ../hw/block/pflash_cfi01.c:394 > >> #22 0x558ed3ba9749 in pflash_write (pfl=0x558ed5eb7b30, > >> offset=231232, value=0, width=1, be=0) at > >> ../hw/block/pflash_cfi01.c:522 > >> #23 0x558ed3ba9cda in pflash_mem_write_with_attrs > >> (opaque=0x558ed5eb7b30, addr=231232, value=0, len=1, attrs=...) at > >> ../hw/block/pflash_cfi01.c:681 > >> #24 0x558ed402a36a in memory_region_write_with_attrs_accessor > >> (mr=0x558ed5eb7ef0, addr=231232, value=0x7f8ffcff40c8, size=1, > >> shift=0, mask=255, attrs=...) at ../softmmu/memory.c:514 > >> #25 0x558ed402a4a9 in access_with_adjusted_size (addr=231232, > >> value=0x7f8ffcff40c8, size=1, access_size_min=1, access_size_max=4, > >> access_fn=0x558ed402a270 , > >> mr=0x558ed5eb7ef0, attrs=...) at ../softmmu/memory.c:555 > >> #26 0x558ed402d5de in memory_region_dispatch_write > >> (mr=0x558ed5eb7ef0, addr=231232, data=0, op=MO_8, attrs=...) at > >> ../softmmu/memory.c:1522 > >> #27 0x558ed403a6f4 in flatview_write_continue > >> (fv=0x558ed66d62c0, addr=4291004224, attrs=..., ptr=0x7f9029957028, > >> len=1, addr1=231232, l=1, mr=0x558ed5eb7ef0) at > >> ../softmmu/physmem.c:2641 > >> #28 0x558ed403a857 in flatview_write (fv=0x558ed66d62c0, > >> addr=4291004224, attrs=..., buf=0x7f9029957028, len=1) at > >> ../softmmu/physmem.c:2683 > >> #29 0x558ed403ac07 in address_space_write (as=0x558ed4ca2b20 > >> , addr=4291004224, attrs=..., > >> buf=0x7f9029957028, len=1) at ../softmmu/physmem.c:2779 > >> #30 0x558ed403ac74 in address_space_rw (as=0x558ed4ca2b20 > >> , addr=4291004224, attrs=..., > >> buf=0x7f9029957028, len=1, is_write=true) at > >> ../softmmu/physmem.c:2789 > >> #31 0x558ed40cea88 in kvm_cpu_exec (cpu=0x558ed622a910) at > >> ../accel/kvm/kvm-all.c:2989 > >> #32 0x558ed40d179a in kvm_vcpu_thread_fn (arg=0x558ed622a910) at > >> ../accel/kvm/kvm-accel-ops.c:51 > >> #33 0x558ed42d925f in qemu_thread_start (args=0x558ed5c68c80) at > >> ../util/qemu-thread-posix.c:541 > >> #34 0x7f9028ab7ea7 in start_thread (arg=) at > >> pthread_create.c:477 > >> #35 0x7f9027c18a2f in clone () at > >> ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 Totally unfamiliar with block jobs, but... does it mean that snapshot_*_job_bh()s should just always make sure BQL taken? Thanks, -- Peter Xu
Re: [PATCH 5/9] qemu-file: No need to check for shutdown in qemu_file_rate_limit
On Thu, May 04, 2023 at 01:38:37PM +0200, Juan Quintela wrote: > After calling qemu_file_shutdown() we set the error as -EIO if there > is no another previous error, so no need to check it here. > > Signed-off-by: Juan Quintela Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH 6/9] qemu-file: remove shutdown member
On Thu, May 04, 2023 at 01:38:38PM +0200, Juan Quintela wrote: > The first thing that we do after setting the shutdown value is set the > error as -EIO if there is not a previous error. > > So this value is reduntant. Just remove it and use > qemu_file_get_error() in the places that it was tested. > > Signed-off-by: Juan Quintela Nit: I'd squash this with previous. Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH 8/9] qemu-file: Make ram_control_save_page() use accessors for rate_limit
On Thu, May 04, 2023 at 01:38:40PM +0200, Juan Quintela wrote: > Signed-off-by: Juan Quintela Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH 9/9] qemu-file: Account for rate_limit usage on qemu_fflush()
On Thu, May 04, 2023 at 01:38:41PM +0200, Juan Quintela wrote: > That is the moment we know we have transferred something. > > Signed-off-by: Juan Quintela There'll be a slight side effect that qemu_file_rate_limit() can be triggered later than before because data cached in the qemufile won't be accounted until flushed. Two limits here: - IO_BUF_SIZE==32K, the real buffer - MAX_IOV_SIZE==64 (I think), the async buffer to put guest page ptrs directly, on x86 it's 64*4K=256K So the impact should be no more than 288KB on x86. Looks still fine.. Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH 0/9] QEMU file cleanups
On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote: > - convince and review code to see that everything is uint64_t. One general question to patches regarding this - what's the major benefit of using uint64_t? It doubles the possible numbers to hold, but it's already 64bits so I don't think it matters a lot. The thing is we're removing some code trying to detect negative which seems to be still helpful to detect e.g. overflows (even though I don't think it'll happen). I just still think it's good to know when overflow happens, and not sure what I missed on benefits of using unsigned here. I've reviewed all the rest patches and all look good here. Thanks, -- Peter Xu
Re: [PATCH 0/9] QEMU file cleanups
On Thu, May 04, 2023 at 04:56:46PM +0200, Juan Quintela wrote: > Peter Xu wrote: > > On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote: > >> - convince and review code to see that everything is uint64_t. > > > > One general question to patches regarding this - what's the major benefit > > of using uint64_t? > > > > It doubles the possible numbers to hold, but it's already 64bits so I don't > > think it matters a lot. > > We were checking for negatives even when that can't be. > And we are doing this dance of > > int64_t x, y; > uint64_t a, b; > > x = a; > b = y; > > This is always confusing and not always right. Yeah this is confusing, but if anything can go wrong with this I assume we could have some bigger problem anyway.. > > > The thing is we're removing some code trying to > > detect negative which seems to be still helpful to detect e.g. overflows > > (even though I don't think it'll happen). I just still think it's good to > > know when overflow happens, and not sure what I missed on benefits of using > > unsigned here. > > If you grep through the code, you see that half of the things are > int64_t and the other half is uint64_t. I find it always confusing. Right, I'm personally curious whether we should just use int64_t always unless necessary. :) Another good thing with int64_t is it's also suitable for error report when used in retvals. But no strong opinion here, I don't think that's a huge deal for now. Having such an alignment on types makes sense to me. Thanks, -- Peter Xu
Re: [PATCH] migration: for snapshots, hold the BQL during setup callbacks
On Fri, May 05, 2023 at 03:46:52PM +0200, Fiona Ebner wrote: > To fix it, ensure that the BQL is held during setup. To avoid changing > the behavior for migration too, introduce conditionals for the setup > callbacks that need the BQL and only take the lock if it's not already > held. The major complexity of this patch is the "conditionally taking" part. Pure question: what is the benefit of not holding BQL always during save_setup(), if after all we have this coroutine issue to be solved? I can understand that it helps us to avoid taking BQL too long, but we'll need to take it anyway during ramblock dirty track initializations, and so far IIUC it's the major time to be consumed during setup(). Commit message of 9b0950375277467 says, "Only the migration_bitmap_sync() call needs the iothread lock". Firstly I think it's also covering enablement of dirty tracking: +qemu_mutex_lock_iothread(); +qemu_mutex_lock_ramlist(); +bytes_transferred = 0; +reset_ram_globals(); + memory_global_dirty_log_start(); migration_bitmap_sync(); +qemu_mutex_unlock_iothread(); And I think enablement itself can be slow too, maybe even slower than migration_bitmap_sync() especially with KVM_DIRTY_LOG_INITIALLY_SET supported in the kernel. Meanwhile I always got confused on why we need to sync dirty bitmap when setup at all. Say, what if we drop migration_bitmap_sync() here? After all, shouldn't all pages be dirty from start (ram_list_init_bitmaps())? This is slightly off-topic, but I'd like to know if someone can help answer. My whole point is still questioning whether we can unconditionally take bql during save_setup(). I could have missed something, though, where we want to do in setup() but avoid holding BQL. Help needed on figuring this out (and if there is, IMHO it'll be worthwhile to put that into comment of save_setup() hook). Thanks, -- Peter Xu
Re: [PATCH] migration: for snapshots, hold the BQL during setup callbacks
On Wed, May 10, 2023 at 08:31:13AM +0200, Juan Quintela wrote: > Peter Xu wrote: > > Hi > > [Adding Kevin to the party] > > > On Fri, May 05, 2023 at 03:46:52PM +0200, Fiona Ebner wrote: > >> To fix it, ensure that the BQL is held during setup. To avoid changing > >> the behavior for migration too, introduce conditionals for the setup > >> callbacks that need the BQL and only take the lock if it's not already > >> held. > > > > The major complexity of this patch is the "conditionally taking" part. > > Yeap. > > I don't want that bit. > > This is another case of: > - I have a problem > - I will use recursive mutexes to solve it > > Now you have two problems O:-) > > > Pure question: what is the benefit of not holding BQL always during > > save_setup(), if after all we have this coroutine issue to be solved? > > Dunno. > > I would like that paolo commented on this. I "reviewed the code" 10 > years ago. I don't remember at all why we wanted to change that. > > > I can understand that it helps us to avoid taking BQL too long, but we'll > > need to take it anyway during ramblock dirty track initializations, and so > > far IIUC it's the major time to be consumed during setup(). > > > > Commit message of 9b0950375277467 says, "Only the migration_bitmap_sync() > > call needs the iothread lock". Firstly I think it's also covering > > enablement of dirty tracking: > > > > +qemu_mutex_lock_iothread(); > > +qemu_mutex_lock_ramlist(); > > +bytes_transferred = 0; > > +reset_ram_globals(); > > + > > memory_global_dirty_log_start(); > > migration_bitmap_sync(); > > +qemu_mutex_unlock_iothread(); > > > > And I think enablement itself can be slow too, maybe even slower than > > migration_bitmap_sync() especially with KVM_DIRTY_LOG_INITIALLY_SET > > supported in the kernel. > > > > Meanwhile I always got confused on why we need to sync dirty bitmap when > > setup at all. Say, what if we drop migration_bitmap_sync() here? After > > all, shouldn't all pages be dirty from start (ram_list_init_bitmaps())? > > How do you convince KVM (or the other lists) to start doing dirty > tracking? Doing a bitmap sync. I think memory_global_dirty_log_start() kicks off the tracking already. Take KVM as example, normally the case is KVM_MEM_LOG_DIRTY_PAGES is set there, then ioctl(KVM_SET_USER_MEMORY_REGION) will start dirty tracking for all of the guest memory slots necessary (including wr-protect all pages). KVM_DIRTY_LOG_INITIALLY_SET is slightly special, it'll skip that procedure during ioctl(KVM_SET_USER_MEMORY_REGION), but that also means the kernel assumed the userapp (QEMU) marked all pages dirty initially (always the case for QEMU, I think..). Hence in this case the sync doesn't help either because we'll simply get no new dirty bits in this shot.. > > And yeap, probably there is a better way of doing it. > > > This is slightly off-topic, but I'd like to know if someone can help > > answer. > > > > My whole point is still questioning whether we can unconditionally take bql > > during save_setup(). > > I agree with you. > > > I could have missed something, though, where we want to do in setup() but > > avoid holding BQL. Help needed on figuring this out (and if there is, IMHO > > it'll be worthwhile to put that into comment of save_setup() hook). > > I am more towards revert completely > 9b0950375277467fd74a9075624477ae43b9bb22 > > and call it a day. On migration we don't use coroutines on the sending > side (I mean the migration code, the block layer uses coroutines for > everything/anything). > > Paolo, Stefan any clues for not run setup with the BKL? > > Later, Juan. > -- Peter Xu
Re: [PATCH 21/21] migration/multifd: Compute transferred bytes correctly
On Mon, May 08, 2023 at 03:09:09PM +0200, Juan Quintela wrote: > In the past, we had to put the in the main thread all the operations > related with sizes due to qemu_file not beeing thread safe. As now > all counters are atomic, we can update the counters just after the > do the write. As an aditional bonus, we are able to use the right > value for the compression methods. Right now we were assuming that > there were no compression at all. Maybe worth mention that initial packet is also accounted after this. > > Signed-off-by: Juan Quintela Two more trivial nits: > --- > migration/multifd.c | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/migration/multifd.c b/migration/multifd.c > index 9d2ade7abc..3a19d8e304 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods *ops) > static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) > { > MultiFDInit_t msg = {}; > +size_t size = sizeof(msg); > int ret; > > msg.magic = cpu_to_be32(MULTIFD_MAGIC); > @@ -182,10 +183,12 @@ static int > multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) > msg.id = p->id; > memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid)); > > -ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp); > +ret = qio_channel_write_all(p->c, (char *)&msg, size, errp); > if (ret != 0) { > return -1; > } > +stat64_add(&mig_stats.multifd_bytes, size); > +stat64_add(&mig_stats.transferred, size); > return 0; > } > > @@ -396,7 +399,6 @@ static int multifd_send_pages(QEMUFile *f) > static int next_channel; > MultiFDSendParams *p = NULL; /* make happy gcc */ > MultiFDPages_t *pages = multifd_send_state->pages; > -uint64_t transferred; > > if (qatomic_read(&multifd_send_state->exiting)) { > return -1; > @@ -431,10 +433,7 @@ static int multifd_send_pages(QEMUFile *f) > p->packet_num = multifd_send_state->packet_num++; > multifd_send_state->pages = p->pages; > p->pages = pages; > -transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len; > qemu_mutex_unlock(&p->mutex); > -stat64_add(&mig_stats.transferred, transferred); > -stat64_add(&mig_stats.multifd_bytes, transferred); > qemu_sem_post(&p->sem); > > return 1; > @@ -716,6 +715,8 @@ static void *multifd_send_thread(void *opaque) > if (ret != 0) { > break; > } > +stat64_add(&mig_stats.multifd_bytes, p->packet_len); > +stat64_add(&mig_stats.transferred, p->packet_len); > } else { > /* Send header using the same writev call */ > p->iov[0].iov_len = p->packet_len; > @@ -728,6 +729,8 @@ static void *multifd_send_thread(void *opaque) > break; > } > > +stat64_add(&mig_stats.multifd_bytes, p->next_packet_size); > +stat64_add(&mig_stats.transferred, p->next_packet_size); Two nits: Maybe merge the two so half atomic operations? Also maybe also worth having a inline helper for adding both multifd_bytes and transferred? With/without that, all look good: Reviewed-by: Peter Xu Thanks, > qemu_mutex_lock(&p->mutex); > p->pending_job--; > qemu_mutex_unlock(&p->mutex); > -- > 2.40.0 > -- Peter Xu
Re: [PATCH 21/21] migration/multifd: Compute transferred bytes correctly
On Thu, May 18, 2023 at 06:40:18PM +0200, Juan Quintela wrote: > Peter Xu wrote: > > On Mon, May 08, 2023 at 03:09:09PM +0200, Juan Quintela wrote: > >> In the past, we had to put the in the main thread all the operations > >> related with sizes due to qemu_file not beeing thread safe. As now > >> all counters are atomic, we can update the counters just after the > >> do the write. As an aditional bonus, we are able to use the right > >> value for the compression methods. Right now we were assuming that > >> there were no compression at all. > > > > Maybe worth mention that initial packet is also accounted after this. > > Ok. > >> > >> +stat64_add(&mig_stats.multifd_bytes, p->next_packet_size); > >> +stat64_add(&mig_stats.transferred, p->next_packet_size); > > > > Two nits: > > > > Maybe merge the two so half atomic operations? > > On my tree, to send after this got in: > > 77fdd3475c migration: Remove transferred atomic counter > > O:-) Ah this looks even better, indeed. :) What I meant was we can also do atomic update in one shot for both next_packet_size + packet_len. > > > Also maybe also worth having a inline helper for adding both multifd_bytes > > and transferred? > > I am removing it. > > After next set of packates: > > rate limit is calulated as: > > begining_period = migration_transferred_bytes(); > ... > > bytes_this_period = migration_transferred_bytes() - begining_period; > > transferred is calculated as: > - multifd_bytes + qemu_file_bytes; > > So things get really simple. As all counters are atomic, you do a > write and after the write to increse the write size to the qemu_file or > to the multifd_bytes. And that is it. Agreed. Thanks, -- Peter Xu
Re: [PATCH v2 04/20] qemu-file: We only call qemu_file_transferred_* on the sending side
On Tue, May 30, 2023 at 08:39:25PM +0200, Juan Quintela wrote: > Remove the increase in qemu_file_fill_buffer() and add asserts to > qemu_file_transferred* functions. > > Signed-off-by: Juan Quintela The read side accounting does look a bit weird and never caught my notice.. Maybe worth also touching the document of QEMUFile::total_transferred to clarify what it accounts? Reviewed-by: Peter Xu Though when I'm looking at the counters (didn't follow every single recent patch on this..), I found that now reading transferred value is actually more expensive - qemu_file_transferred() needs flushing, even if for the fast version, qemu_file_transferred_fast() loops over all possible iovs, which can be as large as MAX_IOV_SIZE==64. To be explicit, I _think_ for each guest page we now need to flush... ram_save_iterate migration_rate_exceeded migration_transferred_bytes qemu_file_transferred I hope I'm wrong.. Does it mean that perhaps we simply need "sent and put into send buffer" more than "what really got transferred"? So I start to wonder what's the origianl purpose of this change, and which one is better.. -- Peter Xu
Re: [PATCH v2 05/20] qemu_file: Use a stat64 for qemu_file_transferred
On Tue, May 30, 2023 at 08:39:26PM +0200, Juan Quintela wrote: > This way we can read it from any thread. > I checked that it gives the same value than the current one. We never > use to qemu_files at the same time. > > Signed-off-by: Juan Quintela The follow up patch may be better to be squashed or it's very confusing.. Why do we need to convert mostly everything into atomics? Is it modified outside migration thread? AFAIR atomic ops are still expensive, and will get more expensive on larger hosts, IOW it'll be good for us to keep non-atomic when possible (and that's why when I changed some counters in postcopy work I only changed the limited set because then the rest are still accessed in 1 single thread and keep running fast). > --- > migration/migration-stats.h | 4 > migration/qemu-file.c | 5 +++-- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/migration/migration-stats.h b/migration/migration-stats.h > index 2358caad63..b7795e7914 100644 > --- a/migration/migration-stats.h > +++ b/migration/migration-stats.h > @@ -81,6 +81,10 @@ typedef struct { > * Number of bytes sent during precopy stage. > */ > Stat64 precopy_bytes; > +/* > + * Number of bytes transferred with QEMUFile. > + */ > +Stat64 qemu_file_transferred; > /* > * Amount of transferred data at the start of current cycle. > */ > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > index be3dab85cb..eb0497e532 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -288,6 +288,7 @@ void qemu_fflush(QEMUFile *f) > } else { > uint64_t size = iov_size(f->iov, f->iovcnt); > f->total_transferred += size; > +stat64_add(&mig_stats.qemu_file_transferred, size); > } > > qemu_iovec_release_ram(f); > @@ -628,7 +629,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f) > > uint64_t qemu_file_transferred_noflush(QEMUFile *f) > { > -uint64_t ret = f->total_transferred; > +uint64_t ret = stat64_get(&mig_stats.qemu_file_transferred); > int i; > > g_assert(qemu_file_is_writable(f)); > @@ -644,7 +645,7 @@ uint64_t qemu_file_transferred(QEMUFile *f) > { > g_assert(qemu_file_is_writable(f)); > qemu_fflush(f); > -return f->total_transferred; > +return stat64_get(&mig_stats.qemu_file_transferred); > } > > void qemu_put_be16(QEMUFile *f, unsigned int v) > -- > 2.40.1 > -- Peter Xu
Re: [RFC 4/6] migration: Deprecate -incoming
On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote: > Only "defer" is recommended. After setting all migation parameters, > start incoming migration with "migrate-incoming uri" command. > > Signed-off-by: Juan Quintela > --- > docs/about/deprecated.rst | 7 +++ > softmmu/vl.c | 2 ++ > 2 files changed, 9 insertions(+) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 47e98dc95e..518672722d 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -447,3 +447,10 @@ The new way to modify migration is using migration > parameters. > ``blk`` functionality can be acchieved using > ``migrate_set_parameter block-incremental true``. > > +``-incoming uri`` (since 8.1) > +''''''''''''''''''''''''''''' > + > +Everything except ``-incoming defer`` are deprecated. This allows to > +setup parameters before launching the proper migration with > +``migrate-incoming uri``. > + > diff --git a/softmmu/vl.c b/softmmu/vl.c > index b0b96f67fa..7fe865ab59 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) > if (incoming) { > Error *local_err = NULL; > if (strcmp(incoming, "defer") != 0) { > +warn_report("-incoming %s is deprecated, use -incoming defer and > " > +" set the uri with migrate-incoming.", incoming); I still use uri for all my scripts, alongside with "-global migration.xxx" and it works. Shall we just leave it there? Or is deprecating it helps us in any form? -- Peter Xu
Re: [RFC 4/6] migration: Deprecate -incoming
On Mon, Jun 12, 2023 at 10:51:08PM +0200, Juan Quintela wrote: > Peter Xu wrote: > > On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote: > >> Only "defer" is recommended. After setting all migation parameters, > >> start incoming migration with "migrate-incoming uri" command. > >> > >> Signed-off-by: Juan Quintela > >> --- > >> docs/about/deprecated.rst | 7 +++ > >> softmmu/vl.c | 2 ++ > >> 2 files changed, 9 insertions(+) > >> > >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > >> index 47e98dc95e..518672722d 100644 > >> --- a/docs/about/deprecated.rst > >> +++ b/docs/about/deprecated.rst > >> @@ -447,3 +447,10 @@ The new way to modify migration is using migration > >> parameters. > >> ``blk`` functionality can be acchieved using > >> ``migrate_set_parameter block-incremental true``. > >> > >> +``-incoming uri`` (since 8.1) > >> +''''''''''''''''''''''''''''' > >> + > >> +Everything except ``-incoming defer`` are deprecated. This allows to > >> +setup parameters before launching the proper migration with > >> +``migrate-incoming uri``. > >> + > >> diff --git a/softmmu/vl.c b/softmmu/vl.c > >> index b0b96f67fa..7fe865ab59 100644 > >> --- a/softmmu/vl.c > >> +++ b/softmmu/vl.c > >> @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) > >> if (incoming) { > >> Error *local_err = NULL; > >> if (strcmp(incoming, "defer") != 0) { > >> +warn_report("-incoming %s is deprecated, use -incoming defer > >> and " > >> +" set the uri with migrate-incoming.", incoming); > > > > I still use uri for all my scripts, alongside with "-global migration.xxx" > > and it works. > > You know what you are doing (TM). > And remember that we don't support -gobal migration.x-foo. > Yes, I know, we should drop the "x-" prefixes. I hope they'll always be there. :) They're pretty handy for tests, when we want to boot a VM without the need to script the sequences of qmp cmds. Yes, we probably should just always drop the x-. We can always declare debugging purpose for all -global migration.* fields. > > > Shall we just leave it there? Or is deprecating it helps us in any form? > > See the patches two weeks ago when people complained that lisen(.., num) > was too low. And there are other parameters that work the same way > (that I convenientely had forgotten). So the easiest way to get things > right is to use "defer" always. Using -incoming "uri" should only be > for people that "know what they are doing", so we had to ways to do it: > - review all migration options and see which ones work without defer > and document it > - deprecate everything that is not defer. > > Anything else is not going to be very user unfriendly. > What do you think. IIRC Wei Wang had a series just for that, so after that patchset applied we should have fixed all issues cleanly? Is there one more thing that's not working right there? > > Later, Juan. > > PD. This series are RFC for multiple reasons O:-) Happy to know the rest (besides which I know will break my script :). -- Peter Xu
Re: [PATCH v2 04/20] qemu-file: We only call qemu_file_transferred_* on the sending side
On Tue, Jun 13, 2023 at 06:02:05PM +0200, Juan Quintela wrote: > Peter Xu wrote: > > On Tue, May 30, 2023 at 08:39:25PM +0200, Juan Quintela wrote: > >> Remove the increase in qemu_file_fill_buffer() and add asserts to > >> qemu_file_transferred* functions. > >> > >> Signed-off-by: Juan Quintela > > > > The read side accounting does look a bit weird and never caught my notice.. > > > > Maybe worth also touching the document of QEMUFile::total_transferred to > > clarify what it accounts? > > > > Reviewed-by: Peter Xu > > > > Though when I'm looking at the counters (didn't follow every single recent > > patch on this..), I found that now reading transferred value is actually > > more expensive - qemu_file_transferred() needs flushing, even if for the > > fast version, qemu_file_transferred_fast() loops over all possible iovs, > > which can be as large as MAX_IOV_SIZE==64. > > > > To be explicit, I _think_ for each guest page we now need to flush... > > > > ram_save_iterate > > migration_rate_exceeded > > migration_transferred_bytes > > qemu_file_transferred > > > > I hope I'm wrong.. > > See patch 7: > > diff --git a/migration/migration-stats.c b/migration/migration-stats.c > index 79eea8d865..1696185694 100644 > --- a/migration/migration-stats.c > +++ b/migration/migration-stats.c > @@ -62,7 +62,7 @@ uint64_t migration_transferred_bytes(QEMUFile *f) > { > uint64_t multifd = stat64_get(&mig_stats.multifd_bytes); > uint64_t rdma = stat64_get(&mig_stats.rdma_bytes); > -uint64_t qemu_file = qemu_file_transferred(f); > +uint64_t qemu_file = stat64_get(&mig_stats.qemu_file_transferred); > > trace_migration_transferred_bytes(qemu_file, multifd, rdma); > return qemu_file + multifd + rdma; If this is a known regression, should we make a first patchset fix it and make it higher priority to merge? It seems this is even not mentioned in the cover letter.. while IMHO this is the most important bit to have in it.. > > > > Does it mean that perhaps we simply need "sent and put into send buffer" > > more than "what really got transferred"? So I start to wonder what's the > > origianl purpose of this change, and which one is better.. > > That is basically what patch 5 and 6 do O:-) > > Problem is arriving to something that is bisectable (for correctness) > and is easy to review. > > And yes, my choices can be different from the ones tat you do. > > The other reason for the small patches is that: > a - sometimes I found a different test where things broke, and have to > bisect > b - small patches are much easier to rebase (that I am doing a lot) That's okay. Thanks, -- Peter Xu
Re: [PATCH v2 06/20] qemu_file: total_transferred is not used anymore
On Tue, May 30, 2023 at 08:39:27PM +0200, Juan Quintela wrote: > Signed-off-by: Juan Quintela > --- > migration/qemu-file.c | 4 > 1 file changed, 4 deletions(-) > > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > index eb0497e532..6b6deea19b 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -41,9 +41,6 @@ struct QEMUFile { > QIOChannel *ioc; > bool is_writable; > > -/* The sum of bytes transferred on the wire */ > -uint64_t total_transferred; > - > int buf_index; > int buf_size; /* 0 when writing */ > uint8_t buf[IO_BUF_SIZE]; > @@ -287,7 +284,6 @@ void qemu_fflush(QEMUFile *f) > qemu_file_set_error_obj(f, -EIO, local_error); > } else { > uint64_t size = iov_size(f->iov, f->iovcnt); > -f->total_transferred += size; I think this patch is another example why I think sometimes the way patch is split are pretty much adding more complexity on review... Here we removed a variable operation but it seems all fine if it's not used anywhere. But it also means current code base (before this patch applied) doesn't make sense already because it contains this useless addition. So IMHO it means some previous patch does it just wrong. I think it means it's caused by a wrong split of patches, then each patch stops to make much sense as a standalone one. I can go back and try to find whatever patch on the list that will explain this. But it'll also go into git log. Anyone reads this later will be confused once again. Even harder for them to figure out what happened. Do you think we could reorganize the patches so each of a single patch explains itself? The other thing is about priority of patches - I still have ~80 patches pending reviews on migration only.. Would you think it makes sense we pick up important ones first and merge them with higher priority? What I have in mind are: - The regression you mentioned on qemu_fflush() when ram save (I hope I understand the problem right, though...). - The slowness of migration-test. I'm not sure whether you have anything better than either Dan's approach or the switchover-hold flags, I just think that seems more important to resolve for us upstream. We can merge Dan's or mine, you can also propose something else, but IMHO that seems to be a higher priority? And whatever else I haven't noticed. I'll continue reading but I'm sure you know the best on this.. so I'd really rely on you. What do you think? Thanks, -- Peter Xu
Re: [PATCH v2 16/20] migration/rdma: Split qemu_fopen_rdma() into input/output functions
On Tue, May 30, 2023 at 08:39:37PM +0200, Juan Quintela wrote: > This is how everything else in QEMUFile is structured. > As a bonus they are three less lines of code. > > Signed-off-by: Juan Quintela > --- > migration/rdma.c | 35 --- > 1 file changed, 16 insertions(+), 19 deletions(-) > > diff --git a/migration/rdma.c b/migration/rdma.c > index 3ef35fc635..606837bd45 100644 > --- a/migration/rdma.c > +++ b/migration/rdma.c > @@ -4046,25 +4046,22 @@ static void qio_channel_rdma_register_types(void) > > type_init(qio_channel_rdma_register_types); > > -static QEMUFile *qemu_fopen_rdma(RDMAContext *rdma, const char *mode) > +static QEMUFile *rdma_new_input(RDMAContext *rdma) > { > -QIOChannelRDMA *rioc; > +QIOChannelRDMA *rioc = > QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA)); > +rioc->file = qemu_file_new_input(QIO_CHANNEL(rioc)); > +rioc->rdmain = rdma; > +rioc->rdmaout = rdma->return_path; > > -if (qemu_file_mode_is_not_valid(mode)) { We can also drop this function together. If with that: Reviewed-by: Peter Xu > -return NULL; > -} > +return rioc->file; > +} > > -rioc = QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA)); > - > -if (mode[0] == 'w') { > -rioc->file = qemu_file_new_output(QIO_CHANNEL(rioc)); > -rioc->rdmaout = rdma; > -rioc->rdmain = rdma->return_path; > -} else { > -rioc->file = qemu_file_new_input(QIO_CHANNEL(rioc)); > -rioc->rdmain = rdma; > -rioc->rdmaout = rdma->return_path; > -} > +static QEMUFile *rdma_new_output(RDMAContext *rdma) > +{ > +QIOChannelRDMA *rioc = > QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA)); > +rioc->file = qemu_file_new_output(QIO_CHANNEL(rioc)); > +rioc->rdmaout = rdma; > +rioc->rdmain = rdma->return_path; > > return rioc->file; > } > @@ -4090,9 +4087,9 @@ static void rdma_accept_incoming_migration(void *opaque) > return; > } > > -f = qemu_fopen_rdma(rdma, "rb"); > +f = rdma_new_input(rdma); > if (f == NULL) { > -fprintf(stderr, "RDMA ERROR: could not qemu_fopen_rdma\n"); > +fprintf(stderr, "RDMA ERROR: could not open RDMA for input\n"); > qemu_rdma_cleanup(rdma); > return; > } > @@ -4217,7 +4214,7 @@ void rdma_start_outgoing_migration(void *opaque, > trace_rdma_start_outgoing_migration_after_rdma_connect(); > > s->rdma_migration = true; > -s->to_dst_file = qemu_fopen_rdma(rdma, "wb"); > +s->to_dst_file = rdma_new_output(rdma); > migrate_fd_connect(s, NULL); > return; > return_path_err: > -- > 2.40.1 > -- Peter Xu
Re: [PATCH v2 18/20] qemu_file: Make qemu_file_is_writable() static
On Tue, May 30, 2023 at 08:39:39PM +0200, Juan Quintela wrote: > It is not used outside of qemu_file, and it shouldn't. > > Signed-off-by: Juan Quintela Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH v2 20/20] qemu-file: Make qemu_file_get_error_obj() static
On Tue, May 30, 2023 at 08:39:41PM +0200, Juan Quintela wrote: > It was not used outside of qemu_file.c anyways. > > Signed-off-by: Juan Quintela Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH v2 19/20] qemu-file: Simplify qemu_file_shutdown()
On Tue, May 30, 2023 at 08:39:40PM +0200, Juan Quintela wrote: > Signed-off-by: Juan Quintela Reviewed-by: Peter Xu -- Peter Xu
Re: [RFC 4/6] migration: Deprecate -incoming
On Tue, Jun 20, 2023 at 01:10:55PM +0100, Daniel P. Berrangé wrote: > In some cases it is worth having a convenience option for user friendliness. > > In this case, however, users are already needing to use QMP/HMP on the > source side to set migration parameters. I think it is reasonable to say > that doing *exactly* the same with QMP/HMP on the destination side is > actually more friendly than requiring use of -global on the dest side > which is different syntax. The -global parameters also work for the src side for my scripts, so I only need to attach "-incoming tcp:XXX" for dst cmdline here. > > We don't document the way to use -global with migration parameters so > when people see '-incoming' I think we are steering them into a trap, > making it look like -incoming is sufficient on its own. Hence that user's > mistake recently about not setting migration parameters. > > Overall I agree with deprecating '-incoming' != 'defer', as I think it > will better guide users to following the correct steps, as is not a > usability burden as they're already using QMP/HMP on the source side. I'd say -global is definitely not for users but for developers. Just like we keep around HMP - I'm not sure whether it's used in productions, I thought it was only for developers and we don't deprecate it eagerly. No strong opinions here if everyone wants to get rid of that, but before that I hope we can have some kind of cmdline tool that can easily tunnel qmp commands so whoever developers using pure cmdlines can switch to the new way easily. Thanks, -- Peter Xu
Re: [PATCH v2 06/20] qemu_file: total_transferred is not used anymore
On Thu, Jun 22, 2023 at 01:05:49AM +0200, Juan Quintela wrote: > Peter Xu wrote: > > On Tue, May 30, 2023 at 08:39:27PM +0200, Juan Quintela wrote: > >> Signed-off-by: Juan Quintela > >> --- > >> migration/qemu-file.c | 4 > >> 1 file changed, 4 deletions(-) > >> > >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c > >> index eb0497e532..6b6deea19b 100644 > >> --- a/migration/qemu-file.c > >> +++ b/migration/qemu-file.c > >> @@ -41,9 +41,6 @@ struct QEMUFile { > >> QIOChannel *ioc; > >> bool is_writable; > >> > >> -/* The sum of bytes transferred on the wire */ > >> -uint64_t total_transferred; > >> - > >> int buf_index; > >> int buf_size; /* 0 when writing */ > >> uint8_t buf[IO_BUF_SIZE]; > >> @@ -287,7 +284,6 @@ void qemu_fflush(QEMUFile *f) > >> qemu_file_set_error_obj(f, -EIO, local_error); > >> } else { > >> uint64_t size = iov_size(f->iov, f->iovcnt); > >> -f->total_transferred += size; > > > > I think this patch is another example why I think sometimes the way patch > > is split are pretty much adding more complexity on review... > > It depends of taste. > > You are doing one thing in way1. > Then you find a better way to do it, lets call it way2. > > Now we have two options to see how we arrived there. > > a- You got any declarations/definition/initializations for way2 > b- You write way2 alongside way1 > c- You test that both ways give the same result, and you see that they >give the same result. > d- you remove the way1. > > Or you squash the four patches in a single patch. But then the reviewer > lost the place where one can see why it is the same than the old one. For this patch to remove total_transferred, IMHO as a reviewer it'll be easier to me if it's put in the same patch where it got replaced. It might be different if we're going to remove a large chunk of code, but for this patch it's a few lines of change. > > Sometimes is better the longer way, sometimes is better the short one. > > Clearly we don't agree about what is the best way in this case. > > > Here we removed a variable operation but it seems all fine if it's not used > > anywhere. But it also means current code base (before this patch applied) > > doesn't make sense already because it contains this useless addition. So > > IMHO it means some previous patch does it just wrong. > > No. It is how it is developed. And being respectful with the > reviewer. Given it enough information to do a proper review. I never doubted about that. I trust that you were trying to provide the best for a reviewer and I appreciate that. > > During the development of this series, there were lots of: > > if (old_counter != new_counter) >printf(""); I assume you meant when debugging? I may not have read all the patches; I hope we just don't do that if possible in any real git commit. > > traces were in the several thousand lines long. If I have to review > that change, I would love any help that writer can give me. That is why > it is done this way. Yeah, I think it's probably that even from reviewers' perspective the need can be different from individuals. > > > I think it means it's caused by a wrong split of patches, then each patch > > stops to make much sense as a standalone one. > > It stops making sense if you want each feature to be a single patch. > Before the patch no feature. After the patch full feature. That brings > us to very long patches. > > What is easier to review (to do the same) > > a - 1 x 1000 lines patch > b - 10 x 100 lines patch > > I will go with b any time. Except if the split is arbitrary. AFAIU, this is a different thing. I'm never against splitting patch, but about how to split. I was never asking for a 1000 LOC patch, right? :) > > > I can go back and try to find whatever patch on the list that will explain > > this. But it'll also go into git log. Anyone reads this later will be > > confused once again. Even harder for them to figure out what > > happened. > > As said before, I completely disagree here. And what is worse. If it > gets wrong, with your approach git bisect will not help as much than > with my appreach. > > > Do you think we could reorganize the patches so each of a single patch > > explains itself? > > No. See before. We go for a very spaguetti code to a much less > spaguety code. >
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 12:01:55PM +0200, Juan Quintela wrote: > Paolo Bonzini wrote: > > On 6/22/23 10:52, Juan Quintela wrote: > >> User friendliness. > >> The problem is that if you use more than two channels with multifd, on > >> the incoming side, you need to do: > > > > You're sacrificing user-friendliness for the 99.99% that don't use > > multifd, for an error (i.e. it's not even fixing the issue) for the > > 0.01% that use multifd. That's not user-friendly. > > You are forgeting of the 0.01% that uses postocopy preempt (that is easy > just changing the default value to 2), and the 0.001% that uses > compression (they have exactly the same problem with compress_threads, > compress_zlib, etc). > > >> - migrate_set_parameter multifd-channels 16 > >> - migrate_incoming > >> > >>> The issue is not how many features the command line has, but how > >>> they're implemented. > >> Or if they are confusing for the user? > > > > Anyone using multifd is not a typical user anyway. > > >>> If they're just QMP wrappers and as such they're self-contained in > >>> softmmu/vl.c, that's fine. > >>> > >>> In fact, even for parameters, we could use keyval to parse "-incoming" > >> What is keyval? > > > > util/keyval.c and include/qemu/keyval.h. It parses a list of > > key=value pairs into a QDict. Once you have removed the "source" key > > from the QDict you can use a visitor to parse the rest into a > > MigrateSetParameters. See the handling of QEMU_OPTION_audio, it could > > be something like > > > > > > case QEMU_OPTION_incoing: { > > Visitor *v; > > MigrateSetParameters *incoming_params = NULL; > > QDict *dict = keyval_parse(optarg, "source", NULL, > > &error_fatal); > > > > if (incoming) { > > if (qdict_haskey(dict, "source")) { > > error_setg(&error_fatal, "Parameter 'source' > > is duplicate"); > > } > > } else { > > if (!qdict_haskey(dict, "source")) { > > error_setg(&error_fatal, "Parameter 'source' > > is missing"); > > } > > runstate_set(RUN_STATE_INMIGRATE); > > incoming = g_strdup(qdict_get_str(dict, "source")); > > qdict_del(dict, "source"); > > } > > > > v = qobject_input_visitor_new_keyval(QOBJECT(dict)); > > qobject_unref(dict); > > visit_type_MigrateSetParameters(v, NULL, > > &incoming_params, &error_fatal); > > visit_free(v); > > qmp_migration_set_parameters(incoming_params, PS: we may want to postpone this to be later than migration_object_init(), when/if there's a real patch. > > &error_fatal); > > qapi_free_MigrateSetParameters(incoming_params); > > } > > > > > > For example "-incoming [source=]tcp:foo,multifd-channels=16" would > > desugar to > > > > migrate_set_parameter multifd-channels 16 > > migrate_incoming tcp:foo > > > > The only incompatibility is for people who are using "," in an URI, > > which is rare and only an issue for the "exec" protocol. If we worry on breaking anyone, we can apply the keyval parsing only when !exec. Not sure whether it matters a huge lot.. > > Aha, that makes sense. And will allow us to deprecate/remove the > --global migration.* stuff. We may still need a way to set the caps/params for src qemu?.. Thanks, -- Peter Xu
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 11:22:56AM +0200, Thomas Huth wrote: > Then simply forbid "migrate_set_parameter multifd-channels ..." if the uri > has been specified on the command line? Yeah, actually already in a pull (even though the pr may need a new one..): https://lore.kernel.org/r/20230622021320.66124-23-quint...@redhat.com -- Peter Xu
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 10:59:58AM +0100, Daniel P. Berrangé wrote: > On Thu, Jun 22, 2023 at 10:52:12AM +0200, Juan Quintela wrote: > > Paolo Bonzini wrote: > > > On 6/12/23 22:51, Juan Quintela wrote: > > >>> Shall we just leave it there? Or is deprecating it helps us in any > > >>> form? > > >> See the patches two weeks ago when people complained that lisen(.., num) > > >> was too low. And there are other parameters that work the same way > > >> (that I convenientely had forgotten). So the easiest way to get things > > >> right is to use "defer" always. Using -incoming "uri" should only be > > >> for people that "know what they are doing", so we had to ways to do it: > > >> - review all migration options and see which ones work without defer > > >>and document it > > >> - deprecate everything that is not defer. > > > > > > "-incoming " is literally the same as running "migrate-incoming" > > > as the first thing on the monitor: > > > > > > if (incoming) { > > > Error *local_err = NULL; > > > if (strcmp(incoming, "defer") != 0) { > > > qmp_migrate_incoming(incoming, &local_err); > > > if (local_err) { > > > error_reportf_err(local_err, "-incoming %s: ", incoming); > > > exit(1); > > > } > > > } > > > } else if (autostart) { > > > qmp_cont(NULL); > > > } > > > > > > It's the only piece of code which distinguishes "-incoming defer" from > > > "-incoming ". > > > > > > So I'm not sure what the problem would be with keeping it? > > > > User friendliness. > > > > First of all, I use it all the time. And I know that it is useful for > > developers. I was the one asking peter to implement -global > > migration.foo to be able to test multifd with it. > > > > The problem is that if you use more than two channels with multifd, on > > the incoming side, you need to do: > > > > - migrate_set_parameter multifd-channels 16 > > - migrate_incoming > > > > And people continue to do: > > > > - qemu -incoming > > - migrate_set_parameter multifd-channels 16 (on the command line) > > > > And they complain that it fails, because we are calling listen with the > > wrong value. > > IMHO if we want to improve user friendliness then arguing about use > of the CLI vs QMP for migration is completely missing the bigger > picture IMHO. > > I've mentioned several times before that the user should never need to > set this multifd-channels parameter (nor many other parameters) on the > destination in the first place. > > The QEMU migration stream should be changed to add a full > bi-directional handshake, with negotiation of most parameters. > IOW, the src QEMU should be configured with 16 channels, and > it should connect the primary control channel, and then directly > tell the dest that it wants to use 16 multifd channels. > > If we're expecting the user to pass this info across to the dest > manually we've already spectacularly failed wrt user friendliness. I can try to move the todo even higher. Trying to list the initial goals here: - One extra phase of handshake between src/dst (maybe the time to boost QEMU_VM_FILE_VERSION) before anything else happens. - Dest shouldn't need to apply any cap/param, it should get all from src. Dest still need to be setup with an URI and that should be all it needs. - Src shouldn't need to worry on the binary version of dst anymore as long as dest qemu supports handshake, because src can fetch it from dest. - Handshake can always fail gracefully if anything wrong happened, it normally should mean dest qemu is not compatible with src's setup (either machine, device, or migration configs) for whatever reason. Src should be able to get a solid error from dest if so. - Handshake protocol should always be self-bootstrap-able, it means when we change the handshake protocol it should always works with old binaries. - When src is newer it should be able to know what's missing on dest and skip the new bits. - When dst is newer it should all rely on src (which is older) and it should always understand src's language. - All !main channels need to be established later than the handshake - if we're going to do this anyway we probably should do it altogether to make channels named, so each channel used in migration needs to have a common header. Prepare to deprecate the old tricks of channel orderings. Does it look reasonable? Anything to add/remove? Thanks, -- Peter Xu
Re: [RFC 4/6] migration: Deprecate -incoming
On Thu, Jun 22, 2023 at 05:33:29PM +0100, Daniel P. Berrangé wrote: > On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote: > > I can try to move the todo even higher. Trying to list the initial goals > > here: > > > > - One extra phase of handshake between src/dst (maybe the time to boost > > QEMU_VM_FILE_VERSION) before anything else happens. > > > > - Dest shouldn't need to apply any cap/param, it should get all from src. > > Dest still need to be setup with an URI and that should be all it needs. > > > > - Src shouldn't need to worry on the binary version of dst anymore as long > > as dest qemu supports handshake, because src can fetch it from dest. > > I'm not sure that works in general. Even if we have a handshake and > bi-directional comms for live migration, we still haave the save/restore > to file codepath to deal with. The dst QEMU doesn't exist at the time > the save process is done, so we can't add logic to VMSate handling that > assumes knowledge of the dst version at time of serialization. My current thought was still based on a new cap or anything the user would need to specify first on both sides (but hopefully the last cap to set on dest). E.g. if with a new handshake cap we shouldn't set it on a exec: or file: protocol migration, and it should just fail on qmp_migrate() telling that the URI is not supported if the cap is set. Return path is definitely required here. > > > - Handshake can always fail gracefully if anything wrong happened, it > > normally should mean dest qemu is not compatible with src's setup (either > > machine, device, or migration configs) for whatever reason. Src should > > be able to get a solid error from dest if so. > > > > - Handshake protocol should always be self-bootstrap-able, it means when we > > change the handshake protocol it should always works with old binaries. > > > > - When src is newer it should be able to know what's missing on dest and > > skip the new bits. > > > > - When dst is newer it should all rely on src (which is older) and it > > should always understand src's language. > > I'm not convinced it can reliably self-bootstrap in a backwards > compatible manner, precisely because the current migration stream > has no handshake and only requires a unidirectional channel. Yes, please see above. I meant when we grow the handshake protocol we should make sure we don't need anything new to be setup either on src/dst of qemu. It won't apply to before-handshake binaries. > I don't think its possible for QEMU to validate that it has a fully > bi-directional channel, without adding timeouts to its detection which I > think we should strive to avoid. > > I don't think we actually need self-bootstrapping anyway. > > I think the mgmt app can just indicate the new v2 bi-directional > protocol when issuing the 'migrate' and 'migrate-incoming' > commands. This becomes trivial when Het's refactoring of the > migrate address QAPI is accepted: > > https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04851.html > > eg: > > { "execute": "migrate", > "arguments": { > "channels": [ { "channeltype": "main", > "addr": { "transport": "socket", "type": "inet", >"host": "10.12.34.9", > "port": "1050" } } ] } } > > note the 'channeltype' parameter here. If we declare the 'main' > refers to the existing migration protocol, then we merely need > to define a new 'channeltype' to use as an indicator for the > v2 migration handshake protocol. Using a new channeltype would also work at least on src qemu, but I'm not sure on how dest qemu would know that it needs a handshake in that case, because it knows nothing until the connection is established. Maybe we still need QEMU_VM_FILE_VERSION to be boosted at least in this case, so dest can read this at the very beginning, old binaries will fail immediately, new binaries will start to talk with v2 language. > > > - All !main channels need to be established later than the handshake - if > > we're going to do this anyway we probably should do it altogether to make > > channels named, so each channel used in migration needs to have a common > > header. Prepare to deprecate the old tricks of channel orderings. > > Once the primary channel involves a bi-directional handshake, >
Re: [RFC 4/6] migration: Deprecate -incoming
On Fri, Jun 23, 2023 at 08:17:46AM +0100, Daniel P. Berrangé wrote: > On Thu, Jun 22, 2023 at 03:20:01PM -0400, Peter Xu wrote: > > On Thu, Jun 22, 2023 at 05:33:29PM +0100, Daniel P. Berrangé wrote: > > > On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote: > > > > I can try to move the todo even higher. Trying to list the initial > > > > goals > > > > here: > > > > > > > > - One extra phase of handshake between src/dst (maybe the time to boost > > > > QEMU_VM_FILE_VERSION) before anything else happens. > > > > > > > > - Dest shouldn't need to apply any cap/param, it should get all from > > > > src. > > > > Dest still need to be setup with an URI and that should be all it > > > > needs. > > > > > > > > - Src shouldn't need to worry on the binary version of dst anymore as > > > > long > > > > as dest qemu supports handshake, because src can fetch it from dest. > > > > > > I'm not sure that works in general. Even if we have a handshake and > > > bi-directional comms for live migration, we still haave the save/restore > > > to file codepath to deal with. The dst QEMU doesn't exist at the time > > > the save process is done, so we can't add logic to VMSate handling that > > > assumes knowledge of the dst version at time of serialization. > > > > My current thought was still based on a new cap or anything the user would > > need to specify first on both sides (but hopefully the last cap to set on > > dest). > > > > E.g. if with a new handshake cap we shouldn't set it on a exec: or file: > > protocol migration, and it should just fail on qmp_migrate() telling that > > the URI is not supported if the cap is set. Return path is definitely > > required here. > > exec can support bi-directional migration - we have both stdin + stdout > for the command. For exec it is mostly a documentation problem - you > can't merely use 'cat' for example, but if you used 'socat' that could > be made to work bi-directionally. Okay. Just an example that the handshake just cannot work for all the cases, and it should always be able to fail. So when exec doesn't properly provide return path, I think with handshake=on we should get a timeout of not getting response properly and fail the migration after the timeout, then. There're a bunch of implications and details that need to be investigated around such a handshake if it'll be proposed for real, so I'm not yet sure whether there's something that may be surprising. For channeltypes it seems all fine for now. Hopefully nothing obvious overlooked. > > > > I don't think its possible for QEMU to validate that it has a fully > > > bi-directional channel, without adding timeouts to its detection which I > > > think we should strive to avoid. > > > > > > I don't think we actually need self-bootstrapping anyway. > > > > > > I think the mgmt app can just indicate the new v2 bi-directional > > > protocol when issuing the 'migrate' and 'migrate-incoming' > > > commands. This becomes trivial when Het's refactoring of the > > > migrate address QAPI is accepted: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04851.html > > > > > > eg: > > > > > > { "execute": "migrate", > > > "arguments": { > > > "channels": [ { "channeltype": "main", > > > "addr": { "transport": "socket", "type": "inet", > > > "host": "10.12.34.9", > > > "port": "1050" } } ] } } > > > > > > note the 'channeltype' parameter here. If we declare the 'main' > > > refers to the existing migration protocol, then we merely need > > > to define a new 'channeltype' to use as an indicator for the > > > v2 migration handshake protocol. > > > > Using a new channeltype would also work at least on src qemu, but I'm not > > sure on how dest qemu would know that it needs a handshake in that case, > > because it knows nothing until the connection is established. > > In Het's series the 'migrate_incoming' command similarly has a chaneltype > parameter. Oh, yeah then that'll just work. Thanks. -- Peter Xu
Re: [RFC 4/6] migration: Deprecate -incoming
On Fri, Jun 23, 2023 at 09:23:18AM +0100, Daniel P. Berrangé wrote: > On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote: > > On Thu, Jun 22, 2023 at 10:59:58AM +0100, Daniel P. Berrangé wrote: > > > I've mentioned several times before that the user should never need to > > > set this multifd-channels parameter (nor many other parameters) on the > > > destination in the first place. > > > > > > The QEMU migration stream should be changed to add a full > > > bi-directional handshake, with negotiation of most parameters. > > > IOW, the src QEMU should be configured with 16 channels, and > > > it should connect the primary control channel, and then directly > > > tell the dest that it wants to use 16 multifd channels. > > > > > > If we're expecting the user to pass this info across to the dest > > > manually we've already spectacularly failed wrt user friendliness. > > > > I can try to move the todo even higher. Trying to list the initial goals > > here: > > > > - One extra phase of handshake between src/dst (maybe the time to boost > > QEMU_VM_FILE_VERSION) before anything else happens. > > > > - Dest shouldn't need to apply any cap/param, it should get all from src. > > Dest still need to be setup with an URI and that should be all it needs. > > There are a few that the dest will still need set explicitly. Specifically > the TLS parameters - tls-authz and tls-creds, because those are both > related to --object parameters configured on the dst QEMU. Potentially > there's an argument to be made for the TLS parameters to be part fo the > initial 'migrate' and 'migrate-incoming' command data, as they're > specifically related to the connection establishment, while (most) of > the other params are related to the migration protocol running inside > the connection. Ideally we can even make tls options to be after the main connection is established, IOW the gnutls handshake can be part of the generic handshake. But yeah I agree that may contain much more work, so we may start with assuming the v2 handshake just happen on the tls channel built for now. I think the new protocol should allow extension so when we want to move the tls handshake into it v2 protocol should be able to first detect src/dst binary support of that, and switch to that if we want - then we can even got a src qemu migration failure which tells "dest qemu forget to setup tls credentials in cmdlines", or anything wrong on dest during tls setup. > > A few other parameters are also related to the connection establishment, > most notably the enablement multifd, postcopy and postcopy-pre-empt. As I mentioned in the list, I plan to make this part of the default v2 where v2 handshake will take care of managing the connections rather than relying on the old code. I'm not sure how complicated it'll be, but the v2 protocol just sounds a good fit for having such a major change on how we setup the channels, and chance we get all things alright from the start. > > I think with those ones we don't need to set them on the src either. > With the new migration handshake we should probably use multifd > codepaths unconditionally, with a single channel. The v2 handshake will be beneficial to !multifd as well. Right now I tend to make it also work for !multifd, e.g., it always makes sense to do a device tree comparision before migration, even if someone used special tunneling so multifd may not be able to be enabled for whatever reason, but as long as a return path is available so they can talk. > By matching with the introduction of new protocol, we have a nice point > against which to deprecate the old non-multifd codepaths. We'll need to > keep the non-multifd code around *alot* longer than the normal > deprecation cycle though, as we need mig to/from very old QEMUs. I actually had a feeling that we should always keep it.. I'm not sure whether we must combine a new handshake to "making multifd the default". I do think we can make the !multifd path very simple though, e.g., I'd think we should start considering deprecate things like !multifd+compressions etc earlier than that. > > The enablement of postcopy could be automatic too - src & dst can > both detect if their host OS supports it. That would make all > migrations post-copy capable. The mgmt app just needs to trigger > the switch to post-copy mode *if* they want to use it. Sounds doable. > > Likewise we can just always assume postcopy-pre-empt is available. > > I think 'return-path' becomes another one we can just assume too. Right, handshake cap (or with the new QAPI of URI replacement) should imply return path already. Thanks, -- Peter Xu
Re: [PATCH v2 1/5] migration: Use proper indentation for migration.json
On Thu, Jun 22, 2023 at 09:50:15PM +0200, Juan Quintela wrote: > We broke it with dirtyrate limit patches. > > Signed-off-by: Juan Quintela Acked-by: Peter Xu -- Peter Xu
Re: [PATCH v2 5/5] migration: Deprecate old compression method
On Thu, Jun 22, 2023 at 09:50:19PM +0200, Juan Quintela wrote: > Signed-off-by: Juan Quintela Acked-by: Peter Xu -- Peter Xu
Re: [PATCH v2 2/2] hw/pflash: implement update buffer for block writes
hat this is overlooked in the commit a19cbfb346 ("spice: add qxl device") where VMSTATE_VBUFFER_UINT32 is introduced. I don't have a good way to trivially fix it because we don't remember that type of size in vmsd. However a simple solution could be that we convert all existing VMSTATE_VBUFFER() (potentially, VMSTATE_PARTIAL_VBUFFER users; there seems to have only 1 caller..) to always use uint32_t. I don't think it's wise to try using a signed for size anyway, and it should be compatible change as we doubled the size. I'll hold a bit to see whether there's some comment, then I can try to post a patch. > + * that is in some other struct field, but it's a runtime constant and > + * we can assume the memory has already been allocated. > +*/ > + > #define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, > _field_size) { \ > @@ -688,2 +698,9 @@ extern const VMStateInfo vmstate_info_qlist; > > +/** > + * VMSTATE_VBUFFER_ALLOC_UINT32: > + * > + * We need to migrate an array of uint32_t of variable size dependent > + * on the inbound migration data, and so the migration code must > + * allocate it. > +*/ > #define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, \ > --- > -- Peter Xu
Re: [PATCH 00/15] qapi: Require member documentation (with loophole)
On Mon, Feb 05, 2024 at 08:46:54AM +0100, Markus Armbruster wrote: > qapi/migration.json > MigrateSetParameters 1 It's tls-authz. I'll send a patch for this one. Thanks, -- Peter Xu
Re: [PATCH for 9.0] migration: Skip only empty block devices
On Tue, Mar 12, 2024 at 01:04:31PM +0100, Cédric Le Goater wrote: > The block .save_setup() handler calls a helper routine > init_blk_migration() which builds a list of block devices to take into > account for migration. When one device is found to be empty (sectors > == 0), the loop exits and all the remaining devices are ignored. This > is a regression introduced when bdrv_iterate() was removed. > > Change that by skipping only empty devices. > > Cc: Markus Armbruster > Suggested: Kevin Wolf This should be: Suggested-by: Kevin Wolf I think the missed "by" caused Kevin not in the cc list, I added Kevin in. I'll hold a bit for Kevin to ACK, no repost needed just for above; I can fix it. Thanks, > Fixes: fea68bb6e9fa ("block: Eliminate bdrv_iterate(), use bdrv_next()") > Signed-off-by: Cédric Le Goater > --- > migration/block.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/migration/block.c b/migration/block.c > index > 8c6ebafacc1ffe930d1d4f19d968817b14852c69..2b9054889ad2ba739828594c50cf047703757e96 > 100644 > --- a/migration/block.c > +++ b/migration/block.c > @@ -402,7 +402,10 @@ static int init_blk_migration(QEMUFile *f) > } > > sectors = bdrv_nb_sectors(bs); > -if (sectors <= 0) { > +if (sectors == 0) { > +continue; > +} > +if (sectors < 0) { > ret = sectors; > bdrv_next_cleanup(&it); > goto out; > -- > 2.44.0 > > -- Peter Xu
Re: [PATCH for 9.0] migration: Skip only empty block devices
On Tue, Mar 12, 2024 at 05:34:26PM -0400, Stefan Hajnoczi wrote: > I understand now. I missed that returning from init_blk_migration_it() > did not abort iteration. Thank you! I queued it, thanks both! -- Peter Xu
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
On Thu, Mar 28, 2024 at 11:18:04AM -0300, Fabiano Rosas wrote: > Philippe Mathieu-Daudé writes: > > > The whole RDMA subsystem was deprecated in commit e9a54265f5 > > ("hw/rdma: Deprecate the pvrdma device and the rdma subsystem") > > released in v8.2. > > > > Remove: > > - RDMA handling from migration > > - dependencies on libibumad, libibverbs and librdmacm > > > > Keep the RAM_SAVE_FLAG_HOOK definition since it might appears > > in old migration streams. > > > > Cc: Peter Xu > > Cc: Li Zhijian > > Acked-by: Fabiano Rosas > > Signed-off-by: Philippe Mathieu-Daudé > > Just to be clear, because people raised the point in the last version, > the first link in the deprecation commit links to a thread comprising > entirely of rdma migration patches. I don't see any ambiguity on whether > the deprecation was intended to include migration. There's even an ack > from Juan. Yes I remember that's the plan. > > So on the basis of not reverting the previous maintainer's decision, my > Ack stands here. > > We also had pretty obvious bugs ([1], [2]) in the past that would have > been caught if we had any kind of testing for the feature, so I can't > even say this thing works currently. > > @Peter Xu, @Li Zhijian, what are your thoughts on this? Generally I definitely agree with such a removal sooner or later, as that's how deprecation works, and even after Juan's left I'm not aware of any other new RDMA users. Personally, I'd slightly prefer postponing it one more release which might help a bit of our downstream maintenance, however I assume that's not a blocker either, as I think we can also manage it. IMHO it's more important to know whether there are still users and whether they would still like to see it around. That's also one thing I notice that e9a54265f533f didn't yet get acks from RDMA users that we are aware, even if they're rare. According to [2] it could be that such user may only rely on the release versions of QEMU when it broke things. So I'm copying Yu too (while Zhijian is already in the loop), just in case someone would like to stand up and speak. Thanks, > > 1- https://lore.kernel.org/r/20230920090412.726725-1-lizhij...@fujitsu.com > 2- > https://lore.kernel.org/r/cahecvy7hxswn4ow_kog+q+tn6f_kmeichevz1qgm-fbxbpp...@mail.gmail.com > -- Peter Xu
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
On Thu, Mar 28, 2024 at 04:22:27PM +0100, Thomas Huth wrote: > Since e9a54265f5 was not very clear about rdma migration code, should we > maybe rather add a separate deprecation note for the migration part, and add > a proper warning message to the migration code in case someone tries to use > it there, and then only remove the rdma migration code after two more > releases? Definitely a valid option to me. So far RDMA isn't covered in tests (actually same to COLO, and I wonder our position of COLO too in this case..), so unfortunately we don't even know when it'll break just like before. >From other activities that I can see when new code comes, maintaining RDMA code should be fairly manageable so far (and whoever will write new rdma codes in those two releases will also need to take the maintainer's role). We did it for those years, and we can keep that for two more releases. Hopefully that can ring a louder alarm to the current users with such warnings, so that people can either stick with old binaries, or invest developer/test resources to the community. Thanks, -- Peter Xu
Re: [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives
On Thu, Mar 28, 2024 at 02:20:45PM +0400, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau > > ../migration/dirtyrate.c:186:5: error: ‘records’ may be used uninitialized > [-Werror=maybe-uninitialized] > ../migration/dirtyrate.c:168:12: error: ‘gen_id’ may be used uninitialized > [-Werror=maybe-uninitialized] > ../migration/migration.c:2273:5: error: ‘file’ may be used uninitialized > [-Werror=maybe-uninitialized] > > Signed-off-by: Marc-André Lureau Acked-by: Peter Xu -- Peter Xu