| Message ID | 1714406135-451286-8-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Live update: cpr-exec | expand |
Steve Sistare <steven.sistare@oracle.com> writes: > Define a type for the 256 byte id string to guarantee the same length is > used and enforced everywhere. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
On Mon, Apr 29, 2024 at 08:55:16AM -0700, Steve Sistare wrote: > Define a type for the 256 byte id string to guarantee the same length is > used and enforced everywhere. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > include/exec/ramblock.h | 3 ++- > include/migration/vmstate.h | 2 ++ > migration/savevm.c | 8 ++++---- > migration/vmstate.c | 3 ++- > 4 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h > index 0babd10..61deefe 100644 > --- a/include/exec/ramblock.h > +++ b/include/exec/ramblock.h > @@ -23,6 +23,7 @@ > #include "cpu-common.h" > #include "qemu/rcu.h" > #include "exec/ramlist.h" > +#include "migration/vmstate.h" > > struct RAMBlock { > struct rcu_head rcu; > @@ -35,7 +36,7 @@ struct RAMBlock { > void (*resized)(const char*, uint64_t length, void *host); > uint32_t flags; > /* Protected by the BQL. */ > - char idstr[256]; > + VMStateId idstr; > /* RCU-enabled, writes protected by the ramlist lock */ > QLIST_ENTRY(RAMBlock) next; > QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers; Hmm.. Don't look like a good idea to include a migration header in ramblock.h? Is this ramblock change needed for this work?
On 5/27/2024 2:20 PM, Peter Xu wrote: > On Mon, Apr 29, 2024 at 08:55:16AM -0700, Steve Sistare wrote: >> Define a type for the 256 byte id string to guarantee the same length is >> used and enforced everywhere. >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> include/exec/ramblock.h | 3 ++- >> include/migration/vmstate.h | 2 ++ >> migration/savevm.c | 8 ++++---- >> migration/vmstate.c | 3 ++- >> 4 files changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h >> index 0babd10..61deefe 100644 >> --- a/include/exec/ramblock.h >> +++ b/include/exec/ramblock.h >> @@ -23,6 +23,7 @@ >> #include "cpu-common.h" >> #include "qemu/rcu.h" >> #include "exec/ramlist.h" >> +#include "migration/vmstate.h" >> >> struct RAMBlock { >> struct rcu_head rcu; >> @@ -35,7 +36,7 @@ struct RAMBlock { >> void (*resized)(const char*, uint64_t length, void *host); >> uint32_t flags; >> /* Protected by the BQL. */ >> - char idstr[256]; >> + VMStateId idstr; >> /* RCU-enabled, writes protected by the ramlist lock */ >> QLIST_ENTRY(RAMBlock) next; >> QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers; > > Hmm.. Don't look like a good idea to include a migration header in > ramblock.h? Is this ramblock change needed for this work? Well, entities that are migrated include migration headers, and now that includes RAMBlock. There is precedent: 0 include/exec/ramblock.h 26 #include "migration/vmstate.h" 1 include/hw/acpi/ich9_tco. 14 #include "migration/vmstate.h" 2 include/hw/display/ramfb. 4 #include "migration/vmstate.h" 3 include/hw/hyperv/vmbus.h 16 #include "migration/vmstate.h" 4 include/hw/input/pl050.h 14 #include "migration/vmstate.h" 5 include/hw/pci/shpc.h 7 #include "migration/vmstate.h" 6 include/hw/virtio/virtio. 20 #include "migration/vmstate.h" 7 include/migration/cpu.h 8 #include "migration/vmstate.h" Granted, only some of the C files that include ramblock.h need all of vmstate.h. I could define VMStateId in a smaller file such as migration/misc.h, or a new file migration/vmstateid.h, and include that in ramblock.h. Any preference? - Steve
On Tue, May 28, 2024 at 11:10:03AM -0400, Steven Sistare via wrote: > On 5/27/2024 2:20 PM, Peter Xu wrote: > > On Mon, Apr 29, 2024 at 08:55:16AM -0700, Steve Sistare wrote: > > > Define a type for the 256 byte id string to guarantee the same length is > > > used and enforced everywhere. > > > > > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > > > --- > > > include/exec/ramblock.h | 3 ++- > > > include/migration/vmstate.h | 2 ++ > > > migration/savevm.c | 8 ++++---- > > > migration/vmstate.c | 3 ++- > > > 4 files changed, 10 insertions(+), 6 deletions(-) > > > > > > diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h > > > index 0babd10..61deefe 100644 > > > --- a/include/exec/ramblock.h > > > +++ b/include/exec/ramblock.h > > > @@ -23,6 +23,7 @@ > > > #include "cpu-common.h" > > > #include "qemu/rcu.h" > > > #include "exec/ramlist.h" > > > +#include "migration/vmstate.h" > > > struct RAMBlock { > > > struct rcu_head rcu; > > > @@ -35,7 +36,7 @@ struct RAMBlock { > > > void (*resized)(const char*, uint64_t length, void *host); > > > uint32_t flags; > > > /* Protected by the BQL. */ > > > - char idstr[256]; > > > + VMStateId idstr; > > > /* RCU-enabled, writes protected by the ramlist lock */ > > > QLIST_ENTRY(RAMBlock) next; > > > QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers; > > > > Hmm.. Don't look like a good idea to include a migration header in > > ramblock.h? Is this ramblock change needed for this work? > > Well, entities that are migrated include migration headers, and now that > includes RAMBlock. There is precedent: > > 0 include/exec/ramblock.h 26 #include "migration/vmstate.h" > 1 include/hw/acpi/ich9_tco. 14 #include "migration/vmstate.h" > 2 include/hw/display/ramfb. 4 #include "migration/vmstate.h" > 3 include/hw/hyperv/vmbus.h 16 #include "migration/vmstate.h" > 4 include/hw/input/pl050.h 14 #include "migration/vmstate.h" > 5 include/hw/pci/shpc.h 7 #include "migration/vmstate.h" > 6 include/hw/virtio/virtio. 20 #include "migration/vmstate.h" > 7 include/migration/cpu.h 8 #include "migration/vmstate.h" > > Granted, only some of the C files that include ramblock.h need all of vmstate.h. > I could define VMStateId in a smaller file such as migration/misc.h, or a > new file migration/vmstateid.h, and include that in ramblock.h. > Any preference? One issue here is currently the idstr[] of ramblock is a verbose name of the ramblock, and logically it doesn't need to have anything to do with vmstate. I'll continue to read to see why we need VMStateID here for a ramblock. So if it is necessary, maybe the name VMStateID to be used here is misleading? It'll also be nice to separate the changes, as ramblock.h doesn't belong to migration subsystem but memory api. Thanks,
On 5/28/2024 1:44 PM, Peter Xu wrote: > On Tue, May 28, 2024 at 11:10:03AM -0400, Steven Sistare via wrote: >> On 5/27/2024 2:20 PM, Peter Xu wrote: >>> On Mon, Apr 29, 2024 at 08:55:16AM -0700, Steve Sistare wrote: >>>> Define a type for the 256 byte id string to guarantee the same length is >>>> used and enforced everywhere. >>>> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>>> --- >>>> include/exec/ramblock.h | 3 ++- >>>> include/migration/vmstate.h | 2 ++ >>>> migration/savevm.c | 8 ++++---- >>>> migration/vmstate.c | 3 ++- >>>> 4 files changed, 10 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h >>>> index 0babd10..61deefe 100644 >>>> --- a/include/exec/ramblock.h >>>> +++ b/include/exec/ramblock.h >>>> @@ -23,6 +23,7 @@ >>>> #include "cpu-common.h" >>>> #include "qemu/rcu.h" >>>> #include "exec/ramlist.h" >>>> +#include "migration/vmstate.h" >>>> struct RAMBlock { >>>> struct rcu_head rcu; >>>> @@ -35,7 +36,7 @@ struct RAMBlock { >>>> void (*resized)(const char*, uint64_t length, void *host); >>>> uint32_t flags; >>>> /* Protected by the BQL. */ >>>> - char idstr[256]; >>>> + VMStateId idstr; >>>> /* RCU-enabled, writes protected by the ramlist lock */ >>>> QLIST_ENTRY(RAMBlock) next; >>>> QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers; >>> >>> Hmm.. Don't look like a good idea to include a migration header in >>> ramblock.h? Is this ramblock change needed for this work? >> >> Well, entities that are migrated include migration headers, and now that >> includes RAMBlock. There is precedent: >> >> 0 include/exec/ramblock.h 26 #include "migration/vmstate.h" >> 1 include/hw/acpi/ich9_tco. 14 #include "migration/vmstate.h" >> 2 include/hw/display/ramfb. 4 #include "migration/vmstate.h" >> 3 include/hw/hyperv/vmbus.h 16 #include "migration/vmstate.h" >> 4 include/hw/input/pl050.h 14 #include "migration/vmstate.h" >> 5 include/hw/pci/shpc.h 7 #include "migration/vmstate.h" >> 6 include/hw/virtio/virtio. 20 #include "migration/vmstate.h" >> 7 include/migration/cpu.h 8 #include "migration/vmstate.h" >> >> Granted, only some of the C files that include ramblock.h need all of vmstate.h. >> I could define VMStateId in a smaller file such as migration/misc.h, or a >> new file migration/vmstateid.h, and include that in ramblock.h. >> Any preference? > > One issue here is currently the idstr[] of ramblock is a verbose name of > the ramblock, and logically it doesn't need to have anything to do with > vmstate. > > I'll continue to read to see why we need VMStateID here for a ramblock. So > if it is necessary, maybe the name VMStateID to be used here is misleading? > It'll also be nice to separate the changes, as ramblock.h doesn't belong to > migration subsystem but memory api. cpr requires migrating vmstate for ramblock. See the physmem patches for why. vmstate requires a unique id, and ramblock already defines a unique id which is used to identify the block and dirty bitmap in the migration stream. How about a more general name for the type: migration/misc.h typedef char (MigrationId)[256]; exec/ramblock.h struct RAMBlock { MigrationId idstr; migration/savevm.c typedef struct CompatEntry { MigrationId idstr; typedef struct SaveStateEntry { MigrationId idstr; - Steve
On Wed, May 29, 2024 at 01:30:18PM -0400, Steven Sistare wrote: > How about a more general name for the type: > > migration/misc.h > typedef char (MigrationId)[256]; How about qemu/typedefs.h? Not sure whether it's applicable. Markus (in the loop) may have a better idea. Meanwhile, s/MigrationID/IDString/? > > exec/ramblock.h > struct RAMBlock { > MigrationId idstr; > > migration/savevm.c > typedef struct CompatEntry { > MigrationId idstr; > > typedef struct SaveStateEntry { > MigrationId idstr; > > > - Steve >
On 5/29/2024 2:53 PM, Peter Xu wrote: > On Wed, May 29, 2024 at 01:30:18PM -0400, Steven Sistare wrote: >> How about a more general name for the type: >> >> migration/misc.h >> typedef char (MigrationId)[256]; > > How about qemu/typedefs.h? Not sure whether it's applicable. Markus (in > the loop) may have a better idea. > > Meanwhile, s/MigrationID/IDString/? typedefs.h has a different purpose; giving short names to types defined in internal include files. This id is specific to migration, so I still think its name should reflect migration and it belongs in some include/migration/*.h file. ramblocks and migration are already closely related. There is nothing wrong with including a migration header in ramblock.h so it can use a migration type. We already have: include/hw/acpi/ich9_tco.h:#include "migration/vmstate.h" include/hw/display/ramfb.h:#include "migration/vmstate.h" include/hw/input/pl050.h:#include "migration/vmstate.h" include/hw/pci/shpc.h:#include "migration/vmstate.h" include/hw/virtio/virtio.h:#include "migration/vmstate.h" include/hw/hyperv/vmbus.h:#include "migration/vmstate.h" The 256 byte magic length already appears in too many places, and my code would add more occurrences, so I really think that abstracting this type would be cleaner. - Steve >> exec/ramblock.h >> struct RAMBlock { >> MigrationId idstr; >> >> migration/savevm.c >> typedef struct CompatEntry { >> MigrationId idstr; >> >> typedef struct SaveStateEntry { >> MigrationId idstr; >> >> >> - Steve >> >
On Thu, May 30, 2024 at 01:11:26PM -0400, Steven Sistare wrote: > On 5/29/2024 2:53 PM, Peter Xu wrote: > > On Wed, May 29, 2024 at 01:30:18PM -0400, Steven Sistare wrote: > > > How about a more general name for the type: > > > > > > migration/misc.h > > > typedef char (MigrationId)[256]; > > > > How about qemu/typedefs.h? Not sure whether it's applicable. Markus (in > > the loop) may have a better idea. > > > > Meanwhile, s/MigrationID/IDString/? > > typedefs.h has a different purpose; giving short names to types > defined in internal include files. > > This id is specific to migration, so I still think its name should reflect > migration and it belongs in some include/migration/*.h file. > > ramblocks and migration are already closely related. There is nothing wrong > with including a migration header in ramblock.h so it can use a migration type. > We already have: > include/hw/acpi/ich9_tco.h:#include "migration/vmstate.h" > include/hw/display/ramfb.h:#include "migration/vmstate.h" > include/hw/input/pl050.h:#include "migration/vmstate.h" > include/hw/pci/shpc.h:#include "migration/vmstate.h" > include/hw/virtio/virtio.h:#include "migration/vmstate.h" > include/hw/hyperv/vmbus.h:#include "migration/vmstate.h" > > The 256 byte magic length already appears in too many places, and my code > would add more occurrences, so I really think that abstracting this type > would be cleaner. I agree having a typedef is nicer, but I don't understand why it must be migration related. It can be the type QEMU uses to represent any string based ID, and that's a generic concept to me. Migration can have a wrapper to process that type, but then migration will include the generic header in that case, it feels more natural that way?
diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h index 0babd10..61deefe 100644 --- a/include/exec/ramblock.h +++ b/include/exec/ramblock.h @@ -23,6 +23,7 @@ #include "cpu-common.h" #include "qemu/rcu.h" #include "exec/ramlist.h" +#include "migration/vmstate.h" struct RAMBlock { struct rcu_head rcu; @@ -35,7 +36,7 @@ struct RAMBlock { void (*resized)(const char*, uint64_t length, void *host); uint32_t flags; /* Protected by the BQL. */ - char idstr[256]; + VMStateId idstr; /* RCU-enabled, writes protected by the ramlist lock */ QLIST_ENTRY(RAMBlock) next; QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers; diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 4691334..a39c0e6 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -1210,6 +1210,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque); +typedef char (VMStateId)[256]; + #define VMSTATE_INSTANCE_ID_ANY -1 /* Returns: 0 on success, -1 on failure */ diff --git a/migration/savevm.c b/migration/savevm.c index a30bcd9..9b1a335 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -197,13 +197,13 @@ const VMStateInfo vmstate_info_timer = { typedef struct CompatEntry { - char idstr[256]; + VMStateId idstr; int instance_id; } CompatEntry; typedef struct SaveStateEntry { QTAILQ_ENTRY(SaveStateEntry) entry; - char idstr[256]; + VMStateId idstr; uint32_t instance_id; int alias_id; int version_id; @@ -814,7 +814,7 @@ int register_savevm_live(const char *idstr, void unregister_savevm(VMStateIf *obj, const char *idstr, void *opaque) { SaveStateEntry *se, *new_se; - char id[256] = ""; + VMStateId id = ""; if (obj) { char *oid = vmstate_if_get_id(obj); @@ -2650,7 +2650,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type) uint32_t instance_id, version_id, section_id; int64_t start_ts, end_ts; SaveStateEntry *se; - char idstr[256]; + VMStateId idstr; int ret; /* Read section start */ diff --git a/migration/vmstate.c b/migration/vmstate.c index ef26f26..437f156 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -471,7 +471,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, trace_vmstate_subsection_load(vmsd->name); while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) { - char idstr[256], *idstr_ret; + VMStateId idstr; + char *idstr_ret; int ret; uint8_t version_id, len, size; const VMStateDescription *sub_vmsd;
Define a type for the 256 byte id string to guarantee the same length is used and enforced everywhere. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- include/exec/ramblock.h | 3 ++- include/migration/vmstate.h | 2 ++ migration/savevm.c | 8 ++++---- migration/vmstate.c | 3 ++- 4 files changed, 10 insertions(+), 6 deletions(-)