[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 2/2] processNicRxFilterChangedEvent: Take appropriate actions for NET_TYPE_NETWORK too



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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]