[PATCH v2 0/2] sched: use existing helper function for accessing avg_rt, avg_dl and avg_irq

[PATCH v2 0/2] sched: use existing helper function for accessing avg_rt, avg_dl and avg_irq

[Shrikanth Hegde]

Split the patches into two since there is subtle change with avg_irq.
There is no functionality change expected due to first patch, while
second patch may have. In case of any future regressions it would be
easy to bisect.

This is minor code simplification. Using existing helper function would
be simpler and easier to maintain.

v2->v1:
Vincent pointed out the READ_ONCE change for avg_irq. It would be
correct to use READ_ONCE for accessing avg_irq. Changed the current
helper function to do so.

v1: https://lore.kernel.org/lkml/20231220065522.351915-1-sshegde@linux.vnet.ibm.com/

Shrikanth Hegde (2):
  sched: use existing helper functions to access ->avg_rt and ->avg_dl
  sched: add READ_ONCE and use existing helper function to access ->avg_irq

 kernel/sched/fair.c  | 12 +++++-------
 kernel/sched/sched.h |  2 +-
 2 files changed, 6 insertions(+), 8 deletions(-)

--
2.39.3

[PATCH v2 1/2] sched: use existing helper functions to access ->avg_rt and ->avg_dl

[Shrikanth Hegde]

There are helper functions called cpu_util_dl and cpu_util_rt which gives
the average utilization of DL and RT respectively. But there are few
places in code where these variables are used directly.

Instead use the helper function so that code becomes simpler and easy to
maintain later on. This patch doesn't intend any functional changes.

Signed-off-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>
---
 kernel/sched/fair.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index bcea3d55d95d..1aeca3f943a8 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9212,10 +9212,10 @@ static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq)

 static inline bool others_have_blocked(struct rq *rq)
 {
-	if (READ_ONCE(rq->avg_rt.util_avg))
+	if (cpu_util_rt(rq))
 		return true;

-	if (READ_ONCE(rq->avg_dl.util_avg))
+	if (cpu_util_dl(rq))
 		return true;

 	if (thermal_load_avg(rq))
@@ -9481,8 +9481,8 @@ static unsigned long scale_rt_capacity(int cpu)
 	 * avg_thermal.load_avg tracks thermal pressure and the weighted
 	 * average uses the actual delta max capacity(load).
 	 */
-	used = READ_ONCE(rq->avg_rt.util_avg);
-	used += READ_ONCE(rq->avg_dl.util_avg);
+	used = cpu_util_rt(rq);
+	used += cpu_util_dl(rq);
 	used += thermal_load_avg(rq);

 	if (unlikely(used >= max))
--
2.39.3

Re: [PATCH v2 1/2] sched: use existing helper functions to access ->avg_rt and ->avg_dl

[Vincent Guittot]

On Mon, 1 Jan 2024 at 16:49, Shrikanth Hegde <sshegde@linux.vnet.ibm.com> wrote:
>
> There are helper functions called cpu_util_dl and cpu_util_rt which gives
> the average utilization of DL and RT respectively. But there are few
> places in code where these variables are used directly.
>
> Instead use the helper function so that code becomes simpler and easy to
> maintain later on. This patch doesn't intend any functional changes.
>
> Signed-off-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>

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

> ---
>  kernel/sched/fair.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index bcea3d55d95d..1aeca3f943a8 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9212,10 +9212,10 @@ static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq)
>
>  static inline bool others_have_blocked(struct rq *rq)
>  {
> -       if (READ_ONCE(rq->avg_rt.util_avg))
> +       if (cpu_util_rt(rq))
>                 return true;
>
> -       if (READ_ONCE(rq->avg_dl.util_avg))
> +       if (cpu_util_dl(rq))
>                 return true;
>
>         if (thermal_load_avg(rq))
> @@ -9481,8 +9481,8 @@ static unsigned long scale_rt_capacity(int cpu)
>          * avg_thermal.load_avg tracks thermal pressure and the weighted
>          * average uses the actual delta max capacity(load).
>          */
> -       used = READ_ONCE(rq->avg_rt.util_avg);
> -       used += READ_ONCE(rq->avg_dl.util_avg);
> +       used = cpu_util_rt(rq);
> +       used += cpu_util_dl(rq);
>         used += thermal_load_avg(rq);
>
>         if (unlikely(used >= max))
> --
> 2.39.3
>

[tip: sched/core] sched/fair: Use existing helper functions to access ->avg_rt and ->avg_dl

[tip-bot2 for Shrikanth Hegde]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     8b936fc1d84f7d70009ea34d66bbf6c54c09fae7
Gitweb:        https://git.kernel.org/tip/8b936fc1d84f7d70009ea34d66bbf6c54c09fae7
Author:        Shrikanth Hegde <sshegde@linux.vnet.ibm.com>
AuthorDate:    Mon, 01 Jan 2024 21:16:23 +05:30
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 28 Feb 2024 15:11:14 +01:00

sched/fair: Use existing helper functions to access ->avg_rt and ->avg_dl

There are helper functions called cpu_util_dl() and cpu_util_rt() which give
the average utilization of DL and RT respectively. But there are a few
places in code where access to these variables is open-coded.

Instead use the helper function so that code becomes simpler and easier to
maintain later on.

No functional changes intended.

Signed-off-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
Link: https://lore.kernel.org/r/20240101154624.100981-2-sshegde@linux.vnet.ibm.com
---
 kernel/sched/fair.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 8e30e2b..127e727 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9237,10 +9237,10 @@ static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq)
 
 static inline bool others_have_blocked(struct rq *rq)
 {
-	if (READ_ONCE(rq->avg_rt.util_avg))
+	if (cpu_util_rt(rq))
 		return true;
 
-	if (READ_ONCE(rq->avg_dl.util_avg))
+	if (cpu_util_dl(rq))
 		return true;
 
 	if (thermal_load_avg(rq))
@@ -9506,8 +9506,8 @@ static unsigned long scale_rt_capacity(int cpu)
 	 * avg_thermal.load_avg tracks thermal pressure and the weighted
 	 * average uses the actual delta max capacity(load).
 	 */
-	used = READ_ONCE(rq->avg_rt.util_avg);
-	used += READ_ONCE(rq->avg_dl.util_avg);
+	used = cpu_util_rt(rq);
+	used += cpu_util_dl(rq);
 	used += thermal_load_avg(rq);
 
 	if (unlikely(used >= max))

[PATCH v2 2/2] sched: add READ_ONCE and use existing helper function to access ->avg_irq

[Shrikanth Hegde]

Use existing helper function cpu_util_irq instead of referencing it
directly.

It was noted that avg_irq could be updated by different CPU than the one
which is trying to access it. avg_irq is updated with WRITE_ONCE. Use
READ_ONCE to access it in order to avoid any compiler optimizations.

Signed-off-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>
---
 kernel/sched/fair.c  | 4 +---
 kernel/sched/sched.h | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 1aeca3f943a8..02631060ca7e 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9221,10 +9221,8 @@ static inline bool others_have_blocked(struct rq *rq)
 	if (thermal_load_avg(rq))
 		return true;

-#ifdef CONFIG_HAVE_SCHED_AVG_IRQ
-	if (READ_ONCE(rq->avg_irq.util_avg))
+	if (cpu_util_irq(rq))
 		return true;
-#endif

 	return false;
 }
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index e58a54bda77d..edc20c5cc7ce 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -3125,7 +3125,7 @@ static inline bool uclamp_rq_is_idle(struct rq *rq)
 #ifdef CONFIG_HAVE_SCHED_AVG_IRQ
 static inline unsigned long cpu_util_irq(struct rq *rq)
 {
-	return rq->avg_irq.util_avg;
+	return READ_ONCE(rq->avg_irq.util_avg);
 }

 static inline
--
2.39.3

Re: [PATCH v2 2/2] sched: add READ_ONCE and use existing helper function to access ->avg_irq

[Vincent Guittot]

On Mon, 1 Jan 2024 at 16:47, Shrikanth Hegde <sshegde@linux.vnet.ibm.com> wrote:
>
> Use existing helper function cpu_util_irq instead of referencing it
> directly.
>
> It was noted that avg_irq could be updated by different CPU than the one
> which is trying to access it. avg_irq is updated with WRITE_ONCE. Use
> READ_ONCE to access it in order to avoid any compiler optimizations.
>
> Signed-off-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>

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

> ---
>  kernel/sched/fair.c  | 4 +---
>  kernel/sched/sched.h | 2 +-
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 1aeca3f943a8..02631060ca7e 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9221,10 +9221,8 @@ static inline bool others_have_blocked(struct rq *rq)
>         if (thermal_load_avg(rq))
>                 return true;
>
> -#ifdef CONFIG_HAVE_SCHED_AVG_IRQ
> -       if (READ_ONCE(rq->avg_irq.util_avg))
> +       if (cpu_util_irq(rq))
>                 return true;
> -#endif
>
>         return false;
>  }
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index e58a54bda77d..edc20c5cc7ce 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3125,7 +3125,7 @@ static inline bool uclamp_rq_is_idle(struct rq *rq)
>  #ifdef CONFIG_HAVE_SCHED_AVG_IRQ
>  static inline unsigned long cpu_util_irq(struct rq *rq)
>  {
> -       return rq->avg_irq.util_avg;
> +       return READ_ONCE(rq->avg_irq.util_avg);
>  }
>
>  static inline
> --
> 2.39.3
>

[tip: sched/core] sched/fair: Add READ_ONCE() and use existing helper function to access ->avg_irq

[tip-bot2 for Shrikanth Hegde]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     a6965b31888501f889261a6783f0de6afff84f8d
Gitweb:        https://git.kernel.org/tip/a6965b31888501f889261a6783f0de6afff84f8d
Author:        Shrikanth Hegde <sshegde@linux.vnet.ibm.com>
AuthorDate:    Mon, 01 Jan 2024 21:16:24 +05:30
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 28 Feb 2024 15:11:15 +01:00

sched/fair: Add READ_ONCE() and use existing helper function to access ->avg_irq

Use existing helper function cpu_util_irq() instead of open-coding
access to ->avg_irq.

During review it was noted that ->avg_irq could be updated by a
different CPU than the one which is trying to access it.

->avg_irq is updated with WRITE_ONCE(), use READ_ONCE to access it
in order to avoid any compiler optimizations.

Signed-off-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
Link: https://lore.kernel.org/r/20240101154624.100981-3-sshegde@linux.vnet.ibm.com
---
 kernel/sched/fair.c  | 4 +---
 kernel/sched/sched.h | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 127e727..ba36339 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9246,10 +9246,8 @@ static inline bool others_have_blocked(struct rq *rq)
 	if (thermal_load_avg(rq))
 		return true;
 
-#ifdef CONFIG_HAVE_SCHED_AVG_IRQ
-	if (READ_ONCE(rq->avg_irq.util_avg))
+	if (cpu_util_irq(rq))
 		return true;
-#endif
 
 	return false;
 }
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 001fe04..d224267 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -3136,7 +3136,7 @@ static inline bool uclamp_rq_is_idle(struct rq *rq)
 #ifdef CONFIG_HAVE_SCHED_AVG_IRQ
 static inline unsigned long cpu_util_irq(struct rq *rq)
 {
-	return rq->avg_irq.util_avg;
+	return READ_ONCE(rq->avg_irq.util_avg);
 }
 
 static inline