All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] sched: Minor changes for rd->overload access
@ 2024-03-13 15:01 Shrikanth Hegde
  2024-03-13 15:01 ` [PATCH 1/2] sched/fair: Check rd->overload value before update Shrikanth Hegde
  2024-03-13 15:01 ` [PATCH 2/2] sched/fair: Use helper functions to access rd->overload Shrikanth Hegde
  0 siblings, 2 replies; 6+ messages in thread
From: Shrikanth Hegde @ 2024-03-13 15:01 UTC (permalink / raw
  To: mingo, peterz, vincent.guittot
  Cc: sshegde, dietmar.eggemann, qyousef, linux-kernel, vschneid,
	qperret, yu.c.chen, srikar, pierre.gondois

tl;dr
When running workloads in large systems, it was observed that access to
rd->overload was taking time. It would be better to check the value
before updating since value changes less often. Patch 1/2 does that.
With patch updates happen only if necessary. CPU Bus traffic reduced a
bit. No significant gains in workload performance.

Qais Suggested that it would be better to use the helper functions to
access the rd->overload instead. Patch 2/2 does that.

These patches depend on below to be applied first.
https://lore.kernel.org/all/20240307085725.444486-1-sshegde@linux.ibm.com/


-----------------------------------------------------------------------
Detailed Perf annotation and probes stat
-----------------------------------------------------------------------
=======
6.8-rc5
=======
320 CPU system, SMT8
  NUMA node(s):          4
  NUMA node0 CPU(s):     0-79
  NUMA node1 CPU(s):     80-159
  NUMA node6 CPU(s):     160-239
  NUMA node7 CPU(s):     240-319

Perf annoate while running "schbench -t 320 -i 30 -r 30"
       │     if (!READ_ONCE(this_rq->rd->overload) ||
 18.05 │       ld       r9,2752(r31)
       │     sd = rcu_dereference_check_sched_domain(this_rq->sd);
  6.97 │       ld       r30,2760(r31)


Added some dummy codes so the probes can be put at required places.
perf probe -L update_sd_lb_stats
     46         if (env->sd->flags & SD_NUMA)
     47                 env->fbq_type = fbq_classify_group(&sds->busiest_stat);

     49         if (!env->sd->parent) {
                        /* update overload indicator if we are at root domain */
     51                 WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);

perf -probe -L newidle_balance
                rcu_read_lock();
     38         sd = rcu_dereference_check_sched_domain(this_rq->sd);

                if (!READ_ONCE(this_rq->rd->overload) ||
                    (sd && this_rq->avg_idle < sd->max_newidle_lb_cost)) {

perf probe -L add_nr_running
         #ifdef CONFIG_SMP
     11         if (prev_nr < 2 && rq->nr_running >= 2) {
     12                 if (!READ_ONCE(rq->rd->overload)) {
     13                         a = a +10;
                                WRITE_ONCE(rq->rd->overload, 1);
                        }

probe hits when running different workload.
idle
320 probe:add_nr_running_L12
260 probe:add_nr_running_L13
1K probe:newidle_balance_L38
596 probe:update_sd_lb_stats_L51

./hackbench 10 process 100000 loops
130K probe:add_nr_running_L12
93 probe:add_nr_running_L13
1M probe:newidle_balance_L38
109K probe:update_sd_lb_stats_L51

./schbench -t 320 -i 30 -r 30
3K probe:add_nr_running_L12
436 probe:add_nr_running_L13
125K probe:newidle_balance_L38
33K probe:update_sd_lb_stats_L51

Modified stress-ng --wait
3K probe:add_nr_running_L12
1K probe:add_nr_running_L13
6M probe:newidle_balance_L38
11K probe:update_sd_lb_stats_L51

stress-ng --cpu=400 -l 20
833 probe:add_nr_running_L12
280 probe:add_nr_running_L13
2K probe:newidle_balance_L38
1K probe:update_sd_lb_stats_L51

stress-ng --cpu=400 -l 100
730 probe:add_nr_running_L12
0 probe:add_nr_running_L13
0 probe:newidle_balance_L38
0 probe:update_sd_lb_stats_L51

stress-ng --cpu=800 -l 50
2K probe:add_nr_running_L12
0 probe:add_nr_running_L13
2K probe:newidle_balance_L38
946 probe:update_sd_lb_stats_L51

stress-ng --cpu=800 -l 100
361 probe:add_nr_running_L12
0 probe:add_nr_running_L13
0 probe:newidle_balance_L38
0 probe:update_sd_lb_stats_L51

L13 numbers are quite less compared to L12. This indicates that it might
not change often.

------------------------------------------------------------------------------
==========
With Patch:
==========
Perf annoate while running "schbench -t 320 -i 30 -r 30"
       │     if (!READ_ONCE(this_rq->rd->overload) ||
       │       ld       r9,2752(r31)
       │     sd = rcu_dereference_check_sched_domain(this_rq->sd);
       │       ld       r30,2760(r31)
       │     if (!READ_ONCE(this_rq->rd->overload) ||
       │       lwz      r9,536(r9)
       │       cmpwi    r9,0
       │     ↓ beq      2b4
       │100:   mflr     r0
       │       cmpdi    r30,0
  0.38 │       std      r0,240(r1)
  1.56 │     ↓ beq      120


perf probe -L update_sd_lb_stats
     49         if (!env->sd->parent) {
     50                 int a;
                        /* update overload indicator if we are at root domain */
                        if ( READ_ONCE(env->dst_rq->rd->overload) != sg_status & SG_OVERLOAD) {
     53                         a= a+10;
                                WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
                        }

perf probe -L newidle_balance
     38         sd = rcu_dereference_check_sched_domain(this_rq->sd);

                if (!READ_ONCE(this_rq->rd->overload) ||
                    (sd && this_rq->avg_idle < sd->max_newidle_lb_cost)) {

perf probe -L add_nr_running
		#ifdef CONFIG_SMP
     11         if (prev_nr < 2 && rq->nr_running >= 2) {
     12                 if (!READ_ONCE(rq->rd->overload)) {
     13                         a = a +10;
                                WRITE_ONCE(rq->rd->overload, 1);
                        }

perf probes when running different workloads. How many times actual
value changes in update_sd_lb_stats is indicated as L53/L50*100.
idle
818 probe:newidle_balance_L38
262 probe:update_sd_lb_stats_L53	<-- 86%
321 probe:add_nr_running_L12
261 probe:add_nr_running_L13
304 probe:update_sd_lb_stats_L50

./hackbench 10 process 100000 loops
1M probe:newidle_balance_L38
139 probe:update_sd_lb_stats_L53	<-- 0.25%
129K probe:add_nr_running_L12
74 probe:add_nr_running_L13
54K probe:update_sd_lb_stats_L50

./schbench -t 320 -i 30 -r 30
101K probe:newidle_balance_L38
2K probe:update_sd_lb_stats_L53		<-- 9.09%
5K probe:add_nr_running_L12
1K probe:add_nr_running_L13
22K probe:update_sd_lb_stats_L50

Modified stress-ng --wait
6M probe:newidle_balance_L38
2K probe:update_sd_lb_stats_L53		<-- 25%
4K probe:add_nr_running_L12
2K probe:add_nr_running_L13
8K probe:update_sd_lb_stats_L50

stress-ng --cpu=400 -l 20
2K probe:newidle_balance_L38
286 probe:update_sd_lb_stats_L53	<-- 36.11%
746 probe:add_nr_running_L12
256 probe:add_nr_running_L13
792 probe:update_sd_lb_stats_L50

stress-ng --cpu=400 -l 100
2 probe:newidle_balance_L38
0 probe:update_sd_lb_stats_L53		<-- NA
923 probe:add_nr_running_L12
0 probe:add_nr_running_L13
0 probe:update_sd_lb_stats_L50

stress-ng --cpu=800 -l 50
2K probe:newidle_balance_L38
0 probe:update_sd_lb_stats_L53		<-- 0%
2K probe:add_nr_running_L12
0 probe:add_nr_running_L13
429 probe:update_sd_lb_stats_L50

stress-ng --cpu=800 -l 100
0 probe:newidle_balance_L38
0 probe:update_sd_lb_stats_L53		<-- NA
424 probe:add_nr_running_L12
0 probe:add_nr_running_L13
1 probe:update_sd_lb_stats_L50

This indicates that likely that value changes less often. So adding a
read before update would help in generic workloads.
-------------------------------------------------------------------------------

Shrikanth Hegde (2):
  sched/fair: Check rd->overload value before update
  sched/fair: Use helper functions to access rd->overload

 kernel/sched/fair.c  |  6 ++++--
 kernel/sched/sched.h | 14 ++++++++++++--
 2 files changed, 16 insertions(+), 4 deletions(-)

--
2.39.3


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] sched/fair: Check rd->overload value before update
  2024-03-13 15:01 [PATCH 0/2] sched: Minor changes for rd->overload access Shrikanth Hegde
@ 2024-03-13 15:01 ` Shrikanth Hegde
  2024-03-21  7:44   ` Vincent Guittot
  2024-03-13 15:01 ` [PATCH 2/2] sched/fair: Use helper functions to access rd->overload Shrikanth Hegde
  1 sibling, 1 reply; 6+ messages in thread
From: Shrikanth Hegde @ 2024-03-13 15:01 UTC (permalink / raw
  To: mingo, peterz, vincent.guittot
  Cc: sshegde, dietmar.eggemann, qyousef, linux-kernel, vschneid,
	qperret, yu.c.chen, srikar, pierre.gondois

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 <sshegde@linux.ibm.com>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] sched/fair: Use helper functions to access rd->overload
  2024-03-13 15:01 [PATCH 0/2] sched: Minor changes for rd->overload access Shrikanth Hegde
  2024-03-13 15:01 ` [PATCH 1/2] sched/fair: Check rd->overload value before update Shrikanth Hegde
@ 2024-03-13 15:01 ` Shrikanth Hegde
  2024-03-21  8:01   ` Vincent Guittot
  1 sibling, 1 reply; 6+ messages in thread
From: Shrikanth Hegde @ 2024-03-13 15:01 UTC (permalink / raw
  To: mingo, peterz, vincent.guittot
  Cc: sshegde, dietmar.eggemann, qyousef, linux-kernel, vschneid,
	qperret, yu.c.chen, srikar, pierre.gondois

rd->overload is accessed at multiple places. Instead it could use helper
get/set functions. This would make the code more readable and easy to
maintain.

No change in functionality intended.

Suggested-by: Qais Yousef <qyousef@layalina.io>
Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
 kernel/sched/fair.c  |  7 ++++---
 kernel/sched/sched.h | 14 ++++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index eeebadd7d9ae..cee99c93e6a4 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10621,8 +10621,9 @@ 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 */
-		if (READ_ONCE(env->dst_rq->rd->overload) != (sg_status & SG_OVERLOAD))
-			WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
+		if (is_rd_overloaded(env->dst_rq->rd) != (sg_status & SG_OVERLOAD))
+			set_rd_overload_status(env->dst_rq->rd,
+					       sg_status & SG_OVERLOAD);

 		/* Update over-utilization (tipping point, U >= 0) indicator */
 		set_rd_overutilized_status(env->dst_rq->rd,
@@ -12344,7 +12345,7 @@ static int sched_balance_newidle(struct rq *this_rq, struct rq_flags *rf)
 	rcu_read_lock();
 	sd = rcu_dereference_check_sched_domain(this_rq->sd);

-	if (!READ_ONCE(this_rq->rd->overload) ||
+	if (!is_rd_overloaded(this_rq->rd) ||
 	    (sd && this_rq->avg_idle < sd->max_newidle_lb_cost)) {

 		if (sd)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 41024c1c49b4..c91eb8811bef 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -918,6 +918,16 @@ extern void rq_attach_root(struct rq *rq, struct root_domain *rd);
 extern void sched_get_rd(struct root_domain *rd);
 extern void sched_put_rd(struct root_domain *rd);

+static inline int is_rd_overloaded(struct root_domain *rd)
+{
+	return READ_ONCE(rd->overload);
+}
+
+static inline void set_rd_overload_status(struct root_domain *rd, int status)
+{
+	WRITE_ONCE(rd->overload, status);
+}
+
 #ifdef HAVE_RT_PUSH_IPI
 extern void rto_push_irq_work_func(struct irq_work *work);
 #endif
@@ -2518,8 +2528,8 @@ static inline void add_nr_running(struct rq *rq, unsigned count)

 #ifdef CONFIG_SMP
 	if (prev_nr < 2 && rq->nr_running >= 2) {
-		if (!READ_ONCE(rq->rd->overload))
-			WRITE_ONCE(rq->rd->overload, 1);
+		if (!is_rd_overloaded(rq->rd))
+			set_rd_overload_status(rq->rd, 1);
 	}
 #endif

--
2.39.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] sched/fair: Check rd->overload value before update
  2024-03-13 15:01 ` [PATCH 1/2] sched/fair: Check rd->overload value before update Shrikanth Hegde
@ 2024-03-21  7:44   ` Vincent Guittot
  2024-03-21 17:21     ` Shrikanth Hegde
  0 siblings, 1 reply; 6+ messages in thread
From: Vincent Guittot @ 2024-03-21  7:44 UTC (permalink / raw
  To: Shrikanth Hegde
  Cc: mingo, peterz, dietmar.eggemann, qyousef, linux-kernel, vschneid,
	qperret, yu.c.chen, srikar, pierre.gondois

On Wed, 13 Mar 2024 at 16:02, Shrikanth Hegde <sshegde@linux.ibm.com> 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 <vincent.guittot@linaro.org>

> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>  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
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] sched/fair: Use helper functions to access rd->overload
  2024-03-13 15:01 ` [PATCH 2/2] sched/fair: Use helper functions to access rd->overload Shrikanth Hegde
@ 2024-03-21  8:01   ` Vincent Guittot
  0 siblings, 0 replies; 6+ messages in thread
From: Vincent Guittot @ 2024-03-21  8:01 UTC (permalink / raw
  To: Shrikanth Hegde
  Cc: mingo, peterz, dietmar.eggemann, qyousef, linux-kernel, vschneid,
	qperret, yu.c.chen, srikar, pierre.gondois

On Wed, 13 Mar 2024 at 16:02, Shrikanth Hegde <sshegde@linux.ibm.com> wrote:
>
> rd->overload is accessed at multiple places. Instead it could use helper
> get/set functions. This would make the code more readable and easy to
> maintain.
>
> No change in functionality intended.
>
> Suggested-by: Qais Yousef <qyousef@layalina.io>
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>

Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>

> ---
>  kernel/sched/fair.c  |  7 ++++---
>  kernel/sched/sched.h | 14 ++++++++++++--
>  2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index eeebadd7d9ae..cee99c93e6a4 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10621,8 +10621,9 @@ 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 */
> -               if (READ_ONCE(env->dst_rq->rd->overload) != (sg_status & SG_OVERLOAD))
> -                       WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
> +               if (is_rd_overloaded(env->dst_rq->rd) != (sg_status & SG_OVERLOAD))
> +                       set_rd_overload_status(env->dst_rq->rd,
> +                                              sg_status & SG_OVERLOAD);
>
>                 /* Update over-utilization (tipping point, U >= 0) indicator */
>                 set_rd_overutilized_status(env->dst_rq->rd,
> @@ -12344,7 +12345,7 @@ static int sched_balance_newidle(struct rq *this_rq, struct rq_flags *rf)
>         rcu_read_lock();
>         sd = rcu_dereference_check_sched_domain(this_rq->sd);
>
> -       if (!READ_ONCE(this_rq->rd->overload) ||
> +       if (!is_rd_overloaded(this_rq->rd) ||
>             (sd && this_rq->avg_idle < sd->max_newidle_lb_cost)) {
>
>                 if (sd)
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 41024c1c49b4..c91eb8811bef 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -918,6 +918,16 @@ extern void rq_attach_root(struct rq *rq, struct root_domain *rd);
>  extern void sched_get_rd(struct root_domain *rd);
>  extern void sched_put_rd(struct root_domain *rd);
>
> +static inline int is_rd_overloaded(struct root_domain *rd)
> +{
> +       return READ_ONCE(rd->overload);
> +}
> +
> +static inline void set_rd_overload_status(struct root_domain *rd, int status)
> +{
> +       WRITE_ONCE(rd->overload, status);
> +}
> +
>  #ifdef HAVE_RT_PUSH_IPI
>  extern void rto_push_irq_work_func(struct irq_work *work);
>  #endif
> @@ -2518,8 +2528,8 @@ static inline void add_nr_running(struct rq *rq, unsigned count)
>
>  #ifdef CONFIG_SMP
>         if (prev_nr < 2 && rq->nr_running >= 2) {
> -               if (!READ_ONCE(rq->rd->overload))
> -                       WRITE_ONCE(rq->rd->overload, 1);
> +               if (!is_rd_overloaded(rq->rd))
> +                       set_rd_overload_status(rq->rd, 1);
>         }
>  #endif
>
> --
> 2.39.3
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] sched/fair: Check rd->overload value before update
  2024-03-21  7:44   ` Vincent Guittot
@ 2024-03-21 17:21     ` Shrikanth Hegde
  0 siblings, 0 replies; 6+ messages in thread
From: Shrikanth Hegde @ 2024-03-21 17:21 UTC (permalink / raw
  To: Vincent Guittot
  Cc: mingo, peterz, dietmar.eggemann, qyousef, linux-kernel, vschneid,
	qperret, yu.c.chen, srikar, pierre.gondois



On 3/21/24 1:14 PM, Vincent Guittot wrote:
> On Wed, 13 Mar 2024 at 16:02, Shrikanth Hegde <sshegde@linux.ibm.com> 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 <vincent.guittot@linaro.org>
> 

Thank you!  I will send out v2 with these minor edits. 

>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>  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
>>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-03-21 17:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-13 15:01 [PATCH 0/2] sched: Minor changes for rd->overload access Shrikanth Hegde
2024-03-13 15:01 ` [PATCH 1/2] sched/fair: Check rd->overload value before update Shrikanth Hegde
2024-03-21  7:44   ` Vincent Guittot
2024-03-21 17:21     ` Shrikanth Hegde
2024-03-13 15:01 ` [PATCH 2/2] sched/fair: Use helper functions to access rd->overload Shrikanth Hegde
2024-03-21  8:01   ` Vincent Guittot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.