Hi Qian,
It would be helpful if you can test this patchset and confirm if the races you
mentioned went away or not and that the FIE code works as we wanted it to.
I don't have a real setup and so it won't be easy for me to test this out.
I have already sent a temporary fix for 5.13 and this patchset is targeted for
5.14 and is based over that.
-------------------------8<-------------------------
The CPPC driver currently stops the frequency invariance related
kthread_work and irq_work from cppc_freq_invariance_exit() which is only
called during driver's removal.
This is not sufficient as the CPUs can get hot-plugged out while the
driver is in use, the same also happens during system suspend/resume.
In such a cases we can reach a state where the CPU is removed by the
kernel but its kthread_work or irq_work aren't stopped.
Fix this by implementing the start_cpu() and stop_cpu() callbacks in the
cpufreq core, which will be called for each CPU's addition/removal.
A similar call was already available in the cpufreq core, which isn't required
anymore and so its users are migrated to use exit() callback instead.
This is targeted for v5.14-rc1.
--
Viresh
Viresh Kumar (5):
cpufreq: cppc: Migrate to ->exit() callback instead of ->stop_cpu()
cpufreq: intel_pstate: Migrate to ->exit() callback instead of
->stop_cpu()
cpufreq: powerenv: Migrate to ->exit() callback instead of
->stop_cpu()
cpufreq: Add start_cpu() and stop_cpu() callbacks
cpufreq: cppc: Fix suspend/resume specific races with the FIE code
Documentation/cpu-freq/cpu-drivers.rst | 7 +-
drivers/cpufreq/Kconfig.arm | 1 -
drivers/cpufreq/cppc_cpufreq.c | 163 ++++++++++++++-----------
drivers/cpufreq/cpufreq.c | 11 +-
drivers/cpufreq/intel_pstate.c | 9 +-
drivers/cpufreq/powernv-cpufreq.c | 23 ++--
include/linux/cpufreq.h | 5 +-
7 files changed, 119 insertions(+), 100 deletions(-)
--
2.31.1.272.g89b43f80a514
commit 367dc4aa932b ("cpufreq: Add stop CPU callback to cpufreq_driver
interface") added the stop_cpu() callback to allow the drivers to do
clean up before the CPU is completely down and its state cannot be
modified.
At that time the CPU hotplug framework used to call the cpufreq core's
registered notifier for different events like CPU_DOWN_PREPARE and
CPU_POST_DEAD. The stop_cpu() callback was called during the
CPU_DOWN_PREPARE event.
This is no longer the case, cpuhp_cpufreq_offline() is called only once
by the CPU hotplug core now and we don't really need two separate
callbacks for cpufreq drivers, i.e. stop_cpu() and exit(), as everything
can be done from the exit() callback itself.
Migrate to using the exit() callback instead of stop_cpu().
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/powernv-cpufreq.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
@@ -875,7 +875,15 @@ static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)staticintpowernv_cpufreq_cpu_exit(structcpufreq_policy*policy){-/* timer is deleted in cpufreq_cpu_stop() */+structpowernv_smp_call_datafreq_data;+structglobal_pstate_info*gpstates=policy->driver_data;++freq_data.pstate_id=idx_to_pstate(powernv_pstate_info.min);+freq_data.gpstate_id=idx_to_pstate(powernv_pstate_info.min);+smp_call_function_single(policy->cpu,set_pstate,&freq_data,1);+if(gpstates)+del_timer_sync(&gpstates->timer);+kfree(policy->driver_data);return0;
Hi Qian,
It would be helpful if you can test this patchset and confirm if the races you
mentioned went away or not and that the FIE code works as we wanted it to.
I don't have a real setup and so it won't be easy for me to test this out.
I have already sent a temporary fix for 5.13 and this patchset is targeted for
5.14 and is based over that.
-------------------------8<-------------------------
The CPPC driver currently stops the frequency invariance related
kthread_work and irq_work from cppc_freq_invariance_exit() which is only
called during driver's removal.
This is not sufficient as the CPUs can get hot-plugged out while the
driver is in use, the same also happens during system suspend/resume.
In such a cases we can reach a state where the CPU is removed by the
kernel but its kthread_work or irq_work aren't stopped.
Fix this by implementing the start_cpu() and stop_cpu() callbacks in the
cpufreq core, which will be called for each CPU's addition/removal.
A similar call was already available in the cpufreq core, which isn't required
anymore and so its users are migrated to use exit() callback instead.
This is targeted for v5.14-rc1.
--
Viresh
Viresh Kumar (5):
cpufreq: cppc: Migrate to ->exit() callback instead of ->stop_cpu()
cpufreq: intel_pstate: Migrate to ->exit() callback instead of
->stop_cpu()
cpufreq: powerenv: Migrate to ->exit() callback instead of
->stop_cpu()
cpufreq: Add start_cpu() and stop_cpu() callbacks
cpufreq: cppc: Fix suspend/resume specific races with the FIE code
Documentation/cpu-freq/cpu-drivers.rst | 7 +-
drivers/cpufreq/Kconfig.arm | 1 -
drivers/cpufreq/cppc_cpufreq.c | 163 ++++++++++++++-----------
drivers/cpufreq/cpufreq.c | 11 +-
drivers/cpufreq/intel_pstate.c | 9 +-
drivers/cpufreq/powernv-cpufreq.c | 23 ++--
include/linux/cpufreq.h | 5 +-
7 files changed, 119 insertions(+), 100 deletions(-)
Hi Qian,
First of all thanks for testing this, I need more of your help to test
this out :)
FWIW, I did test this on my Hikey board today, with some hacks, and
tried multiple insmod/rmmod operations for the driver, and I wasn't
able to reproduce the issue you reported. I did enable the list-debug
config option.
On 14-06-21, 09:48, Qian Cai wrote:
Can you give details on what exactly did you try to do, to get this ?
Normal boot or something more ?
I have made some changes to the way calls were happening, may get this
thing sorted. Can you please try this branch ?
https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=cpufreq/cppc
I can see one place where race can happen, i.e. between
topology_clear_scale_freq_source() and topology_scale_freq_tick(). It
is possible that sfd->set_freq_scale() may get called for a previously
set handler as there is no protection there.
I will see how to fix that. But I am not sure if the issue reported
above comes from there.
Anyway, please give my branch a try, lets see.
--
viresh
I can see one place where race can happen, i.e. between
topology_clear_scale_freq_source() and topology_scale_freq_tick(). It
is possible that sfd->set_freq_scale() may get called for a previously
set handler as there is no protection there.
I will see how to fix that. But I am not sure if the issue reported
above comes from there.
I have tried to fix this race and pushed the relevant patch to my
branch. Please pick the latest branch and hopefully everything will
just work.
--
viresh
Hi Qian,
First of all thanks for testing this, I need more of your help to test
this out :)
FWIW, I did test this on my Hikey board today, with some hacks, and
tried multiple insmod/rmmod operations for the driver, and I wasn't
able to reproduce the issue you reported. I did enable the list-debug
config option.
Can you give details on what exactly did you try to do, to get this ?
Normal boot or something more ?
Basically, it has the cpufreq driver as CPPC and the governor as schedutil. Running a few workloads to get CPU scaling up and down. Later, try to offline all CPUs until the last one and then online all CPUs.
I have made some changes to the way calls were happening, may get this
thing sorted. Can you please try this branch ?
https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=cpufreq/cppc
I can see one place where race can happen, i.e. between
topology_clear_scale_freq_source() and topology_scale_freq_tick(). It
is possible that sfd->set_freq_scale() may get called for a previously
set handler as there is no protection there.
I will see how to fix that. But I am not sure if the issue reported
above comes from there.
Anyway, please give my branch a try, lets see.
I am hesitate to try this at the moment because this all feel like shooting in the dark. Ideally, you will be able to get access to one of those arm64 servers (Huawei, Ampere, TX2, FJ etc) eventually and really try the same exercises yourself with those debugging options like list debugging and KASAN on. That way you could fix things way efficiently. I could share you the .config once you are there. Last but not least, once you get better narrow down of the issues, I'd hope to see someone else familiar with the code there to get review of those patches first (feel free to Cc me once you are ready to post) before I'll rerun the whole things again. That way we don't waste time on each other backing and forth chasing the shadow.
Can you give details on what exactly did you try to do, to get this ?
Normal boot or something more ?
Basically, it has the cpufreq driver as CPPC and the governor as
schedutil. Running a few workloads to get CPU scaling up and down.
Later, try to offline all CPUs until the last one and then online
all CPUs.
Hmm, okay.
So I basically have very similar setup with 8 cores (1-policy
per-cpu), the only difference is I don't end up reading the
performance counters, everything else remains same. So I should see
issues now just like you, in case there are any.
Since the insmod/rmmod setup is a bit different, this is what I tried
today for around an hour with CONFIG_DEBUG_LIST and RCU debugging
options.
while true; do
for i in `seq 1 7`;
do
echo 0 > /sys/devices/system/cpu/cpu$i/online;
done;
for i in `seq 1 7`;
do
echo 1 > /sys/devices/system/cpu/cpu$i/online;
done;
done
I don't see any crashes, oops or warnings with latest stuff.
I am hesitate to try this at the moment because this all feel like
shooting in the dark.
I understand your point and you aren't completely wrong here. It
wasn't completely in dark but since I am unable to reproduce the issue
at my end, I asked for help.
FWIW, I think one of the possible cause of corruption of kthread thing
could have been because of the race in the topology related code. I
already fixed that in my tree yesterday.
Ideally, you will be able to get access to one
of those arm64 servers (Huawei, Ampere, TX2, FJ etc) eventually and
really try the same exercises yourself with those debugging options
like list debugging and KASAN on. That way you could fix things way
efficiently.
Yeah, I thought of this work being over and I am not a user of it
normally. I had to enable it for ARM servers and I took help of my
colleagues (Vincent Guittot and Ionela) for testing the same.
I have also asked Vincent to give it a try again.
I could share you the .config once you are there. Last
but not least, once you get better narrow down of the issues, I'd
hope to see someone else familiar with the code there to get review
of those patches first (feel free to Cc me once you are ready to
post) before I'll rerun the whole things again. That way we don't
waste time on each other backing and forth chasing the shadow.
I did send the stuff up for review and this last thing (you reported)
was a different race altogether, so asked for testing without reviews.
Anyway, I am quite sure my tests have covered such issues now. I will
send out patches again soon.
Thanks Qian.
--
viresh