diff mbox

[01/12] migration: Introduce announce parameters

Message ID 1495649128-10529-2-git-send-email-vyasevic@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vlad Yasevich May 24, 2017, 6:05 p.m. UTC
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(+)

Comments

Jason Wang May 26, 2017, 4:03 a.m. UTC | #1
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
Vlad Yasevich May 26, 2017, 1:06 p.m. UTC | #2
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
Eric Blake May 26, 2017, 1:08 p.m. UTC | #3
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.
Vlad Yasevich May 26, 2017, 1:17 p.m. UTC | #4
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
Juan Quintela May 30, 2017, 9:58 a.m. UTC | #5
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.
Vlad Yasevich May 30, 2017, 1:45 p.m. UTC | #6
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
Dr. David Alan Gilbert May 30, 2017, 6:57 p.m. UTC | #7
* 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
Dr. David Alan Gilbert May 30, 2017, 7:08 p.m. UTC | #8
* 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(&params, 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
Jason Wang June 1, 2017, 7:02 a.m. UTC | #9
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
Vlad Yasevich June 1, 2017, 1:45 p.m. UTC | #10
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
Dr. David Alan Gilbert June 1, 2017, 2:02 p.m. UTC | #11
* 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 mbox

Patch

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(&params, 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:
 #