Re: [Qemu-devel] [PATCH v6] net: add support of mac-programming over macvtap in QEMU side

[Michael S. Tsirkin]

On Fri, Jun 14, 2013 at 03:45:52PM +0800, Amos Kong wrote:
> Currently macvtap based macvlan device is working in promiscuous
> mode, we want to implement mac-programming over macvtap through
> Libvirt for better performance.
> 
> Design:
>  QEMU notifies Libvirt when rx-filter config is changed in guest,
>  then Libvirt query the rx-filter information by a monitor command,
>  and sync the change to macvtap device. Related rx-filter config
>  of the nic contains main mac, rx-mode items and vlan table.
> 
> This patch adds a QMP event to notify management of rx-filter change,
> and adds a monitor command for management to query rx-filter
> information.
> 
> Test:
>  If we repeatedly add/remove vlan, and change macaddr of vlan
>  interfaces in guest by a loop script.
> 
> Result:
>  The events will flood the QMP client(management), management takes
>  too much resource to process the events.
> 
>  Event_throttle API (set rate to 1 ms) can avoid the events to flood
>  QMP client, but it could cause an unexpected delay (~1ms), guests
>  guests normally expect rx-filter updates immediately.
> 
>  So we use a flag for each nic to avoid events flooding, the event
>  is emitted once until the query command is executed. The flag
>  implementation could not introduce unexpected delay.
> 
> There maybe exist an uncontrollable delay if we let Libvirt do the
> real change, guests normally expect rx-filter updates immediately.
> But it's another separate issue, we can investigate it when the
> work in Libvirt side is done.
> 
> Signed-off-by: Amos Kong <address@hidden>
> ---
> v2: add argument to filter mac-table info of single nic (Stefan)
>     update the document, add event notification
> v3: rename to rx-filter, add main mac, avoid events flooding (MST)
>     fix error process (Stefan), fix qmp interface (Eric)
> v4: process qerror in hmp, cleanup (Luiz)
>     set flag for each device, add device path in event, add
>     helper for g_strdup_printf (MST)
>     fix qmp document (Eric)
> v5: add path in doc, define notify flag to unsigned (Eric)
>     add vlan table (Jason), drop monitor cmd
> v6: return vids list, add test results/analysis to commitlog
> ---
>  QMP/qmp-events.txt        |  17 ++++++++
>  hw/net/virtio-net.c       | 108 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  include/monitor/monitor.h |   1 +
>  include/net/net.h         |   3 ++
>  monitor.c                 |   1 +
>  net/net.c                 |  47 ++++++++++++++++++++
>  qapi-schema.json          |  75 ++++++++++++++++++++++++++++++++
>  qmp-commands.hx           |  63 +++++++++++++++++++++++++++
>  8 files changed, 315 insertions(+)
> 
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index 24e804e9..39b6016 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -172,6 +172,23 @@ Data:
>    },
>    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  
> +NIC_RX_FILTER_CHANGED
> +-----------------
> +
> +The event is emitted once until the query command is executed,
> +the first event will always be emitted.
> +
> +Data:
> +
> +- "name": net client name (json-string)
> +- "path": device path (json-string)
> +
> +{ "event": "NIC_RX_FILTER_CHANGED",
> +  "data": { "name": "vnet0",
> +            "path": "/machine/peripheral/vnet0/virtio-backend" },
> +  "timestamp": { "seconds": 1368697518, "microseconds": 326866 } }
> +}
> +
>  RESET
>  -----
>  
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 1ea9556..4137959 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
>  
> @@ -192,6 +194,100 @@ static void virtio_net_set_link_status(NetClientState 
> *nc)
>      virtio_net_set_status(vdev, vdev->status);
>  }
>  
> +static void rxfilter_notify(NetClientState *nc)
> +{
> +    QObject *event_data;
> +    VirtIONet *n = qemu_get_nic_opaque(nc);
> +
> +    if (nc->rxfilter_notify_enabled) {
> +        event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }",
> +                           n->netclient_name,
> +                           object_get_canonical_path(OBJECT(n->qdev)));
> +        monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data);
> +        qobject_decref(event_data);
> +
> +        /* disable event notification to avoid events flooding */
> +        nc->rxfilter_notify_enabled = 0;
> +    }

I applied this, but then I got worried about two things:

1. rxfilter_notify_enabled is initialized to 0.
   while it's possible to require management to
   do a query first thing when it's started,
   and this is likely what libvirt will do,
   it's also reasonable for it to expect that
   if it is starting qemu and specifying all macs,
   it knows the configuration until there's a change event.

2. same thing after migration: this state is not migrated,
   so we won't get an event. again management can work
   around this, but it seems awkward to require this.

Both issues can be solved by the simple means of initializing
rxfilter_notify_enabled to 1.
So I did just that on top, and applied this in my tree:

Signed-off-by: Michael S. Tsirkin <address@hidden>

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 4137959..936ea21 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1547,6 +1547,8 @@ static int virtio_net_device_init(VirtIODevice *vdev)
 
     n->vlans = g_malloc0(MAX_VLAN >> 3);
 
+    nc->rxfilter_notify_enabled = 1;
+
     n->qdev = qdev;
     register_savevm(qdev, "virtio-net", -1, VIRTIO_NET_VM_VERSION,
                     virtio_net_save, virtio_net_load, n);