On 10/6/14, 11:16 AM, "Laine Stump" <laine@xxxxxxxxx> wrote:
>On 10/06/2014 02:07 AM, Martin Kletzander wrote:
>> On Fri, Sep 26, 2014 at 10:52:57AM -0700, Anirban Chakraborty wrote:
>>> V2:
>>> Addressed comments raised in review of V1.
>>> Consolidate calls to virNetDevBandwidthSet.
>>> Clear bandwidth settings when the interface is detached or domain
>>> destroyed.
>>>
>>> V1:
>>> 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>
>>>
>>> Signed-off-by: Anirban Chakraborty <abchak@xxxxxxxxxxx>
>>>
>>> ---
>>> src/lxc/lxc_process.c | 26 +++++++++++++-------------
>>> src/network/bridge_driver.c | 7 ++++---
>>> src/qemu/qemu_command.c | 9 ++++-----
>>> src/qemu/qemu_driver.c | 22 +++++++++++++++++++++-
>>> src/qemu/qemu_hotplug.c | 14 +++++++++++++-
>>> src/util/virnetdevbandwidth.c | 23 ++++++++++++++++++++---
>>> src/util/virnetdevbandwidth.h | 7 ++++---
>>> src/util/virnetdevmacvlan.c | 10 ----------
>>> src/util/virnetdevmacvlan.h | 1 -
>>> tests/virnetdevbandwidthtest.c | 3 ++-
>>> 10 files changed, 81 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>>> index ed30c37..7f7e4ad 100644
>>> --- a/src/lxc/lxc_process.c
>>> +++ b/src/lxc/lxc_process.c
>>> @@ -300,6 +295,7 @@ char
>>> *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
>>> virNetDevBandwidthPtr bw;
>>> virNetDevVPortProfilePtr prof;
>>> virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
>>> + const char *linkdev = virDomainNetGetActualDirectDev(net);
>>>
>>> /* XXX how todo bandwidth controls ?
>>> * Since the 'net-ifname' is about to be moved to a different
>>> @@ -329,16 +325,15 @@ char
>>> *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
>>>
>>> if (virNetDevMacVLanCreateWithVPortProfile(
>>> net->ifname, &net->mac,
>>> - virDomainNetGetActualDirectDev(net),
>>> + linkdev,
>>> virDomainNetGetActualDirectMode(net),
>>> false, def->uuid,
>>> - virDomainNetGetActualVirtPortProfile(net),
>>> + prof,
>>> &res_ifname,
>>> VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
>>> cfg->stateDir,
>>> - virDomainNetGetActualBandwidth(net), 0) < 0)
>>> + 0) < 0)
>>> goto cleanup;
>>> -
>>> ret = res_ifname;
>>>
>>> cleanup:
>>> @@ -368,6 +363,7 @@ static int
>>> virLXCProcessSetupInterfaces(virConnectPtr conn,
>>> int ret = -1;
>>> size_t i;
>>> size_t niface = 0;
>>> + int actualType;
>>>
>>> for (i = 0; i < def->nnets; i++) {
>>> char *veth = NULL;
>>> @@ -381,7 +377,8 @@ static int
>>> virLXCProcessSetupInterfaces(virConnectPtr conn,
>>> if (VIR_EXPAND_N(*veths, *nveths, 1) < 0)
>>> goto cleanup;
>>>
>>> - switch (virDomainNetGetActualType(def->nets[i])) {
>>> + actualType = virDomainNetGetActualType(def->nets[i]);
>>> + switch (actualType) {
>>> case VIR_DOMAIN_NET_TYPE_NETWORK: {
>>> virNetworkPtr network;
>>> char *brname = NULL;
>>> @@ -444,11 +441,14 @@ static int
>>> virLXCProcessSetupInterfaces(virConnectPtr conn,
>>> case VIR_DOMAIN_NET_TYPE_LAST:
>>> virReportError(VIR_ERR_INTERNAL_ERROR,
>>> _("Unsupported network type %s"),
>>> - virDomainNetTypeToString(
>>> - virDomainNetGetActualType(def->nets[i])
>>> - ));
>>> + virDomainNetTypeToString(actualType));
>>> goto cleanup;
>>> }
>>
>> Hunks like these (that do not have any functional impact) could be
>> separated to ease the review.
>
>I second that - sometimes I spend time trying to figure out why a change
>was needed, and when I don't find a reason I assume that I must be
>confused about something...
I¹ll break up the patch into two pieces (one for the refactoring part and
the other to add the support to ethernet type interfaces), which should
ease out these things.
>
>>
>>
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index 979fb13..2e1f821 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -2090,7 +2091,7 @@
>>> networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>>> return 0;
>>>
>>> err5:
>>> - virNetDevBandwidthClear(network->def->bridge);
>>> + virNetDevBandwidthClear(network->def->bridge,
>>> VIR_DOMAIN_NET_TYPE_BRIDGE);
>>
>> You could change the virNetDevBandwidthClear() function to take the
>> device definition as an argument and you wouldn't have to supply
>> additional information for that device.
>
>Actually you can't do that, because functions in the util directory
>cannot #include anything from the conf directory.
>
>For that matter, even what you've done here (using
>VIR_DOMAIN_NET_TYPE_*) is illegal, for the same reason. It looks like
>you're only using the type to decide if you want to return early. Going
>quickly through the places where virNetDevBandwidth(Set|Clear) are
>called, I think in most cases you wouldn't even get there if you didn't
>have the right kind of interface, so you can likely just add in the
>check in the couple of places where it is ambiguous.
Ok, will do that. Thanks for pointing it out.
>
>BTW, I notice that you're allowing setting of bandwidth for macvtap
>interfaces (VIR_DOMAIN_NET_TYPE_DIRECT). This was not originally
>supported by the macvtap code in the kernels, although it has recently
>been added (e.g. it is in Fedora 20, but not in RHEL7.0 or CentOS7.0).
>Was this intentional, or accidental?
It appears to me that the existing code sets bandwidth for
VIR_DOMAIN_NET_TYPE_DIRECT device type. qemuBuildInterfaceCommandLine()
has code to create macvtap device (qemuPhysIfaceConnect) for
VIR_DOMAIN_NET_TYPE_DIRECT. Subsequently, qemuPhysIfaceConnect() calls
virNetDevMacVLanCreateWithVPortProfile(), which calls
virNetDevBandwidthSet() to set the bandwidth.
Anirban
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list