On 04/21/2015 08:22 AM, Michal Privoznik wrote:

In one of my previous patches (b68a56bcfe) I made class_id to format more frequently. Well, now it's formatting way too frequent - even for regular active XML. Users don't need to see it, so lets format it only for the status XML where it's really needed.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 479b4c2..9aad782 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18522,7 +18522,8 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf,

virBufferAddLit(buf, "/>\n"); } - if (def->data.network.actual && def->data.network.actual->class_id) { + if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS && + def->data.network.actual && def->data.network.actual->class_id) { virBufferAsprintf(buf, "<class id='%u'/>\n", def->data.network.actual->class_id); }

ACK.

On 04/21/2015 08:22 AM, Michal Privoznik wrote:

Because packets going through the egress from a bridge (where our bandwidth limiting takes place) have no information about which interface they came from, the QoS rules that we create instead use the source MAC address of the packets to make their decisions about which QDisc the packet should be in.

One flaw in this is that when a guest changed the MAC address it used, packets from the guest would no longer be put into the correct QDisc, but would instead be put in an "unprivileged" class, resulting in the bandwidth "floor" (minimum guaranteed) being no longer honored.

Now that libvirt has infrastructure to capture and respond to RX_FILTER_CHANGE events from qemu (sent whenever a guest interface modifies its MAC address, among other things), we can notice when a guest MAC address changes, and update the QoS rules accordingly, so that bandwidth floor is honored even after a guest MAC address change.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6fc9696..6209754b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4377,6 +4377,18 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver, syncNicRxFilterDeviceOptions(def->ifname, guestFilter, hostFilter); }

+ if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_NETWORK) { + const char *brname = virDomainNetGetActualBridgeName(def); + + /* For libivrt network connections, set the following TUN/TAP network + * device attributes to match those of the guest network device: + * - QoS filters (which are based on MAC address) + */

Don't you want to check to make sure this interface actually has bandwidth settings before trying to update them? Maybe by prepending "if (virDomainNetGetActualBandwidth(def)) to this following if clause.

+ if (virNetDevBandwidthUpdateFilter(brname, &guestFilter->mac, + def->data.network.actual->class_id) < 0)

Just to be defensive, you should check that def->data.network.actual != NULL too (it *should* always point to something when the domain is active, but just in case)

+ goto endjob; + } + endjob: qemuDomainObjEndJob(driver, vm);

ACK with those two changes.