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

[Amos Kong]

On Fri, Jun 07, 2013 at 09:46:12AM -0400, Luiz Capitulino wrote:
> On Wed,  5 Jun 2013 18:42:13 +0800
> Amos Kong <address@hidden> 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.
> > 
> > For reducing length of output, we just return the entries of vlan
> > filter table that have active vlan.
> > 
> > Event_throttle API can avoid the events to flood QMP client, but it
> > could cause an unexpected delay. So a flag for each nic is used to
> > avoid events flooding, if management doesn't query rx-filter after
> > it receives one event, new events won't be emitted to QMP monitor.
> > 
> > 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.
> 
> I think I completely misunderstood your testing results.
> 
> I had understood that: 1. changing the mac often & quick enough to
> be a problem was a corner case and 2. you actually can overflow mngt
> 
> I hope I really got it wrong, otherwise you'll be using that
> flag as a replacement for the event throttle API, which would be
> a big mistake.

It's not replacement, the flag could not cause delay.
 
> Can you please add your test results & analysis to this commit message?

OK.
 
> Two small comments below.
> 
> > 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
> > ---
> >  QMP/qmp-events.txt        |  20 +++++++++
> >  hw/net/virtio-net.c       | 112 
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/monitor/monitor.h |   1 +
> >  include/net/net.h         |   3 ++
> >  monitor.c                 |   1 +
> >  net/net.c                 |  47 +++++++++++++++++++
> >  qapi-schema.json          |  89 ++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx           |  66 +++++++++++++++++++++++++++
> >  8 files changed, 339 insertions(+)
> > 
> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > index 92fe5fb..885230e 100644
> > --- a/QMP/qmp-events.txt
> > +++ b/QMP/qmp-events.txt
> > @@ -172,6 +172,26 @@ Data:
> >    },
> >    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> >  
> > +NIC_RX_FILTER_CHANGED
> > +-----------------
> > +
> > +Emitted when rx-filter configuration of nic is changed by the guest.
> > +Each nic has a flag to control event emit, the flag is set to false
> > +when it emits one event of the nic, the flag is set to true when
> > +management queries the rx-filter of the nic. This is used to avoid
> > +events flooding.
> 
> Having this flag is an implementation detail. I think you should only
> say that the event is emitted once until the query command is executed.

OK.

> > +
> > +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..ae1eab6 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,104 @@ 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;
> > +    }
> > +}
> > +
> > +static char *mac_strdup_printf(uint8_t *mac)

     static char *mac_strdup_printf(const uint8_t *mac)

> > +{
> 
> mac can be const. Is there more code in QEMU that could use this
> function? 

No.

> If there's, then it's better to move this function to a
> more generic .c file.
 
> > +    return g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x", mac[0],
> > +                            mac[1], mac[2], mac[3], mac[4], mac[5]);
> > +}
> > +

-- 
                        Amos.