RE: [patch net-next RFC v1] mlxsw: core: Add the hottest thermal zone detection

[Vadim Pasternak <vadimp@nvidia.com>]

Hi Daniel,

> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> Sent: Wednesday, June 15, 2022 11:32 PM
> To: Vadim Pasternak <vadimp@nvidia.com>; davem@davemloft.net
> Cc: netdev@vger.kernel.org; linux@roeck-us.net; rui.zhang@intel.com;
> edubezval@gmail.com; jiri@resnulli.us; Ido Schimmel <idosch@nvidia.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;
	}

> 
> 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.

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