[Qemu-devel] [PATCH v2 0/2] mac programming over macvtap

[Qemu-devel] [PATCH v2 0/2] mac programming over macvtap

[Amos Kong]

This patchset introduces a QMP event and a monitor command.
The event is used to notify management when mac-table
configuration is changed by guest. Management can use the
new monitor command to query mac-table information, and
sync the changes to macvtap devices.

There maybe exist an uncontrollable delay, guests normally expect
rx-mode updates immediately, but it's another separate issue, we
can investigate it after Libvirt work is done.

Patches are based on Michael's patchset [1], you can also find patches
in my github [2].

[1] [PATCH v3 00/11] qapi: add support for lists of native types
[2] https://github.com/kongove/qemu/tree/01-query-mactable

v2: add argument to filter mac-table info of single nic (Stefan)
    update the document
    add event notification

Amos Kong (2):
  net: introduce MAC_TABLE_CHANGED event
  net: introduce command to query mac-table information

 QMP/qmp-events.txt        | 14 +++++++++
 hmp-commands.hx           |  2 ++
 hmp.c                     | 71 ++++++++++++++++++++++++++++++++++++++++++++
 hmp.h                     |  1 +
 hw/net/virtio-net.c       | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 include/monitor/monitor.h |  1 +
 include/net/net.h         |  2 ++
 monitor.c                 |  9 ++++++
 net/net.c                 | 38 ++++++++++++++++++++++++
 qapi-schema.json          | 57 +++++++++++++++++++++++++++++++++++
 qmp-commands.hx           | 53 +++++++++++++++++++++++++++++++++
 11 files changed, 323 insertions(+)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

Introduce this new QMP event to notify management after guest changes
mac-table configuration.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 QMP/qmp-events.txt        | 14 ++++++++++++++
 hw/net/virtio-net.c       | 12 ++++++++++++
 include/monitor/monitor.h |  1 +
 monitor.c                 |  1 +
 4 files changed, 28 insertions(+)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 92fe5fb..24d62df 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -154,6 +154,20 @@ Data:
             "path": "/machine/peripheral/virtio-net-pci-0" },
   "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 
+MAC_TABLE_CHANGED
+-----------------
+
+Emitted mac-table configuration is changed by the guest.
+
+Data:
+
+- "name": net client name (json-string)
+
+{ "event": "MAC_TABLE_CHANGED",
+  "data": { "name": "vnet0" },
+  "timestamp": { "seconds": 1368697518, "microseconds": 326866 }}
+}
+
 DEVICE_TRAY_MOVED
 -----------------
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index bed0822..a9b8f53 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -21,6 +21,8 @@
 #include "hw/virtio/virtio-net.h"
 #include "net/vhost_net.h"
 #include "hw/virtio/virtio-bus.h"
+#include "qapi/qmp/qjson.h"
+#include "monitor/monitor.h"
 
 #define VIRTIO_NET_VM_VERSION    11
 
@@ -395,6 +397,7 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
 {
     uint8_t on;
     size_t s;
+    QObject *event_data;
 
     s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
     if (s != sizeof(on)) {
@@ -417,6 +420,10 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
     }
 
+    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
+    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
+    qobject_decref(event_data);
+
     return VIRTIO_NET_OK;
 }
 
@@ -425,6 +432,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 {
     struct virtio_net_ctrl_mac mac_data;
     size_t s;
+    QObject *event_data;
 
     if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
         if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
@@ -497,6 +505,10 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
         n->mac_table.multi_overflow = 1;
     }
 
+    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
+    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
+    qobject_decref(event_data);
+
     return VIRTIO_NET_OK;
 }
 
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 1a6cfcf..e88c70f 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -40,6 +40,7 @@ typedef enum MonitorEvent {
     QEVENT_BLOCK_JOB_ERROR,
     QEVENT_BLOCK_JOB_READY,
     QEVENT_DEVICE_DELETED,
+    QEVENT_MAC_TABLE_CHANGED,
     QEVENT_DEVICE_TRAY_MOVED,
     QEVENT_SUSPEND,
     QEVENT_SUSPEND_DISK,
diff --git a/monitor.c b/monitor.c
index 62aaebe..9e51545 100644
--- a/monitor.c
+++ b/monitor.c
@@ -490,6 +490,7 @@ static const char *monitor_event_names[] = {
     [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
     [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
     [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED",
+    [QEVENT_MAC_TABLE_CHANGED] = "MAC_TABLE_CHANGED",
     [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
     [QEVENT_SUSPEND] = "SUSPEND",
     [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK",
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Amos Kong]

We want to implement mac programming over macvtap through Libvirt.
The previous patch adds QMP event to notify management of mac-table
change. This patch adds a monitor command to query rx mode information
of mac-tables.

(qemu) info mac-table vnet0
vnet0:
 \ promisc: on
 \ allmulti: off
 \ alluni: off
 \ nomulti: off
 \ nouni: off
 \ nobcast: off
 \ multi_overflow: off
 \ uni_overflow: off
 \ multicast:
    01:00:5e:00:00:01
    33:33:00:00:00:01
    33:33:ff:12:34:56

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hmp-commands.hx     |  2 ++
 hmp.c               | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp.h               |  1 +
 hw/net/virtio-net.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 include/net/net.h   |  2 ++
 monitor.c           |  8 ++++++
 net/net.c           | 38 ++++++++++++++++++++++++++++
 qapi-schema.json    | 57 ++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx     | 53 +++++++++++++++++++++++++++++++++++++++
 9 files changed, 295 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9cea415..e5c1b14 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1639,6 +1639,8 @@ show qdev device model list
 show roms
 @item info tpm
 show the TPM device
+@item info mac-table [net client name]
+show the mac-table information for all nics (or for the given nic)
 @end table
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index 4fb76ec..3e19df0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -653,6 +653,77 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
     qapi_free_TPMInfoList(info_list);
 }
 
+void hmp_info_mac_table(Monitor *mon, const QDict *qdict)
+{
+    MacTableInfoList *table_list, *entry;
+    strList *str_entry;
+    bool has_name = qdict_haskey(qdict, "name");
+    const char *name = NULL;
+
+    if (has_name) {
+        name = qdict_get_str(qdict, "name");
+    }
+
+    table_list = qmp_query_mac_table(has_name, name, NULL);
+    entry = table_list;
+
+    while (entry) {
+        monitor_printf(mon, "%s:\n", entry->value->name);
+        if (entry->value->has_promisc) {
+            monitor_printf(mon, " \\ promisc: %s\n",
+                           entry->value->promisc ? "on" : "off");
+        }
+        if (entry->value->has_allmulti) {
+            monitor_printf(mon, " \\ allmulti: %s\n",
+                           entry->value->allmulti ? "on" : "off");
+        }
+        if (entry->value->has_alluni) {
+            monitor_printf(mon, " \\ alluni: %s\n",
+                           entry->value->alluni ? "on" : "off");
+        }
+        if (entry->value->has_nomulti) {
+            monitor_printf(mon, " \\ nomulti: %s\n",
+                           entry->value->nomulti ? "on" : "off");
+        }
+        if (entry->value->has_nouni) {
+            monitor_printf(mon, " \\ nouni: %s\n",
+                           entry->value->nouni ? "on" : "off");
+        }
+        if (entry->value->has_nobcast) {
+            monitor_printf(mon, " \\ nobcast: %s\n",
+                           entry->value->nobcast ? "on" : "off");
+        }
+        if (entry->value->has_multi_overflow) {
+            monitor_printf(mon, " \\ multi_overflow: %s\n",
+                           entry->value->multi_overflow ? "on" : "off");
+        }
+        if (entry->value->has_uni_overflow) {
+            monitor_printf(mon, " \\ uni_overflow: %s\n",
+                           entry->value->uni_overflow ? "on" : "off");
+        }
+
+        if (entry->value->has_unicast) {
+            str_entry = entry->value->unicast;
+            monitor_printf(mon, " \\ unicast:\n");
+            while (str_entry) {
+                monitor_printf(mon, "    %s\n", str_entry->value);
+                str_entry = str_entry->next;
+            }
+        }
+        if (entry->value->has_multicast) {
+            str_entry = entry->value->multicast;
+            monitor_printf(mon, " \\ multicast:\n");
+            while (str_entry) {
+                monitor_printf(mon, "    %s\n", str_entry->value);
+                str_entry = str_entry->next;
+            }
+        }
+
+        entry = entry->next;
+    }
+    qapi_free_MacTableInfoList(table_list);
+}
+
 void hmp_quit(Monitor *mon, const QDict *qdict)
 {
     monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 95fe76e..278c722 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
 void hmp_info_pci(Monitor *mon, const QDict *qdict);
 void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
 void hmp_info_tpm(Monitor *mon, const QDict *qdict);
+void hmp_info_mac_table(Monitor *mon, const QDict *qdict);
 void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a9b8f53..e4b2358 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -194,6 +194,68 @@ static void virtio_net_set_link_status(NetClientState *nc)
     virtio_net_set_status(vdev, vdev->status);
 }
 
+static MacTableInfo *virtio_net_query_mactable(NetClientState *nc)
+{
+    VirtIONet *n = qemu_get_nic_opaque(nc);
+    MacTableInfo *info;
+    strList *str_list = NULL;
+    strList *entry;
+    int i;
+
+    info = g_malloc0(sizeof(*info));
+    info->name = g_strdup(nc->name);
+
+    info->promisc = n->promisc;
+    info->has_promisc = true;
+    info->allmulti = n->allmulti;
+    info->has_allmulti = true;
+    info->alluni = n->alluni;
+    info->has_alluni = true;
+    info->nomulti = n->nomulti;
+    info->has_nomulti = true;
+    info->nouni = n->nouni;
+    info->has_nouni = true;
+    info->nobcast = n->nobcast;
+    info->has_nobcast = true;
+    info->multi_overflow = n->mac_table.multi_overflow;
+    info->has_multi_overflow = true;
+    info->uni_overflow = n->mac_table.uni_overflow;
+    info->has_uni_overflow = true;
+
+    for (i = 0; i < n->mac_table.first_multi; i++) {
+        info->has_unicast = true;
+        entry = g_malloc0(sizeof(*entry));
+        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
+                                       n->mac_table.macs[i * ETH_ALEN],
+                                       n->mac_table.macs[i * ETH_ALEN + 1],
+                                       n->mac_table.macs[i * ETH_ALEN + 2],
+                                       n->mac_table.macs[i * ETH_ALEN + 3],
+                                       n->mac_table.macs[i * ETH_ALEN + 4],
+                                       n->mac_table.macs[i * ETH_ALEN + 5]);
+        entry->next = str_list;
+        str_list = entry;
+    }
+    info->unicast = str_list;
+
+    str_list = NULL;
+    for (i = n->mac_table.first_multi; i < n->mac_table.in_use; i++) {
+        info->has_multicast = true;
+        entry = g_malloc0(sizeof(*entry));
+        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
+                                       n->mac_table.macs[i * ETH_ALEN],
+                                       n->mac_table.macs[i * ETH_ALEN + 1],
+                                       n->mac_table.macs[i * ETH_ALEN + 2],
+                                       n->mac_table.macs[i * ETH_ALEN + 3],
+                                       n->mac_table.macs[i * ETH_ALEN + 4],
+                                       n->mac_table.macs[i * ETH_ALEN + 5]);
+        entry->next = str_list;
+        str_list = entry;
+    }
+    info->multicast = str_list;
+
+    return info;
+}
+
 static void virtio_net_reset(VirtIODevice *vdev)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
@@ -1255,6 +1317,7 @@ static NetClientInfo net_virtio_info = {
     .receive = virtio_net_receive,
         .cleanup = virtio_net_cleanup,
     .link_status_changed = virtio_net_set_link_status,
+    .query_mac_table = virtio_net_query_mactable,
 };
 
 static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
diff --git a/include/net/net.h b/include/net/net.h
index 43d85a1..c3ca4ea 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -49,6 +49,7 @@ typedef ssize_t (NetReceiveIOV)(NetClientState *, const struct iovec *, int);
 typedef void (NetCleanup) (NetClientState *);
 typedef void (LinkStatusChanged)(NetClientState *);
 typedef void (NetClientDestructor)(NetClientState *);
+typedef MacTableInfo *(QueryMacTable)(NetClientState *);
 
 typedef struct NetClientInfo {
     NetClientOptionsKind type;
@@ -59,6 +60,7 @@ typedef struct NetClientInfo {
     NetCanReceive *can_receive;
     NetCleanup *cleanup;
     LinkStatusChanged *link_status_changed;
+    QueryMacTable *query_mac_table;
     NetPoll *poll;
 } NetClientInfo;
 
diff --git a/monitor.c b/monitor.c
index 9e51545..a01cdbd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2765,6 +2765,14 @@ static mon_cmd_t info_cmds[] = {
         .mhandler.cmd = hmp_info_tpm,
     },
     {
+        .name       = "mac-table",
+        .args_type  = "name:s?",
+        .params     = "[net client name]",
+        .help       = "show the mac-table information for all nics (or"
+                      " for the given nic)",
+        .mhandler.cmd = hmp_info_mac_table,
+    },
+    {
         .name       = NULL,
     },
 };
diff --git a/net/net.c b/net/net.c
index 43a74e4..9ff3006 100644
--- a/net/net.c
+++ b/net/net.c
@@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
                    nc->info_str);
 }
 
+MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
+                                      Error **errp)
+{
+    NetClientState *nc;
+    MacTableInfoList *table_list = NULL, *last_entry = NULL;
+
+    QTAILQ_FOREACH(nc, &net_clients, next) {
+        MacTableInfoList *entry;
+        MacTableInfo *info;
+
+        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
+            continue;
+        }
+        if (has_name && strcmp(nc->name, name) != 0) {
+            continue;
+        }
+
+        if (nc->info->query_mac_table) {
+            info = nc->info->query_mac_table(nc);
+            entry = g_malloc0(sizeof(*entry));
+            entry->value = info;
+
+            if (!table_list) {
+                table_list = entry;
+            } else {
+                last_entry->next = entry;
+            }
+            last_entry = entry;
+        }
+    }
+
+    if (table_list == NULL) {
+        error_setg(errp, "invalid net client name: %s", name);
+    }
+
+    return table_list;
+}
+
 void do_info_network(Monitor *mon, const QDict *qdict)
 {
     NetClientState *nc, *peer;
diff --git a/qapi-schema.json b/qapi-schema.json
index 199744a..866650c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3619,3 +3619,60 @@
             '*cpuid-input-ecx': 'int',
             'cpuid-register': 'X86CPURegister32',
             'features': 'int' } }
+
+# @MacTableInfo:
+#
+# Rx-mode information of mac-table for a net client.
+#
+# @name: the net client name
+#
+# @promisc: #optional promiscuous mode (default: false)
+#
+# @allmulti: #optional all multicast mode (default: false)
+#
+# @alluni: #optional all unicast mode (default: false)
+#
+# @nomulti: #optional no multicast mode (default: false)
+#
+# @nouni: #optional no unicast mode (default: false)
+#
+# @nobcast: #optional no broadcast mode (default: false)
+#
+# @multi_overflow: #optional multicast overflow mode (default: false)
+#
+# @uni_overflow: #optional unicast overflow mode (default: false)
+#
+# @unicast: #optional a list of unicast macaddr string
+#
+# @multicast: #optional a list of multicast macaddr string
+#
+# Since 1.6
+##
+{ 'type': 'MacTableInfo',
+  'data': {
+    'name':            'str',
+    '*promisc':        'bool',
+    '*allmulti':       'bool',
+    '*alluni':         'bool',
+    '*nomulti':        'bool',
+    '*nouni':          'bool',
+    '*nobcast':        'bool',
+    '*multi-overflow': 'bool',
+    '*uni-overflow':   'bool',
+    '*unicast':        ['str'],
+    '*multicast':      ['str'] }}
+
+##
+# @query-mac-table:
+#
+# Return mac-table information for all nics (or for the given nic).
+#
+# @name: #optional net client name
+#
+# Returns: list of @MacTableInfo for all nics (or for the given nic).
+#          Returns an error if the given @name doesn't exist.
+#
+# Since: 1.6
+##
+{ 'command': 'query-mac-table', 'data': { '*name': 'str' },
+  'returns': ['MacTableInfo'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ffd130e..66826ab 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2932,3 +2932,56 @@ Example:
 <- { "return": {} }
 
 EQMP
+    {
+        .name       = "query-mac-table",
+        .args_type  = "name:s?",
+        .mhandler.cmd_new = qmp_marshal_input_query_mac_table,
+    },
+
+SQMP
+query-mac-table
+---------------
+
+Show mac-table information.
+
+Returns a json-array of mac-table information for all nics (or for the
+given nic), returning an error if the given nic doesn't exist.
+
+Each array entry contains the following:
+
+- "name": net client name (jaso-string)
+- "promisc": promiscuous mode (json-bool, optional)
+- "allmulti": all multicast mode (json-bool, optional)
+- "alluni": all unicast mode (json-bool, optional)
+- "nomulti":no multicast mode (json-bool, optional)
+- "nouni": no unicast mode (json-bool, optional)
+- "nobcast": no broadcast mode (json-bool, optional)
+- "multi-overflow": multicast overflow mode (json-bool, optional)
+- "uni-overflow": unicast overflow mode (json-bool, optional)
+- "unicast": a jason-array of unicast macaddr string (optional)
+- "multicast": a jason-array of multicast macaddr string (optional)
+
+Example:
+
+-> { "execute": "query-mac-table", "arguments": { "name": "vnet0" }}
+<- { "return": [
+        {
+            "multi-overflow": false,
+            "name": "vnet0",
+            "uni-overflow": false,
+            "nobcast": false,
+            "promisc": true,
+            "multicast": [
+                "01:00:5e:00:00:01",
+                "33:33:00:00:00:01",
+                "33:33:ff:12:34:56"
+            ],
+            "nouni": false,
+            "nomulti": false,
+            "allmulti": false,
+            "alluni": false
+        }
+      ]
+   }
+
+EQMP
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> Introduce this new QMP event to notify management after guest changes
> mac-table configuration.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  QMP/qmp-events.txt        | 14 ++++++++++++++
>  hw/net/virtio-net.c       | 12 ++++++++++++
>  include/monitor/monitor.h |  1 +
>  monitor.c                 |  1 +
>  4 files changed, 28 insertions(+)
> 
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index 92fe5fb..24d62df 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -154,6 +154,20 @@ Data:
>              "path": "/machine/peripheral/virtio-net-pci-0" },
>    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  
> +MAC_TABLE_CHANGED
> +-----------------
> +
> +Emitted mac-table configuration is changed by the guest.
> +
> +Data:
> +
> +- "name": net client name (json-string)
> +
> +{ "event": "MAC_TABLE_CHANGED",
> +  "data": { "name": "vnet0" },
> +  "timestamp": { "seconds": 1368697518, "microseconds": 326866 }}
> +}
> +

It seems clear that if management wants to know about
RX filter changes, it also cares about RX mode changes.
So what's the plan for RX mode changes?
Want to add more events or extend this one?
I'd like to see how it all works together.

>  DEVICE_TRAY_MOVED
>  -----------------
>  
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index bed0822..a9b8f53 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -21,6 +21,8 @@
>  #include "hw/virtio/virtio-net.h"
>  #include "net/vhost_net.h"
>  #include "hw/virtio/virtio-bus.h"
> +#include "qapi/qmp/qjson.h"
> +#include "monitor/monitor.h"
>  
>  #define VIRTIO_NET_VM_VERSION    11
>  
> @@ -395,6 +397,7 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
>  {
>      uint8_t on;
>      size_t s;
> +    QObject *event_data;
>  
>      s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
>      if (s != sizeof(on)) {
> @@ -417,6 +420,10 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
>          return VIRTIO_NET_ERR;
>      }
>  
> +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> +    qobject_decref(event_data);
> +
>      return VIRTIO_NET_OK;
>  }
>  
> @@ -425,6 +432,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>  {
>      struct virtio_net_ctrl_mac mac_data;
>      size_t s;
> +    QObject *event_data;
>  
>      if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
>          if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
> @@ -497,6 +505,10 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>          n->mac_table.multi_overflow = 1;
>      }
>  
> +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> +    qobject_decref(event_data);
> +
>      return VIRTIO_NET_OK;
>  }
>  
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 1a6cfcf..e88c70f 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -40,6 +40,7 @@ typedef enum MonitorEvent {
>      QEVENT_BLOCK_JOB_ERROR,
>      QEVENT_BLOCK_JOB_READY,
>      QEVENT_DEVICE_DELETED,
> +    QEVENT_MAC_TABLE_CHANGED,
>      QEVENT_DEVICE_TRAY_MOVED,
>      QEVENT_SUSPEND,
>      QEVENT_SUSPEND_DISK,
> diff --git a/monitor.c b/monitor.c
> index 62aaebe..9e51545 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -490,6 +490,7 @@ static const char *monitor_event_names[] = {
>      [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
>      [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
>      [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED",
> +    [QEVENT_MAC_TABLE_CHANGED] = "MAC_TABLE_CHANGED",
>      [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
>      [QEVENT_SUSPEND] = "SUSPEND",
>      [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK",
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> Introduce this new QMP event to notify management after guest changes
> mac-table configuration.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  QMP/qmp-events.txt        | 14 ++++++++++++++
>  hw/net/virtio-net.c       | 12 ++++++++++++
>  include/monitor/monitor.h |  1 +
>  monitor.c                 |  1 +
>  4 files changed, 28 insertions(+)
> 
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index 92fe5fb..24d62df 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -154,6 +154,20 @@ Data:
>              "path": "/machine/peripheral/virtio-net-pci-0" },
>    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  
> +MAC_TABLE_CHANGED
> +-----------------
> +
> +Emitted mac-table configuration is changed by the guest.
> +
> +Data:
> +
> +- "name": net client name (json-string)
> +
> +{ "event": "MAC_TABLE_CHANGED",
> +  "data": { "name": "vnet0" },
> +  "timestamp": { "seconds": 1368697518, "microseconds": 326866 }}
> +}
> +
>  DEVICE_TRAY_MOVED
>  -----------------
>  
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index bed0822..a9b8f53 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -21,6 +21,8 @@
>  #include "hw/virtio/virtio-net.h"
>  #include "net/vhost_net.h"
>  #include "hw/virtio/virtio-bus.h"
> +#include "qapi/qmp/qjson.h"
> +#include "monitor/monitor.h"
>  
>  #define VIRTIO_NET_VM_VERSION    11
>  
> @@ -395,6 +397,7 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
>  {
>      uint8_t on;
>      size_t s;
> +    QObject *event_data;
>  
>      s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
>      if (s != sizeof(on)) {
> @@ -417,6 +420,10 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
>          return VIRTIO_NET_ERR;
>      }
>  
> +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> +    qobject_decref(event_data);
> +
>      return VIRTIO_NET_OK;
>  }
>  

Sorry, pls ignore my previous mail, I see you actually
emit this on rx mode change as well.

I find the name misleading or at least it mislead me :)
RX_FILTER_CHANGED?

> @@ -425,6 +432,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>  {
>      struct virtio_net_ctrl_mac mac_data;
>      size_t s;
> +    QObject *event_data;
>  
>      if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
>          if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
> @@ -497,6 +505,10 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>          n->mac_table.multi_overflow = 1;
>      }
>  
> +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> +    qobject_decref(event_data);
> +
>      return VIRTIO_NET_OK;
>  }
> 

This makes it easy for guest to flood management with
spurious events.
How about we set a flag after this, and avoid sending any more
events until management queries the filter status?
 
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 1a6cfcf..e88c70f 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -40,6 +40,7 @@ typedef enum MonitorEvent {
>      QEVENT_BLOCK_JOB_ERROR,
>      QEVENT_BLOCK_JOB_READY,
>      QEVENT_DEVICE_DELETED,
> +    QEVENT_MAC_TABLE_CHANGED,
>      QEVENT_DEVICE_TRAY_MOVED,
>      QEVENT_SUSPEND,
>      QEVENT_SUSPEND_DISK,
> diff --git a/monitor.c b/monitor.c
> index 62aaebe..9e51545 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -490,6 +490,7 @@ static const char *monitor_event_names[] = {
>      [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
>      [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
>      [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED",
> +    [QEVENT_MAC_TABLE_CHANGED] = "MAC_TABLE_CHANGED",
>      [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
>      [QEVENT_SUSPEND] = "SUSPEND",
>      [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK",
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Michael S. Tsirkin]

On Thu, May 16, 2013 at 07:07:25PM +0800, Amos Kong wrote:
> We want to implement mac programming over macvtap through Libvirt.
> The previous patch adds QMP event to notify management of mac-table
> change. This patch adds a monitor command to query rx mode information
> of mac-tables.
> 
> (qemu) info mac-table vnet0
> vnet0:
>  \ promisc: on
>  \ allmulti: off
>  \ alluni: off
>  \ nomulti: off
>  \ nouni: off
>  \ nobcast: off
>  \ multi_overflow: off
>  \ uni_overflow: off
>  \ multicast:
>     01:00:5e:00:00:01
>     33:33:00:00:00:01
>     33:33:ff:12:34:56
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

It's an interesting example.
There are no unicast addresses at all?

I'm guessing you are missing the main MAC.

> ---
>  hmp-commands.hx     |  2 ++
>  hmp.c               | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h               |  1 +
>  hw/net/virtio-net.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/net/net.h   |  2 ++
>  monitor.c           |  8 ++++++
>  net/net.c           | 38 ++++++++++++++++++++++++++++
>  qapi-schema.json    | 57 ++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx     | 53 +++++++++++++++++++++++++++++++++++++++
>  9 files changed, 295 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9cea415..e5c1b14 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1639,6 +1639,8 @@ show qdev device model list
>  show roms
>  @item info tpm
>  show the TPM device
> +@item info mac-table [net client name]
> +show the mac-table information for all nics (or for the given nic)
>  @end table
>  ETEXI
>  
> diff --git a/hmp.c b/hmp.c
> index 4fb76ec..3e19df0 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -653,6 +653,77 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
>      qapi_free_TPMInfoList(info_list);
>  }
>  
> +void hmp_info_mac_table(Monitor *mon, const QDict *qdict)
> +{
> +    MacTableInfoList *table_list, *entry;
> +    strList *str_entry;
> +    bool has_name = qdict_haskey(qdict, "name");
> +    const char *name = NULL;
> +
> +    if (has_name) {
> +        name = qdict_get_str(qdict, "name");
> +    }
> +
> +    table_list = qmp_query_mac_table(has_name, name, NULL);
> +    entry = table_list;
> +
> +    while (entry) {
> +        monitor_printf(mon, "%s:\n", entry->value->name);
> +        if (entry->value->has_promisc) {
> +            monitor_printf(mon, " \\ promisc: %s\n",
> +                           entry->value->promisc ? "on" : "off");
> +        }
> +        if (entry->value->has_allmulti) {
> +            monitor_printf(mon, " \\ allmulti: %s\n",
> +                           entry->value->allmulti ? "on" : "off");
> +        }
> +        if (entry->value->has_alluni) {
> +            monitor_printf(mon, " \\ alluni: %s\n",
> +                           entry->value->alluni ? "on" : "off");
> +        }
> +        if (entry->value->has_nomulti) {
> +            monitor_printf(mon, " \\ nomulti: %s\n",
> +                           entry->value->nomulti ? "on" : "off");
> +        }
> +        if (entry->value->has_nouni) {
> +            monitor_printf(mon, " \\ nouni: %s\n",
> +                           entry->value->nouni ? "on" : "off");
> +        }
> +        if (entry->value->has_nobcast) {
> +            monitor_printf(mon, " \\ nobcast: %s\n",
> +                           entry->value->nobcast ? "on" : "off");
> +        }
> +        if (entry->value->has_multi_overflow) {
> +            monitor_printf(mon, " \\ multi_overflow: %s\n",
> +                           entry->value->multi_overflow ? "on" : "off");
> +        }
> +        if (entry->value->has_uni_overflow) {
> +            monitor_printf(mon, " \\ uni_overflow: %s\n",
> +                           entry->value->uni_overflow ? "on" : "off");
> +        }
> +
> +        if (entry->value->has_unicast) {
> +            str_entry = entry->value->unicast;
> +            monitor_printf(mon, " \\ unicast:\n");
> +            while (str_entry) {
> +                monitor_printf(mon, "    %s\n", str_entry->value);
> +                str_entry = str_entry->next;
> +            }
> +        }
> +        if (entry->value->has_multicast) {
> +            str_entry = entry->value->multicast;
> +            monitor_printf(mon, " \\ multicast:\n");
> +            while (str_entry) {
> +                monitor_printf(mon, "    %s\n", str_entry->value);
> +                str_entry = str_entry->next;
> +            }
> +        }
> +
> +        entry = entry->next;
> +    }
> +    qapi_free_MacTableInfoList(table_list);
> +}
> +
>  void hmp_quit(Monitor *mon, const QDict *qdict)
>  {
>      monitor_suspend(mon);
> diff --git a/hmp.h b/hmp.h
> index 95fe76e..278c722 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
>  void hmp_info_pci(Monitor *mon, const QDict *qdict);
>  void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
>  void hmp_info_tpm(Monitor *mon, const QDict *qdict);
> +void hmp_info_mac_table(Monitor *mon, const QDict *qdict);
>  void hmp_quit(Monitor *mon, const QDict *qdict);
>  void hmp_stop(Monitor *mon, const QDict *qdict);
>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a9b8f53..e4b2358 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -194,6 +194,68 @@ static void virtio_net_set_link_status(NetClientState *nc)
>      virtio_net_set_status(vdev, vdev->status);
>  }
>  
> +static MacTableInfo *virtio_net_query_mactable(NetClientState *nc)
> +{
> +    VirtIONet *n = qemu_get_nic_opaque(nc);
> +    MacTableInfo *info;
> +    strList *str_list = NULL;
> +    strList *entry;
> +    int i;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->name = g_strdup(nc->name);
> +
> +    info->promisc = n->promisc;
> +    info->has_promisc = true;
> +    info->allmulti = n->allmulti;
> +    info->has_allmulti = true;
> +    info->alluni = n->alluni;
> +    info->has_alluni = true;
> +    info->nomulti = n->nomulti;
> +    info->has_nomulti = true;
> +    info->nouni = n->nouni;
> +    info->has_nouni = true;
> +    info->nobcast = n->nobcast;
> +    info->has_nobcast = true;
> +    info->multi_overflow = n->mac_table.multi_overflow;
> +    info->has_multi_overflow = true;
> +    info->uni_overflow = n->mac_table.uni_overflow;
> +    info->has_uni_overflow = true;
> +
> +    for (i = 0; i < n->mac_table.first_multi; i++) {
> +        info->has_unicast = true;
> +        entry = g_malloc0(sizeof(*entry));
> +        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
> +                                       n->mac_table.macs[i * ETH_ALEN],
> +                                       n->mac_table.macs[i * ETH_ALEN + 1],
> +                                       n->mac_table.macs[i * ETH_ALEN + 2],
> +                                       n->mac_table.macs[i * ETH_ALEN + 3],
> +                                       n->mac_table.macs[i * ETH_ALEN + 4],
> +                                       n->mac_table.macs[i * ETH_ALEN + 5]);
> +        entry->next = str_list;
> +        str_list = entry;
> +    }
> +    info->unicast = str_list;
> +
> +    str_list = NULL;
> +    for (i = n->mac_table.first_multi; i < n->mac_table.in_use; i++) {
> +        info->has_multicast = true;
> +        entry = g_malloc0(sizeof(*entry));
> +        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
> +                                       n->mac_table.macs[i * ETH_ALEN],
> +                                       n->mac_table.macs[i * ETH_ALEN + 1],
> +                                       n->mac_table.macs[i * ETH_ALEN + 2],
> +                                       n->mac_table.macs[i * ETH_ALEN + 3],
> +                                       n->mac_table.macs[i * ETH_ALEN + 4],
> +                                       n->mac_table.macs[i * ETH_ALEN + 5]);
> +        entry->next = str_list;
> +        str_list = entry;
> +    }
> +    info->multicast = str_list;
> +
> +    return info;
> +}
> +
>  static void virtio_net_reset(VirtIODevice *vdev)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
> @@ -1255,6 +1317,7 @@ static NetClientInfo net_virtio_info = {
>      .receive = virtio_net_receive,
>          .cleanup = virtio_net_cleanup,
>      .link_status_changed = virtio_net_set_link_status,
> +    .query_mac_table = virtio_net_query_mactable,
>  };
>  
>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> diff --git a/include/net/net.h b/include/net/net.h
> index 43d85a1..c3ca4ea 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -49,6 +49,7 @@ typedef ssize_t (NetReceiveIOV)(NetClientState *, const struct iovec *, int);
>  typedef void (NetCleanup) (NetClientState *);
>  typedef void (LinkStatusChanged)(NetClientState *);
>  typedef void (NetClientDestructor)(NetClientState *);
> +typedef MacTableInfo *(QueryMacTable)(NetClientState *);
>  
>  typedef struct NetClientInfo {
>      NetClientOptionsKind type;
> @@ -59,6 +60,7 @@ typedef struct NetClientInfo {
>      NetCanReceive *can_receive;
>      NetCleanup *cleanup;
>      LinkStatusChanged *link_status_changed;
> +    QueryMacTable *query_mac_table;
>      NetPoll *poll;
>  } NetClientInfo;
>  
> diff --git a/monitor.c b/monitor.c
> index 9e51545..a01cdbd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2765,6 +2765,14 @@ static mon_cmd_t info_cmds[] = {
>          .mhandler.cmd = hmp_info_tpm,
>      },
>      {
> +        .name       = "mac-table",
> +        .args_type  = "name:s?",
> +        .params     = "[net client name]",
> +        .help       = "show the mac-table information for all nics (or"
> +                      " for the given nic)",
> +        .mhandler.cmd = hmp_info_mac_table,
> +    },
> +    {
>          .name       = NULL,
>      },
>  };
> diff --git a/net/net.c b/net/net.c
> index 43a74e4..9ff3006 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>                     nc->info_str);
>  }
>  
> +MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
> +                                      Error **errp)
> +{
> +    NetClientState *nc;
> +    MacTableInfoList *table_list = NULL, *last_entry = NULL;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        MacTableInfoList *entry;
> +        MacTableInfo *info;
> +
> +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> +            continue;
> +        }
> +        if (has_name && strcmp(nc->name, name) != 0) {
> +            continue;
> +        }
> +
> +        if (nc->info->query_mac_table) {
> +            info = nc->info->query_mac_table(nc);
> +            entry = g_malloc0(sizeof(*entry));
> +            entry->value = info;
> +
> +            if (!table_list) {
> +                table_list = entry;
> +            } else {
> +                last_entry->next = entry;
> +            }
> +            last_entry = entry;
> +        }
> +    }
> +
> +    if (table_list == NULL) {
> +        error_setg(errp, "invalid net client name: %s", name);
> +    }
> +
> +    return table_list;
> +}
> +
>  void do_info_network(Monitor *mon, const QDict *qdict)
>  {
>      NetClientState *nc, *peer;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 199744a..866650c 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3619,3 +3619,60 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +# @MacTableInfo:
> +#
> +# Rx-mode information of mac-table for a net client.
> +#
> +# @name: the net client name
> +#
> +# @promisc: #optional promiscuous mode (default: false)
> +#
> +# @allmulti: #optional all multicast mode (default: false)
> +#
> +# @alluni: #optional all unicast mode (default: false)
> +#
> +# @nomulti: #optional no multicast mode (default: false)
> +#
> +# @nouni: #optional no unicast mode (default: false)
> +#
> +# @nobcast: #optional no broadcast mode (default: false)
> +#
> +# @multi_overflow: #optional multicast overflow mode (default: false)
> +#
> +# @uni_overflow: #optional unicast overflow mode (default: false)
> +#
> +# @unicast: #optional a list of unicast macaddr string
> +#
> +# @multicast: #optional a list of multicast macaddr string
> +#
> +# Since 1.6
> +##
> +{ 'type': 'MacTableInfo',
> +  'data': {
> +    'name':            'str',
> +    '*promisc':        'bool',
> +    '*allmulti':       'bool',
> +    '*alluni':         'bool',
> +    '*nomulti':        'bool',
> +    '*nouni':          'bool',
> +    '*nobcast':        'bool',
> +    '*multi-overflow': 'bool',
> +    '*uni-overflow':   'bool',
> +    '*unicast':        ['str'],
> +    '*multicast':      ['str'] }}
> +
> +##
> +# @query-mac-table:
> +#
> +# Return mac-table information for all nics (or for the given nic).
> +#
> +# @name: #optional net client name
> +#
> +# Returns: list of @MacTableInfo for all nics (or for the given nic).
> +#          Returns an error if the given @name doesn't exist.
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-mac-table', 'data': { '*name': 'str' },
> +  'returns': ['MacTableInfo'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ffd130e..66826ab 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2932,3 +2932,56 @@ Example:
>  <- { "return": {} }
>  
>  EQMP
> +    {
> +        .name       = "query-mac-table",
> +        .args_type  = "name:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_query_mac_table,
> +    },
> +
> +SQMP
> +query-mac-table
> +---------------
> +
> +Show mac-table information.
> +
> +Returns a json-array of mac-table information for all nics (or for the
> +given nic), returning an error if the given nic doesn't exist.
> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-string)
> +- "promisc": promiscuous mode (json-bool, optional)
> +- "allmulti": all multicast mode (json-bool, optional)
> +- "alluni": all unicast mode (json-bool, optional)
> +- "nomulti":no multicast mode (json-bool, optional)
> +- "nouni": no unicast mode (json-bool, optional)
> +- "nobcast": no broadcast mode (json-bool, optional)
> +- "multi-overflow": multicast overflow mode (json-bool, optional)
> +- "uni-overflow": unicast overflow mode (json-bool, optional)
> +- "unicast": a jason-array of unicast macaddr string (optional)
> +- "multicast": a jason-array of multicast macaddr string (optional)
> +
> +Example:
> +
> +-> { "execute": "query-mac-table", "arguments": { "name": "vnet0" }}
> +<- { "return": [
> +        {
> +            "multi-overflow": false,
> +            "name": "vnet0",
> +            "uni-overflow": false,
> +            "nobcast": false,
> +            "promisc": true,
> +            "multicast": [
> +                "01:00:5e:00:00:01",
> +                "33:33:00:00:00:01",
> +                "33:33:ff:12:34:56"
> +            ],
> +            "nouni": false,
> +            "nomulti": false,
> +            "allmulti": false,
> +            "alluni": false
> +        }
> +      ]
> +   }
> +
> +EQMP
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Luiz Capitulino]

On Thu, 16 May 2013 15:17:45 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> > Introduce this new QMP event to notify management after guest changes
> > mac-table configuration.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  QMP/qmp-events.txt        | 14 ++++++++++++++
> >  hw/net/virtio-net.c       | 12 ++++++++++++
> >  include/monitor/monitor.h |  1 +
> >  monitor.c                 |  1 +
> >  4 files changed, 28 insertions(+)
> > 
> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > index 92fe5fb..24d62df 100644
> > --- a/QMP/qmp-events.txt
> > +++ b/QMP/qmp-events.txt
> > @@ -154,6 +154,20 @@ Data:
> >              "path": "/machine/peripheral/virtio-net-pci-0" },
> >    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> >  
> > +MAC_TABLE_CHANGED
> > +-----------------
> > +
> > +Emitted mac-table configuration is changed by the guest.
> > +
> > +Data:
> > +
> > +- "name": net client name (json-string)
> > +
> > +{ "event": "MAC_TABLE_CHANGED",
> > +  "data": { "name": "vnet0" },
> > +  "timestamp": { "seconds": 1368697518, "microseconds": 326866 }}
> > +}
> > +
> >  DEVICE_TRAY_MOVED
> >  -----------------
> >  
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index bed0822..a9b8f53 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -21,6 +21,8 @@
> >  #include "hw/virtio/virtio-net.h"
> >  #include "net/vhost_net.h"
> >  #include "hw/virtio/virtio-bus.h"
> > +#include "qapi/qmp/qjson.h"
> > +#include "monitor/monitor.h"
> >  
> >  #define VIRTIO_NET_VM_VERSION    11
> >  
> > @@ -395,6 +397,7 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> >  {
> >      uint8_t on;
> >      size_t s;
> > +    QObject *event_data;
> >  
> >      s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
> >      if (s != sizeof(on)) {
> > @@ -417,6 +420,10 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> >          return VIRTIO_NET_ERR;
> >      }
> >  
> > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > +    qobject_decref(event_data);
> > +
> >      return VIRTIO_NET_OK;
> >  }
> >  
> 
> Sorry, pls ignore my previous mail, I see you actually
> emit this on rx mode change as well.
> 
> I find the name misleading or at least it mislead me :)
> RX_FILTER_CHANGED?
> 
> > @@ -425,6 +432,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >  {
> >      struct virtio_net_ctrl_mac mac_data;
> >      size_t s;
> > +    QObject *event_data;
> >  
> >      if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
> >          if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
> > @@ -497,6 +505,10 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >          n->mac_table.multi_overflow = 1;
> >      }
> >  
> > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > +    qobject_decref(event_data);
> > +
> >      return VIRTIO_NET_OK;
> >  }
> > 
> 
> This makes it easy for guest to flood management with
> spurious events.
> How about we set a flag after this, and avoid sending any more
> events until management queries the filter status?

We have an API for that, look at monitor_protocol_event_init().

>  
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index 1a6cfcf..e88c70f 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -40,6 +40,7 @@ typedef enum MonitorEvent {
> >      QEVENT_BLOCK_JOB_ERROR,
> >      QEVENT_BLOCK_JOB_READY,
> >      QEVENT_DEVICE_DELETED,
> > +    QEVENT_MAC_TABLE_CHANGED,
> >      QEVENT_DEVICE_TRAY_MOVED,
> >      QEVENT_SUSPEND,
> >      QEVENT_SUSPEND_DISK,
> > diff --git a/monitor.c b/monitor.c
> > index 62aaebe..9e51545 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -490,6 +490,7 @@ static const char *monitor_event_names[] = {
> >      [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
> >      [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
> >      [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED",
> > +    [QEVENT_MAC_TABLE_CHANGED] = "MAC_TABLE_CHANGED",
> >      [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
> >      [QEVENT_SUSPEND] = "SUSPEND",
> >      [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK",
> > -- 
> > 1.8.1.4
> 

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 16, 2013 at 08:24:03AM -0400, Luiz Capitulino wrote:
> On Thu, 16 May 2013 15:17:45 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> > > Introduce this new QMP event to notify management after guest changes
> > > mac-table configuration.
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > >  QMP/qmp-events.txt        | 14 ++++++++++++++
> > >  hw/net/virtio-net.c       | 12 ++++++++++++
> > >  include/monitor/monitor.h |  1 +
> > >  monitor.c                 |  1 +
> > >  4 files changed, 28 insertions(+)
> > > 
> > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > > index 92fe5fb..24d62df 100644
> > > --- a/QMP/qmp-events.txt
> > > +++ b/QMP/qmp-events.txt
> > > @@ -154,6 +154,20 @@ Data:
> > >              "path": "/machine/peripheral/virtio-net-pci-0" },
> > >    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> > >  
> > > +MAC_TABLE_CHANGED
> > > +-----------------
> > > +
> > > +Emitted mac-table configuration is changed by the guest.
> > > +
> > > +Data:
> > > +
> > > +- "name": net client name (json-string)
> > > +
> > > +{ "event": "MAC_TABLE_CHANGED",
> > > +  "data": { "name": "vnet0" },
> > > +  "timestamp": { "seconds": 1368697518, "microseconds": 326866 }}
> > > +}
> > > +
> > >  DEVICE_TRAY_MOVED
> > >  -----------------
> > >  
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index bed0822..a9b8f53 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -21,6 +21,8 @@
> > >  #include "hw/virtio/virtio-net.h"
> > >  #include "net/vhost_net.h"
> > >  #include "hw/virtio/virtio-bus.h"
> > > +#include "qapi/qmp/qjson.h"
> > > +#include "monitor/monitor.h"
> > >  
> > >  #define VIRTIO_NET_VM_VERSION    11
> > >  
> > > @@ -395,6 +397,7 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> > >  {
> > >      uint8_t on;
> > >      size_t s;
> > > +    QObject *event_data;
> > >  
> > >      s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
> > >      if (s != sizeof(on)) {
> > > @@ -417,6 +420,10 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> > >          return VIRTIO_NET_ERR;
> > >      }
> > >  
> > > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > > +    qobject_decref(event_data);
> > > +
> > >      return VIRTIO_NET_OK;
> > >  }
> > >  
> > 
> > Sorry, pls ignore my previous mail, I see you actually
> > emit this on rx mode change as well.
> > 
> > I find the name misleading or at least it mislead me :)
> > RX_FILTER_CHANGED?
> > 
> > > @@ -425,6 +432,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > >  {
> > >      struct virtio_net_ctrl_mac mac_data;
> > >      size_t s;
> > > +    QObject *event_data;
> > >  
> > >      if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
> > >          if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
> > > @@ -497,6 +505,10 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > >          n->mac_table.multi_overflow = 1;
> > >      }
> > >  
> > > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > > +    qobject_decref(event_data);
> > > +
> > >      return VIRTIO_NET_OK;
> > >  }
> > > 
> > 
> > This makes it easy for guest to flood management with
> > spurious events.
> > How about we set a flag after this, and avoid sending any more
> > events until management queries the filter status?
> 
> We have an API for that, look at monitor_protocol_event_init().

You mean monitor_protocol_event_throttle?
So what happens if guest triggers more
MAC changes per second?

> >  
> > > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > > index 1a6cfcf..e88c70f 100644
> > > --- a/include/monitor/monitor.h
> > > +++ b/include/monitor/monitor.h
> > > @@ -40,6 +40,7 @@ typedef enum MonitorEvent {
> > >      QEVENT_BLOCK_JOB_ERROR,
> > >      QEVENT_BLOCK_JOB_READY,
> > >      QEVENT_DEVICE_DELETED,
> > > +    QEVENT_MAC_TABLE_CHANGED,
> > >      QEVENT_DEVICE_TRAY_MOVED,
> > >      QEVENT_SUSPEND,
> > >      QEVENT_SUSPEND_DISK,
> > > diff --git a/monitor.c b/monitor.c
> > > index 62aaebe..9e51545 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -490,6 +490,7 @@ static const char *monitor_event_names[] = {
> > >      [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
> > >      [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
> > >      [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED",
> > > +    [QEVENT_MAC_TABLE_CHANGED] = "MAC_TABLE_CHANGED",
> > >      [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
> > >      [QEVENT_SUSPEND] = "SUSPEND",
> > >      [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK",
> > > -- 
> > > 1.8.1.4
> > 

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Luiz Capitulino]

On Thu, 16 May 2013 15:45:17 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, May 16, 2013 at 08:24:03AM -0400, Luiz Capitulino wrote:
> > On Thu, 16 May 2013 15:17:45 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> > > > Introduce this new QMP event to notify management after guest changes
> > > > mac-table configuration.
> > > > 
> > > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > > ---
> > > >  QMP/qmp-events.txt        | 14 ++++++++++++++
> > > >  hw/net/virtio-net.c       | 12 ++++++++++++
> > > >  include/monitor/monitor.h |  1 +
> > > >  monitor.c                 |  1 +
> > > >  4 files changed, 28 insertions(+)
> > > > 
> > > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > > > index 92fe5fb..24d62df 100644
> > > > --- a/QMP/qmp-events.txt
> > > > +++ b/QMP/qmp-events.txt
> > > > @@ -154,6 +154,20 @@ Data:
> > > >              "path": "/machine/peripheral/virtio-net-pci-0" },
> > > >    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> > > >  
> > > > +MAC_TABLE_CHANGED
> > > > +-----------------
> > > > +
> > > > +Emitted mac-table configuration is changed by the guest.
> > > > +
> > > > +Data:
> > > > +
> > > > +- "name": net client name (json-string)
> > > > +
> > > > +{ "event": "MAC_TABLE_CHANGED",
> > > > +  "data": { "name": "vnet0" },
> > > > +  "timestamp": { "seconds": 1368697518, "microseconds": 326866 }}
> > > > +}
> > > > +
> > > >  DEVICE_TRAY_MOVED
> > > >  -----------------
> > > >  
> > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > index bed0822..a9b8f53 100644
> > > > --- a/hw/net/virtio-net.c
> > > > +++ b/hw/net/virtio-net.c
> > > > @@ -21,6 +21,8 @@
> > > >  #include "hw/virtio/virtio-net.h"
> > > >  #include "net/vhost_net.h"
> > > >  #include "hw/virtio/virtio-bus.h"
> > > > +#include "qapi/qmp/qjson.h"
> > > > +#include "monitor/monitor.h"
> > > >  
> > > >  #define VIRTIO_NET_VM_VERSION    11
> > > >  
> > > > @@ -395,6 +397,7 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> > > >  {
> > > >      uint8_t on;
> > > >      size_t s;
> > > > +    QObject *event_data;
> > > >  
> > > >      s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
> > > >      if (s != sizeof(on)) {
> > > > @@ -417,6 +420,10 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> > > >          return VIRTIO_NET_ERR;
> > > >      }
> > > >  
> > > > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > > > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > > > +    qobject_decref(event_data);
> > > > +
> > > >      return VIRTIO_NET_OK;
> > > >  }
> > > >  
> > > 
> > > Sorry, pls ignore my previous mail, I see you actually
> > > emit this on rx mode change as well.
> > > 
> > > I find the name misleading or at least it mislead me :)
> > > RX_FILTER_CHANGED?
> > > 
> > > > @@ -425,6 +432,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > > >  {
> > > >      struct virtio_net_ctrl_mac mac_data;
> > > >      size_t s;
> > > > +    QObject *event_data;
> > > >  
> > > >      if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
> > > >          if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
> > > > @@ -497,6 +505,10 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > > >          n->mac_table.multi_overflow = 1;
> > > >      }
> > > >  
> > > > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > > > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > > > +    qobject_decref(event_data);
> > > > +
> > > >      return VIRTIO_NET_OK;
> > > >  }
> > > > 
> > > 
> > > This makes it easy for guest to flood management with
> > > spurious events.
> > > How about we set a flag after this, and avoid sending any more
> > > events until management queries the filter status?
> > 
> > We have an API for that, look at monitor_protocol_event_init().
> 
> You mean monitor_protocol_event_throttle?
> So what happens if guest triggers more
> MAC changes per second?

The QMP client will receive the newest one.

> 
> > >  
> > > > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > > > index 1a6cfcf..e88c70f 100644
> > > > --- a/include/monitor/monitor.h
> > > > +++ b/include/monitor/monitor.h
> > > > @@ -40,6 +40,7 @@ typedef enum MonitorEvent {
> > > >      QEVENT_BLOCK_JOB_ERROR,
> > > >      QEVENT_BLOCK_JOB_READY,
> > > >      QEVENT_DEVICE_DELETED,
> > > > +    QEVENT_MAC_TABLE_CHANGED,
> > > >      QEVENT_DEVICE_TRAY_MOVED,
> > > >      QEVENT_SUSPEND,
> > > >      QEVENT_SUSPEND_DISK,
> > > > diff --git a/monitor.c b/monitor.c
> > > > index 62aaebe..9e51545 100644
> > > > --- a/monitor.c
> > > > +++ b/monitor.c
> > > > @@ -490,6 +490,7 @@ static const char *monitor_event_names[] = {
> > > >      [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
> > > >      [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
> > > >      [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED",
> > > > +    [QEVENT_MAC_TABLE_CHANGED] = "MAC_TABLE_CHANGED",
> > > >      [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
> > > >      [QEVENT_SUSPEND] = "SUSPEND",
> > > >      [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK",
> > > > -- 
> > > > 1.8.1.4
> > > 
> 

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

On 05/16/2013 05:07 AM, Amos Kong wrote:
> Introduce this new QMP event to notify management after guest changes
> mac-table configuration.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  QMP/qmp-events.txt        | 14 ++++++++++++++
>  hw/net/virtio-net.c       | 12 ++++++++++++
>  include/monitor/monitor.h |  1 +
>  monitor.c                 |  1 +
>  4 files changed, 28 insertions(+)
> 
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index 92fe5fb..24d62df 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -154,6 +154,20 @@ Data:
>              "path": "/machine/peripheral/virtio-net-pci-0" },
>    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  
> +MAC_TABLE_CHANGED
> +-----------------
> +
> +Emitted mac-table configuration is changed by the guest.
> +
> +Data:
> +
> +- "name": net client name (json-string)
> +
> +{ "event": "MAC_TABLE_CHANGED",
> +  "data": { "name": "vnet0" },
> +  "timestamp": { "seconds": 1368697518, "microseconds": 326866 }}
> +}

Is it worth trying to also provide details about the change as part of
the event, to avoid having to do a round-trip query- command just to
learn what the new values are?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

On 05/16/2013 06:17 AM, Michael S. Tsirkin wrote:
> On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
>> Introduce this new QMP event to notify management after guest changes
>> mac-table configuration.
>>
> 
> This makes it easy for guest to flood management with
> spurious events.
> How about we set a flag after this, and avoid sending any more
> events until management queries the filter status?
>  

Or use rate-limiting, similar to what we have done for other
guest-triggered events (such as BALLOON_CHANGE), where management can
then tweak the maximum frequency at which it is willing to receive events.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 16, 2013 at 08:56:42AM -0600, Eric Blake wrote:
> On 05/16/2013 05:07 AM, Amos Kong wrote:
> > Introduce this new QMP event to notify management after guest changes
> > mac-table configuration.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  QMP/qmp-events.txt        | 14 ++++++++++++++
> >  hw/net/virtio-net.c       | 12 ++++++++++++
> >  include/monitor/monitor.h |  1 +
> >  monitor.c                 |  1 +
> >  4 files changed, 28 insertions(+)
> > 
> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > index 92fe5fb..24d62df 100644
> > --- a/QMP/qmp-events.txt
> > +++ b/QMP/qmp-events.txt
> > @@ -154,6 +154,20 @@ Data:
> >              "path": "/machine/peripheral/virtio-net-pci-0" },
> >    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> >  
> > +MAC_TABLE_CHANGED
> > +-----------------
> > +
> > +Emitted mac-table configuration is changed by the guest.
> > +
> > +Data:
> > +
> > +- "name": net client name (json-string)
> > +
> > +{ "event": "MAC_TABLE_CHANGED",
> > +  "data": { "name": "vnet0" },
> > +  "timestamp": { "seconds": 1368697518, "microseconds": 326866 }}
> > +}
> 
> Is it worth trying to also provide details about the change as part of
> the event, to avoid having to do a round-trip query- command just to
> learn what the new values are?
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

That really depends on the device though.
Some give you incremental add/delete mac commands,
others might let you replace the whole rx filter
in one go.

So if yes I'd say we should dump the whole table
not what changed.

-- 
MST

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 16, 2013 at 08:58:38AM -0600, Eric Blake wrote:
> On 05/16/2013 06:17 AM, Michael S. Tsirkin wrote:
> > On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> >> Introduce this new QMP event to notify management after guest changes
> >> mac-table configuration.
> >>
> > 
> > This makes it easy for guest to flood management with
> > spurious events.
> > How about we set a flag after this, and avoid sending any more
> > events until management queries the filter status?
> >  
> 
> Or use rate-limiting, similar to what we have done for other
> guest-triggered events (such as BALLOON_CHANGE), where management can
> then tweak the maximum frequency at which it is willing to receive events.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

I'm not sure how would management set the rate though,
and any throttling here might hurt the guest,
unlike the balloon.

OTOH what I proposed kind of moderates itself automatically.

-- 
MST

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 16, 2013 at 06:03:26PM +0300, Michael S. Tsirkin wrote:
> On Thu, May 16, 2013 at 08:58:38AM -0600, Eric Blake wrote:
> > On 05/16/2013 06:17 AM, Michael S. Tsirkin wrote:
> > > On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> > >> Introduce this new QMP event to notify management after guest changes
> > >> mac-table configuration.
> > >>
> > > 
> > > This makes it easy for guest to flood management with
> > > spurious events.
> > > How about we set a flag after this, and avoid sending any more
> > > events until management queries the filter status?
> > >  
> > 
> > Or use rate-limiting, similar to what we have done for other
> > guest-triggered events (such as BALLOON_CHANGE), where management can
> > then tweak the maximum frequency at which it is willing to receive events.
> > 
> > -- 
> > Eric Blake   eblake redhat com    +1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> > 
> 
> I'm not sure how would management set the rate though,
> and any throttling here might hurt the guest,
> unlike the balloon.
> 
> OTOH what I proposed kind of moderates itself automatically.

To clarify the issue:
- guest might be changing macs a lot not because it
	is malicious, but because it has a reason to do it.
	delaying the filter update for such a guest
	would drop lots of packets.

To clarify what I am proposing:
- on info mac-table -> clear flag
- on mac-table change -> test and set flag
	if was not set -> send event to management
	if was set -> do not send event

This way management does not get events faster than
it can handle them.

> -- 
> MST

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3044 bytes --]

On 05/16/2013 09:03 AM, Michael S. Tsirkin wrote:
> On Thu, May 16, 2013 at 08:58:38AM -0600, Eric Blake wrote:
>> On 05/16/2013 06:17 AM, Michael S. Tsirkin wrote:
>>> On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
>>>> Introduce this new QMP event to notify management after guest changes
>>>> mac-table configuration.
>>>>
>>>
>>> This makes it easy for guest to flood management with
>>> spurious events.
>>> How about we set a flag after this, and avoid sending any more
>>> events until management queries the filter status?
>>>  
>>
>> Or use rate-limiting, similar to what we have done for other
>> guest-triggered events (such as BALLOON_CHANGE), where management can
>> then tweak the maximum frequency at which it is willing to receive events.
>>
>> -- 
>> Eric Blake   eblake redhat com    +1-919-301-3266
>> Libvirt virtualization library http://libvirt.org
>>
> 
> I'm not sure how would management set the rate though,
> and any throttling here might hurt the guest,
> unlike the balloon.

If I understand how memballoon throttling works, throttling is NOT
guest-visible; it merely controls how frequently the guest can trigger
an event to the host.  The host always gets the latest guest status, but
only after a timeout has occurred since the last event sent (therefore,
2 back-to-back changes mean that the second event isn't sent until the
timeout elapses; 3 back-to-back means that the 2nd is dropped and only
the first and third changes get sent, with the 3rd waiting until after
the timeout).  That is, not all changes reach the host, the first change
always happens immediately, but subsequent changes may be deferred until
the timeout elapses, but the host will eventually see the final change,
and no slower than the frequency it configures for the throttling.

Or are you are arguing that if the guest makes a request, but the host
waits until a second has elapsed before it even gets the event to act on
the request, then the guest has suffered a performance loss?

> 
> OTOH what I proposed kind of moderates itself automatically.

Your approach (no more events until the host has acknowleged) has a
potential problem if the host misses the event (because of a libvirtd
restart, for example - but then again, on a libvirtd restart, libvirt
should probably query current state to get itself back in sync); and
also means that the host sees stale event data if subsequent events were
squelched because the host hasn't reacted to the first event yet.  The
existing throttling approach ensures that if the event includes latest
guest information, then the host doesn't even have to do do a query, and
is guaranteed that reacting to the final event will always see the most
recent request.  But most importantly, if the existing throttling works,
why do we have to invent a one-off approach for this event instead of
reusing existing code?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 16, 2013 at 09:12:36AM -0600, Eric Blake wrote:
> On 05/16/2013 09:03 AM, Michael S. Tsirkin wrote:
> > On Thu, May 16, 2013 at 08:58:38AM -0600, Eric Blake wrote:
> >> On 05/16/2013 06:17 AM, Michael S. Tsirkin wrote:
> >>> On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> >>>> Introduce this new QMP event to notify management after guest changes
> >>>> mac-table configuration.
> >>>>
> >>>
> >>> This makes it easy for guest to flood management with
> >>> spurious events.
> >>> How about we set a flag after this, and avoid sending any more
> >>> events until management queries the filter status?
> >>>  
> >>
> >> Or use rate-limiting, similar to what we have done for other
> >> guest-triggered events (such as BALLOON_CHANGE), where management can
> >> then tweak the maximum frequency at which it is willing to receive events.
> >>
> >> -- 
> >> Eric Blake   eblake redhat com    +1-919-301-3266
> >> Libvirt virtualization library http://libvirt.org
> >>
> > 
> > I'm not sure how would management set the rate though,
> > and any throttling here might hurt the guest,
> > unlike the balloon.
> 
> If I understand how memballoon throttling works, throttling is NOT
> guest-visible; it merely controls how frequently the guest can trigger
> an event to the host.  The host always gets the latest guest status, but
> only after a timeout has occurred since the last event sent (therefore,
> 2 back-to-back changes mean that the second event isn't sent until the
> timeout elapses; 3 back-to-back means that the 2nd is dropped and only
> the first and third changes get sent, with the 3rd waiting until after
> the timeout).  That is, not all changes reach the host, the first change
> always happens immediately, but subsequent changes may be deferred until
> the timeout elapses, but the host will eventually see the final change,
> and no slower than the frequency it configures for the throttling.
> 
> Or are you are arguing that if the guest makes a request, but the host
> waits until a second has elapsed before it even gets the event to act on
> the request, then the guest has suffered a performance loss?

Yes, that's what I'm saying.

> > 
> > OTOH what I proposed kind of moderates itself automatically.
> 
> Your approach (no more events until the host has acknowleged) has a
> potential problem if the host misses the event (because of a libvirtd
> restart, for example - but then again, on a libvirtd restart, libvirt
> should probably query current state to get itself back in sync);

exactly
> and
> also means that the host sees stale event data if subsequent events were
> squelched because the host hasn't reacted to the first event yet.

So, let's not send any data in the event. Amos's patch does exafctly
that.

> The
> existing throttling approach ensures that if the event includes latest
> guest information, then the host doesn't even have to do do a query, and
> is guaranteed that reacting to the final event will always see the most
> recent request.  But most importantly, if the existing throttling works,
> why do we have to invent a one-off approach for this event instead of
> reusing existing code?

Because of the 1st issue above. A large delay because we
exceed an arbitrary throttling rate would be bad
for the guest. Contrast with delay in e.g.
device delete event.
The throttling mechanism is good for events that host cares
about, not for events that guest cares about.

> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

On 05/16/2013 09:17 AM, Michael S. Tsirkin wrote:

>> The
>> existing throttling approach ensures that if the event includes latest
>> guest information, then the host doesn't even have to do do a query, and
>> is guaranteed that reacting to the final event will always see the most
>> recent request.  But most importantly, if the existing throttling works,
>> why do we have to invent a one-off approach for this event instead of
>> reusing existing code?
> 
> Because of the 1st issue above. A large delay because we
> exceed an arbitrary throttling rate would be bad
> for the guest. Contrast with delay in e.g.
> device delete event.
> The throttling mechanism is good for events that host cares
> about, not for events that guest cares about.

Alright, your argument has me convinced :)  Looks like we DO want to
react to the guest as fast as possible, for less missed traffic in the
guest, but also without overwhelming the host with events.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5020 bytes --]

On 05/16/2013 05:07 AM, Amos Kong wrote:
> We want to implement mac programming over macvtap through Libvirt.
> The previous patch adds QMP event to notify management of mac-table
> change. This patch adds a monitor command to query rx mode information
> of mac-tables.
> 

Focusing my review on just the QMP interface this go-around:

> +++ b/qapi-schema.json
> @@ -3619,3 +3619,60 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +# @MacTableInfo:
> +#
> +# Rx-mode information of mac-table for a net client.
> +#
> +# @name: the net client name
> +#
> +# @promisc: #optional promiscuous mode (default: false)
> +#
> +# @allmulti: #optional all multicast mode (default: false)
> +#
> +# @alluni: #optional all unicast mode (default: false)
> +#
> +# @nomulti: #optional no multicast mode (default: false)

Are allmulti and numulti orthogonal (all four pairings of possible), or
are they tied together (tri-state)?  If the latter, then maybe it's
better to have an enum value for the three states (none, normal, all)
and a singe @multi that resolves to one of those three states.

We don't necessarily need to abbreviate; would it be better giving these
fields a longer name, such as no-multicast?

> +#
> +# @nouni: #optional no unicast mode (default: false)

Same orthogonal vs. tri-state question about alluni/nouni.

> +#
> +# @nobcast: #optional no broadcast mode (default: false)

Again, if we don't abbreviate, should this be no-broadcast?

Double negatives can be awkward to work with; is it better to name this
'broadcast-allowed' with true being the default?

Is the point of labeling these fields #optional so that you can avoid
emitting them if they are in their default state?  Does it hurt to
always list all fields, instead of omitting ones in their default state?

> +#
> +# @multi_overflow: #optional multicast overflow mode (default: false)

New QMP interfaces should use '-' rather than '_'.

> +#
> +# @uni_overflow: #optional unicast overflow mode (default: false)
> +#
> +# @unicast: #optional a list of unicast macaddr string
> +#
> +# @multicast: #optional a list of multicast macaddr string
> +#
> +# Since 1.6
> +##
> +{ 'type': 'MacTableInfo',
> +  'data': {
> +    'name':            'str',
> +    '*promisc':        'bool',
> +    '*allmulti':       'bool',
> +    '*alluni':         'bool',
> +    '*nomulti':        'bool',
> +    '*nouni':          'bool',
> +    '*nobcast':        'bool',
> +    '*multi-overflow': 'bool',
> +    '*uni-overflow':   'bool',

Oh, you DID use dash, so your doc comments above are mismatched.

> +    '*unicast':        ['str'],
> +    '*multicast':      ['str'] }}
> +
> +##
> +# @query-mac-table:
> +#
> +# Return mac-table information for all nics (or for the given nic).
> +#
> +# @name: #optional net client name
> +#
> +# Returns: list of @MacTableInfo for all nics (or for the given nic).
> +#          Returns an error if the given @name doesn't exist.
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-mac-table', 'data': { '*name': 'str' },
> +  'returns': ['MacTableInfo'] }

I don't know if we answered the generic question of whether query-
commands should allow filtering; I kind of like it, though.  And for
this particular command, where we have an event telling us WHICH device
had a change to the table, libvirt is likely to take advantage of the
filtering (different from the query-command-line-options where I argued
that libvirt is unlikely to use the filtering).

> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-string)

s/jaso/json/

> +- "promisc": promiscuous mode (json-bool, optional)
> +- "allmulti": all multicast mode (json-bool, optional)
> +- "alluni": all unicast mode (json-bool, optional)
> +- "nomulti":no multicast mode (json-bool, optional)
> +- "nouni": no unicast mode (json-bool, optional)
> +- "nobcast": no broadcast mode (json-bool, optional)
> +- "multi-overflow": multicast overflow mode (json-bool, optional)
> +- "uni-overflow": unicast overflow mode (json-bool, optional)

For all of the optional bools, please document what the default is if
the field is omitted.  Or maybe we should just always emit them.

> +- "unicast": a jason-array of unicast macaddr string (optional)

s/jason/json/

Isn't it better to omit a 0-length array when there is explicitly no
unicast MAC registered, rather than omitting the field?  An omitted
field implies that there is not enough information available to decide
how many unicast addresses are registered.

> +- "multicast": a jason-array of multicast macaddr string (optional)

Likewise on preferring a 0-length array rather than omitting the field
altogether.

Looking forward to v2.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Stefan Hajnoczi]

On Thu, May 16, 2013 at 07:07:25PM +0800, Amos Kong wrote:
> @@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>                     nc->info_str);
>  }
>  
> +MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
> +                                      Error **errp)
> +{
> +    NetClientState *nc;
> +    MacTableInfoList *table_list = NULL, *last_entry = NULL;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        MacTableInfoList *entry;
> +        MacTableInfo *info;
> +
> +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> +            continue;
> +        }
> +        if (has_name && strcmp(nc->name, name) != 0) {
> +            continue;
> +        }
> +
> +        if (nc->info->query_mac_table) {
> +            info = nc->info->query_mac_table(nc);
> +            entry = g_malloc0(sizeof(*entry));
> +            entry->value = info;
> +
> +            if (!table_list) {
> +                table_list = entry;
> +            } else {
> +                last_entry->next = entry;
> +            }
> +            last_entry = entry;
> +        }
> +    }
> +
> +    if (table_list == NULL) {
> +        error_setg(errp, "invalid net client name: %s", name);
> +    }

Produces confusing errors:

1. If query-mac-table is used without a name argument and the guest has
   no NIC or no NICs support ->query_mac_table().

2. If the named NIC does not support ->query_mac_table().

Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Amos Kong]

On Thu, May 16, 2013 at 03:19:54PM +0300, Michael S. Tsirkin wrote:
> On Thu, May 16, 2013 at 07:07:25PM +0800, Amos Kong wrote:
> > We want to implement mac programming over macvtap through Libvirt.
> > The previous patch adds QMP event to notify management of mac-table
> > change. This patch adds a monitor command to query rx mode information
> > of mac-tables.
> > 
> > (qemu) info mac-table vnet0
> > vnet0:
> >  \ promisc: on
> >  \ allmulti: off
> >  \ alluni: off
> >  \ nomulti: off
> >  \ nouni: off
> >  \ nobcast: off
> >  \ multi_overflow: off
> >  \ uni_overflow: off
> >  \ multicast:
> >     01:00:5e:00:00:01
> >     33:33:00:00:00:01
> >     33:33:ff:12:34:56
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
 
Hi Michael,

> It's an interesting example.
>
> There are no unicast addresses at all?

Currently we save the main MAC in n->mac (not in n->mactable).
I only output the content of n->mactable.

And main MAC doesn't exist in guest's virtio_net_dev->uc, so it's not
outputted.

The main MAC can also be got by 'info network', but not coverted to QMP.
I will add main MAC in query result and add event for its change.

----
I just started a guest by :
qemu .. -device virtio-net-pci,netdev=ndev1,id=id1 -netdev tap,id=ndev1 \
        -device e1000,netdev=ndev2,id=id2 -netdev tap,id=ndev2

The default mac address was used.
    virtio-nic: 52:54:00:12:34:56
     e1000 nic: 52:54:00:12:34:57

52:54:00:12:34:56 doesn't exist in guest's virtio_net_dev->uc.

----
Another example:
I tried to add a new vlan for virtio-nic in guest
 # vconfig add eth1 3 up

then changed mac of eth1
 # ifconfig eth1 hw ether 12:34:00:00:00:00

then I can see a mac address in unicast part of mac-table
 (qemu) info mac-table 
 id1:
 ...
  \ unicast:
     33:33:ff:12:34:56
  \ multicast:
     00:00:00:00:00:00
     01:80:c2:00:00:21
     01:00:5e:00:00:01
     33:33:00:00:00:01
     33:33:ff:00:00:00
 
> I'm guessing you are missing the main MAC.

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Amos Kong]

On Fri, May 17, 2013 at 09:39:31AM +0200, Stefan Hajnoczi wrote:
> On Thu, May 16, 2013 at 07:07:25PM +0800, Amos Kong wrote:

Hi Stefan,

> > @@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
> >                     nc->info_str);
> >  }
> >  
> > +MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
> > +                                      Error **errp)
> > +{
> > +    NetClientState *nc;
> > +    MacTableInfoList *table_list = NULL, *last_entry = NULL;
> > +
> > +    QTAILQ_FOREACH(nc, &net_clients, next) {
> > +        MacTableInfoList *entry;
> > +        MacTableInfo *info;
> > +
> > +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> > +            continue;
> > +        }

> > +        if (has_name && strcmp(nc->name, name) != 0) {
> > +            continue;
> > +        }

             if (has_name && strcmp(nc->name, name) != 0) {
                 error_setg(errp, "invalid net client name: %s", name);
                 break;
             }

> > +
> > +        if (nc->info->query_mac_table) {
> > +            info = nc->info->query_mac_table(nc);
> > +            entry = g_malloc0(sizeof(*entry));
> > +            entry->value = info;
> > +
> > +            if (!table_list) {
> > +                table_list = entry;
> > +            } else {
> > +                last_entry->next = entry;
> > +            }
> > +            last_entry = entry;

> > +        }

             } else if (has_name) {
                 error_setg(errp, "net client(%s) doesn't support mac-table querying", name);
                 break;
             }

> > +    }
> > +
> > +    if (table_list == NULL) {
> > +        error_setg(errp, "invalid net client name: %s", name);
> > +    }

         if (table_list == NULL && !error_is_set(errp)) {
             error_setg(errp, "no net client supports mac-table querying");
         }

> 
> Produces confusing errors:
> 
> 1. If query-mac-table is used without a name argument and the guest has
>    no NIC or no NICs support ->query_mac_table().
> 
> 2. If the named NIC does not support ->query_mac_table().

Thanks, will fix it.

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

On Thu, May 16, 2013 at 03:17:45PM +0300, Michael S. Tsirkin wrote:
> On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> > Introduce this new QMP event to notify management after guest changes
> > mac-table configuration.


> > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > +    qobject_decref(event_data);
> > +
> >      return VIRTIO_NET_OK;
> >  }
> >  
> 
> Sorry, pls ignore my previous mail, I see you actually
> emit this on rx mode change as well.
> 
> I find the name misleading or at least it mislead me :)
> RX_FILTER_CHANGED?

Agree.

What we query contain some of rx modes & mac-table content,
rx_filter is better.

I will also change monitor cmd to 'query-rx-filter'.
 
> > @@ -425,6 +432,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >  {
> >      struct virtio_net_ctrl_mac mac_data;
> >      size_t s;
> > +    QObject *event_data;
> >  
> >      if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
> >          if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
> > @@ -497,6 +505,10 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >          n->mac_table.multi_overflow = 1;
> >      }
> >  
> > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > +    qobject_decref(event_data);
> > +
> >      return VIRTIO_NET_OK;
> >  }
> > 
> 
> This makes it easy for guest to flood management with
> spurious events.

> How about we set a flag after this, and avoid sending any more
> events until management queries the filter status?

As you discussed in this thread, we need a flag to turn on/off
the event notification to avoid the flooding.

But we could not set the flag in first mac-table change to turn off
the notification. Becase one action(execute one cmd in guest) might
cause multiple events.

It would be flexible to add a parameter for query-mac-table to change
the flag. Or add a new command to change the flag.
 

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Stefan Hajnoczi]

On Tue, May 21, 2013 at 12:46:09PM +0800, Amos Kong wrote:
> On Fri, May 17, 2013 at 09:39:31AM +0200, Stefan Hajnoczi wrote:
> > On Thu, May 16, 2013 at 07:07:25PM +0800, Amos Kong wrote:
> 
> Hi Stefan,
> 
> > > @@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
> > >                     nc->info_str);
> > >  }
> > >  
> > > +MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
> > > +                                      Error **errp)
> > > +{
> > > +    NetClientState *nc;
> > > +    MacTableInfoList *table_list = NULL, *last_entry = NULL;
> > > +
> > > +    QTAILQ_FOREACH(nc, &net_clients, next) {
> > > +        MacTableInfoList *entry;
> > > +        MacTableInfo *info;
> > > +
> > > +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> > > +            continue;
> > > +        }
> 
> > > +        if (has_name && strcmp(nc->name, name) != 0) {
> > > +            continue;
> > > +        }
> 
>              if (has_name && strcmp(nc->name, name) != 0) {
>                  error_setg(errp, "invalid net client name: %s", name);
>                  break;
>              }

We still need to search the other NICs.  Bailing early doesn't work:
imagine we have nic1 and nic2.  If the user invokes query-mac-table nic2
then this code would return an error!

Stefan

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Tue, May 21, 2013 at 01:04:55PM +0800, Amos Kong wrote:
> On Thu, May 16, 2013 at 03:17:45PM +0300, Michael S. Tsirkin wrote:
> > On Thu, May 16, 2013 at 07:07:24PM +0800, Amos Kong wrote:
> > > Introduce this new QMP event to notify management after guest changes
> > > mac-table configuration.
> 
> 
> > > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > > +    qobject_decref(event_data);
> > > +
> > >      return VIRTIO_NET_OK;
> > >  }
> > >  
> > 
> > Sorry, pls ignore my previous mail, I see you actually
> > emit this on rx mode change as well.
> > 
> > I find the name misleading or at least it mislead me :)
> > RX_FILTER_CHANGED?
> 
> Agree.
> 
> What we query contain some of rx modes & mac-table content,
> rx_filter is better.
> 
> I will also change monitor cmd to 'query-rx-filter'.
>  
> > > @@ -425,6 +432,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > >  {
> > >      struct virtio_net_ctrl_mac mac_data;
> > >      size_t s;
> > > +    QObject *event_data;
> > >  
> > >      if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
> > >          if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
> > > @@ -497,6 +505,10 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > >          n->mac_table.multi_overflow = 1;
> > >      }
> > >  
> > > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > > +    qobject_decref(event_data);
> > > +
> > >      return VIRTIO_NET_OK;
> > >  }
> > > 
> > 
> > This makes it easy for guest to flood management with
> > spurious events.
> 
> > How about we set a flag after this, and avoid sending any more
> > events until management queries the filter status?
> 
> As you discussed in this thread, we need a flag to turn on/off
> the event notification to avoid the flooding.
> 
> But we could not set the flag in first mac-table change to turn off
> the notification. Becase one action(execute one cmd in guest) might
> cause multiple events.

I still think it will work, since the event does not have any
information, what does it matter that we send one and not many events?

> It would be flexible to add a parameter for query-mac-table to change
> the flag. Or add a new command to change the flag.
>  
> 
> -- 
> 			Amos.

Looks a bit too complex, to me.

-- 
MST

Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Amos Kong]

On Thu, May 16, 2013 at 09:38:01AM -0600, Eric Blake wrote:
> On 05/16/2013 05:07 AM, Amos Kong wrote:
> > We want to implement mac programming over macvtap through Libvirt.
> > The previous patch adds QMP event to notify management of mac-table
> > change. This patch adds a monitor command to query rx mode information
> > of mac-tables.
> > 
> 
> Focusing my review on just the QMP interface this go-around:
> 
> > +++ b/qapi-schema.json
> > @@ -3619,3 +3619,60 @@
> >              '*cpuid-input-ecx': 'int',
> >              'cpuid-register': 'X86CPURegister32',
> >              'features': 'int' } }
> > +
> > +# @MacTableInfo:
> > +#
> > +# Rx-mode information of mac-table for a net client.
> > +#
> > +# @name: the net client name
> > +#
> > +# @promisc: #optional promiscuous mode (default: false)
> > +#
> > +# @allmulti: #optional all multicast mode (default: false)
> > +#
> > +# @alluni: #optional all unicast mode (default: false)
> > +#
> > +# @nomulti: #optional no multicast mode (default: false)
> 
> Are allmulti and numulti orthogonal (all four pairings of possible), or
> are they tied together (tri-state)?

allmulti: receive all multicast packets
nomulti: don't receive multicast packets
normal: only receive multicast (in mac-table) packets

> If the latter, then maybe it's
> better to have an enum value for the three states (none, normal, all)
> and a singe @multi that resolves to one of those three states.

Sounds good. I frankly output the raw RX filter controls without
process & integrate.
 
> We don't necessarily need to abbreviate; would it be better giving these
> fields a longer name, such as no-multicast?

I used the abbreviations because they are same as parameters of config
tool. Management don't need this reminder, I will use no-multicast.
 
> > +#
> > +# @nouni: #optional no unicast mode (default: false)
> 
> Same orthogonal vs. tri-state question about alluni/nouni.
> 
> > +#
> > +# @nobcast: #optional no broadcast mode (default: false)
> 
> Again, if we don't abbreviate, should this be no-broadcast?
> 
> Double negatives can be awkward to work with; is it better to name this
> 'broadcast-allowed' with true being the default?

Ok.
 
> Is the point of labeling these fields #optional so that you can avoid
> emitting them if they are in their default state?

No.

Currently we only use rx-filter for virtio-nic, some rx-filer may
not be used by other emulated nic, so I used optional.

option 1:
   nic doesn't have the rx filter, nothing emitted (default :false)
   nic have the rx filter, emit the real value no matter it's true/falue

option 2:
   only emit when nic has the rx filter and value is true

option 2 should be right.

> Does it hurt to
> always list all fields, instead of omitting ones in their default state?
> 
> > +#
> > +# @multi_overflow: #optional multicast overflow mode (default: false)
 
...
 
> > +- "promisc": promiscuous mode (json-bool, optional)
> > +- "allmulti": all multicast mode (json-bool, optional)
> > +- "alluni": all unicast mode (json-bool, optional)
> > +- "nomulti":no multicast mode (json-bool, optional)
> > +- "nouni": no unicast mode (json-bool, optional)
> > +- "nobcast": no broadcast mode (json-bool, optional)
> > +- "multi-overflow": multicast overflow mode (json-bool, optional)
> > +- "uni-overflow": unicast overflow mode (json-bool, optional)
> 
> For all of the optional bools, please document what the default is if
> the field is omitted.  Or maybe we should just always emit them.

Yes, emit them all the time. If nic doesn't have the rx-filter, just set
it to default 'false'.
 
> > +- "unicast": a jason-array of unicast macaddr string (optional)
> 
> s/jason/json/
> 
> Isn't it better to omit a 0-length array when there is explicitly no
> unicast MAC registered, rather than omitting the field?  An omitted
> field implies that there is not enough information available to decide
> how many unicast addresses are registered.

So will remove the optional for unicast/multicast
 
> > +- "multicast": a jason-array of multicast macaddr string (optional)
> 
> Likewise on preferring a 0-length array rather than omitting the field
> altogether.
> 
> Looking forward to v2.

Thanks.

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

On Tue, May 21, 2013 at 11:51:17AM +0300, Michael S. Tsirkin wrote:
> On Tue, May 21, 2013 at 01:04:55PM +0800, Amos Kong wrote:
> > > >  

<snip>
> > > > +    event_data = qobject_from_jsonf("{ 'name': %s }", n->netclient_name);
> > > > +    monitor_protocol_event(QEVENT_MAC_TABLE_CHANGED, event_data);
> > > > +    qobject_decref(event_data);
> > > > +
> > > >      return VIRTIO_NET_OK;
> > > >  }
> > > > 
> > > 
> > > This makes it easy for guest to flood management with
> > > spurious events.
> > 
> > > How about we set a flag after this, and avoid sending any more
> > > events until management queries the filter status?
> > 
> > As you discussed in this thread, we need a flag to turn on/off
> > the event notification to avoid the flooding.
> > 
> > But we could not set the flag in first mac-table change to turn off
> > the notification.

I'm wrong. Management's query for first event will enable
notification. If management don't query, no problem here,
because we only need to know the latest rx-filter state.

> > Becase one action(execute one cmd in guest) might
> > cause multiple events.

|| To clarify what I am proposing:
|| - on info mac-table -> clear flag
|| - on mac-table change -> test and set flag
||       if was not set -> send event to management
||       if was set -> do not send event
 
> I still think it will work.

Yes, it works, effectively avoid the event flooding.

> since the event does not have any
> information, what does it matter that we send one and not many events?
> 
> > It would be flexible to add a parameter for query-mac-table to change
> > the flag. Or add a new command to change the flag.
> >  
> > -- 
> > 			Amos.
> 
> Looks a bit too complex, to me.

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Luiz Capitulino]

On Thu, 16 May 2013 18:17:23 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> > The
> > existing throttling approach ensures that if the event includes latest
> > guest information, then the host doesn't even have to do do a query, and
> > is guaranteed that reacting to the final event will always see the most
> > recent request.  But most importantly, if the existing throttling works,
> > why do we have to invent a one-off approach for this event instead of
> > reusing existing code?

Sorry to restart this week old discussion, but I'm now reviewing the patch
in question and I dislike how we're coupling the event and the query
command.

> Because of the 1st issue above. A large delay because we

Has this been measured? How long is this large delay?

Also, is it impossible for management to issue query-rx-filter
on a reasonable rate that would also cause the same problems?
IOW, how can we be sure we're fixing anything without trying it
on a real use-case scenario?

> exceed an arbitrary throttling rate would be bad
> for the guest. Contrast with delay in e.g.
> device delete event.
> The throttling mechanism is good for events that host cares
> about, not for events that guest cares about.
> 
> > -- 
> > Eric Blake   eblake redhat com    +1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> > 
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 23, 2013 at 11:54:03AM -0400, Luiz Capitulino wrote:
> On Thu, 16 May 2013 18:17:23 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > The
> > > existing throttling approach ensures that if the event includes latest
> > > guest information, then the host doesn't even have to do do a query, and
> > > is guaranteed that reacting to the final event will always see the most
> > > recent request.  But most importantly, if the existing throttling works,
> > > why do we have to invent a one-off approach for this event instead of
> > > reusing existing code?
> 
> Sorry to restart this week old discussion, but I'm now reviewing the patch
> in question and I dislike how we're coupling the event and the query
> command.
> 
> > Because of the 1st issue above. A large delay because we
> 
> Has this been measured? How long is this large delay?
> 
> Also, is it impossible for management to issue query-rx-filter
> on a reasonable rate that would also cause the same problems?
> IOW, how can we be sure we're fixing anything without trying it
> on a real use-case scenario?

Play with priorities, you can make management arbitrarily slow.  It's
just not sane to assume any timing guarantees for tasks running on
Linux.

> > exceed an arbitrary throttling rate would be bad
> > for the guest. Contrast with delay in e.g.
> > device delete event.
> > The throttling mechanism is good for events that host cares
> > about, not for events that guest cares about.
> > 
> > > -- 
> > > Eric Blake   eblake redhat com    +1-919-301-3266
> > > Libvirt virtualization library http://libvirt.org
> > > 
> > 
> > 
> > 

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Luiz Capitulino]

On Thu, 23 May 2013 20:18:34 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, May 23, 2013 at 11:54:03AM -0400, Luiz Capitulino wrote:
> > On Thu, 16 May 2013 18:17:23 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > > The
> > > > existing throttling approach ensures that if the event includes latest
> > > > guest information, then the host doesn't even have to do do a query, and
> > > > is guaranteed that reacting to the final event will always see the most
> > > > recent request.  But most importantly, if the existing throttling works,
> > > > why do we have to invent a one-off approach for this event instead of
> > > > reusing existing code?
> > 
> > Sorry to restart this week old discussion, but I'm now reviewing the patch
> > in question and I dislike how we're coupling the event and the query
> > command.
> > 
> > > Because of the 1st issue above. A large delay because we
> > 
> > Has this been measured? How long is this large delay?
> > 
> > Also, is it impossible for management to issue query-rx-filter
> > on a reasonable rate that would also cause the same problems?
> > IOW, how can we be sure we're fixing anything without trying it
> > on a real use-case scenario?
> 
> Play with priorities, you can make management arbitrarily slow.  It's
> just not sane to assume any timing guarantees for tasks running on
> Linux.

Would you mind to elaborate? I'm not sure I understand how this answers
my questions.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 23, 2013 at 01:26:33PM -0400, Luiz Capitulino wrote:
> On Thu, 23 May 2013 20:18:34 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, May 23, 2013 at 11:54:03AM -0400, Luiz Capitulino wrote:
> > > On Thu, 16 May 2013 18:17:23 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > > The
> > > > > existing throttling approach ensures that if the event includes latest
> > > > > guest information, then the host doesn't even have to do do a query, and
> > > > > is guaranteed that reacting to the final event will always see the most
> > > > > recent request.  But most importantly, if the existing throttling works,
> > > > > why do we have to invent a one-off approach for this event instead of
> > > > > reusing existing code?
> > > 
> > > Sorry to restart this week old discussion, but I'm now reviewing the patch
> > > in question and I dislike how we're coupling the event and the query
> > > command.
> > > 
> > > > Because of the 1st issue above. A large delay because we
> > > 
> > > Has this been measured? How long is this large delay?
> > > 
> > > Also, is it impossible for management to issue query-rx-filter
> > > on a reasonable rate that would also cause the same problems?
> > > IOW, how can we be sure we're fixing anything without trying it
> > > on a real use-case scenario?
> > 
> > Play with priorities, you can make management arbitrarily slow.  It's
> > just not sane to assume any timing guarantees for tasks running on
> > Linux.
> 
> Would you mind to elaborate? I'm not sure I understand how this answers
> my questions.

Maybe I don't understand the questions.
You are asking why doesn't usual throttling sufficient?
This was discussed in this thread already.
That's because it would introduce a huge delay if guest
changes the mac too often. People don't except that
changing a mac is a thing the should do slowly.

Event throttling was not designed for events where
guest asks management to do something.

-- 
MST

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Luiz Capitulino]

On Fri, 24 May 2013 15:10:16 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, May 23, 2013 at 01:26:33PM -0400, Luiz Capitulino wrote:
> > On Thu, 23 May 2013 20:18:34 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, May 23, 2013 at 11:54:03AM -0400, Luiz Capitulino wrote:
> > > > On Thu, 16 May 2013 18:17:23 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > > The
> > > > > > existing throttling approach ensures that if the event includes latest
> > > > > > guest information, then the host doesn't even have to do do a query, and
> > > > > > is guaranteed that reacting to the final event will always see the most
> > > > > > recent request.  But most importantly, if the existing throttling works,
> > > > > > why do we have to invent a one-off approach for this event instead of
> > > > > > reusing existing code?
> > > > 
> > > > Sorry to restart this week old discussion, but I'm now reviewing the patch
> > > > in question and I dislike how we're coupling the event and the query
> > > > command.
> > > > 
> > > > > Because of the 1st issue above. A large delay because we
> > > > 
> > > > Has this been measured? How long is this large delay?
> > > > 
> > > > Also, is it impossible for management to issue query-rx-filter
> > > > on a reasonable rate that would also cause the same problems?
> > > > IOW, how can we be sure we're fixing anything without trying it
> > > > on a real use-case scenario?
> > > 
> > > Play with priorities, you can make management arbitrarily slow.  It's
> > > just not sane to assume any timing guarantees for tasks running on
> > > Linux.
> > 
> > Would you mind to elaborate? I'm not sure I understand how this answers
> > my questions.
> 
> Maybe I don't understand the questions.
> You are asking why doesn't usual throttling sufficient?
> This was discussed in this thread already.
> That's because it would introduce a huge delay if guest
> changes the mac too often. People don't except that
> changing a mac is a thing the should do slowly.

You meant shouldn't?

If I got it correctly, all you want to avoid is to call qobject_from_jsonf()
and monitor_protocol_event() in the mac change path, because this will
slow down the guest. Did I get it?

If I did, my main point is whether or not the solution you're proposing
(which is to couple the event with the query command) is
appropriate. We're in user-space already, many things could slow
the guest down apart from the event generation.

Two questions:

 1. Do we know how slow (or how many packets are actually dropped)
    if the mac is changed too often *and* the event is always sent?

 2. Does this solution consider what happens if the QMP client does
    respond timely to the event by issuing the query-rx-filter
    command?

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

On Fri, May 24, 2013 at 08:51:36AM -0400, Luiz Capitulino wrote:
> On Fri, 24 May 2013 15:10:16 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, May 23, 2013 at 01:26:33PM -0400, Luiz Capitulino wrote:
> > > On Thu, 23 May 2013 20:18:34 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Thu, May 23, 2013 at 11:54:03AM -0400, Luiz Capitulino wrote:
> > > > > On Thu, 16 May 2013 18:17:23 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > > The
> > > > > > > existing throttling approach ensures that if the event includes latest
> > > > > > > guest information, then the host doesn't even have to do do a query, and
> > > > > > > is guaranteed that reacting to the final event will always see the most
> > > > > > > recent request.  But most importantly, if the existing throttling works,
> > > > > > > why do we have to invent a one-off approach for this event instead of
> > > > > > > reusing existing code?
> > > > > 
> > > > > Sorry to restart this week old discussion, but I'm now reviewing the patch
> > > > > in question and I dislike how we're coupling the event and the query
> > > > > command.
> > > > > 
> > > > > > Because of the 1st issue above. A large delay because we
> > > > > 
> > > > > Has this been measured? How long is this large delay?
> > > > > 
> > > > > Also, is it impossible for management to issue query-rx-filter
> > > > > on a reasonable rate that would also cause the same problems?
> > > > > IOW, how can we be sure we're fixing anything without trying it
> > > > > on a real use-case scenario?
> > > > 
> > > > Play with priorities, you can make management arbitrarily slow.  It's
> > > > just not sane to assume any timing guarantees for tasks running on
> > > > Linux.
> > > 
> > > Would you mind to elaborate? I'm not sure I understand how this answers
> > > my questions.
> > 
> > Maybe I don't understand the questions.
> > You are asking why doesn't usual throttling sufficient?
> > This was discussed in this thread already.
> > That's because it would introduce a huge delay if guest
> > changes the mac too often. People don't except that
> > changing a mac is a thing the should do slowly.

> You meant shouldn't?
> 
> If I got it correctly, all you want to avoid is to call qobject_from_jsonf()
> and monitor_protocol_event() in the mac change path, because this will
> slow down the guest. Did I get it?

No.

We use the QMP event to notify management about the mac changing.

In this thread, we _wrongly_ considered to use qmp approach to delay
the event for avoiding the flooding.

  eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);

Now we have a solution (using a flag to turn on/off the notify) to avoid the
flooding, only emit the event if we have no un-read event.

If we want to (flag is on) emit the event, we wish the event be sent ASAP
(so event_throttle isn't needed).
 
> If I did, my main point is whether or not the solution you're proposing
> (which is to couple the event with the query command) is
> appropriate. We're in user-space already, many things could slow
> the guest down apart from the event generation.
> 
> Two questions:
> 
>  1. Do we know how slow (or how many packets are actually dropped)
>     if the mac is changed too often *and* the event is always sent?

We always disable interface first, then change the macaddr.
But we just have patch to allow guest to change macaddr of
virtio-net when the interface is running.

| commit 2dcd0cce551983afe2f900125457f10bb5d980ae
| Author: Jiri Pirko <jpirko@redhat.com>
| Date:   Tue Dec 11 15:33:56 2012 -0500
| 
|     [virt] virtio_net: allow to change mac when iface is running

>  2. Does this solution consider what happens if the QMP client does
>     respond timely to the event by issuing the query-rx-filter
>     command?

We assume that the QMP client (management) cares about the mac changing
event, and will query the latest rx-filter state and sync to macvtap
device.

1) If QMP client respond timely to the event: that's what we expected :)

2) If QMP client doesn't respond timely to the event: packets might drop.
   If we change mac when the interface is running, we can accept trivial
   packets dropping.

For second condition, we need to test in real environment when libvirt
finishs the work of processing events.

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Luiz Capitulino]

On Mon, 27 May 2013 17:34:25 +0800
Amos Kong <akong@redhat.com> wrote:

> On Fri, May 24, 2013 at 08:51:36AM -0400, Luiz Capitulino wrote:
> > On Fri, 24 May 2013 15:10:16 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, May 23, 2013 at 01:26:33PM -0400, Luiz Capitulino wrote:
> > > > On Thu, 23 May 2013 20:18:34 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Thu, May 23, 2013 at 11:54:03AM -0400, Luiz Capitulino wrote:
> > > > > > On Thu, 16 May 2013 18:17:23 +0300
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > 
> > > > > > > > The
> > > > > > > > existing throttling approach ensures that if the event includes latest
> > > > > > > > guest information, then the host doesn't even have to do do a query, and
> > > > > > > > is guaranteed that reacting to the final event will always see the most
> > > > > > > > recent request.  But most importantly, if the existing throttling works,
> > > > > > > > why do we have to invent a one-off approach for this event instead of
> > > > > > > > reusing existing code?
> > > > > > 
> > > > > > Sorry to restart this week old discussion, but I'm now reviewing the patch
> > > > > > in question and I dislike how we're coupling the event and the query
> > > > > > command.
> > > > > > 
> > > > > > > Because of the 1st issue above. A large delay because we
> > > > > > 
> > > > > > Has this been measured? How long is this large delay?
> > > > > > 
> > > > > > Also, is it impossible for management to issue query-rx-filter
> > > > > > on a reasonable rate that would also cause the same problems?
> > > > > > IOW, how can we be sure we're fixing anything without trying it
> > > > > > on a real use-case scenario?
> > > > > 
> > > > > Play with priorities, you can make management arbitrarily slow.  It's
> > > > > just not sane to assume any timing guarantees for tasks running on
> > > > > Linux.
> > > > 
> > > > Would you mind to elaborate? I'm not sure I understand how this answers
> > > > my questions.
> > > 
> > > Maybe I don't understand the questions.
> > > You are asking why doesn't usual throttling sufficient?
> > > This was discussed in this thread already.
> > > That's because it would introduce a huge delay if guest
> > > changes the mac too often. People don't except that
> > > changing a mac is a thing the should do slowly.
> 
> > You meant shouldn't?
> > 
> > If I got it correctly, all you want to avoid is to call qobject_from_jsonf()
> > and monitor_protocol_event() in the mac change path, because this will
> > slow down the guest. Did I get it?
> 
> No.
> 
> We use the QMP event to notify management about the mac changing.
> 
> In this thread, we _wrongly_ considered to use qmp approach to delay
> the event for avoiding the flooding.
> 
>   eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);
> 
> Now we have a solution (using a flag to turn on/off the notify) to avoid the
> flooding, only emit the event if we have no un-read event.
> 
> If we want to (flag is on) emit the event, we wish the event be sent ASAP
> (so event_throttle isn't needed).

Unfortunately this doesn't answer my question. I did understand why you're
not using the event throttle API (which is because you don't want to slow down
the guest, not the QMP client).

My point is whether coupling the event with the query command is really
justified or even if it really fixes the problem. Two points:

 1. Coupling them is bad design, and will probably strike back, as we plan
    for a better API for events where events can be disabled

 2. Can you actually show the problem does exist so that we ensure this is
    not premature optimization? Might be a good idea to have this in the
    commit log

> > (which is to couple the event with the query command) is
> > appropriate. We're in user-space already, many things could slow
> > the guest down apart from the event generation.
> > 
> > Two questions:
> > 
> >  1. Do we know how slow (or how many packets are actually dropped)
> >     if the mac is changed too often *and* the event is always sent?
> 
> We always disable interface first, then change the macaddr.
> But we just have patch to allow guest to change macaddr of
> virtio-net when the interface is running.
> 
> | commit 2dcd0cce551983afe2f900125457f10bb5d980ae
> | Author: Jiri Pirko <jpirko@redhat.com>
> | Date:   Tue Dec 11 15:33:56 2012 -0500
> | 
> |     [virt] virtio_net: allow to change mac when iface is running
> 
> >  2. Does this solution consider what happens if the QMP client does
> >     respond timely to the event by issuing the query-rx-filter
> >     command?
> 
> We assume that the QMP client (management) cares about the mac changing
> event, and will query the latest rx-filter state and sync to macvtap
> device.
> 
> 1) If QMP client respond timely to the event: that's what we expected :)

Won't this slow down the guest? If not, why?

> 
> 2) If QMP client doesn't respond timely to the event: packets might drop.
>    If we change mac when the interface is running, we can accept trivial
>    packets dropping.
> 
> For second condition, we need to test in real environment when libvirt
> finishs the work of processing events.
> 

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Luiz Capitulino]

On Mon, 27 May 2013 09:10:11 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> > We use the QMP event to notify management about the mac changing.
> > 
> > In this thread, we _wrongly_ considered to use qmp approach to delay
> > the event for avoiding the flooding.
> > 
> >   eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);
> > 
> > Now we have a solution (using a flag to turn on/off the notify) to avoid the
> > flooding, only emit the event if we have no un-read event.
> > 
> > If we want to (flag is on) emit the event, we wish the event be sent ASAP
> > (so event_throttle isn't needed).
> 
> Unfortunately this doesn't answer my question. I did understand why you're
> not using the event throttle API (which is because you don't want to slow down
> the guest, not the QMP client).
> 
> My point is whether coupling the event with the query command is really
> justified or even if it really fixes the problem. Two points:
> 
>  1. Coupling them is bad design, and will probably strike back, as we plan
>     for a better API for events where events can be disabled

I meant we may in the future, for example, introduce the ability to disable
commands (and events). One could argue that the event w/o a query command
is not that useful, as events can be lost. But loosing an event is one thing,
not having it because it got disabled by a side effect is another.

But anyway, my main point in this thread is to make sure we at least
justify having this coupling. Aren't we optimizing prematurely? Aren't
we optimizing for a corner case? That's what I want to see answered.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

On Mon, May 27, 2013 at 09:24:28AM -0400, Luiz Capitulino wrote:
> On Mon, 27 May 2013 09:10:11 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
> > > We use the QMP event to notify management about the mac changing.
> > > 
> > > In this thread, we _wrongly_ considered to use qmp approach to delay
> > > the event for avoiding the flooding.
> > > 
> > >   eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);
> > > 
> > > Now we have a solution (using a flag to turn on/off the notify) to avoid the
> > > flooding, only emit the event if we have no un-read event.
> > > 
> > > If we want to (flag is on) emit the event, we wish the event be sent ASAP
> > > (so event_throttle isn't needed).
> > 
> > Unfortunately this doesn't answer my question. I did understand why you're
> > not using the event throttle API (which is because you don't want to slow down
> > the guest, not the QMP client).
> > 
> > My point is whether coupling the event with the query command is really
> > justified or even if it really fixes the problem. Two points:
> > 
> >  1. Coupling them is bad design, and will probably strike back, as we plan
> >     for a better API for events where events can be disabled
> 
> I meant we may in the future, for example, introduce the ability to disable
> commands (and events). One could argue that the event w/o a query command
> is not that useful, as events can be lost. But loosing an event is one thing,
> not having it because it got disabled by a side effect is another.

event_throttle() couples the event in QMP framework, but we use flags
to disabled the event from real source (emit points/senders). 

If we set the evstate->rate to -1, we can ignore the events in
monitor_protocol_event_queue(), but we could not control the event
emitting of each emit point (each nic).
 
> But anyway, my main point in this thread is to make sure we at least
> justify having this coupling. Aren't we optimizing prematurely? Aren't
> we optimizing for a corner case? That's what I want to see answered.

If it's a corner case, we don't need a general API to disable event.

We can disable this event by a flag, and introduce a new API
if we have same request from other events.

> >  2. Can you actually show the problem does exist so that we ensure this is
> >     not premature optimization? Might be a good idea to have this in the
> >     commit log
> > 
> > > > (which is to couple the event with the query command) is
> > > > appropriate. We're in user-space already, many things could slow
> > > > the guest down apart from the event generation.
> > > > 
> > > > Two questions:
> > > > 
> > > >  1. Do we know how slow (or how many packets are actually dropped)
> > > >     if the mac is changed too often *and* the event is always sent?
> > > 
> > > We always disable interface first, then change the macaddr.
> > > But we just have patch to allow guest to change macaddr of
> > > virtio-net when the interface is running.
> > > 
> > > | commit 2dcd0cce551983afe2f900125457f10bb5d980ae
> > > | Author: Jiri Pirko <jpirko@redhat.com>
> > > | Date:   Tue Dec 11 15:33:56 2012 -0500
> > > | 
> > > |     [virt] virtio_net: allow to change mac when iface is running
> > > 
> > > >  2. Does this solution consider what happens if the QMP client does
> > > >     respond timely to the event by issuing the query-rx-filter
> > > >     command?
> > > 
> > > We assume that the QMP client (management) cares about the mac changing
> > > event, and will query the latest rx-filter state and sync to macvtap
> > > device.
> > > 
> > > 1) If QMP client respond timely to the event: that's what we expected :)
> > 
> > Won't this slow down the guest? If not, why?

If guest changes fx-filter configs frequently & management always query the
event very timely, this will slow down the guest.

We should detect & process the abnormal behavior from management.
Management (qmp client) always respond timely to the event in the
begining. If guest changes rx-filter very frequently & continuous.
Then we increase the evstate->rate, even disable the event.

In the normal usecase, we should consider packet losing first (caused by
event delay + the delay is used by management to execute the change)

---
btw, currently we could not test in real environment. If related
libvirt work finishes, we can evaluate with real delays, packet
losing, etc.

The worst condition is we could not accept the delay(packet losing),
we need to consider other solution for mac programming of macvtap.

> > > 2) If QMP client doesn't respond timely to the event: packets might drop.
> > >    If we change mac when the interface is running, we can accept trivial
> > >    packets dropping.
> > > 
> > > For second condition, we need to test in real environment when libvirt
> > > finishs the work of processing events.

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Luiz Capitulino]

On Tue, 28 May 2013 06:43:04 +0800
Amos Kong <akong@redhat.com> wrote:

> On Mon, May 27, 2013 at 09:24:28AM -0400, Luiz Capitulino wrote:
> > On Mon, 27 May 2013 09:10:11 -0400
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > 
> > > > We use the QMP event to notify management about the mac changing.
> > > > 
> > > > In this thread, we _wrongly_ considered to use qmp approach to delay
> > > > the event for avoiding the flooding.
> > > > 
> > > >   eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);
> > > > 
> > > > Now we have a solution (using a flag to turn on/off the notify) to avoid the
> > > > flooding, only emit the event if we have no un-read event.
> > > > 
> > > > If we want to (flag is on) emit the event, we wish the event be sent ASAP
> > > > (so event_throttle isn't needed).
> > > 
> > > Unfortunately this doesn't answer my question. I did understand why you're
> > > not using the event throttle API (which is because you don't want to slow down
> > > the guest, not the QMP client).
> > > 
> > > My point is whether coupling the event with the query command is really
> > > justified or even if it really fixes the problem. Two points:
> > > 
> > >  1. Coupling them is bad design, and will probably strike back, as we plan
> > >     for a better API for events where events can be disabled
> > 
> > I meant we may in the future, for example, introduce the ability to disable
> > commands (and events). One could argue that the event w/o a query command
> > is not that useful, as events can be lost. But loosing an event is one thing,
> > not having it because it got disabled by a side effect is another.
> 
> event_throttle() couples the event in QMP framework, but we use flags
> to disabled the event from real source (emit points/senders). 
> 
> If we set the evstate->rate to -1, we can ignore the events in
> monitor_protocol_event_queue(), but we could not control the event
> emitting of each emit point (each nic).
>  
> > But anyway, my main point in this thread is to make sure we at least
> > justify having this coupling. Aren't we optimizing prematurely? Aren't
> > we optimizing for a corner case? That's what I want to see answered.
> 
> If it's a corner case, we don't need a general API to disable event.

If it's a corner case, it's really worth to fix it?

I think that what we need a real world test-case to show us we're
doing the right thing.

> We can disable this event by a flag, and introduce a new API
> if we have same request from other events.
> 
> > >  2. Can you actually show the problem does exist so that we ensure this is
> > >     not premature optimization? Might be a good idea to have this in the
> > >     commit log
> > > 
> > > > > (which is to couple the event with the query command) is
> > > > > appropriate. We're in user-space already, many things could slow
> > > > > the guest down apart from the event generation.
> > > > > 
> > > > > Two questions:
> > > > > 
> > > > >  1. Do we know how slow (or how many packets are actually dropped)
> > > > >     if the mac is changed too often *and* the event is always sent?
> > > > 
> > > > We always disable interface first, then change the macaddr.
> > > > But we just have patch to allow guest to change macaddr of
> > > > virtio-net when the interface is running.
> > > > 
> > > > | commit 2dcd0cce551983afe2f900125457f10bb5d980ae
> > > > | Author: Jiri Pirko <jpirko@redhat.com>
> > > > | Date:   Tue Dec 11 15:33:56 2012 -0500
> > > > | 
> > > > |     [virt] virtio_net: allow to change mac when iface is running
> > > > 
> > > > >  2. Does this solution consider what happens if the QMP client does
> > > > >     respond timely to the event by issuing the query-rx-filter
> > > > >     command?
> > > > 
> > > > We assume that the QMP client (management) cares about the mac changing
> > > > event, and will query the latest rx-filter state and sync to macvtap
> > > > device.
> > > > 
> > > > 1) If QMP client respond timely to the event: that's what we expected :)
> > > 
> > > Won't this slow down the guest? If not, why?
> 
> If guest changes fx-filter configs frequently & management always query the
> event very timely, this will slow down the guest.
> 
> We should detect & process the abnormal behavior from management.

That's not abnormal. Management is doing what it should do.

Maybe using the event throttle API can solve the mngt side of the problem,
but I still think we need a reproducible test-case to ensure we're doing
the right thing.

> Management (qmp client) always respond timely to the event in the
> begining. If guest changes rx-filter very frequently & continuous.
> Then we increase the evstate->rate, even disable the event.
> 
> In the normal usecase, we should consider packet losing first (caused by
> event delay + the delay is used by management to execute the change)
> 
> ---
> btw, currently we could not test in real environment. If related
> libvirt work finishes, we can evaluate with real delays, packet
> losing, etc.
> 
> The worst condition is we could not accept the delay(packet losing),
> we need to consider other solution for mac programming of macvtap.
> 
> > > > 2) If QMP client doesn't respond timely to the event: packets might drop.
> > > >    If we change mac when the interface is running, we can accept trivial
> > > >    packets dropping.
> > > > 
> > > > For second condition, we need to test in real environment when libvirt
> > > > finishs the work of processing events.
> 

Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Jason Wang]

On 05/16/2013 07:07 PM, Amos Kong wrote:
> We want to implement mac programming over macvtap through Libvirt.
> The previous patch adds QMP event to notify management of mac-table
> change. This patch adds a monitor command to query rx mode information
> of mac-tables.
>
> (qemu) info mac-table vnet0
> vnet0:
>  \ promisc: on
>  \ allmulti: off
>  \ alluni: off
>  \ nomulti: off
>  \ nouni: off
>  \ nobcast: off
>  \ multi_overflow: off
>  \ uni_overflow: off
>  \ multicast:
>     01:00:5e:00:00:01
>     33:33:00:00:00:01
>     33:33:ff:12:34:56
>
> Signed-off-by: Amos Kong <akong@redhat.com>

Maybe you also need a command to query the vlan table, or rename the
command as "info filter" and do it here.
> ---
>  hmp-commands.hx     |  2 ++
>  hmp.c               | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h               |  1 +
>  hw/net/virtio-net.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/net/net.h   |  2 ++
>  monitor.c           |  8 ++++++
>  net/net.c           | 38 ++++++++++++++++++++++++++++
>  qapi-schema.json    | 57 ++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx     | 53 +++++++++++++++++++++++++++++++++++++++
>  9 files changed, 295 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9cea415..e5c1b14 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1639,6 +1639,8 @@ show qdev device model list
>  show roms
>  @item info tpm
>  show the TPM device
> +@item info mac-table [net client name]
> +show the mac-table information for all nics (or for the given nic)
>  @end table
>  ETEXI
>  
> diff --git a/hmp.c b/hmp.c
> index 4fb76ec..3e19df0 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -653,6 +653,77 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
>      qapi_free_TPMInfoList(info_list);
>  }
>  
> +void hmp_info_mac_table(Monitor *mon, const QDict *qdict)
> +{
> +    MacTableInfoList *table_list, *entry;
> +    strList *str_entry;
> +    bool has_name = qdict_haskey(qdict, "name");
> +    const char *name = NULL;
> +
> +    if (has_name) {
> +        name = qdict_get_str(qdict, "name");
> +    }
> +
> +    table_list = qmp_query_mac_table(has_name, name, NULL);
> +    entry = table_list;
> +
> +    while (entry) {
> +        monitor_printf(mon, "%s:\n", entry->value->name);
> +        if (entry->value->has_promisc) {
> +            monitor_printf(mon, " \\ promisc: %s\n",
> +                           entry->value->promisc ? "on" : "off");
> +        }
> +        if (entry->value->has_allmulti) {
> +            monitor_printf(mon, " \\ allmulti: %s\n",
> +                           entry->value->allmulti ? "on" : "off");
> +        }
> +        if (entry->value->has_alluni) {
> +            monitor_printf(mon, " \\ alluni: %s\n",
> +                           entry->value->alluni ? "on" : "off");
> +        }
> +        if (entry->value->has_nomulti) {
> +            monitor_printf(mon, " \\ nomulti: %s\n",
> +                           entry->value->nomulti ? "on" : "off");
> +        }
> +        if (entry->value->has_nouni) {
> +            monitor_printf(mon, " \\ nouni: %s\n",
> +                           entry->value->nouni ? "on" : "off");
> +        }
> +        if (entry->value->has_nobcast) {
> +            monitor_printf(mon, " \\ nobcast: %s\n",
> +                           entry->value->nobcast ? "on" : "off");
> +        }
> +        if (entry->value->has_multi_overflow) {
> +            monitor_printf(mon, " \\ multi_overflow: %s\n",
> +                           entry->value->multi_overflow ? "on" : "off");
> +        }
> +        if (entry->value->has_uni_overflow) {
> +            monitor_printf(mon, " \\ uni_overflow: %s\n",
> +                           entry->value->uni_overflow ? "on" : "off");
> +        }
> +
> +        if (entry->value->has_unicast) {
> +            str_entry = entry->value->unicast;
> +            monitor_printf(mon, " \\ unicast:\n");
> +            while (str_entry) {
> +                monitor_printf(mon, "    %s\n", str_entry->value);
> +                str_entry = str_entry->next;
> +            }
> +        }
> +        if (entry->value->has_multicast) {
> +            str_entry = entry->value->multicast;
> +            monitor_printf(mon, " \\ multicast:\n");
> +            while (str_entry) {
> +                monitor_printf(mon, "    %s\n", str_entry->value);
> +                str_entry = str_entry->next;
> +            }
> +        }
> +
> +        entry = entry->next;
> +    }
> +    qapi_free_MacTableInfoList(table_list);
> +}
> +
>  void hmp_quit(Monitor *mon, const QDict *qdict)
>  {
>      monitor_suspend(mon);
> diff --git a/hmp.h b/hmp.h
> index 95fe76e..278c722 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
>  void hmp_info_pci(Monitor *mon, const QDict *qdict);
>  void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
>  void hmp_info_tpm(Monitor *mon, const QDict *qdict);
> +void hmp_info_mac_table(Monitor *mon, const QDict *qdict);
>  void hmp_quit(Monitor *mon, const QDict *qdict);
>  void hmp_stop(Monitor *mon, const QDict *qdict);
>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a9b8f53..e4b2358 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -194,6 +194,68 @@ static void virtio_net_set_link_status(NetClientState *nc)
>      virtio_net_set_status(vdev, vdev->status);
>  }
>  
> +static MacTableInfo *virtio_net_query_mactable(NetClientState *nc)
> +{
> +    VirtIONet *n = qemu_get_nic_opaque(nc);
> +    MacTableInfo *info;
> +    strList *str_list = NULL;
> +    strList *entry;
> +    int i;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->name = g_strdup(nc->name);
> +
> +    info->promisc = n->promisc;
> +    info->has_promisc = true;
> +    info->allmulti = n->allmulti;
> +    info->has_allmulti = true;
> +    info->alluni = n->alluni;
> +    info->has_alluni = true;
> +    info->nomulti = n->nomulti;
> +    info->has_nomulti = true;
> +    info->nouni = n->nouni;
> +    info->has_nouni = true;
> +    info->nobcast = n->nobcast;
> +    info->has_nobcast = true;
> +    info->multi_overflow = n->mac_table.multi_overflow;
> +    info->has_multi_overflow = true;
> +    info->uni_overflow = n->mac_table.uni_overflow;
> +    info->has_uni_overflow = true;
> +
> +    for (i = 0; i < n->mac_table.first_multi; i++) {
> +        info->has_unicast = true;
> +        entry = g_malloc0(sizeof(*entry));
> +        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
> +                                       n->mac_table.macs[i * ETH_ALEN],
> +                                       n->mac_table.macs[i * ETH_ALEN + 1],
> +                                       n->mac_table.macs[i * ETH_ALEN + 2],
> +                                       n->mac_table.macs[i * ETH_ALEN + 3],
> +                                       n->mac_table.macs[i * ETH_ALEN + 4],
> +                                       n->mac_table.macs[i * ETH_ALEN + 5]);
> +        entry->next = str_list;
> +        str_list = entry;
> +    }
> +    info->unicast = str_list;
> +
> +    str_list = NULL;
> +    for (i = n->mac_table.first_multi; i < n->mac_table.in_use; i++) {
> +        info->has_multicast = true;
> +        entry = g_malloc0(sizeof(*entry));
> +        entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x",
> +                                       n->mac_table.macs[i * ETH_ALEN],
> +                                       n->mac_table.macs[i * ETH_ALEN + 1],
> +                                       n->mac_table.macs[i * ETH_ALEN + 2],
> +                                       n->mac_table.macs[i * ETH_ALEN + 3],
> +                                       n->mac_table.macs[i * ETH_ALEN + 4],
> +                                       n->mac_table.macs[i * ETH_ALEN + 5]);
> +        entry->next = str_list;
> +        str_list = entry;
> +    }
> +    info->multicast = str_list;
> +
> +    return info;
> +}
> +
>  static void virtio_net_reset(VirtIODevice *vdev)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
> @@ -1255,6 +1317,7 @@ static NetClientInfo net_virtio_info = {
>      .receive = virtio_net_receive,
>          .cleanup = virtio_net_cleanup,
>      .link_status_changed = virtio_net_set_link_status,
> +    .query_mac_table = virtio_net_query_mactable,
>  };
>  
>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> diff --git a/include/net/net.h b/include/net/net.h
> index 43d85a1..c3ca4ea 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -49,6 +49,7 @@ typedef ssize_t (NetReceiveIOV)(NetClientState *, const struct iovec *, int);
>  typedef void (NetCleanup) (NetClientState *);
>  typedef void (LinkStatusChanged)(NetClientState *);
>  typedef void (NetClientDestructor)(NetClientState *);
> +typedef MacTableInfo *(QueryMacTable)(NetClientState *);
>  
>  typedef struct NetClientInfo {
>      NetClientOptionsKind type;
> @@ -59,6 +60,7 @@ typedef struct NetClientInfo {
>      NetCanReceive *can_receive;
>      NetCleanup *cleanup;
>      LinkStatusChanged *link_status_changed;
> +    QueryMacTable *query_mac_table;
>      NetPoll *poll;
>  } NetClientInfo;
>  
> diff --git a/monitor.c b/monitor.c
> index 9e51545..a01cdbd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2765,6 +2765,14 @@ static mon_cmd_t info_cmds[] = {
>          .mhandler.cmd = hmp_info_tpm,
>      },
>      {
> +        .name       = "mac-table",
> +        .args_type  = "name:s?",
> +        .params     = "[net client name]",
> +        .help       = "show the mac-table information for all nics (or"
> +                      " for the given nic)",
> +        .mhandler.cmd = hmp_info_mac_table,
> +    },
> +    {
>          .name       = NULL,
>      },
>  };
> diff --git a/net/net.c b/net/net.c
> index 43a74e4..9ff3006 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -961,6 +961,44 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>                     nc->info_str);
>  }
>  
> +MacTableInfoList *qmp_query_mac_table(bool has_name, const char *name,
> +                                      Error **errp)
> +{
> +    NetClientState *nc;
> +    MacTableInfoList *table_list = NULL, *last_entry = NULL;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        MacTableInfoList *entry;
> +        MacTableInfo *info;
> +
> +        if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) {
> +            continue;
> +        }
> +        if (has_name && strcmp(nc->name, name) != 0) {
> +            continue;
> +        }
> +
> +        if (nc->info->query_mac_table) {
> +            info = nc->info->query_mac_table(nc);
> +            entry = g_malloc0(sizeof(*entry));
> +            entry->value = info;
> +
> +            if (!table_list) {
> +                table_list = entry;
> +            } else {
> +                last_entry->next = entry;
> +            }
> +            last_entry = entry;
> +        }
> +    }
> +
> +    if (table_list == NULL) {
> +        error_setg(errp, "invalid net client name: %s", name);
> +    }
> +
> +    return table_list;
> +}
> +
>  void do_info_network(Monitor *mon, const QDict *qdict)
>  {
>      NetClientState *nc, *peer;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 199744a..866650c 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3619,3 +3619,60 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +# @MacTableInfo:
> +#
> +# Rx-mode information of mac-table for a net client.
> +#
> +# @name: the net client name
> +#
> +# @promisc: #optional promiscuous mode (default: false)
> +#
> +# @allmulti: #optional all multicast mode (default: false)
> +#
> +# @alluni: #optional all unicast mode (default: false)
> +#
> +# @nomulti: #optional no multicast mode (default: false)
> +#
> +# @nouni: #optional no unicast mode (default: false)
> +#
> +# @nobcast: #optional no broadcast mode (default: false)
> +#
> +# @multi_overflow: #optional multicast overflow mode (default: false)
> +#
> +# @uni_overflow: #optional unicast overflow mode (default: false)
> +#
> +# @unicast: #optional a list of unicast macaddr string
> +#
> +# @multicast: #optional a list of multicast macaddr string
> +#
> +# Since 1.6
> +##
> +{ 'type': 'MacTableInfo',
> +  'data': {
> +    'name':            'str',
> +    '*promisc':        'bool',
> +    '*allmulti':       'bool',
> +    '*alluni':         'bool',
> +    '*nomulti':        'bool',
> +    '*nouni':          'bool',
> +    '*nobcast':        'bool',
> +    '*multi-overflow': 'bool',
> +    '*uni-overflow':   'bool',
> +    '*unicast':        ['str'],
> +    '*multicast':      ['str'] }}
> +
> +##
> +# @query-mac-table:
> +#
> +# Return mac-table information for all nics (or for the given nic).
> +#
> +# @name: #optional net client name
> +#
> +# Returns: list of @MacTableInfo for all nics (or for the given nic).
> +#          Returns an error if the given @name doesn't exist.
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-mac-table', 'data': { '*name': 'str' },
> +  'returns': ['MacTableInfo'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ffd130e..66826ab 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2932,3 +2932,56 @@ Example:
>  <- { "return": {} }
>  
>  EQMP
> +    {
> +        .name       = "query-mac-table",
> +        .args_type  = "name:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_query_mac_table,
> +    },
> +
> +SQMP
> +query-mac-table
> +---------------
> +
> +Show mac-table information.
> +
> +Returns a json-array of mac-table information for all nics (or for the
> +given nic), returning an error if the given nic doesn't exist.
> +
> +Each array entry contains the following:
> +
> +- "name": net client name (jaso-string)
> +- "promisc": promiscuous mode (json-bool, optional)
> +- "allmulti": all multicast mode (json-bool, optional)
> +- "alluni": all unicast mode (json-bool, optional)
> +- "nomulti":no multicast mode (json-bool, optional)
> +- "nouni": no unicast mode (json-bool, optional)
> +- "nobcast": no broadcast mode (json-bool, optional)
> +- "multi-overflow": multicast overflow mode (json-bool, optional)
> +- "uni-overflow": unicast overflow mode (json-bool, optional)
> +- "unicast": a jason-array of unicast macaddr string (optional)
> +- "multicast": a jason-array of multicast macaddr string (optional)
> +
> +Example:
> +
> +-> { "execute": "query-mac-table", "arguments": { "name": "vnet0" }}
> +<- { "return": [
> +        {
> +            "multi-overflow": false,
> +            "name": "vnet0",
> +            "uni-overflow": false,
> +            "nobcast": false,
> +            "promisc": true,
> +            "multicast": [
> +                "01:00:5e:00:00:01",
> +                "33:33:00:00:00:01",
> +                "33:33:ff:12:34:56"
> +            ],
> +            "nouni": false,
> +            "nomulti": false,
> +            "allmulti": false,
> +            "alluni": false
> +        }
> +      ]
> +   }
> +
> +EQMP

Re: [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

On Tue, May 28, 2013 at 08:25:56AM -0400, Luiz Capitulino wrote:
> On Tue, 28 May 2013 06:43:04 +0800
> Amos Kong <akong@redhat.com> wrote:

CC: netdev, vlad

Currently we create & open macvtap device by libvirt(management),
and pass the fd to qemu process. And macvtap works in promiscuous
mode, we want to sync the rx-filter setup of virtio-net to macvtap
device for better performance.

qemu might be a non-privileged process, it doesn't have permission
to setup macvtap device. So we want to add an QMP event to notify
management to execute the setup.

mac-programming over macvtap patch for qemu:
http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg03337.html


Can we re-consider to setup macvtap in qemu directly? open some setup
permission of rx-filter to qemu process? will do some investigation.
 

> > On Mon, May 27, 2013 at 09:24:28AM -0400, Luiz Capitulino wrote:
> > > On Mon, 27 May 2013 09:10:11 -0400
> > > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > > 
> > > > > We use the QMP event to notify management about the mac changing.
> > > > > 
> > > > > In this thread, we _wrongly_ considered to use qmp approach to delay
> > > > > the event for avoiding the flooding.
> > > > > 
> > > > >   eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);
> > > > > 
> > > > > Now we have a solution (using a flag to turn on/off the notify) to avoid the
> > > > > flooding, only emit the event if we have no un-read event.
> > > > > 
> > > > > If we want to (flag is on) emit the event, we wish the event be sent ASAP
> > > > > (so event_throttle isn't needed).
> > > > 
> > > > Unfortunately this doesn't answer my question. I did understand why you're
> > > > not using the event throttle API (which is because you don't want to slow down
> > > > the guest, not the QMP client).
> > > > 
> > > > My point is whether coupling the event with the query command is really
> > > > justified or even if it really fixes the problem. Two points:
> > > > 
> > > >  1. Coupling them is bad design, and will probably strike back, as we plan
> > > >     for a better API for events where events can be disabled
> > > 
> > > I meant we may in the future, for example, introduce the ability to disable
> > > commands (and events). One could argue that the event w/o a query command
> > > is not that useful, as events can be lost. But loosing an event is one thing,
> > > not having it because it got disabled by a side effect is another.
> > 
> > event_throttle() couples the event in QMP framework, but we use flags
> > to disabled the event from real source (emit points/senders). 
> > 
> > If we set the evstate->rate to -1, we can ignore the events in
> > monitor_protocol_event_queue(), but we could not control the event
> > emitting of each emit point (each nic).
> >  
> > > But anyway, my main point in this thread is to make sure we at least
> > > justify having this coupling. Aren't we optimizing prematurely? Aren't
> > > we optimizing for a corner case? That's what I want to see answered.
> > 
> > If it's a corner case, we don't need a general API to disable event.
> 
> If it's a corner case, it's really worth to fix it?
> 
> I think that what we need a real world test-case to show us we're
> doing the right thing.
> 
> > We can disable this event by a flag, and introduce a new API
> > if we have same request from other events.
> > 
> > > >  2. Can you actually show the problem does exist so that we ensure this is
> > > >     not premature optimization? Might be a good idea to have this in the
> > > >     commit log
> > > > 
> > > > > > (which is to couple the event with the query command) is
> > > > > > appropriate. We're in user-space already, many things could slow
> > > > > > the guest down apart from the event generation.
> > > > > > 
> > > > > > Two questions:
> > > > > > 
> > > > > >  1. Do we know how slow (or how many packets are actually dropped)
> > > > > >     if the mac is changed too often *and* the event is always sent?
> > > > > 
> > > > > We always disable interface first, then change the macaddr.
> > > > > But we just have patch to allow guest to change macaddr of
> > > > > virtio-net when the interface is running.
> > > > > 
> > > > > | commit 2dcd0cce551983afe2f900125457f10bb5d980ae
> > > > > | Author: Jiri Pirko <jpirko@redhat.com>
> > > > > | Date:   Tue Dec 11 15:33:56 2012 -0500
> > > > > | 
> > > > > |     [virt] virtio_net: allow to change mac when iface is running
> > > > > 
> > > > > >  2. Does this solution consider what happens if the QMP client does
> > > > > >     respond timely to the event by issuing the query-rx-filter
> > > > > >     command?
> > > > > 
> > > > > We assume that the QMP client (management) cares about the mac changing
> > > > > event, and will query the latest rx-filter state and sync to macvtap
> > > > > device.
> > > > > 
> > > > > 1) If QMP client respond timely to the event: that's what we expected :)
> > > > 
> > > > Won't this slow down the guest? If not, why?
> > 
> > If guest changes fx-filter configs frequently & management always query the
> > event very timely, this will slow down the guest.
> > 
> > We should detect & process the abnormal behavior from management.
> 
> That's not abnormal. Management is doing what it should do.
> 
> Maybe using the event throttle API can solve the mngt side of the problem,
> but I still think we need a reproducible test-case to ensure we're doing
> the right thing.


The event is triggered by guest, it will change setup of host device.
and we expect the event to be processed ASAP, event delay / lose might
caused unknown problem.

If management fails to setup rx-filter of macvtap device, the error
should be processed:

  1) when qemu changes rx-filter, send event to management, wait until
     management finishs the setup of macvtap device, then return the
     result to guest kernel. the delay is unacceptabled.

  2) qemu changes rx-filter and return to guest kernel, when
     management finishs the setup to macvtap, then tell the result to
     qemu, qemu tells result to guest kernel.
     what's should guest do if setup macvtap doesn't success?

  3) guest changes rx-filter very frequently, management has to query
     it to sync the rx-filter to macvtap.
     I mentioned a solution for management to avoid the security
     issue. but we could not assume management always be believable.


> > Management (qmp client) always respond timely to the event in the
> > begining. If guest changes rx-filter very frequently & continuous.
> > Then we increase the evstate->rate, even disable the event.
> > 
> > In the normal usecase, we should consider packet losing first (caused by
> > event delay + the delay is used by management to execute the change)
> > 
> > ---
> > btw, currently we could not test in real environment. If related
> > libvirt work finishes, we can evaluate with real delays, packet
> > losing, etc.
> > 
> > The worst condition is we could not accept the delay(packet losing),
> > we need to consider other solution for mac programming of macvtap.
> > 
> > > > > 2) If QMP client doesn't respond timely to the event: packets might drop.
> > > > >    If we change mac when the interface is running, we can accept trivial
> > > > >    packets dropping.
> > > > > 
> > > > > For second condition, we need to test in real environment when libvirt
> > > > > finishs the work of processing events.

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

On Tue, May 28, 2013 at 08:25:56AM -0400, Luiz Capitulino wrote:
> On Tue, 28 May 2013 06:43:04 +0800
> Amos Kong <akong@redhat.com> wrote:

CC: netdev, vlad

Currently we create & open macvtap device by libvirt(management),
and pass the fd to qemu process. And macvtap works in promiscuous
mode, we want to sync the rx-filter setup of virtio-net to macvtap
device for better performance.

qemu might be a non-privileged process, it doesn't have permission
to setup macvtap device. So we want to add an QMP event to notify
management to execute the setup.

mac-programming over macvtap patch for qemu:
http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg03337.html


Can we re-consider to setup macvtap in qemu directly? open some setup
permission of rx-filter to qemu process? will do some investigation.
 

> > On Mon, May 27, 2013 at 09:24:28AM -0400, Luiz Capitulino wrote:
> > > On Mon, 27 May 2013 09:10:11 -0400
> > > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > > 
> > > > > We use the QMP event to notify management about the mac changing.
> > > > > 
> > > > > In this thread, we _wrongly_ considered to use qmp approach to delay
> > > > > the event for avoiding the flooding.
> > > > > 
> > > > >   eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);
> > > > > 
> > > > > Now we have a solution (using a flag to turn on/off the notify) to avoid the
> > > > > flooding, only emit the event if we have no un-read event.
> > > > > 
> > > > > If we want to (flag is on) emit the event, we wish the event be sent ASAP
> > > > > (so event_throttle isn't needed).
> > > > 
> > > > Unfortunately this doesn't answer my question. I did understand why you're
> > > > not using the event throttle API (which is because you don't want to slow down
> > > > the guest, not the QMP client).
> > > > 
> > > > My point is whether coupling the event with the query command is really
> > > > justified or even if it really fixes the problem. Two points:
> > > > 
> > > >  1. Coupling them is bad design, and will probably strike back, as we plan
> > > >     for a better API for events where events can be disabled
> > > 
> > > I meant we may in the future, for example, introduce the ability to disable
> > > commands (and events). One could argue that the event w/o a query command
> > > is not that useful, as events can be lost. But loosing an event is one thing,
> > > not having it because it got disabled by a side effect is another.
> > 
> > event_throttle() couples the event in QMP framework, but we use flags
> > to disabled the event from real source (emit points/senders). 
> > 
> > If we set the evstate->rate to -1, we can ignore the events in
> > monitor_protocol_event_queue(), but we could not control the event
> > emitting of each emit point (each nic).
> >  
> > > But anyway, my main point in this thread is to make sure we at least
> > > justify having this coupling. Aren't we optimizing prematurely? Aren't
> > > we optimizing for a corner case? That's what I want to see answered.
> > 
> > If it's a corner case, we don't need a general API to disable event.
> 
> If it's a corner case, it's really worth to fix it?
> 
> I think that what we need a real world test-case to show us we're
> doing the right thing.
> 
> > We can disable this event by a flag, and introduce a new API
> > if we have same request from other events.
> > 
> > > >  2. Can you actually show the problem does exist so that we ensure this is
> > > >     not premature optimization? Might be a good idea to have this in the
> > > >     commit log
> > > > 
> > > > > > (which is to couple the event with the query command) is
> > > > > > appropriate. We're in user-space already, many things could slow
> > > > > > the guest down apart from the event generation.
> > > > > > 
> > > > > > Two questions:
> > > > > > 
> > > > > >  1. Do we know how slow (or how many packets are actually dropped)
> > > > > >     if the mac is changed too often *and* the event is always sent?
> > > > > 
> > > > > We always disable interface first, then change the macaddr.
> > > > > But we just have patch to allow guest to change macaddr of
> > > > > virtio-net when the interface is running.
> > > > > 
> > > > > | commit 2dcd0cce551983afe2f900125457f10bb5d980ae
> > > > > | Author: Jiri Pirko <jpirko@redhat.com>
> > > > > | Date:   Tue Dec 11 15:33:56 2012 -0500
> > > > > | 
> > > > > |     [virt] virtio_net: allow to change mac when iface is running
> > > > > 
> > > > > >  2. Does this solution consider what happens if the QMP client does
> > > > > >     respond timely to the event by issuing the query-rx-filter
> > > > > >     command?
> > > > > 
> > > > > We assume that the QMP client (management) cares about the mac changing
> > > > > event, and will query the latest rx-filter state and sync to macvtap
> > > > > device.
> > > > > 
> > > > > 1) If QMP client respond timely to the event: that's what we expected :)
> > > > 
> > > > Won't this slow down the guest? If not, why?
> > 
> > If guest changes fx-filter configs frequently & management always query the
> > event very timely, this will slow down the guest.
> > 
> > We should detect & process the abnormal behavior from management.
> 
> That's not abnormal. Management is doing what it should do.
> 
> Maybe using the event throttle API can solve the mngt side of the problem,
> but I still think we need a reproducible test-case to ensure we're doing
> the right thing.


The event is triggered by guest, it will change setup of host device.
and we expect the event to be processed ASAP, event delay / lose might
caused unknown problem.

If management fails to setup rx-filter of macvtap device, the error
should be processed:

  1) when qemu changes rx-filter, send event to management, wait until
     management finishs the setup of macvtap device, then return the
     result to guest kernel. the delay is unacceptabled.

  2) qemu changes rx-filter and return to guest kernel, when
     management finishs the setup to macvtap, then tell the result to
     qemu, qemu tells result to guest kernel.
     what's should guest do if setup macvtap doesn't success?

  3) guest changes rx-filter very frequently, management has to query
     it to sync the rx-filter to macvtap.
     I mentioned a solution for management to avoid the security
     issue. but we could not assume management always be believable.


> > Management (qmp client) always respond timely to the event in the
> > begining. If guest changes rx-filter very frequently & continuous.
> > Then we increase the evstate->rate, even disable the event.
> > 
> > In the normal usecase, we should consider packet losing first (caused by
> > event delay + the delay is used by management to execute the change)
> > 
> > ---
> > btw, currently we could not test in real environment. If related
> > libvirt work finishes, we can evaluate with real delays, packet
> > losing, etc.
> > 
> > The worst condition is we could not accept the delay(packet losing),
> > we need to consider other solution for mac programming of macvtap.
> > 
> > > > > 2) If QMP client doesn't respond timely to the event: packets might drop.
> > > > >    If we change mac when the interface is running, we can accept trivial
> > > > >    packets dropping.
> > > > > 
> > > > > For second condition, we need to test in real environment when libvirt
> > > > > finishs the work of processing events.

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Tue, May 28, 2013 at 08:25:56AM -0400, Luiz Capitulino wrote:
> On Tue, 28 May 2013 06:43:04 +0800
> Amos Kong <akong@redhat.com> wrote:
> 
> > On Mon, May 27, 2013 at 09:24:28AM -0400, Luiz Capitulino wrote:
> > > On Mon, 27 May 2013 09:10:11 -0400
> > > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > > 
> > > > > We use the QMP event to notify management about the mac changing.
> > > > > 
> > > > > In this thread, we _wrongly_ considered to use qmp approach to delay
> > > > > the event for avoiding the flooding.
> > > > > 
> > > > >   eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);
> > > > > 
> > > > > Now we have a solution (using a flag to turn on/off the notify) to avoid the
> > > > > flooding, only emit the event if we have no un-read event.
> > > > > 
> > > > > If we want to (flag is on) emit the event, we wish the event be sent ASAP
> > > > > (so event_throttle isn't needed).
> > > > 
> > > > Unfortunately this doesn't answer my question. I did understand why you're
> > > > not using the event throttle API (which is because you don't want to slow down
> > > > the guest, not the QMP client).
> > > > 
> > > > My point is whether coupling the event with the query command is really
> > > > justified or even if it really fixes the problem. Two points:
> > > > 
> > > >  1. Coupling them is bad design, and will probably strike back, as we plan
> > > >     for a better API for events where events can be disabled
> > > 
> > > I meant we may in the future, for example, introduce the ability to disable
> > > commands (and events). One could argue that the event w/o a query command
> > > is not that useful, as events can be lost. But loosing an event is one thing,
> > > not having it because it got disabled by a side effect is another.
> > 
> > event_throttle() couples the event in QMP framework, but we use flags
> > to disabled the event from real source (emit points/senders). 
> > 
> > If we set the evstate->rate to -1, we can ignore the events in
> > monitor_protocol_event_queue(), but we could not control the event
> > emitting of each emit point (each nic).
> >  
> > > But anyway, my main point in this thread is to make sure we at least
> > > justify having this coupling. Aren't we optimizing prematurely? Aren't
> > > we optimizing for a corner case? That's what I want to see answered.
> > 
> > If it's a corner case, we don't need a general API to disable event.
> 
> If it's a corner case, it's really worth to fix it?
> 
> I think that what we need a real world test-case to show us we're
> doing the right thing.
> 
> > We can disable this event by a flag, and introduce a new API
> > if we have same request from other events.
> > 
> > > >  2. Can you actually show the problem does exist so that we ensure this is
> > > >     not premature optimization? Might be a good idea to have this in the
> > > >     commit log
> > > > 
> > > > > > (which is to couple the event with the query command) is
> > > > > > appropriate. We're in user-space already, many things could slow
> > > > > > the guest down apart from the event generation.
> > > > > > 
> > > > > > Two questions:
> > > > > > 
> > > > > >  1. Do we know how slow (or how many packets are actually dropped)
> > > > > >     if the mac is changed too often *and* the event is always sent?
> > > > > 
> > > > > We always disable interface first, then change the macaddr.
> > > > > But we just have patch to allow guest to change macaddr of
> > > > > virtio-net when the interface is running.
> > > > > 
> > > > > | commit 2dcd0cce551983afe2f900125457f10bb5d980ae
> > > > > | Author: Jiri Pirko <jpirko@redhat.com>
> > > > > | Date:   Tue Dec 11 15:33:56 2012 -0500
> > > > > | 
> > > > > |     [virt] virtio_net: allow to change mac when iface is running
> > > > > 
> > > > > >  2. Does this solution consider what happens if the QMP client does
> > > > > >     respond timely to the event by issuing the query-rx-filter
> > > > > >     command?
> > > > > 
> > > > > We assume that the QMP client (management) cares about the mac changing
> > > > > event, and will query the latest rx-filter state and sync to macvtap
> > > > > device.
> > > > > 
> > > > > 1) If QMP client respond timely to the event: that's what we expected :)
> > > > 
> > > > Won't this slow down the guest? If not, why?
> > 
> > If guest changes fx-filter configs frequently & management always query the
> > event very timely, this will slow down the guest.
> > 
> > We should detect & process the abnormal behavior from management.
> 
> That's not abnormal. Management is doing what it should do.
> 
> Maybe using the event throttle API can solve the mngt side of the problem,
> but I still think we need a reproducible test-case to ensure we're doing
> the right thing.

I agree we should make sure this code is tested.
It's pretty easy: run ifconfig in a loop in guest.

Amos, did you try this? Probably should otherwise
we don't really know whether the logic works.


> > Management (qmp client) always respond timely to the event in the
> > begining. If guest changes rx-filter very frequently & continuous.
> > Then we increase the evstate->rate, even disable the event.
> > 
> > In the normal usecase, we should consider packet losing first (caused by
> > event delay + the delay is used by management to execute the change)
> > 
> > ---
> > btw, currently we could not test in real environment. If related
> > libvirt work finishes, we can evaluate with real delays, packet
> > losing, etc.
> > 
> > The worst condition is we could not accept the delay(packet losing),
> > we need to consider other solution for mac programming of macvtap.
> > 
> > > > > 2) If QMP client doesn't respond timely to the event: packets might drop.
> > > > >    If we change mac when the interface is running, we can accept trivial
> > > > >    packets dropping.
> > > > > 
> > > > > For second condition, we need to test in real environment when libvirt
> > > > > finishs the work of processing events.
> > 

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 30, 2013 at 09:50:35PM +0800, Amos Kong wrote:
> On Tue, May 28, 2013 at 08:25:56AM -0400, Luiz Capitulino wrote:
> > On Tue, 28 May 2013 06:43:04 +0800
> > Amos Kong <akong@redhat.com> wrote:
> 
> CC: netdev, vlad
> 
> Currently we create & open macvtap device by libvirt(management),
> and pass the fd to qemu process. And macvtap works in promiscuous
> mode, we want to sync the rx-filter setup of virtio-net to macvtap
> device for better performance.
> 
> qemu might be a non-privileged process, it doesn't have permission
> to setup macvtap device. So we want to add an QMP event to notify
> management to execute the setup.
> 
> mac-programming over macvtap patch for qemu:
> http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg03337.html
> 
> 
> Can we re-consider to setup macvtap in qemu directly? open some setup
> permission of rx-filter to qemu process? will do some investigation.
>  

I don't think that's a good idea. I expect management to do more
than blindly apply anything qemu tells it to.
For example, check that host admin actually allowed this,
or verify that MAC is unique on this host.

-- 
MST

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Thu, May 30, 2013 at 09:50:35PM +0800, Amos Kong wrote:
> On Tue, May 28, 2013 at 08:25:56AM -0400, Luiz Capitulino wrote:
> > On Tue, 28 May 2013 06:43:04 +0800
> > Amos Kong <akong@redhat.com> wrote:
> 
> CC: netdev, vlad
> 
> Currently we create & open macvtap device by libvirt(management),
> and pass the fd to qemu process. And macvtap works in promiscuous
> mode, we want to sync the rx-filter setup of virtio-net to macvtap
> device for better performance.
> 
> qemu might be a non-privileged process, it doesn't have permission
> to setup macvtap device. So we want to add an QMP event to notify
> management to execute the setup.
> 
> mac-programming over macvtap patch for qemu:
> http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg03337.html
> 
> 
> Can we re-consider to setup macvtap in qemu directly? open some setup
> permission of rx-filter to qemu process? will do some investigation.
>  

I don't think that's a good idea. I expect management to do more
than blindly apply anything qemu tells it to.
For example, check that host admin actually allowed this,
or verify that MAC is unique on this host.

-- 
MST

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

On Thu, May 30, 2013 at 04:54:41PM +0300, Michael S. Tsirkin wrote:
> On Tue, May 28, 2013 at 08:25:56AM -0400, Luiz Capitulino wrote:
> > On Tue, 28 May 2013 06:43:04 +0800
> > Amos Kong <akong@redhat.com> wrote:
> > 
> > > On Mon, May 27, 2013 at 09:24:28AM -0400, Luiz Capitulino wrote:
> > > > On Mon, 27 May 2013 09:10:11 -0400
> > > > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > > > 
> > > > > > We use the QMP event to notify management about the mac changing.
> > > > > > 
> > > > > > In this thread, we _wrongly_ considered to use qmp approach to delay
> > > > > > the event for avoiding the flooding.
> > > > > > 
> > > > > >   eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);
> > > > > > 
> > > > > > Now we have a solution (using a flag to turn on/off the notify) to avoid the
> > > > > > flooding, only emit the event if we have no un-read event.
> > > > > > 
> > > > > > If we want to (flag is on) emit the event, we wish the event be sent ASAP
> > > > > > (so event_throttle isn't needed).
> > > > > 
> > > > > Unfortunately this doesn't answer my question. I did understand why you're
> > > > > not using the event throttle API (which is because you don't want to slow down
> > > > > the guest, not the QMP client).
> > > > > 
> > > > > My point is whether coupling the event with the query command is really
> > > > > justified or even if it really fixes the problem. Two points:
> > > > > 
> > > > >  1. Coupling them is bad design, and will probably strike back, as we plan
> > > > >     for a better API for events where events can be disabled
> > > > 
> > > > I meant we may in the future, for example, introduce the ability to disable
> > > > commands (and events). One could argue that the event w/o a query command
> > > > is not that useful, as events can be lost. But loosing an event is one thing,
> > > > not having it because it got disabled by a side effect is another.
> > > 
> > > event_throttle() couples the event in QMP framework, but we use flags
> > > to disabled the event from real source (emit points/senders). 
> > > 
> > > If we set the evstate->rate to -1, we can ignore the events in
> > > monitor_protocol_event_queue(), but we could not control the event
> > > emitting of each emit point (each nic).
> > >  
> > > > But anyway, my main point in this thread is to make sure we at least
> > > > justify having this coupling. Aren't we optimizing prematurely? Aren't
> > > > we optimizing for a corner case? That's what I want to see answered.
> > > 
> > > If it's a corner case, we don't need a general API to disable event.
> > 
> > If it's a corner case, it's really worth to fix it?
> > 
> > I think that what we need a real world test-case to show us we're
> > doing the right thing.
> > 
> > > We can disable this event by a flag, and introduce a new API
> > > if we have same request from other events.
> > > 
> > > > >  2. Can you actually show the problem does exist so that we ensure this is
> > > > >     not premature optimization? Might be a good idea to have this in the
> > > > >     commit log
> > > > > 
> > > > > > > (which is to couple the event with the query command) is
> > > > > > > appropriate. We're in user-space already, many things could slow
> > > > > > > the guest down apart from the event generation.
> > > > > > > 
> > > > > > > Two questions:
> > > > > > > 
> > > > > > >  1. Do we know how slow (or how many packets are actually dropped)
> > > > > > >     if the mac is changed too often *and* the event is always sent?
> > > > > > 
> > > > > > We always disable interface first, then change the macaddr.
> > > > > > But we just have patch to allow guest to change macaddr of
> > > > > > virtio-net when the interface is running.
> > > > > > 
> > > > > > | commit 2dcd0cce551983afe2f900125457f10bb5d980ae
> > > > > > | Author: Jiri Pirko <jpirko@redhat.com>
> > > > > > | Date:   Tue Dec 11 15:33:56 2012 -0500
> > > > > > | 
> > > > > > |     [virt] virtio_net: allow to change mac when iface is running
> > > > > > 
> > > > > > >  2. Does this solution consider what happens if the QMP client does
> > > > > > >     respond timely to the event by issuing the query-rx-filter
> > > > > > >     command?
> > > > > > 
> > > > > > We assume that the QMP client (management) cares about the mac changing
> > > > > > event, and will query the latest rx-filter state and sync to macvtap
> > > > > > device.
> > > > > > 
> > > > > > 1) If QMP client respond timely to the event: that's what we expected :)
> > > > > 
> > > > > Won't this slow down the guest? If not, why?
> > > 
> > > If guest changes fx-filter configs frequently & management always query the
> > > event very timely, this will slow down the guest.
> > > 
> > > We should detect & process the abnormal behavior from management.
> > 
> > That's not abnormal. Management is doing what it should do.
> > 
> > Maybe using the event throttle API can solve the mngt side of the problem,
> > but I still think we need a reproducible test-case to ensure we're doing
> > the right thing.
> 
> I agree we should make sure this code is tested.
> It's pretty easy: run ifconfig in a loop in guest.
> 
> Amos, did you try this? Probably should otherwise
> we don't really know whether the logic works.

With v4 patch (without using event throttle)

1. continually query rx-filter from monitor
# while true; do echo "info rx-filter" | nc -U /tmp/m; done

2. change mac in guest repeatedly
# while true; do
 ifconfig eth1 down; ifconfig eth1 hw ether 12:00:00:00:00:00
 ifconfig eth1 down; ifconfig eth1 hw ether 14:00:00:00:00:00
 done

One time (down if, change mac, up) takes about 3,500,000 ns in guest
some event will be ignored by qemu. I will try to only query when it
gets NIC_RX_FILTER_CHANGE event from QMP monitor, query ASAP.

==> Resource usage:

  PID USER      PR  NI  VIRT  RES  SHR S  %CPU %MEM    TIME+  COMMAND
16387 root      20   0 2375m 326m 6684 R 104.2  4.2   8:32.16 qemu-system-x86

loop script takes about 10% guest cpu (1 core), guest is not slow

----
If we don't use the flag (same effect as that management taks 0 ns to
response & complete the query after event comming)

  PID USER      PR  NI  VIRT  RES  SHR S  %CPU %MEM    TIME+  COMMAND                                                                                                            
 4317 root      20   0 2377m 285m 6664 R 103.4  3.7   2:06.82 qemu-system-x86

guest is very slow (no response for the keyboard input), output:
clocksource tsc unstable.
 
> > > Management (qmp client) always respond timely to the event in the
> > > begining. If guest changes rx-filter very frequently & continuous.
> > > Then we increase the evstate->rate, even disable the event.
> > > 
> > > In the normal usecase, we should consider packet losing first (caused by
> > > event delay + the delay is used by management to execute the change)
> > > 
> > > ---
> > > btw, currently we could not test in real environment. If related
> > > libvirt work finishes, we can evaluate with real delays, packet
> > > losing, etc.
> > > 
> > > The worst condition is we could not accept the delay(packet losing),
> > > we need to consider other solution for mac programming of macvtap.
> > > 
> > > > > > 2) If QMP client doesn't respond timely to the event: packets might drop.
> > > > > >    If we change mac when the interface is running, we can accept trivial
> > > > > >    packets dropping.
> > > > > > 
> > > > > > For second condition, we need to test in real environment when libvirt
> > > > > > finishs the work of processing events.
> > > 

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

On Fri, May 31, 2013 at 08:35:28AM +0800, Amos Kong wrote:
> On Thu, May 30, 2013 at 04:54:41PM +0300, Michael S. Tsirkin wrote:
> > On Tue, May 28, 2013 at 08:25:56AM -0400, Luiz Capitulino wrote:
> > > On Tue, 28 May 2013 06:43:04 +0800
> > > Amos Kong <akong@redhat.com> wrote:
> > > 
> > > > On Mon, May 27, 2013 at 09:24:28AM -0400, Luiz Capitulino wrote:
> > > > > On Mon, 27 May 2013 09:10:11 -0400
> > > > > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > > > > 
> > > > > > > We use the QMP event to notify management about the mac changing.
> > > > > > > 
> > > > > > > In this thread, we _wrongly_ considered to use qmp approach to delay
> > > > > > > the event for avoiding the flooding.
> > > > > > > 
> > > > > > >   eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);
> > > > > > > 
> > > > > > > Now we have a solution (using a flag to turn on/off the notify) to avoid the
> > > > > > > flooding, only emit the event if we have no un-read event.
> > > > > > > 
> > > > > > > If we want to (flag is on) emit the event, we wish the event be sent ASAP
> > > > > > > (so event_throttle isn't needed).
> > > > > > 
> > > > > > Unfortunately this doesn't answer my question. I did understand why you're
> > > > > > not using the event throttle API (which is because you don't want to slow down
> > > > > > the guest, not the QMP client).
> > > > > > 
> > > > > > My point is whether coupling the event with the query command is really
> > > > > > justified or even if it really fixes the problem. Two points:
> > > > > > 
> > > > > >  1. Coupling them is bad design, and will probably strike back, as we plan
> > > > > >     for a better API for events where events can be disabled
> > > > > 
> > > > > I meant we may in the future, for example, introduce the ability to disable
> > > > > commands (and events). One could argue that the event w/o a query command
> > > > > is not that useful, as events can be lost. But loosing an event is one thing,
> > > > > not having it because it got disabled by a side effect is another.
> > > > 
> > > > event_throttle() couples the event in QMP framework, but we use flags
> > > > to disabled the event from real source (emit points/senders). 
> > > > 
> > > > If we set the evstate->rate to -1, we can ignore the events in
> > > > monitor_protocol_event_queue(), but we could not control the event
> > > > emitting of each emit point (each nic).
> > > >  
> > > > > But anyway, my main point in this thread is to make sure we at least
> > > > > justify having this coupling. Aren't we optimizing prematurely? Aren't
> > > > > we optimizing for a corner case? That's what I want to see answered.
> > > > 
> > > > If it's a corner case, we don't need a general API to disable event.
> > > 
> > > If it's a corner case, it's really worth to fix it?
> > > 
> > > I think that what we need a real world test-case to show us we're
> > > doing the right thing.
> > > 
> > > > We can disable this event by a flag, and introduce a new API
> > > > if we have same request from other events.
> > > > 
> > > > > >  2. Can you actually show the problem does exist so that we ensure this is
> > > > > >     not premature optimization? Might be a good idea to have this in the
> > > > > >     commit log
> > > > > > 
> > > > > > > > (which is to couple the event with the query command) is
> > > > > > > > appropriate. We're in user-space already, many things could slow
> > > > > > > > the guest down apart from the event generation.
> > > > > > > > 
> > > > > > > > Two questions:
> > > > > > > > 
> > > > > > > >  1. Do we know how slow (or how many packets are actually dropped)
> > > > > > > >     if the mac is changed too often *and* the event is always sent?
> > > > > > > 
> > > > > > > We always disable interface first, then change the macaddr.
> > > > > > > But we just have patch to allow guest to change macaddr of
> > > > > > > virtio-net when the interface is running.
> > > > > > > 
> > > > > > > | commit 2dcd0cce551983afe2f900125457f10bb5d980ae
> > > > > > > | Author: Jiri Pirko <jpirko@redhat.com>
> > > > > > > | Date:   Tue Dec 11 15:33:56 2012 -0500
> > > > > > > | 
> > > > > > > |     [virt] virtio_net: allow to change mac when iface is running
> > > > > > > 
> > > > > > > >  2. Does this solution consider what happens if the QMP client does
> > > > > > > >     respond timely to the event by issuing the query-rx-filter
> > > > > > > >     command?
> > > > > > > 
> > > > > > > We assume that the QMP client (management) cares about the mac changing
> > > > > > > event, and will query the latest rx-filter state and sync to macvtap
> > > > > > > device.
> > > > > > > 
> > > > > > > 1) If QMP client respond timely to the event: that's what we expected :)
> > > > > > 
> > > > > > Won't this slow down the guest? If not, why?
> > > > 
> > > > If guest changes fx-filter configs frequently & management always query the
> > > > event very timely, this will slow down the guest.
> > > > 
> > > > We should detect & process the abnormal behavior from management.
> > > 
> > > That's not abnormal. Management is doing what it should do.
> > > 
> > > Maybe using the event throttle API can solve the mngt side of the problem,
> > > but I still think we need a reproducible test-case to ensure we're doing
> > > the right thing.
> > 
> > I agree we should make sure this code is tested.
> > It's pretty easy: run ifconfig in a loop in guest.
> > 
> > Amos, did you try this? Probably should otherwise
> > we don't really know whether the logic works.
> 
> With v4 patch (without using event throttle)
> 
> 1. continually query rx-filter from monitor
> # while true; do echo "info rx-filter" | nc -U /tmp/m; done
> 
> 2. change mac in guest repeatedly
> # while true; do
>  ifconfig eth1 down; ifconfig eth1 hw ether 12:00:00:00:00:00
>  ifconfig eth1 down; ifconfig eth1 hw ether 14:00:00:00:00:00
>  done
> 
> One time (down if, change mac, up) takes about 3,500,000 ns in guest
> some event will be ignored by qemu. I will try to only query when it
> gets NIC_RX_FILTER_CHANGE event from QMP monitor, query ASAP.
> 
> ==> Resource usage:
> 
>   PID USER      PR  NI  VIRT  RES  SHR S  %CPU %MEM    TIME+  COMMAND
> 16387 root      20   0 2375m 326m 6684 R 104.2  4.2   8:32.16 qemu-system-x86
> 
> loop script takes about 10% guest cpu (1 core), guest is not slow
> 
> ----
> If we don't use the flag (same effect as that management taks 0 ns to
> response & complete the query after event comming)


>   PID USER      PR  NI  VIRT  RES  SHR S  %CPU %MEM    TIME+  COMMAND                                                                                                            
>  4317 root      20   0 2377m 285m 6664 R 103.4  3.7   2:06.82 qemu-system-x86
> 
> guest is very slow (no response for the keyboard input), output:
> clocksource tsc unstable.

^^  The reproduce rate : about 10%

I did the following tests, it seems repeatedly executing vq command
with large data will cause guest hung.

Could not slow guest by repeatedly changing rx-filter (reference test1).



NOTE: Test1,2,3,4 didn't use control flag, emit all the events to monitor.

==> test1 (applied v4)
I tried to test by repeatedly change promisc mode (on/off).
The difference is that there is no data passed when it
executes vq command. Changing main mac will pass 1 macaddr (6 bytes).

Result: not reproduce, events emitted to QMP monitor, everything is fine.

==> test2 (applied v4)
So I tried to test by repeatedly change mac of vlan interface.
it will cause guest update mac-table every time, more data will
be passed. 

Result: reproduced (guest becomes slow, stops emit event)

==> test3 (latest qemu)
execute test2 with latest qemu

Result: guest hung

==> test4 (applied v4, + event_throttle change)
set event_throttle for rxfilter event in monitor.c:monitor_protocol_event_init()
  monitor_protocol_event_throttle(QEVENT_NIC_RX_FILTER_CHANGED, 1000);
and execute test2.

Result: guest script stuck, guest gets slow, no event emitted

| Guest) # strace ifconfig
| ...
| ioctrl(4, SIOCGIFCONF, {
| (stuck...)

---
                        Amos

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

On Fri, May 31, 2013 at 11:02:54AM +0800, Amos Kong wrote:
> On Fri, May 31, 2013 at 08:35:28AM +0800, Amos Kong wrote:
> > On Thu, May 30, 2013 at 04:54:41PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, May 28, 2013 at 08:25:56AM -0400, Luiz Capitulino wrote:


> > > > > > >  2. Can you actually show the problem does exist so that we ensure this is
> > > > > > >     not premature optimization? Might be a good idea to have this in the
> > > > > > >     commit log
> > > > > > > 
> > > > > > > > > (which is to couple the event with the query command) is
> > > > > > > > > appropriate. We're in user-space already, many things could slow
> > > > > > > > > the guest down apart from the event generation.
> > > > > > > > > 
> > > > > > > > > Two questions:
> > > > > > > > > 
> > > > > > > > >  1. Do we know how slow (or how many packets are actually dropped)
> > > > > > > > >     if the mac is changed too often *and* the event is always sent?
> > > > > > > > 
> > > > > > > > We always disable interface first, then change the macaddr.
> > > > > > > > But we just have patch to allow guest to change macaddr of
> > > > > > > > virtio-net when the interface is running.
> > > > > > > > 
> > > > > > > > | commit 2dcd0cce551983afe2f900125457f10bb5d980ae
> > > > > > > > | Author: Jiri Pirko <jpirko@redhat.com>
> > > > > > > > | Date:   Tue Dec 11 15:33:56 2012 -0500
> > > > > > > > | 
> > > > > > > > |     [virt] virtio_net: allow to change mac when iface is running
> > > > > > > > 
> > > > > > > > >  2. Does this solution consider what happens if the QMP client does
> > > > > > > > >     respond timely to the event by issuing the query-rx-filter
> > > > > > > > >     command?
> > > > > > > > 
> > > > > > > > We assume that the QMP client (management) cares about the mac changing
> > > > > > > > event, and will query the latest rx-filter state and sync to macvtap
> > > > > > > > device.
> > > > > > > > 
> > > > > > > > 1) If QMP client respond timely to the event: that's what we expected :)
> > > > > > > 
> > > > > > > Won't this slow down the guest? If not, why?
> > > > > 
> > > > > If guest changes fx-filter configs frequently & management always query the
> > > > > event very timely, this will slow down the guest.
> > > > > 
> > > > > We should detect & process the abnormal behavior from management.
> > > > 
> > > > That's not abnormal. Management is doing what it should do.
> > > > 
> > > > Maybe using the event throttle API can solve the mngt side of the problem,
> > > > but I still think we need a reproducible test-case to ensure we're doing
> > > > the right thing.
> > > 
> > > I agree we should make sure this code is tested.
> > > It's pretty easy: run ifconfig in a loop in guest.
> > > 
> > > Amos, did you try this? Probably should otherwise
> > > we don't really know whether the logic works.
> > 
> > With v4 patch (without using event throttle)
> > 
> > 1. continually query rx-filter from monitor
> > # while true; do echo "info rx-filter" | nc -U /tmp/m; done
> > 
> > 2. change mac in guest repeatedly
> > # while true; do
> >  ifconfig eth1 down; ifconfig eth1 hw ether 12:00:00:00:00:00
> >  ifconfig eth1 down; ifconfig eth1 hw ether 14:00:00:00:00:00
> >  done
> > 
> > One time (down if, change mac, up) takes about 3,500,000 ns in guest
> > some event will be ignored by qemu. I will try to only query when it
> > gets NIC_RX_FILTER_CHANGE event from QMP monitor, query ASAP.
> > 
> > ==> Resource usage:
> > 
> >   PID USER      PR  NI  VIRT  RES  SHR S  %CPU %MEM    TIME+  COMMAND
> > 16387 root      20   0 2375m 326m 6684 R 104.2  4.2   8:32.16 qemu-system-x86
> > 
> > loop script takes about 10% guest cpu (1 core), guest is not slow
> > 
> > ----
> > If we don't use the flag (same effect as that management taks 0 ns to
> > response & complete the query after event comming)
> 
> 
> >   PID USER      PR  NI  VIRT  RES  SHR S  %CPU %MEM    TIME+  COMMAND                                                                                                            
> >  4317 root      20   0 2377m 285m 6664 R 103.4  3.7   2:06.82 qemu-system-x86
> > 
> > guest is very slow (no response for the keyboard input), output:
> > clocksource tsc unstable.
> 
> ^^  The reproduce rate : about 10%
> 
> I did the following tests, it seems repeatedly executing vq command
> with large data will cause guest hung.
> 
> Could not slow guest by repeatedly changing rx-filter (reference test1).

I did more tests in clear environment, and found that the guest hang/slow
(no response from monitor) is caused by flooding events. I could not
reproduce it with upstream qemu [1]

If I set event_throttle to 1 ~ 1000, the problem doesn't occur.

It's easier to reproduce this problem by changing vlan config,
not because it passes more data with VQ cmd, but it will cause more
events.


In this case, we can set event_throttle to 1 for _RX_FILTER_CHANGED
event to avoid it slows guest. The 1 ms delay should be acceptabled?

And we can continually use that flag to reduce the events.


[1] 6a4e17711442849bf2cc731ccddef5a2a2d92d29 (Sun Apr 14 18:10:28 2013)

> NOTE: Test1,2,3,4 didn't use control flag, emit all the events to monitor.
> 
> ==> test1 (applied v4)
> I tried to test by repeatedly change promisc mode (on/off).
> The difference is that there is no data passed when it
> executes vq command. Changing main mac will pass 1 macaddr (6 bytes).
> 
> Result: not reproduce, events emitted to QMP monitor, everything is fine.
> 
> ==> test2 (applied v4)
> So I tried to test by repeatedly change mac of vlan interface.
> it will cause guest update mac-table every time, more data will
> be passed. 
> 
> Result: reproduced (guest becomes slow, stops emit event)
> 
> ==> test3 (latest qemu)
> execute test2 with latest qemu
> 
> Result: guest hung

could not reproduce now with [1]

> ==> test4 (applied v4, + event_throttle change)
> set event_throttle for rxfilter event in monitor.c:monitor_protocol_event_init()
>   monitor_protocol_event_throttle(QEVENT_NIC_RX_FILTER_CHANGED, 1000);
> and execute test2.
> 
> Result: guest script stuck, guest gets slow, no event emitted

could not reproduce now with [1]
 
> | Guest) # strace ifconfig
> | ...
> | ioctrl(4, SIOCGIFCONF, {
> | (stuck...)

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Amos Kong]

On Tue, Jun 04, 2013 at 02:43:11PM +0800, Amos Kong wrote:
> 


> I did more tests in clear environment, and found that the guest hang/slow
> (no response from monitor) is caused by flooding events. I could not
> reproduce it with upstream qemu [1]
> 
> If I set event_throttle to 1 ~ 1000, the problem doesn't occur.
> 
> It's easier to reproduce this problem by changing vlan config,
> not because it passes more data with VQ cmd, but it will cause more
> events.
> 
> 
> In this case, we can set event_throttle to 1 for _RX_FILTER_CHANGED
> event to avoid it slows guest. The 1 ms delay should be acceptabled?

Just discussed with mst in IRC.

Here we have two problem:
(1) huge number of events will flood monitor client (management),
(2) emitting huge number of events will slow guest itself.

Both the flag (nc->rxfilter_notify_enabled) and event_throttle API
can be used to avoid problem (1).

Event_throttle API can clearly avoid problem (2).

In real testing, I found it's difficult to reproduce problem (2) if we
already use the flag. It seems response time is larger enough, some
events will be dropped, guest could not be slowed.

Michael told me that we have many ways to slow guest itself, so it's
not a big issue here.

We care about the delay of responsing event, so we should only use
control flag (As my patch v4).

What's your opinion?

Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Tue, Jun 04, 2013 at 03:42:19PM +0800, Amos Kong wrote:
> On Tue, Jun 04, 2013 at 02:43:11PM +0800, Amos Kong wrote:
> > 
> 
> 
> > I did more tests in clear environment, and found that the guest hang/slow
> > (no response from monitor) is caused by flooding events. I could not
> > reproduce it with upstream qemu [1]
> > 
> > If I set event_throttle to 1 ~ 1000, the problem doesn't occur.
> > 
> > It's easier to reproduce this problem by changing vlan config,
> > not because it passes more data with VQ cmd, but it will cause more
> > events.
> > 
> > 
> > In this case, we can set event_throttle to 1 for _RX_FILTER_CHANGED
> > event to avoid it slows guest. The 1 ms delay should be acceptabled?
> 
> Just discussed with mst in IRC.
> 
> Here we have two problem:
> (1) huge number of events will flood monitor client (management),
> (2) emitting huge number of events will slow guest itself.
> 
> Both the flag (nc->rxfilter_notify_enabled) and event_throttle API
> can be used to avoid problem (1).
> 
> Event_throttle API can clearly avoid problem (2).
> 
> In real testing, I found it's difficult to reproduce problem (2) if we
> already use the flag. It seems response time is larger enough, some
> events will be dropped, guest could not be slowed.
> 
> Michael told me that we have many ways to slow guest itself, so it's
> not a big issue here.
> 
> We care about the delay of responsing event, so we should only use
> control flag (As my patch v4).
> 
> What's your opinion?

Sounds reasonable.
Pls send v3 and we'll discuss :)

Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Amos Kong]

On Wed, May 29, 2013 at 01:31:12PM +0800, Jason Wang wrote:
> On 05/16/2013 07:07 PM, Amos Kong wrote:
> > We want to implement mac programming over macvtap through Libvirt.
> > The previous patch adds QMP event to notify management of mac-table
> > change. This patch adds a monitor command to query rx mode information
> > of mac-tables.
> >
> > (qemu) info mac-table vnet0
> > vnet0:
> >  \ promisc: on
> >  \ allmulti: off
> >  \ alluni: off
> >  \ nomulti: off
> >  \ nouni: off
> >  \ nobcast: off
> >  \ multi_overflow: off
> >  \ uni_overflow: off
> >  \ multicast:
> >     01:00:5e:00:00:01
> >     33:33:00:00:00:01
> >     33:33:ff:12:34:56
> >
> > Signed-off-by: Amos Kong <akong@redhat.com>
> 
> Maybe you also need a command to query the vlan table, or rename the
> command as "info filter" and do it here.

Thanks for your reminder.

Yes, we need to include all filters that are used in receive_filter().
It contains main-mac, rx-mode items(mac-table, promisc, unit/multi/broadcast
flags), vlan-table.

It's not good to return all(128) entries of vlan-table to monitor client,
and management only use QMP to query info, so I will drop HMP command.


Amos.