mbox series

[v1,0/3] thermal: core: Remove thermal zones during unregistration

Message ID 1880915.tdWV9SEqCh@kreacher
Headers
Series thermal: core: Remove thermal zones during unregistration |

Message

Rafael J. Wysocki Dec. 8, 2023, 7:11 p.m. UTC 
  Hi All,

This patch series adds a mechanism to guarantee that
thermal_zone_device_unregister() will not return until all of the active
references to the thermal zone device object in question have been dropped
and it has been deleted (patch [1/3]).

This supersedes the approach used so far in which all thermal zone sysfs
attribute callbacks check if the zone device is still registered under the
zone lock, so as to return early if that is not the case, as it means that
device_del() has been called for the thermal zone in question (and returned).
It is not necessary to do that any more after patch [1/3], so patch [2/3]
removes those checks from the code and drops zone locking that is not
necessary any more either.

Patch [3/3] uses the observation that the thermal subsystem does not need to
check if a thermal zone device is registered at all, because it can use its
own data to determine whether or not the thermal zone is going away and so
it may not be worth updating it, for example.

Please refer to the patch changelogs for details.

The series depends on new thermal material in linux-next, but it should not
substantially depend on any changes that have not made it into linux-next yet.

Thanks!

Comments

Lukasz Luba Dec. 11, 2023, 1:38 p.m. UTC | #1 
Hi Rafael,

On 12/8/23 19:11, Rafael J. Wysocki wrote:
> Hi All,
> 
> This patch series adds a mechanism to guarantee that
> thermal_zone_device_unregister() will not return until all of the active
> references to the thermal zone device object in question have been dropped
> and it has been deleted (patch [1/3]).
> 
> This supersedes the approach used so far in which all thermal zone sysfs
> attribute callbacks check if the zone device is still registered under the
> zone lock, so as to return early if that is not the case, as it means that
> device_del() has been called for the thermal zone in question (and returned).
> It is not necessary to do that any more after patch [1/3], so patch [2/3]
> removes those checks from the code and drops zone locking that is not
> necessary any more either.
> 
> Patch [3/3] uses the observation that the thermal subsystem does not need to
> check if a thermal zone device is registered at all, because it can use its
> own data to determine whether or not the thermal zone is going away and so
> it may not be worth updating it, for example.
> 
> Please refer to the patch changelogs for details.
> 
> The series depends on new thermal material in linux-next, but it should not
> substantially depend on any changes that have not made it into linux-next yet.
> 
> Thanks!
> 
> 
> 

I like the concept with completion thing for this.
I have tired to stress test these patches with my mock
thermal zone module load/unload and it works good.

The test was doing the these bits:
for i in $(seq 1 1000000) ; do cat 
/sys/class/thermal/thermal_zone2/trip_point_0_temp > /dev/null 2>&1 ; done &
for i in $(seq 1 10000) ; do insmod /data/selftest_ipa.ko ; rmmod 
selftest_ipa ; done &

I couldn't trigger any issues in reading from this
trip temp file in background, which should go now w/o the
locking. I thought it would be nice test, since we have
direct call to trips array 'tz->trips[trip_id].temperature'.
Let me know if you think about other scenario for stress testing it.
(I have also checked the 'temp' sysfs read, where the mutex for
tz is used - also no issues).

Feel free to add to all patches:

Reviewed-and-tested-by: Lukasz Luba <lukasz.luba@arm.com>

Regards,
Lukasz
Rafael J. Wysocki Dec. 11, 2023, 1:44 p.m. UTC | #2 
Hi Lukasz,

On Mon, Dec 11, 2023 at 2:37 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Rafael,
>
> On 12/8/23 19:11, Rafael J. Wysocki wrote:
> > Hi All,
> >
> > This patch series adds a mechanism to guarantee that
> > thermal_zone_device_unregister() will not return until all of the active
> > references to the thermal zone device object in question have been dropped
> > and it has been deleted (patch [1/3]).
> >
> > This supersedes the approach used so far in which all thermal zone sysfs
> > attribute callbacks check if the zone device is still registered under the
> > zone lock, so as to return early if that is not the case, as it means that
> > device_del() has been called for the thermal zone in question (and returned).
> > It is not necessary to do that any more after patch [1/3], so patch [2/3]
> > removes those checks from the code and drops zone locking that is not
> > necessary any more either.
> >
> > Patch [3/3] uses the observation that the thermal subsystem does not need to
> > check if a thermal zone device is registered at all, because it can use its
> > own data to determine whether or not the thermal zone is going away and so
> > it may not be worth updating it, for example.
> >
> > Please refer to the patch changelogs for details.
> >
> > The series depends on new thermal material in linux-next, but it should not
> > substantially depend on any changes that have not made it into linux-next yet.
> >
> > Thanks!
> >
> >
> >
>
> I like the concept with completion thing for this.
> I have tired to stress test these patches with my mock
> thermal zone module load/unload and it works good.
>
> The test was doing the these bits:
> for i in $(seq 1 1000000) ; do cat
> /sys/class/thermal/thermal_zone2/trip_point_0_temp > /dev/null 2>&1 ; done &
> for i in $(seq 1 10000) ; do insmod /data/selftest_ipa.ko ; rmmod
> selftest_ipa ; done &
>
> I couldn't trigger any issues in reading from this
> trip temp file in background, which should go now w/o the
> locking. I thought it would be nice test, since we have
> direct call to trips array 'tz->trips[trip_id].temperature'.
> Let me know if you think about other scenario for stress testing it.
> (I have also checked the 'temp' sysfs read, where the mutex for
> tz is used - also no issues).
>
> Feel free to add to all patches:
>
> Reviewed-and-tested-by: Lukasz Luba <lukasz.luba@arm.com>

Thank you!