2024-02-11 13:52:36

by Costa Shulyupin

[permalink] [raw]
Subject: [PATCH] hrtimer: select housekeeping CPU during migration

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 <[email protected]>

Signed-off-by: Costa Shulyupin <[email protected]>
---
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



2024-02-11 16:38:04

by Waiman Long

[permalink] [raw]
Subject: Re: [PATCH] hrtimer: select housekeeping CPU during migration


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 <[email protected]>
>
> Signed-off-by: Costa Shulyupin <[email protected]>

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

Other than that, the patch looks good to me.

Reviewed-by:  Waiman Long <[email protected]>

> ---
> 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);


2024-02-12 15:24:01

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [PATCH] hrtimer: select housekeeping CPU during migration

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 <[email protected]>
>
> Signed-off-by: Costa Shulyupin <[email protected]>

Reviewed-by: Frederic Weisbecker <[email protected]>

2024-02-13 12:37:47

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] hrtimer: select housekeeping CPU during migration

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 <[email protected]>

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 <[email protected]>
> ---
> 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



2024-02-13 16:51:32

by Costa Shulyupin

[permalink] [raw]
Subject: [PATCH v2] hrtimer: select housekeeping CPU during migration

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 <[email protected]>
Signed-off-by: Costa Shulyupin <[email protected]>
Reviewed-by: Waiman Long <[email protected]>
Reviewed-by: Frederic Weisbecker <[email protected]>
---

Changes in v2:
- [v1] https://lore.kernel.org/all/[email protected]/
- 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


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

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 <[email protected]>
AuthorDate: Tue, 13 Feb 2024 18:46:51 +02:00
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Costa Shulyupin <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Waiman Long <[email protected]>
Reviewed-by: Frederic Weisbecker <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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);


2024-02-13 20:03:11

by Thomas Gleixner

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

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 <[email protected]>
> Signed-off-by: Costa Shulyupin <[email protected]>
> Reviewed-by: Waiman Long <[email protected]>
> Reviewed-by: Frederic Weisbecker <[email protected]>
> ---
>
> Changes in v2:
> - [v1] https://lore.kernel.org/all/[email protected]/
> - 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

2024-02-14 09:01:54

by Borislav Petkov

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

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 <[email protected]>
> Signed-off-by: Costa Shulyupin <[email protected]>
> Reviewed-by: Waiman Long <[email protected]>
> Reviewed-by: Frederic Weisbecker <[email protected]>
> ---
>
> Changes in v2:
> - [v1] https://lore.kernel.org/all/[email protected]/
> - 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

2024-02-14 09:10:43

by Thomas Gleixner

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

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,

2024-02-19 14:33:42

by Costa Shulyupin

[permalink] [raw]
Subject: [PATCH v3] hrtimer: select housekeeping CPU during migration

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 <[email protected]>
Signed-off-by: Costa Shulyupin <[email protected]>
Reviewed-by: Waiman Long <[email protected]>
Reviewed-by: Frederic Weisbecker <[email protected]>
---

Changes in v3:
- fixed compilation errors
Changes in v2:
- [v1] https://lore.kernel.org/all/[email protected]/
- 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


2024-02-19 16:27:31

by Thomas Gleixner

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

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



2024-02-22 20:35:24

by Costa Shulyupin

[permalink] [raw]
Subject: [PATCH v4] 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 <[email protected]>
Signed-off-by: Costa Shulyupin <[email protected]>
Reviewed-by: Waiman Long <[email protected]>
---

Changes in v4:
- fixed description
Changes in v3:
- fixed compilation errors
Changes in v2:
- [v1] https://lore.kernel.org/all/[email protected]/
- 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


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

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 <[email protected]>
AuthorDate: Thu, 22 Feb 2024 22:08:56 +02:00
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Costa Shulyupin <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Waiman Long <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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);