Re: [libvirt] [PATCH 5/9] qemuDomainSetInterfaceParameters: Use new functions to update bandwidth
On 11.08.2015 03:06, John Ferlan wrote:
On 08/03/2015 02:39 AM, Michal Privoznik wrote:
> As sketched in previous commits, imagine the following scenario:
>
> virsh # domiftune gentoo vnet0
> inbound.average: 100
> inbound.peak : 0
> inbound.burst : 0
> outbound.average: 100
> outbound.peak : 0
> outbound.burst : 0
>
> virsh # domiftune gentoo vnet0 --inbound 0
>
> virsh # shutdown gentoo
> Domain gentoo is being shutdown
>
> virsh # list --all
> error: Failed to list domains
> error: Cannot recv data: Connection reset by peer
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007fffe80ea221 in networkUnplugBandwidth (net=0x7fff9400c1a0,
iface=0x7fff940ea3e0) at network/bridge_driver.c:4881
> 4881 net->floor_sum -= ifaceBand->in->floor;
>
> This is rather unfortunate. We should not SIGSEGV here. The
> problem is, that while in the second step the inbound QoS was
> cleared out, the network part of it was not updated (moreover, we
> don't report that vnet0 had inbound.floor set). Internal
> structure therefore still had some fragments left (e.g.
> class_id). So when qemuProcessStop() started to clean up the
> environment it got to networkUnplugBandwidth(). Here, class_id is
> set therefore function assumes that there is an inbound QoS. This
> actually is a fair assumption to make, there's no need for a
> special QoS box in network's QoS when there's no QoS to set.
> Anyway, the problem is not the networkUnplugBandwidth() rather
> than qemuDomainSetInterfaceParameters() which completely forgot
> about QoS being disperse (some parts are set directly on
> interface itself, some on bridge the interface is plugged into).
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b9278f8..171b58f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -100,6 +100,7 @@
> #include "vircgroup.h"
> #include "virnuma.h"
> #include "dirname.h"
> +#include "network/bridge_driver.h"
>
> #define VIR_FROM_THIS VIR_FROM_QEMU
>
> @@ -11221,7 +11222,11 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
> sizeof(*newBandwidth->out));
> }
>
> - if (virNetDevBandwidthSet(net->ifname, newBandwidth, false) < 0) {
> + if (!networkBandwidthChangeAllowed(net, newBandwidth))
> + goto endjob;
> +
> + if (virNetDevBandwidthSet(net->ifname, newBandwidth, false) < 0 ||
> + networkBandwidthUpdate(net, newBandwidth) < 0) {
Is this the only caller of virNetDevBandwidthSet that needs the
"ChangeAllowed" check first? I think the answer is no based on your
explanation in the commit message, but figured I'd ask... Tough to keep
track of the multiple callers and what they're checking before calling
the Set and/or if they need to Update as well (it's been a long day ;-),
but I figured I'd at least look...
Yes and no. You are right that we should check if new bandwidth can be
fulfilled prior to setting it. And now, with this patch pushed we are
doing that. I admit that it's hard to see since the checks are on a
different places - for instance, in networkAllocateActualDevice() which
calls networkPlugBandwidth() eventually and that one finally calls
networkCheckBandwidth(). Maybe one day I'll pull up my sleeves and
refactor the code (to actually follow the advice I am going to give in
my talk on KVM Forum).
ACK for the change in any case.
John
Michal