| Message ID | 20170529135520.101429-5-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Although we have recently vmstatified the migration of some css > infrastructure, for some css entities there is still state to be > migrated left, because the focus was keeping migration stream > compatibility (that is basically everything as-is). > > Let us add vmstate helpers and extend existing vmstate descriptions so > that we have everything we need. Let us guard the added state with via > css_migration_enabled, so we keep the compatible behavior if css > migration is disabled. > > Let's also annotate the bits which do not need to be migrated for better > readability. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/intc/s390_flic.c | 20 +++++++++++++++ > hw/s390x/css.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 94 insertions(+) Reviewed-by: Juan Quintela <quintela@redhat.com> For the vmstate bits. I have exactly zero clue about s390 > +static const VMStateDescription vmstate_chp_info = { > + .name = "s390_chp_info", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8(in_use, ChpInfo), > + VMSTATE_UINT8(type, ChpInfo), > + VMSTATE_UINT8(is_virtual, ChpInfo), > + VMSTATE_END_OF_LIST() > + } > +}; > + > typedef struct SubchSet { > SubchDev *sch[MAX_SCHID + 1]; > unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)]; > @@ -215,6 +248,19 @@ typedef struct CssImage { > ChpInfo chpids[MAX_CHPID + 1]; > } CssImage; > > +static const VMStateDescription vmstate_css_img = { > + .name = "s390_css_img", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + /* Subchannel sets have no relevant state. */ > + VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0, > + vmstate_chp_info, ChpInfo), > + VMSTATE_END_OF_LIST() > + } > + > +}; > +static const VMStateDescription vmstate_css = { > + .name = "s390_css", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, vmstate_crw_container, > + CrwContainer, sibling), > + VMSTATE_BOOL(sei_pending, ChannelSubSys), > + VMSTATE_BOOL(do_crw_mchk, ChannelSubSys), > + VMSTATE_BOOL(crws_lost, ChannelSubSys), > + /* These were kind of migrated by virtio */ > + VMSTATE_UINT8(max_cssid, ChannelSubSys), > + VMSTATE_UINT8(max_ssid, ChannelSubSys), > + VMSTATE_BOOL(chnmon_active, ChannelSubSys), > + VMSTATE_UINT64(chnmon_area, ChannelSubSys), > + VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1, > + 0, vmstate_css_img, CssImage), I was about to suggest to move css from pointer to an embedded struct, and then noticed that MAX_CSSID is .... 65535. I guess that this is going to be sparse, very sparse. Perhaps there is an easier way to transmit it? You are transmiting: 65000 structs each of size MAX_CHPID (255) and each element is byte + byte +byte = 65000 * 255 * 3 = 47 MB. If it is really sparse, I think that sending something like a list of <index in array> <contents> could make sense, no? Or I am missunderstanding something? Later, Juan.
On 05/31/2017 08:52 PM, Juan Quintela wrote: > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >> Although we have recently vmstatified the migration of some css >> infrastructure, for some css entities there is still state to be >> migrated left, because the focus was keeping migration stream >> compatibility (that is basically everything as-is). >> >> Let us add vmstate helpers and extend existing vmstate descriptions so >> that we have everything we need. Let us guard the added state with via >> css_migration_enabled, so we keep the compatible behavior if css >> migration is disabled. >> >> Let's also annotate the bits which do not need to be migrated for better >> readability. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> hw/intc/s390_flic.c | 20 +++++++++++++++ >> hw/s390x/css.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 94 insertions(+) > > Reviewed-by: Juan Quintela <quintela@redhat.com> > Thanks! > > For the vmstate bits. I have exactly zero clue about s390 > >> +static const VMStateDescription vmstate_chp_info = { >> + .name = "s390_chp_info", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT8(in_use, ChpInfo), >> + VMSTATE_UINT8(type, ChpInfo), >> + VMSTATE_UINT8(is_virtual, ChpInfo), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> typedef struct SubchSet { >> SubchDev *sch[MAX_SCHID + 1]; >> unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)]; >> @@ -215,6 +248,19 @@ typedef struct CssImage { >> ChpInfo chpids[MAX_CHPID + 1]; >> } CssImage; >> >> +static const VMStateDescription vmstate_css_img = { >> + .name = "s390_css_img", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { >> + /* Subchannel sets have no relevant state. */ >> + VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0, >> + vmstate_chp_info, ChpInfo), >> + VMSTATE_END_OF_LIST() >> + } >> + >> +}; > >> +static const VMStateDescription vmstate_css = { >> + .name = "s390_css", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, vmstate_crw_container, >> + CrwContainer, sibling), >> + VMSTATE_BOOL(sei_pending, ChannelSubSys), >> + VMSTATE_BOOL(do_crw_mchk, ChannelSubSys), >> + VMSTATE_BOOL(crws_lost, ChannelSubSys), >> + /* These were kind of migrated by virtio */ >> + VMSTATE_UINT8(max_cssid, ChannelSubSys), >> + VMSTATE_UINT8(max_ssid, ChannelSubSys), >> + VMSTATE_BOOL(chnmon_active, ChannelSubSys), >> + VMSTATE_UINT64(chnmon_area, ChannelSubSys), >> + VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1, >> + 0, vmstate_css_img, CssImage), > > I was about to suggest to move css from pointer to an embedded struct, > and then noticed that MAX_CSSID is .... 65535. I guess that this is > going to be sparse, very sparse. Perhaps there is an easier way to > transmit it? > > You are transmiting: > > 65000 structs each of size MAX_CHPID (255) and each element is byte + > byte +byte = 65000 * 255 * 3 = 47 MB. > > If it is really sparse, I think that sending something like a list > of <index in array> <contents> could make sense, no? > > Or I am missunderstanding something? > I think you are misunderstanding. Sparse arrays of pointers were not supported in the first place. Support was added by me recentily (patch 07d4e69 "migration/vmstate: fix array of ptr with nullptrs"). For a null pointer just a single byte is transmitted. So we should be more like around 64KB, given that it is really sparse. Bottom line, the in stream representation is at least as efficient as the in memory representation. Of course we could do something special for sparse arrays of pointers in general, and indeed, one of the possibilities is a list/vector of non null indexes. I would like to ask the s390x maintainers (mainly Connie) about the particular case, and the migration maintainers (I spontaneously think of you Juan and Dave) about the general case (If we want to have something like VMSTATE_ARRAY_OF_POINTER_TO_STRUCT_SPARSE). Regards, Halil > Later, Juan. >
On Thu, 1 Jun 2017 11:35:27 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 05/31/2017 08:52 PM, Juan Quintela wrote: > > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> Although we have recently vmstatified the migration of some css > >> infrastructure, for some css entities there is still state to be > >> migrated left, because the focus was keeping migration stream > >> compatibility (that is basically everything as-is). > >> > >> Let us add vmstate helpers and extend existing vmstate descriptions so > >> that we have everything we need. Let us guard the added state with via > >> css_migration_enabled, so we keep the compatible behavior if css > >> migration is disabled. > >> > >> Let's also annotate the bits which do not need to be migrated for better > >> readability. > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >> --- > >> hw/intc/s390_flic.c | 20 +++++++++++++++ > >> hw/s390x/css.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 94 insertions(+) > > > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > > > Thanks! > > > > > For the vmstate bits. I have exactly zero clue about s390 > > > >> +static const VMStateDescription vmstate_chp_info = { > >> + .name = "s390_chp_info", > >> + .version_id = 1, > >> + .minimum_version_id = 1, > >> + .fields = (VMStateField[]) { > >> + VMSTATE_UINT8(in_use, ChpInfo), > >> + VMSTATE_UINT8(type, ChpInfo), > >> + VMSTATE_UINT8(is_virtual, ChpInfo), > >> + VMSTATE_END_OF_LIST() > >> + } > >> +}; > >> + > >> typedef struct SubchSet { > >> SubchDev *sch[MAX_SCHID + 1]; > >> unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)]; > >> @@ -215,6 +248,19 @@ typedef struct CssImage { > >> ChpInfo chpids[MAX_CHPID + 1]; > >> } CssImage; > >> > >> +static const VMStateDescription vmstate_css_img = { > >> + .name = "s390_css_img", > >> + .version_id = 1, > >> + .minimum_version_id = 1, > >> + .fields = (VMStateField[]) { > >> + /* Subchannel sets have no relevant state. */ > >> + VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0, > >> + vmstate_chp_info, ChpInfo), > >> + VMSTATE_END_OF_LIST() > >> + } > >> + > >> +}; > > > >> +static const VMStateDescription vmstate_css = { > >> + .name = "s390_css", > >> + .version_id = 1, > >> + .minimum_version_id = 1, > >> + .fields = (VMStateField[]) { > >> + VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, vmstate_crw_container, > >> + CrwContainer, sibling), > >> + VMSTATE_BOOL(sei_pending, ChannelSubSys), > >> + VMSTATE_BOOL(do_crw_mchk, ChannelSubSys), > >> + VMSTATE_BOOL(crws_lost, ChannelSubSys), > >> + /* These were kind of migrated by virtio */ > >> + VMSTATE_UINT8(max_cssid, ChannelSubSys), > >> + VMSTATE_UINT8(max_ssid, ChannelSubSys), > >> + VMSTATE_BOOL(chnmon_active, ChannelSubSys), > >> + VMSTATE_UINT64(chnmon_area, ChannelSubSys), > >> + VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1, > >> + 0, vmstate_css_img, CssImage), > > > > I was about to suggest to move css from pointer to an embedded struct, > > and then noticed that MAX_CSSID is .... 65535. I guess that this is > > going to be sparse, very sparse. Perhaps there is an easier way to > > transmit it? > > > > You are transmiting: > > > > 65000 structs each of size MAX_CHPID (255) and each element is byte + > > byte +byte = 65000 * 255 * 3 = 47 MB. > > > > If it is really sparse, I think that sending something like a list > > of <index in array> <contents> could make sense, no? > > > > Or I am missunderstanding something? > > > > I think you are misunderstanding. Sparse arrays of pointers were not > supported in the first place. Support was added by me recentily (patch > 07d4e69 "migration/vmstate: fix array of ptr with nullptrs"). For a null > pointer just a single byte is transmitted. So we should be more like > around 64KB, given that it is really sparse. Bottom line, the in stream > representation is at least as efficient as the in memory representation. > > Of course we could do something special for sparse arrays of pointers in > general, and indeed, one of the possibilities is a list/vector of non > null indexes. Just as discussion fodder: For a run-of-the-mill qemu guest, I'd expect a low double-digit number of subchannels in css 0xfe, in subchannel set 0, which are clustered right at the beginning, and one chpid (0) in css 0xfe. As the subchannel id is autogenerated based upon the devno (if provided), it is easy to have subchannels in an arbitrary subchannel set (of which there are only four), but to get high subchannel ids with a lot of empty slots before, you'd have to create a lot (really a lot) of devices and then detach most of them again - not a very likely pattern. The 3270 support does not really change much: one additional subchannel (with the same characteristics) and one further chpid. But I don't expect many people to use 3270, and it is not (yet) migratable anyway. vfio-ccw devices will be in another css (possibly mapped), but they are not migrateable either. So if we want to optimize, some "nothing of interest beyond that point" marker might be useful - but not very. > > I would like to ask the s390x maintainers (mainly Connie) about the > particular case, and the migration maintainers (I spontaneously think of > you Juan and Dave) about the general case (If we want to have something > like VMSTATE_ARRAY_OF_POINTER_TO_STRUCT_SPARSE). > > Regards, > Halil > > > Later, Juan. > >
On Mon, 29 May 2017 15:55:17 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Although we have recently vmstatified the migration of some css > infrastructure, for some css entities there is still state to be > migrated left, because the focus was keeping migration stream > compatibility (that is basically everything as-is). > > Let us add vmstate helpers and extend existing vmstate descriptions so > that we have everything we need. Let us guard the added state with via Either 'with' or 'via' ;) > css_migration_enabled, so we keep the compatible behavior if css > migration is disabled. > > Let's also annotate the bits which do not need to be migrated for better > readability. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/intc/s390_flic.c | 20 +++++++++++++++ > hw/s390x/css.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 94 insertions(+) > > diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c > index 7e7546a576..b889e74571 100644 > --- a/hw/intc/s390_flic.c > +++ b/hw/intc/s390_flic.c > @@ -139,6 +139,22 @@ static void qemu_s390_flic_register_types(void) > > type_init(qemu_s390_flic_register_types) > > +static bool adapter_info_so_needed(void *opaque) > +{ > + return css_migration_enabled(); > +} > + > +const VMStateDescription vmstate_adapter_info_so = { > + .name = "s390_adapter_info/summary_offset", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = adapter_info_so_needed, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(summary_offset, AdapterInfo), > + VMSTATE_END_OF_LIST() > + } > +}; > + > const VMStateDescription vmstate_adapter_info = { > .name = "s390_adapter_info", > .version_id = 1, > @@ -152,6 +168,10 @@ const VMStateDescription vmstate_adapter_info = { > */ > VMSTATE_END_OF_LIST() > }, > + .subsections = (const VMStateDescription * []) { > + &vmstate_adapter_info_so, > + NULL > + } > }; > > const VMStateDescription vmstate_adapter_routes = { > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 15517a16e4..a15b58d303 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -27,12 +27,45 @@ typedef struct CrwContainer { > QTAILQ_ENTRY(CrwContainer) sibling; > } CrwContainer; > > +static const VMStateDescription vmstate_crw = { > + .name = "s390_crw", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT16(flags, CRW), > + VMSTATE_UINT16(rsid, CRW), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > +static const VMStateDescription vmstate_crw_container = { > + .name = "s390_crw_container", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_STRUCT(crw, CrwContainer, 0, vmstate_crw, CRW), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > typedef struct ChpInfo { > uint8_t in_use; > uint8_t type; > uint8_t is_virtual; > } ChpInfo; > > +static const VMStateDescription vmstate_chp_info = { > + .name = "s390_chp_info", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8(in_use, ChpInfo), > + VMSTATE_UINT8(type, ChpInfo), > + VMSTATE_UINT8(is_virtual, ChpInfo), > + VMSTATE_END_OF_LIST() > + } > +}; > + > typedef struct SubchSet { > SubchDev *sch[MAX_SCHID + 1]; > unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)]; > @@ -215,6 +248,19 @@ typedef struct CssImage { > ChpInfo chpids[MAX_CHPID + 1]; > } CssImage; > > +static const VMStateDescription vmstate_css_img = { > + .name = "s390_css_img", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + /* Subchannel sets have no relevant state. */ > + VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0, > + vmstate_chp_info, ChpInfo), > + VMSTATE_END_OF_LIST() > + } > + > +}; > + > typedef struct IoAdapter { > uint32_t id; > uint8_t type; > @@ -232,10 +278,34 @@ typedef struct ChannelSubSys { > uint64_t chnmon_area; > CssImage *css[MAX_CSSID + 1]; > uint8_t default_cssid; > + /* don't migrate */ > IoAdapter *io_adapters[CSS_IO_ADAPTER_TYPE_NUMS][MAX_ISC + 1]; > + /* don't migrate */ > QTAILQ_HEAD(, IndAddr) indicator_addresses; > } ChannelSubSys; I think a couple of words as to _why_ they are not migrated would be nice. Imagine revisiting this in a few years :) > > +static const VMStateDescription vmstate_css = { > + .name = "s390_css", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, vmstate_crw_container, > + CrwContainer, sibling), > + VMSTATE_BOOL(sei_pending, ChannelSubSys), > + VMSTATE_BOOL(do_crw_mchk, ChannelSubSys), > + VMSTATE_BOOL(crws_lost, ChannelSubSys), > + /* These were kind of migrated by virtio */ > + VMSTATE_UINT8(max_cssid, ChannelSubSys), > + VMSTATE_UINT8(max_ssid, ChannelSubSys), > + VMSTATE_BOOL(chnmon_active, ChannelSubSys), > + VMSTATE_UINT64(chnmon_area, ChannelSubSys), > + VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1, > + 0, vmstate_css_img, CssImage), > + VMSTATE_UINT8(default_cssid, ChannelSubSys), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static ChannelSubSys channel_subsys = { > .pending_crws = QTAILQ_HEAD_INITIALIZER(channel_subsys.pending_crws), > .do_crw_mchk = true, > @@ -275,6 +345,10 @@ static int subch_dev_post_load(void *opaque, int version_id) > css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, s); > } > > + if (css_migration_enabled()) { > + /* No compat voodoo to do ;) */ > + return 0; > + } > /* > * Hack alert. If we don't migrate the channel subsystem status > * we still need to find out if the guest enabled mss/mcss-e. Other than the nits, Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
On 06/01/2017 01:32 PM, Cornelia Huck wrote: > On Thu, 1 Jun 2017 11:35:27 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> On 05/31/2017 08:52 PM, Juan Quintela wrote: >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >>>> Although we have recently vmstatified the migration of some css >>>> infrastructure, for some css entities there is still state to be >>>> migrated left, because the focus was keeping migration stream >>>> compatibility (that is basically everything as-is). >>>> >>>> Let us add vmstate helpers and extend existing vmstate descriptions so >>>> that we have everything we need. Let us guard the added state with via >>>> css_migration_enabled, so we keep the compatible behavior if css >>>> migration is disabled. >>>> >>>> Let's also annotate the bits which do not need to be migrated for better >>>> readability. >>>> >>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>>> --- >>>> hw/intc/s390_flic.c | 20 +++++++++++++++ >>>> hw/s390x/css.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 94 insertions(+) >>> >>> Reviewed-by: Juan Quintela <quintela@redhat.com> >>> >> >> Thanks! >> >>> >>> For the vmstate bits. I have exactly zero clue about s390 >>> >>>> +static const VMStateDescription vmstate_chp_info = { >>>> + .name = "s390_chp_info", >>>> + .version_id = 1, >>>> + .minimum_version_id = 1, >>>> + .fields = (VMStateField[]) { >>>> + VMSTATE_UINT8(in_use, ChpInfo), >>>> + VMSTATE_UINT8(type, ChpInfo), >>>> + VMSTATE_UINT8(is_virtual, ChpInfo), >>>> + VMSTATE_END_OF_LIST() >>>> + } >>>> +}; >>>> + >>>> typedef struct SubchSet { >>>> SubchDev *sch[MAX_SCHID + 1]; >>>> unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)]; >>>> @@ -215,6 +248,19 @@ typedef struct CssImage { >>>> ChpInfo chpids[MAX_CHPID + 1]; >>>> } CssImage; >>>> >>>> +static const VMStateDescription vmstate_css_img = { >>>> + .name = "s390_css_img", >>>> + .version_id = 1, >>>> + .minimum_version_id = 1, >>>> + .fields = (VMStateField[]) { >>>> + /* Subchannel sets have no relevant state. */ >>>> + VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0, >>>> + vmstate_chp_info, ChpInfo), >>>> + VMSTATE_END_OF_LIST() >>>> + } >>>> + >>>> +}; >>> >>>> +static const VMStateDescription vmstate_css = { >>>> + .name = "s390_css", >>>> + .version_id = 1, >>>> + .minimum_version_id = 1, >>>> + .fields = (VMStateField[]) { >>>> + VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, vmstate_crw_container, >>>> + CrwContainer, sibling), >>>> + VMSTATE_BOOL(sei_pending, ChannelSubSys), >>>> + VMSTATE_BOOL(do_crw_mchk, ChannelSubSys), >>>> + VMSTATE_BOOL(crws_lost, ChannelSubSys), >>>> + /* These were kind of migrated by virtio */ >>>> + VMSTATE_UINT8(max_cssid, ChannelSubSys), >>>> + VMSTATE_UINT8(max_ssid, ChannelSubSys), >>>> + VMSTATE_BOOL(chnmon_active, ChannelSubSys), >>>> + VMSTATE_UINT64(chnmon_area, ChannelSubSys), >>>> + VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1, >>>> + 0, vmstate_css_img, CssImage), >>> >>> I was about to suggest to move css from pointer to an embedded struct, >>> and then noticed that MAX_CSSID is .... 65535. I guess that this is #define MAX_CSSID 255 ("include/hw/s390x/css.h" line 23) >>> going to be sparse, very sparse. Perhaps there is an easier way to >>> transmit it? >>> >>> You are transmiting: >>> >>> 65000 structs each of size MAX_CHPID (255) and each element is byte + >>> byte +byte = 65000 * 255 * 3 = 47 MB. >>> >>> If it is really sparse, I think that sending something like a list >>> of <index in array> <contents> could make sense, no? >>> >>> Or I am missunderstanding something? >>> >> >> I think you are misunderstanding. Sparse arrays of pointers were not >> supported in the first place. Support was added by me recentily (patch >> 07d4e69 "migration/vmstate: fix array of ptr with nullptrs"). For a null >> pointer just a single byte is transmitted. So we should be more like >> around 64KB, given that it is really sparse. Bottom line, the in stream Given that my calculation is also wrong. The overhead is below 255 bytes (overhead compared to not transferring anything for null pointers, which is clearly an unrealistic lower bound case). >> representation is at least as efficient as the in memory representation. >> >> Of course we could do something special for sparse arrays of pointers in >> general, and indeed, one of the possibilities is a list/vector of non >> null indexes. > > Just as discussion fodder: For a run-of-the-mill qemu guest, I'd expect > a low double-digit number of subchannels in css 0xfe, in subchannel set > 0, which are clustered right at the beginning, and one chpid (0) in css > 0xfe. > > As the subchannel id is autogenerated based upon the devno (if > provided), it is easy to have subchannels in an arbitrary subchannel > set (of which there are only four), but to get high subchannel ids with > a lot of empty slots before, you'd have to create a lot (really a lot) > of devices and then detach most of them again - not a very likely > pattern. > > The 3270 support does not really change much: one additional subchannel > (with the same characteristics) and one further chpid. But I don't > expect many people to use 3270, and it is not (yet) migratable anyway. > > vfio-ccw devices will be in another css (possibly mapped), but they are > not migrateable either. > > So if we want to optimize, some "nothing of interest beyond that point" > marker might be useful - but not very. Thus I do not think we need extra sparse array handling for this. (Nevertheless an extra sparse array handling might make sense for vmstate core, provided there is an use case). Halil > >> >> I would like to ask the s390x maintainers (mainly Connie) about the >> particular case, and the migration maintainers (I spontaneously think of >> you Juan and Dave) about the general case (If we want to have something >> like VMSTATE_ARRAY_OF_POINTER_TO_STRUCT_SPARSE). >> >> Regards, >> Halil >> >>> Later, Juan. >>> > >
Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 06/01/2017 01:32 PM, Cornelia Huck wrote: >> On Thu, 1 Jun 2017 11:35:27 +0200 >> Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >> >>>> >>>> I was about to suggest to move css from pointer to an embedded struct, >>>> and then noticed that MAX_CSSID is .... 65535. I guess that this is > > #define MAX_CSSID 255 > ("include/hw/s390x/css.h" line 23) I have to grep better O:-) >>>> going to be sparse, very sparse. Perhaps there is an easier way to >>>> transmit it? >>>> >>>> You are transmiting: >>>> >>>> 65000 structs each of size MAX_CHPID (255) and each element is byte + >>>> byte +byte = 65000 * 255 * 3 = 47 MB. >>>> >>>> If it is really sparse, I think that sending something like a list >>>> of <index in array> <contents> could make sense, no? >>>> >>>> Or I am missunderstanding something? >>>> >>> >>> I think you are misunderstanding. Sparse arrays of pointers were not >>> supported in the first place. Support was added by me recentily (patch >>> 07d4e69 "migration/vmstate: fix array of ptr with nullptrs"). For a null >>> pointer just a single byte is transmitted. So we should be more like >>> around 64KB, given that it is really sparse. Bottom line, the in stream > > Given that my calculation is also wrong. The overhead is below 255 bytes > (overhead compared to not transferring anything for null pointers, which > is clearly an unrealistic lower bound case). Yeap, I greeped wrong. 255 x 255 makes no sense to optimize at all. Sorry for the noise. >>> representation is at least as efficient as the in memory representation. >>> >>> Of course we could do something special for sparse arrays of pointers in >>> general, and indeed, one of the possibilities is a list/vector of non >>> null indexes. >> >> Just as discussion fodder: For a run-of-the-mill qemu guest, I'd expect >> a low double-digit number of subchannels in css 0xfe, in subchannel set >> 0, which are clustered right at the beginning, and one chpid (0) in css >> 0xfe. >> >> As the subchannel id is autogenerated based upon the devno (if >> provided), it is easy to have subchannels in an arbitrary subchannel >> set (of which there are only four), but to get high subchannel ids with >> a lot of empty slots before, you'd have to create a lot (really a lot) >> of devices and then detach most of them again - not a very likely >> pattern. >> >> The 3270 support does not really change much: one additional subchannel >> (with the same characteristics) and one further chpid. But I don't >> expect many people to use 3270, and it is not (yet) migratable anyway. >> >> vfio-ccw devices will be in another css (possibly mapped), but they are >> not migrateable either. >> >> So if we want to optimize, some "nothing of interest beyond that point" >> marker might be useful - but not very. > > Thus I do not think we need extra sparse array handling for this. > > (Nevertheless an extra sparse array handling might make sense for > vmstate core, provided there is an use case). So, I retire all my objections. Again, sorry for the noise. Thanks, Juan.
On 06/07/2017 08:03 PM, Juan Quintela wrote: > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >> On 06/01/2017 01:32 PM, Cornelia Huck wrote: >>> On Thu, 1 Jun 2017 11:35:27 +0200 >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >>> > >>>>> >>>>> I was about to suggest to move css from pointer to an embedded struct, >>>>> and then noticed that MAX_CSSID is .... 65535. I guess that this is >> >> #define MAX_CSSID 255 >> ("include/hw/s390x/css.h" line 23) > > I have to grep better O:-) > >>>>> going to be sparse, very sparse. Perhaps there is an easier way to >>>>> transmit it? >>>>> >>>>> You are transmiting: >>>>> >>>>> 65000 structs each of size MAX_CHPID (255) and each element is byte + >>>>> byte +byte = 65000 * 255 * 3 = 47 MB. >>>>> >>>>> If it is really sparse, I think that sending something like a list >>>>> of <index in array> <contents> could make sense, no? >>>>> >>>>> Or I am missunderstanding something? >>>>> >>>> >>>> I think you are misunderstanding. Sparse arrays of pointers were not >>>> supported in the first place. Support was added by me recentily (patch >>>> 07d4e69 "migration/vmstate: fix array of ptr with nullptrs"). For a null >>>> pointer just a single byte is transmitted. So we should be more like >>>> around 64KB, given that it is really sparse. Bottom line, the in stream >> >> Given that my calculation is also wrong. The overhead is below 255 bytes >> (overhead compared to not transferring anything for null pointers, which >> is clearly an unrealistic lower bound case). > > Yeap, I greeped wrong. 255 x 255 makes no sense to optimize at all. > Sorry for the noise. > >>>> representation is at least as efficient as the in memory representation. >>>> >>>> Of course we could do something special for sparse arrays of pointers in >>>> general, and indeed, one of the possibilities is a list/vector of non >>>> null indexes. >>> >>> Just as discussion fodder: For a run-of-the-mill qemu guest, I'd expect >>> a low double-digit number of subchannels in css 0xfe, in subchannel set >>> 0, which are clustered right at the beginning, and one chpid (0) in css >>> 0xfe. >>> >>> As the subchannel id is autogenerated based upon the devno (if >>> provided), it is easy to have subchannels in an arbitrary subchannel >>> set (of which there are only four), but to get high subchannel ids with >>> a lot of empty slots before, you'd have to create a lot (really a lot) >>> of devices and then detach most of them again - not a very likely >>> pattern. >>> >>> The 3270 support does not really change much: one additional subchannel >>> (with the same characteristics) and one further chpid. But I don't >>> expect many people to use 3270, and it is not (yet) migratable anyway. >>> >>> vfio-ccw devices will be in another css (possibly mapped), but they are >>> not migrateable either. >>> >>> So if we want to optimize, some "nothing of interest beyond that point" >>> marker might be useful - but not very. >> >> Thus I do not think we need extra sparse array handling for this. >> >> (Nevertheless an extra sparse array handling might make sense for >> vmstate core, provided there is an use case). > > So, I retire all my objections. > > Again, sorry for the noise. No problem. Actually, it's appreciated! Better have some false positives than having something stupid slipping in and having to live with it. Thanks for your review! Regards, Halil
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c index 7e7546a576..b889e74571 100644 --- a/hw/intc/s390_flic.c +++ b/hw/intc/s390_flic.c @@ -139,6 +139,22 @@ static void qemu_s390_flic_register_types(void) type_init(qemu_s390_flic_register_types) +static bool adapter_info_so_needed(void *opaque) +{ + return css_migration_enabled(); +} + +const VMStateDescription vmstate_adapter_info_so = { + .name = "s390_adapter_info/summary_offset", + .version_id = 1, + .minimum_version_id = 1, + .needed = adapter_info_so_needed, + .fields = (VMStateField[]) { + VMSTATE_UINT32(summary_offset, AdapterInfo), + VMSTATE_END_OF_LIST() + } +}; + const VMStateDescription vmstate_adapter_info = { .name = "s390_adapter_info", .version_id = 1, @@ -152,6 +168,10 @@ const VMStateDescription vmstate_adapter_info = { */ VMSTATE_END_OF_LIST() }, + .subsections = (const VMStateDescription * []) { + &vmstate_adapter_info_so, + NULL + } }; const VMStateDescription vmstate_adapter_routes = { diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 15517a16e4..a15b58d303 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -27,12 +27,45 @@ typedef struct CrwContainer { QTAILQ_ENTRY(CrwContainer) sibling; } CrwContainer; +static const VMStateDescription vmstate_crw = { + .name = "s390_crw", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT16(flags, CRW), + VMSTATE_UINT16(rsid, CRW), + VMSTATE_END_OF_LIST() + }, +}; + +static const VMStateDescription vmstate_crw_container = { + .name = "s390_crw_container", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_STRUCT(crw, CrwContainer, 0, vmstate_crw, CRW), + VMSTATE_END_OF_LIST() + }, +}; + typedef struct ChpInfo { uint8_t in_use; uint8_t type; uint8_t is_virtual; } ChpInfo; +static const VMStateDescription vmstate_chp_info = { + .name = "s390_chp_info", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT8(in_use, ChpInfo), + VMSTATE_UINT8(type, ChpInfo), + VMSTATE_UINT8(is_virtual, ChpInfo), + VMSTATE_END_OF_LIST() + } +}; + typedef struct SubchSet { SubchDev *sch[MAX_SCHID + 1]; unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)]; @@ -215,6 +248,19 @@ typedef struct CssImage { ChpInfo chpids[MAX_CHPID + 1]; } CssImage; +static const VMStateDescription vmstate_css_img = { + .name = "s390_css_img", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + /* Subchannel sets have no relevant state. */ + VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0, + vmstate_chp_info, ChpInfo), + VMSTATE_END_OF_LIST() + } + +}; + typedef struct IoAdapter { uint32_t id; uint8_t type; @@ -232,10 +278,34 @@ typedef struct ChannelSubSys { uint64_t chnmon_area; CssImage *css[MAX_CSSID + 1]; uint8_t default_cssid; + /* don't migrate */ IoAdapter *io_adapters[CSS_IO_ADAPTER_TYPE_NUMS][MAX_ISC + 1]; + /* don't migrate */ QTAILQ_HEAD(, IndAddr) indicator_addresses; } ChannelSubSys; +static const VMStateDescription vmstate_css = { + .name = "s390_css", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, vmstate_crw_container, + CrwContainer, sibling), + VMSTATE_BOOL(sei_pending, ChannelSubSys), + VMSTATE_BOOL(do_crw_mchk, ChannelSubSys), + VMSTATE_BOOL(crws_lost, ChannelSubSys), + /* These were kind of migrated by virtio */ + VMSTATE_UINT8(max_cssid, ChannelSubSys), + VMSTATE_UINT8(max_ssid, ChannelSubSys), + VMSTATE_BOOL(chnmon_active, ChannelSubSys), + VMSTATE_UINT64(chnmon_area, ChannelSubSys), + VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1, + 0, vmstate_css_img, CssImage), + VMSTATE_UINT8(default_cssid, ChannelSubSys), + VMSTATE_END_OF_LIST() + } +}; + static ChannelSubSys channel_subsys = { .pending_crws = QTAILQ_HEAD_INITIALIZER(channel_subsys.pending_crws), .do_crw_mchk = true, @@ -275,6 +345,10 @@ static int subch_dev_post_load(void *opaque, int version_id) css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, s); } + if (css_migration_enabled()) { + /* No compat voodoo to do ;) */ + return 0; + } /* * Hack alert. If we don't migrate the channel subsystem status * we still need to find out if the guest enabled mss/mcss-e.
Although we have recently vmstatified the migration of some css infrastructure, for some css entities there is still state to be migrated left, because the focus was keeping migration stream compatibility (that is basically everything as-is). Let us add vmstate helpers and extend existing vmstate descriptions so that we have everything we need. Let us guard the added state with via css_migration_enabled, so we keep the compatible behavior if css migration is disabled. Let's also annotate the bits which do not need to be migrated for better readability. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- hw/intc/s390_flic.c | 20 +++++++++++++++ hw/s390x/css.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+)