linux-kernel - Re: [PATCH 2/3] cpuidle,menu: use interactivity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56977980.8080009@arm.com>
Date:	Thu, 14 Jan 2016 10:33:36 +0000
From:	Sudeep Holla <sudeep.holla@....com>
To:	"Rafael J. Wysocki" <rafael@...nel.org>
Cc:	Sudeep Holla <sudeep.holla@....com>, riel@...hat.com,
	Rafael Wysocki <rafael.j.wysocki@...el.com>,
	Linux PM list <linux-pm@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	arjan@...ux.intel.com, Len Brown <len.brown@...el.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>
Subject: Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable
 polling



On 13/01/16 21:58, Rafael J. Wysocki wrote:
> Hi,
>
> On Wed, Jan 13, 2016 at 6:27 PM, Sudeep Holla <sudeep.holla@....com> wrote:
>> Hi Rik,
>>
>> This change break idle on ARM64(may be on other ARM?) platform.
>> Sorry for reporting late, but missed to check cpuidle in -next.
>
> OK, so first of all, how exactly is idle broken on those systems?
>

Sorry for the hasty bug report.

> Do they crash or does something different happen?  If something
> different happens, then what's that?
>

No they just don't enter any idle states(as if CPUIdle was disabled)

[...]

>>
>> diff --git i/drivers/cpuidle/governors/menu.c w/drivers/cpuidle/governors/menu.c
>> index 7b0971d97cc3..7c7f4059705a 100644
>> --- i/drivers/cpuidle/governors/menu.c
>> +++ w/drivers/cpuidle/governors/menu.c
>> @@ -330,7 +330,8 @@ static int menu_select(struct cpuidle_driver *drv,
>> struct cpuidle_device *dev)
>>           * We want to default to C1 (hlt), not to busy polling
>>           * unless the timer is happening really really soon.
>>           */
>> -       if (interactivity_req > 20 &&
>> +       if (((interactivity_req && interactivity_req > 20) ||
>
> Well, if interactivity_req > 20, then it also is different from 0, so
> the first check should not be necessary here.
>

True, I just wanted to avoid using interactivity_req when 0, it was just
a quick hack.

>> +               data->next_timer_us > 20) &&
>
> I guess that this simply restores the previous behavior on the
> platforms in question.
>

Yes.

> Now, the reason why it may matter is because
> CPUIDLE_DRIVER_STATE_START is 0 and so data->last_state_idx may end up
> as -1 on them.  So I think this piece of code only ever makes sense if
> CPUIDLE_DRIVER_STATE_START is 1.
>

That's correct.

-- 
Regards,
Sudeep