diff mbox series

[v6,01/10] cpuidle/poll_state: poll via smp_cond_load_relaxed()

Message ID 20240726201332.626395-2-ankur.a.arora@oracle.com
State Superseded
Headers show
Series Enable haltpoll on arm64 | expand

Commit Message

Ankur Arora July 26, 2024, 8:13 p.m. UTC 
From: Mihai Carabas <mihai.carabas@oracle.com>

The inner loop in poll_idle() polls up to POLL_IDLE_RELAX_COUNT times,
checking to see if the thread has the TIF_NEED_RESCHED bit set. The
loop exits once the condition is met, or if the poll time limit has
been exceeded.

To minimize the number of instructions executed each iteration, the
time check is done only infrequently (once every POLL_IDLE_RELAX_COUNT
iterations). In addition, each loop iteration executes cpu_relax()
which on certain platforms provides a hint to the pipeline that the
loop is busy-waiting, thus allowing the processor to reduce power
consumption.

However, cpu_relax() is defined optimally only on x86. On arm64, for
instance, it is implemented as a YIELD which only serves a hint to the
CPU that it prioritize a different hardware thread if one is available.
arm64, however, does expose a more optimal polling mechanism via
smp_cond_load_relaxed() which uses LDXR, WFE to wait until a store
to a specified region.

So restructure the loop, folding both checks in smp_cond_load_relaxed().
Also, move the time check to the head of the loop allowing it to exit
straight-away once TIF_NEED_RESCHED is set.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
 drivers/cpuidle/poll_state.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Christoph Lameter (Ampere) Aug. 5, 2024, 11:07 p.m. UTC | #1 
On Fri, 26 Jul 2024, Ankur Arora wrote:

> diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c
> index 9b6d90a72601..532e4ed19e0f 100644
> --- a/drivers/cpuidle/poll_state.c
> +++ b/drivers/cpuidle/poll_state.c
> @@ -21,21 +21,21 @@ static int __cpuidle poll_idle(struct cpuidle_device *dev,
>
> 	raw_local_irq_enable();
> 	if (!current_set_polling_and_test()) {
> -		unsigned int loop_count = 0;
> +		unsigned int loop_count;
> 		u64 limit;

loop_count is only used in the while loop below. So the declaration 
could be placed below the while.

>
> 		limit = cpuidle_poll_time(drv, dev);
>
> 		while (!need_resched()) {
> -			cpu_relax();
> -			if (loop_count++ < POLL_IDLE_RELAX_COUNT)
> -				continue;
> -
> 			loop_count = 0;
> 			if (local_clock_noinstr() - time_start > limit) {
> 				dev->poll_time_limit = true;
> 				break;
> 			}

Looks ok otherwise

Reviewed-by: Christoph Lameter <cl@linux.com>
Tomohiro Misono (Fujitsu) Aug. 9, 2024, 5:54 a.m. UTC | #2 
> Subject: [PATCH v6 01/10] cpuidle/poll_state: poll via smp_cond_load_relaxed()
> 
> From: Mihai Carabas <mihai.carabas@oracle.com>
> 
> The inner loop in poll_idle() polls up to POLL_IDLE_RELAX_COUNT times,
> checking to see if the thread has the TIF_NEED_RESCHED bit set. The
> loop exits once the condition is met, or if the poll time limit has
> been exceeded.
> 
> To minimize the number of instructions executed each iteration, the
> time check is done only infrequently (once every POLL_IDLE_RELAX_COUNT
> iterations). In addition, each loop iteration executes cpu_relax()
> which on certain platforms provides a hint to the pipeline that the
> loop is busy-waiting, thus allowing the processor to reduce power
> consumption.
> 
> However, cpu_relax() is defined optimally only on x86. On arm64, for
> instance, it is implemented as a YIELD which only serves a hint to the
> CPU that it prioritize a different hardware thread if one is available.
> arm64, however, does expose a more optimal polling mechanism via
> smp_cond_load_relaxed() which uses LDXR, WFE to wait until a store
> to a specified region.
> 
> So restructure the loop, folding both checks in smp_cond_load_relaxed().
> Also, move the time check to the head of the loop allowing it to exit
> straight-away once TIF_NEED_RESCHED is set.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>  drivers/cpuidle/poll_state.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c
> index 9b6d90a72601..532e4ed19e0f 100644
> --- a/drivers/cpuidle/poll_state.c
> +++ b/drivers/cpuidle/poll_state.c
> @@ -21,21 +21,21 @@ static int __cpuidle poll_idle(struct cpuidle_device *dev,
> 
>  	raw_local_irq_enable();
>  	if (!current_set_polling_and_test()) {
> -		unsigned int loop_count = 0;
> +		unsigned int loop_count;
>  		u64 limit;
> 
>  		limit = cpuidle_poll_time(drv, dev);
> 
>  		while (!need_resched()) {
> -			cpu_relax();
> -			if (loop_count++ < POLL_IDLE_RELAX_COUNT)
> -				continue;
> -
>  			loop_count = 0;
>  			if (local_clock_noinstr() - time_start > limit) {
>  				dev->poll_time_limit = true;
>  				break;
>  			}
> +
> +			smp_cond_load_relaxed(&current_thread_info()->flags,
> +					      VAL & _TIF_NEED_RESCHED ||
> +					      loop_count++ >= POLL_IDLE_RELAX_COUNT);
>  		}
>  	}
>  	raw_local_irq_disable();
> --
> 2.43.5

Reviewed-by: Misono Tomohiro <misono.tomohiro@fujitsu.com>

Regards,
Tomohiro
Ankur Arora Aug. 12, 2024, 10:35 p.m. UTC | #3 
Christoph Lameter (Ampere) <cl@gentwo.org> writes:

> On Fri, 26 Jul 2024, Ankur Arora wrote:
>
>> diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c
>> index 9b6d90a72601..532e4ed19e0f 100644
>> --- a/drivers/cpuidle/poll_state.c
>> +++ b/drivers/cpuidle/poll_state.c
>> @@ -21,21 +21,21 @@ static int __cpuidle poll_idle(struct cpuidle_device *dev,
>>
>> 	raw_local_irq_enable();
>> 	if (!current_set_polling_and_test()) {
>> -		unsigned int loop_count = 0;
>> +		unsigned int loop_count;
>> 		u64 limit;
>
> loop_count is only used in the while loop below. So the declaration could be
> placed below the while.

That's a good idea. Will fix.

>>
>> 		limit = cpuidle_poll_time(drv, dev);
>>
>> 		while (!need_resched()) {
>> -			cpu_relax();
>> -			if (loop_count++ < POLL_IDLE_RELAX_COUNT)
>> -				continue;
>> -
>> 			loop_count = 0;
>> 			if (local_clock_noinstr() - time_start > limit) {
>> 				dev->poll_time_limit = true;
>> 				break;
>> 			}
>
> Looks ok otherwise
>
> Reviewed-by: Christoph Lameter <cl@linux.com>

Thanks for the review.

--
ankur
Ankur Arora Aug. 12, 2024, 10:36 p.m. UTC | #4 
Tomohiro Misono (Fujitsu) <misono.tomohiro@fujitsu.com> writes:

>> Subject: [PATCH v6 01/10] cpuidle/poll_state: poll via smp_cond_load_relaxed()
>>
>> From: Mihai Carabas <mihai.carabas@oracle.com>
>>
>> The inner loop in poll_idle() polls up to POLL_IDLE_RELAX_COUNT times,
>> checking to see if the thread has the TIF_NEED_RESCHED bit set. The
>> loop exits once the condition is met, or if the poll time limit has
>> been exceeded.
>>
>> To minimize the number of instructions executed each iteration, the
>> time check is done only infrequently (once every POLL_IDLE_RELAX_COUNT
>> iterations). In addition, each loop iteration executes cpu_relax()
>> which on certain platforms provides a hint to the pipeline that the
>> loop is busy-waiting, thus allowing the processor to reduce power
>> consumption.
>>
>> However, cpu_relax() is defined optimally only on x86. On arm64, for
>> instance, it is implemented as a YIELD which only serves a hint to the
>> CPU that it prioritize a different hardware thread if one is available.
>> arm64, however, does expose a more optimal polling mechanism via
>> smp_cond_load_relaxed() which uses LDXR, WFE to wait until a store
>> to a specified region.
>>
>> So restructure the loop, folding both checks in smp_cond_load_relaxed().
>> Also, move the time check to the head of the loop allowing it to exit
>> straight-away once TIF_NEED_RESCHED is set.
>>
>> Suggested-by: Peter Zijlstra <peterz@infradead.org>
>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> ---
>>  drivers/cpuidle/poll_state.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c
>> index 9b6d90a72601..532e4ed19e0f 100644
>> --- a/drivers/cpuidle/poll_state.c
>> +++ b/drivers/cpuidle/poll_state.c
>> @@ -21,21 +21,21 @@ static int __cpuidle poll_idle(struct cpuidle_device *dev,
>>
>>  	raw_local_irq_enable();
>>  	if (!current_set_polling_and_test()) {
>> -		unsigned int loop_count = 0;
>> +		unsigned int loop_count;
>>  		u64 limit;
>>
>>  		limit = cpuidle_poll_time(drv, dev);
>>
>>  		while (!need_resched()) {
>> -			cpu_relax();
>> -			if (loop_count++ < POLL_IDLE_RELAX_COUNT)
>> -				continue;
>> -
>>  			loop_count = 0;
>>  			if (local_clock_noinstr() - time_start > limit) {
>>  				dev->poll_time_limit = true;
>>  				break;
>>  			}
>> +
>> +			smp_cond_load_relaxed(&current_thread_info()->flags,
>> +					      VAL & _TIF_NEED_RESCHED ||
>> +					      loop_count++ >= POLL_IDLE_RELAX_COUNT);
>>  		}
>>  	}
>>  	raw_local_irq_disable();
>> --
>> 2.43.5
>
> Reviewed-by: Misono Tomohiro <misono.tomohiro@fujitsu.com>

Thanks!

--
ankur
diff mbox series

Patch

diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c
index 9b6d90a72601..532e4ed19e0f 100644
--- a/drivers/cpuidle/poll_state.c
+++ b/drivers/cpuidle/poll_state.c
@@ -21,21 +21,21 @@  static int __cpuidle poll_idle(struct cpuidle_device *dev,
 
 	raw_local_irq_enable();
 	if (!current_set_polling_and_test()) {
-		unsigned int loop_count = 0;
+		unsigned int loop_count;
 		u64 limit;
 
 		limit = cpuidle_poll_time(drv, dev);
 
 		while (!need_resched()) {
-			cpu_relax();
-			if (loop_count++ < POLL_IDLE_RELAX_COUNT)
-				continue;
-
 			loop_count = 0;
 			if (local_clock_noinstr() - time_start > limit) {
 				dev->poll_time_limit = true;
 				break;
 			}
+
+			smp_cond_load_relaxed(&current_thread_info()->flags,
+					      VAL & _TIF_NEED_RESCHED ||
+					      loop_count++ >= POLL_IDLE_RELAX_COUNT);
 		}
 	}
 	raw_local_irq_disable();