On 9/5/14, 8:52 AM, "Laine Stump" <laine@xxxxxxxxx> 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@xxxxxxxxxxx>
>>> ---
>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list