[libvirt] [libvirt PATCH v3 0/4] 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. Mind that the first two patches are basically preparing the ground for the changes introduced in the last two patches. I know that Pavel Hrdina suggested taking a different path[0] for this series but I do believe Michal's and Jano's points are valid, thus I've decided to just do the changes suggested by Michal (at least for now). [0]: https://www.redhat.com/archives/libvir-list/2018-September/msg00534.html changes since v1: - Michal Privoznik pointed out (as did the `make syntax-check` :-)) that we do want to keep src/util independently of the parsing code (thus, including "conf/domain_conf.h" in vircgroup.h is not the way to go). This has been solved now by partially following Michal's suggestion and splitting the structs and functions that would be use in the common code to new different files. changes since v2: - Fixed `make syntax-check` errors that I've missed before (didn't have cppi installed); - Created a new section in src/libvirt_private.syms to correctly export the virBlkioDeviceArrayClear symbol; - Removed a virlkio.c from libvirt_setuid_rpc_client_la__SOURCES (why did I add it there in the first place? ;-)) - Explicitly included virmem.h in qemu_{domain,command}.c. Although there's nothing enforcing it, I do believe it's a good practive that should be followed and I've checked with Andrea and Peter about doing that and both agreed with the approach. Fabiano Fidêncio (4): domain_conf: split out virBlkioDevice and virDomainBlkiotune definitions domain_conf: split out virDomainMemtune and virDomainHugePage definitions vircgroup: Add virCgroupSetupBlkioTune() vircgroup: Add virCgroupSetupMemTune() src/conf/domain_conf.c | 22 ++++-------- src/conf/domain_conf.h | 70 +++---------------------------------- src/libvirt_private.syms | 7 +++- src/lxc/lxc_cgroup.c | 69 ++----------------------------------- src/qemu/qemu_cgroup.c | 61 ++------------------------------- src/qemu/qemu_command.c | 5 +-- src/qemu/qemu_domain.c | 3 +- src/util/Makefile.inc.am | 2 ++ src/util/virblkio.c | 37 ++++++++++++++++++++ src/util/virblkio.h | 52 ++++++++++++++++++++++++++++ src/util/vircgroup.c | 74 ++++++++++++++++++++++++++++++++++++++++ src/util/vircgroup.h | 5 +++ src/util/virmem.h | 66 +++++++++++++++++++++++++++++++++++ 13 files changed, 263 insertions(+), 210 deletions(-) create mode 100644 src/util/virblkio.c create mode 100644 src/util/virblkio.h create mode 100644 src/util/virmem.h -- 2.17.1 Subject: [libvirt PATCH v2 0/4] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Let's move those to their own newly created files (src/util/virblkio.{c,h}) as this will help us to easily start sharing the cgroup code that's duplicated between QEMU and LXC. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/conf/domain_conf.c | 11 +-------- src/conf/domain_conf.h | 27 ++------------------- src/libvirt_private.syms | 5 +++- src/util/Makefile.inc.am | 2 ++ src/util/virblkio.c | 37 ++++++++++++++++++++++++++++ src/util/virblkio.h | 52 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 98 insertions(+), 36 deletions(-) create mode 100644 src/util/virblkio.c create mode 100644 src/util/virblkio.h diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 409a2291ff..3384a36d76 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -59,6 +59,7 @@ #include "virnetdevmacvlan.h" #include "virhostdev.h" #include "virmdev.h" +#include "virblkio.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -1205,16 +1206,6 @@ virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr xmlopt) } -void -virBlkioDeviceArrayClear(virBlkioDevicePtr devices, - int ndevices) -{ - size_t i; - - for (i = 0; i < ndevices; i++) - VIR_FREE(devices[i].path); -} - /** * virDomainBlkioDeviceParseXML * diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e30a4b2fe7..e9e6b6d6c4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -57,6 +57,7 @@ # include "virtypedparam.h" # include "virsavecookie.h" # include "virresctrl.h" +# include "virblkio.h" /* forward declarations of all device types, required by * virDomainDeviceDef @@ -2084,17 +2085,6 @@ struct _virDomainClockDef { }; -typedef struct _virBlkioDevice virBlkioDevice; -typedef virBlkioDevice *virBlkioDevicePtr; -struct _virBlkioDevice { - char *path; - unsigned int weight; - unsigned int riops; - unsigned int wiops; - unsigned long long rbps; - unsigned long long wbps; -}; - typedef enum { VIR_DOMAIN_RNG_MODEL_VIRTIO, @@ -2184,9 +2174,6 @@ struct _virDomainPanicDef { }; -void virBlkioDeviceArrayClear(virBlkioDevicePtr deviceWeights, - int ndevices); - typedef struct _virDomainResourceDef virDomainResourceDef; typedef virDomainResourceDef *virDomainResourceDefPtr; struct _virDomainResourceDef { @@ -2260,16 +2247,6 @@ struct _virDomainVcpuDef { virObjectPtr privateData; }; -typedef struct _virDomainBlkiotune virDomainBlkiotune; -typedef virDomainBlkiotune *virDomainBlkiotunePtr; - -struct _virDomainBlkiotune { - unsigned int weight; - - size_t ndevices; - virBlkioDevicePtr devices; -}; - typedef struct _virDomainMemtune virDomainMemtune; typedef virDomainMemtune *virDomainMemtunePtr; @@ -2402,7 +2379,7 @@ struct _virDomainDef { char *title; char *description; - virDomainBlkiotune blkio; + virBlkioTune blkio; virDomainMemtune mem; virDomainVcpuDefPtr *vcpus; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a0d229a79f..bf1536ea57 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -190,7 +190,6 @@ virSEVCapabilitiesFree; # conf/domain_conf.h -virBlkioDeviceArrayClear; virDiskNameParse; virDiskNameToBusDeviceIndex; virDiskNameToIndex; @@ -1471,6 +1470,10 @@ virBitmapToDataBuf; virBitmapToString; +# util/virblkio.h +virBlkioDeviceArrayClear; + + # util/virbuffer.h virBufferAdd; virBufferAddBuffer; diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index a22265606c..13f415b23c 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -17,6 +17,8 @@ UTIL_SOURCES = \ util/virauthconfig.h \ util/virbitmap.c \ util/virbitmap.h \ + util/virblkio.c \ + util/virblkio.h \ util/virbuffer.c \ util/virbuffer.h \ util/virperf.c \ diff --git a/src/util/virblkio.c b/src/util/virblkio.c new file mode 100644 index 0000000000..9711077ee8 --- /dev/null +++ b/src/util/virblkio.c @@ -0,0 +1,37 @@ +/* + * virblkio.c: Block IO helpers + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Fabiano Fidêncio <fidencio@redhat.com> + */ + +#include <config.h> + +#include "viralloc.h" +#include "virblkio.h" + +void +virBlkioDeviceArrayClear(virBlkioDevicePtr devices, + int ndevices) +{ + size_t i; + + for (i = 0; i < ndevices; i++) + VIR_FREE(devices[i].path); +} diff --git a/src/util/virblkio.h b/src/util/virblkio.h new file mode 100644 index 0000000000..aa9788636d --- /dev/null +++ b/src/util/virblkio.h @@ -0,0 +1,52 @@ +/* + * virblkio.h: Block IO definitions and helpers + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Fabiano Fidêncio <fidencio@redhat.com> + */ + +#ifndef __VIR_BLKIO_H__ +# define __VIR_BLKIO_H__ + +# include "virutil.h" + +typedef struct _virBlkioDevice virBlkioDevice; +typedef virBlkioDevice *virBlkioDevicePtr; +struct _virBlkioDevice { + char *path; + unsigned int weight; + unsigned int riops; + unsigned int wiops; + unsigned long long rbps; + unsigned long long wbps; +}; + + +typedef struct _virBlkioTune virBlkioTune; +typedef virBlkioTune *virBlkioTunePtr; +struct _virBlkioTune { + unsigned int weight; + + size_t ndevices; + virBlkioDevicePtr devices; +}; + +void virBlkioDeviceArrayClear(virBlkioDevicePtr deviceWeights, + int ndevices); + +#endif /* __VIR_BLKIO_H__ */ -- 2.17.1
Let's move those to their own newly created header (src/util/virmem.h) as this will help us to easily start sharing the cgroup code that's duplicated between QEMU and LXC. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/conf/domain_conf.c | 11 +++---- src/conf/domain_conf.h | 43 ++------------------------- src/qemu/qemu_command.c | 5 ++-- src/qemu/qemu_domain.c | 3 +- src/util/virmem.h | 66 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 79 insertions(+), 49 deletions(-) create mode 100644 src/util/virmem.h diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3384a36d76..78db2a6c2c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -60,6 +60,7 @@ #include "virhostdev.h" #include "virmdev.h" #include "virblkio.h" +#include "virmem.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -6174,7 +6175,7 @@ virDomainDefLifecycleActionValidate(const virDomainDef *def) static int virDomainDefMemtuneValidate(const virDomainDef *def) { - const virDomainMemtune *mem = &(def->mem); + const virMemTune *mem = &(def->mem); size_t i; ssize_t pos = virDomainNumaGetNodeCount(def->numa) - 1; @@ -18188,7 +18189,7 @@ virDomainDefMaybeAddInput(virDomainDefPtr def, static int virDomainHugepagesParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, - virDomainHugePagePtr hugepage) + virMemHugePagePtr hugepage) { int ret = -1; xmlNodePtr oldnode = ctxt->node; @@ -26832,7 +26833,7 @@ virDomainResourceDefFormat(virBufferPtr buf, static int virDomainHugepagesFormatBuf(virBufferPtr buf, - virDomainHugePagePtr hugepage) + virMemHugePagePtr hugepage) { int ret = -1; @@ -26856,7 +26857,7 @@ virDomainHugepagesFormatBuf(virBufferPtr buf, static void virDomainHugepagesFormat(virBufferPtr buf, - virDomainHugePagePtr hugepages, + virMemHugePagePtr hugepages, size_t nhugepages) { size_t i; @@ -27394,7 +27395,7 @@ virDomainIOMMUDefFormat(virBufferPtr buf, static int virDomainMemtuneFormat(virBufferPtr buf, - const virDomainMemtune *mem) + const virMemTune *mem) { virBuffer childBuf = VIR_BUFFER_INITIALIZER; int ret = -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e9e6b6d6c4..10acc39861 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -58,6 +58,7 @@ # include "virsavecookie.h" # include "virresctrl.h" # include "virblkio.h" +# include "virmem.h" /* forward declarations of all device types, required by * virDomainDeviceDef @@ -2180,14 +2181,6 @@ struct _virDomainResourceDef { char *partition; }; -typedef struct _virDomainHugePage virDomainHugePage; -typedef virDomainHugePage *virDomainHugePagePtr; - -struct _virDomainHugePage { - virBitmapPtr nodemask; /* guest's NUMA node mask */ - unsigned long long size; /* hugepage size in KiB */ -}; - # define VIR_DOMAIN_CPUMASK_LEN 1024 typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef; @@ -2247,38 +2240,6 @@ struct _virDomainVcpuDef { virObjectPtr privateData; }; -typedef struct _virDomainMemtune virDomainMemtune; -typedef virDomainMemtune *virDomainMemtunePtr; - -struct _virDomainMemtune { - /* total memory size including memory modules in kibibytes, this field - * should be accessed only via accessors */ - unsigned long long total_memory; - unsigned long long cur_balloon; /* in kibibytes, capped at ulong thanks - to virDomainGetInfo */ - - virDomainHugePagePtr hugepages; - size_t nhugepages; - - /* maximum supported memory for a guest, for hotplugging */ - unsigned long long max_memory; /* in kibibytes */ - unsigned int memory_slots; /* maximum count of RAM memory slots */ - - bool nosharepages; - bool locked; - int dump_core; /* enum virTristateSwitch */ - unsigned long long hard_limit; /* in kibibytes, limit at off_t bytes */ - unsigned long long soft_limit; /* in kibibytes, limit at off_t bytes */ - unsigned long long min_guarantee; /* in kibibytes, limit at off_t bytes */ - unsigned long long swap_hard_limit; /* in kibibytes, limit at off_t bytes */ - - int source; /* enum virDomainMemorySource */ - int access; /* enum virDomainMemoryAccess */ - int allocation; /* enum virDomainMemoryAllocation */ - - virTristateBool discard; -}; - typedef struct _virDomainPowerManagement virDomainPowerManagement; typedef virDomainPowerManagement *virDomainPowerManagementPtr; @@ -2380,7 +2341,7 @@ struct _virDomainDef { char *description; virBlkioTune blkio; - virDomainMemtune mem; + virMemTune mem; virDomainVcpuDefPtr *vcpus; size_t maxvcpus; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0a353f87ba..968234b82a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -60,6 +60,7 @@ #include "virnuma.h" #include "virgic.h" #include "virmdev.h" +#include "virmem.h" #if defined(__linux__) # include <linux/capability.h> #endif @@ -3202,8 +3203,8 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT; if (pagesize == 0) { - virDomainHugePagePtr master_hugepage = NULL; - virDomainHugePagePtr hugepage = NULL; + virMemHugePagePtr master_hugepage = NULL; + virMemHugePagePtr hugepage = NULL; bool thisHugepage = false; /* Find the huge page size we want to use */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e12f05f9d1..661d78f77e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -54,6 +54,7 @@ #include "vircrypto.h" #include "virrandom.h" #include "virsystemd.h" +#include "virmem.h" #include "secret_util.h" #include "logging/log_manager.h" #include "locking/domain_lock.h" @@ -3953,7 +3954,7 @@ static int qemuDomainDefValidateMemory(const virDomainDef *def) { const long system_page_size = virGetSystemPageSizeKB(); - const virDomainMemtune *mem = &def->mem; + const virMemTune *mem = &def->mem; if (mem->nhugepages == 0) return 0; diff --git a/src/util/virmem.h b/src/util/virmem.h new file mode 100644 index 0000000000..1cd077f04b --- /dev/null +++ b/src/util/virmem.h @@ -0,0 +1,66 @@ +/* + * virmem.h: Memory definitions and helpers + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Fabiano Fidêncio <fidencio@redhat.com> + */ + +#ifndef __VIR_MEM_H__ +# define __VIR_MEM_H__ + +# include "virutil.h" + +typedef struct _virMemHugePage virMemHugePage; +typedef virMemHugePage *virMemHugePagePtr; +struct _virMemHugePage { + virBitmapPtr nodemask; /* guest's NUMA node mask */ + unsigned long long size; /* hugepage size in KiB */ +}; + +typedef struct _virMemTune virMemTune; +typedef virMemTune *virMemTunePtr; +struct _virMemTune { + /* total memory size including memory modules in kibibytes, this field + * should be accessed only via accessors */ + unsigned long long total_memory; + unsigned long long cur_balloon; /* in kibibytes, capped at ulong thanks + to virDomainGetInfo */ + + virMemHugePagePtr hugepages; + size_t nhugepages; + + /* maximum supported memory for a guest, for hotplugging */ + unsigned long long max_memory; /* in kibibytes */ + unsigned int memory_slots; /* maximum count of RAM memory slots */ + + bool nosharepages; + bool locked; + int dump_core; /* enum virTristateSwitch */ + unsigned long long hard_limit; /* in kibibytes, limit at off_t bytes */ + unsigned long long soft_limit; /* in kibibytes, limit at off_t bytes */ + unsigned long long min_guarantee; /* in kibibytes, limit at off_t bytes */ + unsigned long long swap_hard_limit; /* in kibibytes, limit at off_t bytes */ + + int source; /* enum virDomainMemorySource */ + int access; /* enum virDomainMemoryAccess */ + int allocation; /* enum virDomainMemoryAllocation */ + + virTristateBool discard; +}; + +#endif /* __VIR_MEM_H__ */ -- 2.17.1
virCgroupSetupBlkioTune() has been introduced in order to remove the code duplication present between virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup(). Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- 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 bf1536ea57..5df48f88f4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1579,6 +1579,7 @@ virCgroupSetMemoryHardLimit; virCgroupSetMemorySoftLimit; virCgroupSetMemSwapHardLimit; virCgroupSetOwner; +virCgroupSetupBlkioTune; virCgroupSupportsCpuBW; virCgroupTerminateMachine; diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index d93a19d684..4a95f2a8b0 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 43e17d786e..0e53678faa 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -514,7 +514,6 @@ static int qemuSetupBlkioCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - size_t i; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) { @@ -527,51 +526,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 64507bf8aa..a08b6f4869 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -4820,3 +4820,57 @@ virCgroupDelThread(virCgroupPtr cgroup, return 0; } + + +int +virCgroupSetupBlkioTune(virCgroupPtr cgroup, + virBlkioTunePtr 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 ee3b7c7222..2908f70372 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -27,6 +27,7 @@ # include "virutil.h" # include "virbitmap.h" +# include "virblkio.h" struct _virCgroup; typedef struct _virCgroup virCgroup; @@ -286,4 +287,6 @@ int virCgroupSetOwner(virCgroupPtr cgroup, int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller); bool virCgroupControllerAvailable(int controller); + +int virCgroupSetupBlkioTune(virCgroupPtr cgroup, virBlkioTunePtr blkio); #endif /* __VIR_CGROUP_H__ */ -- 2.17.1
virCgroupSetupMemTune() has been introduced in order to remove the code duplication present between virLXCCgroupSetupMemTune() and qemuSetupMemoryCgroup(). Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- 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 | 2 ++ 5 files changed, 26 insertions(+), 31 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5df48f88f4..94044c60ec 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1580,6 +1580,7 @@ virCgroupSetMemorySoftLimit; virCgroupSetMemSwapHardLimit; virCgroupSetOwner; virCgroupSetupBlkioTune; +virCgroupSetupMemTune; virCgroupSupportsCpuBW; virCgroupTerminateMachine; diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 4a95f2a8b0..95311fde9e 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 0e53678faa..205098ec30 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -547,19 +547,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 a08b6f4869..3973f6da61 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -4874,3 +4874,23 @@ virCgroupSetupBlkioTune(virCgroupPtr cgroup, return 0; } + + +int +virCgroupSetupMemTune(virCgroupPtr cgroup, + virMemTunePtr 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 2908f70372..c01422ec84 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -28,6 +28,7 @@ # include "virutil.h" # include "virbitmap.h" # include "virblkio.h" +# include "virmem.h" struct _virCgroup; typedef struct _virCgroup virCgroup; @@ -289,4 +290,5 @@ int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller); bool virCgroupControllerAvailable(int controller); int virCgroupSetupBlkioTune(virCgroupPtr cgroup, virBlkioTunePtr blkio); +int virCgroupSetupMemTune(virCgroupPtr cgroup, virMemTunePtr mem); #endif /* __VIR_CGROUP_H__ */ -- 2.17.1
On Thu, Sep 13, 2018 at 09:48:10AM +0200, Fabiano Fidêncio wrote:
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.
Mind that the first two patches are basically preparing the ground for the changes introduced in the last two patches.
I know that Pavel Hrdina suggested taking a different path[0] for this series but I do believe Michal's and Jano's points are valid, thus I've decided to just do the changes suggested by Michal (at least for now).
That's OK, but I'm still not convinced that vircgroup.c is the correct place. These new functions convert some data structure into multiple cgroup API calls. By that logic we could do the same for virTypedParams which would lead into adding several helpers that would covert data structures into virTypedParams and that feels wrong. Another thing that I don't like is moving the blkio and mem tune code into src/util because in util we have helpers to work with OS tools or OS related data and some really generic tools that could be easily used outside of libvirt and virtualization related code (yes, there are some exception that should be most likely fixed). If we want to deduplicate qemu_cgroup.c and lxc_cgroup.c we should introduce src/conf/domain_cgroup.c where we would place all common domain related code that configure cgroups. Pavel
[0]: https://www.redhat.com/archives/libvir-list/2018-September/msg00534.html
changes since v1: - Michal Privoznik pointed out (as did the `make syntax-check` :-)) that we do want to keep src/util independently of the parsing code (thus, including "conf/domain_conf.h" in vircgroup.h is not the way to go). This has been solved now by partially following Michal's suggestion and splitting the structs and functions that would be use in the common code to new different files.
changes since v2: - Fixed `make syntax-check` errors that I've missed before (didn't have cppi installed); - Created a new section in src/libvirt_private.syms to correctly export the virBlkioDeviceArrayClear symbol; - Removed a virlkio.c from libvirt_setuid_rpc_client_la__SOURCES (why did I add it there in the first place? ;-)) - Explicitly included virmem.h in qemu_{domain,command}.c. Although there's nothing enforcing it, I do believe it's a good practive that should be followed and I've checked with Andrea and Peter about doing that and both agreed with the approach.
Fabiano Fidêncio (4): domain_conf: split out virBlkioDevice and virDomainBlkiotune definitions domain_conf: split out virDomainMemtune and virDomainHugePage definitions vircgroup: Add virCgroupSetupBlkioTune() vircgroup: Add virCgroupSetupMemTune()
src/conf/domain_conf.c | 22 ++++-------- src/conf/domain_conf.h | 70 +++---------------------------------- src/libvirt_private.syms | 7 +++- src/lxc/lxc_cgroup.c | 69 ++----------------------------------- src/qemu/qemu_cgroup.c | 61 ++------------------------------- src/qemu/qemu_command.c | 5 +-- src/qemu/qemu_domain.c | 3 +- src/util/Makefile.inc.am | 2 ++ src/util/virblkio.c | 37 ++++++++++++++++++++ src/util/virblkio.h | 52 ++++++++++++++++++++++++++++ src/util/vircgroup.c | 74 ++++++++++++++++++++++++++++++++++++++++ src/util/vircgroup.h | 5 +++ src/util/virmem.h | 66 +++++++++++++++++++++++++++++++++++ 13 files changed, 263 insertions(+), 210 deletions(-) create mode 100644 src/util/virblkio.c create mode 100644 src/util/virblkio.h create mode 100644 src/util/virmem.h
-- 2.17.1
Subject: [libvirt PATCH v2 0/4] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 09/13/2018 02:33 PM, Pavel Hrdina wrote:
On Thu, Sep 13, 2018 at 09:48:10AM +0200, Fabiano Fidêncio wrote:
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.
Mind that the first two patches are basically preparing the ground for the changes introduced in the last two patches.
I know that Pavel Hrdina suggested taking a different path[0] for this series but I do believe Michal's and Jano's points are valid, thus I've decided to just do the changes suggested by Michal (at least for now).
That's OK, but I'm still not convinced that vircgroup.c is the correct place. These new functions convert some data structure into multiple cgroup API calls.
By that logic we could do the same for virTypedParams which would lead into adding several helpers that would covert data structures into virTypedParams and that feels wrong.
Another thing that I don't like is moving the blkio and mem tune code into src/util because in util we have helpers to work with OS tools or OS related data and some really generic tools that could be easily used outside of libvirt and virtualization related code (yes, there are some exception that should be most likely fixed).
Well, blkio and memtune code is process oriented code (just like bandwidth code for instance). It is true we currently use it only in domain code, but that shouldn't be reason to keep it under src/conf. Just like we are not keeping bandwidth code there.
If we want to deduplicate qemu_cgroup.c and lxc_cgroup.c we should introduce src/conf/domain_cgroup.c where we would place all common domain related code that configure cgroups.
In my view, src/conf should contain only XML configuration related code (i.e. XML parser/formatter). It's only our laziness that we dumped a lot of unrelated code there. Anyway, this conversation now looks like bikesheding to me. The code deduplication is what matters and where the code is going to live doesn't matter that much. Therefore, I will no longer object. Michal
participants (3)
-
Fabiano Fidêncio -
Michal Privoznik -
Pavel Hrdina