Overload is an indicator used to inform if there is any rq in the root
domain has 2 or more running tasks. Root domain is a global structure per
cpuset island.
Overload status is updated when adding a task to the runqueue.
It is cleared in update_sd_lb_stats during load balance, if none of the
rq has 2 or more running task. It is used during to newidle
balance to see if its worth doing load balance. If it is set, then
newidle balance will try aggressively to pull a task.
Since commit 630246a06ae2 ("sched/fair: Clean-up update_sg_lb_stats
parameters"), overload is being updated unconditionally. The change in
value of this depends on the workload. On typical workloads, it is
likely that this value doesn't change often. Write causes the
cacheline to be invalidated. This would cause true sharing when the same
variable is accessed in newidle_balance. On large systems this would
cause more CPU bus traffic.
Perf probes prove that actual change in value is less often. So it would
be better to check the value before updating it. Detailed perf probes
and annotation can be found in the cover letter. CPU bus traffic reduces
with the patch.
Fixes: 630246a06ae2 ("sched/fair: Clean-up update_sg_lb_stats parameters")
Signed-off-by: Shrikanth Hegde <[email protected]>
---
kernel/sched/fair.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 02d4d224b436..eeebadd7d9ae 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10621,7 +10621,8 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
if (!env->sd->parent) {
/* update overload indicator if we are at root domain */
- WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
+ if (READ_ONCE(env->dst_rq->rd->overload) != (sg_status & SG_OVERLOAD))
+ WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
/* Update over-utilization (tipping point, U >= 0) indicator */
set_rd_overutilized_status(env->dst_rq->rd,
--
2.39.3
On Wed, 13 Mar 2024 at 16:02, Shrikanth Hegde <[email protected]> wrote:
>
> Overload is an indicator used to inform if there is any rq in the root
> domain has 2 or more running tasks. Root domain is a global structure per
> cpuset island.
>
> Overload status is updated when adding a task to the runqueue.
> It is cleared in update_sd_lb_stats during load balance, if none of the
> rq has 2 or more running task. It is used during to newidle
> balance to see if its worth doing load balance. If it is set, then
> newidle balance will try aggressively to pull a task.
>
> Since commit 630246a06ae2 ("sched/fair: Clean-up update_sg_lb_stats
> parameters"), overload is being updated unconditionally. The change in
> value of this depends on the workload. On typical workloads, it is
> likely that this value doesn't change often. Write causes the
> cacheline to be invalidated. This would cause true sharing when the same
> variable is accessed in newidle_balance. On large systems this would
> cause more CPU bus traffic.
>
> Perf probes prove that actual change in value is less often. So it would
> be better to check the value before updating it. Detailed perf probes
> and annotation can be found in the cover letter. CPU bus traffic reduces
the cover letter will not be merged. It's better to put the figures
here if you want to mention them
> with the patch.
>
> Fixes: 630246a06ae2 ("sched/fair: Clean-up update_sg_lb_stats parameters")
I don't think it's worth a Fixes tag. This is an improvement more than
a fix as the current code works fine but generates more bus traffic
With these minor changes in the commit message:
Reviewed-by: Vincent Guittot <[email protected]>
> Signed-off-by: Shrikanth Hegde <[email protected]>
> ---
> kernel/sched/fair.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 02d4d224b436..eeebadd7d9ae 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10621,7 +10621,8 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
>
> if (!env->sd->parent) {
> /* update overload indicator if we are at root domain */
> - WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
> + if (READ_ONCE(env->dst_rq->rd->overload) != (sg_status & SG_OVERLOAD))
> + WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
>
> /* Update over-utilization (tipping point, U >= 0) indicator */
> set_rd_overutilized_status(env->dst_rq->rd,
> --
> 2.39.3
>
On 3/21/24 1:14 PM, Vincent Guittot wrote:
> On Wed, 13 Mar 2024 at 16:02, Shrikanth Hegde <[email protected]> wrote:
>>
Hi Vincent. Thanks for taking a look and review it.
>>
>> Perf probes prove that actual change in value is less often. So it would
>> be better to check the value before updating it. Detailed perf probes
>> and annotation can be found in the cover letter. CPU bus traffic reduces
>
> the cover letter will not be merged. It's better to put the figures
> here if you want to mention them
ok. yes, since it was long i didnt put here. maybe i can put hackbench results
here in the changelog. rest can be found in cover-letter if one is interested.
>
>> with the patch.
>>
>> Fixes: 630246a06ae2 ("sched/fair: Clean-up update_sg_lb_stats parameters")
>
> I don't think it's worth a Fixes tag. This is an improvement more than
> a fix as the current code works fine but generates more bus traffic
>
got it. will remove it. I wasn't sure if I have to put it.
> With these minor changes in the commit message:
>
> Reviewed-by: Vincent Guittot <[email protected]>
>
Thank you! I will send out v2 with these minor edits.
>> Signed-off-by: Shrikanth Hegde <[email protected]>
>> ---
>> kernel/sched/fair.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 02d4d224b436..eeebadd7d9ae 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -10621,7 +10621,8 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
>>
>> if (!env->sd->parent) {
>> /* update overload indicator if we are at root domain */
>> - WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
>> + if (READ_ONCE(env->dst_rq->rd->overload) != (sg_status & SG_OVERLOAD))
>> + WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
>>
>> /* Update over-utilization (tipping point, U >= 0) indicator */
>> set_rd_overutilized_status(env->dst_rq->rd,
>> --
>> 2.39.3
>>