[PATCH] hrtimer: select housekeeping CPU during migration

[PATCH] hrtimer: select housekeeping CPU during migration

[Costa Shulyupin]

because during CPU deactivation a timer can migrate
to isolated CPU and break CPU isolation.

For reference see function get_nohz_timer_target,
which selects CPU for new timers from housekeeping_cpumask(HK_TYPE_TIMER)

Inspired by Waiman Long <longman@redhat.com>

Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
---
 kernel/time/hrtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index f82997cf53b6..460d916e24b7 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -2227,7 +2227,7 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
 int hrtimers_cpu_dying(unsigned int dying_cpu)
 {
 	struct hrtimer_cpu_base *old_base, *new_base;
-	int i, ncpu = cpumask_first(cpu_active_mask);
+	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));
 	pr_debug("ncpu=%d, dying_cpu=%d\n", ncpu, dying_cpu);
 
 	tick_cancel_sched_timer(dying_cpu);
-- 
2.43.0

Re: [PATCH] hrtimer: select housekeeping CPU during migration

[Waiman Long]

On 2/11/24 08:52, Costa Shulyupin wrote:
> because during CPU deactivation a timer can migrate
> to isolated CPU and break CPU isolation.
>
> For reference see function get_nohz_timer_target,
> which selects CPU for new timers from housekeeping_cpumask(HK_TYPE_TIMER)
>
> Inspired by Waiman Long <longman@redhat.com>
>
> Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>

Nit: It is unusual to begin the commit log with "because".

Other than that, the patch looks good to me.

Reviewed-by:  Waiman Long <longman@redhat.com>

> ---
>   kernel/time/hrtimer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index f82997cf53b6..460d916e24b7 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -2227,7 +2227,7 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
>   int hrtimers_cpu_dying(unsigned int dying_cpu)
>   {
>   	struct hrtimer_cpu_base *old_base, *new_base;
> -	int i, ncpu = cpumask_first(cpu_active_mask);
> +	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));
>   	pr_debug("ncpu=%d, dying_cpu=%d\n", ncpu, dying_cpu);
>   
>   	tick_cancel_sched_timer(dying_cpu);

Re: [PATCH] hrtimer: select housekeeping CPU during migration

[Frederic Weisbecker]

Le Sun, Feb 11, 2024 at 03:52:13PM +0200, Costa Shulyupin a écrit :
> because during CPU deactivation a timer can migrate
> to isolated CPU and break CPU isolation.
> 
> For reference see function get_nohz_timer_target,
> which selects CPU for new timers from housekeeping_cpumask(HK_TYPE_TIMER)
> 
> Inspired by Waiman Long <longman@redhat.com>
> 
> Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Re: [PATCH] hrtimer: select housekeeping CPU during migration

[Thomas Gleixner]

On Sun, Feb 11 2024 at 15:52, Costa Shulyupin wrote:
> because during CPU deactivation a timer can migrate
> to isolated CPU and break CPU isolation.

That's not a sentence.

> For reference see function get_nohz_timer_target,

get_nohz_timer_target()

> which selects CPU for new timers from
> housekeeping_cpumask(HK_TYPE_TIMER)

But what is the point of this statement?

> Inspired by Waiman Long <longman@redhat.com>

Can you please use a proper tag, i.e. Suggested-by and not invent some
random free form text just because?

> Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
> ---
>  kernel/time/hrtimer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index f82997cf53b6..460d916e24b7 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -2227,7 +2227,7 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
>  int hrtimers_cpu_dying(unsigned int dying_cpu)
>  {
>  	struct hrtimer_cpu_base *old_base, *new_base;
> -	int i, ncpu = cpumask_first(cpu_active_mask);
> +	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));
>  	pr_debug("ncpu=%d, dying_cpu=%d\n", ncpu, dying_cpu);
>  
>  	tick_cancel_sched_timer(dying_cpu);

Q: Against which tree is this supposed to apply?

A: Against some private tree of yours which added the pr_debug() in a
   previous commit.

Can you please read and follow Documentation/process/ and provide
patches which actually can be applied without fixing them up manually?

Thanks,

        tglx

        

[PATCH v2] hrtimer: select housekeeping CPU during migration

[Costa Shulyupin]

During CPU-down hotplug, hrtimers may migrate to isolated CPUs,
compromising CPU isolation. This commit addresses this issue by
masking valid CPUs for hrtimers using housekeeping_cpumask(HK_TYPE_TIMER).

Suggested-by: Waiman Long <longman@redhat.com>
Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
Reviewed-by: Waiman Long <longman@redhat.com>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
---

Changes in v2:
- [v1] https://lore.kernel.org/all/20240211135213.2518068-1-costa.shul@redhat.com/
- reworded and rebased on linux-next
---
 kernel/time/hrtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index edb0f821dcea..947bd6cf7105 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -2224,7 +2224,7 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
 int hrtimers_cpu_dying(unsigned int dying_cpu)
 {
 	struct hrtimer_cpu_base *old_base, *new_base;
-	int i, ncpu = cpumask_first(cpu_active_mask);
+	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));
 
 	tick_cancel_sched_timer(dying_cpu);
 
-- 
2.43.0

Re: [PATCH v2] hrtimer: select housekeeping CPU during migration

[Thomas Gleixner]

On Tue, Feb 13 2024 at 18:46, Costa Shulyupin wrote:

Way better now. Two nitpicks though:

> During CPU-down hotplug, hrtimers may migrate to isolated CPUs,
> compromising CPU isolation. This commit addresses this issue by
> masking valid CPUs for hrtimers using housekeeping_cpumask(HK_TYPE_TIMER).

'This commit' is pointless.

 # git grep 'This patch' Documentation/process/

gives you an hint.

> Suggested-by: Waiman Long <longman@redhat.com>
> Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
> Reviewed-by: Waiman Long <longman@redhat.com>
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> ---
>
> Changes in v2:
> - [v1] https://lore.kernel.org/all/20240211135213.2518068-1-costa.shul@redhat.com/
> - reworded and rebased on linux-next
> ---
>  kernel/time/hrtimer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index edb0f821dcea..947bd6cf7105 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -2224,7 +2224,7 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
>  int hrtimers_cpu_dying(unsigned int dying_cpu)
>  {
>  	struct hrtimer_cpu_base *old_base, *new_base;
> -	int i, ncpu = cpumask_first(cpu_active_mask);
> +	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));

The tip tree managed code has rules for variable declarations (and more):

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations

No need to send a v3, I fix it up this time.

Thanks,

        tglx

[tip: timers/core] hrtimer: Select housekeeping CPU during migration

[tip-bot2 for Costa Shulyupin]

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

Commit-ID:     86342554e102b0d18d50abec43d40f4fc92f1993
Gitweb:        https://git.kernel.org/tip/86342554e102b0d18d50abec43d40f4fc92f1993
Author:        Costa Shulyupin <costa.shul@redhat.com>
AuthorDate:    Tue, 13 Feb 2024 18:46:51 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 13 Feb 2024 20:44:14 +01:00

hrtimer: Select housekeeping CPU during migration

During CPU-down hotplug, hrtimers may migrate to isolated CPUs,
compromising CPU isolation.

Address this issue by masking valid CPUs for hrtimers using
housekeeping_cpumask(HK_TYPE_TIMER).

Suggested-by: Waiman Long <longman@redhat.com>
Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Waiman Long <longman@redhat.com>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Link: https://lore.kernel.org/r/20240213164650.2935909-3-costa.shul@redhat.com
---
 kernel/time/hrtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 7607939..438208a 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -2221,8 +2221,8 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
 
 int hrtimers_cpu_dying(unsigned int dying_cpu)
 {
+	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));
 	struct hrtimer_cpu_base *old_base, *new_base;
-	int i, ncpu = cpumask_first(cpu_active_mask);
 
 	tick_cancel_sched_timer(dying_cpu);
 

Re: [PATCH v2] hrtimer: select housekeeping CPU during migration

[Borislav Petkov]

On Tue, Feb 13, 2024 at 06:46:51PM +0200, Costa Shulyupin wrote:
> During CPU-down hotplug, hrtimers may migrate to isolated CPUs,
> compromising CPU isolation. This commit addresses this issue by
> masking valid CPUs for hrtimers using housekeeping_cpumask(HK_TYPE_TIMER).
> 
> Suggested-by: Waiman Long <longman@redhat.com>
> Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
> Reviewed-by: Waiman Long <longman@redhat.com>
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> ---
> 
> Changes in v2:
> - [v1] https://lore.kernel.org/all/20240211135213.2518068-1-costa.shul@redhat.com/
> - reworded and rebased on linux-next
> ---
>  kernel/time/hrtimer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index edb0f821dcea..947bd6cf7105 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -2224,7 +2224,7 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
>  int hrtimers_cpu_dying(unsigned int dying_cpu)
>  {
>  	struct hrtimer_cpu_base *old_base, *new_base;
> -	int i, ncpu = cpumask_first(cpu_active_mask);
> +	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));
							^^^^^^^^^^^^^^
How was this patch tested?

It even says housekeeping_cpumask() in the commit message so the
*intent* to use the correct function is there:

kernel/time/hrtimer.c: In function ‘hrtimers_cpu_dying’:
kernel/time/hrtimer.c:2226:56: error: implicit declaration of function ‘housekeeping’ [-Werror=implicit-function-declaration]
 2226 |         int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));
      |                                                        ^~~~~~~~~~~~
./include/linux/cpumask.h:774:67: note: in definition of macro ‘cpumask_any_and’
  774 | #define cpumask_any_and(mask1, mask2) cpumask_first_and((mask1), (mask2))
      |                                                                   ^~~~~
kernel/time/hrtimer.c:2226:69: error: ‘HK_TYPE_TIMER’ undeclared (first use in this function)
 2226 |         int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));
      |                                                                     ^~~~~~~~~~~~~
./include/linux/cpumask.h:774:67: note: in definition of macro ‘cpumask_any_and’
  774 | #define cpumask_any_and(mask1, mask2) cpumask_first_and((mask1), (mask2))
      |                                                                   ^~~~~
kernel/time/hrtimer.c:2226:69: note: each undeclared identifier is reported only once for each function it appears in
 2226 |         int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));
      |                                                                     ^~~~~~~~~~~~~
./include/linux/cpumask.h:774:67: note: in definition of macro ‘cpumask_any_and’
  774 | #define cpumask_any_and(mask1, mask2) cpumask_first_and((mask1), (mask2))
      |                                                                   ^~~~~
cc1: some warnings being treated as errors
make[4]: *** [scripts/Makefile.build:243: kernel/time/hrtimer.o] Error 1
make[3]: *** [scripts/Makefile.build:481: kernel/time] Error 2
make[2]: *** [scripts/Makefile.build:481: kernel] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/mnt/kernel/kernel/2nd/linux/Makefile:1921: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2

This makes it build again at least:

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 2cbdf64d746c..6057fe2e179b 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -38,6 +38,7 @@
 #include <linux/sched/deadline.h>
 #include <linux/sched/nohz.h>
 #include <linux/sched/debug.h>
+#include <linux/sched/isolation.h>
 #include <linux/timer.h>
 #include <linux/freezer.h>
 #include <linux/compat.h>
@@ -2223,7 +2224,7 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
 
 int hrtimers_cpu_dying(unsigned int dying_cpu)
 {
-	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));
+	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping_cpumask(HK_TYPE_TIMER));
 	struct hrtimer_cpu_base *old_base, *new_base;
 
 	tick_cancel_sched_timer(dying_cpu);


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2] hrtimer: select housekeeping CPU during migration

[Thomas Gleixner]

On Tue, Feb 13 2024 at 18:46, Costa Shulyupin wrote:
> @@ -2224,7 +2224,7 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
>  int hrtimers_cpu_dying(unsigned int dying_cpu)
>  {
>  	struct hrtimer_cpu_base *old_base, *new_base;
> -	int i, ncpu = cpumask_first(cpu_active_mask);
> +	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping(HK_TYPE_TIMER));

kernel/time/hrtimer.c:2224:56: error: implicit declaration of function ‘housekeeping’ [-Werror=implicit-function-declaration]

Sigh,

[PATCH v3] hrtimer: select housekeeping CPU during migration

[Costa Shulyupin]

During CPU-down hotplug, hrtimers may migrate to isolated CPUs,
compromising CPU isolation. This commit addresses this issue by
masking valid CPUs for hrtimers using housekeeping_cpumask(HK_TYPE_TIMER).

Suggested-by: Waiman Long <longman@redhat.com>
Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
Reviewed-by: Waiman Long <longman@redhat.com>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
---

Changes in v3:
- fixed compilation errors
Changes in v2:
- [v1] https://lore.kernel.org/all/20240211135213.2518068-1-costa.shul@redhat.com/
- reworded and rebased on next

---
 kernel/time/hrtimer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index edb0f821dcea..6057fe2e179b 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -38,6 +38,7 @@
 #include <linux/sched/deadline.h>
 #include <linux/sched/nohz.h>
 #include <linux/sched/debug.h>
+#include <linux/sched/isolation.h>
 #include <linux/timer.h>
 #include <linux/freezer.h>
 #include <linux/compat.h>
@@ -2223,8 +2224,8 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
 
 int hrtimers_cpu_dying(unsigned int dying_cpu)
 {
+	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping_cpumask(HK_TYPE_TIMER));
 	struct hrtimer_cpu_base *old_base, *new_base;
-	int i, ncpu = cpumask_first(cpu_active_mask);
 
 	tick_cancel_sched_timer(dying_cpu);
 
-- 
2.43.0

Re: [PATCH v3] hrtimer: select housekeeping CPU during migration

[Thomas Gleixner]

Costa!

On Mon, Feb 19 2024 at 16:33, Costa Shulyupin wrote:
> During CPU-down hotplug, hrtimers may migrate to isolated CPUs,
> compromising CPU isolation. This commit addresses this issue by
> masking valid CPUs for hrtimers using housekeeping_cpumask(HK_TYPE_TIMER).

Review feedback is meant to be addressed. I fixed it up for you in V2
and told you what needs to be changed and you got the fixed up version
in your inbox. Feel free to ignore me, but don't expect me to mop up
your stuff.

Thanks,

        tglx

[PATCH v4] hrtimer: select housekeeping CPU during migration

[Costa Shulyupin]

During CPU-down hotplug, hrtimers may migrate to isolated CPUs,
compromising CPU isolation.

Address this issue by masking valid CPUs for hrtimers using
housekeeping_cpumask(HK_TYPE_TIMER).

Suggested-by: Waiman Long <longman@redhat.com>
Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
Reviewed-by: Waiman Long <longman@redhat.com>
---

Changes in v4:
- fixed description
Changes in v3:
- fixed compilation errors
Changes in v2:
- [v1] https://lore.kernel.org/all/20240211135213.2518068-1-costa.shul@redhat.com/
- reworded and rebased on next

---
 kernel/time/hrtimer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 5a98b35b0576..1fd106af747d 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -38,6 +38,7 @@
 #include <linux/sched/deadline.h>
 #include <linux/sched/nohz.h>
 #include <linux/sched/debug.h>
+#include <linux/sched/isolation.h>
 #include <linux/timer.h>
 #include <linux/freezer.h>
 #include <linux/compat.h>
@@ -2225,8 +2226,8 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
 
 int hrtimers_cpu_dying(unsigned int dying_cpu)
 {
+	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping_cpumask(HK_TYPE_TIMER));
 	struct hrtimer_cpu_base *old_base, *new_base;
-	int i, ncpu = cpumask_first(cpu_active_mask);
 
 	tick_cancel_sched_timer(dying_cpu);
 
-- 
2.43.0

[tip: timers/core] hrtimer: Select housekeeping CPU during migration

[tip-bot2 for Costa Shulyupin]

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

Commit-ID:     56c2cb10120894be40c40a9bf0ce798da14c50f6
Gitweb:        https://git.kernel.org/tip/56c2cb10120894be40c40a9bf0ce798da14c50f6
Author:        Costa Shulyupin <costa.shul@redhat.com>
AuthorDate:    Thu, 22 Feb 2024 22:08:56 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 22 Feb 2024 22:18:21 +01:00

hrtimer: Select housekeeping CPU during migration

During CPU-down hotplug, hrtimers may migrate to isolated CPUs,
compromising CPU isolation.

Address this issue by masking valid CPUs for hrtimers using
housekeeping_cpumask(HK_TYPE_TIMER).

Suggested-by: Waiman Long <longman@redhat.com>
Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Waiman Long <longman@redhat.com>
Link: https://lore.kernel.org/r/20240222200856.569036-1-costa.shul@redhat.com
---
 kernel/time/hrtimer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 5a98b35..1fd106a 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -38,6 +38,7 @@
 #include <linux/sched/deadline.h>
 #include <linux/sched/nohz.h>
 #include <linux/sched/debug.h>
+#include <linux/sched/isolation.h>
 #include <linux/timer.h>
 #include <linux/freezer.h>
 #include <linux/compat.h>
@@ -2225,8 +2226,8 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
 
 int hrtimers_cpu_dying(unsigned int dying_cpu)
 {
+	int i, ncpu = cpumask_any_and(cpu_active_mask, housekeeping_cpumask(HK_TYPE_TIMER));
 	struct hrtimer_cpu_base *old_base, *new_base;
-	int i, ncpu = cpumask_first(cpu_active_mask);
 
 	tick_cancel_sched_timer(dying_cpu);