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

2014-09-07 Thread Martin Kletzander 

On Fri, Sep 05, 2014 at 11:42:58PM +, Anirban Chakraborty wrote:



On 9/5/14, 1:31 AM, Martin Kletzander mklet...@redhat.com 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 abc...@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.


Got it. Thanks for the pointer.




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.


If I am not mistaken, currently, the bandwidth setting has been cleared
only for bridged devices (via
networkShutdownNetworkVirtual‹virNetDevBandwidthClear ). Are you saying
that we should explicitly clear the bandwidth settings for unplugging and
shut down events? As far as my understanding, for both of these events,
the tap interface gets deleted, so there is no need to cleanup qdisc
settings of these devices.



It doesn't, actually.  interface type='ethernet'source
dev=tap123/ ... makes use of the device tap123 that already exists
and leaves it in the system.  When I start and stop the domain with
the bandwidth set, it is kept in there.  If I were to start another
domain without any bandwidth setting (or just use the device somehow),
the speed would be affected.

Probably taking Laine's approach would be cleaner and easier (it could
also take care of this automagically).



Looking forward to v2,


I am working on it right now and will send it out soon. Thanks for the
feedback.

Anirban



signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-09-07 Thread Anirban Chakraborty 


On 9/7/14, 2:01 AM, Martin Kletzander mklet...@redhat.com wrote:

On Fri, Sep 05, 2014 at 11:42:58PM +, Anirban Chakraborty wrote:


On 9/5/14, 1:31 AM, Martin Kletzander mklet...@redhat.com wrote:

On Thu, Sep 04, 2014 at 03:02:54PM -0700, Anirban Chakraborty wrote:
snip
It doesn't, actually.  interface type='ethernet'source
dev=tap123/ ... makes use of the device tap123 that already exists
and leaves it in the system.  When I start and stop the domain with
the bandwidth set, it is kept in there.  If I were to start another
domain without any bandwidth setting (or just use the device somehow),
the speed would be affected.

Probably taking Laine's approach would be cleaner and easier (it could
also take care of this automagically).

You are right that tap device is not deleted (as libvirt doesn¹t create it
either) for ethernet type interfaces. However, for all other interface
types, corresponding tap device is deleted. So, if I clear the bandwidth
setting in a generic way it will become redundant for these other
interface types. In any case, I¹ll ensure that the tap device
configurations are cleared out in the unplug and shutdown path.

Anirban



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-09-07 Thread Anirban Chakraborty 


On 9/5/14, 8:52 AM, Laine Stump la...@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 abc...@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-09-05 Thread Martin Kletzander 

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

Looking forward to v2,

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-09-05 Thread 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 abc...@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.)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-09-05 Thread Anirban Chakraborty 


On 9/5/14, 1:31 AM, Martin Kletzander mklet...@redhat.com 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 abc...@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.

Got it. Thanks for the pointer.


 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.

If I am not mistaken, currently, the bandwidth setting has been cleared
only for bridged devices (via
networkShutdownNetworkVirtual‹virNetDevBandwidthClear ). Are you saying
that we should explicitly clear the bandwidth settings for unplugging and
shut down events? As far as my understanding, for both of these events,
the tap interface gets deleted, so there is no need to cleanup qdisc
settings of these devices.


Looking forward to v2,

I am working on it right now and will send it out soon. Thanks for the
feedback.

Anirban


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list