RE: [Xen-devel] [PATCH] X86: Prefer TSC-deadline timer in Xen

["Wei, Gang" <gang.wei@xxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxxxx], October 28, 2010 7:43 PM
> >>> On 28.10.10 at 11:48, "Wei, Gang" <gang.wei@xxxxxxxxx> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@xxxxxxxxxx], Thursday, October 28,
> 2010
> > 3:46 PM
> >> >>> On 28.10.10 at 07:45, "Wei, Gang" <gang.wei@xxxxxxxxx> wrote:
> >> > --- a/xen/arch/x86/apic.c        Wed Oct 20 17:26:51 2010 +0100
> >> > +++ b/xen/arch/x86/apic.c        Fri Oct 29 19:24:56 2010 +0800
> >> > @@ -37,6 +37,15 @@
> >> >  #include <asm/asm_defns.h> /* for BUILD_SMP_INTERRUPT */
> >> >  #include <mach_apic.h>
> >> >  #include <io_ports.h>
> >> > +
> >> > +#define APIC_TIMER_MODE_ONESHOT         (0 << 17)
> >> > +#define APIC_TIMER_MODE_PERIODIC        (1 << 17)
> >> > +#define APIC_TIMER_MODE_TSC_DEADLINE    (2 << 17)
> >> > +#define APIC_TIMER_MODE_MASK            (3 << 17)
> >> > +
> >> > +static int tdt_enabled;
> >> > +static int tdt_disable;
> >> > +boolean_param("tdt_off", tdt_disable);
> >>
> >> It would be more natural to call the parameter just "tdt", and
> >> use a non-zero initialized variable that gets set to zero when
> >> the user passes "tdt=off" (or another of the boolean false
> >> indicators). Perhaps you could even get away with just the
> >> single "tdt_enabled" variable then.
> >
> > Rename the parameter should be ok. But I prefer to keep two variable there
> > to avoid check both tdt_enabled &
> boot_cpu_has(X86_FEATURE_TSC_DEADLINE)
> > everywhere.
> 
> Why? Just clear tdt_enabled when you find
> !boot_cpu_has(X86_FEATURE_TSC_DEADLINE) during initialization.
> 
> And btw., this (or if you really want to keep them separate, both)
> variable(s) are pretty reasonable candidates for __read_mostly.

I still want to keep them because __setup_APIC_LVTT() will be called multiple 
times - the first call with tdt_enabled == false, and the following calls with 
tdt_enabled == true. I will add __read_mostly for the two variables.

> 
> >> > @@ -1360,12 +1382,24 @@ int reprogram_timer(s_time_t timeout)
> >> >      if ( !cpu_has_apic )
> >> >          return 1;
> >> >
> >> > -    if ( timeout && ((expire = timeout - NOW()) > 0) )
> >> > -        apic_tmict = min_t(u64, (bus_scale * expire) >> 18, UINT_MAX);
> >> > -
> >> > -    apic_write(APIC_TMICT, (unsigned long)apic_tmict);
> >> > -
> >> > -    return apic_tmict || !timeout;
> >> > +    if ( tdt_enabled )
> >> > +    {
> >> > +        u64 tsc = 0;
> >>
> >> Is zero really a proper "no-timeout" indicator here?
> >
> > Yes, it is. Writing zero to MSR_IA32_TSC_DEADLINE will disarm the tdt
> > according to SDM.
> 
> Okay, then (albeit unlikely) you should check that you don't
> unintentionally write zero into the MSR.

I am sure this MSR will only be written with zero while timeout is 0.

> 
> >> > +
> >> > +        if ( timeout )
> >> > +            tsc = stime2tsc(timeout);
> >> > +
> >> > +        wrmsrl(MSR_IA32_TSC_DEADLINE, tsc);
> >> > +    }
> >> > +    else
> >> > +    {
> >> > +        if ( timeout && ((expire = timeout - NOW()) > 0) )
> >> > +            apic_tmict = min_t(u64, (bus_scale * expire) >> 18,
> >> UINT_MAX);
> >> > +
> >> > +        apic_write(APIC_TMICT, (unsigned long)apic_tmict);
> >> > +    }
> >> > +
> >> > +    return apic_tmict || !timeout || tdt_enabled;
> >>
> >> How can this always be successful if tdt_enabled?
> >
> > If tdt_enabled, there are only three cases: 1st, timeout=0, then write 0 to
> > tdt msr to stop timer, return successful; 2nd, timeout <= NOW(), a tsc value
> > less than or equal current tsc will be written to tdt msr, then a expiring
> > interrupt will be generated right now, return successful; 3rd, timeout >
> > NOW(), a tsc value > current tsc will be written to tdt msr, also return
> > successful. No need to return failed if tdt_enabled.
> 
> Ah, okay - I should have fully read the doc first. Sorry for the
> noise. Now rather than extending the return expression, why
> don't you "return 1" inside the if()'s body (and in that case
> you could leave the original code mostly unchanged since then
> you also don't need an else).

Yes, this is also a good comment. I will take it even it has the side effect of 
adding a return point in the middle of the function.

Jimmy

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel