Change from v1:
According to Jan and Stefano''s comments, drop the previous logic which
setting the value in an arbitrary way and use Stefano''s suggestion
instead.
Currently, the max expiration time is 2147483647ns(INT32_MAX ns)n This is enough
when guest is busy, but when guest is idle, the next timer will be later than
INT32_MAX ns. And those meaningless alarm will harm the pkg C-state.
Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
vl.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/vl.c b/vl.c
index be8587a..a9b7a0d 100644
--- a/vl.c
+++ b/vl.c
@@ -1410,8 +1410,7 @@ static int64_t qemu_next_deadline(void)
delta = active_timers[QEMU_TIMER_VIRTUAL]->expire_time -
qemu_get_clock(vm_clock);
} else {
- /* To avoid problems with overflow limit this to 2^32. */
- delta = INT32_MAX;
+ delta = INT64_MAX;
}
if (delta < 0)
@@ -1427,9 +1426,11 @@ static uint64_t qemu_next_deadline_dyntick(void)
int64_t rtdelta;
if (use_icount)
- delta = INT32_MAX;
- else
- delta = (qemu_next_deadline() + 999) / 1000;
+ delta = INT64_MAX;
+ else {
+ delta = qemu_next_deadline();
+ delta = (delta / 1000) + (delta % 1000 > 0 ? 1 : 0);
+ }
if (active_timers[QEMU_TIMER_REALTIME]) {
rtdelta = (active_timers[QEMU_TIMER_REALTIME]->expire_time -
--
1.7.1
>>> On 15.03.12 at 01:57, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: > Change from v1: > According to Jan and Stefano''s comments, drop the previous logic which > setting the value in an arbitrary way and use Stefano''s suggestion instead. > > Currently, the max expiration time is 2147483647ns(INT32_MAX ns)n This is > enough when guest is busy, but when guest is idle, the next timer will be > later than INT32_MAX ns. And those meaningless alarm will harm the pkg > C-state. > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > --- > vl.c | 11 ++++++----- > 1 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/vl.c b/vl.c > index be8587a..a9b7a0d 100644 > --- a/vl.c > +++ b/vl.c > @@ -1410,8 +1410,7 @@ static int64_t qemu_next_deadline(void) > delta = active_timers[QEMU_TIMER_VIRTUAL]->expire_time - > qemu_get_clock(vm_clock); > } else { > - /* To avoid problems with overflow limit this to 2^32. */ > - delta = INT32_MAX; > + delta = INT64_MAX;You''re silently removing the comment - was it wrong (i.e. is there no potential for overflow)? If so, this should be explained in the commit message. If not, the change is wrong (quite possibly, using e.g. 1 << 48 might be an alternative then, but it needs to be clarified what particular overflow is possible here, and that whatever new value gets chosen doesn''t trigger any). Jan> } > > if (delta < 0) > @@ -1427,9 +1426,11 @@ static uint64_t qemu_next_deadline_dyntick(void) > int64_t rtdelta; > > if (use_icount) > - delta = INT32_MAX; > - else > - delta = (qemu_next_deadline() + 999) / 1000; > + delta = INT64_MAX; > + else { > + delta = qemu_next_deadline(); > + delta = (delta / 1000) + (delta % 1000 > 0 ? 1 : 0); > + } > > if (active_timers[QEMU_TIMER_REALTIME]) { > rtdelta = (active_timers[QEMU_TIMER_REALTIME]->expire_time - > -- > 1.7.1
On Thu, 15 Mar 2012, Jan Beulich wrote:> >>> On 15.03.12 at 01:57, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: > > Change from v1: > > According to Jan and Stefano''s comments, drop the previous logic which > > setting the value in an arbitrary way and use Stefano''s suggestion instead. > > > > Currently, the max expiration time is 2147483647ns(INT32_MAX ns)n This is > > enough when guest is busy, but when guest is idle, the next timer will be > > later than INT32_MAX ns. And those meaningless alarm will harm the pkg > > C-state. > > > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > > --- > > vl.c | 11 ++++++----- > > 1 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/vl.c b/vl.c > > index be8587a..a9b7a0d 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -1410,8 +1410,7 @@ static int64_t qemu_next_deadline(void) > > delta = active_timers[QEMU_TIMER_VIRTUAL]->expire_time - > > qemu_get_clock(vm_clock); > > } else { > > - /* To avoid problems with overflow limit this to 2^32. */ > > - delta = INT32_MAX; > > + delta = INT64_MAX; > > You''re silently removing the comment - was it wrong (i.e. is there no > potential for overflow)? If so, this should be explained in the commit > message. If not, the change is wrong (quite possibly, using e.g. > 1 << 48 might be an alternative then, but it needs to be clarified > what particular overflow is possible here, and that whatever new > value gets chosen doesn''t trigger any).I agree with Jan: we could use a good comment there. Other then that, ack.
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, March 15, 2012 4:41 PM > To: Zhang, Yang Z > Cc: Ian Jackson; ''Stefano Stabellini(stefano.stabellini@eu.citrix.com)''; xen-devel > Subject: Re: [PATCH v2] use INT64_MAX as max expiration > > >>> On 15.03.12 at 01:57, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: > > Change from v1: > > According to Jan and Stefano''s comments, drop the previous logic which > > setting the value in an arbitrary way and use Stefano''s suggestion instead. > > > > Currently, the max expiration time is 2147483647ns(INT32_MAX ns)n This is > > enough when guest is busy, but when guest is idle, the next timer will be > > later than INT32_MAX ns. And those meaningless alarm will harm the pkg > > C-state. > > > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > > --- > > vl.c | 11 ++++++----- > > 1 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/vl.c b/vl.c > > index be8587a..a9b7a0d 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -1410,8 +1410,7 @@ static int64_t qemu_next_deadline(void) > > delta = active_timers[QEMU_TIMER_VIRTUAL]->expire_time - > > qemu_get_clock(vm_clock); > > } else { > > - /* To avoid problems with overflow limit this to 2^32. */ > > - delta = INT32_MAX; > > + delta = INT64_MAX; > > You''re silently removing the comment - was it wrong (i.e. is there no > potential for overflow)? If so, this should be explained in the commit > message. If not, the change is wrong (quite possibly, using e.g. > 1 << 48 might be an alternative then, but it needs to be clarified > what particular overflow is possible here, and that whatever new > value gets chosen doesn''t trigger any). >In fact, the original patch is using INT64_MAX too. When introducing instruction count support, it try to round up the next tick(like: (delta + 999) / 1000), and it will overflow if still using INT64_MAX, so it choose INT32_MAX instead. Now, we use ( (delta / 1000) + (delta % 1000 > 0 ? 1 : 0) and the overflow will not happen with this, so I removed the comments. best regards yang