Re: [Qemu-devel] [PATCH 1.1?] qemu_rearm_alarm_timer: do not call rearm if the next deadline is INT64_MAX
On Wed, 30 May 2012, Anthony Liguori wrote: Reviewed-by: Stefan Weils...@weilnetz.de This patch clearly improves the current code and fixes an abort on Darwin (reported by Andreas Färber) and maybe other hosts. Therefore I changed the subject and suggest to consider this patch for QEMU 1.1. Not for 1.1.0. I'm just a few hours away from pushing the 1.1.0-rc4 tag and I don't plan on doing any updates before GA unless something critical emerges. Anthony, are you going to pick this one up for 1.1.1? Do you need me to do anything?
Re: [Qemu-devel] [PATCH 1.1?] qemu_rearm_alarm_timer: do not call rearm if the next deadline is INT64_MAX
Am 11.06.2012 11:55, schrieb Stefano Stabellini: On Wed, 30 May 2012, Anthony Liguori wrote: Reviewed-by: Stefan Weils...@weilnetz.de This patch clearly improves the current code and fixes an abort on Darwin (reported by Andreas Färber) and maybe other hosts. Therefore I changed the subject and suggest to consider this patch for QEMU 1.1. Not for 1.1.0. I'm just a few hours away from pushing the 1.1.0-rc4 tag and I don't plan on doing any updates before GA unless something critical emerges. Anthony, are you going to pick this one up for 1.1.1? Do you need me to do anything? I think I still need to test this one... thanks for the reminder. ;) Andreas
Re: [Qemu-devel] [PATCH 1.1?] qemu_rearm_alarm_timer: do not call rearm if the next deadline is INT64_MAX
Am 29.05.2012 15:35, schrieb Stefano Stabellini:
qemu_rearm_alarm_timer partially duplicates the code in
qemu_next_alarm_deadline to figure out if it needs to rearm the timer.
If it calls qemu_next_alarm_deadline, it always rearms the timer even if
the next deadline is INT64_MAX.
This patch simplifies the behavior of qemu_rearm_alarm_timer and removes
the duplicated code, always calling qemu_next_alarm_deadline and only
rearming the timer if the deadline is less than INT64_MAX.
Signed-off-by: Stefano Stabellinistefano.stabell...@eu.citrix.com
diff --git a/qemu-timer.c b/qemu-timer.c
index de98977..81ff824 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -112,14 +112,10 @@ static int64_t qemu_next_alarm_deadline(void)
static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t)
{
-int64_t nearest_delta_ns;
-if (!rt_clock-active_timers
-!vm_clock-active_timers
-!host_clock-active_timers) {
-return;
+int64_t nearest_delta_ns = qemu_next_alarm_deadline();
+if (nearest_delta_ns INT64_MAX) {
+t-rearm(t, nearest_delta_ns);
}
-nearest_delta_ns = qemu_next_alarm_deadline();
-t-rearm(t, nearest_delta_ns);
}
/* TODO: MIN_TIMER_REARM_NS should be optimized */
Reviewed-by: Stefan Weil s...@weilnetz.de
This patch clearly improves the current code and fixes
an abort on Darwin (reported by Andreas Färber) and maybe
other hosts. Therefore I changed the subject and suggest
to consider this patch for QEMU 1.1.
There remain issues which can be fixed after 1.1:
nearest_delta_ns also gets negative values (rtdelta 0,
maybe because the expiration time already expired).
I did not check whether all different timers handle
a negative time gracefully.
nearest_delta_ns should also be limited to INT32_MAX
seconds, because some timers assign the seconds
to a long (see setitimer) or UINT value. On 32 bit
Linux and on all variants of Windows, long is less
or equal INT32_MAX. If we limit nearest_delta_ns
to 100 seconds (or some other limit which allows
ULONG milliseconds), we could further simplify the code
because most timers would no longer have to test the
upper limit.
Regards,
Stefan W.
Re: [Qemu-devel] [PATCH 1.1?] qemu_rearm_alarm_timer: do not call rearm if the next deadline is INT64_MAX
On Tue, 29 May 2012, Stefan Weil wrote:
Am 29.05.2012 15:35, schrieb Stefano Stabellini:
qemu_rearm_alarm_timer partially duplicates the code in
qemu_next_alarm_deadline to figure out if it needs to rearm the timer.
If it calls qemu_next_alarm_deadline, it always rearms the timer even if
the next deadline is INT64_MAX.
This patch simplifies the behavior of qemu_rearm_alarm_timer and removes
the duplicated code, always calling qemu_next_alarm_deadline and only
rearming the timer if the deadline is less than INT64_MAX.
Signed-off-by: Stefano Stabellinistefano.stabell...@eu.citrix.com
diff --git a/qemu-timer.c b/qemu-timer.c
index de98977..81ff824 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -112,14 +112,10 @@ static int64_t qemu_next_alarm_deadline(void)
static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t)
{
-int64_t nearest_delta_ns;
-if (!rt_clock-active_timers
-!vm_clock-active_timers
-!host_clock-active_timers) {
-return;
+int64_t nearest_delta_ns = qemu_next_alarm_deadline();
+if (nearest_delta_ns INT64_MAX) {
+t-rearm(t, nearest_delta_ns);
}
-nearest_delta_ns = qemu_next_alarm_deadline();
-t-rearm(t, nearest_delta_ns);
}
/* TODO: MIN_TIMER_REARM_NS should be optimized */
Reviewed-by: Stefan Weil s...@weilnetz.de
thanks
This patch clearly improves the current code and fixes
an abort on Darwin (reported by Andreas Färber) and maybe
other hosts. Therefore I changed the subject and suggest
to consider this patch for QEMU 1.1.
There remain issues which can be fixed after 1.1:
nearest_delta_ns also gets negative values (rtdelta 0,
maybe because the expiration time already expired).
I did not check whether all different timers handle
a negative time gracefully.
nearest_delta_ns should also be limited to INT32_MAX
seconds, because some timers assign the seconds
to a long (see setitimer) or UINT value. On 32 bit
Linux and on all variants of Windows, long is less
or equal INT32_MAX. If we limit nearest_delta_ns
to 100 seconds (or some other limit which allows
ULONG milliseconds), we could further simplify the code
because most timers would no longer have to test the
upper limit.
If that's the issue we could limit nearest_delta_ns to LONG_MAX.
However I got the feeling that Darwin has an undocumented limit
for tv_sec, lower than INT32_MAX.
Re: [Qemu-devel] [PATCH 1.1?] qemu_rearm_alarm_timer: do not call rearm if the next deadline is INT64_MAX
Am 29.05.2012 19:23, schrieb Stefano Stabellini:
On Tue, 29 May 2012, Stefan Weil wrote:
Am 29.05.2012 15:35, schrieb Stefano Stabellini:
qemu_rearm_alarm_timer partially duplicates the code in
qemu_next_alarm_deadline to figure out if it needs to rearm the timer.
If it calls qemu_next_alarm_deadline, it always rearms the timer even if
the next deadline is INT64_MAX.
This patch simplifies the behavior of qemu_rearm_alarm_timer and removes
the duplicated code, always calling qemu_next_alarm_deadline and only
rearming the timer if the deadline is less than INT64_MAX.
Signed-off-by: Stefano Stabellinistefano.stabell...@eu.citrix.com
diff --git a/qemu-timer.c b/qemu-timer.c
index de98977..81ff824 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -112,14 +112,10 @@ static int64_t qemu_next_alarm_deadline(void)
static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t)
{
-int64_t nearest_delta_ns;
-if (!rt_clock-active_timers
-!vm_clock-active_timers
-!host_clock-active_timers) {
-return;
+int64_t nearest_delta_ns = qemu_next_alarm_deadline();
+if (nearest_delta_ns INT64_MAX) {
+t-rearm(t, nearest_delta_ns);
}
-nearest_delta_ns = qemu_next_alarm_deadline();
-t-rearm(t, nearest_delta_ns);
}
/* TODO: MIN_TIMER_REARM_NS should be optimized */
Reviewed-by: Stefan Weils...@weilnetz.de
thanks
This patch clearly improves the current code and fixes
an abort on Darwin (reported by Andreas Färber) and maybe
other hosts. Therefore I changed the subject and suggest
to consider this patch for QEMU 1.1.
There remain issues which can be fixed after 1.1:
nearest_delta_ns also gets negative values (rtdelta 0,
maybe because the expiration time already expired).
I did not check whether all different timers handle
a negative time gracefully.
nearest_delta_ns should also be limited to INT32_MAX
seconds, because some timers assign the seconds
to a long (see setitimer) or UINT value. On 32 bit
Linux and on all variants of Windows, long is less
or equal INT32_MAX. If we limit nearest_delta_ns
to 100 seconds (or some other limit which allows
ULONG milliseconds), we could further simplify the code
because most timers would no longer have to test the
upper limit.
If that's the issue we could limit nearest_delta_ns to LONG_MAX.
However I got the feeling that Darwin has an undocumented limit
for tv_sec, lower than INT32_MAX.
Yes, we could set the upper limit to LONG_MAX seconds for some
timers, but I did not want to have a dependency of the
upper limit on sizeof(long). The function win32_rearm_timer
only allows 4294967 seconds. Is there any reason why we
should allow timers which expire after more than 11 days
(100 seconds are about 11 days)? If there is none,
100 seconds would be a good upper limit for most timers
(maybe also for Darwin). mm_rearm_timer is the only timer
which then still needs its own upper limit.
Regards,
Stefan W.
Re: [Qemu-devel] [PATCH 1.1?] qemu_rearm_alarm_timer: do not call rearm if the next deadline is INT64_MAX
On Tue, 29 May 2012, Stefan Weil wrote: Yes, we could set the upper limit to LONG_MAX seconds for some timers, but I did not want to have a dependency of the upper limit on sizeof(long). The function win32_rearm_timer only allows 4294967 seconds. Is there any reason why we should allow timers which expire after more than 11 days (100 seconds are about 11 days)? If there is none, 100 seconds would be a good upper limit for most timers (maybe also for Darwin). mm_rearm_timer is the only timer which then still needs its own upper limit. If 100 works for Darwin, then I think it is a good idea.
Re: [Qemu-devel] [PATCH 1.1?] qemu_rearm_alarm_timer: do not call rearm if the next deadline is INT64_MAX
Il 29/05/2012 19:09, Stefan Weil ha scritto:
Am 29.05.2012 15:35, schrieb Stefano Stabellini:
qemu_rearm_alarm_timer partially duplicates the code in
qemu_next_alarm_deadline to figure out if it needs to rearm the timer.
If it calls qemu_next_alarm_deadline, it always rearms the timer even if
the next deadline is INT64_MAX.
This patch simplifies the behavior of qemu_rearm_alarm_timer and removes
the duplicated code, always calling qemu_next_alarm_deadline and only
rearming the timer if the deadline is less than INT64_MAX.
Signed-off-by: Stefano Stabellinistefano.stabell...@eu.citrix.com
diff --git a/qemu-timer.c b/qemu-timer.c
index de98977..81ff824 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -112,14 +112,10 @@ static int64_t qemu_next_alarm_deadline(void)
static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t)
{
-int64_t nearest_delta_ns;
-if (!rt_clock-active_timers
-!vm_clock-active_timers
-!host_clock-active_timers) {
-return;
+int64_t nearest_delta_ns = qemu_next_alarm_deadline();
+if (nearest_delta_ns INT64_MAX) {
+t-rearm(t, nearest_delta_ns);
}
-nearest_delta_ns = qemu_next_alarm_deadline();
-t-rearm(t, nearest_delta_ns);
}
/* TODO: MIN_TIMER_REARM_NS should be optimized */
Reviewed-by: Stefan Weil s...@weilnetz.de
This patch clearly improves the current code and fixes
an abort on Darwin (reported by Andreas Färber) and maybe
other hosts. Therefore I changed the subject and suggest
to consider this patch for QEMU 1.1.
Only with a Tested-by from Andreas.
Paolo
Re: [Qemu-devel] [PATCH 1.1?] qemu_rearm_alarm_timer: do not call rearm if the next deadline is INT64_MAX
On 05/30/2012 05:07 AM, Paolo Bonzini wrote:
Il 29/05/2012 19:09, Stefan Weil ha scritto:
Am 29.05.2012 15:35, schrieb Stefano Stabellini:
qemu_rearm_alarm_timer partially duplicates the code in
qemu_next_alarm_deadline to figure out if it needs to rearm the timer.
If it calls qemu_next_alarm_deadline, it always rearms the timer even if
the next deadline is INT64_MAX.
This patch simplifies the behavior of qemu_rearm_alarm_timer and removes
the duplicated code, always calling qemu_next_alarm_deadline and only
rearming the timer if the deadline is less than INT64_MAX.
Signed-off-by: Stefano Stabellinistefano.stabell...@eu.citrix.com
diff --git a/qemu-timer.c b/qemu-timer.c
index de98977..81ff824 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -112,14 +112,10 @@ static int64_t qemu_next_alarm_deadline(void)
static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t)
{
-int64_t nearest_delta_ns;
-if (!rt_clock-active_timers
-!vm_clock-active_timers
-!host_clock-active_timers) {
-return;
+int64_t nearest_delta_ns = qemu_next_alarm_deadline();
+if (nearest_delta_ns INT64_MAX) {
+t-rearm(t, nearest_delta_ns);
}
-nearest_delta_ns = qemu_next_alarm_deadline();
-t-rearm(t, nearest_delta_ns);
}
/* TODO: MIN_TIMER_REARM_NS should be optimized */
Reviewed-by: Stefan Weils...@weilnetz.de
This patch clearly improves the current code and fixes
an abort on Darwin (reported by Andreas Färber) and maybe
other hosts. Therefore I changed the subject and suggest
to consider this patch for QEMU 1.1.
Not for 1.1.0. I'm just a few hours away from pushing the 1.1.0-rc4 tag and I
don't plan on doing any updates before GA unless something critical emerges.
Regards,
Anthony Liguori
Only with a Tested-by from Andreas.
Paolo
