On Fri, Jun 07, 2013 at 09:46:12AM -0400, Luiz Capitulino wrote:
> On Wed, 5 Jun 2013 18:42:13 +0800
> Amos Kong <ak...@redhat.com> 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 <ak...@redhat.com>
> > ---
> > 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.
