[libvirt] [PATCH 1/3] virutil: Introduce a new macro named CLAMP.
This patch introduce a new macro to return a value clamped to a given range. Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/util/virutil.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/util/virutil.h b/src/util/virutil.h index 2bb74e2..e8536d8 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -37,6 +37,12 @@ # ifndef MAX # define MAX(a, b) ((a) > (b) ? (a) : (b)) # endif +# ifndef CLAMP +# define CLAMP(v, min, max) ({ \ + typeof(v) _v = v; \ + _v = _v < min ? min: _v; \ + _v > max ? max: _v; }) +# endif int virSetBlocking(int fd, bool blocking) ATTRIBUTE_RETURN_CHECK; int virSetNonBlock(int fd) ATTRIBUTE_RETURN_CHECK; -- 1.8.2.1
As shown in 'man virsh' about schedinfo: Note: The cpu_shares parameter has a valid value range of 0-262144; Negative values are wrapped to positive, and larger values are capped at the maximum. Therefore, -1 is a useful shorthand for 262144. On the Linux kernel, the values 0 and 1 are automatically converted to a minimal value of 2. it works well with --live, but not with --config. Example: # virsh schedinfo rhel7-ful --set cpu_shares=0 --config Scheduler : posix cpu_shares : 0 vcpu_period : 0 vcpu_quota : 0 emulator_period: 0 emulator_quota : 0 cpu_shares is 0 rather than expected 2. What's worse, when we start it again, it is the default value of cpu_shares 1024. Because when we set the value of cpu_shares, when flag is --live, write the value into cgroup/cpu.shares. Then it will convert the value into the range of [2, 262144]. When flag is --config, we set the value into vmdef immidiately and 0 means no settting for cpu_shares. When we start vm again, libvirt use default value(1024) for it. This patch clamp the cpu_shares value when flag is --config, then we will get then "correct" settting in output of virsh schedinfo and value in cgroup after next booting of vm. Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52ca47c..7648865 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,6 +112,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_SCHED_MAX_PERIOD 1000000LL #define QEMU_SCHED_MIN_QUOTA 1000LL #define QEMU_SCHED_MAX_QUOTA 18446744073709551LL +#define QEMU_SCHED_MIN_SHARES 2LL +#define QEMU_SCHED_MAX_SHARES 262144LL #if HAVE_LINUX_KVM_H # include <linux/kvm.h> @@ -9063,7 +9065,9 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.shares = value_ul; + vmdef->cputune.shares = CLAMP(value_ul, + QEMU_SCHED_MIN_SHARES, + QEMU_SCHED_MAX_SHARES); vmdef->cputune.sharesSpecified = true; } -- 1.8.2.1
On Thu, May 15, 2014 at 06:39:50PM +0900, Dongsheng Yang wrote:
As shown in 'man virsh' about schedinfo:
Note: The cpu_shares parameter has a valid value range of 0-262144; Negative values are wrapped to positive, and larger values are capped at the maximum. Therefore, -1 is a useful shorthand for 262144. On the Linux kernel, the values 0 and 1 are automatically converted to a minimal value of 2. it works well with --live, but not with --config.
Example: # virsh schedinfo rhel7-ful --set cpu_shares=0 --config Scheduler : posix cpu_shares : 0 vcpu_period : 0 vcpu_quota : 0 emulator_period: 0 emulator_quota : 0 cpu_shares is 0 rather than expected 2.
What's worse, when we start it again, it is the default value of cpu_shares 1024.
Because when we set the value of cpu_shares, when flag is --live, write the value into cgroup/cpu.shares. Then it will convert the value into the range of [2, 262144]. When flag is --config, we set the value into vmdef immidiately and 0 means no settting for cpu_shares. When we start vm again, libvirt use default value(1024) for it.
This patch clamp the cpu_shares value when flag is --config, then we will get then "correct" settting in output of virsh schedinfo and value in cgroup after next booting of vm.
I was under the impression that this was meant to be fixed by commit 97814d8a, what's the difference to that? Martin
Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52ca47c..7648865 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,6 +112,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_SCHED_MAX_PERIOD 1000000LL #define QEMU_SCHED_MIN_QUOTA 1000LL #define QEMU_SCHED_MAX_QUOTA 18446744073709551LL +#define QEMU_SCHED_MIN_SHARES 2LL +#define QEMU_SCHED_MAX_SHARES 262144LL
#if HAVE_LINUX_KVM_H # include <linux/kvm.h> @@ -9063,7 +9065,9 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.shares = value_ul; + vmdef->cputune.shares = CLAMP(value_ul, + QEMU_SCHED_MIN_SHARES, + QEMU_SCHED_MAX_SHARES); vmdef->cputune.sharesSpecified = true; }
-- 1.8.2.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/15/2014 08:56 PM, Martin Kletzander wrote:
On Thu, May 15, 2014 at 06:39:50PM +0900, Dongsheng Yang wrote:
As shown in 'man virsh' about schedinfo:
Note: The cpu_shares parameter has a valid value range of 0-262144; Negative values are wrapped to positive, and larger values are capped at the maximum. Therefore, -1 is a useful shorthand for 262144. On the Linux kernel, the values 0 and 1 are automatically converted to a minimal value of 2. it works well with --live, but not with --config.
Example: # virsh schedinfo rhel7-ful --set cpu_shares=0 --config Scheduler : posix cpu_shares : 0 vcpu_period : 0 vcpu_quota : 0 emulator_period: 0 emulator_quota : 0 cpu_shares is 0 rather than expected 2.
What's worse, when we start it again, it is the default value of cpu_shares 1024.
Because when we set the value of cpu_shares, when flag is --live, write the value into cgroup/cpu.shares. Then it will convert the value into the range of [2, 262144]. When flag is --config, we set the value into vmdef immidiately and 0 means no settting for cpu_shares. When we start vm again, libvirt use default value(1024) for it.
This patch clamp the cpu_shares value when flag is --config, then we will get then "correct" settting in output of virsh schedinfo and value in cgroup after next booting of vm.
I was under the impression that this was meant to be fixed by commit 97814d8a, what's the difference to that?
Commit 97814d8a is to make the output of virsh schedinfo --live showing the real value in cgroup. This patch here is to do another thing, it is about --config. When we use --live to set cpu_shares, as shown in "man virsh", we depend on the conversion by cgroup. But when we set it with --config, we did not actually write the value into cgroup, then the value will not be converted to the expected value in manpage. And as I described above, when we set cpu_shares=0 with --config, we will get 1024 in next booting.
Martin
Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52ca47c..7648865 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,6 +112,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_SCHED_MAX_PERIOD 1000000LL #define QEMU_SCHED_MIN_QUOTA 1000LL #define QEMU_SCHED_MAX_QUOTA 18446744073709551LL +#define QEMU_SCHED_MIN_SHARES 2LL +#define QEMU_SCHED_MAX_SHARES 262144LL
#if HAVE_LINUX_KVM_H # include <linux/kvm.h> @@ -9063,7 +9065,9 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.shares = value_ul; + vmdef->cputune.shares = CLAMP(value_ul, + QEMU_SCHED_MIN_SHARES, + QEMU_SCHED_MAX_SHARES); vmdef->cputune.sharesSpecified = true; }
-- 1.8.2.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/15/2014 11:39 AM, Dongsheng Yang wrote:
As shown in 'man virsh' about schedinfo:
Note: The cpu_shares parameter has a valid value range of 0-262144; Negative values are wrapped to positive, and larger values are capped at the maximum. Therefore, -1 is a useful shorthand for 262144. On the Linux kernel, the values 0 and 1 are automatically converted to a minimal value of 2. it works well with --live, but not with --config.
Example: # virsh schedinfo rhel7-ful --set cpu_shares=0 --config Scheduler : posix cpu_shares : 0 vcpu_period : 0 vcpu_quota : 0 emulator_period: 0 emulator_quota : 0 cpu_shares is 0 rather than expected 2.
What's worse, when we start it again, it is the default value of cpu_shares 1024.
Because when we set the value of cpu_shares, when flag is --live, write the value into cgroup/cpu.shares. Then it will convert the value into the range of [2, 262144]. When flag is --config, we set the value into vmdef immidiately and 0 means no settting for cpu_shares. When we start vm again, libvirt use default value(1024) for it.
commit bdffab0d5c52d31bd71422b0b69665efb6650953 Author: Ján Tomko <jtomko@redhat.com> CommitDate: 2014-03-26 10:10:02 +0100 Treat zero cpu shares as a valid value changed this behavior. After this commit, if you set cpu_shares to 0 via schedinfo --config, the 0 will be written in the XML and used on domain startup. To use the default value, the <shares> element needs to be removed from the XML before restarting the domain, this cannot be currently done via 'virsh schedinfo'. Jan
On 05/15/2014 09:11 PM, Ján Tomko wrote:
On 05/15/2014 11:39 AM, Dongsheng Yang wrote:
As shown in 'man virsh' about schedinfo:
Note: The cpu_shares parameter has a valid value range of 0-262144; Negative values are wrapped to positive, and larger values are capped at the maximum. Therefore, -1 is a useful shorthand for 262144. On the Linux kernel, the values 0 and 1 are automatically converted to a minimal value of 2. it works well with --live, but not with --config.
Example: # virsh schedinfo rhel7-ful --set cpu_shares=0 --config Scheduler : posix cpu_shares : 0 vcpu_period : 0 vcpu_quota : 0 emulator_period: 0 emulator_quota : 0 cpu_shares is 0 rather than expected 2.
What's worse, when we start it again, it is the default value of cpu_shares 1024.
Because when we set the value of cpu_shares, when flag is --live, write the value into cgroup/cpu.shares. Then it will convert the value into the range of [2, 262144]. When flag is --config, we set the value into vmdef immidiately and 0 means no settting for cpu_shares. When we start vm again, libvirt use default value(1024) for it. commit bdffab0d5c52d31bd71422b0b69665efb6650953 Author: Ján Tomko <jtomko@redhat.com> CommitDate: 2014-03-26 10:10:02 +0100
Treat zero cpu shares as a valid value
changed this behavior.
After this commit, if you set cpu_shares to 0 via schedinfo --config, the 0 will be written in the XML and used on domain startup.
Indeed , it works.
To use the default value, the <shares> element needs to be removed from the XML before restarting the domain, this cannot be currently done via 'virsh schedinfo'.
And yes, my patch here is attempting to do the similar work in 'virsh schedinfo'. Also, it can make the other "invalid" input, such as -1 and 262144+1, get the expected output as manpage described.
Jan
On 05/15/2014 05:18 AM, Dongsheng Yang wrote:
Also, it can make the other "invalid" input, such as -1 and 262144+1,
Auto-wrapping -1 to maximum makes sense. But making other out-of-bounds values, like 262144+1, be auto-clamped sounds risky, especially if the kernel ever changes clamping boundaries. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/15/2014 09:20 PM, Eric Blake wrote:
On 05/15/2014 05:18 AM, Dongsheng Yang wrote:
Also, it can make the other "invalid" input, such as -1 and 262144+1, Auto-wrapping -1 to maximum makes sense. Actually, -1 is not the minmum-1, because the range is [2, 262144]. The reason -1 is converted to maxmum is that -1 is treated as unsigned long of 2^64-1. Then clamp it to range is 262144. But making other out-of-bounds values, like 262144+1, be auto-clamped sounds risky, especially if the kernel ever changes clamping boundaries.
There are two approaches for this issue I think. (1), use SCHED_RANGE_CHECK() for cpu_shares, same with period and quota, then if the value is our of the range, raise an error. (2), keep the behavior for out-of-bounds values in --config as what it is in --live. --live is depending on the conversion of cgroup, then we should follow the style of cgroup in --config too, right? It means 262144+1 should clamped to maxmum. About the concern you mentioned that boundaries may be changed in future, as I explained in another mail in this thread, it is defined in private zone of scheduler, I can not catch up with a good idea to solve it. :(
On 05/15/2014 03:39 AM, Dongsheng Yang wrote:
As shown in 'man virsh' about schedinfo:
Note: The cpu_shares parameter has a valid value range of 0-262144;
This note documents historical kernel limits; if the kernel has changed, this may be out of date.
Because when we set the value of cpu_shares, when flag is --live, write the value into cgroup/cpu.shares. Then it will convert the value into the range of [2, 262144]. When flag is --config, we set the value into vmdef immidiately and 0 means no settting for
s/immidiately/immediately/ s/settting/setting/
cpu_shares. When we start vm again, libvirt use default value(1024)
s/use/uses/
for it.
This patch clamp the cpu_shares value when flag is --config, then
s/clamp/clamps/
we will get then "correct" settting in output of virsh schedinfo
s/settting/setting/
and value in cgroup after next booting of vm.
+++ b/src/qemu/qemu_driver.c @@ -112,6 +112,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_SCHED_MAX_PERIOD 1000000LL #define QEMU_SCHED_MIN_QUOTA 1000LL #define QEMU_SCHED_MAX_QUOTA 18446744073709551LL +#define QEMU_SCHED_MIN_SHARES 2LL +#define QEMU_SCHED_MAX_SHARES 262144LL
I'm a bit reluctant to add these numbers - if the kernel ever changes its range again (which HAS happened for some cgroup tunables), then we are needlessly preventing use of the newer range. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/15/2014 09:17 PM, Eric Blake wrote:
As shown in 'man virsh' about schedinfo:
Note: The cpu_shares parameter has a valid value range of 0-262144; This note documents historical kernel limits; if the kernel has changed,
On 05/15/2014 03:39 AM, Dongsheng Yang wrote: this may be out of date.
...
+++ b/src/qemu/qemu_driver.c @@ -112,6 +112,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_SCHED_MAX_PERIOD 1000000LL #define QEMU_SCHED_MIN_QUOTA 1000LL #define QEMU_SCHED_MAX_QUOTA 18446744073709551LL +#define QEMU_SCHED_MIN_SHARES 2LL +#define QEMU_SCHED_MAX_SHARES 262144LL I'm a bit reluctant to add these numbers - if the kernel ever changes its range again (which HAS happened for some cgroup tunables), then we are needlessly preventing use of the newer range.
Yes, I hate these numbers too. But the range is defined in kernel/sched/sched.h #define MIN_SHARES (1UL << 1) #define MAX_SHARES (1UL << 18) and used in scheduler. shares = clamp(shares, scale_load(MIN_SHARES), scale_load(MAX_SHARES)); So we can not access it out from kernel. As I found there was some numbers for period and quota here, I added the shares number here.
As we have CLAMP macro in util.h, we should use it to replace the open implementations of it. Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 8 ++------ src/test/test_driver.c | 7 ++----- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7648865..96f325d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4547,9 +4547,7 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, if (maxcpu > hostcpus) maxcpu = hostcpus; - /* Clamp to actual number of vcpus */ - if (ncpumaps > targetDef->vcpus) - ncpumaps = targetDef->vcpus; + ncpumaps = CLAMP(ncpumaps, ncpumaps, targetDef->vcpus); if (ncpumaps < 1) { goto cleanup; @@ -4857,9 +4855,7 @@ qemuDomainGetVcpus(virDomainPtr dom, if (maxcpu > hostcpus) maxcpu = hostcpus; - /* Clamp to actual number of vcpus */ - if (maxinfo > priv->nvcpupids) - maxinfo = priv->nvcpupids; + maxinfo = CLAMP(maxinfo, maxinfo, priv->nvcpupids); if (maxinfo >= 1) { if (info != NULL) { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 37756e7..024f63d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2773,12 +2773,9 @@ static int testDomainGetVcpus(virDomainPtr domain, hostcpus = VIR_NODEINFO_MAXCPUS(privconn->nodeInfo); maxcpu = maplen * 8; - if (maxcpu > hostcpus) - maxcpu = hostcpus; - /* Clamp to actual number of vcpus */ - if (maxinfo > privdom->def->vcpus) - maxinfo = privdom->def->vcpus; + maxcpu = CLAMP(maxcpu, maxcpu, hostcpus); + maxinfo = CLAMP(maxinfo, maxinfo, privdom->def->vcpus); /* Populate virVcpuInfo structures */ if (info != NULL) { -- 1.8.2.1
On Thu, May 15, 2014 at 06:39:51PM +0900, Dongsheng Yang wrote:
As we have CLAMP macro in util.h, we should use it to replace the open implementations of it.
Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 8 ++------ src/test/test_driver.c | 7 ++----- 2 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7648865..96f325d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4547,9 +4547,7 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, if (maxcpu > hostcpus) maxcpu = hostcpus;
- /* Clamp to actual number of vcpus */ - if (ncpumaps > targetDef->vcpus) - ncpumaps = targetDef->vcpus; + ncpumaps = CLAMP(ncpumaps, ncpumaps, targetDef->vcpus);
Useless comparison to self; I'd suggest using MIN() instead. This applies to the patch. Martin
if (ncpumaps < 1) { goto cleanup; @@ -4857,9 +4855,7 @@ qemuDomainGetVcpus(virDomainPtr dom, if (maxcpu > hostcpus) maxcpu = hostcpus;
- /* Clamp to actual number of vcpus */ - if (maxinfo > priv->nvcpupids) - maxinfo = priv->nvcpupids; + maxinfo = CLAMP(maxinfo, maxinfo, priv->nvcpupids);
if (maxinfo >= 1) { if (info != NULL) { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 37756e7..024f63d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2773,12 +2773,9 @@ static int testDomainGetVcpus(virDomainPtr domain,
hostcpus = VIR_NODEINFO_MAXCPUS(privconn->nodeInfo); maxcpu = maplen * 8; - if (maxcpu > hostcpus) - maxcpu = hostcpus;
- /* Clamp to actual number of vcpus */ - if (maxinfo > privdom->def->vcpus) - maxinfo = privdom->def->vcpus; + maxcpu = CLAMP(maxcpu, maxcpu, hostcpus); + maxinfo = CLAMP(maxinfo, maxinfo, privdom->def->vcpus);
/* Populate virVcpuInfo structures */ if (info != NULL) { -- 1.8.2.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/15/2014 08:37 PM, Martin Kletzander wrote:
On Thu, May 15, 2014 at 06:39:51PM +0900, Dongsheng Yang wrote:
As we have CLAMP macro in util.h, we should use it to replace the open implementations of it.
Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 8 ++------ src/test/test_driver.c | 7 ++----- 2 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7648865..96f325d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4547,9 +4547,7 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, if (maxcpu > hostcpus) maxcpu = hostcpus;
- /* Clamp to actual number of vcpus */ - if (ncpumaps > targetDef->vcpus) - ncpumaps = targetDef->vcpus; + ncpumaps = CLAMP(ncpumaps, ncpumaps, targetDef->vcpus);
Useless comparison to self; I'd suggest using MIN() instead. This applies to the patch.
Oops, did not realize that. Just found the key word of "Clamp" in comment and replaced it with CLAMP. So it is not related with the CLAMP, and I will sent a v2 of this patch independently. Thanx.
Martin
if (ncpumaps < 1) { goto cleanup; @@ -4857,9 +4855,7 @@ qemuDomainGetVcpus(virDomainPtr dom, if (maxcpu > hostcpus) maxcpu = hostcpus;
- /* Clamp to actual number of vcpus */ - if (maxinfo > priv->nvcpupids) - maxinfo = priv->nvcpupids; + maxinfo = CLAMP(maxinfo, maxinfo, priv->nvcpupids);
if (maxinfo >= 1) { if (info != NULL) { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 37756e7..024f63d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2773,12 +2773,9 @@ static int testDomainGetVcpus(virDomainPtr domain,
hostcpus = VIR_NODEINFO_MAXCPUS(privconn->nodeInfo); maxcpu = maplen * 8; - if (maxcpu > hostcpus) - maxcpu = hostcpus;
- /* Clamp to actual number of vcpus */ - if (maxinfo > privdom->def->vcpus) - maxinfo = privdom->def->vcpus; + maxcpu = CLAMP(maxcpu, maxcpu, hostcpus); + maxinfo = CLAMP(maxinfo, maxinfo, privdom->def->vcpus);
/* Populate virVcpuInfo structures */ if (info != NULL) { -- 1.8.2.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, May 15, 2014 at 06:39:49PM +0900, Dongsheng Yang wrote:
This patch introduce a new macro to return a value clamped to a given range.
Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/util/virutil.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/virutil.h b/src/util/virutil.h index 2bb74e2..e8536d8 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -37,6 +37,12 @@ # ifndef MAX # define MAX(a, b) ((a) > (b) ? (a) : (b)) # endif +# ifndef CLAMP +# define CLAMP(v, min, max) ({ \ + typeof(v) _v = v; \ + _v = _v < min ? min: _v; \ + _v > max ? max: _v; }) +# endif
It's just my subjective impression, but wouldn't the following be a bit more readable and less obfuscated? #define CLAMP(v, min, max) MAX(MIN(v, max), min) Martin.
On 05/15/2014 08:53 PM, Martin Kletzander wrote:
On Thu, May 15, 2014 at 06:39:49PM +0900, Dongsheng Yang wrote:
This patch introduce a new macro to return a value clamped to a given range.
Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/util/virutil.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/virutil.h b/src/util/virutil.h index 2bb74e2..e8536d8 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -37,6 +37,12 @@ # ifndef MAX # define MAX(a, b) ((a) > (b) ? (a) : (b)) # endif +# ifndef CLAMP +# define CLAMP(v, min, max) ({ \ + typeof(v) _v = v; \ + _v = _v < min ? min: _v; \ + _v > max ? max: _v; }) +# endif
It's just my subjective impression, but wouldn't the following be a bit more readable and less obfuscated?
#define CLAMP(v, min, max) MAX(MIN(v, max), min)
Neat! I think it works. I will use it in v2. Thanx
Martin.
On 05/15/2014 03:39 AM, Dongsheng Yang wrote:
This patch introduce a new macro to return a value clamped to a given range.
[when sending a series, it's nice to include a cover letter with 'git send-email --cover-letter to generate the 0/N message that all other messages in the series reply to]
Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/util/virutil.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/virutil.h b/src/util/virutil.h index 2bb74e2..e8536d8 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -37,6 +37,12 @@ # ifndef MAX # define MAX(a, b) ((a) > (b) ? (a) : (b)) # endif +# ifndef CLAMP +# define CLAMP(v, min, max) ({ \
This is gcc-specific. I'd rather avoid it, and stick to portable C99 code, if possible - which means doing this as an inline function rather than a macro. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/15/2014 09:14 PM, Eric Blake wrote:
On 05/15/2014 03:39 AM, Dongsheng Yang wrote:
This patch introduce a new macro to return a value clamped to a given range. [when sending a series, it's nice to include a cover letter with 'git send-email --cover-letter to generate the 0/N message that all other messages in the series reply to]
Okey, Thanx :)
Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- src/util/virutil.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/virutil.h b/src/util/virutil.h index 2bb74e2..e8536d8 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -37,6 +37,12 @@ # ifndef MAX # define MAX(a, b) ((a) > (b) ? (a) : (b)) # endif +# ifndef CLAMP +# define CLAMP(v, min, max) ({ \ This is gcc-specific. I'd rather avoid it, and stick to portable C99 code, if possible - which means doing this as an inline function rather than a macro.
I prefer inline function too, but I found MAX and MIN are implemented with macro, then appended CLAMP to them. Okey, I will use inline function in next version if this patch is acceptable. Thanx
On 05/15/2014 05:25 AM, Dongsheng Yang wrote:
+# define CLAMP(v, min, max) ({ \ This is gcc-specific. I'd rather avoid it, and stick to portable C99 code, if possible - which means doing this as an inline function rather than a macro.
I prefer inline function too, but I found MAX and MIN are implemented with macro, then appended CLAMP to them.
Okey, I will use inline function in next version if this patch is acceptable.
Martin's suggestion of using MIN(MAX()) is also C99 compliant, and usable as a macro. For this particular code, a macro is preferable to an inline function, because it is type-agnostic. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Dongsheng Yang -
Eric Blake -
Ján Tomko -
Martin Kletzander