[libvirt] [PATCH 1/2] vircgroup: Add virCgroupSetupBlkiotune()

Fabiano Fidêncio posted 2 patches 6 years, 11 months ago
There is a newer version of this series
[libvirt] [PATCH 1/2] vircgroup: Add virCgroupSetupBlkiotune()
Posted by Fabiano Fidêncio 6 years, 11 months ago
virCgroupSetupBlkiotune() has been introduced in order to remove the
code duplication present between virLXCCgroupSetupBlkioTune() and
qemuSetupBlkioCgroup().

Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
---
 src/libvirt_private.syms |  1 +
 src/lxc/lxc_cgroup.c     | 49 +------------------------------------------
 src/qemu/qemu_cgroup.c   | 47 +----------------------------------------
 src/util/vircgroup.c     | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/util/vircgroup.h     |  3 +++
 5 files changed, 60 insertions(+), 94 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6001635916..4bec93c786 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1545,6 +1545,7 @@ virCgroupSetMemoryHardLimit;
 virCgroupSetMemorySoftLimit;
 virCgroupSetMemSwapHardLimit;
 virCgroupSetOwner;
+virCgroupSetupBlkiotune;
 virCgroupSupportsCpuBW;
 virCgroupTerminateMachine;
 
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index 8e937ec389..2158c1c12b 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -106,54 +106,7 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
 static int virLXCCgroupSetupBlkioTune(virDomainDefPtr def,
                                       virCgroupPtr cgroup)
 {
-    size_t i;
-
-    if (def->blkio.weight &&
-        virCgroupSetBlkioWeight(cgroup, def->blkio.weight) < 0)
-        return -1;
-
-    if (def->blkio.ndevices) {
-        for (i = 0; i < def->blkio.ndevices; i++) {
-            virBlkioDevicePtr dev = &def->blkio.devices[i];
-
-            if (dev->weight &&
-                (virCgroupSetBlkioDeviceWeight(cgroup, dev->path,
-                                               dev->weight) < 0 ||
-                 virCgroupGetBlkioDeviceWeight(cgroup, dev->path,
-                                               &dev->weight) < 0))
-                return -1;
-
-            if (dev->riops &&
-                (virCgroupSetBlkioDeviceReadIops(cgroup, dev->path,
-                                                 dev->riops) < 0 ||
-                 virCgroupGetBlkioDeviceReadIops(cgroup, dev->path,
-                                                 &dev->riops) < 0))
-                return -1;
-
-            if (dev->wiops &&
-                (virCgroupSetBlkioDeviceWriteIops(cgroup, dev->path,
-                                                  dev->wiops) < 0 ||
-                 virCgroupGetBlkioDeviceWriteIops(cgroup, dev->path,
-                                                  &dev->wiops) < 0))
-                return -1;
-
-            if (dev->rbps &&
-                (virCgroupSetBlkioDeviceReadBps(cgroup, dev->path,
-                                                dev->rbps) < 0 ||
-                 virCgroupGetBlkioDeviceReadBps(cgroup, dev->path,
-                                                &dev->rbps) < 0))
-                return -1;
-
-            if (dev->wbps &&
-                (virCgroupSetBlkioDeviceWriteBps(cgroup, dev->path,
-                                                 dev->wbps) < 0 ||
-                 virCgroupGetBlkioDeviceWriteBps(cgroup, dev->path,
-                                                 &dev->wbps) < 0))
-                return -1;
-        }
-    }
-
-    return 0;
+    return virCgroupSetupBlkiotune(cgroup, &def->blkio);
 }
 
 
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 546a4c8e63..47ee23796c 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -510,7 +510,6 @@ static int
 qemuSetupBlkioCgroup(virDomainObjPtr vm)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
-    size_t i;
 
     if (!virCgroupHasController(priv->cgroup,
                                 VIR_CGROUP_CONTROLLER_BLKIO)) {
@@ -523,51 +522,7 @@ qemuSetupBlkioCgroup(virDomainObjPtr vm)
         }
     }
 
-    if (vm->def->blkio.weight != 0 &&
-        virCgroupSetBlkioWeight(priv->cgroup, vm->def->blkio.weight) < 0)
-        return -1;
-
-    if (vm->def->blkio.ndevices) {
-        for (i = 0; i < vm->def->blkio.ndevices; i++) {
-            virBlkioDevicePtr dev = &vm->def->blkio.devices[i];
-            if (dev->weight &&
-                (virCgroupSetBlkioDeviceWeight(priv->cgroup, dev->path,
-                                               dev->weight) < 0 ||
-                 virCgroupGetBlkioDeviceWeight(priv->cgroup, dev->path,
-                                               &dev->weight) < 0))
-                return -1;
-
-            if (dev->riops &&
-                (virCgroupSetBlkioDeviceReadIops(priv->cgroup, dev->path,
-                                                 dev->riops) < 0 ||
-                 virCgroupGetBlkioDeviceReadIops(priv->cgroup, dev->path,
-                                                 &dev->riops) < 0))
-                return -1;
-
-            if (dev->wiops &&
-                (virCgroupSetBlkioDeviceWriteIops(priv->cgroup, dev->path,
-                                                  dev->wiops) < 0 ||
-                 virCgroupGetBlkioDeviceWriteIops(priv->cgroup, dev->path,
-                                                  &dev->wiops) < 0))
-                return -1;
-
-            if (dev->rbps &&
-                (virCgroupSetBlkioDeviceReadBps(priv->cgroup, dev->path,
-                                                dev->rbps) < 0 ||
-                 virCgroupGetBlkioDeviceReadBps(priv->cgroup, dev->path,
-                                                &dev->rbps) < 0))
-                return -1;
-
-            if (dev->wbps &&
-                (virCgroupSetBlkioDeviceWriteBps(priv->cgroup, dev->path,
-                                                 dev->wbps) < 0 ||
-                 virCgroupGetBlkioDeviceWriteBps(priv->cgroup, dev->path,
-                                                 &dev->wbps) < 0))
-                return -1;
-        }
-    }
-
-    return 0;
+    return virCgroupSetupBlkiotune(priv->cgroup, &vm->def->blkio);
 }
 
 
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 0a31947b0d..4e57f1322f 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -4920,3 +4920,57 @@ virCgroupDelThread(virCgroupPtr cgroup,
 
     return 0;
 }
+
+
+int
+virCgroupSetupBlkiotune(virCgroupPtr cgroup,
+                        virDomainBlkiotunePtr blkio)
+{
+    size_t i;
+
+    if (blkio->weight != 0 &&
+        virCgroupSetBlkioWeight(cgroup, blkio->weight) < 0)
+        return -1;
+
+    if (blkio->ndevices) {
+        for (i = 0; i < blkio->ndevices; i++) {
+            virBlkioDevicePtr dev = &blkio->devices[i];
+            if (dev->weight &&
+                (virCgroupSetBlkioDeviceWeight(cgroup, dev->path,
+                                               dev->weight) < 0 ||
+                 virCgroupGetBlkioDeviceWeight(cgroup, dev->path,
+                                               &dev->weight) < 0))
+                return -1;
+
+            if (dev->riops &&
+                (virCgroupSetBlkioDeviceReadIops(cgroup, dev->path,
+                                                 dev->riops) < 0 ||
+                 virCgroupGetBlkioDeviceReadIops(cgroup, dev->path,
+                                                 &dev->riops) < 0))
+                return -1;
+
+            if (dev->wiops &&
+                (virCgroupSetBlkioDeviceWriteIops(cgroup, dev->path,
+                                                  dev->wiops) < 0 ||
+                 virCgroupGetBlkioDeviceWriteIops(cgroup, dev->path,
+                                                  &dev->wiops) < 0))
+                return -1;
+
+            if (dev->rbps &&
+                (virCgroupSetBlkioDeviceReadBps(cgroup, dev->path,
+                                                dev->rbps) < 0 ||
+                 virCgroupGetBlkioDeviceReadBps(cgroup, dev->path,
+                                                &dev->rbps) < 0))
+                return -1;
+
+            if (dev->wbps &&
+                (virCgroupSetBlkioDeviceWriteBps(cgroup, dev->path,
+                                                 dev->wbps) < 0 ||
+                 virCgroupGetBlkioDeviceWriteBps(cgroup, dev->path,
+                                                 &dev->wbps) < 0))
+                return -1;
+        }
+    }
+
+    return 0;
+}
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index d833927678..bd7e7c6d70 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -27,6 +27,7 @@
 
 # include "virutil.h"
 # include "virbitmap.h"
+# include "conf/domain_conf.h"
 
 struct virCgroup;
 typedef struct virCgroup *virCgroupPtr;
@@ -297,4 +298,6 @@ int virCgroupSetOwner(virCgroupPtr cgroup,
 int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller);
 
 bool virCgroupControllerAvailable(int controller);
+
+int virCgroupSetupBlkiotune(virCgroupPtr cgroup, virDomainBlkiotunePtr blkio);
 #endif /* __VIR_CGROUP_H__ */
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] vircgroup: Add virCgroupSetupBlkiotune()
Posted by Michal Privoznik 6 years, 11 months ago
On 06/03/2018 08:09 PM, Fabiano Fidêncio wrote:
> virCgroupSetupBlkiotune() has been introduced in order to remove the
> code duplication present between virLXCCgroupSetupBlkioTune() and
> qemuSetupBlkioCgroup().
> 
> Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
> ---
>  src/libvirt_private.syms |  1 +
>  src/lxc/lxc_cgroup.c     | 49 +------------------------------------------
>  src/qemu/qemu_cgroup.c   | 47 +----------------------------------------
>  src/util/vircgroup.c     | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/vircgroup.h     |  3 +++
>  5 files changed, 60 insertions(+), 94 deletions(-)
> 


> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
> index d833927678..bd7e7c6d70 100644
> --- a/src/util/vircgroup.h
> +++ b/src/util/vircgroup.h
> @@ -27,6 +27,7 @@
>  
>  # include "virutil.h"
>  # include "virbitmap.h"
> +# include "conf/domain_conf.h"

I don't think we want to do this. The point of this code separation is
to use src/util/ independently of parsing code (even though we break
this rule in two places: src/util/virhostdev.h and
src/util/virclosecallbacks.h).

What we can do is moving virDomainBlkiotunePtr into vircgroup.h (in
which case it will need renaming too).

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] vircgroup: Add virCgroupSetupBlkiotune()
Posted by Fabiano Fidêncio 6 years, 11 months ago
On Tue, Jun 5, 2018 at 4:16 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
> On 06/03/2018 08:09 PM, Fabiano Fidêncio wrote:
>> virCgroupSetupBlkiotune() has been introduced in order to remove the
>> code duplication present between virLXCCgroupSetupBlkioTune() and
>> qemuSetupBlkioCgroup().
>>
>> Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
>> ---
>>  src/libvirt_private.syms |  1 +
>>  src/lxc/lxc_cgroup.c     | 49 +------------------------------------------
>>  src/qemu/qemu_cgroup.c   | 47 +----------------------------------------
>>  src/util/vircgroup.c     | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/util/vircgroup.h     |  3 +++
>>  5 files changed, 60 insertions(+), 94 deletions(-)
>>
>
>
>> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
>> index d833927678..bd7e7c6d70 100644
>> --- a/src/util/vircgroup.h
>> +++ b/src/util/vircgroup.h
>> @@ -27,6 +27,7 @@
>>
>>  # include "virutil.h"
>>  # include "virbitmap.h"
>> +# include "conf/domain_conf.h"
>
> I don't think we want to do this. The point of this code separation is
> to use src/util/ independently of parsing code (even though we break
> this rule in two places: src/util/virhostdev.h and
> src/util/virclosecallbacks.h).

Hmm. Makes sense.

>
> What we can do is moving virDomainBlkiotunePtr into vircgroup.h (in
> which case it will need renaming too).

Right, I'll re-spin the patches soon and submit a v2.
Thanks for the review!

>
> Michal


Best Regards,
-- 
Fabiano Fidêncio

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] vircgroup: Add virCgroupSetupBlkiotune()
Posted by Ján Tomko 6 years, 11 months ago
On Tue, Jun 05, 2018 at 05:23:18PM +0200, Fabiano Fidêncio wrote:
>On Tue, Jun 5, 2018 at 4:16 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
>> On 06/03/2018 08:09 PM, Fabiano Fidêncio wrote:
>>> virCgroupSetupBlkiotune() has been introduced in order to remove the
>>> code duplication present between virLXCCgroupSetupBlkioTune() and
>>> qemuSetupBlkioCgroup().
>>>
>>> Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
>>> ---
>>>  src/libvirt_private.syms |  1 +
>>>  src/lxc/lxc_cgroup.c     | 49 +------------------------------------------
>>>  src/qemu/qemu_cgroup.c   | 47 +----------------------------------------
>>>  src/util/vircgroup.c     | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  src/util/vircgroup.h     |  3 +++
>>>  5 files changed, 60 insertions(+), 94 deletions(-)
>>>
>>
>>
>>> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
>>> index d833927678..bd7e7c6d70 100644
>>> --- a/src/util/vircgroup.h
>>> +++ b/src/util/vircgroup.h
>>> @@ -27,6 +27,7 @@
>>>
>>>  # include "virutil.h"
>>>  # include "virbitmap.h"
>>> +# include "conf/domain_conf.h"
>>
>> I don't think we want to do this. The point of this code separation is
>> to use src/util/ independently of parsing code (even though we break
>> this rule in two places: src/util/virhostdev.h and
>> src/util/virclosecallbacks.h).
>
>Hmm. Makes sense.
>

This is also pointed out by 'make syntax-check':

src/util/vircgroup.h:30:# include "conf/domain_conf.h"
maint.mk: unsafe cross-directory include
make: *** [cfg.mk:786: sc_prohibit_cross_inclusion] Error 1

Perhaps our HACKING page [0] could use a rework to mention the important
stuff (running make check and make syntax-check with cppi installed) and
we can have the detailed coding style rules somewhere else (many of
those are caught by syntax-check anyway).

Jano

[0] https://libvirt.org/hacking.html
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list