Messages in this thread Patch in this message | | | Date | Mon, 15 Nov 2021 16:51:34 +0100 | From | Peter Zijlstra <> | Subject | Re: [GIT pull] timers/urgent for v5.16-rc1 |
| |
On Sun, Nov 14, 2021 at 11:02:31AM -0800, Linus Torvalds wrote: > On Sun, Nov 14, 2021 at 5:31 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> But apparently it matters for posix_cputimers_work and for numa_work, > and so I think it's very illogical that init_task_work() will not > actually initialize it properly.
The problem with the posix timers thing seems to be that it can race against fork() but afaict it can't actually mis-behave if it has garbage in ->next, so the clearing here is pure paranoia.
> And that task_tick_numa() special case is truly horrendous, and really > should go after the init_task_work() regardless, exactly because you'd > expect that init_task_work() to initialize the work even if it doesn't > happen to right now.
Yeah, it's a wee bit 'special' allright :-)
> Or is somebody doing init_task_work() to only change the work-function > on an already initialized work entry? Becuase that sounds both racy > and broken to me, and none of the things I looked at from a quick grep > looked like that at all.
The worst I found is someone sharing an rcu_head between task_work and call_rcu (supposedly at different stages in the life-time).
I couldn't find any other weird cases.
--- include/linux/task_work.h | 1 + kernel/sched/fair.c | 4 ++-- kernel/time/posix-cpu-timers.c | 2 -- 3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/include/linux/task_work.h b/include/linux/task_work.h index 5b8a93f288bb..fbbc9aa8e4ae 100644 --- a/include/linux/task_work.h +++ b/include/linux/task_work.h @@ -10,6 +10,7 @@ typedef void (*task_work_func_t)(struct callback_head *); static inline void init_task_work(struct callback_head *twork, task_work_func_t func) { + twork->next = NULL; twork->func = func; } diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 6e476f6d9435..d03dacdecf73 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -2823,14 +2823,14 @@ void init_numa_balancing(unsigned long clone_flags, struct task_struct *p) p->node_stamp = 0; p->numa_scan_seq = mm ? mm->numa_scan_seq : 0; p->numa_scan_period = sysctl_numa_balancing_scan_delay; - /* Protect against double add, see task_tick_numa and task_numa_work */ - p->numa_work.next = &p->numa_work; p->numa_faults = NULL; RCU_INIT_POINTER(p->numa_group, NULL); p->last_task_numa_placement = 0; p->last_sum_exec_runtime = 0; init_task_work(&p->numa_work, task_numa_work); + /* Protect against double add, see task_tick_numa and task_numa_work */ + p->numa_work.next = &p->numa_work; /* New address space, reset the preferred nid */ if (!(clone_flags & CLONE_VM)) { diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c index 96b4e7810426..3352759e6916 100644 --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -1167,8 +1167,6 @@ void clear_posix_cputimers_work(struct task_struct *p) * A copied work entry from the old task is not meaningful, clear it. * N.B. init_task_work will not do this. */ - memset(&p->posix_cputimers_work.work, 0, - sizeof(p->posix_cputimers_work.work)); init_task_work(&p->posix_cputimers_work.work, posix_cpu_timers_work); p->posix_cputimers_work.scheduled = false;
| |