| 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
| ||
|
Date: Mon, 21 Mar 2016 19:14:44 +0700
From: Vladyslav Frolov <frolvlad@...il.com>
To: Zhang Rui <rui.zhang@...el.com>
Cc: Laura Abbott <labbott@...hat.com>,
Javi Merino <javi.merino@....com>,
Chen Yu <yu.c.chen@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Manuel Krause <manuelkrause@...scape.net>,
szegad <szegadlo@...zta.onet.pl>, prash <prash.n.rao@...il.com>,
amish <ammdispose-arch@...oo.com>,
Matthias <morpheusxyz123@...oo.de>, linux-pm@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [REGRESSION] 774ac8b7eff6 ("Thermal: initialize thermal zone
device correctly") causes performance drop
Hi Rui,
The patch seems to work fine! I have tried both `rmmod & modprobe
thermal` and suspend & resume. Great!
On 21 March 2016 at 18:14, Vladyslav Frolov <frolvlad@...il.com> wrote:
> Hi Rui,
>
> I'm sorry for the delay; Reading the e-mail the first time I have
> missed the part about the patch, so I'm building the kernel now.
>
> By the way, the patch reported a slight offset missmatch:
>
> patching file drivers/thermal/thermal_core.c
> Hunk #2 succeeded at 1800 (offset -4 lines).
> Hunk #3 succeeded at 1872 (offset -4 lines).
>
> Is this fine?
>
>
> On 21 March 2016 at 18:09, Vladyslav Frolov <frolvlad@...il.com> wrote:
>> Hi Rui,
>>
>> I'm sorry for the delay; Reading the e-mail the first time I have missed the
>> part about the patch, so I'm building the kernel now.
>>
>> By the way, the patch reported a slight offset missmatch:
>>
>> patching file drivers/thermal/thermal_core.c
>> Hunk #2 succeeded at 1800 (offset -4 lines).
>> Hunk #3 succeeded at 1872 (offset -4 lines).
>>
>> Is this fine?
>>
>>
>>
>> On 19 March 2016 at 11:43, Zhang Rui <rui.zhang@...el.com> wrote:
>>>
>>>
>>> Hi, all,
>>>
>>> On Wed, 2016-03-16 at 15:27 -0700, Laura Abbott wrote:
>>> > Hi,
>>> >
>>> > Fedora received a bug report
>>> > (https://bugzilla.redhat.com/show_bug.cgi?id=1317190)
>>> > of a major performance drop on various bench marks and general system
>>> > sluggishness with the 4.4.4 kernel update. The benchmarks were showing
>>> > a reduction to about 18% performance (not minor).
>>> >
>>> > Bisection showed the first bad commit was
>>> >
>>> > commit 774ac8b7eff69e0786970157de2157e68b22f456
>>> > Author: Zhang Rui <rui.zhang@...el.com>
>>> > Date: Fri Oct 30 16:31:47 2015 +0800
>>> >
>>> > Thermal: initialize thermal zone device correctly
>>> >
>>> > commit bb431ba26c5cd0a17c941ca6c3a195a3a6d5d461 upstream.
>>> >
>>> > After thermal zone device registered, as we have not read any
>>> > temperature before, thus tz->temperature should not be 0,
>>> > which actually means 0C, and thermal trend is not available.
>>> > In this case, we need specially handling for the first
>>> > thermal_zone_device_update().
>>> >
>>> > Both thermal core framework and step_wise governor is
>>> > enhanced to handle this. And since the step_wise governor
>>> > is the only one that uses trends, so it's the only thermal
>>> > governor that needs to be updated.
>>> >
>>> > Tested-by: Manuel Krause <manuelkrause@...scape.net>
>>> > Tested-by: szegad <szegadlo@...zta.onet.pl>
>>> > Tested-by: prash <prash.n.rao@...il.com>
>>> > Tested-by: amish <ammdispose-arch@...oo.com>
>>> > Tested-by: Matthias <morpheusxyz123@...oo.de>
>>> > Reviewed-by: Javi Merino <javi.merino@....com>
>>> > Signed-off-by: Zhang Rui <rui.zhang@...el.com>
>>> > Signed-off-by: Chen Yu <yu.c.chen@...el.com>
>>> > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>>>
>>> According to https://bugzilla.redhat.com/show_bug.cgi?id=1317190#c30
>>> and https://bugzilla.kernel.org/show_bug.cgi?id=114551,
>>> the root cause of the problem is that
>>> 1. BIOS reports bogus passive trip point value, which is 0 degree
>>> Celsius
>>> 2. Without the thermal patches, thermal core does nothing upon driver
>>> probe, thus processor cooling states are not changed.
>>> 3. With the thermal patches applied, thermal core checks the new
>>> registered thermal zone driver when it is probed, and set the cooling
>>> devices to proper state, for example, fan device could be spinning
>>> during boot, even if the system is cool.
>>> 4. Thus, on these Lenovo laptops, processor cooling devices are
>>> throttled because the current temperature (around 50C) is higher than
>>> the passive trip point (0C), and the thermal zone is considered as
>>> overheating.
>>>
>>> In order to workaround this bogus BIOS, we should disable those invalid
>>> trip points by checking the trip point value, like the patch below,
>>> Laura and Vlad,
>>> can you please try the patch below and confirm if it fixes the problem
>>> for you?
>>>
>>> From 81ad4276b505e987dd8ebbdf63605f92cd172b52 Mon Sep 17 00:00:00 2001
>>> From: Zhang Rui <rui.zhang@...el.com>
>>> Date: Fri, 18 Mar 2016 10:03:24 +0800
>>> Subject: [PATCH] Thermal: Ignore invalid trip points
>>>
>>> In some cases, platform thermal driver may report invalid trip points,
>>> thermal core should not take any action for these trip points.
>>>
>>> This fixed a regression that bogus trip point starts to screw up thermal
>>> control on some Lenovo laptops, after
>>> commit bb431ba26c5cd0a17c941ca6c3a195a3a6d5d461
>>> Author: Zhang Rui <rui.zhang@...el.com>
>>> Date: Fri Oct 30 16:31:47 2015 +0800
>>>
>>> Thermal: initialize thermal zone device correctly
>>>
>>> After thermal zone device registered, as we have not read any
>>> temperature before, thus tz->temperature should not be 0,
>>> which actually means 0C, and thermal trend is not available.
>>> In this case, we need specially handling for the first
>>> thermal_zone_device_update().
>>>
>>> Both thermal core framework and step_wise governor is
>>> enhanced to handle this. And since the step_wise governor
>>> is the only one that uses trends, so it's the only thermal
>>> governor that needs to be updated.
>>>
>>> Tested-by: Manuel Krause <manuelkrause@...scape.net>
>>> Tested-by: szegad <szegadlo@...zta.onet.pl>
>>> Tested-by: prash <prash.n.rao@...il.com>
>>> Tested-by: amish <ammdispose-arch@...oo.com>
>>> Tested-by: Matthias <morpheusxyz123@...oo.de>
>>> Reviewed-by: Javi Merino <javi.merino@....com>
>>> Signed-off-by: Zhang Rui <rui.zhang@...el.com>
>>> Signed-off-by: Chen Yu <yu.c.chen@...el.com>
>>>
>>> CC: <stable@...r.kernel.org> #3.18+
>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1317190
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=114551
>>> Signed-off-by: Zhang Rui <rui.zhang@...el.com>
>>> ---
>>> drivers/thermal/thermal_core.c | 13 ++++++++++++-
>>> include/linux/thermal.h | 2 ++
>>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/thermal/thermal_core.c
>>> b/drivers/thermal/thermal_core.c
>>> index a0a8fd1..d4b5465 100644
>>> --- a/drivers/thermal/thermal_core.c
>>> +++ b/drivers/thermal/thermal_core.c
>>> @@ -454,6 +454,10 @@ static void handle_thermal_trip(struct
>>> thermal_zone_device *tz, int trip)
>>> {
>>> enum thermal_trip_type type;
>>>
>>> + /* Ignore disabled trip points */
>>> + if (test_bit(trip, &tz->trips_disabled))
>>> + return;
>>> +
>>> tz->ops->get_trip_type(tz, trip, &type);
>>>
>>> if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
>>> @@ -1800,6 +1804,7 @@ struct thermal_zone_device
>>> *thermal_zone_device_register(const char *type,
>>> {
>>> struct thermal_zone_device *tz;
>>> enum thermal_trip_type trip_type;
>>> + int trip_temp;
>>> int result;
>>> int count;
>>> int passive = 0;
>>> @@ -1871,9 +1876,15 @@ struct thermal_zone_device
>>> *thermal_zone_device_register(const char *type,
>>> goto unregister;
>>>
>>> for (count = 0; count < trips; count++) {
>>> - tz->ops->get_trip_type(tz, count, &trip_type);
>>> + if (tz->ops->get_trip_type(tz, count, &trip_type))
>>> + set_bit(count, &tz->trips_disabled);
>>> if (trip_type == THERMAL_TRIP_PASSIVE)
>>> passive = 1;
>>> + if (tz->ops->get_trip_temp(tz, count, &trip_temp))
>>> + set_bit(count, &tz->trips_disabled);
>>> + /* Check for bogus trip points */
>>> + if (trip_temp == 0)
>>> + set_bit(count, &tz->trips_disabled);
>>> }
>>>
>>> if (!passive) {
>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>> index 9c48199..a55d052 100644
>>> --- a/include/linux/thermal.h
>>> +++ b/include/linux/thermal.h
>>> @@ -156,6 +156,7 @@ struct thermal_attr {
>>> * @trip_hyst_attrs: attributes for trip points for sysfs: trip
>>> hysteresis
>>> * @devdata: private pointer for device private data
>>> * @trips: number of trip points the thermal zone supports
>>> + * @trips_disabled; bitmap for disabled trips
>>> * @passive_delay: number of milliseconds to wait between polls when
>>> * performing passive cooling.
>>> * @polling_delay: number of milliseconds to wait between polls when
>>> @@ -191,6 +192,7 @@ struct thermal_zone_device {
>>> struct thermal_attr *trip_hyst_attrs;
>>> void *devdata;
>>> int trips;
>>> + unsigned long trips_disabled; /* bitmap for disabled trips */
>>> int passive_delay;
>>> int polling_delay;
>>> int temperature;
>>> --
>>> 1.9.1
>>>
>>>
>>>
>>
Powered by blists - more mailing lists