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

[Anirban Chakraborty]

On 9/5/14, 8:52 AM, "Laine Stump" <laine at laine.org> wrote:

>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.

Thanks for your suggestions. As a first step, I am creating a patch to
call virNetDevBandwidthSet() and Clear() from a higher level.


>
>(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.)

This sounds great. However, this would require a lot more code changes. I
guess it could be done at a later point in time.

-Anirban