Re: [Qemu-devel] [PATCH V2] migration: expose qemu_announce_self() via qmp

[Vlad Yasevich]

On 05/12/2017 03:24 PM, Dr. David Alan Gilbert wrote:
> * Vlad Yasevich (address@hidden) wrote:
>> On 02/20/2017 07:16 PM, Germano Veit Michel wrote:
>>> qemu_announce_self() is triggered by qemu at the end of migrations
>>> to update the network regarding the path to the guest l2addr.
>>>
>>> however it is also useful when there is a network change such as
>>> an active bond slave swap. Essentially, it's the same as a migration
>>> from a network perspective - the guest moves to a different point
>>> in the network topology.
>>>
>>> this exposes the function via qmp.
>>>
>>> Signed-off-by: Germano Veit Michel <address@hidden>
>>> ---
>>>  include/migration/vmstate.h |  5 +++++
>>>  migration/savevm.c          | 30 +++++++++++++++++++-----------
>>>  qapi-schema.json            | 18 ++++++++++++++++++
>>>  3 files changed, 42 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>> index 63e7b02..a08715c 100644
>>> --- a/include/migration/vmstate.h
>>> +++ b/include/migration/vmstate.h
>>> @@ -1042,6 +1042,11 @@ int64_t self_announce_delay(int round)
>>>      return 50 + (SELF_ANNOUNCE_ROUNDS - round - 1) * 100;
>>>  }
>>>
>>> +struct AnnounceRound {
>>> +    QEMUTimer *timer;
>>> +    int count;
>>> +};
>>> +
>>>  void dump_vmstate_json_to_file(FILE *out_fp);
>>>
>>>  #endif
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index 5ecd264..44e196b 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -118,29 +118,37 @@ static void qemu_announce_self_iter(NICState
>>> *nic, void *opaque)
>>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
>>>  }
>>>
>>> -
>>>  static void qemu_announce_self_once(void *opaque)
>>>  {
>>> -    static int count = SELF_ANNOUNCE_ROUNDS;
>>> -    QEMUTimer *timer = *(QEMUTimer **)opaque;
>>> +    struct AnnounceRound *round = opaque;
>>>
>>>      qemu_foreach_nic(qemu_announce_self_iter, NULL);
>>>
>>> -    if (--count) {
>>> +    round->count--;
>>> +    if (round->count) {
>>>          /* delay 50ms, 150ms, 250ms, ... */
>>> -        timer_mod(timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
>>> -                  self_announce_delay(count));
>>> +        timer_mod(round->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
>>> +                  self_announce_delay(round->count));
>>>      } else {
>>> -            timer_del(timer);
>>> -            timer_free(timer);
>>> +            timer_del(round->timer);
>>> +            timer_free(round->timer);
>>> +            g_free(round);
>>>      }
>>>  }
>>>
>>>  void qemu_announce_self(void)
>>>  {
>>> -    static QEMUTimer *timer;
>>> -    timer = timer_new_ms(QEMU_CLOCK_REALTIME, qemu_announce_self_once, 
>>> &timer);
>>> -    qemu_announce_self_once(&timer);
>>> +    struct AnnounceRound *round = g_malloc(sizeof(struct AnnounceRound));
>>> +    if (!round)
>>> +        return;
>>> +    round->count = SELF_ANNOUNCE_ROUNDS;
>>> +    round->timer = timer_new_ms(QEMU_CLOCK_REALTIME,
>>> qemu_announce_self_once, round);
>>> +    qemu_announce_self_once(round);
>>> +}
>>
>> So, I've been looking and this code and have been playing with it and with 
>> David's
>> patches and my patches to include virtio self announcements as well.  What 
>> I've discovered
>> is what I think is a possible packet amplification issue here.
>>
>> This creates a new timer every time we do do a announce_self.  With just 
>> migration,
>> this is not an issue since you only migrate once at a time, so there is only 
>> 1 timer.
>> With exposing this as an API, a user can potentially call it in a tight loop
>> and now you have a ton of timers being created.  Add in David's patches 
>> allowing timeouts
>> and retries to be configurable, and you may now have a ton of long lived 
>> timers.
>> Add in the patches I am working on to let virtio do self announcements too 
>> (to really fix
>> bonding issues), and now you add in a possibility of a lot of packets being 
>> sent for
>> each timeout (RARP, GARP, NA, IGMPv4 Reports, IGMPv6 Reports [even worse if 
>> MLD1 is used]).
>>
>> As you can see, this can get rather ugly...
>>
>> I think we need timer user here.  Migration and QMP being two to begin with. 
>>  Each
>> one would get a single timer to play with.  If a given user already has a 
>> timer running,
>> we could return an error or just not do anything.
> 
> If you did have specific timers, then you could add to/reset the counts
> rather than doing nothing.  That way it's less racy; if you issue the
> command just as you reconfigure your network, there's no chance the
> command would fail, you will send the packets out.

Yes.  That's another possible way to handle this.

-vlad
> 
> Dave
> 
>> -vlad
>>
>>> +
>>> +void qmp_announce_self(Error **errp)
>>> +{
>>> +    qemu_announce_self();
>>>  }
>>>
>>>  /***********************************************************/
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index baa0d26..0d9bffd 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -6080,3 +6080,21 @@
>>>  #
>>>  ##
>>>  { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
>>> +
>>> +##
>>> +# @announce-self:
>>> +#
>>> +# Trigger generation of broadcast RARP frames to update network switches.
>>> +# This can be useful when network bonds fail-over the active slave.
>>> +#
>>> +# Arguments: None.
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "announce-self" }
>>> +# <- { "return": {} }
>>> +#
>>> +# Since: 2.9
>>> +##
>>> +{ 'command': 'announce-self' }
>>> +
>>>
>>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>