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