[libvirt] [PATCH 0/2] change the minimum blkio weight according to kernel version
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> Since 2.6.39, kernel changed the minimum weight of device blkio. We need to follow them according to different kernel version. Chen Hanxiao (2): [libvirt][PATCH]cgroup: introduce kernel version check function for cgroup [libvirt][PATCH]blkio: change the minimum weight according to kernel version src/util/vircgroup.c | 76 ++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 65 insertions(+), 11 deletions(-) -- 1.8.2.1
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> Some cgroup value range may change in the further kernel. Introduce kernel version check function for cgroup. This will be helpful to determine the proper values. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/util/vircgroup.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index e99caf5..498bc20 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -38,6 +38,7 @@ #include <sys/types.h> #include <signal.h> #include <dirent.h> +#include <sys/utsname.h> #define __VIR_CGROUP_ALLOW_INCLUDE_PRIV_H__ #include "vircgrouppriv.h" @@ -74,6 +75,37 @@ typedef enum { */ } virCgroupFlags; +static int virCgroupGetKernelVersion(unsigned long *kernelVersion) +{ + struct utsname ver; + + uname(&ver); + + if (virParseVersionString(ver.release, kernelVersion, true) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown release: %s"), ver.release); + return -1; + } + + return 0; +} + +static bool virCgroupVersionCheck(const char * dstVersion) +{ + unsigned long currentVersion; + unsigned long version; + + if (virCgroupGetKernelVersion(¤tVersion) < 0) { + return -1; + } + + if (virParseVersionString(dstVersion, &version, true) < 0) { + return -1; + } + + return (((long long)currentVersion - (long long)version) >= 0) ? true : false; +} + + #ifdef VIR_CGROUP_SUPPORTED bool -- 1.8.2.1
On 10/08/2013 02:38 AM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Some cgroup value range may change
s/range/ranges/
in the further kernel.
s/the further kernel/future kernels/
Introduce kernel version check function for cgroup. This will be helpful to determine the proper values.
I'm half-inclined to NACK. Version checks are lousy, as features may be backported to a distro-style kernel that reports an older version number. Is there any way you can probe for an actual feature, rather than relying on brittle version number checks?
+ +static bool virCgroupVersionCheck(const char * dstVersion)
Style: no space after '*' in pointer type declarations.
+{ + unsigned long currentVersion; + unsigned long version; + + if (virCgroupGetKernelVersion(¤tVersion) < 0) { + return -1; + } + + if (virParseVersionString(dstVersion, &version, true) < 0) { + return -1; + } + + return (((long long)currentVersion - (long long)version) >= 0) ? true : false;
The casts are pointless. Either we're on a 64-bit platform where it adds no precision, or we're on a 32-bit platform where the extra precision slows us down; but either way the version number already fits within a long. Style: 'bool_expr ? true: false' is wasteful; just write 'bool_expr'. Thus, this would be: return currentVersion >= version; if we still determine we need this function. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Wednesday, October 09, 2013 11:49 AM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/2]cgroup: introduce kernel version check function for cgroup
On 10/08/2013 02:38 AM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Some cgroup value range may change
s/range/ranges/
in the further kernel.
s/the further kernel/future kernels/
Introduce kernel version check function for cgroup. This will be helpful to determine the proper values.
I'm half-inclined to NACK. Version checks are lousy, as features may be backported to a distro-style kernel that reports an older version number. Is there any way you can probe for an actual feature, rather than relying on brittle version number checks?
Maybe we could leave this job to kernel.
+ +static bool virCgroupVersionCheck(const char * dstVersion)
Style: no space after '*' in pointer type declarations.
+{ + unsigned long currentVersion; + unsigned long version; + + if (virCgroupGetKernelVersion(¤tVersion) < 0) { + return -1; + } + + if (virParseVersionString(dstVersion, &version, true) < 0) { + return -1; + } + + return (((long long)currentVersion - (long long)version) >= 0) ? true : false;
The casts are pointless. Either we're on a 64-bit platform where it adds no precision, or we're on a 32-bit platform where the extra precision slows us down; but either way the version number already fits within a long.
Style: 'bool_expr ? true: false' is wasteful; just write 'bool_expr'.
Thus, this would be:
return currentVersion >= version;
Thanks for your explanation.
if we still determine we need this function.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> kernel had changed the minimum weight of device blkio from 100 to 10 in commit df457f845e5449be2e7d96668791f789b3770ac7. So we should to use new blkio weight range after 2.6.39. libvirt should follow kernel according to kernel version. Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/util/vircgroup.c | 44 +++++++++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 498bc20..fa3b67e 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -57,6 +57,8 @@ #define VIR_FROM_THIS VIR_FROM_CGROUP +#define BLKIO_WEIGHT_DEVICE_RANGE_CHANGED_VERSION "2.6.39" + #if defined(__linux__) && defined(HAVE_GETMNTENT_R) && \ defined(_DIRENT_HAVE_D_TYPE) && defined(_SC_CLK_TCK) # define VIR_CGROUP_SUPPORTED @@ -1820,11 +1822,20 @@ virCgroupPathOfController(virCgroupPtr group, int virCgroupSetBlkioWeight(virCgroupPtr group, unsigned int weight) { - if (weight > 1000 || weight < 100) { - virReportError(VIR_ERR_INVALID_ARG, - _("weight '%u' must be in range (100, 1000)"), - weight); - return -1; + if (virCgroupVersionCheck(BLKIO_WEIGHT_DEVICE_RANGE_CHANGED_VERSION)) { + if (weight > 1000 || weight < 10) { + virReportError(VIR_ERR_INVALID_ARG, + _("weight '%u' must be in range (10, 1000)"), + weight); + return -1; + } + } else { + if (weight > 1000 || weight < 100) { + virReportError(VIR_ERR_INVALID_ARG, + _("weight '%u' must be in range (100, 1000)"), + weight); + return -1; + } } return virCgroupSetValueU64(group, @@ -1861,7 +1872,8 @@ virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight) * * @group: The cgroup to change io device weight device for * @path: The device with a weight to alter - * @weight: The new device weight (100-1000), or 0 to clear + * @weight: The new device weight (100-1000) or + * (10-1000) after kernel 2.6.39, or 0 to clear * * device_weight is treated as a write-only parameter, so * there isn't a getter counterpart. @@ -1877,11 +1889,21 @@ virCgroupSetBlkioDeviceWeight(virCgroupPtr group, struct stat sb; int ret; - if (weight && (weight > 1000 || weight < 100)) { - virReportError(VIR_ERR_INVALID_ARG, - _("weight '%u' must be in range (100, 1000)"), - weight); - return -1; + if (virCgroupVersionCheck(BLKIO_WEIGHT_DEVICE_RANGE_CHANGED_VERSION)) { + if (weight && + (weight > 1000 || weight < 10)) { + virReportError(VIR_ERR_INVALID_ARG, + _("weight '%u' must be in range (10, 1000)"), + weight); + return -1; + } + } else { + if (weight && (weight > 1000 || weight < 100)) { + virReportError(VIR_ERR_INVALID_ARG, + _("weight '%u' must be in range (100, 1000)"), + weight); + return -1; + } } if (stat(path, &sb) < 0) { -- 1.8.2.1
On 10/08/2013 02:38 AM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
kernel had changed the minimum weight of device blkio from 100 to 10 in commit df457f845e5449be2e7d96668791f789b3770ac7. So we should to use new blkio weight range after 2.6.39.
Why not do it unconditionally, and then just have graceful error detection when on older kernels when the input is rejected by the kernel as out of range? That would be feature-based instead of version-check based, which makes it more reliable even if the feature of the kernel is backported to something that uname reports as having an older version number.
libvirt should follow kernel according to kernel version.
Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/util/vircgroup.c | 44 +++++++++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 11 deletions(-)
Incomplete. You also need to touch virsh.pod and probably formatdomain.html.in to document the larger range, and mention that different connections may have different ranges (due to different kernel versions on the other ends of those two connections). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Wednesday, October 09, 2013 11:52 AM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 2/2]blkio: change the minimum weight according to kernel version
On 10/08/2013 02:38 AM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
kernel had changed the minimum weight of device blkio from 100 to 10 in commit df457f845e5449be2e7d96668791f789b3770ac7. So we should to use new blkio weight range after 2.6.39.
Why not do it unconditionally, and then just have graceful error detection when on older kernels when the input is rejected by the kernel as out of range? That would be feature-based instead of version-check based, which makes it more reliable even if the feature of the kernel is backported to something that uname reports as having an older version number.
How about just add the range to the doc and tell users that the range was changed. Since the kernel will check whether the value is valid or not, how do you think we leave this to kernel and check the errno? That means libvirt did not check the input value ranges, send it to kernel and catch the error. Then we could not care how kernel would change its range. Thanks
libvirt should follow kernel according to kernel version.
Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/util/vircgroup.c | 44 +++++++++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 11 deletions(-)
Incomplete. You also need to touch virsh.pod and probably formatdomain.html.in to document the larger range, and mention that different connections may have different ranges (due to different kernel versions on the other ends of those two connections).
Thanks. I will add these in the next patch.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Chen Hanxiao -
Eric Blake