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

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

[Germano Veit Michel]

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 <germano@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);
+}
+
+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' }
+
-- 
2.9.3

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

[Dr. David Alan Gilbert]

* Germano Veit Michel (germano@redhat.com) 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.

Markus: Since you're asking for tests for qmp commands; how would you
test this?

Jason: Does this look OK from the networking side of things?

> Signed-off-by: Germano Veit Michel <germano@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));

I prefer g_new0 - i.e.
   struct AnnounceRound *round = g_new0(struct AnnounceRound, 1)

> +    if (!round)
> +        return;
> +    round->count = SELF_ANNOUNCE_ROUNDS;
> +    round->timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> qemu_announce_self_once, round);

An odd line break?

> +    qemu_announce_self_once(round);
> +}
> +
> +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' }
> +

Please wire hmp up as well (see hmp-commands.hx).

Dave

> -- 
> 2.9.3
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Germano Veit Michel (germano@redhat.com) 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.
>
> Markus: Since you're asking for tests for qmp commands; how would you
> test this?

Good question, as tests/ isn't exactly full of examples you could crib.

Let me look at the patch...

> Jason: Does this look OK from the networking side of things?
>
>> Signed-off-by: Germano Veit Michel <germano@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));
>
> I prefer g_new0 - i.e.
>    struct AnnounceRound *round = g_new0(struct AnnounceRound, 1)
>
>> +    if (!round)
>> +        return;
>> +    round->count = SELF_ANNOUNCE_ROUNDS;
>> +    round->timer = timer_new_ms(QEMU_CLOCK_REALTIME,
>> qemu_announce_self_once, round);
>
> An odd line break?
>
>> +    qemu_announce_self_once(round);
>> +}
>> +
>> +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.

Please drop this line.

>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "announce-self" }
>> +# <- { "return": {} }
>> +#
>> +# Since: 2.9
>> +##
>> +{ 'command': 'announce-self' }
>> +

>From QMP's point of view, this command is as simple as they get: no
arguments, no return values, no errors.

I think a basic smoke test would do: try the command, check no magic
smoke comes out.  Untested sketch adapted from qmp-test.c:

    /*
     * Test cases for network-related QMP commands
     *
     * Copyright (c) 2017 Red Hat Inc.
     *
     * Authors:
     *  Markus Armbruster <armbru@redhat.com>,
     *
     * 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': 'qmp_capabilities' }");
        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();
    }

If you want to go the extra mile: is there a way to set up networking so
you can observe the RARPs this should trigger?

I'd call this qmp-net-test.  Add to Makefile.include exactly like
qmp-test.

Test cases for existing networking-related QMP commands should be added,
but not in this patch, and not necessarily by you.

Alternatively, have a more general test program for networking stuff,
and make this one of its test cases.  Your choice.

Hope this helps!

>
> Please wire hmp up as well (see hmp-commands.hx).
>
> Dave
>
>> -- 
>> 2.9.3
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Jason Wang]

On 2017年03月03日 18:39, Dr. David Alan Gilbert wrote:
> * Germano Veit Michel (germano@redhat.com) 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.
> Markus: Since you're asking for tests for qmp commands; how would you
> test this?
>
> Jason: Does this look OK from the networking side of things?
>

Good as a start I think. We probably want to add callbacks for each 
kinds of nic. This will be useful for virtio, since some guest can 
announce themselves with complex configurations (e.g vlans).

Thanks

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

[Germano Veit Michel]

On Fri, Mar 3, 2017 at 10:06 PM, Markus Armbruster <armbru@redhat.com> wrote:
>
> Please drop this line.
>
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "announce-self" }
>>> +# <- { "return": {} }
>>> +#
>>> +# Since: 2.9
>>> +##
>>> +{ 'command': 'announce-self' }
>>> +
>

Hi Markus,

Are you talking about the whole example section or just the empty line
at the end?

I'm preparing a V3 with hmp commands and g_new0.

Thanks,
Germano

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

[Markus Armbruster]

Germano Veit Michel <germano@redhat.com> writes:

> On Fri, Mar 3, 2017 at 10:06 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> 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.
>>
>> Please drop this line.
>>
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +# -> { "execute": "announce-self" }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +# Since: 2.9
>>>> +##
>>>> +{ 'command': 'announce-self' }
>>>> +
>>
>
> Hi Markus,
>
> Are you talking about the whole example section or just the empty line
> at the end?

The "Arguments: None" line.

[...]

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
>> * Germano Veit Michel (germano@redhat.com) 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.
>>
>> Markus: Since you're asking for tests for qmp commands; how would you
>> test this?
>
> Good question, as tests/ isn't exactly full of examples you could crib.
>
> Let me look at the patch...
>
>> Jason: Does this look OK from the networking side of things?
>>
>>> Signed-off-by: Germano Veit Michel <germano@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));
>>
>> I prefer g_new0 - i.e.
>>    struct AnnounceRound *round = g_new0(struct AnnounceRound, 1)
>>
>>> +    if (!round)
>>> +        return;
>>> +    round->count = SELF_ANNOUNCE_ROUNDS;
>>> +    round->timer = timer_new_ms(QEMU_CLOCK_REALTIME,
>>> qemu_announce_self_once, round);
>>
>> An odd line break?
>>
>>> +    qemu_announce_self_once(round);
>>> +}
>>> +
>>> +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.
>
> Please drop this line.
>
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "announce-self" }
>>> +# <- { "return": {} }
>>> +#
>>> +# Since: 2.9
>>> +##
>>> +{ 'command': 'announce-self' }
>>> +
>
> From QMP's point of view, this command is as simple as they get: no
> arguments, no return values, no errors.
>
> I think a basic smoke test would do: try the command, check no magic
> smoke comes out.  Untested sketch adapted from qmp-test.c:

Missing the obvious: should test both the success and the error case!

[...]

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

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Germano Veit Michel (germano@redhat.com) 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.
> >
> > Markus: Since you're asking for tests for qmp commands; how would you
> > test this?
> 
> Good question, as tests/ isn't exactly full of examples you could crib.
> 
> Let me look at the patch...
> 
> > Jason: Does this look OK from the networking side of things?
> >
> >> Signed-off-by: Germano Veit Michel <germano@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));
> >
> > I prefer g_new0 - i.e.
> >    struct AnnounceRound *round = g_new0(struct AnnounceRound, 1)
> >
> >> +    if (!round)
> >> +        return;
> >> +    round->count = SELF_ANNOUNCE_ROUNDS;
> >> +    round->timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> >> qemu_announce_self_once, round);
> >
> > An odd line break?
> >
> >> +    qemu_announce_self_once(round);
> >> +}
> >> +
> >> +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.
> 
> Please drop this line.
> 
> >> +#
> >> +# Example:
> >> +#
> >> +# -> { "execute": "announce-self" }
> >> +# <- { "return": {} }
> >> +#
> >> +# Since: 2.9
> >> +##
> >> +{ 'command': 'announce-self' }
> >> +
> 
> From QMP's point of view, this command is as simple as they get: no
> arguments, no return values, no errors.
> 
> I think a basic smoke test would do: try the command, check no magic
> smoke comes out.  Untested sketch adapted from qmp-test.c:
> 
>     /*
>      * Test cases for network-related QMP commands
>      *
>      * Copyright (c) 2017 Red Hat Inc.
>      *
>      * Authors:
>      *  Markus Armbruster <armbru@redhat.com>,
>      *
>      * 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': 'qmp_capabilities' }");
>         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();
>     }
> 
> If you want to go the extra mile: is there a way to set up networking so
> you can observe the RARPs this should trigger?

I've been fiddling with the RARP code for other changes and noticed you
can do:

./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -netdev user,id=netuser -object filter-dump,id=dump,netdev=netuser,file=foo2 -device e1000,netdev=netuser,mac=00:11:22:33:44:55 -monitor stdio

(Thanks to Thomas for helping explain filter-dump).

So those land in foo2 as a pcap file.

Dave

> I'd call this qmp-net-test.  Add to Makefile.include exactly like
> qmp-test.
> 
> Test cases for existing networking-related QMP commands should be added,
> but not in this patch, and not necessarily by you.
> 
> Alternatively, have a more general test program for networking stuff,
> and make this one of its test cases.  Your choice.
> 
> Hope this helps!
> 
> >
> > Please wire hmp up as well (see hmp-commands.hx).
> >
> > Dave
> >
> >> -- 
> >> 2.9.3
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Germano Veit Michel]

Hi guys,

Finally got some time to prepare V3.

First of all Dave's trick is really useful to test it:

./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -netdev
user,id=netuser -object filter-dump,id=dump,netdev=netuser,file=foo2
-device e1000,netdev=netuser,mac=00:11:22:33:44:55 -monitor stdio
QEMU 2.8.91 monitor - type 'help' for more information
(qemu) announce-self
(qemu) announce-self
(qemu) qemu-system-x86_64: terminating on signal 2

tshark -r foo2 | grep RARP
    1   0.000000 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
00:11:22:33:44:55? Tell 00:11:22:33:44:55
    2   0.050017 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
00:11:22:33:44:55? Tell 00:11:22:33:44:55
    3   0.200077 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
00:11:22:33:44:55? Tell 00:11:22:33:44:55
    4   0.450112 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
00:11:22:33:44:55? Tell 00:11:22:33:44:55
    5   0.800090 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
00:11:22:33:44:55? Tell 00:11:22:33:44:55
   13   5.583887 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
00:11:22:33:44:55? Tell 00:11:22:33:44:55
   14   5.633079 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
00:11:22:33:44:55? Tell 00:11:22:33:44:55
   15   5.783152 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
00:11:22:33:44:55? Tell 00:11:22:33:44:55
   16   6.033130 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
00:11:22:33:44:55? Tell 00:11:22:33:44:55
   17   6.383144 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
00:11:22:33:44:55? Tell 00:11:22:33:44:55

Now for qtest:

It is compiling and running my test:

  [....]
  CC      tests/qmp-net-test.o
  LINK    tests/qmp-net-test
  [....]
  GTESTER check-qtest-x86_64

/bin/sh -c printf "  %-7s %s\n" "GTESTER" "check-qtest-x86_64" &&
QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$((RANDOM % 255 + 1))} gtester -k -q
-m=quick [.....] tests/qmp-net-test [....]

Weird is... is the test qemu running without NICs?

x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/qtest-17545.sock,nowait
-qtest-log /dev/null -qmp unix:/tmp/qtest-17545.qmp,nowait -machine
accel=qtest -display none -M q35,accel=tcg -chardev
file,id=serial0,path=/tmp/qtest-boot-serial-HYHJ2e -no-shutdown -serial
chardev:serial0 -device sga

I was looking at this
http://events.linuxfoundation.org/sites/events/files/slides/Testing%20QEMU%20emulated%20devices%20using%20qtest.pdf
and it's pretty helpful. But I have no clues on how to actually check if
the RARP packets really go out on each NIC. Any idea on how to implement
this or is the smoke test enough?

Thanks

On Tue, Mar 28, 2017 at 2:31 AM, Dr. David Alan Gilbert <dgilbert@redhat.com
> wrote:

> * Markus Armbruster (armbru@redhat.com) wrote:
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> >
> > > * Germano Veit Michel (germano@redhat.com) 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.
> > >
> > > Markus: Since you're asking for tests for qmp commands; how would you
> > > test this?
> >
> > Good question, as tests/ isn't exactly full of examples you could crib.
> >
> > Let me look at the patch...
> >
> > > Jason: Does this look OK from the networking side of things?
> > >
> > >> Signed-off-by: Germano Veit Michel <germano@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));
> > >
> > > I prefer g_new0 - i.e.
> > >    struct AnnounceRound *round = g_new0(struct AnnounceRound, 1)
> > >
> > >> +    if (!round)
> > >> +        return;
> > >> +    round->count = SELF_ANNOUNCE_ROUNDS;
> > >> +    round->timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> > >> qemu_announce_self_once, round);
> > >
> > > An odd line break?
> > >
> > >> +    qemu_announce_self_once(round);
> > >> +}
> > >> +
> > >> +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.
> >
> > Please drop this line.
> >
> > >> +#
> > >> +# Example:
> > >> +#
> > >> +# -> { "execute": "announce-self" }
> > >> +# <- { "return": {} }
> > >> +#
> > >> +# Since: 2.9
> > >> +##
> > >> +{ 'command': 'announce-self' }
> > >> +
> >
> > From QMP's point of view, this command is as simple as they get: no
> > arguments, no return values, no errors.
> >
> > I think a basic smoke test would do: try the command, check no magic
> > smoke comes out.  Untested sketch adapted from qmp-test.c:
> >
> >     /*
> >      * Test cases for network-related QMP commands
> >      *
> >      * Copyright (c) 2017 Red Hat Inc.
> >      *
> >      * Authors:
> >      *  Markus Armbruster <armbru@redhat.com>,
> >      *
> >      * 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': 'qmp_capabilities' }");
> >         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();
> >     }
> >
> > If you want to go the extra mile: is there a way to set up networking so
> > you can observe the RARPs this should trigger?
>
> I've been fiddling with the RARP code for other changes and noticed you
> can do:
>
> ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -netdev
> user,id=netuser -object filter-dump,id=dump,netdev=netuser,file=foo2
> -device e1000,netdev=netuser,mac=00:11:22:33:44:55 -monitor stdio
>
> (Thanks to Thomas for helping explain filter-dump).
>
> So those land in foo2 as a pcap file.
>
> Dave
>
> > I'd call this qmp-net-test.  Add to Makefile.include exactly like
> > qmp-test.
> >
> > Test cases for existing networking-related QMP commands should be added,
> > but not in this patch, and not necessarily by you.
> >
> > Alternatively, have a more general test program for networking stuff,
> > and make this one of its test cases.  Your choice.
> >
> > Hope this helps!
> >
> > >
> > > Please wire hmp up as well (see hmp-commands.hx).
> > >
> > > Dave
> > >
> > >> --
> > >> 2.9.3
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>



-- 
Germano Veit Michel <germano@redhat.com>
Senior Software Maintenance Engineer, Virtualization, Red Hat

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

[Markus Armbruster]

Germano Veit Michel <germano@redhat.com> writes:

> Hi guys,
>
> Finally got some time to prepare V3.
>
> First of all Dave's trick is really useful to test it:
>
> ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -netdev
> user,id=netuser -object filter-dump,id=dump,netdev=netuser,file=foo2
> -device e1000,netdev=netuser,mac=00:11:22:33:44:55 -monitor stdio
> QEMU 2.8.91 monitor - type 'help' for more information
> (qemu) announce-self
> (qemu) announce-self
> (qemu) qemu-system-x86_64: terminating on signal 2
>
> tshark -r foo2 | grep RARP
>     1   0.000000 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>     2   0.050017 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>     3   0.200077 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>     4   0.450112 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>     5   0.800090 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>    13   5.583887 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>    14   5.633079 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>    15   5.783152 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>    16   6.033130 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>    17   6.383144 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>
> Now for qtest:
>
> It is compiling and running my test:
>
>   [....]
>   CC      tests/qmp-net-test.o
>   LINK    tests/qmp-net-test
>   [....]
>   GTESTER check-qtest-x86_64
>
> /bin/sh -c printf "  %-7s %s\n" "GTESTER" "check-qtest-x86_64" &&
> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$((RANDOM % 255 + 1))} gtester -k -q
> -m=quick [.....] tests/qmp-net-test [....]
>
> Weird is... is the test qemu running without NICs?

Please show us test/qmp-net-test.c.

> x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/qtest-17545.sock,nowait
> -qtest-log /dev/null -qmp unix:/tmp/qtest-17545.qmp,nowait -machine
> accel=qtest -display none -M q35,accel=tcg -chardev
> file,id=serial0,path=/tmp/qtest-boot-serial-HYHJ2e -no-shutdown -serial
> chardev:serial0 -device sga
>
> I was looking at this
> http://events.linuxfoundation.org/sites/events/files/slides/Testing%20QEMU%20emulated%20devices%20using%20qtest.pdf
> and it's pretty helpful. But I have no clues on how to actually check if
> the RARP packets really go out on each NIC. Any idea on how to implement
> this or is the smoke test enough?

I'd try to use filter-dump to capture the traffic, then compare the
actual captured traffic to the expected one.

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

[Jason Wang]

On 2017年05月05日 14:13, Markus Armbruster wrote:
> Germano Veit Michel <germano@redhat.com> writes:
>
>> Hi guys,
>>
>> Finally got some time to prepare V3.
>>
>> First of all Dave's trick is really useful to test it:
>>
>> ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -netdev
>> user,id=netuser -object filter-dump,id=dump,netdev=netuser,file=foo2
>> -device e1000,netdev=netuser,mac=00:11:22:33:44:55 -monitor stdio
>> QEMU 2.8.91 monitor - type 'help' for more information
>> (qemu) announce-self
>> (qemu) announce-self
>> (qemu) qemu-system-x86_64: terminating on signal 2
>>
>> tshark -r foo2 | grep RARP
>>      1   0.000000 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
>> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>>      2   0.050017 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
>> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>>      3   0.200077 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
>> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>>      4   0.450112 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
>> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>>      5   0.800090 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
>> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>>     13   5.583887 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
>> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>>     14   5.633079 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
>> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>>     15   5.783152 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
>> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>>     16   6.033130 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
>> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>>     17   6.383144 Cimsys_33:44:55 → Broadcast    RARP 60 Who is
>> 00:11:22:33:44:55? Tell 00:11:22:33:44:55
>>
>> Now for qtest:
>>
>> It is compiling and running my test:
>>
>>    [....]
>>    CC      tests/qmp-net-test.o
>>    LINK    tests/qmp-net-test
>>    [....]
>>    GTESTER check-qtest-x86_64
>>
>> /bin/sh -c printf "  %-7s %s\n" "GTESTER" "check-qtest-x86_64" &&
>> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img
>> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$((RANDOM % 255 + 1))} gtester -k -q
>> -m=quick [.....] tests/qmp-net-test [....]
>>
>> Weird is... is the test qemu running without NICs?
> Please show us test/qmp-net-test.c.
>
>> x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/qtest-17545.sock,nowait
>> -qtest-log /dev/null -qmp unix:/tmp/qtest-17545.qmp,nowait -machine
>> accel=qtest -display none -M q35,accel=tcg -chardev
>> file,id=serial0,path=/tmp/qtest-boot-serial-HYHJ2e -no-shutdown -serial
>> chardev:serial0 -device sga
>>
>> I was looking at this
>> http://events.linuxfoundation.org/sites/events/files/slides/Testing%20QEMU%20emulated%20devices%20using%20qtest.pdf
>> and it's pretty helpful. But I have no clues on how to actually check if
>> the RARP packets really go out on each NIC. Any idea on how to implement
>> this or is the smoke test enough?
> I'd try to use filter-dump to capture the traffic, then compare the
> actual captured traffic to the expected one.

Or use a socket backend like virtio-net-test.c.

Thanks

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

[Vlad Yasevich]

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 <germano@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.

-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' }
> +
> 

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

[Dr. David Alan Gilbert]

* Vlad Yasevich (vyasevic@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 <germano@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 / dgilbert@redhat.com / Manchester, UK

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 (vyasevic@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 <germano@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 / dgilbert@redhat.com / Manchester, UK
> 

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 <armbru@redhat.com>,
>  *
>  * 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 <vyasevic@redhat.com> wrote:

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


-- 
Germano Veit Michel <germano@redhat.com>
Senior Software Maintenance Engineer, Virtualization, Red Hat