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

[Eduardo Valentin <evalenti@kernel.org>]

Hey guys,

On Thu, Jun 16, 2022 at 12:27:39AM +0200, Daniel Lezcano wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Hi Vadim,
> 
> On 16/06/2022 00:06, Vadim Pasternak wrote:
> > 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;
> >       }
> 
> 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.

I agree with the suggestion that these commit shall be considered for revert. 

The thermal core will do aggregation while handling cooling devices
to set the cooling state to whatever is currently the one requiring more impact
on temperature.

And, yes, please check with thermal emulation.

Side note, please use my evalenti@kernel.org address if you need my attention.
I will keep on monitoring messages sent to that address.

I will be updating my contacts at MAINTAINERS soon.
> 
> 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

-- 
All the best,
Eduardo Valentin