On 05/12/2017 03:24 PM, Dr. David Alan Gilbert wrote:
> * Vlad Yasevich (vyase...@redhat.com) 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 <germ...@redhat.com>
>>> ---
>>> 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 / dgilb...@redhat.com / Manchester, UK
>
