Prev: Remove 9 second reboot delay on Lenovo T400/T500
Next: [RFC] implement size_append in perl
From: Rik van Riel on 14 Jul 2010 10:50
On 07/12/2010 10:25 PM, Zachary Amsden wrote:
> This simplifies much of the init code; we can now simply always
> call tsc_khz_changed, optionally passing it a new value, or letting
> it figure out the existing value (while interrupts are disabled, and
> thus, by inference from the rule, not raceful against CPU hotplug or
> frequency updates, which will issue IPIs to the local CPU to perform
> this very same task).
>
> Signed-off-by: Zachary Amsden<zamsden(a)redhat.com>

Acked-by: Rik van Riel <riel(a)redhat.com>

--
All rights reversed
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Avi Kivity on 18 Jul 2010 10:50
On 07/13/2010 05:25 AM, Zachary Amsden wrote:
> This simplifies much of the init code; we can now simply always
> call tsc_khz_changed, optionally passing it a new value, or letting
> it figure out the existing value (while interrupts are disabled, and
> thus, by inference from the rule, not raceful against CPU hotplug or
> frequency updates, which will issue IPIs to the local CPU to perform
> this very same task).
>
>
> @@ -893,6 +893,15 @@ static void kvm_set_time_scale(uint32_t tsc_khz, struct pvclock_vcpu_time_info *
>
> static DEFINE_PER_CPU(unsigned long, cpu_tsc_khz);
>
> +static inline int kvm_tsc_changes_freq(void)
> +{
> + int cpu = get_cpu();
> + int ret = !boot_cpu_has(X86_FEATURE_CONSTANT_TSC)&&
> + cpufreq_quick_get(cpu) != 0;
> + put_cpu();
> + return ret;
> +}
> +
> void guest_write_tsc(struct kvm_vcpu *vcpu, u64 data)
> {
> struct kvm *kvm = vcpu->kvm;
> @@ -937,7 +946,7 @@ void guest_write_tsc(struct kvm_vcpu *vcpu, u64 data)
> }
> EXPORT_SYMBOL_GPL(guest_write_tsc);
>
> -static void kvm_write_guest_time(struct kvm_vcpu *v)
> +static int kvm_write_guest_time(struct kvm_vcpu *v)
> {
> struct timespec ts;
> unsigned long flags;
> @@ -946,24 +955,27 @@ static void kvm_write_guest_time(struct kvm_vcpu *v)
> unsigned long this_tsc_khz;
>
> if ((!vcpu->time_page))
> - return;
> -
> - this_tsc_khz = get_cpu_var(cpu_tsc_khz);
> - if (unlikely(vcpu->hv_clock_tsc_khz != this_tsc_khz)) {
> - kvm_set_time_scale(this_tsc_khz,&vcpu->hv_clock);
> - vcpu->hv_clock_tsc_khz = this_tsc_khz;
> - }
> - put_cpu_var(cpu_tsc_khz);
> + return 0;
>
> /* Keep irq disabled to prevent changes to the clock */
> local_irq_save(flags);
> kvm_get_msr(v, MSR_IA32_TSC,&vcpu->hv_clock.tsc_timestamp);
> ktime_get_ts(&ts);
> monotonic_to_bootbased(&ts);
> + this_tsc_khz = __get_cpu_var(cpu_tsc_khz);
> local_irq_restore(flags);
>
> - /* With all the info we got, fill in the values */
> + if (unlikely(this_tsc_khz == 0)) {
> + kvm_make_request(KVM_REQ_KVMCLOCK_UPDATE, v);
> + return 1;
> + }
>

Presumably, this will spin until cpufreq writes the frequency?

> @@ -4131,9 +4138,23 @@ int kvm_fast_pio_out(struct kvm_vcpu *vcpu, int size, unsigned short port)
> }
> EXPORT_SYMBOL_GPL(kvm_fast_pio_out);
>
> -static void bounce_off(void *info)
> +static void tsc_bad(void *info)
> +{
> + __get_cpu_var(cpu_tsc_khz) = 0;
> +}
> +
> +static void tsc_khz_changed(void *data)
> {
> - /* nothing */
> + struct cpufreq_freqs *freq = data;
> + unsigned long khz = 0;
> +
> + if (data)
> + khz = freq->new;
> + else if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
> + khz = cpufreq_quick_get(raw_smp_processor_id());
> + if (!khz)
> + khz = tsc_khz;
> + __get_cpu_var(cpu_tsc_khz) = khz;
> }
>

Do we really need to cache cpufreq_quick_get()? If it's really quick,
why not just use it everywhere instead of cacheing it? Not a comment on
this patch.

--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Zachary Amsden on 19 Jul 2010 16:10
On 07/18/2010 04:45 AM, Avi Kivity wrote:
> On 07/13/2010 05:25 AM, Zachary Amsden wrote:
>> This simplifies much of the init code; we can now simply always
>> call tsc_khz_changed, optionally passing it a new value, or letting
>> it figure out the existing value (while interrupts are disabled, and
>> thus, by inference from the rule, not raceful against CPU hotplug or
>> frequency updates, which will issue IPIs to the local CPU to perform
>> this very same task).
>>
>>
>> @@ -893,6 +893,15 @@ static void kvm_set_time_scale(uint32_t tsc_khz,
>> struct pvclock_vcpu_time_info *
>>
>> static DEFINE_PER_CPU(unsigned long, cpu_tsc_khz);
>>
>> +static inline int kvm_tsc_changes_freq(void)
>> +{
>> + int cpu = get_cpu();
>> + int ret = !boot_cpu_has(X86_FEATURE_CONSTANT_TSC)&&
>> + cpufreq_quick_get(cpu) != 0;
>> + put_cpu();
>> + return ret;
>> +}
>> +
>> void guest_write_tsc(struct kvm_vcpu *vcpu, u64 data)
>> {
>> struct kvm *kvm = vcpu->kvm;
>> @@ -937,7 +946,7 @@ void guest_write_tsc(struct kvm_vcpu *vcpu, u64
>> data)
>> }
>> EXPORT_SYMBOL_GPL(guest_write_tsc);
>>
>> -static void kvm_write_guest_time(struct kvm_vcpu *v)
>> +static int kvm_write_guest_time(struct kvm_vcpu *v)
>> {
>> struct timespec ts;
>> unsigned long flags;
>> @@ -946,24 +955,27 @@ static void kvm_write_guest_time(struct
>> kvm_vcpu *v)
>> unsigned long this_tsc_khz;
>>
>> if ((!vcpu->time_page))
>> - return;
>> -
>> - this_tsc_khz = get_cpu_var(cpu_tsc_khz);
>> - if (unlikely(vcpu->hv_clock_tsc_khz != this_tsc_khz)) {
>> - kvm_set_time_scale(this_tsc_khz,&vcpu->hv_clock);
>> - vcpu->hv_clock_tsc_khz = this_tsc_khz;
>> - }
>> - put_cpu_var(cpu_tsc_khz);
>> + return 0;
>>
>> /* Keep irq disabled to prevent changes to the clock */
>> local_irq_save(flags);
>> kvm_get_msr(v, MSR_IA32_TSC,&vcpu->hv_clock.tsc_timestamp);
>> ktime_get_ts(&ts);
>> monotonic_to_bootbased(&ts);
>> + this_tsc_khz = __get_cpu_var(cpu_tsc_khz);
>> local_irq_restore(flags);
>>
>> - /* With all the info we got, fill in the values */
>> + if (unlikely(this_tsc_khz == 0)) {
>> + kvm_make_request(KVM_REQ_KVMCLOCK_UPDATE, v);
>> + return 1;
>> + }
>
> Presumably, this will spin until cpufreq writes the frequency?

Only during CPU re-add; we can only get here if running before cpu
notifiers have told us about the new CPU.

>
>> @@ -4131,9 +4138,23 @@ int kvm_fast_pio_out(struct kvm_vcpu *vcpu,
>> int size, unsigned short port)
>> }
>> EXPORT_SYMBOL_GPL(kvm_fast_pio_out);
>>
>> -static void bounce_off(void *info)
>> +static void tsc_bad(void *info)
>> +{
>> + __get_cpu_var(cpu_tsc_khz) = 0;
>> +}
>> +
>> +static void tsc_khz_changed(void *data)
>> {
>> - /* nothing */
>> + struct cpufreq_freqs *freq = data;
>> + unsigned long khz = 0;
>> +
>> + if (data)
>> + khz = freq->new;
>> + else if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
>> + khz = cpufreq_quick_get(raw_smp_processor_id());
>> + if (!khz)
>> + khz = tsc_khz;
>> + __get_cpu_var(cpu_tsc_khz) = khz;
>> }
>
> Do we really need to cache cpufreq_quick_get()? If it's really quick,
> why not just use it everywhere instead of cacheing it? Not a comment
> on this patch.
>

If cpufreq is compiled in, but disabled, it returns zero, so we need
some sort of logic.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Avi Kivity on 20 Jul 2010 05:00
On 07/19/2010 11:06 PM, Zachary Amsden wrote:
>>> +static void tsc_khz_changed(void *data)
>>> {
>>> - /* nothing */
>>> + struct cpufreq_freqs *freq = data;
>>> + unsigned long khz = 0;
>>> +
>>> + if (data)
>>> + khz = freq->new;
>>> + else if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
>>> + khz = cpufreq_quick_get(raw_smp_processor_id());
>>> + if (!khz)
>>> + khz = tsc_khz;
>>> + __get_cpu_var(cpu_tsc_khz) = khz;
>>> }
>>
>> Do we really need to cache cpufreq_quick_get()? If it's really
>> quick, why not just use it everywhere instead of cacheing it? Not a
>> comment on this patch.
>>
>
>
> If cpufreq is compiled in, but disabled, it returns zero, so we need
> some sort of logic.

Maybe it's better to put it into cpufreq_quick_get(). Inconsistent APIs
that appear to work are bad.

--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Zachary Amsden on 20 Jul 2010 18:00
On 07/19/2010 10:53 PM, Avi Kivity wrote:
> On 07/19/2010 11:06 PM, Zachary Amsden wrote:
>>>> +static void tsc_khz_changed(void *data)
>>>> {
>>>> - /* nothing */
>>>> + struct cpufreq_freqs *freq = data;
>>>> + unsigned long khz = 0;
>>>> +
>>>> + if (data)
>>>> + khz = freq->new;
>>>> + else if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
>>>> + khz = cpufreq_quick_get(raw_smp_processor_id());
>>>> + if (!khz)
>>>> + khz = tsc_khz;
>>>> + __get_cpu_var(cpu_tsc_khz) = khz;
>>>> }
>>>
>>> Do we really need to cache cpufreq_quick_get()? If it's really
>>> quick, why not just use it everywhere instead of cacheing it? Not a
>>> comment on this patch.
>>>
>>
>>
>> If cpufreq is compiled in, but disabled, it returns zero, so we need
>> some sort of logic.
>
> Maybe it's better to put it into cpufreq_quick_get(). Inconsistent
> APIs that appear to work are bad.
>

I don't think it's quite so simple; cpufreq is platform independent and
tsc_khz is a platform specific export. It seems cpufreq is designed to
return zero when disabled and we're the unusual ones for wanting to use it.

Zach
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: Remove 9 second reboot delay on Lenovo T400/T500
Next: [RFC] implement size_append in perl