| Message ID | 1495649128-10529-2-git-send-email-vyasevic@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2017年05月25日 02:05, Vladislav Yasevich wrote: > Add parameters that control RARP/GARP announcement timeouts. > The parameters structure is added to the QAPI and a qmp command > is added to set/get the parameter data. > > Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> I think it's better to explain e.g under which condition do we need to tweak such parameters. Thanks
On 05/26/2017 12:03 AM, Jason Wang wrote: > > On 2017年05月25日 02:05, Vladislav Yasevich wrote: >> Add parameters that control RARP/GARP announcement timeouts. >> The parameters structure is added to the QAPI and a qmp command >> is added to set/get the parameter data. >> >> Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > > I think it's better to explain e.g under which condition do we need to tweak such parameters. > > Thanks > OK. I'll rip off what dgilbert wrote in his original series for the description. Dave, if you have any text to add as to why migration might need to tweak this, I'd appreciate it. Thanks -vlad
On 05/24/2017 01:05 PM, Vladislav Yasevich wrote: > Add parameters that control RARP/GARP announcement timeouts. > The parameters structure is added to the QAPI and a qmp command > is added to set/get the parameter data. > > Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > --- Just an interface review for now: > +++ b/qapi-schema.json > @@ -569,6 +569,90 @@ > ## > { 'command': 'query-events', 'returns': ['EventInfo'] } > > + > +## > +# @AnnounceParameter: > +# > +# @AnnounceParameter enumeration > +# > +# @initial: Initial delay (in ms) before sending the first GARP/RARP > +# announcement > +# > +# @max: Maximum delay (in ms) to between GARP/RARP announcemnt packets s/announcemnt/announcement/ > +# > +# @rounds: Number of self-announcement attempts > +# > +# @step: Delay increate (in ms) after each self-announcment attempt s/increate/increase/ s/announcment/announcement/ > +# > +# Since: 2.10 > +## > +{ 'enum' : 'AnnounceParameter', > + 'data' : [ 'initial', 'max', 'rounds', 'step' ] } Why are we creating an enum here? Without reading further, it looks like you plan on using the enum to delineate members of a union? But that feels like it will be overly complicated. A struct should be sufficient (each parameter being an optional member of the struct, where you can supply as many or as few on input, but all are reported on output). > + > +## > +# @AnnounceParameters: > +# > +# Optional members may be omited on input, but all values will be present s/omited/omitted/ > +# on output. > +# > +# @initial: Initial delay (in ms) before sending the first GARP/RARP > +# announcement > +# > +# @max: Maximum delay (in ms) to between GARP/RARP announcemnt packets s/announcemnt/announcement/ > +# > +# @rounds: Number of self-announcement attempts > +# > +# @step: Delay increate (in ms) after each self-announcment attempt s/increate/increase/ s/announcment/announcement/ > +# > +# Since: 2.10 > +## > + > +{ 'struct': 'AnnounceParameters', > + 'data': { '*initial': 'int', > + '*max': 'int', > + '*rounds': 'int', > + '*step': 'int' } } > + > +## > +# @announce-set-parameters: > +# > +# Set qemu announce parameters. > +# > +# Since: 2.10 > +# > +# Example: > +# > +# -> { "execute": "announce-set-parameters", > +# "arguments": { "announce-rounds": 10 } } > +# > +## > +{ 'command': 'announce-set-parameters', 'boxed': true, > + 'data': 'AnnounceParameters' } > + > +## > +# @query-announce-parameters: > +# > +# Returns information about the current announce parameters > +# > +# Returns: @AnnounceParameters > +# > +# Since: 2.10 > +# > +# Example: > +# > +# -> { "execute": "query-announce-parameters" } > +# <- { "return": { > +# "initial": 50, > +# "max": 500, > +# "rounds": 5, > +# "step": 100 > +# } > +# } > +# > +## > +{ 'command': 'query-announce-parameters', > + 'returns': 'AnnounceParameters' } Yep, I'm right. The enum is bogus. The struct is sufficient, so you don't need the enum.
On 05/26/2017 09:08 AM, Eric Blake wrote: > On 05/24/2017 01:05 PM, Vladislav Yasevich wrote: >> Add parameters that control RARP/GARP announcement timeouts. >> The parameters structure is added to the QAPI and a qmp command >> is added to set/get the parameter data. >> >> Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> >> --- > > Just an interface review for now: > >> +++ b/qapi-schema.json >> @@ -569,6 +569,90 @@ >> ## >> { 'command': 'query-events', 'returns': ['EventInfo'] } >> >> + >> +## >> +# @AnnounceParameter: >> +# >> +# @AnnounceParameter enumeration >> +# >> +# @initial: Initial delay (in ms) before sending the first GARP/RARP >> +# announcement >> +# >> +# @max: Maximum delay (in ms) to between GARP/RARP announcemnt packets > > s/announcemnt/announcement/ > >> +# >> +# @rounds: Number of self-announcement attempts >> +# >> +# @step: Delay increate (in ms) after each self-announcment attempt > > s/increate/increase/ > s/announcment/announcement/ > >> +# >> +# Since: 2.10 >> +## >> +{ 'enum' : 'AnnounceParameter', >> + 'data' : [ 'initial', 'max', 'rounds', 'step' ] } > > Why are we creating an enum here? Without reading further, it looks > like you plan on using the enum to delineate members of a union? But > that feels like it will be overly complicated. A struct should be > sufficient (each parameter being an optional member of the struct, where > you can supply as many or as few on input, but all are reported on output). > I end up using them for the HMP interface. If it's better, I can move this definition to the HMP patch. Thanks -vlad
Vladislav Yasevich <vyasevic@redhat.com> wrote: > Add parameters that control RARP/GARP announcement timeouts. > The parameters structure is added to the QAPI and a qmp command > is added to set/get the parameter data. > > Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> Hi > diff --git a/migration/savevm.c b/migration/savevm.c > index f5e8194..cee2837 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -78,6 +78,104 @@ static struct mig_cmd_args { > [MIG_CMD_MAX] = { .len = -1, .name = "MAX" }, > }; > Once that you are touching this, shouldn't it be better to be in net/<somewhere>? They have nothing to do with migration really. > +#define QEMU_ANNOUNCE_INITIAL 50 > +#define QEMU_ANNOUNCE_MAX 550 > +#define QEMU_ANNOUNCE_ROUNDS 5 > +#define QEMU_ANNOUNCE_STEP 100 > + > +AnnounceParameters *qemu_get_announce_params(void) > +{ > + static AnnounceParameters announce = { > + .initial = QEMU_ANNOUNCE_INITIAL, > + .max = QEMU_ANNOUNCE_MAX, > + .rounds = QEMU_ANNOUNCE_ROUNDS, > + .step = QEMU_ANNOUNCE_STEP, > + }; > + > + return &announce; > +} > + > +void qemu_fill_announce_parameters(AnnounceParameters **to, > + AnnounceParameters *from) > +{ > + AnnounceParameters *params; > + > + params = *to = g_malloc0(sizeof(*params)); > + params->has_initial = true; > + params->initial = from->initial; > + params->has_max = true; > + params->max = from->max; > + params->has_rounds = true; > + params->rounds = from->rounds; > + params->has_step = true; > + params->step = from->step; > +} > + > +bool qemu_validate_announce_parameters(AnnounceParameters *params, Error **errp) > +{ > + if (params->has_initial && > + (params->initial < 1 || params->initial > 100000)) { This is independent of this patch, but we really need a macro: CHECK(field, mininum, maximum) We repeat this left and right. > +void qemu_set_announce_parameters(AnnounceParameters *announce_params, > + AnnounceParameters *params) > +void qmp_announce_set_parameters(AnnounceParameters *params, Error **errp) Really similar functions name. I assume by know that the 1st function is used somewhere else in the series.
On 05/30/2017 05:58 AM, Juan Quintela wrote: > Vladislav Yasevich <vyasevic@redhat.com> wrote: >> Add parameters that control RARP/GARP announcement timeouts. >> The parameters structure is added to the QAPI and a qmp command >> is added to set/get the parameter data. >> >> Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > > Hi > >> diff --git a/migration/savevm.c b/migration/savevm.c >> index f5e8194..cee2837 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -78,6 +78,104 @@ static struct mig_cmd_args { >> [MIG_CMD_MAX] = { .len = -1, .name = "MAX" }, >> }; >> > > Once that you are touching this, shouldn't it be better to be in > net/<somewhere>? > They have nothing to do with migration really. > Yeah, I considered this, but could really find a good slot for them. I can either put them into net.c directly or pull them out into their own little file. > >> +#define QEMU_ANNOUNCE_INITIAL 50 >> +#define QEMU_ANNOUNCE_MAX 550 >> +#define QEMU_ANNOUNCE_ROUNDS 5 >> +#define QEMU_ANNOUNCE_STEP 100 >> + >> +AnnounceParameters *qemu_get_announce_params(void) >> +{ >> + static AnnounceParameters announce = { >> + .initial = QEMU_ANNOUNCE_INITIAL, >> + .max = QEMU_ANNOUNCE_MAX, >> + .rounds = QEMU_ANNOUNCE_ROUNDS, >> + .step = QEMU_ANNOUNCE_STEP, >> + }; >> + >> + return &announce; >> +} >> + >> +void qemu_fill_announce_parameters(AnnounceParameters **to, >> + AnnounceParameters *from) >> +{ >> + AnnounceParameters *params; >> + >> + params = *to = g_malloc0(sizeof(*params)); >> + params->has_initial = true; >> + params->initial = from->initial; >> + params->has_max = true; >> + params->max = from->max; >> + params->has_rounds = true; >> + params->rounds = from->rounds; >> + params->has_step = true; >> + params->step = from->step; >> +} >> + >> +bool qemu_validate_announce_parameters(AnnounceParameters *params, Error **errp) >> +{ >> + if (params->has_initial && >> + (params->initial < 1 || params->initial > 100000)) { > > This is independent of this patch, but we really need a macro: > CHECK(field, mininum, maximum) > > We repeat this left and right. > >> +void qemu_set_announce_parameters(AnnounceParameters *announce_params, >> + AnnounceParameters *params) > >> +void qmp_announce_set_parameters(AnnounceParameters *params, Error **errp) > > Really similar functions name. I assume by know that the 1st function > is used somewhere else in the series. > Yes, the first function is going to be used later. Thanks -vlad
* Vlad Yasevich (vyasevic@redhat.com) wrote: > On 05/26/2017 12:03 AM, Jason Wang wrote: > > > > On 2017年05月25日 02:05, Vladislav Yasevich wrote: > >> Add parameters that control RARP/GARP announcement timeouts. > >> The parameters structure is added to the QAPI and a qmp command > >> is added to set/get the parameter data. > >> > >> Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >> > >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > > > > I think it's better to explain e.g under which condition do we need to tweak such parameters. > > > > Thanks > > > > OK. I'll rip off what dgilbert wrote in his original series for the description. > > Dave, if you have any text to add as to why migration might need to tweak this, I'd > appreciate it. Pretty much what I originally said; that the existing values are arbitrary and fixed, and for systems with complex/sluggish network reconfiguration systems they can be too slow. Dave > Thanks > -vlad -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Vladislav Yasevich (vyasevic@redhat.com) wrote: > Add parameters that control RARP/GARP announcement timeouts. > The parameters structure is added to the QAPI and a qmp command > is added to set/get the parameter data. > > Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> Thanks; this is pretty much duplicating the migrate-parameter code; that's OK but I wonder if there's an easier/better way. If you made a global gom object with these values as properties, then they could be set with either -global or with 'qom-set' Would that work/be simpler? You wouldn't need to add any new commands. (Some typo spots below) Dave > --- > include/sysemu/sysemu.h | 7 ++++ > migration/savevm.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++ > qapi-schema.json | 84 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 189 insertions(+) > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index 83c1ceb..7fd49c4 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -78,6 +78,13 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify); > int save_vmstate(const char *name, Error **errp); > int load_vmstate(const char *name, Error **errp); > > +AnnounceParameters *qemu_get_announce_params(void); > +void qemu_fill_announce_parameters(AnnounceParameters **to, > + AnnounceParameters *from); > +bool qemu_validate_announce_parameters(AnnounceParameters *params, > + Error **errp); > +void qemu_set_announce_parameters(AnnounceParameters *announce_params, > + AnnounceParameters *params); > void qemu_announce_self(void); > > /* Subcommands for QEMU_VM_COMMAND */ > diff --git a/migration/savevm.c b/migration/savevm.c > index f5e8194..cee2837 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -78,6 +78,104 @@ static struct mig_cmd_args { > [MIG_CMD_MAX] = { .len = -1, .name = "MAX" }, > }; > > +#define QEMU_ANNOUNCE_INITIAL 50 > +#define QEMU_ANNOUNCE_MAX 550 > +#define QEMU_ANNOUNCE_ROUNDS 5 > +#define QEMU_ANNOUNCE_STEP 100 > + > +AnnounceParameters *qemu_get_announce_params(void) > +{ > + static AnnounceParameters announce = { > + .initial = QEMU_ANNOUNCE_INITIAL, > + .max = QEMU_ANNOUNCE_MAX, > + .rounds = QEMU_ANNOUNCE_ROUNDS, > + .step = QEMU_ANNOUNCE_STEP, > + }; > + > + return &announce; > +} > + > +void qemu_fill_announce_parameters(AnnounceParameters **to, > + AnnounceParameters *from) > +{ > + AnnounceParameters *params; > + > + params = *to = g_malloc0(sizeof(*params)); > + params->has_initial = true; > + params->initial = from->initial; > + params->has_max = true; > + params->max = from->max; > + params->has_rounds = true; > + params->rounds = from->rounds; > + params->has_step = true; > + params->step = from->step; > +} > + > +bool qemu_validate_announce_parameters(AnnounceParameters *params, Error **errp) > +{ > + if (params->has_initial && > + (params->initial < 1 || params->initial > 100000)) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "initial", > + "is invalid, it should be in the range of 1 to 100000 ms"); > + return false; > + } > + if (params->has_max && > + (params->max < 1 || params->max > 100000)) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "max", > + "is invalid, it should be in the range of 1 to 100000 ms"); > + return false; > + } > + if (params->has_rounds && > + (params->rounds < 1 || params->rounds > 1000)) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "rounds", > + "is invalid, it should be in the range of 1 to 1000"); > + return false; > + } > + if (params->has_step && > + (params->step < 0 || params->step > 10000)) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "step", > + "is invalid, it should be in the range of 1 to 10000 ms"); > + return false; > + } > + > + return true; > +} > + > +void qemu_set_announce_parameters(AnnounceParameters *announce_params, > + AnnounceParameters *params) > +{ > + if (params->has_initial) { > + announce_params->initial = params->initial; > + } > + if (params->has_max) { > + announce_params->max = params->max; > + } > + if (params->has_rounds) { > + announce_params->rounds = params->rounds; > + } > + if (params->has_step) { > + announce_params->step = params->step; > + } > +} > + > +AnnounceParameters *qmp_query_announce_parameters(Error **errp) > +{ > + AnnounceParameters *params = NULL; > + > + qemu_fill_announce_parameters(¶ms, qemu_get_announce_params()); > + return params; > +} > + > +void qmp_announce_set_parameters(AnnounceParameters *params, Error **errp) > +{ > + AnnounceParameters *current = qemu_get_announce_params(); > + > + if (!qemu_validate_announce_parameters(params, errp)) > + return; > + > + qemu_set_announce_parameters(current, params); > +} > + > static int announce_self_create(uint8_t *buf, > uint8_t *mac_addr) > { > diff --git a/qapi-schema.json b/qapi-schema.json > index 80603cf..2030087 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -569,6 +569,90 @@ > ## > { 'command': 'query-events', 'returns': ['EventInfo'] } > > + > +## > +# @AnnounceParameter: > +# > +# @AnnounceParameter enumeration > +# > +# @initial: Initial delay (in ms) before sending the first GARP/RARP > +# announcement > +# > +# @max: Maximum delay (in ms) to between GARP/RARP announcemnt packets > +# > +# @rounds: Number of self-announcement attempts > +# > +# @step: Delay increate (in ms) after each self-announcment attempt ^-s ^e > +# > +# Since: 2.10 > +## > +{ 'enum' : 'AnnounceParameter', > + 'data' : [ 'initial', 'max', 'rounds', 'step' ] } > + > +## > +# @AnnounceParameters: > +# > +# Optional members may be omited on input, but all values will be present ^t > +# on output. > +# > +# @initial: Initial delay (in ms) before sending the first GARP/RARP > +# announcement > +# > +# @max: Maximum delay (in ms) to between GARP/RARP announcemnt packets > +# > +# @rounds: Number of self-announcement attempts > +# > +# @step: Delay increate (in ms) after each self-announcment attempt > +# > +# Since: 2.10 > +## > + > +{ 'struct': 'AnnounceParameters', > + 'data': { '*initial': 'int', > + '*max': 'int', > + '*rounds': 'int', > + '*step': 'int' } } > + > +## > +# @announce-set-parameters: > +# > +# Set qemu announce parameters. > +# > +# Since: 2.10 > +# > +# Example: > +# > +# -> { "execute": "announce-set-parameters", > +# "arguments": { "announce-rounds": 10 } } > +# > +## > +{ 'command': 'announce-set-parameters', 'boxed': true, > + 'data': 'AnnounceParameters' } > + > +## > +# @query-announce-parameters: > +# > +# Returns information about the current announce parameters > +# > +# Returns: @AnnounceParameters > +# > +# Since: 2.10 > +# > +# Example: > +# > +# -> { "execute": "query-announce-parameters" } > +# <- { "return": { > +# "initial": 50, > +# "max": 500, > +# "rounds": 5, > +# "step": 100 > +# } > +# } > +# > +## > +{ 'command': 'query-announce-parameters', > + 'returns': 'AnnounceParameters' } > + > ## > # @MigrationStats: > # > -- > 2.7.4 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 2017年05月31日 02:57, Dr. David Alan Gilbert wrote: > * Vlad Yasevich (vyasevic@redhat.com) wrote: >> On 05/26/2017 12:03 AM, Jason Wang wrote: >>> On 2017å¹´05月25æ—¥ 02:05, Vladislav Yasevich wrote: >>>> Add parameters that control RARP/GARP announcement timeouts. >>>> The parameters structure is added to the QAPI and a qmp command >>>> is added to set/get the parameter data. >>>> >>>> Based on work by "Dr. David Alan Gilbert"<dgilbert@redhat.com> >>>> >>>> Signed-off-by: Vladislav Yasevich<vyasevic@redhat.com> >>> I think it's better to explain e.g under which condition do we need to tweak such parameters. >>> >>> Thanks >>> >> OK. I'll rip off what dgilbert wrote in his original series for the description. >> >> Dave, if you have any text to add as to why migration might need to tweak this, I'd >> appreciate it. > Pretty much what I originally said; that the existing values > are arbitrary and fixed, and for systems with complex/sluggish > network reconfiguration systems they can be too slow. > > Dave > I agree, but I'm not sure how much it can help in fact unless management can set configuration specific parameters. And what we did here is best effort, there's no guarantee that G(R)ARP packet can reach the destination. Thanks
On 06/01/2017 03:02 AM, Jason Wang wrote: > > > On 2017年05月31日 02:57, Dr. David Alan Gilbert wrote: >> * Vlad Yasevich (vyasevic@redhat.com) wrote: >>> On 05/26/2017 12:03 AM, Jason Wang wrote: >>>> On 2017å¹´05月25æ—¥ 02:05, Vladislav Yasevich wrote: >>>>> Add parameters that control RARP/GARP announcement timeouts. >>>>> The parameters structure is added to the QAPI and a qmp command >>>>> is added to set/get the parameter data. >>>>> >>>>> Based on work by "Dr. David Alan Gilbert"<dgilbert@redhat.com> >>>>> >>>>> Signed-off-by: Vladislav Yasevich<vyasevic@redhat.com> >>>> I think it's better to explain e.g under which condition do we need to tweak such >>>> parameters. >>>> >>>> Thanks >>>> >>> OK. I'll rip off what dgilbert wrote in his original series for the description. >>> >>> Dave, if you have any text to add as to why migration might need to tweak this, I'd >>> appreciate it. >> Pretty much what I originally said; that the existing values >> are arbitrary and fixed, and for systems with complex/sluggish >> network reconfiguration systems they can be too slow. >> >> Dave >> > > I agree, but I'm not sure how much it can help in fact unless management can set > configuration specific parameters. And what we did here is best effort, there's no > guarantee that G(R)ARP packet can reach the destination. > So, that's what the series allows. If management knows something new, it can set appropriate parameter values. Additionally, the management is also free to trigger additional announcements through the new commands. I am starting to think that just for the sake of migration, exposing announce_self interface to management might be sufficient. Management, when it deems migration complete, may use the interface to trigger announcements in addition to whatever best effort QEMU may attempt itself. Dave, would that be enough, or do the parameters still make sense? Thanks -vlad
* Vlad Yasevich (vyasevic@redhat.com) wrote: > On 06/01/2017 03:02 AM, Jason Wang wrote: > > > > > > On 2017年05月31日 02:57, Dr. David Alan Gilbert wrote: > >> * Vlad Yasevich (vyasevic@redhat.com) wrote: > >>> On 05/26/2017 12:03 AM, Jason Wang wrote: > >>>> On 2017å¹´05月25æ—¥ 02:05, Vladislav Yasevich wrote: > >>>>> Add parameters that control RARP/GARP announcement timeouts. > >>>>> The parameters structure is added to the QAPI and a qmp command > >>>>> is added to set/get the parameter data. > >>>>> > >>>>> Based on work by "Dr. David Alan Gilbert"<dgilbert@redhat.com> > >>>>> > >>>>> Signed-off-by: Vladislav Yasevich<vyasevic@redhat.com> > >>>> I think it's better to explain e.g under which condition do we need to tweak such > >>>> parameters. > >>>> > >>>> Thanks > >>>> > >>> OK. I'll rip off what dgilbert wrote in his original series for the description. > >>> > >>> Dave, if you have any text to add as to why migration might need to tweak this, I'd > >>> appreciate it. > >> Pretty much what I originally said; that the existing values > >> are arbitrary and fixed, and for systems with complex/sluggish > >> network reconfiguration systems they can be too slow. > >> > >> Dave > >> > > > > I agree, but I'm not sure how much it can help in fact unless management can set > > configuration specific parameters. And what we did here is best effort, there's no > > guarantee that G(R)ARP packet can reach the destination. > > > > So, that's what the series allows. If management knows something new, it can set > appropriate parameter values. Additionally, the management is also free to trigger > additional announcements through the new commands. > > I am starting to think that just for the sake of migration, exposing announce_self > interface to management might be sufficient. Management, when it deems migration > complete, may use the interface to trigger announcements in addition to whatever > best effort QEMU may attempt itself. > > Dave, would that be enough, or do the parameters still make sense? I'd still like to be able to set the parameters to be able to work around the very broken setups that are out there. The way I got to this point was a hack for a user where I just increased it so it was doing about 10 seconds of announce which was long enough for the network to get it's act together. Dave > Thanks > -vlad > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 83c1ceb..7fd49c4 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -78,6 +78,13 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify); int save_vmstate(const char *name, Error **errp); int load_vmstate(const char *name, Error **errp); +AnnounceParameters *qemu_get_announce_params(void); +void qemu_fill_announce_parameters(AnnounceParameters **to, + AnnounceParameters *from); +bool qemu_validate_announce_parameters(AnnounceParameters *params, + Error **errp); +void qemu_set_announce_parameters(AnnounceParameters *announce_params, + AnnounceParameters *params); void qemu_announce_self(void); /* Subcommands for QEMU_VM_COMMAND */ diff --git a/migration/savevm.c b/migration/savevm.c index f5e8194..cee2837 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -78,6 +78,104 @@ static struct mig_cmd_args { [MIG_CMD_MAX] = { .len = -1, .name = "MAX" }, }; +#define QEMU_ANNOUNCE_INITIAL 50 +#define QEMU_ANNOUNCE_MAX 550 +#define QEMU_ANNOUNCE_ROUNDS 5 +#define QEMU_ANNOUNCE_STEP 100 + +AnnounceParameters *qemu_get_announce_params(void) +{ + static AnnounceParameters announce = { + .initial = QEMU_ANNOUNCE_INITIAL, + .max = QEMU_ANNOUNCE_MAX, + .rounds = QEMU_ANNOUNCE_ROUNDS, + .step = QEMU_ANNOUNCE_STEP, + }; + + return &announce; +} + +void qemu_fill_announce_parameters(AnnounceParameters **to, + AnnounceParameters *from) +{ + AnnounceParameters *params; + + params = *to = g_malloc0(sizeof(*params)); + params->has_initial = true; + params->initial = from->initial; + params->has_max = true; + params->max = from->max; + params->has_rounds = true; + params->rounds = from->rounds; + params->has_step = true; + params->step = from->step; +} + +bool qemu_validate_announce_parameters(AnnounceParameters *params, Error **errp) +{ + if (params->has_initial && + (params->initial < 1 || params->initial > 100000)) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "initial", + "is invalid, it should be in the range of 1 to 100000 ms"); + return false; + } + if (params->has_max && + (params->max < 1 || params->max > 100000)) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "max", + "is invalid, it should be in the range of 1 to 100000 ms"); + return false; + } + if (params->has_rounds && + (params->rounds < 1 || params->rounds > 1000)) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "rounds", + "is invalid, it should be in the range of 1 to 1000"); + return false; + } + if (params->has_step && + (params->step < 0 || params->step > 10000)) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "step", + "is invalid, it should be in the range of 1 to 10000 ms"); + return false; + } + + return true; +} + +void qemu_set_announce_parameters(AnnounceParameters *announce_params, + AnnounceParameters *params) +{ + if (params->has_initial) { + announce_params->initial = params->initial; + } + if (params->has_max) { + announce_params->max = params->max; + } + if (params->has_rounds) { + announce_params->rounds = params->rounds; + } + if (params->has_step) { + announce_params->step = params->step; + } +} + +AnnounceParameters *qmp_query_announce_parameters(Error **errp) +{ + AnnounceParameters *params = NULL; + + qemu_fill_announce_parameters(¶ms, qemu_get_announce_params()); + return params; +} + +void qmp_announce_set_parameters(AnnounceParameters *params, Error **errp) +{ + AnnounceParameters *current = qemu_get_announce_params(); + + if (!qemu_validate_announce_parameters(params, errp)) + return; + + qemu_set_announce_parameters(current, params); +} + static int announce_self_create(uint8_t *buf, uint8_t *mac_addr) { diff --git a/qapi-schema.json b/qapi-schema.json index 80603cf..2030087 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -569,6 +569,90 @@ ## { 'command': 'query-events', 'returns': ['EventInfo'] } + +## +# @AnnounceParameter: +# +# @AnnounceParameter enumeration +# +# @initial: Initial delay (in ms) before sending the first GARP/RARP +# announcement +# +# @max: Maximum delay (in ms) to between GARP/RARP announcemnt packets +# +# @rounds: Number of self-announcement attempts +# +# @step: Delay increate (in ms) after each self-announcment attempt +# +# Since: 2.10 +## +{ 'enum' : 'AnnounceParameter', + 'data' : [ 'initial', 'max', 'rounds', 'step' ] } + +## +# @AnnounceParameters: +# +# Optional members may be omited on input, but all values will be present +# on output. +# +# @initial: Initial delay (in ms) before sending the first GARP/RARP +# announcement +# +# @max: Maximum delay (in ms) to between GARP/RARP announcemnt packets +# +# @rounds: Number of self-announcement attempts +# +# @step: Delay increate (in ms) after each self-announcment attempt +# +# Since: 2.10 +## + +{ 'struct': 'AnnounceParameters', + 'data': { '*initial': 'int', + '*max': 'int', + '*rounds': 'int', + '*step': 'int' } } + +## +# @announce-set-parameters: +# +# Set qemu announce parameters. +# +# Since: 2.10 +# +# Example: +# +# -> { "execute": "announce-set-parameters", +# "arguments": { "announce-rounds": 10 } } +# +## +{ 'command': 'announce-set-parameters', 'boxed': true, + 'data': 'AnnounceParameters' } + +## +# @query-announce-parameters: +# +# Returns information about the current announce parameters +# +# Returns: @AnnounceParameters +# +# Since: 2.10 +# +# Example: +# +# -> { "execute": "query-announce-parameters" } +# <- { "return": { +# "initial": 50, +# "max": 500, +# "rounds": 5, +# "step": 100 +# } +# } +# +## +{ 'command': 'query-announce-parameters', + 'returns': 'AnnounceParameters' } + ## # @MigrationStats: #
Add parameters that control RARP/GARP announcement timeouts. The parameters structure is added to the QAPI and a qmp command is added to set/get the parameter data. Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> --- include/sysemu/sysemu.h | 7 ++++ migration/savevm.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++ qapi-schema.json | 84 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 189 insertions(+)