[libvirt] [PATCH 0/2] Share cgroup code that is duplicated between QEMU and LXC
virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup() and virLXCCgroupSetupCpuTune() and qemuSetupCpuCgroup() are the most similar functions between QEMU and LXC code. Let's move their common code to virCgroup. Fabiano Fidêncio (2): vircgroup: Add virCgroupSetupBlkiotune() vircgroup: Add virCgroupSetupMemtune() src/libvirt_private.syms | 2 ++ src/lxc/lxc_cgroup.c | 69 ++------------------------------------------ src/qemu/qemu_cgroup.c | 61 ++------------------------------------- src/util/vircgroup.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/vircgroup.h | 4 +++ 5 files changed, 85 insertions(+), 125 deletions(-) -- 2.14.3
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
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
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
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
virCgroupSetupMemtune() has been introduced in order to remove the code duplication present between virLXCCgroupSetupMemTune() and qemuSetupMemoryCgroup(). Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/libvirt_private.syms | 1 + src/lxc/lxc_cgroup.c | 20 ++------------------ src/qemu/qemu_cgroup.c | 14 +------------- src/util/vircgroup.c | 20 ++++++++++++++++++++ src/util/vircgroup.h | 1 + 5 files changed, 25 insertions(+), 31 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4bec93c786..39fce2e169 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1546,6 +1546,7 @@ virCgroupSetMemorySoftLimit; virCgroupSetMemSwapHardLimit; virCgroupSetOwner; virCgroupSetupBlkiotune; +virCgroupSetupMemtune; virCgroupSupportsCpuBW; virCgroupTerminateMachine; diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 2158c1c12b..a5e71aee53 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -113,26 +113,10 @@ static int virLXCCgroupSetupBlkioTune(virDomainDefPtr def, static int virLXCCgroupSetupMemTune(virDomainDefPtr def, virCgroupPtr cgroup) { - int ret = -1; - if (virCgroupSetMemory(cgroup, virDomainDefGetMemoryInitial(def)) < 0) - goto cleanup; - - if (virMemoryLimitIsSet(def->mem.hard_limit)) - if (virCgroupSetMemoryHardLimit(cgroup, def->mem.hard_limit) < 0) - goto cleanup; - - if (virMemoryLimitIsSet(def->mem.soft_limit)) - if (virCgroupSetMemorySoftLimit(cgroup, def->mem.soft_limit) < 0) - goto cleanup; - - if (virMemoryLimitIsSet(def->mem.swap_hard_limit)) - if (virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return virCgroupSetupMemtune(cgroup, &def->mem); } diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 47ee23796c..7b6a4ec796 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -543,19 +543,7 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm) } } - if (virMemoryLimitIsSet(vm->def->mem.hard_limit)) - if (virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 0) - return -1; - - if (virMemoryLimitIsSet(vm->def->mem.soft_limit)) - if (virCgroupSetMemorySoftLimit(priv->cgroup, vm->def->mem.soft_limit) < 0) - return -1; - - if (virMemoryLimitIsSet(vm->def->mem.swap_hard_limit)) - if (virCgroupSetMemSwapHardLimit(priv->cgroup, vm->def->mem.swap_hard_limit) < 0) - return -1; - - return 0; + return virCgroupSetupMemtune(priv->cgroup, &vm->def->mem); } diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 4e57f1322f..88f213e6da 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -4974,3 +4974,23 @@ virCgroupSetupBlkiotune(virCgroupPtr cgroup, return 0; } + + +int +virCgroupSetupMemtune(virCgroupPtr cgroup, + virDomainMemtunePtr mem) +{ + if (virMemoryLimitIsSet(mem->hard_limit)) + if (virCgroupSetMemoryHardLimit(cgroup, mem->hard_limit) < 0) + return -1; + + if (virMemoryLimitIsSet(mem->soft_limit)) + if (virCgroupSetMemorySoftLimit(cgroup, mem->soft_limit) < 0) + return -1; + + if (virMemoryLimitIsSet(mem->swap_hard_limit)) + if (virCgroupSetMemSwapHardLimit(cgroup, mem->swap_hard_limit) < 0) + return -1; + + return 0; +} diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index bd7e7c6d70..5d7973b272 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -300,4 +300,5 @@ int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller); bool virCgroupControllerAvailable(int controller); int virCgroupSetupBlkiotune(virCgroupPtr cgroup, virDomainBlkiotunePtr blkio); +int virCgroupSetupMemtune(virCgroupPtr cgroup, virDomainMemtunePtr mem); #endif /* __VIR_CGROUP_H__ */ -- 2.14.3
participants (3)
-
Fabiano Fidêncio -
Ján Tomko -
Michal Privoznik