[libvirt] [PATCH-RFC] qemu: Add network bandwidth setting for ethernet interfaces

[Laine Stump]

On 09/05/2014 04:31 AM, Martin Kletzander wrote:
> On Thu, Sep 04, 2014 at 03:02:54PM -0700, Anirban Chakraborty wrote:
>> ethernet interfaces in libvirt currently do not support bandwidth
>> setting.
>> For example, following xml file for an interface will not apply these
>> settings to corresponding qdiscs.
>>
>> ----
>>    <interface type="ethernet">
>>      <mac address="02:36:1d:18:2a:e4"/>
>>      <model type="virtio"/>
>>      <script path=""/>
>>      <target dev="tap361d182a-e4"/>
>>      <bandwidth>
>>        <inbound average="984" peak="1024" burst="64"/>
>>        <outbound average="2000" peak="2048" burst="128"/>
>>      </bandwidth>
>>    </interface>
>> -----
>>
>> This patch fixes the behavior.
>
> Although this doesn't confuse git, it might confuse something else.
> Leaving it without '----' is ok.
>
>> Please review it and if it appears ok, please apply.
>>
>> thanks,
>> Anirban Chakraborty
>>
>
> No need to have this in the commit message, it's kept in the log then.
>
>>
>> Signed-off-by: Anirban Chakraborty <abchak at juniper.net>
>> ---
>> src/qemu/qemu_command.c | 5 +++++
>> src/qemu/qemu_hotplug.c | 3 +++
>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 2184caa..258c6a7 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7251,6 +7251,11 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>>         if (tapfd[0] < 0)
>>             goto cleanup;
>>     }
>> +    /* Configure network bandwidth for ethernet type network
>> interfaces */
>> +    if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)
>> +        if (virNetDevBandwidthSet(net->ifname,
>> +            virDomainNetGetActualBandwidth(net), false) < 0)
>> +            goto cleanup;
>>
>
> In libvirt, we indent by spaces.  This would be caught by running
> 'make syntax-check'.  Anyway, running 'make syntax-check check' is a
> good practice to follow before formatting the patches.
>
>>     if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>>          actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index a364c52..aeb53c5 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -940,6 +940,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>>         if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd,
>> &vhostfdSize) < 0)
>>             goto cleanup;
>>     } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
>> +        if (virNetDevBandwidthSet(net->ifname,
>> +                virDomainNetGetActualBandwidth(net), false) < 0)
>> +            goto cleanup;
>
> Same here.
>
> We need to clear the bandwidth settings when shutting down the domain
> or unplugging the device.

Aside from that, I would prefer if we could consolidate to *fewer* calls
to virNetDevBandwidthSet() rather than adding more special case warts on
the code. If the current calls to that function could be moved up in the
call stack from their current low-level locations that are specific to a
particular type of interface, perhaps they could all converge to a
single place. Then, that single place could make the call for all
interface types that supported bandwidth setting, and log an error
message for all interface types that didn't support it.

(as a matter of fact, I would eventually like to get all stuff like this
moved into the equivalent location to networkAllocateActualDevice(), so
that 1) a single call would take care of *everything* for a network
device, regardless of type, and 2) it would be possible to place that
single function behind a public API that could be callable from
session-mode libvirtd to enable unprivileged guests to have access to
all the different types of network connectivity.)