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

[Germano Veit Michel]

Ohh, sorry. I don't know how or why, but I missed all your replies (this
was archived in gmail)

Below is qmp-net-test.c, It's just a copy and paste of what Markus
suggested. I could try doing it with a socket (virtio-net-test.c) as Jason
suggested but I'm afraid I won't have time this week as support is quite
busy.

Thanks Vlad for actively working on this.

/*
>  * Test cases for network-related QMP commands
>  *
>  * Copyright (c) 2017 Red Hat Inc.
>  *
>  * Authors:
>  *  Markus Armbruster <address@hidden>,
>  *
>  * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
>  * See the COPYING file in the top-level directory.
>  */
>
> #include "qemu/osdep.h"
> #include "libqtest.h"
> #include "qapi/error.h"
>
> const char common_args[] = "-nodefaults -machine none";
>
> static void test_qmp_announce_self(void)
> {
>     QDict *resp, *ret;
>
>     qtest_start(common_args);
>
>     resp = qmp("{ 'execute': 'announce-self' }");
>     ret = qdict_get_qdict(resp, "return");
>     g_assert(ret && !qdict_size(ret));
>     QDECREF(resp);
>
>     qtest_end();
> }
>
> int main(int argc, char *argv[])
> {
>     g_test_init(&argc, &argv, NULL);
>
>     qtest_add_func("qmp/net/announce_self", test_qmp_announce_self);
>
>     return g_test_run();
> }
>


On Sat, May 13, 2017 at 7:16 AM, Vlad Yasevich <address@hidden> wrote:

> 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
> >
>
>


-- 
Germano Veit Michel <address@hidden>
Senior Software Maintenance Engineer, Virtualization, Red Hat