[V2] arm64: psci: Reduce waiting time of cpu_psci_cpu_kill()
diff mbox series

Message ID 18068756-0f39-6388-3290-cf03746e767d@huawei.com
State New
Headers show
Series
  • [V2] arm64: psci: Reduce waiting time of cpu_psci_cpu_kill()
Related show

Commit Message

yeyunfeng Sept. 21, 2019, 11:21 a.m. UTC
If psci_ops.affinity_info() fails, it will sleep 10ms, which will not
take so long in the right case. Use usleep_range() instead of msleep(),
reduce the waiting time, and give a chance to busy wait before sleep.

Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
---
V1->V2:
- use usleep_range() instead of udelay() after waiting for a while

 arch/arm64/kernel/psci.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Will Deacon Oct. 15, 2019, 4:23 p.m. UTC | #1
Hi,

On Sat, Sep 21, 2019 at 07:21:17PM +0800, Yunfeng Ye wrote:
> If psci_ops.affinity_info() fails, it will sleep 10ms, which will not
> take so long in the right case. Use usleep_range() instead of msleep(),
> reduce the waiting time, and give a chance to busy wait before sleep.

Can you elaborate on "the right case" please? It's not clear to me
exactly what problem you're solving here.

I've also added Sudeep to the thread, since I'd like his ack on the change.

Will

>  arch/arm64/kernel/psci.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
> index c9f72b2..99b3122 100644
> --- a/arch/arm64/kernel/psci.c
> +++ b/arch/arm64/kernel/psci.c
> @@ -82,6 +82,7 @@ static void cpu_psci_cpu_die(unsigned int cpu)
>  static int cpu_psci_cpu_kill(unsigned int cpu)
>  {
>  	int err, i;
> +	unsigned long timeout;
> 
>  	if (!psci_ops.affinity_info)
>  		return 0;
> @@ -91,16 +92,24 @@ 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++) {
> +	i = 0;
> +	timeout = jiffies + msecs_to_jiffies(100);
> +	do {
>  		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);
>  			return 0;
>  		}
> 
> -		msleep(10);
> -		pr_info("Retrying again to check for CPU kill\n");
> -	}
> +		/* busy-wait max 1ms */
> +		if (i++ < 100) {
> +			cond_resched();
> +			udelay(10);
> +			continue;
> +		}
> +
> +		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);
> -- 
> 2.7.4.huawei.3
>
yeyunfeng Oct. 16, 2019, 3:22 a.m. UTC | #2
On 2019/10/16 0:23, Will Deacon wrote:
> Hi,
> 
> On Sat, Sep 21, 2019 at 07:21:17PM +0800, Yunfeng Ye wrote:
>> If psci_ops.affinity_info() fails, it will sleep 10ms, which will not
>> take so long in the right case. Use usleep_range() instead of msleep(),
>> reduce the waiting time, and give a chance to busy wait before sleep.
> 
> Can you elaborate on "the right case" please? It's not clear to me
> exactly what problem you're solving here.
> 
The situation is that when the power is off, we have a battery to save some
information, but the battery power is limited, so we reduce the power consumption
by turning off the cores, and need fastly to complete the core shutdown. However, the
time of cpu_psci_cpu_kill() will take 10ms. We have tested the time that it does not
need 10ms, and most case is about 50us-500us. if we reduce the time of cpu_psci_cpu_kill(),
we can reduce 10% - 30% of the total time.

So change msleep (10) to usleep_range() to reduce the waiting time. In addition,
we don't want to be scheduled during the sleeping time, some threads may take a
long time and don't give up the CPU, which affects the time of core shutdown,
Therefore, we add a chance to busy-wait max 1ms.

thanks.

> I've also added Sudeep to the thread, since I'd like his ack on the change.
> 
> Will
> 
>>  arch/arm64/kernel/psci.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
>> index c9f72b2..99b3122 100644
>> --- a/arch/arm64/kernel/psci.c
>> +++ b/arch/arm64/kernel/psci.c
>> @@ -82,6 +82,7 @@ static void cpu_psci_cpu_die(unsigned int cpu)
>>  static int cpu_psci_cpu_kill(unsigned int cpu)
>>  {
>>  	int err, i;
>> +	unsigned long timeout;
>>
>>  	if (!psci_ops.affinity_info)
>>  		return 0;
>> @@ -91,16 +92,24 @@ 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++) {
>> +	i = 0;
>> +	timeout = jiffies + msecs_to_jiffies(100);
>> +	do {
>>  		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);
>>  			return 0;
>>  		}
>>
>> -		msleep(10);
>> -		pr_info("Retrying again to check for CPU kill\n");
>> -	}
>> +		/* busy-wait max 1ms */
>> +		if (i++ < 100) {
>> +			cond_resched();
>> +			udelay(10);
>> +			continue;
>> +		}
>> +
>> +		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);
>> -- 
>> 2.7.4.huawei.3
>>
> 
> .
>
Sudeep Holla Oct. 16, 2019, 10:25 a.m. UTC | #3
On Wed, Oct 16, 2019 at 11:22:23AM +0800, Yunfeng Ye wrote:
>
>
> On 2019/10/16 0:23, Will Deacon wrote:
> > Hi,
> >
> > On Sat, Sep 21, 2019 at 07:21:17PM +0800, Yunfeng Ye wrote:
> >> If psci_ops.affinity_info() fails, it will sleep 10ms, which will not
> >> take so long in the right case. Use usleep_range() instead of msleep(),
> >> reduce the waiting time, and give a chance to busy wait before sleep.
> >
> > Can you elaborate on "the right case" please? It's not clear to me
> > exactly what problem you're solving here.
> >
> The situation is that when the power is off, we have a battery to save some
> information, but the battery power is limited, so we reduce the power consumption
> by turning off the cores, and need fastly to complete the core shutdown. However, the
> time of cpu_psci_cpu_kill() will take 10ms. We have tested the time that it does not
> need 10ms, and most case is about 50us-500us. if we reduce the time of cpu_psci_cpu_kill(),
> we can reduce 10% - 30% of the total time.
>

Have you checked why PSCI AFFINITY_INFO not returning LEVEL_OFF quickly
then ? We wait for upto 5s in cpu_wait_death(worst case) before cpu_kill
is called from __cpu_die.

Moreover I don't understand the argument here. The cpu being killed
will be OFF, as soon as it can and firmware controls that and this
change is not related to CPU_OFF. And this CPU calling cpu_kill can
sleep and 10ms is good to enter idle states if it's idle saving power,
so I fail to map the power saving you mention above.

> So change msleep (10) to usleep_range() to reduce the waiting time. In addition,
> we don't want to be scheduled during the sleeping time, some threads may take a
> long time and don't give up the CPU, which affects the time of core shutdown,
> Therefore, we add a chance to busy-wait max 1ms.
>

On the other hand, usleep_range reduces the timer interval and hence
increases the chance of the callee CPU not to enter deeper idle states.

What am I missing here ? What's the use case or power off situation
you are talking about above ?

>
> > I've also added Sudeep to the thread, since I'd like his ack on the change.
> >

Thanks Will.

--
Regards,
Sudeep
yeyunfeng Oct. 16, 2019, 11:29 a.m. UTC | #4
On 2019/10/16 18:25, Sudeep Holla wrote:
> On Wed, Oct 16, 2019 at 11:22:23AM +0800, Yunfeng Ye wrote:
>>
>>
>> On 2019/10/16 0:23, Will Deacon wrote:
>>> Hi,
>>>
>>> On Sat, Sep 21, 2019 at 07:21:17PM +0800, Yunfeng Ye wrote:
>>>> If psci_ops.affinity_info() fails, it will sleep 10ms, which will not
>>>> take so long in the right case. Use usleep_range() instead of msleep(),
>>>> reduce the waiting time, and give a chance to busy wait before sleep.
>>>
>>> Can you elaborate on "the right case" please? It's not clear to me
>>> exactly what problem you're solving here.
>>>
>> The situation is that when the power is off, we have a battery to save some
>> information, but the battery power is limited, so we reduce the power consumption
>> by turning off the cores, and need fastly to complete the core shutdown. However, the
>> time of cpu_psci_cpu_kill() will take 10ms. We have tested the time that it does not
>> need 10ms, and most case is about 50us-500us. if we reduce the time of cpu_psci_cpu_kill(),
>> we can reduce 10% - 30% of the total time.
>>
> 
> Have you checked why PSCI AFFINITY_INFO not returning LEVEL_OFF quickly
> then ? We wait for upto 5s in cpu_wait_death(worst case) before cpu_kill
> is called from __cpu_die.
> 
When cpu_wait_death() is done, it means that the cpu core's hardware prepare to
die. I think not returning LEVEL_OFF quickly is that hardware need time to handle.
I don't know how much time it need is reasonable, but I test that it need about
50us - 500us.

In addition I have not meat the worst case that cpu_wait_death() need upto 5s, and
we only take normal case into account.

thanks.

> Moreover I don't understand the argument here. The cpu being killed
> will be OFF, as soon as it can and firmware controls that and this
> change is not related to CPU_OFF. And this CPU calling cpu_kill can
> sleep and 10ms is good to enter idle states if it's idle saving power,
> so I fail to map the power saving you mention above.
> 
We have hundreds of CPU cores that need to be shut down. For example,
a CPU has 200 cores, and the thread to shut down the core is in CPU 0.
and the thread need to shut down from core 1 to core 200. However, the
implementation of the kernel can only shut down cpu cores one by one, so we
need to wait for cpu_kill() to finish before shutting down the next
CPU core. If it wait for 10ms each time in cpu_kill, it will takes up
about 2 seconds in cpu_kill() total.

It is not to save power through msleep to idle state, but to quickly
turn off other CPU core's hardware to reduce power consumption.

thanks.

>> So change msleep (10) to usleep_range() to reduce the waiting time. In addition,
>> we don't want to be scheduled during the sleeping time, some threads may take a
>> long time and don't give up the CPU, which affects the time of core shutdown,
>> Therefore, we add a chance to busy-wait max 1ms.
>>
> 
> On the other hand, usleep_range reduces the timer interval and hence
> increases the chance of the callee CPU not to enter deeper idle states.
> 
> What am I missing here ? What's the use case or power off situation
> you are talking about above ?
> 
As mentioned above, we are not to save power through msleep to idle state,  but to quickly
turn off other CPU core's hardware to reduce power consumption.

>>
>>> I've also added Sudeep to the thread, since I'd like his ack on the change.
>>>
> 
> Thanks Will.
> 
> --
> Regards,
> Sudeep
> 
> .
>
Sudeep Holla Oct. 16, 2019, 3:05 p.m. UTC | #5
On Wed, Oct 16, 2019 at 07:29:59PM +0800, Yunfeng Ye wrote:
>
>
> On 2019/10/16 18:25, Sudeep Holla wrote:
> > On Wed, Oct 16, 2019 at 11:22:23AM +0800, Yunfeng Ye wrote:
> >>
> >>
> >> On 2019/10/16 0:23, Will Deacon wrote:
> >>> Hi,
> >>>
> >>> On Sat, Sep 21, 2019 at 07:21:17PM +0800, Yunfeng Ye wrote:
> >>>> If psci_ops.affinity_info() fails, it will sleep 10ms, which will not
> >>>> take so long in the right case. Use usleep_range() instead of msleep(),
> >>>> reduce the waiting time, and give a chance to busy wait before sleep.
> >>>
> >>> Can you elaborate on "the right case" please? It's not clear to me
> >>> exactly what problem you're solving here.
> >>>
> >> The situation is that when the power is off, we have a battery to save some
> >> information, but the battery power is limited, so we reduce the power consumption
> >> by turning off the cores, and need fastly to complete the core shutdown. However, the
> >> time of cpu_psci_cpu_kill() will take 10ms. We have tested the time that it does not
> >> need 10ms, and most case is about 50us-500us. if we reduce the time of cpu_psci_cpu_kill(),
> >> we can reduce 10% - 30% of the total time.
> >>
> >
> > Have you checked why PSCI AFFINITY_INFO not returning LEVEL_OFF quickly
> > then ? We wait for upto 5s in cpu_wait_death(worst case) before cpu_kill
> > is called from __cpu_die.
> >
> When cpu_wait_death() is done, it means that the cpu core's hardware prepare to
> die. I think not returning LEVEL_OFF quickly is that hardware need time to handle.
> I don't know how much time it need is reasonable, but I test that it need about
> 50us - 500us.
>

Fair enough.

> In addition I have not meat the worst case that cpu_wait_death() need upto
> 5s, and we only take normal case into account.
>

Good

>
> > Moreover I don't understand the argument here. The cpu being killed
> > will be OFF, as soon as it can and firmware controls that and this
> > change is not related to CPU_OFF. And this CPU calling cpu_kill can
> > sleep and 10ms is good to enter idle states if it's idle saving power,
> > so I fail to map the power saving you mention above.
> >
> We have hundreds of CPU cores that need to be shut down. For example,
> a CPU has 200 cores, and the thread to shut down the core is in CPU 0.
> and the thread need to shut down from core 1 to core 200. However, the
> implementation of the kernel can only shut down cpu cores one by one, so we
> need to wait for cpu_kill() to finish before shutting down the next
> CPU core. If it wait for 10ms each time in cpu_kill, it will takes up
> about 2 seconds in cpu_kill() total.
>

OK, thanks for the illustrative example. This make sense to me now. But
you comparing with battery powered devices confused me and I assumed
it as some hack to optimise mobile workload.

> >> So change msleep (10) to usleep_range() to reduce the waiting time. In addition,
> >> we don't want to be scheduled during the sleeping time, some threads may take a
> >> long time and don't give up the CPU, which affects the time of core shutdown,
> >> Therefore, we add a chance to busy-wait max 1ms.
> >>
> >
> > On the other hand, usleep_range reduces the timer interval and hence
> > increases the chance of the callee CPU not to enter deeper idle states.
> >
> > What am I missing here ? What's the use case or power off situation
> > you are talking about above ?
> >
> As mentioned above, we are not to save power through msleep to idle state,
> but to quickly turn off other CPU core's hardware to reduce power consumption.

You still don't provide your use-case in which this is required. I know
this will be useful for suspend-to-ram. Do you have any other use-case
that you need to power-off large number of CPUs like this ? Also you
mentioned battery powered, and I don't think any battery powered device
has 200 thread like in your example :)

You need to mention few things clearly in the commit log:
1. How the CPU hotplug operation is serialised in some use-case like
   suspend-to-ram
2. How that may impact systems with large number of CPUs
3. How your change helps to improve that

It may it easy for anyone to understand the motivation for this change.
The commit message you have doesn't give any clue on all the above and
hence we have lot of questions.

I will respond to the original patch separately.

--
Regards,
Sudeep
yeyunfeng Oct. 17, 2019, 2:08 p.m. UTC | #6
On 2019/10/16 23:05, Sudeep Holla wrote:
> On Wed, Oct 16, 2019 at 07:29:59PM +0800, Yunfeng Ye wrote:
>>
>>
>> On 2019/10/16 18:25, Sudeep Holla wrote:
>>> On Wed, Oct 16, 2019 at 11:22:23AM +0800, Yunfeng Ye wrote:
>>>>
>>>>
>>>> On 2019/10/16 0:23, Will Deacon wrote:
>>>>> Hi,
>>>>>
>>>>> On Sat, Sep 21, 2019 at 07:21:17PM +0800, Yunfeng Ye wrote:
>>>>>> If psci_ops.affinity_info() fails, it will sleep 10ms, which will not
>>>>>> take so long in the right case. Use usleep_range() instead of msleep(),
>>>>>> reduce the waiting time, and give a chance to busy wait before sleep.
>>>>>
>>>>> Can you elaborate on "the right case" please? It's not clear to me
>>>>> exactly what problem you're solving here.
>>>>>
>>>> The situation is that when the power is off, we have a battery to save some
>>>> information, but the battery power is limited, so we reduce the power consumption
>>>> by turning off the cores, and need fastly to complete the core shutdown. However, the
>>>> time of cpu_psci_cpu_kill() will take 10ms. We have tested the time that it does not
>>>> need 10ms, and most case is about 50us-500us. if we reduce the time of cpu_psci_cpu_kill(),
>>>> we can reduce 10% - 30% of the total time.
>>>>
>>>
>>> Have you checked why PSCI AFFINITY_INFO not returning LEVEL_OFF quickly
>>> then ? We wait for upto 5s in cpu_wait_death(worst case) before cpu_kill
>>> is called from __cpu_die.
>>>
>> When cpu_wait_death() is done, it means that the cpu core's hardware prepare to
>> die. I think not returning LEVEL_OFF quickly is that hardware need time to handle.
>> I don't know how much time it need is reasonable, but I test that it need about
>> 50us - 500us.
>>
> 
> Fair enough.
> 
>> In addition I have not meat the worst case that cpu_wait_death() need upto
>> 5s, and we only take normal case into account.
>>
> 
> Good
> 
>>
>>> Moreover I don't understand the argument here. The cpu being killed
>>> will be OFF, as soon as it can and firmware controls that and this
>>> change is not related to CPU_OFF. And this CPU calling cpu_kill can
>>> sleep and 10ms is good to enter idle states if it's idle saving power,
>>> so I fail to map the power saving you mention above.
>>>
>> We have hundreds of CPU cores that need to be shut down. For example,
>> a CPU has 200 cores, and the thread to shut down the core is in CPU 0.
>> and the thread need to shut down from core 1 to core 200. However, the
>> implementation of the kernel can only shut down cpu cores one by one, so we
>> need to wait for cpu_kill() to finish before shutting down the next
>> CPU core. If it wait for 10ms each time in cpu_kill, it will takes up
>> about 2 seconds in cpu_kill() total.
>>
> 
> OK, thanks for the illustrative example. This make sense to me now. But
> you comparing with battery powered devices confused me and I assumed
> it as some hack to optimise mobile workload.
> 
It is not mobile workload, but a arm64 server with hundreds of cpu cores.
Battery powered is a backup battery for reliability and to prevent
accidental power failure.

>>>> So change msleep (10) to usleep_range() to reduce the waiting time. In addition,
>>>> we don't want to be scheduled during the sleeping time, some threads may take a
>>>> long time and don't give up the CPU, which affects the time of core shutdown,
>>>> Therefore, we add a chance to busy-wait max 1ms.
>>>>
>>>
>>> On the other hand, usleep_range reduces the timer interval and hence
>>> increases the chance of the callee CPU not to enter deeper idle states.
>>>
>>> What am I missing here ? What's the use case or power off situation
>>> you are talking about above ?
>>>
>> As mentioned above, we are not to save power through msleep to idle state,
>> but to quickly turn off other CPU core's hardware to reduce power consumption.
> 
> You still don't provide your use-case in which this is required. I know
> this will be useful for suspend-to-ram. Do you have any other use-case
> that you need to power-off large number of CPUs like this ? Also you
> mentioned battery powered, and I don't think any battery powered device
> has 200 thread like in your example :)
> 
The use-case is like suspend-to-disk, but a little different:
In the abnormal power failure of server equipment, in order to increase
reliability, there is a backup battery for power supply. Before the battery runs out,
we need to save the key datas to the disk. In order to maintain the battery power
supply, a series of power reduction processing is needed, include which all the cores
need to be shut down quickly, we have max near 200 cores need to shutdown.

Also this modify will be useful for suspend-to-ram too.
thanks.

> You need to mention few things clearly in the commit log:
> 1. How the CPU hotplug operation is serialised in some use-case like
>    suspend-to-ram
> 2. How that may impact systems with large number of CPUs
> 3. How your change helps to improve that
> 
> It may it easy for anyone to understand the motivation for this change.
> The commit message you have doesn't give any clue on all the above and
> hence we have lot of questions.
> 
ok, thanks.

> I will respond to the original patch separately.
> 
> --
> Regards,
> Sudeep
> 
> .
>

Patch
diff mbox series

diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index c9f72b2..99b3122 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -82,6 +82,7 @@  static void cpu_psci_cpu_die(unsigned int cpu)
 static int cpu_psci_cpu_kill(unsigned int cpu)
 {
 	int err, i;
+	unsigned long timeout;

 	if (!psci_ops.affinity_info)
 		return 0;
@@ -91,16 +92,24 @@  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++) {
+	i = 0;
+	timeout = jiffies + msecs_to_jiffies(100);
+	do {
 		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);
 			return 0;
 		}

-		msleep(10);
-		pr_info("Retrying again to check for CPU kill\n");
-	}
+		/* busy-wait max 1ms */
+		if (i++ < 100) {
+			cond_resched();
+			udelay(10);
+			continue;
+		}
+
+		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);