2013-04-04 17:35:47

by Dirk Brandewie

[permalink] [raw]
Subject: [PATCH] cpufreq/intel_pstate: Set timer timeout correctly

From: Dirk Brandewie <[email protected]>

The current calculation of the delay time is wrong and a cut and paste
error from a previous experimental driver. This can result in the
timeout being set to jiffies + 1 which setup the driver to race with
it's self if the apic timer interrupt happen at just the right time.


https://bugzilla.redhat.com/show_bug.cgi?id=920289

Reported-by: Adam Williamson <[email protected]>
Reported-by: Parag Warudkar <[email protected]>

Signed-off-by: Dirk Brandewie <[email protected]>
---
drivers/cpufreq/intel_pstate.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 43ffe1c..4d6b988 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -502,7 +502,6 @@ static inline void intel_pstate_set_sample_time(struct cpudata *cpu)

sample_time = cpu->pstate_policy->sample_rate_ms;
delay = msecs_to_jiffies(sample_time);
- delay -= jiffies % delay;
mod_timer_pinned(&cpu->timer, jiffies + delay);
}

--
1.7.7.6


2013-04-05 05:19:17

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH] cpufreq/intel_pstate: Set timer timeout correctly

On Thu, Apr 4, 2013 at 11:05 PM, <[email protected]> wrote:
> From: Dirk Brandewie <[email protected]>
>
> The current calculation of the delay time is wrong and a cut and paste
> error from a previous experimental driver. This can result in the
> timeout being set to jiffies + 1 which setup the driver to race with
> it's self if the apic timer interrupt happen at just the right time.
>
>
> https://bugzilla.redhat.com/show_bug.cgi?id=920289
>
> Reported-by: Adam Williamson <[email protected]>
> Reported-by: Parag Warudkar <[email protected]>
>
> Signed-off-by: Dirk Brandewie <[email protected]>
> ---
> drivers/cpufreq/intel_pstate.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)

Looks fine, but I would like to see a Tested-by from Adam/Parag
as they haven't said anything about this patch (even in bugzilla).

2013-04-05 05:43:13

by Adam Williamson

[permalink] [raw]
Subject: Re: [PATCH] cpufreq/intel_pstate: Set timer timeout correctly

On 04/04/13 10:19 PM, Viresh Kumar wrote:
> On Thu, Apr 4, 2013 at 11:05 PM, <[email protected]> wrote:
>> From: Dirk Brandewie <[email protected]>
>>
>> The current calculation of the delay time is wrong and a cut and paste
>> error from a previous experimental driver. This can result in the
>> timeout being set to jiffies + 1 which setup the driver to race with
>> it's self if the apic timer interrupt happen at just the right time.
>>
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=920289
>>
>> Reported-by: Adam Williamson <[email protected]>
>> Reported-by: Parag Warudkar <[email protected]>
>>
>> Signed-off-by: Dirk Brandewie <[email protected]>
>> ---
>> drivers/cpufreq/intel_pstate.c | 1 -
>> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> Looks fine, but I would like to see a Tested-by from Adam/Parag
> as they haven't said anything about this patch (even in bugzilla).

I'll try. Note the bug is not reliably reproducible, all I can really do
is run for a day or two and see if it crashes.
--
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | identi.ca: adamwfedora
http://www.happyassassin.net

2013-04-05 23:46:16

by Parag Warudkar

[permalink] [raw]
Subject: Re: [PATCH] cpufreq/intel_pstate: Set timer timeout correctly



On Fri, 5 Apr 2013, Viresh Kumar wrote:

> On Thu, Apr 4, 2013 at 11:05 PM, <[email protected]> wrote:
> > From: Dirk Brandewie <[email protected]>
> >
> > The current calculation of the delay time is wrong and a cut and paste
> > error from a previous experimental driver. This can result in the
> > timeout being set to jiffies + 1 which setup the driver to race with
> > it's self if the apic timer interrupt happen at just the right time.
> >
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=920289
> >
> > Reported-by: Adam Williamson <[email protected]>
> > Reported-by: Parag Warudkar <[email protected]>
> >
> > Signed-off-by: Dirk Brandewie <[email protected]>
> > ---
> > drivers/cpufreq/intel_pstate.c | 1 -
> > 1 files changed, 0 insertions(+), 1 deletions(-)
>
> Looks fine, but I would like to see a Tested-by from Adam/Parag
> as they haven't said anything about this patch (even in bugzilla).
>

I am running with the patch since yesterday - everything looks good.

The issue hasn't been reproducible on demand but some code reading and
Dirk's explanation says the patch should fix the issue.

So - Tested-by: Parag Warudkar <[email protected]>

Parag