[Qemu-devel] [PATCH v4 0/5] network announce; interface selection & IDs

[Qemu-devel] [PATCH v4 0/5] network announce; interface selection & IDs

[Dr. David Alan Gilbert (git)]

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

Laine asked for some extra features on the network announce support;

The first allows the announce timer to announce on a subset of the
interfaces.

The second allows there to be multiple timers, each with their own
parameters (including the interface list).

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

v4
  Minor typo fixes
  Expanded the test to check we can stop a running announce

Dr. David Alan Gilbert (5):
  net/announce: Allow optional list of interfaces
  net/announce: Add HMP optional interface list
  net/announce: Add optional ID
  net/announce: Add HMP optional ID
  net/announce: Expand test for stopping self announce

 hmp-commands.hx         |  7 +++-
 hmp.c                   | 41 +++++++++++++++++++-
 hw/net/virtio-net.c     |  4 +-
 include/net/announce.h  |  8 +++-
 net/announce.c          | 83 ++++++++++++++++++++++++++++++++++-------
 net/trace-events        |  3 +-
 qapi/net.json           | 16 ++++++--
 tests/virtio-net-test.c | 57 ++++++++++++++++++++++++++--
 8 files changed, 192 insertions(+), 27 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v4 1/5] net/announce: Allow optional list of interfaces

[Dr. David Alan Gilbert (git)]

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

Allow the caller to restrict the set of interfaces that announces are
sent on.  The default is still to send on all interfaces.

e.g.

  { "execute": "announce-self", "arguments": { "initial": 50, "max": 550, "rounds": 5, "step": 50, "interfaces": ["vn2", "vn1"] } }

This doesn't affect the behaviour of migraiton announcments.

Note: There's still only one timer for the qmp command, so that
performing an 'announce-self' on one list of interfaces followed
by another 'announce-self' on another list will stop the announces
on the existing set.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/net/announce.h |  2 +-
 net/announce.c         | 39 ++++++++++++++++++++++++++++++++-------
 net/trace-events       |  2 +-
 qapi/net.json          | 11 ++++++++---
 4 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/include/net/announce.h b/include/net/announce.h
index 04a035f679..773470428b 100644
--- a/include/net/announce.h
+++ b/include/net/announce.h
@@ -22,7 +22,7 @@ struct AnnounceTimer {
 /* Returns: update the timer to the next time point */
 int64_t qemu_announce_timer_step(AnnounceTimer *timer);
 
-/* Delete the underlying timer */
+/* Delete the underlying timer and other data */
 void qemu_announce_timer_del(AnnounceTimer *timer);
 
 /*
diff --git a/net/announce.c b/net/announce.c
index 91e9a6e267..1ce42b571d 100644
--- a/net/announce.c
+++ b/net/announce.c
@@ -38,6 +38,8 @@ void qemu_announce_timer_del(AnnounceTimer *timer)
         timer_free(timer->tm);
         timer->tm = NULL;
     }
+    qapi_free_strList(timer->params.interfaces);
+    timer->params.interfaces = NULL;
 }
 
 /*
@@ -96,24 +98,47 @@ static int announce_self_create(uint8_t *buf,
 
 static void qemu_announce_self_iter(NICState *nic, void *opaque)
 {
+    AnnounceTimer *timer = opaque;
     uint8_t buf[60];
     int len;
+    bool skip;
+
+    if (timer->params.has_interfaces) {
+        strList *entry = timer->params.interfaces;
+        /* Skip unless we find our name in the requested list */
+        skip = true;
+
+        while (entry) {
+            if (!strcmp(entry->value, nic->ncs->name)) {
+                /* Found us */
+                skip = false;
+                break;
+            }
+            entry = entry->next;
+        }
+    } else {
+        skip = false;
+    }
+
+    trace_qemu_announce_self_iter(nic->ncs->name,
+                                  qemu_ether_ntoa(&nic->conf->macaddr), skip);
 
-    trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
-    len = announce_self_create(buf, nic->conf->macaddr.a);
+    if (!skip) {
+        len = announce_self_create(buf, nic->conf->macaddr.a);
 
-    qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
+        qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
 
-    /* if the NIC provides it's own announcement support, use it as well */
-    if (nic->ncs->info->announce) {
-        nic->ncs->info->announce(nic->ncs);
+        /* if the NIC provides it's own announcement support, use it as well */
+        if (nic->ncs->info->announce) {
+            nic->ncs->info->announce(nic->ncs);
+        }
     }
 }
 static void qemu_announce_self_once(void *opaque)
 {
     AnnounceTimer *timer = (AnnounceTimer *)opaque;
 
-    qemu_foreach_nic(qemu_announce_self_iter, NULL);
+    qemu_foreach_nic(qemu_announce_self_iter, timer);
 
     if (--timer->round) {
         qemu_announce_timer_step(timer);
diff --git a/net/trace-events b/net/trace-events
index a7937f3f3a..875ef2a0f3 100644
--- a/net/trace-events
+++ b/net/trace-events
@@ -1,7 +1,7 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
 # announce.c
-qemu_announce_self_iter(const char *mac) "%s"
+qemu_announce_self_iter(const char *name, const char *mac, int skip) "%s:%s skip: %d"
 
 # vhost-user.c
 vhost_user_event(const char *chr, int event) "chr: %s got event: %d"
diff --git a/qapi/net.json b/qapi/net.json
index 5f7bff1637..6f2cd4f530 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -699,6 +699,9 @@
 #
 # @step: Delay increase (in ms) after each self-announcement attempt
 #
+# @interfaces: An optional list of interface names, which restricts the
+#        announcement to the listed interfaces. (Since 4.1)
+#
 # Since: 4.0
 ##
 
@@ -706,7 +709,8 @@
   'data': { 'initial': 'int',
             'max': 'int',
             'rounds': 'int',
-            'step': 'int' } }
+            'step': 'int',
+            '*interfaces': ['str'] } }
 
 ##
 # @announce-self:
@@ -718,9 +722,10 @@
 #
 # Example:
 #
-# -> { "execute": "announce-self"
+# -> { "execute": "announce-self",
 #      "arguments": {
-#          "initial": 50, "max": 550, "rounds": 10, "step": 50 } }
+#          "initial": 50, "max": 550, "rounds": 10, "step": 50,
+#          "interfaces": ["vn2", "vn3"] } }
 # <- { "return": {} }
 #
 # Since: 4.0
-- 
2.21.0

[Qemu-devel] [PATCH v4 2/5] net/announce: Add HMP optional interface list

[Dr. David Alan Gilbert (git)]

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

Add the optional interface list to the HMP command.

i.e.

   All interfaces
        announce_self

   Just the named interfaces:
        announce_self vn1,vn2

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp-commands.hx |  6 ++++--
 hmp.c           | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 3dc421cb6a..1b63372713 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -955,8 +955,8 @@ ETEXI
 
     {
         .name       = "announce_self",
-        .args_type  = "",
-        .params     = "",
+        .args_type  = "interfaces:s?",
+        .params     = "[interfaces]",
         .help       = "Trigger GARP/RARP announcements",
         .cmd        = hmp_announce_self,
     },
@@ -967,6 +967,8 @@ STEXI
 Trigger a round of GARP/RARP broadcasts; this is useful for explicitly updating the
 network infrastructure after a reconfiguration or some forms of migration.
 The timings of the round are set by the migration announce parameters.
+An optional comma separated @var{interfaces} list restricts the announce to the
+named set of interfaces.
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index d4460992b6..52efb4a4fa 100644
--- a/hmp.c
+++ b/hmp.c
@@ -27,6 +27,7 @@
 #include "monitor/monitor.h"
 #include "monitor/qdev.h"
 #include "qapi/error.h"
+#include "qapi/clone-visitor.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/qapi-builtin-visit.h"
 #include "qapi/qapi-commands-block.h"
@@ -38,6 +39,7 @@
 #include "qapi/qapi-commands-run-state.h"
 #include "qapi/qapi-commands-tpm.h"
 #include "qapi/qapi-commands-ui.h"
+#include "qapi/qapi-visit-net.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/string-input-visitor.h"
@@ -67,6 +69,32 @@ static void hmp_handle_error(Monitor *mon, Error **errp)
     }
 }
 
+/*
+ * Produce a strList from a comma separated list.
+ * A NULL or empty input string return NULL.
+ */
+static strList *strList_from_comma_list(const char *in)
+{
+    strList *res = NULL;
+    strList **hook = &res;
+
+    while (in && in[0]) {
+        char *comma = strchr(in, ',');
+        *hook = g_new0(strList, 1);
+
+        if (comma) {
+            (*hook)->value = g_strndup(in, comma - in);
+            in = comma + 1; /* skip the , */
+        } else {
+            (*hook)->value = g_strdup(in);
+            in = NULL;
+        }
+        hook = &(*hook)->next;
+    }
+
+    return res;
+}
+
 void hmp_info_name(Monitor *mon, const QDict *qdict)
 {
     NameInfo *info;
@@ -1640,7 +1668,15 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 
 void hmp_announce_self(Monitor *mon, const QDict *qdict)
 {
-    qmp_announce_self(migrate_announce_params(), NULL);
+    const char *interfaces_str = qdict_get_try_str(qdict, "interfaces");
+    AnnounceParameters *params = QAPI_CLONE(AnnounceParameters,
+                                            migrate_announce_params());
+
+    qapi_free_strList(params->interfaces);
+    params->interfaces = strList_from_comma_list(interfaces_str);
+    params->has_interfaces = params->interfaces != NULL;
+    qmp_announce_self(params, NULL);
+    qapi_free_AnnounceParameters(params);
 }
 
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
-- 
2.21.0

[Qemu-devel] [PATCH v4 3/5] net/announce: Add optional ID

[Dr. David Alan Gilbert (git)]

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

Previously there was a single instance of the timer used by
monitor triggered announces, that's OK, but when combined with the
previous change that lets you have announces for subsets of interfaces
it's a bit restrictive if you want to do different things to different
interfaces.

Add an 'id' field to the announce, and maintain a list of the
timers based on id.

This allows you to for example:
    a) Start an announce going on interface eth0 for a long time
    b) Start an announce going on interface eth1 for a long time
    c) Kill the announce on eth0 while leaving eth1 going.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/virtio-net.c    |  4 ++--
 include/net/announce.h |  8 ++++++--
 net/announce.c         | 46 +++++++++++++++++++++++++++++++++++-------
 net/trace-events       |  3 ++-
 qapi/net.json          |  9 +++++++--
 5 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index c3f5fccfd1..b9e1cd71cf 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2360,7 +2360,7 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
             timer_mod(n->announce_timer.tm,
                       qemu_clock_get_ms(n->announce_timer.type));
         } else {
-            qemu_announce_timer_del(&n->announce_timer);
+            qemu_announce_timer_del(&n->announce_timer, false);
         }
     }
 
@@ -2784,7 +2784,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp)
         virtio_net_del_queue(n, i);
     }
 
-    qemu_announce_timer_del(&n->announce_timer);
+    qemu_announce_timer_del(&n->announce_timer, false);
     g_free(n->vqs);
     qemu_del_nic(n->nic);
     virtio_net_rsc_cleanup(n);
diff --git a/include/net/announce.h b/include/net/announce.h
index 773470428b..3d90c83c23 100644
--- a/include/net/announce.h
+++ b/include/net/announce.h
@@ -22,8 +22,12 @@ struct AnnounceTimer {
 /* Returns: update the timer to the next time point */
 int64_t qemu_announce_timer_step(AnnounceTimer *timer);
 
-/* Delete the underlying timer and other data */
-void qemu_announce_timer_del(AnnounceTimer *timer);
+/*
+ * Delete the underlying timer and other data
+ * If 'free_named' true and the timer is a named timer, then remove
+ * it from the list of named timers and free the AnnounceTimer itself.
+ */
+void qemu_announce_timer_del(AnnounceTimer *timer, bool free_named);
 
 /*
  * Under BQL/main thread
diff --git a/net/announce.c b/net/announce.c
index 1ce42b571d..4d4e2c22a1 100644
--- a/net/announce.c
+++ b/net/announce.c
@@ -15,6 +15,8 @@
 #include "qapi/qapi-commands-net.h"
 #include "trace.h"
 
+static GData *named_timers;
+
 int64_t qemu_announce_timer_step(AnnounceTimer *timer)
 {
     int64_t step;
@@ -31,8 +33,13 @@ int64_t qemu_announce_timer_step(AnnounceTimer *timer)
     return step;
 }
 
-void qemu_announce_timer_del(AnnounceTimer *timer)
+/*
+ * If 'free_named' is true, then remove the timer from the list
+ * and free the timer itself.
+ */
+void qemu_announce_timer_del(AnnounceTimer *timer, bool free_named)
 {
+    bool free_timer = false;
     if (timer->tm) {
         timer_del(timer->tm);
         timer_free(timer->tm);
@@ -40,6 +47,18 @@ void qemu_announce_timer_del(AnnounceTimer *timer)
     }
     qapi_free_strList(timer->params.interfaces);
     timer->params.interfaces = NULL;
+    if (free_named && timer->params.has_id) {
+        free_timer = timer ==
+                     g_datalist_get_data(&named_timers, timer->params.id);
+        g_datalist_remove_data(&named_timers, timer->params.id);
+    }
+    trace_qemu_announce_timer_del(free_named, free_timer, timer->params.id);
+    g_free(timer->params.id);
+    timer->params.id = NULL;
+
+    if (free_timer) {
+        g_free(timer);
+    }
 }
 
 /*
@@ -56,7 +75,7 @@ void qemu_announce_timer_reset(AnnounceTimer *timer,
      * We're under the BQL, so the current timer can't
      * be firing, so we should be able to delete it.
      */
-    qemu_announce_timer_del(timer);
+    qemu_announce_timer_del(timer, false);
 
     QAPI_CLONE_MEMBERS(AnnounceParameters, &timer->params, params);
     timer->round = params->rounds;
@@ -120,7 +139,8 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
         skip = false;
     }
 
-    trace_qemu_announce_self_iter(nic->ncs->name,
+    trace_qemu_announce_self_iter(timer->params.has_id ? timer->params.id : "_",
+                                  nic->ncs->name,
                                   qemu_ether_ntoa(&nic->conf->macaddr), skip);
 
     if (!skip) {
@@ -143,7 +163,7 @@ static void qemu_announce_self_once(void *opaque)
     if (--timer->round) {
         qemu_announce_timer_step(timer);
     } else {
-        qemu_announce_timer_del(timer);
+        qemu_announce_timer_del(timer, true);
     }
 }
 
@@ -154,12 +174,24 @@ void qemu_announce_self(AnnounceTimer *timer, AnnounceParameters *params)
     if (params->rounds) {
         qemu_announce_self_once(timer);
     } else {
-        qemu_announce_timer_del(timer);
+        qemu_announce_timer_del(timer, true);
     }
 }
 
 void qmp_announce_self(AnnounceParameters *params, Error **errp)
 {
-    static AnnounceTimer announce_timer;
-    qemu_announce_self(&announce_timer, params);
+    AnnounceTimer *named_timer;
+    if (!params->has_id) {
+        params->id = g_strdup("");
+        params->has_id = true;
+    }
+
+    named_timer = g_datalist_get_data(&named_timers, params->id);
+
+    if (!named_timer) {
+        named_timer = g_new0(AnnounceTimer, 1);
+        g_datalist_set_data(&named_timers, params->id, named_timer);
+    }
+
+    qemu_announce_self(named_timer, params);
 }
diff --git a/net/trace-events b/net/trace-events
index 875ef2a0f3..ac57056497 100644
--- a/net/trace-events
+++ b/net/trace-events
@@ -1,7 +1,8 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
 # announce.c
-qemu_announce_self_iter(const char *name, const char *mac, int skip) "%s:%s skip: %d"
+qemu_announce_self_iter(const char *id, const char *name, const char *mac, int skip) "%s:%s:%s skip: %d"
+qemu_announce_timer_del(bool free_named, bool free_timer, char *id) "free named: %d free timer: %d id: %s"
 
 # vhost-user.c
 vhost_user_event(const char *chr, int event) "chr: %s got event: %d"
diff --git a/qapi/net.json b/qapi/net.json
index 6f2cd4f530..728990f4fb 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -702,6 +702,10 @@
 # @interfaces: An optional list of interface names, which restricts the
 #        announcement to the listed interfaces. (Since 4.1)
 #
+# @id: A name to be used to identify an instance of announce-timers
+#        and to allow it to modified later.  Not for use as
+#        part of the migration parameters. (Since 4.1)
+#
 # Since: 4.0
 ##
 
@@ -710,7 +714,8 @@
             'max': 'int',
             'rounds': 'int',
             'step': 'int',
-            '*interfaces': ['str'] } }
+            '*interfaces': ['str'],
+            '*id' : 'str' } }
 
 ##
 # @announce-self:
@@ -725,7 +730,7 @@
 # -> { "execute": "announce-self",
 #      "arguments": {
 #          "initial": 50, "max": 550, "rounds": 10, "step": 50,
-#          "interfaces": ["vn2", "vn3"] } }
+#          "interfaces": ["vn2", "vn3"], "id": "bob" } }
 # <- { "return": {} }
 #
 # Since: 4.0
-- 
2.21.0

[Qemu-devel] [PATCH v4 4/5] net/announce: Add HMP optional ID

[Dr. David Alan Gilbert (git)]

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

Add the optional ID to the HMP command.

e.g.
   # start an announce for a long time on eth1
   migrate_set_parameter announce-rounds 1000
   announce_self "eth1" e1

   # start an announce on eth2
   announce_self "eth2" e2

   # Change e1 to be announcing on eth1 and eth3
   announce_self "eth1,eth3" e1

   # Cancel e1
   migrate_set_parameter announce-rounds 0
   announce_self "" e1

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp-commands.hx | 7 ++++---
 hmp.c           | 3 +++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 1b63372713..7ba8543cc3 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -955,8 +955,8 @@ ETEXI
 
     {
         .name       = "announce_self",
-        .args_type  = "interfaces:s?",
-        .params     = "[interfaces]",
+        .args_type  = "interfaces:s?,id:s?",
+        .params     = "[interfaces] [id]",
         .help       = "Trigger GARP/RARP announcements",
         .cmd        = hmp_announce_self,
     },
@@ -968,7 +968,8 @@ Trigger a round of GARP/RARP broadcasts; this is useful for explicitly updating
 network infrastructure after a reconfiguration or some forms of migration.
 The timings of the round are set by the migration announce parameters.
 An optional comma separated @var{interfaces} list restricts the announce to the
-named set of interfaces.
+named set of interfaces. An optional @var{id} can be used to start a separate announce
+timer and to change the parameters of it later.
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index 52efb4a4fa..fd498ca0a8 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1669,12 +1669,15 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 void hmp_announce_self(Monitor *mon, const QDict *qdict)
 {
     const char *interfaces_str = qdict_get_try_str(qdict, "interfaces");
+    const char *id = qdict_get_try_str(qdict, "id");
     AnnounceParameters *params = QAPI_CLONE(AnnounceParameters,
                                             migrate_announce_params());
 
     qapi_free_strList(params->interfaces);
     params->interfaces = strList_from_comma_list(interfaces_str);
     params->has_interfaces = params->interfaces != NULL;
+    params->id = g_strdup(id);
+    params->has_id = !!params->id;
     qmp_announce_self(params, NULL);
     qapi_free_AnnounceParameters(params);
 }
-- 
2.21.0

[Qemu-devel] [PATCH v4 5/5] net/announce: Expand test for stopping self announce

[Dr. David Alan Gilbert (git)]

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

Expand self-announce test to check we can stop an announce timer.
We set it up to send 300 packets, but after we receive
the first one we tell it to stop.

We error if:
   a) We receive more than 30 of the packets
   b) We're still receiving packets after a lot longer than the
      30 seconds should have arrived

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tests/virtio-net-test.c | 57 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 3 deletions(-)

diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index 663cf7ea7e..3b49b431dc 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -184,21 +184,72 @@ static void announce_self(void *obj, void *data, QGuestAllocator *t_alloc)
     QDict *rsp;
     int ret;
     uint16_t *proto = (uint16_t *)&buffer[12];
+    size_t total_received = 0;
+    uint64_t start, now, last_rxt, deadline;
 
+    /* Send a set of packets over a few second period */
     rsp = qmp("{ 'execute' : 'announce-self', "
                   " 'arguments': {"
-                      " 'initial': 50, 'max': 550,"
-                      " 'rounds': 10, 'step': 50 } }");
+                      " 'initial': 20, 'max': 100,"
+                      " 'rounds': 300, 'step': 10, 'id': 'bob' } }");
     assert(!qdict_haskey(rsp, "error"));
     qobject_unref(rsp);
 
-    /* Catch the packet and make sure it's a RARP */
+    /* Catch the first packet and make sure it's a RARP */
     ret = qemu_recv(sv[0], &len, sizeof(len), 0);
     g_assert_cmpint(ret, ==,  sizeof(len));
     len = ntohl(len);
 
     ret = qemu_recv(sv[0], buffer, len, 0);
     g_assert_cmpint(*proto, ==, htons(ETH_P_RARP));
+
+    /*
+     * Stop the announcment by settings rounds to 0 on the
+     * existing timer.
+     */
+    rsp = qmp("{ 'execute' : 'announce-self', "
+                  " 'arguments': {"
+                      " 'initial': 20, 'max': 100,"
+                      " 'rounds': 0, 'step': 10, 'id': 'bob' } }");
+    assert(!qdict_haskey(rsp, "error"));
+    qobject_unref(rsp);
+
+    /* Now make sure the packets stop */
+
+    /* Times are in us */
+    start = g_get_monotonic_time();
+    /* 30 packets, max gap 100ms, * 2 for wiggle */
+    deadline = start + 1000 * (100 * 30 * 2);
+    last_rxt = start;
+
+    do {
+        int saved_err;
+        ret = qemu_recv(sv[0], buffer, 60, MSG_DONTWAIT);
+        saved_err = errno;
+        now = g_get_monotonic_time();
+        g_assert_cmpint(now, <, deadline);
+
+        if (ret >= 0) {
+            if (ret) {
+                last_rxt = now;
+            }
+            total_received += ret;
+
+            /* Check it's not spewing loads */
+            g_assert_cmpint(total_received, <, 60 * 30 * 2);
+        } else {
+            g_assert_cmpint(saved_err, ==, EAGAIN);
+
+            /* 400ms, i.e. 4 worst case gaps */
+            if ((now - last_rxt) > (1000 * 100 * 4)) {
+                /* Nothings arrived for a while - must have stopped */
+                break;
+            };
+
+            /* 100ms */
+            g_usleep(1000 * 100);
+        }
+    } while (true);
 }
 
 static void virtio_net_test_cleanup(void *sockets)
-- 
2.21.0

Re: [Qemu-devel] [PATCH v4 0/5] network announce; interface selection & IDs

[Jason Wang]

On 2019/6/13 下午5:59, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Laine asked for some extra features on the network announce support;


It's better to explain why this feature is needed.  Is this because 
libvirt can change the host network topology on the fly?

Thanks


>
> The first allows the announce timer to announce on a subset of the
> interfaces.
>
> The second allows there to be multiple timers, each with their own
> parameters (including the interface list).
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> v4
>    Minor typo fixes
>    Expanded the test to check we can stop a running announce
>
> Dr. David Alan Gilbert (5):
>    net/announce: Allow optional list of interfaces
>    net/announce: Add HMP optional interface list
>    net/announce: Add optional ID
>    net/announce: Add HMP optional ID
>    net/announce: Expand test for stopping self announce
>
>   hmp-commands.hx         |  7 +++-
>   hmp.c                   | 41 +++++++++++++++++++-
>   hw/net/virtio-net.c     |  4 +-
>   include/net/announce.h  |  8 +++-
>   net/announce.c          | 83 ++++++++++++++++++++++++++++++++++-------
>   net/trace-events        |  3 +-
>   qapi/net.json           | 16 ++++++--
>   tests/virtio-net-test.c | 57 ++++++++++++++++++++++++++--
>   8 files changed, 192 insertions(+), 27 deletions(-)
>

Re: [Qemu-devel] [PATCH v4 1/5] net/announce: Allow optional list of interfaces

[Jason Wang]

On 2019/6/13 下午5:59, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Allow the caller to restrict the set of interfaces that announces are
> sent on.  The default is still to send on all interfaces.
>
> e.g.
>
>    { "execute": "announce-self", "arguments": { "initial": 50, "max": 550, "rounds": 5, "step": 50, "interfaces": ["vn2", "vn1"] } }
>
> This doesn't affect the behaviour of migraiton announcments.
>
> Note: There's still only one timer for the qmp command, so that
> performing an 'announce-self' on one list of interfaces followed
> by another 'announce-self' on another list will stop the announces
> on the existing set.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   include/net/announce.h |  2 +-
>   net/announce.c         | 39 ++++++++++++++++++++++++++++++++-------
>   net/trace-events       |  2 +-
>   qapi/net.json          | 11 ++++++++---
>   4 files changed, 42 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/announce.h b/include/net/announce.h
> index 04a035f679..773470428b 100644
> --- a/include/net/announce.h
> +++ b/include/net/announce.h
> @@ -22,7 +22,7 @@ struct AnnounceTimer {
>   /* Returns: update the timer to the next time point */
>   int64_t qemu_announce_timer_step(AnnounceTimer *timer);
>   
> -/* Delete the underlying timer */
> +/* Delete the underlying timer and other data */
>   void qemu_announce_timer_del(AnnounceTimer *timer);
>   
>   /*
> diff --git a/net/announce.c b/net/announce.c
> index 91e9a6e267..1ce42b571d 100644
> --- a/net/announce.c
> +++ b/net/announce.c
> @@ -38,6 +38,8 @@ void qemu_announce_timer_del(AnnounceTimer *timer)
>           timer_free(timer->tm);
>           timer->tm = NULL;
>       }
> +    qapi_free_strList(timer->params.interfaces);
> +    timer->params.interfaces = NULL;
>   }
>   
>   /*
> @@ -96,24 +98,47 @@ static int announce_self_create(uint8_t *buf,
>   
>   static void qemu_announce_self_iter(NICState *nic, void *opaque)
>   {
> +    AnnounceTimer *timer = opaque;
>       uint8_t buf[60];
>       int len;
> +    bool skip;
> +
> +    if (timer->params.has_interfaces) {
> +        strList *entry = timer->params.interfaces;
> +        /* Skip unless we find our name in the requested list */
> +        skip = true;
> +
> +        while (entry) {
> +            if (!strcmp(entry->value, nic->ncs->name)) {
> +                /* Found us */
> +                skip = false;
> +                break;
> +            }
> +            entry = entry->next;
> +        }
> +    } else {
> +        skip = false;
> +    }


I wonder whether or not it's better to filter the name on the caller.

Thanks


> +
> +    trace_qemu_announce_self_iter(nic->ncs->name,
> +                                  qemu_ether_ntoa(&nic->conf->macaddr), skip);
>   
> -    trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> -    len = announce_self_create(buf, nic->conf->macaddr.a);
> +    if (!skip) {
> +        len = announce_self_create(buf, nic->conf->macaddr.a);
>   
> -    qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> +        qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
>   
> -    /* if the NIC provides it's own announcement support, use it as well */
> -    if (nic->ncs->info->announce) {
> -        nic->ncs->info->announce(nic->ncs);
> +        /* if the NIC provides it's own announcement support, use it as well */
> +        if (nic->ncs->info->announce) {
> +            nic->ncs->info->announce(nic->ncs);
> +        }
>       }
>   }
>   static void qemu_announce_self_once(void *opaque)
>   {
>       AnnounceTimer *timer = (AnnounceTimer *)opaque;
>   
> -    qemu_foreach_nic(qemu_announce_self_iter, NULL);
> +    qemu_foreach_nic(qemu_announce_self_iter, timer);
>   
>       if (--timer->round) {
>           qemu_announce_timer_step(timer);
> diff --git a/net/trace-events b/net/trace-events
> index a7937f3f3a..875ef2a0f3 100644
> --- a/net/trace-events
> +++ b/net/trace-events
> @@ -1,7 +1,7 @@
>   # See docs/devel/tracing.txt for syntax documentation.
>   
>   # announce.c
> -qemu_announce_self_iter(const char *mac) "%s"
> +qemu_announce_self_iter(const char *name, const char *mac, int skip) "%s:%s skip: %d"
>   
>   # vhost-user.c
>   vhost_user_event(const char *chr, int event) "chr: %s got event: %d"
> diff --git a/qapi/net.json b/qapi/net.json
> index 5f7bff1637..6f2cd4f530 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -699,6 +699,9 @@
>   #
>   # @step: Delay increase (in ms) after each self-announcement attempt
>   #
> +# @interfaces: An optional list of interface names, which restricts the
> +#        announcement to the listed interfaces. (Since 4.1)
> +#
>   # Since: 4.0
>   ##
>   
> @@ -706,7 +709,8 @@
>     'data': { 'initial': 'int',
>               'max': 'int',
>               'rounds': 'int',
> -            'step': 'int' } }
> +            'step': 'int',
> +            '*interfaces': ['str'] } }
>   
>   ##
>   # @announce-self:
> @@ -718,9 +722,10 @@
>   #
>   # Example:
>   #
> -# -> { "execute": "announce-self"
> +# -> { "execute": "announce-self",
>   #      "arguments": {
> -#          "initial": 50, "max": 550, "rounds": 10, "step": 50 } }
> +#          "initial": 50, "max": 550, "rounds": 10, "step": 50,
> +#          "interfaces": ["vn2", "vn3"] } }
>   # <- { "return": {} }
>   #
>   # Since: 4.0

Re: [Qemu-devel] [PATCH v4 3/5] net/announce: Add optional ID

[Jason Wang]

On 2019/6/13 下午5:59, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Previously there was a single instance of the timer used by
> monitor triggered announces, that's OK, but when combined with the
> previous change that lets you have announces for subsets of interfaces
> it's a bit restrictive if you want to do different things to different
> interfaces.
>
> Add an 'id' field to the announce, and maintain a list of the
> timers based on id.
>
> This allows you to for example:
>      a) Start an announce going on interface eth0 for a long time
>      b) Start an announce going on interface eth1 for a long time
>      c) Kill the announce on eth0 while leaving eth1 going.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   hw/net/virtio-net.c    |  4 ++--
>   include/net/announce.h |  8 ++++++--
>   net/announce.c         | 46 +++++++++++++++++++++++++++++++++++-------
>   net/trace-events       |  3 ++-
>   qapi/net.json          |  9 +++++++--
>   5 files changed, 56 insertions(+), 14 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index c3f5fccfd1..b9e1cd71cf 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2360,7 +2360,7 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>               timer_mod(n->announce_timer.tm,
>                         qemu_clock_get_ms(n->announce_timer.type));
>           } else {
> -            qemu_announce_timer_del(&n->announce_timer);
> +            qemu_announce_timer_del(&n->announce_timer, false);
>           }
>       }
>   
> @@ -2784,7 +2784,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp)
>           virtio_net_del_queue(n, i);
>       }
>   
> -    qemu_announce_timer_del(&n->announce_timer);
> +    qemu_announce_timer_del(&n->announce_timer, false);
>       g_free(n->vqs);
>       qemu_del_nic(n->nic);
>       virtio_net_rsc_cleanup(n);
> diff --git a/include/net/announce.h b/include/net/announce.h
> index 773470428b..3d90c83c23 100644
> --- a/include/net/announce.h
> +++ b/include/net/announce.h
> @@ -22,8 +22,12 @@ struct AnnounceTimer {
>   /* Returns: update the timer to the next time point */
>   int64_t qemu_announce_timer_step(AnnounceTimer *timer);
>   
> -/* Delete the underlying timer and other data */
> -void qemu_announce_timer_del(AnnounceTimer *timer);
> +/*
> + * Delete the underlying timer and other data
> + * If 'free_named' true and the timer is a named timer, then remove
> + * it from the list of named timers and free the AnnounceTimer itself.
> + */
> +void qemu_announce_timer_del(AnnounceTimer *timer, bool free_named);
>   
>   /*
>    * Under BQL/main thread
> diff --git a/net/announce.c b/net/announce.c
> index 1ce42b571d..4d4e2c22a1 100644
> --- a/net/announce.c
> +++ b/net/announce.c
> @@ -15,6 +15,8 @@
>   #include "qapi/qapi-commands-net.h"
>   #include "trace.h"
>   
> +static GData *named_timers;
> +
>   int64_t qemu_announce_timer_step(AnnounceTimer *timer)
>   {
>       int64_t step;
> @@ -31,8 +33,13 @@ int64_t qemu_announce_timer_step(AnnounceTimer *timer)
>       return step;
>   }
>   
> -void qemu_announce_timer_del(AnnounceTimer *timer)
> +/*
> + * If 'free_named' is true, then remove the timer from the list
> + * and free the timer itself.
> + */
> +void qemu_announce_timer_del(AnnounceTimer *timer, bool free_named)
>   {
> +    bool free_timer = false;
>       if (timer->tm) {
>           timer_del(timer->tm);
>           timer_free(timer->tm);
> @@ -40,6 +47,18 @@ void qemu_announce_timer_del(AnnounceTimer *timer)
>       }
>       qapi_free_strList(timer->params.interfaces);
>       timer->params.interfaces = NULL;
> +    if (free_named && timer->params.has_id) {
> +        free_timer = timer ==
> +                     g_datalist_get_data(&named_timers, timer->params.id);


Any chance that the timer get from datalist is different from the one 
that caller passed to us?

Thanks


> +        g_datalist_remove_data(&named_timers, timer->params.id);
> +    }
> +    trace_qemu_announce_timer_del(free_named, free_timer, timer->params.id);
> +    g_free(timer->params.id);
> +    timer->params.id = NULL;
> +
> +    if (free_timer) {
> +        g_free(timer);
> +    }
>   }
>   
>   /*
> @@ -56,7 +75,7 @@ void qemu_announce_timer_reset(AnnounceTimer *timer,
>        * We're under the BQL, so the current timer can't
>        * be firing, so we should be able to delete it.
>        */
> -    qemu_announce_timer_del(timer);
> +    qemu_announce_timer_del(timer, false);
>   
>       QAPI_CLONE_MEMBERS(AnnounceParameters, &timer->params, params);
>       timer->round = params->rounds;
> @@ -120,7 +139,8 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
>           skip = false;
>       }
>   
> -    trace_qemu_announce_self_iter(nic->ncs->name,
> +    trace_qemu_announce_self_iter(timer->params.has_id ? timer->params.id : "_",
> +                                  nic->ncs->name,
>                                     qemu_ether_ntoa(&nic->conf->macaddr), skip);
>   
>       if (!skip) {
> @@ -143,7 +163,7 @@ static void qemu_announce_self_once(void *opaque)
>       if (--timer->round) {
>           qemu_announce_timer_step(timer);
>       } else {
> -        qemu_announce_timer_del(timer);
> +        qemu_announce_timer_del(timer, true);
>       }
>   }
>   
> @@ -154,12 +174,24 @@ void qemu_announce_self(AnnounceTimer *timer, AnnounceParameters *params)
>       if (params->rounds) {
>           qemu_announce_self_once(timer);
>       } else {
> -        qemu_announce_timer_del(timer);
> +        qemu_announce_timer_del(timer, true);
>       }
>   }
>   
>   void qmp_announce_self(AnnounceParameters *params, Error **errp)
>   {
> -    static AnnounceTimer announce_timer;
> -    qemu_announce_self(&announce_timer, params);
> +    AnnounceTimer *named_timer;
> +    if (!params->has_id) {
> +        params->id = g_strdup("");
> +        params->has_id = true;
> +    }
> +
> +    named_timer = g_datalist_get_data(&named_timers, params->id);
> +
> +    if (!named_timer) {
> +        named_timer = g_new0(AnnounceTimer, 1);
> +        g_datalist_set_data(&named_timers, params->id, named_timer);
> +    }
> +
> +    qemu_announce_self(named_timer, params);
>   }
> diff --git a/net/trace-events b/net/trace-events
> index 875ef2a0f3..ac57056497 100644
> --- a/net/trace-events
> +++ b/net/trace-events
> @@ -1,7 +1,8 @@
>   # See docs/devel/tracing.txt for syntax documentation.
>   
>   # announce.c
> -qemu_announce_self_iter(const char *name, const char *mac, int skip) "%s:%s skip: %d"
> +qemu_announce_self_iter(const char *id, const char *name, const char *mac, int skip) "%s:%s:%s skip: %d"
> +qemu_announce_timer_del(bool free_named, bool free_timer, char *id) "free named: %d free timer: %d id: %s"
>   
>   # vhost-user.c
>   vhost_user_event(const char *chr, int event) "chr: %s got event: %d"
> diff --git a/qapi/net.json b/qapi/net.json
> index 6f2cd4f530..728990f4fb 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -702,6 +702,10 @@
>   # @interfaces: An optional list of interface names, which restricts the
>   #        announcement to the listed interfaces. (Since 4.1)
>   #
> +# @id: A name to be used to identify an instance of announce-timers
> +#        and to allow it to modified later.  Not for use as
> +#        part of the migration parameters. (Since 4.1)
> +#
>   # Since: 4.0
>   ##
>   
> @@ -710,7 +714,8 @@
>               'max': 'int',
>               'rounds': 'int',
>               'step': 'int',
> -            '*interfaces': ['str'] } }
> +            '*interfaces': ['str'],
> +            '*id' : 'str' } }
>   
>   ##
>   # @announce-self:
> @@ -725,7 +730,7 @@
>   # -> { "execute": "announce-self",
>   #      "arguments": {
>   #          "initial": 50, "max": 550, "rounds": 10, "step": 50,
> -#          "interfaces": ["vn2", "vn3"] } }
> +#          "interfaces": ["vn2", "vn3"], "id": "bob" } }
>   # <- { "return": {} }
>   #
>   # Since: 4.0

Re: [Qemu-devel] [PATCH v4 5/5] net/announce: Expand test for stopping self announce

[Jason Wang]

On 2019/6/13 下午5:59, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Expand self-announce test to check we can stop an announce timer.
> We set it up to send 300 packets, but after we receive
> the first one we tell it to stop.
>
> We error if:
>     a) We receive more than 30 of the packets
>     b) We're still receiving packets after a lot longer than the
>        30 seconds should have arrived
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   tests/virtio-net-test.c | 57 ++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
> index 663cf7ea7e..3b49b431dc 100644
> --- a/tests/virtio-net-test.c
> +++ b/tests/virtio-net-test.c
> @@ -184,21 +184,72 @@ static void announce_self(void *obj, void *data, QGuestAllocator *t_alloc)
>       QDict *rsp;
>       int ret;
>       uint16_t *proto = (uint16_t *)&buffer[12];
> +    size_t total_received = 0;
> +    uint64_t start, now, last_rxt, deadline;
>   
> +    /* Send a set of packets over a few second period */
>       rsp = qmp("{ 'execute' : 'announce-self', "
>                     " 'arguments': {"
> -                      " 'initial': 50, 'max': 550,"
> -                      " 'rounds': 10, 'step': 50 } }");
> +                      " 'initial': 20, 'max': 100,"
> +                      " 'rounds': 300, 'step': 10, 'id': 'bob' } }");
>       assert(!qdict_haskey(rsp, "error"));
>       qobject_unref(rsp);
>   
> -    /* Catch the packet and make sure it's a RARP */
> +    /* Catch the first packet and make sure it's a RARP */
>       ret = qemu_recv(sv[0], &len, sizeof(len), 0);
>       g_assert_cmpint(ret, ==,  sizeof(len));
>       len = ntohl(len);
>   
>       ret = qemu_recv(sv[0], buffer, len, 0);
>       g_assert_cmpint(*proto, ==, htons(ETH_P_RARP));
> +
> +    /*
> +     * Stop the announcment by settings rounds to 0 on the
> +     * existing timer.
> +     */
> +    rsp = qmp("{ 'execute' : 'announce-self', "
> +                  " 'arguments': {"
> +                      " 'initial': 20, 'max': 100,"
> +                      " 'rounds': 0, 'step': 10, 'id': 'bob' } }");
> +    assert(!qdict_haskey(rsp, "error"));
> +    qobject_unref(rsp);
> +
> +    /* Now make sure the packets stop */
> +
> +    /* Times are in us */
> +    start = g_get_monotonic_time();
> +    /* 30 packets, max gap 100ms, * 2 for wiggle */
> +    deadline = start + 1000 * (100 * 30 * 2);
> +    last_rxt = start;
> +
> +    do {


while (ture) looks better here.


> +        int saved_err;
> +        ret = qemu_recv(sv[0], buffer, 60, MSG_DONTWAIT);
> +        saved_err = errno;
> +        now = g_get_monotonic_time();
> +        g_assert_cmpint(now, <, deadline);

The maximum gap allowed is 1000 * 100 * 4, and we allow at most 30 
packets that's 30 * 1000 * 100 * 4 which is 1200000.

But the deadline is 1000 * 100 * 30 * 2 which is 6000000.

Does this mean deadline is conflict with the assumption above?

Thanks


> +
> +        if (ret >= 0) {
> +            if (ret) {
> +                last_rxt = now;
> +            }
> +            total_received += ret;
> +
> +            /* Check it's not spewing loads */
> +            g_assert_cmpint(total_received, <, 60 * 30 * 2);
> +        } else {
> +            g_assert_cmpint(saved_err, ==, EAGAIN);
> +
> +            /* 400ms, i.e. 4 worst case gaps */
> +            if ((now - last_rxt) > (1000 * 100 * 4)) {
> +                /* Nothings arrived for a while - must have stopped */
> +                break;
> +            };
> +
> +            /* 100ms */
> +            g_usleep(1000 * 100);
> +        }
> +    } while (true);
>   }
>   
>   static void virtio_net_test_cleanup(void *sockets)

Re: [Qemu-devel] [PATCH v4 1/5] net/announce: Allow optional list of interfaces

[Dr. David Alan Gilbert]

* Jason Wang (jasowang@redhat.com) wrote:
> 
> On 2019/6/13 下午5:59, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Allow the caller to restrict the set of interfaces that announces are
> > sent on.  The default is still to send on all interfaces.
> > 
> > e.g.
> > 
> >    { "execute": "announce-self", "arguments": { "initial": 50, "max": 550, "rounds": 5, "step": 50, "interfaces": ["vn2", "vn1"] } }
> > 
> > This doesn't affect the behaviour of migraiton announcments.
> > 
> > Note: There's still only one timer for the qmp command, so that
> > performing an 'announce-self' on one list of interfaces followed
> > by another 'announce-self' on another list will stop the announces
> > on the existing set.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >   include/net/announce.h |  2 +-
> >   net/announce.c         | 39 ++++++++++++++++++++++++++++++++-------
> >   net/trace-events       |  2 +-
> >   qapi/net.json          | 11 ++++++++---
> >   4 files changed, 42 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/net/announce.h b/include/net/announce.h
> > index 04a035f679..773470428b 100644
> > --- a/include/net/announce.h
> > +++ b/include/net/announce.h
> > @@ -22,7 +22,7 @@ struct AnnounceTimer {
> >   /* Returns: update the timer to the next time point */
> >   int64_t qemu_announce_timer_step(AnnounceTimer *timer);
> > -/* Delete the underlying timer */
> > +/* Delete the underlying timer and other data */
> >   void qemu_announce_timer_del(AnnounceTimer *timer);
> >   /*
> > diff --git a/net/announce.c b/net/announce.c
> > index 91e9a6e267..1ce42b571d 100644
> > --- a/net/announce.c
> > +++ b/net/announce.c
> > @@ -38,6 +38,8 @@ void qemu_announce_timer_del(AnnounceTimer *timer)
> >           timer_free(timer->tm);
> >           timer->tm = NULL;
> >       }
> > +    qapi_free_strList(timer->params.interfaces);
> > +    timer->params.interfaces = NULL;
> >   }
> >   /*
> > @@ -96,24 +98,47 @@ static int announce_self_create(uint8_t *buf,
> >   static void qemu_announce_self_iter(NICState *nic, void *opaque)
> >   {
> > +    AnnounceTimer *timer = opaque;
> >       uint8_t buf[60];
> >       int len;
> > +    bool skip;
> > +
> > +    if (timer->params.has_interfaces) {
> > +        strList *entry = timer->params.interfaces;
> > +        /* Skip unless we find our name in the requested list */
> > +        skip = true;
> > +
> > +        while (entry) {
> > +            if (!strcmp(entry->value, nic->ncs->name)) {
> > +                /* Found us */
> > +                skip = false;
> > +                break;
> > +            }
> > +            entry = entry->next;
> > +        }
> > +    } else {
> > +        skip = false;
> > +    }
> 
> 
> I wonder whether or not it's better to filter the name on the caller.

Doing it this way means I don't have to worry about any hotplug that
might happen during the announce period.

Dave

> Thanks
> 
> 
> > +
> > +    trace_qemu_announce_self_iter(nic->ncs->name,
> > +                                  qemu_ether_ntoa(&nic->conf->macaddr), skip);
> > -    trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> > -    len = announce_self_create(buf, nic->conf->macaddr.a);
> > +    if (!skip) {
> > +        len = announce_self_create(buf, nic->conf->macaddr.a);
> > -    qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> > +        qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> > -    /* if the NIC provides it's own announcement support, use it as well */
> > -    if (nic->ncs->info->announce) {
> > -        nic->ncs->info->announce(nic->ncs);
> > +        /* if the NIC provides it's own announcement support, use it as well */
> > +        if (nic->ncs->info->announce) {
> > +            nic->ncs->info->announce(nic->ncs);
> > +        }
> >       }
> >   }
> >   static void qemu_announce_self_once(void *opaque)
> >   {
> >       AnnounceTimer *timer = (AnnounceTimer *)opaque;
> > -    qemu_foreach_nic(qemu_announce_self_iter, NULL);
> > +    qemu_foreach_nic(qemu_announce_self_iter, timer);
> >       if (--timer->round) {
> >           qemu_announce_timer_step(timer);
> > diff --git a/net/trace-events b/net/trace-events
> > index a7937f3f3a..875ef2a0f3 100644
> > --- a/net/trace-events
> > +++ b/net/trace-events
> > @@ -1,7 +1,7 @@
> >   # See docs/devel/tracing.txt for syntax documentation.
> >   # announce.c
> > -qemu_announce_self_iter(const char *mac) "%s"
> > +qemu_announce_self_iter(const char *name, const char *mac, int skip) "%s:%s skip: %d"
> >   # vhost-user.c
> >   vhost_user_event(const char *chr, int event) "chr: %s got event: %d"
> > diff --git a/qapi/net.json b/qapi/net.json
> > index 5f7bff1637..6f2cd4f530 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -699,6 +699,9 @@
> >   #
> >   # @step: Delay increase (in ms) after each self-announcement attempt
> >   #
> > +# @interfaces: An optional list of interface names, which restricts the
> > +#        announcement to the listed interfaces. (Since 4.1)
> > +#
> >   # Since: 4.0
> >   ##
> > @@ -706,7 +709,8 @@
> >     'data': { 'initial': 'int',
> >               'max': 'int',
> >               'rounds': 'int',
> > -            'step': 'int' } }
> > +            'step': 'int',
> > +            '*interfaces': ['str'] } }
> >   ##
> >   # @announce-self:
> > @@ -718,9 +722,10 @@
> >   #
> >   # Example:
> >   #
> > -# -> { "execute": "announce-self"
> > +# -> { "execute": "announce-self",
> >   #      "arguments": {
> > -#          "initial": 50, "max": 550, "rounds": 10, "step": 50 } }
> > +#          "initial": 50, "max": 550, "rounds": 10, "step": 50,
> > +#          "interfaces": ["vn2", "vn3"] } }
> >   # <- { "return": {} }
> >   #
> >   # Since: 4.0
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 3/5] net/announce: Add optional ID

[Dr. David Alan Gilbert]

* Jason Wang (jasowang@redhat.com) wrote:
> 
> On 2019/6/13 下午5:59, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Previously there was a single instance of the timer used by
> > monitor triggered announces, that's OK, but when combined with the
> > previous change that lets you have announces for subsets of interfaces
> > it's a bit restrictive if you want to do different things to different
> > interfaces.
> > 
> > Add an 'id' field to the announce, and maintain a list of the
> > timers based on id.
> > 
> > This allows you to for example:
> >      a) Start an announce going on interface eth0 for a long time
> >      b) Start an announce going on interface eth1 for a long time
> >      c) Kill the announce on eth0 while leaving eth1 going.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >   hw/net/virtio-net.c    |  4 ++--
> >   include/net/announce.h |  8 ++++++--
> >   net/announce.c         | 46 +++++++++++++++++++++++++++++++++++-------
> >   net/trace-events       |  3 ++-
> >   qapi/net.json          |  9 +++++++--
> >   5 files changed, 56 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index c3f5fccfd1..b9e1cd71cf 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -2360,7 +2360,7 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
> >               timer_mod(n->announce_timer.tm,
> >                         qemu_clock_get_ms(n->announce_timer.type));
> >           } else {
> > -            qemu_announce_timer_del(&n->announce_timer);
> > +            qemu_announce_timer_del(&n->announce_timer, false);
> >           }
> >       }
> > @@ -2784,7 +2784,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp)
> >           virtio_net_del_queue(n, i);
> >       }
> > -    qemu_announce_timer_del(&n->announce_timer);
> > +    qemu_announce_timer_del(&n->announce_timer, false);
> >       g_free(n->vqs);
> >       qemu_del_nic(n->nic);
> >       virtio_net_rsc_cleanup(n);
> > diff --git a/include/net/announce.h b/include/net/announce.h
> > index 773470428b..3d90c83c23 100644
> > --- a/include/net/announce.h
> > +++ b/include/net/announce.h
> > @@ -22,8 +22,12 @@ struct AnnounceTimer {
> >   /* Returns: update the timer to the next time point */
> >   int64_t qemu_announce_timer_step(AnnounceTimer *timer);
> > -/* Delete the underlying timer and other data */
> > -void qemu_announce_timer_del(AnnounceTimer *timer);
> > +/*
> > + * Delete the underlying timer and other data
> > + * If 'free_named' true and the timer is a named timer, then remove
> > + * it from the list of named timers and free the AnnounceTimer itself.
> > + */
> > +void qemu_announce_timer_del(AnnounceTimer *timer, bool free_named);
> >   /*
> >    * Under BQL/main thread
> > diff --git a/net/announce.c b/net/announce.c
> > index 1ce42b571d..4d4e2c22a1 100644
> > --- a/net/announce.c
> > +++ b/net/announce.c
> > @@ -15,6 +15,8 @@
> >   #include "qapi/qapi-commands-net.h"
> >   #include "trace.h"
> > +static GData *named_timers;
> > +
> >   int64_t qemu_announce_timer_step(AnnounceTimer *timer)
> >   {
> >       int64_t step;
> > @@ -31,8 +33,13 @@ int64_t qemu_announce_timer_step(AnnounceTimer *timer)
> >       return step;
> >   }
> > -void qemu_announce_timer_del(AnnounceTimer *timer)
> > +/*
> > + * If 'free_named' is true, then remove the timer from the list
> > + * and free the timer itself.
> > + */
> > +void qemu_announce_timer_del(AnnounceTimer *timer, bool free_named)
> >   {
> > +    bool free_timer = false;
> >       if (timer->tm) {
> >           timer_del(timer->tm);
> >           timer_free(timer->tm);
> > @@ -40,6 +47,18 @@ void qemu_announce_timer_del(AnnounceTimer *timer)
> >       }
> >       qapi_free_strList(timer->params.interfaces);
> >       timer->params.interfaces = NULL;
> > +    if (free_named && timer->params.has_id) {
> > +        free_timer = timer ==
> > +                     g_datalist_get_data(&named_timers, timer->params.id);
> 
> 
> Any chance that the timer get from datalist is different from the one that
> caller passed to us?

No, I've replaced this with:
+    if (free_named && timer->params.has_id) {
+        AnnounceTimer *listTimer;
+        /*
+         * Sanity check: There should only be one timer on the list with
+         * the id.
+         */
+        list_timer = g_datalist_get_data(&named_timers, timer->params.id);
+        assert(timer == list_timer);
+        free_timer = true;
+        g_datalist_remove_data(&named_timers, timer->params.id);
+    }


> Thanks
> 
> 
> > +        g_datalist_remove_data(&named_timers, timer->params.id);
> > +    }
> > +    trace_qemu_announce_timer_del(free_named, free_timer, timer->params.id);
> > +    g_free(timer->params.id);
> > +    timer->params.id = NULL;
> > +
> > +    if (free_timer) {
> > +        g_free(timer);
> > +    }
> >   }
> >   /*
> > @@ -56,7 +75,7 @@ void qemu_announce_timer_reset(AnnounceTimer *timer,
> >        * We're under the BQL, so the current timer can't
> >        * be firing, so we should be able to delete it.
> >        */
> > -    qemu_announce_timer_del(timer);
> > +    qemu_announce_timer_del(timer, false);
> >       QAPI_CLONE_MEMBERS(AnnounceParameters, &timer->params, params);
> >       timer->round = params->rounds;
> > @@ -120,7 +139,8 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
> >           skip = false;
> >       }
> > -    trace_qemu_announce_self_iter(nic->ncs->name,
> > +    trace_qemu_announce_self_iter(timer->params.has_id ? timer->params.id : "_",
> > +                                  nic->ncs->name,
> >                                     qemu_ether_ntoa(&nic->conf->macaddr), skip);
> >       if (!skip) {
> > @@ -143,7 +163,7 @@ static void qemu_announce_self_once(void *opaque)
> >       if (--timer->round) {
> >           qemu_announce_timer_step(timer);
> >       } else {
> > -        qemu_announce_timer_del(timer);
> > +        qemu_announce_timer_del(timer, true);
> >       }
> >   }
> > @@ -154,12 +174,24 @@ void qemu_announce_self(AnnounceTimer *timer, AnnounceParameters *params)
> >       if (params->rounds) {
> >           qemu_announce_self_once(timer);
> >       } else {
> > -        qemu_announce_timer_del(timer);
> > +        qemu_announce_timer_del(timer, true);
> >       }
> >   }
> >   void qmp_announce_self(AnnounceParameters *params, Error **errp)
> >   {
> > -    static AnnounceTimer announce_timer;
> > -    qemu_announce_self(&announce_timer, params);
> > +    AnnounceTimer *named_timer;
> > +    if (!params->has_id) {
> > +        params->id = g_strdup("");
> > +        params->has_id = true;
> > +    }
> > +
> > +    named_timer = g_datalist_get_data(&named_timers, params->id);
> > +
> > +    if (!named_timer) {
> > +        named_timer = g_new0(AnnounceTimer, 1);
> > +        g_datalist_set_data(&named_timers, params->id, named_timer);
> > +    }
> > +
> > +    qemu_announce_self(named_timer, params);
> >   }
> > diff --git a/net/trace-events b/net/trace-events
> > index 875ef2a0f3..ac57056497 100644
> > --- a/net/trace-events
> > +++ b/net/trace-events
> > @@ -1,7 +1,8 @@
> >   # See docs/devel/tracing.txt for syntax documentation.
> >   # announce.c
> > -qemu_announce_self_iter(const char *name, const char *mac, int skip) "%s:%s skip: %d"
> > +qemu_announce_self_iter(const char *id, const char *name, const char *mac, int skip) "%s:%s:%s skip: %d"
> > +qemu_announce_timer_del(bool free_named, bool free_timer, char *id) "free named: %d free timer: %d id: %s"
> >   # vhost-user.c
> >   vhost_user_event(const char *chr, int event) "chr: %s got event: %d"
> > diff --git a/qapi/net.json b/qapi/net.json
> > index 6f2cd4f530..728990f4fb 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -702,6 +702,10 @@
> >   # @interfaces: An optional list of interface names, which restricts the
> >   #        announcement to the listed interfaces. (Since 4.1)
> >   #
> > +# @id: A name to be used to identify an instance of announce-timers
> > +#        and to allow it to modified later.  Not for use as
> > +#        part of the migration parameters. (Since 4.1)
> > +#
> >   # Since: 4.0
> >   ##
> > @@ -710,7 +714,8 @@
> >               'max': 'int',
> >               'rounds': 'int',
> >               'step': 'int',
> > -            '*interfaces': ['str'] } }
> > +            '*interfaces': ['str'],
> > +            '*id' : 'str' } }
> >   ##
> >   # @announce-self:
> > @@ -725,7 +730,7 @@
> >   # -> { "execute": "announce-self",
> >   #      "arguments": {
> >   #          "initial": 50, "max": 550, "rounds": 10, "step": 50,
> > -#          "interfaces": ["vn2", "vn3"] } }
> > +#          "interfaces": ["vn2", "vn3"], "id": "bob" } }
> >   # <- { "return": {} }
> >   #
> >   # Since: 4.0
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 5/5] net/announce: Expand test for stopping self announce

[Dr. David Alan Gilbert]

* Jason Wang (jasowang@redhat.com) wrote:
> 
> On 2019/6/13 下午5:59, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Expand self-announce test to check we can stop an announce timer.
> > We set it up to send 300 packets, but after we receive
> > the first one we tell it to stop.
> > 
> > We error if:
> >     a) We receive more than 30 of the packets
> >     b) We're still receiving packets after a lot longer than the
> >        30 seconds should have arrived
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >   tests/virtio-net-test.c | 57 ++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 54 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
> > index 663cf7ea7e..3b49b431dc 100644
> > --- a/tests/virtio-net-test.c
> > +++ b/tests/virtio-net-test.c
> > @@ -184,21 +184,72 @@ static void announce_self(void *obj, void *data, QGuestAllocator *t_alloc)
> >       QDict *rsp;
> >       int ret;
> >       uint16_t *proto = (uint16_t *)&buffer[12];
> > +    size_t total_received = 0;
> > +    uint64_t start, now, last_rxt, deadline;
> > +    /* Send a set of packets over a few second period */
> >       rsp = qmp("{ 'execute' : 'announce-self', "
> >                     " 'arguments': {"
> > -                      " 'initial': 50, 'max': 550,"
> > -                      " 'rounds': 10, 'step': 50 } }");
> > +                      " 'initial': 20, 'max': 100,"
> > +                      " 'rounds': 300, 'step': 10, 'id': 'bob' } }");
> >       assert(!qdict_haskey(rsp, "error"));
> >       qobject_unref(rsp);
> > -    /* Catch the packet and make sure it's a RARP */
> > +    /* Catch the first packet and make sure it's a RARP */
> >       ret = qemu_recv(sv[0], &len, sizeof(len), 0);
> >       g_assert_cmpint(ret, ==,  sizeof(len));
> >       len = ntohl(len);
> >       ret = qemu_recv(sv[0], buffer, len, 0);
> >       g_assert_cmpint(*proto, ==, htons(ETH_P_RARP));
> > +
> > +    /*
> > +     * Stop the announcment by settings rounds to 0 on the
> > +     * existing timer.
> > +     */
> > +    rsp = qmp("{ 'execute' : 'announce-self', "
> > +                  " 'arguments': {"
> > +                      " 'initial': 20, 'max': 100,"
> > +                      " 'rounds': 0, 'step': 10, 'id': 'bob' } }");
> > +    assert(!qdict_haskey(rsp, "error"));
> > +    qobject_unref(rsp);
> > +
> > +    /* Now make sure the packets stop */
> > +
> > +    /* Times are in us */
> > +    start = g_get_monotonic_time();
> > +    /* 30 packets, max gap 100ms, * 2 for wiggle */
> > +    deadline = start + 1000 * (100 * 30 * 2);
> > +    last_rxt = start;
> > +
> > +    do {
> 
> 
> while (ture) looks better here.

OK, changed.

> 
> > +        int saved_err;
> > +        ret = qemu_recv(sv[0], buffer, 60, MSG_DONTWAIT);
> > +        saved_err = errno;
> > +        now = g_get_monotonic_time();
> > +        g_assert_cmpint(now, <, deadline);
> 
> The maximum gap allowed is 1000 * 100 * 4, and we allow at most 30 packets
> that's 30 * 1000 * 100 * 4 which is 1200000.
> 
> But the deadline is 1000 * 100 * 30 * 2 which is 6000000.
> 
> Does this mean deadline is conflict with the assumption above?

I've changed deadline to match (i.e. * 4) - but it's only a worst-case
that we shouldn't hit anyway; I'm expecting it to actually stop after
1 or 2 packets worst case, the *4 in the maximum gap is there just to
deal with a busy system that takes a bit longer.


Dave

> Thanks
> 
> 
> > +
> > +        if (ret >= 0) {
> > +            if (ret) {
> > +                last_rxt = now;
> > +            }
> > +            total_received += ret;
> > +
> > +            /* Check it's not spewing loads */
> > +            g_assert_cmpint(total_received, <, 60 * 30 * 2);
> > +        } else {
> > +            g_assert_cmpint(saved_err, ==, EAGAIN);
> > +
> > +            /* 400ms, i.e. 4 worst case gaps */
> > +            if ((now - last_rxt) > (1000 * 100 * 4)) {
> > +                /* Nothings arrived for a while - must have stopped */
> > +                break;
> > +            };
> > +
> > +            /* 100ms */
> > +            g_usleep(1000 * 100);
> > +        }
> > +    } while (true);
> >   }
> >   static void virtio_net_test_cleanup(void *sockets)
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 0/5] network announce; interface selection & IDs

[Dr. David Alan Gilbert]

* Jason Wang (jasowang@redhat.com) wrote:
> 
> On 2019/6/13 下午5:59, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Laine asked for some extra features on the network announce support;
> 
> 
> It's better to explain why this feature is needed.

Yes, I'll reword.

> Is this because libvirt
> can change the host network topology on the fly?

It's because something can change the network topology on the fly - not
necessarily just libvirt.  Where as previously we were using the
announce mechanism for mainly migration reasons, now we also want
to use it to announce topology changes; those include potentially things
that libvirt gets told by a higher management layer - such as the
failure or one network path.

Dave

> 
> Thanks
> 
> 
> > 
> > The first allows the announce timer to announce on a subset of the
> > interfaces.
> > 
> > The second allows there to be multiple timers, each with their own
> > parameters (including the interface list).
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > v4
> >    Minor typo fixes
> >    Expanded the test to check we can stop a running announce
> > 
> > Dr. David Alan Gilbert (5):
> >    net/announce: Allow optional list of interfaces
> >    net/announce: Add HMP optional interface list
> >    net/announce: Add optional ID
> >    net/announce: Add HMP optional ID
> >    net/announce: Expand test for stopping self announce
> > 
> >   hmp-commands.hx         |  7 +++-
> >   hmp.c                   | 41 +++++++++++++++++++-
> >   hw/net/virtio-net.c     |  4 +-
> >   include/net/announce.h  |  8 +++-
> >   net/announce.c          | 83 ++++++++++++++++++++++++++++++++++-------
> >   net/trace-events        |  3 +-
> >   qapi/net.json           | 16 ++++++--
> >   tests/virtio-net-test.c | 57 ++++++++++++++++++++++++++--
> >   8 files changed, 192 insertions(+), 27 deletions(-)
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK