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