| Message ID | 20211027082237.26759-3-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/3] intel_idle: cleanup code for handling with cpuidle framework | expand |
On Wed, Oct 27, 2021 at 10:07 AM Zhang Rui <rui.zhang@intel.com> wrote: > > cpuidle_device is allocated as percpu data, and it is registered for every > CPU that has ever been onlined. > When unregistering, checking current online CPUs is not sufficient, > because some cpu may be offlined later with its cpuidle_device registered. But the unregistration happens only in the error code path of intel_idle_init(), doesn't it? While I agree that doing a for_each_present_cpu() walk for that is more prudent', I'm not sure if that makes any difference in practice. > Fix this by using for_each_present_cpu() instead, and unregistering all > the cpuidle_devices that have been registered. > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/idle/intel_idle.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > index e7f2a5f85bf9..9e916e2adc89 100644 > --- a/drivers/idle/intel_idle.c > +++ b/drivers/idle/intel_idle.c > @@ -1687,8 +1687,13 @@ static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) > > if (intel_idle_cpuhp_state > 0) > cpuhp_remove_state(intel_idle_cpuhp_state); > - for_each_online_cpu(i) > - cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i)); > + for_each_present_cpu(i) { > + struct cpuidle_device *dev; > + > + dev = per_cpu_ptr(intel_idle_cpuidle_devices, i); > + if (dev->registered) dev->registered is checked by cpuidle_unregister_device(). > + cpuidle_unregister_device(dev); > + } > cpuidle_unregister_driver(drv); > free_percpu(intel_idle_cpuidle_devices); > } > --
On Wed, 2021-11-24 at 14:20 +0100, Rafael J. Wysocki wrote: > On Wed, Oct 27, 2021 at 10:07 AM Zhang Rui <rui.zhang@intel.com> > wrote: > > > > cpuidle_device is allocated as percpu data, and it is registered > > for every > > CPU that has ever been onlined. > > When unregistering, checking current online CPUs is not sufficient, > > because some cpu may be offlined later with its cpuidle_device > > registered. > > But the unregistration happens only in the error code path of > intel_idle_init(), doesn't it? yes. > > While I agree that doing a for_each_present_cpu() walk for that is > more prudent', I'm not sure if that makes any difference in practice. And yes, exactly. This is not a problem as long as intel_idle driver can not be unloaded. There is no technical gap either to unregister the intel_idle cpuidle driver, or to unload the intel_idle module. And this potential issue will be exposed only when we decided to do so. If you prefer to describe this *potential* issue more precisely, I totally agree. If you want to fix it only when really needed, that's also okay to me. > > > Fix this by using for_each_present_cpu() instead, and unregistering > > all > > the cpuidle_devices that have been registered. > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > --- > > drivers/idle/intel_idle.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > > index e7f2a5f85bf9..9e916e2adc89 100644 > > --- a/drivers/idle/intel_idle.c > > +++ b/drivers/idle/intel_idle.c > > @@ -1687,8 +1687,13 @@ static void __init > > intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) > > > > if (intel_idle_cpuhp_state > 0) > > cpuhp_remove_state(intel_idle_cpuhp_state); > > - for_each_online_cpu(i) > > - cpuidle_unregister_device(per_cpu_ptr(intel_idle_cp > > uidle_devices, i)); > > + for_each_present_cpu(i) { > > + struct cpuidle_device *dev; > > + > > + dev = per_cpu_ptr(intel_idle_cpuidle_devices, i); > > + if (dev->registered) > > dev->registered is checked by cpuidle_unregister_device(). right, this check is not needed. thanks, rui > > > + cpuidle_unregister_device(dev); > > + } > > cpuidle_unregister_driver(drv); > > free_percpu(intel_idle_cpuidle_devices); > > } > > --
On Wed, Nov 24, 2021 at 2:44 PM Zhang Rui <rui.zhang@intel.com> wrote: > > On Wed, 2021-11-24 at 14:20 +0100, Rafael J. Wysocki wrote: > > On Wed, Oct 27, 2021 at 10:07 AM Zhang Rui <rui.zhang@intel.com> > > wrote: > > > > > > cpuidle_device is allocated as percpu data, and it is registered > > > for every > > > CPU that has ever been onlined. > > > When unregistering, checking current online CPUs is not sufficient, > > > because some cpu may be offlined later with its cpuidle_device > > > registered. > > > > But the unregistration happens only in the error code path of > > intel_idle_init(), doesn't it? > > yes. > > > > While I agree that doing a for_each_present_cpu() walk for that is > > more prudent', I'm not sure if that makes any difference in practice. > > And yes, exactly. > This is not a problem as long as intel_idle driver can not be unloaded. > > There is no technical gap either to unregister the intel_idle cpuidle > driver, or to unload the intel_idle module. And this potential issue > will be exposed only when we decided to do so. > > If you prefer to describe this *potential* issue more precisely, I > totally agree. > If you want to fix it only when really needed, that's also okay to me. I would prefer to fix it first and then extend as needed.
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index e7f2a5f85bf9..9e916e2adc89 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -1687,8 +1687,13 @@ static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) if (intel_idle_cpuhp_state > 0) cpuhp_remove_state(intel_idle_cpuhp_state); - for_each_online_cpu(i) - cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i)); + for_each_present_cpu(i) { + struct cpuidle_device *dev; + + dev = per_cpu_ptr(intel_idle_cpuidle_devices, i); + if (dev->registered) + cpuidle_unregister_device(dev); + } cpuidle_unregister_driver(drv); free_percpu(intel_idle_cpuidle_devices); }
cpuidle_device is allocated as percpu data, and it is registered for every CPU that has ever been onlined. When unregistering, checking current online CPUs is not sufficient, because some cpu may be offlined later with its cpuidle_device registered. Fix this by using for_each_present_cpu() instead, and unregistering all the cpuidle_devices that have been registered. Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/idle/intel_idle.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)