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

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> 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.

>> > @@ -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.

>> > +
>> > +        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).

Jan


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