| Message ID | 1416562192-14114-1-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 11/21/2014 04:05 PM, Rafael J. Wysocki wrote: > On Friday, November 21, 2014 10:29:51 AM Daniel Lezcano wrote: >> The commit 8e92b6605d introduced the TIME_VALID flag for the C1 state >> if this one is a mwait state assuming the interrupt will be enabled >> before reading the end time of the idle state. >> >> The changelog of this commit mention a potential problem with the menu >> governor but not a real observation and I assume it described an old >> code as the commit is from 2008. >> >> I have been digging through the code and I didn't find any place where the >> interrupts are enabled before reading the time. Moreover with the changes >> in the meantime, we moved the time measurements in the cpuidle core as well >> as the interrupts enabling making sure the time is measured before the >> interrupt are enabled again in a single place. >> >> Remove this test as the time measurement is always valid for this >> state. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > Well, I need Len to have a look at this. Ok thanks. If you have time, is it possible also you have a look at the patchset I sent : [PATCH V3 0/6] sched: idle: cpuidle: cleanups and fixes and give your opinion about the poll state Peter and I we were discussing ? Regards -- Daniel >> --- >> drivers/acpi/processor_idle.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c >> index 380b4b4..7afba40 100644 >> --- a/drivers/acpi/processor_idle.c >> +++ b/drivers/acpi/processor_idle.c >> @@ -985,8 +985,6 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr) >> state->flags = 0; >> switch (cx->type) { >> case ACPI_STATE_C1: >> - if (cx->entry_method != ACPI_CSTATE_FFH) >> - state->flags |= CPUIDLE_FLAG_TIME_INVALID; >> >> state->enter = acpi_idle_enter_c1; >> state->enter_dead = acpi_idle_play_dead; >> >
On Friday, November 21, 2014 10:29:51 AM Daniel Lezcano wrote: > The commit 8e92b6605d introduced the TIME_VALID flag for the C1 state > if this one is a mwait state assuming the interrupt will be enabled > before reading the end time of the idle state. > > The changelog of this commit mention a potential problem with the menu > governor but not a real observation and I assume it described an old > code as the commit is from 2008. > > I have been digging through the code and I didn't find any place where the > interrupts are enabled before reading the time. Moreover with the changes > in the meantime, we moved the time measurements in the cpuidle core as well > as the interrupts enabling making sure the time is measured before the > interrupt are enabled again in a single place. > > Remove this test as the time measurement is always valid for this > state. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Well, I need Len to have a look at this. > --- > drivers/acpi/processor_idle.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 380b4b4..7afba40 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -985,8 +985,6 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr) > state->flags = 0; > switch (cx->type) { > case ACPI_STATE_C1: > - if (cx->entry_method != ACPI_CSTATE_FFH) > - state->flags |= CPUIDLE_FLAG_TIME_INVALID; > > state->enter = acpi_idle_enter_c1; > state->enter_dead = acpi_idle_play_dead; >
On 11/21/2014 04:01 PM, Daniel Lezcano wrote: > On 11/21/2014 04:05 PM, Rafael J. Wysocki wrote: >> On Friday, November 21, 2014 10:29:51 AM Daniel Lezcano wrote: >>> The commit 8e92b6605d introduced the TIME_VALID flag for the C1 state >>> if this one is a mwait state assuming the interrupt will be enabled >>> before reading the end time of the idle state. >>> >>> The changelog of this commit mention a potential problem with the menu >>> governor but not a real observation and I assume it described an old >>> code as the commit is from 2008. >>> >>> I have been digging through the code and I didn't find any place >>> where the >>> interrupts are enabled before reading the time. Moreover with the >>> changes >>> in the meantime, we moved the time measurements in the cpuidle core >>> as well >>> as the interrupts enabling making sure the time is measured before the >>> interrupt are enabled again in a single place. >>> >>> Remove this test as the time measurement is always valid for this >>> state. >>> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> >> Well, I need Len to have a look at this. Hi, any news on that ? Thanks -- Daniel > Ok thanks. > > If you have time, is it possible also you have a look at the patchset I > sent : > > [PATCH V3 0/6] sched: idle: cpuidle: cleanups and fixes > > and give your opinion about the poll state Peter and I we were discussing ? > > Regards > > -- Daniel > >>> --- >>> drivers/acpi/processor_idle.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/drivers/acpi/processor_idle.c >>> b/drivers/acpi/processor_idle.c >>> index 380b4b4..7afba40 100644 >>> --- a/drivers/acpi/processor_idle.c >>> +++ b/drivers/acpi/processor_idle.c >>> @@ -985,8 +985,6 @@ static int >>> acpi_processor_setup_cpuidle_states(struct acpi_processor *pr) >>> state->flags = 0; >>> switch (cx->type) { >>> case ACPI_STATE_C1: >>> - if (cx->entry_method != ACPI_CSTATE_FFH) >>> - state->flags |= CPUIDLE_FLAG_TIME_INVALID; >>> >>> state->enter = acpi_idle_enter_c1; >>> state->enter_dead = acpi_idle_play_dead; >>> >> > >
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 380b4b4..7afba40 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -985,8 +985,6 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr) state->flags = 0; switch (cx->type) { case ACPI_STATE_C1: - if (cx->entry_method != ACPI_CSTATE_FFH) - state->flags |= CPUIDLE_FLAG_TIME_INVALID; state->enter = acpi_idle_enter_c1; state->enter_dead = acpi_idle_play_dead;
The commit 8e92b6605d introduced the TIME_VALID flag for the C1 state if this one is a mwait state assuming the interrupt will be enabled before reading the end time of the idle state. The changelog of this commit mention a potential problem with the menu governor but not a real observation and I assume it described an old code as the commit is from 2008. I have been digging through the code and I didn't find any place where the interrupts are enabled before reading the time. Moreover with the changes in the meantime, we moved the time measurements in the cpuidle core as well as the interrupts enabling making sure the time is measured before the interrupt are enabled again in a single place. Remove this test as the time measurement is always valid for this state. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/acpi/processor_idle.c | 2 -- 1 file changed, 2 deletions(-)