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]
Date:   Thu, 16 Jun 2022 00:27:39 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Vadim Pasternak <vadimp@...dia.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux@...ck-us.net" <linux@...ck-us.net>,
        "rui.zhang@...el.com" <rui.zhang@...el.com>,
        "edubezval@...il.com" <edubezval@...il.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        Ido Schimmel <idosch@...dia.com>
Subject: Re: [patch net-next RFC v1] mlxsw: core: Add the hottest thermal zone
 detection


Hi Vadim,

On 16/06/2022 00:06, Vadim Pasternak wrote:
> Hi Daniel,
> 
>> -----Original Message-----
>> From: Daniel Lezcano <daniel.lezcano@...aro.org>
>> Sent: Wednesday, June 15, 2022 11:32 PM
>> To: Vadim Pasternak <vadimp@...dia.com>; davem@...emloft.net
>> Cc: netdev@...r.kernel.org; linux@...ck-us.net; rui.zhang@...el.com;
>> edubezval@...il.com; jiri@...nulli.us; Ido Schimmel <idosch@...dia.com>
>> Subject: Re: [patch net-next RFC v1] mlxsw: core: Add the hottest thermal
>> zone detection
>>
>>
>> Hi Vadim,
>>
>> On 29/05/2019 15:52, Vadim Pasternak wrote:
>>> When multiple sensors are mapped to the same cooling device, the
>>> cooling device should be set according the worst sensor from the
>>> sensors associated with this cooling device.
>>>
>>> Provide the hottest thermal zone detection and enforce cooling device
>>> to follow the temperature trends the hottest zone only.
>>> Prevent competition for the cooling device control from others zones,
>>> by "stable trend" indication. A cooling device will not perform any
>>> actions associated with a zone with "stable trend".
>>>
>>> When other thermal zone is detected as a hottest, a cooling device is
>>> to be switched to following temperature trends of new hottest zone.
>>>
>>> Thermal zone score is represented by 32 bits unsigned integer and
>>> calculated according to the next formula:
>>> For T < TZ<t><i>, where t from {normal trip = 0, high trip = 1, hot
>>> trip = 2, critical = 3}:
>>> TZ<i> score = (T + (TZ<t><i> - T) / 2) / (TZ<t><i> - T) * 256 ** j;
>>> Highest thermal zone score s is set as MAX(TZ<i>score); Following this
>>> formula, if TZ<i> is in trip point higher than TZ<k>, the higher score
>>> is to be always assigned to TZ<i>.
>>>
>>> For two thermal zones located at the same kind of trip point, the
>>> higher score will be assigned to the zone, which closer to the next trip
>> point.
>>> Thus, the highest score will always be assigned objectively to the
>>> hottest thermal zone.
>>
>> While reading the code I noticed this change and I was wondering why it was
>> needed.
>>
>> The thermal framework does already aggregates the mitigation decisions,
>> taking the highest cooling state [1].
>>
>> That allows for instance a spanning fan on a dual socket. Two thermal zones
>> for one cooling device.
> 
> Here the hottest thermal zone is calculated for different thermal zone_devices, for example, each
> optical transceiver or gearbox is separated 'tzdev', while all of them share the same cooling device.
> It could up to 128 transceivers.
> 
> It was also intention to avoid a competition between thermal zones when some of them
> can be in trend up state and some  in trend down.
> 
> Are you saying that the below code will work for such case?
> 
> 	/* Make sure cdev enters the deepest cooling state */
> 	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> 		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
> 			instance->tz->id, instance->target);
> 		if (instance->target == THERMAL_NO_TARGET)
> 			continue;
> 		if (instance->target > target)
> 			target = instance->target;
> 	}

Yes, that is my understanding.

For the thermal zones which are under the temperature limit, their 
instance->target will be THERMAL_NO_TARGET and will be discarded by this 
loop.

For the ones being mitigated, the highest cooling state will be used.

The purpose of this loop is exactly for your use case. If it happens it 
does not work as expected, then it is something in the core code to be 
fixed.


>> AFAICS, the code hijacked the get_trend function just for the sake of
>> returning 1 for the hotter thermal zone leading to a computation of the trend
>> in the thermal core code.
> 
> Yes, get_trend() returns one just to indicate that cooling device should not be
> touched for a thermal zone, which is not hottest.
> 
>>
>> I would like to get rid of the get_trend ops in the thermal framework and the
>> changes in this patch sounds like pointless as the aggregation of the cooling
>> action is already handled in the thermal framework.
>>
>> Given the above, it would make sense to revert commit 6f73862fabd93 and
>> 2dc2f760052da ?
> 
> I believe we should run thermal emulation to validate we are OK.

Sure, let me know

Thanks

   -- Daniel

> 
> Thanks,
> Vadim.
> 
>>
>> Thanks
>>
>>     -- Daniel
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drive
>> rs/thermal/thermal_helpers.c#n190
>>
>>
>> [ ... ]
>>
>>
>> --
>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Powered by blists - more mailing lists