| Message ID | 04ab51e4-bc08-8250-4e70-4c87c58c8ad0@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] arm64: psci: Reduce the waiting time for cpu_psci_cpu_kill() | expand |
On Fri, Oct 18, 2019 at 08:46:37PM +0800, Yunfeng Ye wrote: > In case like suspend-to-disk and uspend-to-ram, a large number of CPU s/case/cases/ s/uspend-to-ram/suspend-to-ram/ > cores need to be shut down. At present, the CPU hotplug operation is > serialised, and the CPU cores can only be shut down one by one. In this > process, if PSCI affinity_info() does not return LEVEL_OFF quickly, > cpu_psci_cpu_kill() needs to wait for 10ms. If hundreds of CPU cores > need to be shut down, it will take a long time. > > Normally, there is no need to wait 10ms in cpu_psci_cpu_kill(). So > change the wait interval from 10 ms to max 1 ms and use usleep_range() > instead of msleep() for more accurate timer. > > In addition, reducing the time interval will increase the messages > output, so remove the "Retry ..." message, instead, put the number of > waiting times to the sucessful message. > > Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com> > --- > v3 -> v4: > - using time_before(jiffies, timeout) to check > - update the comment as review suggest > > v2 -> v3: > - update the comment > - remove the busy-wait logic, modify the loop logic and output message > > v1 -> v2: > - use usleep_range() instead of udelay() after waiting for a while > arch/arm64/kernel/psci.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c > index c9f72b2665f1..77965c3ba477 100644 > --- a/arch/arm64/kernel/psci.c > +++ b/arch/arm64/kernel/psci.c > @@ -81,7 +81,8 @@ static void cpu_psci_cpu_die(unsigned int cpu) > > static int cpu_psci_cpu_kill(unsigned int cpu) > { > - int err, i; > + int err, i = 0; > + unsigned long timeout; > > if (!psci_ops.affinity_info) > return 0; > @@ -91,16 +92,17 @@ static int cpu_psci_cpu_kill(unsigned int cpu) > * while it is dying. So, try again a few times. > */ > > - for (i = 0; i < 10; i++) { > + timeout = jiffies + msecs_to_jiffies(100); > + do { > + i++; > err = psci_ops.affinity_info(cpu_logical_map(cpu), 0); > if (err == PSCI_0_2_AFFINITY_LEVEL_OFF) { > - pr_info("CPU%d killed.\n", cpu); > + pr_info("CPU%d killed (polled %d times)\n", cpu, i); We can even drop loop counter completely, track time and log that instead of loop counter that doesn't give any indication without looking into the code. start = jiffies, end = start + msecs_to_jiffies(100); do { .... pr_info("CPU%d killed (polled %u ms)\n", cpu, jiffies_to_msecs(jiffies - start)); .... } while (time_before(jiffies, end)); Just my preference. Looks good otherwise. -- Regards, Sudeep
On 2019/10/18 23:20, Sudeep Holla wrote: > On Fri, Oct 18, 2019 at 08:46:37PM +0800, Yunfeng Ye wrote: >> In case like suspend-to-disk and uspend-to-ram, a large number of CPU > > s/case/cases/ > s/uspend-to-ram/suspend-to-ram/ > ok, thanks. >> cores need to be shut down. At present, the CPU hotplug operation is >> serialised, and the CPU cores can only be shut down one by one. In this >> process, if PSCI affinity_info() does not return LEVEL_OFF quickly, >> cpu_psci_cpu_kill() needs to wait for 10ms. If hundreds of CPU cores >> need to be shut down, it will take a long time. >> >> Normally, there is no need to wait 10ms in cpu_psci_cpu_kill(). So >> change the wait interval from 10 ms to max 1 ms and use usleep_range() >> instead of msleep() for more accurate timer. >> >> In addition, reducing the time interval will increase the messages >> output, so remove the "Retry ..." message, instead, put the number of >> waiting times to the sucessful message. >> >> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com> >> --- >> v3 -> v4: >> - using time_before(jiffies, timeout) to check >> - update the comment as review suggest >> >> v2 -> v3: >> - update the comment >> - remove the busy-wait logic, modify the loop logic and output message >> >> v1 -> v2: >> - use usleep_range() instead of udelay() after waiting for a while >> arch/arm64/kernel/psci.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c >> index c9f72b2665f1..77965c3ba477 100644 >> --- a/arch/arm64/kernel/psci.c >> +++ b/arch/arm64/kernel/psci.c >> @@ -81,7 +81,8 @@ static void cpu_psci_cpu_die(unsigned int cpu) >> >> static int cpu_psci_cpu_kill(unsigned int cpu) >> { >> - int err, i; >> + int err, i = 0; >> + unsigned long timeout; >> >> if (!psci_ops.affinity_info) >> return 0; >> @@ -91,16 +92,17 @@ static int cpu_psci_cpu_kill(unsigned int cpu) >> * while it is dying. So, try again a few times. >> */ >> >> - for (i = 0; i < 10; i++) { >> + timeout = jiffies + msecs_to_jiffies(100); >> + do { >> + i++; >> err = psci_ops.affinity_info(cpu_logical_map(cpu), 0); >> if (err == PSCI_0_2_AFFINITY_LEVEL_OFF) { >> - pr_info("CPU%d killed.\n", cpu); >> + pr_info("CPU%d killed (polled %d times)\n", cpu, i); > > We can even drop loop counter completely, track time and log that > instead of loop counter that doesn't give any indication without looking > into the code. > ok, I will modify as your suggest. thanks. > start = jiffies, end = start + msecs_to_jiffies(100); > do { > .... > pr_info("CPU%d killed (polled %u ms)\n", cpu, > jiffies_to_msecs(jiffies - start)); > .... > } while (time_before(jiffies, end)); > > Just my preference. Looks good otherwise. > > -- > Regards, > Sudeep > > > . >
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index c9f72b2665f1..77965c3ba477 100644 --- a/arch/arm64/kernel/psci.c +++ b/arch/arm64/kernel/psci.c @@ -81,7 +81,8 @@ static void cpu_psci_cpu_die(unsigned int cpu) static int cpu_psci_cpu_kill(unsigned int cpu) { - int err, i; + int err, i = 0; + unsigned long timeout; if (!psci_ops.affinity_info) return 0; @@ -91,16 +92,17 @@ static int cpu_psci_cpu_kill(unsigned int cpu) * while it is dying. So, try again a few times. */ - for (i = 0; i < 10; i++) { + timeout = jiffies + msecs_to_jiffies(100); + do { + i++; err = psci_ops.affinity_info(cpu_logical_map(cpu), 0); if (err == PSCI_0_2_AFFINITY_LEVEL_OFF) { - pr_info("CPU%d killed.\n", cpu); + pr_info("CPU%d killed (polled %d times)\n", cpu, i); return 0; } - msleep(10); - pr_info("Retrying again to check for CPU kill\n"); - } + usleep_range(100, 1000); + } while (time_before(jiffies, timeout)); pr_warn("CPU%d may not have shut down cleanly (AFFINITY_INFO reports %d)\n", cpu, err);
In case like suspend-to-disk and uspend-to-ram, a large number of CPU cores need to be shut down. At present, the CPU hotplug operation is serialised, and the CPU cores can only be shut down one by one. In this process, if PSCI affinity_info() does not return LEVEL_OFF quickly, cpu_psci_cpu_kill() needs to wait for 10ms. If hundreds of CPU cores need to be shut down, it will take a long time. Normally, there is no need to wait 10ms in cpu_psci_cpu_kill(). So change the wait interval from 10 ms to max 1 ms and use usleep_range() instead of msleep() for more accurate timer. In addition, reducing the time interval will increase the messages output, so remove the "Retry ..." message, instead, put the number of waiting times to the sucessful message. Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com> --- v3 -> v4: - using time_before(jiffies, timeout) to check - update the comment as review suggest v2 -> v3: - update the comment - remove the busy-wait logic, modify the loop logic and output message v1 -> v2: - use usleep_range() instead of udelay() after waiting for a while arch/arm64/kernel/psci.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)