Patchwork [3/3] alleviate time drift with HPET periodic timers

login
register
mail settings
Submitter Ulrich Obergfell
Date March 18, 2011, 3:55 p.m.
Message ID <946357411.421760.1300463741632.JavaMail.root@zmail07.collab.prod.int.phx2.redhat.com>
Download mbox | patch
Permalink /patch/87543/
State New
Headers show

Comments

Ulrich Obergfell - March 18, 2011, 3:55 p.m.
Part 3 of the patch implements the following options for the 'configure' script.

--disable-hpet-driftfix
--enable-hpet-driftfix

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
Jan Kiszka - March 19, 2011, 9:44 a.m.
On 2011-03-18 16:55, Ulrich Obergfell wrote:
> 
> Part 3 of the patch implements the following options for the 'configure' script.
> 
> --disable-hpet-driftfix
> --enable-hpet-driftfix

I see no benefit in this configurability. Just make the driftfix
unconditionally available, runtime-disabled by default for now until it
matured and there is no downside in enabling it all the time.

Jan
Ulrich Obergfell - March 22, 2011, 10:03 a.m.
>> Part 3 of the patch implements the following options for the
>> 'configure' script.
>>
>> --disable-hpet-driftfix
>> --enable-hpet-driftfix
> 
> I see no benefit in this configurability. Just make the driftfix
> unconditionally available, runtime-disabled by default for now until it
> matured and there is no downside in enabling it all the time.


Many Thanks Jan,

I enclosed the code in '#ifdef CONFIG_HPET_DRIFTFIX ... #endif'
so that it can be easily identified (and removed if the generic API
would be implemented some day). Since the ifdef's are already there
I added the configuration option for convenience. As you don't see
any benefit in this option, I can remove that part of the patch.
However, I'd suggest to keep the ifdef's and do the following:

- Rename to '#ifdef HPET_DRIFTFIX ... #endif' to make it clear that
  this is not controlled via a configuration option.

- Add '#define HPET_DRIFTFIX' to hw/hpet_emul.h.

Do you agree ?


Regards,

Uli
Jan Kiszka - March 22, 2011, 10:08 a.m.
On 2011-03-22 11:03, Ulrich Obergfell wrote:
> 
>>> Part 3 of the patch implements the following options for the
>>> 'configure' script.
>>>
>>> --disable-hpet-driftfix
>>> --enable-hpet-driftfix
>>
>> I see no benefit in this configurability. Just make the driftfix
>> unconditionally available, runtime-disabled by default for now until it
>> matured and there is no downside in enabling it all the time.
> 
> 
> Many Thanks Jan,
> 
> I enclosed the code in '#ifdef CONFIG_HPET_DRIFTFIX ... #endif'
> so that it can be easily identified (and removed if the generic API
> would be implemented some day). Since the ifdef's are already there
> I added the configuration option for convenience. As you don't see
> any benefit in this option, I can remove that part of the patch.
> However, I'd suggest to keep the ifdef's and do the following:
> 
> - Rename to '#ifdef HPET_DRIFTFIX ... #endif' to make it clear that
>   this is not controlled via a configuration option.
> 
> - Add '#define HPET_DRIFTFIX' to hw/hpet_emul.h.
> 
> Do you agree ?

Thanks to versioning control and feature-oriented commits, it's not very
hard to identify what code changes relate to which feature additions. So
I still don't see a need for that.

Jan

Patch

diff -up ./configure.orig3 ./configure
--- ./configure.orig3	2011-02-18 22:48:06.000000000 +0100
+++ ./configure	2011-03-13 12:43:44.870966181 +0100
@@ -140,6 +140,7 @@  linux_aio=""
 attr=""
 vhost_net=""
 xfs=""
+hpet_driftfix="no"
 
 gprof="no"
 debug_tcg="no"
@@ -749,6 +750,10 @@  for opt do
   ;;
   --enable-rbd) rbd="yes"
   ;;
+  --disable-hpet-driftfix) hpet_driftfix="no"
+  ;;
+  --enable-hpet-driftfix) hpet_driftfix="yes"
+  ;;
   *) echo "ERROR: unknown option $opt"; show_help="yes"
   ;;
   esac
@@ -2602,6 +2607,7 @@  echo "Trace output file $trace_file-<pid
 echo "spice support     $spice"
 echo "rbd support       $rbd"
 echo "xfsctl support    $xfs"
+echo "HPET drift fix    $hpet_driftfix"
 
 if test $sdl_too_old = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -2893,6 +2899,10 @@  if test "$rbd" = "yes" ; then
   echo "CONFIG_RBD=y" >> $config_host_mak
 fi
 
+if test "$hpet_driftfix" = "yes" ; then
+  echo "CONFIG_HPET_DRIFTFIX=y" >> $config_host_mak
+fi
+
 # USB host support
 case "$usb" in
 linux)