posix-cpu-timers: Cleanup and consolidation
Subsystems (6)
cover posix-cpu-timers: Cleanup and consolidation
Folks!
While working on splitting the posix CPU timer expiry out into task context, I took a deeper look at that code.
It contains quite some duplicated code and the abuse of struct task_cputime along with the defines to rename the struct members just made my eyes bleed.
The following series cleans that up and consolidates and simplifies the implementation. The resulting .text is about 15% smaller than the orignal one.
It's split into small patches to make the review easier as this code is a true can of worms and the larger patches I had initially were just an invitation to overlook subtle issues.
The series applies on top of:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
and is available from git as well:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.timers/core
Thanks,
tglx
8<--------------
include/linux/alarmtimer.h | 3 include/linux/init_task.h | 11 include/linux/posix-timers.h | 112 +++- include/linux/sched.h | 26 - include/linux/sched/cputime.h | 12 include/linux/sched/signal.h | 13 include/linux/sched/types.h | 21 include/linux/timerqueue.h | 10 init/init_task.c | 2 kernel/fork.c | 34 - kernel/sched/rt.c | 6 kernel/time/alarmtimer.c | 14 kernel/time/itimer.c | 11 kernel/time/posix-cpu-timers.c | 1033 ++++++++++++++++++----------------------- kernel/time/posix-timers.c | 32 - kernel/time/posix-timers.h | 1 kernel/time/timer.c | 2 17 files changed, 659 insertions(+), 684 deletions(-)
PATCH 1/44 posix-timers: Cleanup forward declarations and includes
- Rename struct siginfo to kernel_siginfo
- Add a forward declaration for task_struct and remove sched.h include
- Remove timex.h include as it is not needed
--- include/linux/posix-timers.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
include/linux/posix-timers.h +2 -3
@@ -4,11 +4,10 @@#include <linux/spinlock.h> #include <linux/list.h> -#include <linux/sched.h> -#include <linux/timex.h> #include <linux/alarmtimer.h> -struct siginfo ; +struct kernel_siginfo ; +struct task_struct ;struct cpu_timer_list {struct list_head entry;
Frederic Weisbecker
maintainerRe: [patch 01/44] posix-timers: Cleanup forward declarations and includes
Quoted 1 line · Thomas Gleixner
- Rename struct siginfo to
kernel_siginfo
That's fine because struct siginfo isn't actually used in that header, right?
Quoted 26 lines
- Add a forward declaration for
task_structand removesched.hinclude
- Removetimex.hinclude as it is not neededSigned-off-by Thomas Gleixner <[email protected]>--- include/linux/posix-timers.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)include/linux/posix-timers.h +2
-3@@ -4,11 +4,10 @@#include <linux/spinlock.h> #include <linux/list.h> -#include <linux/sched.h> -#include <linux/timex.h> #include <linux/alarmtimer.h> -struct siginfo ; +struct kernel_siginfo ; +struct task_struct ;struct cpu_timer_list {struct list_head entry;
Re: [patch 01/44] posix-timers: Cleanup forward declarations and includes
Quoted 5 lines · Frederic Weisbecker
Quoted 1 line · Thomas Gleixner
- Rename struct siginfo to
kernel_siginfoThat's fine because struct siginfo isn't actually used in that header, right?
Yes, just kernel_siginfo needs a forward declaration.
Re: [patch 01/44] posix-timers: Cleanup forward declarations and includes
In reply to Thomas GleixnerQuoted 5 lines · Thomas Gleixner
- Rename struct siginfo to
kernel_siginfo
- Add a forward declaration fortask_structand removesched.hinclude
- Removetimex.hinclude as it is not neededSigned-off-by Thomas Gleixner <[email protected]>
[tip: timers/core] posix-timers: Cleanup forward declarations and includes
In reply to Thomas GleixnerThe following commit has been merged into the timers/core branch of tip:
The following commit has been merged into the timers/core branch of tip:
Commit-ID: ce03f613461642669d6150c405dd28f4bfd54bbb Gitweb: https://git.kernel.org/tip/ce03f613461642669d6150c405dd28f4bfd54bbb Author: Thomas Gleixner <[email protected]> AuthorDate: Mon, 19 Aug 2019 16:31:42 +02:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Tue, 20 Aug 2019 22:09:52 +02:00
posix-timers: Cleanup forward declarations and includes
- Rename struct siginfo to kernel_siginfo as that is used and required
- Add a forward declaration for task_struct and remove sched.h include
- Remove timex.h include as it is not needed
--- include/linux/posix-timers.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
include/linux/posix-timers.h +2 -3
@@ -4,11 +4,10 @@#include <linux/spinlock.h> #include <linux/list.h> -#include <linux/sched.h> -#include <linux/timex.h> #include <linux/alarmtimer.h> -struct siginfo ; +struct kernel_siginfo ; +struct task_struct ;struct cpu_timer_list {struct list_head entry;
PATCH 2/44 alarmtimers: Avoid rtc.h include
In reply to Thomas Gleixnerrtc.h is not needed in alarmtimers when a forward declaration of struct rtc_device is provided. That allows to include posix-timers.h without adding more includes to alarmtimer.h or creating circular include dependencies.
--- include/linux/alarmtimer.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
include/linux/alarmtimer.h +2 -1
@@ -5,7 +5,8 @@#include <linux/time.h> #include <linux/hrtimer.h> #include <linux/timerqueue.h> -#include <linux/rtc.h> + +struct rtc_device ;enum alarmtimer_type {ALARM_REALTIME ,
Re: [patch 02/44] alarmtimers: Avoid rtc.h include
Quoted 6 lines · Thomas Gleixner
rtc.his not needed in alarmtimers when a forward declaration of structrtc_deviceis provided. That allows to includeposix-timers.hwithout adding more includes toalarmtimer.hor creating circular include dependencies.Signed-off-by Thomas Gleixner <[email protected]>
Quoted 18 lines
--- include/linux/alarmtimer.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)include/linux/alarmtimer.h +2
-1@@ -5,7 +5,8 @@#include <linux/time.h> #include <linux/hrtimer.h> #include <linux/timerqueue.h> -#include <linux/rtc.h> + +struct rtc_device ;enum alarmtimer_type {ALARM_REALTIME ,
[tip: timers/core] alarmtimers: Avoid rtc.h include
In reply to Thomas GleixnerThe following commit has been merged into the timers/core branch of tip:
The following commit has been merged into the timers/core branch of tip:
Commit-ID: 3758b0f86ef502e2f342055caef6d2232c2558b7 Gitweb: https://git.kernel.org/tip/3758b0f86ef502e2f342055caef6d2232c2558b7 Author: Thomas Gleixner <[email protected]> AuthorDate: Mon, 19 Aug 2019 16:31:43 +02:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Tue, 20 Aug 2019 22:09:52 +02:00
alarmtimers: Avoid rtc.h include
rtc.h is not needed in alarmtimers when a forward declaration of struct rtc_device is provided. That allows to include posix-timers.h without adding more includes to alarmtimer.h or creating circular include dependencies.
--- include/linux/alarmtimer.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
include/linux/alarmtimer.h +2 -1
@@ -5,7 +5,8 @@#include <linux/time.h> #include <linux/hrtimer.h> #include <linux/timerqueue.h> -#include <linux/rtc.h> + +struct rtc_device ;enum alarmtimer_type {ALARM_REALTIME ,
PATCH 3/44 posix-timer: Use a callback for cancel synchronization
In reply to Thomas GleixnerCleanup timer_wait_running() by using a kclock callback. Get's rid of the ifdeffery and the odd conditionals. Should have thought about this right away.
--- kernel/time/alarmtimer.c | 14 ++++++++++++++ kernel/time/posix-timers.c | 32 ++++++++++++-------------------- kernel/time/posix-timers.h | 1 + 3 files changed, 27 insertions(+), 20 deletions(-)
kernel/time/alarmtimer.c +14 -0
@@ -606,6 +606,19 @@ static int alarm_timer_try_to_cancel(str }/** +* alarm_timer_wait_running - Posix timer callback to wait for a timer +* @timr: Pointer to the posixtimer data struct +* +* Called from the core code when timer cancel detected that the callback +* is running. @timr is unlocked and rcu read lock is held to prevent it +* from being freed. +*/ +static void alarm_timer_wait_running (struct k_itimer *timr) +{ +hrtimer_cancel_wait_running (&timr->it.alarm.alarmtimer.timer); +} + +/** * alarm_timer_arm - Posix timer callback to arm a timer * @timr: Pointer to the posixtimer data struct * @expires: The new expiry time
@@ -834,6 +847,7 @@ const struct k_clock alarm_clock = { .timer_forward = alarm_timer_forward, .timer_remaining = alarm_timer_remaining, .timer_try_to_cancel = alarm_timer_try_to_cancel, + .timer_wait_running = alarm_timer_wait_running, .nsleep = alarm_timer_nsleep, };#endif /* CONFIG_POSIX_TIMERS */
kernel/time/posix-timers.c +12 -20
@@ -805,7 +805,11 @@ static int common_hrtimer_try_to_cancel(return hrtimer_try_to_cancel (&timr->it.real.timer); } -#ifdef CONFIG_PREEMPT_RT +static void common_timer_wait_running (struct k_itimer *timer) +{ +hrtimer_cancel_wait_running (&timer->it.real.timer); +} +static struct k_itimer *timer_wait_running (struct k_itimer *timer,unsigned long *flags) {
@@ -816,29 +820,13 @@ static struct k_itimer *timer_wait_runnircu_read_lock ();unlock_timer (timer, *flags); -if (kc->timer_arm == common_hrtimer_arm) -hrtimer_cancel_wait_running (&timer->it.real.timer); -else if (kc == &alarm_clock) -hrtimer_cancel_wait_running (&timer->it.alarm.alarmtimer.timer); -else -WARN_ON_ONCE (1 ); -rcu_read_unlock (); - -/* Relock the timer. It might be not longer hashed. */ -return lock_timer (timer_id, flags); -} -#else -static struct k_itimer *timer_wait_running (struct k_itimer *timer, -unsigned long *flags) -{ -timer_t timer_id =READ_ONCE (timer->it_id); +if (!WARN_ON_ONCE (kc->timer_wait_running)) + kc->timer_wait_running (timer); -unlock_timer (timer, *flags); -cpu_relax (); +rcu_read_unlock ();/* Relock the timer. It might be not longer hashed. */ return lock_timer (timer_id, flags); } -#endif /* Set a POSIX.1b interval timer. */ int common_timer_set(struct k_itimer *timr, int flags,
@@ -1279,6 +1267,7 @@ static const struct k_clock clock_realti .timer_forward = common_hrtimer_forward, .timer_remaining = common_hrtimer_remaining, .timer_try_to_cancel = common_hrtimer_try_to_cancel, + .timer_wait_running = common_timer_wait_running, .timer_arm = common_hrtimer_arm, };
@@ -1294,6 +1283,7 @@ static const struct k_clock clock_monoto .timer_forward = common_hrtimer_forward, .timer_remaining = common_hrtimer_remaining, .timer_try_to_cancel = common_hrtimer_try_to_cancel, + .timer_wait_running = common_timer_wait_running, .timer_arm = common_hrtimer_arm, };
@@ -1324,6 +1314,7 @@ static const struct k_clock clock_tai = .timer_forward = common_hrtimer_forward, .timer_remaining = common_hrtimer_remaining, .timer_try_to_cancel = common_hrtimer_try_to_cancel, + .timer_wait_running = common_timer_wait_running, .timer_arm = common_hrtimer_arm, };
@@ -1339,6 +1330,7 @@ static const struct k_clock clock_bootti .timer_forward = common_hrtimer_forward, .timer_remaining = common_hrtimer_remaining, .timer_try_to_cancel = common_hrtimer_try_to_cancel, + .timer_wait_running = common_timer_wait_running, .timer_arm = common_hrtimer_arm, };
kernel/time/posix-timers.h +1 -0
@@ -24,6 +24,7 @@ struct k_clock {int (*timer_try_to_cancel)(struct k_itimer *timr);void (*timer_arm)(struct k_itimer *timr,ktime_t expires,bool absolute,bool sigev_none); +void (*timer_wait_running)(struct k_itimer *timr); };extern const struct k_clock clock_posix_cpu;
Re: [patch 03/44] posix-timer: Use a callback for cancel synchronization
kernel/time/posix-timers.c static struct k_itimer *timer_wait_runni · line 824
Quoted 2 lines
kernel/time/posix-timers.c +2
-0+if (!WARN_ON_ONCE (kc->timer_wait_running)) + kc->timer_wait_running (timer);
This looks weird. The only place calling yor new method only does so after checking that it is not set and actually warns if it set?
Re: [patch 03/44] posix-timer: Use a callback for cancel synchronization
Quoted 5 lines · Christoph Hellwig
kernel/time/posix-timers.c static struct k_itimer *timer_wait_runni · line 824
Quoted 2 lines
kernel/time/posix-timers.c +2
-0+if (!WARN_ON_ONCE (kc->timer_wait_running)) + kc->timer_wait_running (timer);This looks weird. The only place calling yor new method only does so after checking that it is not set and actually warns if it set?
Yeah, that wants to be;
if (!WARN_ON_ONCE(!kc->timer_wait_running)) kc->timer_wait_running(timer);
Re: [patch 03/44] posix-timer: Use a callback for cancel synchronization
In reply to Thomas Gleixnerkernel/time/posix-timers.c static struct k_itimer *timer_wait_runni · line 824
Quoted 6 lines · Thomas Gleixner
kernel/time/posix-timers.c +2
-4-static struct k_itimer *timer_wait_running (struct k_itimer *timer, -unsigned long *flags) -{ -timer_t timer_id =READ_ONCE (timer->it_id); +if (!WARN_ON_ONCE (kc->timer_wait_running)) + kc->timer_wait_running (timer);
With the fix after Christoph pinpoint:
[tip: timers/core] posix-timers: Use a callback for cancel synchronization on PREEMPT_RT
In reply to Thomas GleixnerThe following commit has been merged into the timers/core branch of tip:
The following commit has been merged into the timers/core branch of tip:
Commit-ID: ec8f954a40da8cd3d159713b608e901f0cd909a9 Gitweb: https://git.kernel.org/tip/ec8f954a40da8cd3d159713b608e901f0cd909a9 Author: Thomas Gleixner <[email protected]> AuthorDate: Fri, 02 Aug 2019 07:35:59 +02:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Tue, 20 Aug 2019 22:05:46 +02:00
posix-timers: Use a callback for cancel synchronization on PREEMPT_RT
Posix timer delete retry loops are affected by the same priority inversion and live lock issues as the other timers.
Provide a RT specific synchronization function which keeps a reference to the timer by holding rcu read lock to prevent the timer from being freed, dropping the timer lock and invoking the timer specific wait function via a new callback.
This does not yet cover posix CPU timers because they need more special treatment on PREEMPT_RT.
[ This is folded into the original attempt which did not use a callback. ]
--- kernel/time/alarmtimer.c | 14 ++++++++++++++ kernel/time/posix-timers.c | 18 +++++++++++++++++- kernel/time/posix-timers.h | 1 + 3 files changed, 32 insertions(+), 1 deletion(-)
kernel/time/alarmtimer.c +14 -0
@@ -606,6 +606,19 @@ static int alarm_timer_try_to_cancel(struct k_itimer *timr) }/** +* alarm_timer_wait_running - Posix timer callback to wait for a timer +* @timr: Pointer to the posixtimer data struct +* +* Called from the core code when timer cancel detected that the callback +* is running. @timr is unlocked and rcu read lock is held to prevent it +* from being freed. +*/ +static void alarm_timer_wait_running (struct k_itimer *timr) +{ +hrtimer_cancel_wait_running (&timr->it.alarm.alarmtimer.timer); +} + +/** * alarm_timer_arm - Posix timer callback to arm a timer * @timr: Pointer to the posixtimer data struct * @expires: The new expiry time
@@ -834,6 +847,7 @@ const struct k_clock alarm_clock = { .timer_forward = alarm_timer_forward, .timer_remaining = alarm_timer_remaining, .timer_try_to_cancel = alarm_timer_try_to_cancel, + .timer_wait_running = alarm_timer_wait_running, .nsleep = alarm_timer_nsleep, };#endif /* CONFIG_POSIX_TIMERS */
kernel/time/posix-timers.c +17 -1
@@ -805,13 +805,25 @@ static int common_hrtimer_try_to_cancel(struct k_itimer *timr)return hrtimer_try_to_cancel (&timr->it.real.timer); } +static void common_timer_wait_running (struct k_itimer *timer) +{ +hrtimer_cancel_wait_running (&timer->it.real.timer); +} +static struct k_itimer *timer_wait_running (struct k_itimer *timer,unsigned long *flags) { +const struct k_clock *kc =READ_ONCE (timer->kclock);timer_t timer_id =READ_ONCE (timer->it_id); +/* Prevent kfree(timer) after dropping the lock */ +rcu_read_lock ();unlock_timer (timer, *flags); -cpu_relax (); + +if (!WARN_ON_ONCE (!kc->timer_wait_running)) + kc->timer_wait_running (timer); + +rcu_read_unlock ();/* Relock the timer. It might be not longer hashed. */ return lock_timer (timer_id, flags); }
@@ -1255,6 +1267,7 @@ static const struct k_clock clock_realtime = { .timer_forward = common_hrtimer_forward, .timer_remaining = common_hrtimer_remaining, .timer_try_to_cancel = common_hrtimer_try_to_cancel, + .timer_wait_running = common_timer_wait_running, .timer_arm = common_hrtimer_arm, };
@@ -1270,6 +1283,7 @@ static const struct k_clock clock_monotonic = { .timer_forward = common_hrtimer_forward, .timer_remaining = common_hrtimer_remaining, .timer_try_to_cancel = common_hrtimer_try_to_cancel, + .timer_wait_running = common_timer_wait_running, .timer_arm = common_hrtimer_arm, };
@@ -1300,6 +1314,7 @@ static const struct k_clock clock_tai = { .timer_forward = common_hrtimer_forward, .timer_remaining = common_hrtimer_remaining, .timer_try_to_cancel = common_hrtimer_try_to_cancel, + .timer_wait_running = common_timer_wait_running, .timer_arm = common_hrtimer_arm, };
@@ -1315,6 +1330,7 @@ static const struct k_clock clock_boottime = { .timer_forward = common_hrtimer_forward, .timer_remaining = common_hrtimer_remaining, .timer_try_to_cancel = common_hrtimer_try_to_cancel, + .timer_wait_running = common_timer_wait_running, .timer_arm = common_hrtimer_arm, };
kernel/time/posix-timers.h +1 -0
@@ -24,6 +24,7 @@ struct k_clock {int (*timer_try_to_cancel)(struct k_itimer *timr);void (*timer_arm)(struct k_itimer *timr,ktime_t expires,bool absolute,bool sigev_none); +void (*timer_wait_running)(struct k_itimer *timr); };extern const struct k_clock clock_posix_cpu;
PATCH 4/44 posix-cpu-timers: Fixup stale comment
In reply to Thomas GleixnerThe comment above cleanup_timers() is outdated. The timers are only removed from the task/process list heads but not modified in any other way.
--- kernel/time/posix-cpu-timers.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
kernel/time/posix-cpu-timers.c +4 -3
@@ -412,9 +412,10 @@ static void cleanup_timers_list(struct l }/* -* Clean out CPU timers still ticking when a thread exited. The task -* pointer is cleared, and the expiry time is replaced with the residual -* time for later timer_gettime calls to return. +* Clean out CPU timers which are still armed when a thread exits. The +* timers are only removed from the list. No other updates are done. The +* corresponding posix timers are still accessible, but cannot be rearmed. +* * This must be called with the siglock held. */ static void cleanup_timers (struct list_head *head)
Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
Quoted 14 lines · Thomas Gleixner
The comment above
cleanup_timers()is outdated. The timers are only removed from the task/process list heads but not modified in any other way.Signed-off-by Thomas Gleixner <[email protected]>--- kernel/time/posix-cpu-timers.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)kernel/time/posix-cpu-timers.c +0
-0@@ -412,9 +412,10 @@ static void cleanup_timers_list(struct l }/*
kernel/time/posix-cpu-timers.c static void cleanup_timers_list(struct l · line 421
Quoted 10 lines
kernel/time/posix-cpu-timers.c +4
-3-* Clean out CPU timers still ticking when a thread exited. The task -* pointer is cleared, and the expiry time is replaced with the residual -* time for later timer_gettime calls to return. +* Clean out CPU timers which are still armed when a thread exits. The +* timers are only removed from the list. No other updates are done. The +* corresponding posix timers are still accessible, but cannot be rearmed. +* * This must be called with the siglock held. */ static void cleanup_timers (struct list_head *head)
Indeed and I believe we could avoid that step. We remove the sighand at the same time so those can't be accessed anymore anyway.
exit_itimers() takes care of the last call release and could force remove from the list (although it might be taken care of in your series, haven't checked yet):
kernel/time/posix-cpu-timers.c +1 -5
@@ -383,11 +383,7 @@ static int posix_cpu_timer_del(struct k_itimer *timer)*/ sighand =lock_task_sighand (p, &flags);if (unlikely (sighand ==NULL )) { -/* -* We raced with the reaping of the task. -* The deletion should have cleared us off the list. -*/ -WARN_ON_ONCE (!list_empty (&timer->it.cpu.entry)); +list_del (&timer->it.cpu.entry); }else {if (timer->it.cpu.firing) ret =TIMER_RETRY ;
Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
Quoted 13 lines · Frederic Weisbecker
Quoted 11 lines · Thomas Gleixner
/*
- * Clean out CPU timers still ticking when a thread exited. The task
- * pointer is cleared, and the expiry time is replaced with the residual
- * time for latertimer_gettimecalls to return.
+ * Clean out CPU timers which are still armed when a thread exits. The
+ * timers are only removed from the list. No other updates are done. The
+ * corresponding posix timers are still accessible, but cannot be rearmed.
+ *
* This must be called with the siglock held.
*/
static voidcleanup_timers(struct list_head *head)
Quoted 5 lines
Indeed and I believe we could avoid that step. We remove the sighand at the same time so those can't be accessed anymore anyway.
exit_itimers()takes care of the last call release and could force remove from the list (although it might be taken care of in your series, haven't checked yet):
No. The posix timer is not necessarily owned by the exiting task or process. It can be owned by a different entity which has permissions, e.g. parent.
So those are not in the posix timer list of the exiting task, which gets cleaned up in exit_itimers(). Those are in the list of the task which armed the timer. The timer is merily queued in the 'active timers' list of the exiting task and posix_cpu_timers_exit()/posix_cpu_timers_exit_group() remove it before the task/signal structs go away.
Thanks,
tglx
Frederic Weisbecker
maintainerRe: [patch 04/44] posix-cpu-timers: Fixup stale comment
Quoted 24 lines · Thomas Gleixner
Quoted 13 lines · Frederic Weisbecker
Quoted 11 lines · Thomas Gleixner
/*
- * Clean out CPU timers still ticking when a thread exited. The task
- * pointer is cleared, and the expiry time is replaced with the residual
- * time for latertimer_gettimecalls to return.
+ * Clean out CPU timers which are still armed when a thread exits. The
+ * timers are only removed from the list. No other updates are done. The
+ * corresponding posix timers are still accessible, but cannot be rearmed.
+ *
* This must be called with the siglock held.
*/
static voidcleanup_timers(struct list_head *head)Quoted 5 lines
Indeed and I believe we could avoid that step. We remove the sighand at the same time so those can't be accessed anymore anyway.
exit_itimers()takes care of the last call release and could force remove from the list (although it might be taken care of in your series, haven't checked yet):No. The posix timer is not necessarily owned by the exiting task or process. It can be owned by a different entity which has permissions, e.g. parent.
Quoted 5 lines
So those are not in the posix timer list of the exiting task, which gets cleaned up in
exit_itimers(). Those are in the list of the task which armed the timer. The timer is merily queued in the 'active timers' list of the exiting task and posix_cpu_timers_exit()/posix_cpu_timers_exit_group() remove it before the task/signal structs go away.
Sure, I understand there's two distinct things here: the owner that queues timers in owner->sig->posix_timers (cleaned in exit_itimers()) and the target that queues in target->[signal->]cputime_expires (cleaned in posix_cpu_timers_exit[_group]().
So I'm wondering why we bother with posix_cpu_timers_exit[_group]() at all when exit_itimers() could handle the list deletion from target->[signal]->cputime_expires throughout posix_cpu_timer_del() as it already does on targets that still have their sighands.
It would make things more simple to delete the timer off the target from the same caller and place and we could remove posix_cpu_timers_exit*().
Or is there something I'm awkwardly missing as usual? :-)
Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
Quoted 31 lines · Frederic Weisbecker
Quoted 24 lines · Thomas Gleixner
Quoted 13 lines · Frederic Weisbecker
Quoted 11 lines · Thomas Gleixner
/*
- * Clean out CPU timers still ticking when a thread exited. The task
- * pointer is cleared, and the expiry time is replaced with the residual
- * time for latertimer_gettimecalls to return.
+ * Clean out CPU timers which are still armed when a thread exits. The
+ * timers are only removed from the list. No other updates are done. The
+ * corresponding posix timers are still accessible, but cannot be rearmed.
+ *
* This must be called with the siglock held.
*/
static voidcleanup_timers(struct list_head *head)Quoted 5 lines
Indeed and I believe we could avoid that step. We remove the sighand at the same time so those can't be accessed anymore anyway.
exit_itimers()takes care of the last call release and could force remove from the list (although it might be taken care of in your series, haven't checked yet):No. The posix timer is not necessarily owned by the exiting task or process. It can be owned by a different entity which has permissions, e.g. parent.
Quoted 5 lines
So those are not in the posix timer list of the exiting task, which gets cleaned up in
exit_itimers(). Those are in the list of the task which armed the timer. The timer is merily queued in the 'active timers' list of the exiting task and posix_cpu_timers_exit()/posix_cpu_timers_exit_group() remove it before the task/signal structs go away.
Quoted 8 lines
Sure, I understand there's two distinct things here: the owner that queues timers in
owner->sig->posix_timers(cleaned inexit_itimers()) and the target that queues in target->[signal->]cputime_expires (cleaned in posix_cpu_timers_exit[_group]().So I'm wondering why we bother with posix_cpu_timers_exit[_group]() at all when
exit_itimers()could handle the list deletion from target->[signal]->cputime_expires throughoutposix_cpu_timer_del()as it already does on targets that still have their sighands.
No it can't do that throughout posix_cpu_timer_del() because exit_itimers() can only look at current->signal->posix_timers which does not contain the posix timers owned by a different task/process.
We could of course invoke posix_cpu_timers_exit() from exit_itimers() but does that buy anything?
Quoted 2 lines
It would make things more simple to delete the timer off the target from the same caller and place and we could remove posix_cpu_timers_exit*().
We can't. The foreign owned cpu timers are not in cur->signal->posix_timers so how should we invoke posix_cpu_timer_del() on them. Only the owner task can. The only thing the exiting task can do is to remove the foreign timer from it's expiry list which has nothing to do with cur->signal->posix_timers.
cur->signal->posix_timers only contains posix timers which are owned by current not those which are owned by a different task and armed on the exiting one.
exit_itimers() handles cur->signal->posix_timers, i.e. timers owned by current.
posix_cpu_timers_exit() handles timers enqueued on current, which are foreign owned timers because exit_itimers() removed those which were owned by current already.
posix_cpu_timers_exit_group() handles timers enqueued on current->signal, which are foreign owned timers because exit_itimers() removed those which were owned by current already.
Quoted 1 line
Or is there something I'm awkwardly missing as usual? :-)
I think so :)
Thanks,
tglx
Frederic Weisbecker
maintainerRe: [patch 04/44] posix-cpu-timers: Fixup stale comment
Quoted 8 lines · Thomas Gleixner
On Tue, 20 Aug 2019, Frederic Weisbecker wrote:
No it can't do that throughoutposix_cpu_timer_del()becauseexit_itimers()can only look atcurrent->signal->posix_timerswhich does not contain the posix timers owned by a different task/process.We could of course invoke
posix_cpu_timers_exit()fromexit_itimers()but does that buy anything?
Quoted 7 lines
Quoted 2 lines
It would make things more simple to delete the timer off the target from the same caller and place and we could remove posix_cpu_timers_exit*().
We can't. The foreign owned cpu timers are not in
cur->signal->posix_timersso how should we invokeposix_cpu_timer_del()on them. Only the owner task can. The only thing the exiting task can do is to remove the foreign timer from it's expiry list which has nothing to do withcur->signal->posix_timers.
That's exactly what I'm proposing. I think you're misunderstanding me.
I want the owner to handle all the list deletion work from the target.
Ok let's imagine a timer $ITIMER, owned by task $OWNER and whose target is task $TARGET.
So it's enqueued on $OWNER->signal->posix_timers and $TARGET->cputime_expires.
Two scenarios can happen:
1) $TARGET exits first and is released. So it calls posix_cpu_timers_exit() which deletes $ITIMER from $TARGET->cputime_expires.
Later on, $OWNER exits and calls exit_itimers() -> timer_delete_hook($ITIMER) -> posix_cpu_timer_del($ITIMER). It finds $TARGET as the target of $ITIMER but no more sighand. So it returns.
2) $OWNER exits first and calls exit_itimer() -> timer_delete_hook($ITIMER) -> posix_cpu_timer_del($ITIMER). It finds $TARGET as the target of $ITIMER and it finds a sighand to lock. So it deletes $ITIMER from $TARGET->cputime_expires (see list_del(&timer->it.cpu.entry)).
So I propose to change the behaviour of case 1) so that $TARGET doesn't call posix_cpu_timers_exit(). We instead wait for $OWNER to exit and call exit_itimers() -> timer_delete_hook($ITIMER) -> posix_cpu_timer_del($ITIMER). It is going to find $TARGET as the target of $ITIMER but no more sighand. Then finally it removes $ITIMER from $TARGET->cputime_expires.
We basically do the same thing as in 2) but without locking sighand since it's NULL on $TARGET at this time.
I hope I'm less confusing (if not confused).
Thanks.
Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
Quoted 40 lines · Frederic Weisbecker
Quoted 8 lines · Thomas Gleixner
On Tue, 20 Aug 2019, Frederic Weisbecker wrote:
No it can't do that throughoutposix_cpu_timer_del()becauseexit_itimers()can only look atcurrent->signal->posix_timerswhich does not contain the posix timers owned by a different task/process.We could of course invoke
posix_cpu_timers_exit()fromexit_itimers()but does that buy anything?Quoted 7 lines
Quoted 2 lines
It would make things more simple to delete the timer off the target from the same caller and place and we could remove posix_cpu_timers_exit*().
We can't. The foreign owned cpu timers are not in
cur->signal->posix_timersso how should we invokeposix_cpu_timer_del()on them. Only the owner task can. The only thing the exiting task can do is to remove the foreign timer from it's expiry list which has nothing to do withcur->signal->posix_timers.That's exactly what I'm proposing. I think you're misunderstanding me.
I want the owner to handle all the list deletion work from the target.
Ok let's imagine a timer $ITIMER, owned by task $OWNER and whose target is task $TARGET.
So it's enqueued on $OWNER->signal->posix_timers and $TARGET->cputime_expires.
Two scenarios can happen:
1) $TARGET exits first and is released. So it calls
posix_cpu_timers_exit()which deletes $ITIMER from $TARGET->cputime_expires.Later on, $OWNER exits and calls
exit_itimers()->timer_delete_hook($ITIMER)->posix_cpu_timer_del($ITIMER). It finds $TARGET as the target of $ITIMER but no more sighand. So it returns.2) $OWNER exits first and calls
exit_itimer()->timer_delete_hook($ITIMER)->posix_cpu_timer_del($ITIMER). It finds $TARGET as the target of $ITIMER and it finds a sighand to lock. So it deletes $ITIMER from $TARGET->cputime_expires (seelist_del(&timer->it.cpu.entry)).
Quoted 7 lines
So I propose to change the behaviour of case 1) so that $TARGET doesn't call
posix_cpu_timers_exit(). We instead wait for $OWNER to exit and callexit_itimers()->timer_delete_hook($ITIMER)->posix_cpu_timer_del($ITIMER). It is going to find $TARGET as the target of $ITIMER but no more sighand. Then finally it removes $ITIMER from $TARGET->cputime_expires.
We basically do the same thing as in 2) but without locking sighand since it's NULL on $TARGET at this time.
But what do we win with that? Horrors like this:
task A task B task C
arm_timer(A) arm_timer(A)
do_exit()
del_timer(A) del_timer(A) no sighand no_sighand list_del() list_del()
Guess how well concurrent list deletion works.
We must remove armed timers from the task/signal _before_ dropping sighand, really.
Thanks,
tglx
Frederic Weisbecker
maintainerRe: [patch 04/44] posix-cpu-timers: Fixup stale comment
Quoted 17 lines · Thomas Gleixner
Quoted 7 lines · Frederic Weisbecker
So I propose to change the behaviour of case 1) so that $TARGET doesn't call
posix_cpu_timers_exit(). We instead wait for $OWNER to exit and callexit_itimers()->timer_delete_hook($ITIMER)->posix_cpu_timer_del($ITIMER). It is going to find $TARGET as the target of $ITIMER but no more sighand. Then finally it removes $ITIMER from $TARGET->cputime_expires.
We basically do the same thing as in 2) but without locking sighand since it's NULL on $TARGET at this time.But what do we win with that? Horrors like this:
task A task B task C
arm_timer(A) arm_timer(A)
do_exit()
Quoted 8 lines
del_timer(A) del_timer(A) no sighand no_sighand list_del() list_del()Guess how well concurrent list deletion works.
We must remove armed timers from the task/signal _before_ dropping sighand, really.
Ah right, there can be concurrent owners, nevermind.
[tip: timers/core] posix-cpu-timers: Fixup stale comment
In reply to Thomas GleixnerThe following commit has been merged into the timers/core branch of tip:
The following commit has been merged into the timers/core branch of tip:
Commit-ID: 7cb9a94c158b956f46cf093ed966d0c1e996dddb Gitweb: https://git.kernel.org/tip/7cb9a94c158b956f46cf093ed966d0c1e996dddb Author: Thomas Gleixner <[email protected]> AuthorDate: Mon, 19 Aug 2019 16:31:45 +02:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Tue, 20 Aug 2019 22:09:53 +02:00
posix-cpu-timers: Fixup stale comment
The comment above cleanup_timers() is outdated. The timers are only removed from the task/process list heads but not modified in any other way.
--- kernel/time/posix-cpu-timers.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
kernel/time/posix-cpu-timers.c +4 -3
@@ -412,9 +412,10 @@ static void cleanup_timers_list(struct list_head *head) }/* -* Clean out CPU timers still ticking when a thread exited. The task -* pointer is cleared, and the expiry time is replaced with the residual -* time for later timer_gettime calls to return. +* Clean out CPU timers which are still armed when a thread exits. The +* timers are only removed from the list. No other updates are done. The +* corresponding posix timers are still accessible, but cannot be rearmed. +* * This must be called with the siglock held. */ static void cleanup_timers (struct list_head *head)
PATCH 5/44 posix-cpu-timers: Sanitize bogus WARNONS
In reply to Thomas GleixnerWarning when p == NULL and then proceeding and dereferencing p does not make any sense as the kernel will crash with a NULL pointer dereference right away.
Bailing out when p == NULL and returning an error code does not cure the underlying problem which caused p to be NULL. Though it might allow to do proper debugging.
Same applies to the clock id check in set_process_cpu_timer().
Clean them up and make them return without trying to do further damage.
--- kernel/time/posix-cpu-timers.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
kernel/time/posix-cpu-timers.c +13 -7
@@ -375,7 +375,8 @@ static int posix_cpu_timer_del(struct k_struct sighand_struct *sighand;struct task_struct *p = timer->it.cpu.task; -WARN_ON_ONCE (p ==NULL ); +if (WARN_ON_ONCE (!p)) +return -EINVAL ;/* * Protect against sighand release/switch in exit/exec and process/
@@ -581,7 +582,8 @@ static int posix_cpu_timer_set(struct k_u64 old_expires, new_expires, old_incr, val;int ret; -WARN_ON_ONCE (p ==NULL ); +if (WARN_ON_ONCE (!p)) +return -EINVAL ;/* * Use the to_ktime conversion because that clamps the maximum
@@ -716,10 +718,11 @@ static int posix_cpu_timer_set(struct k_static void posix_cpu_timer_get (struct k_itimer *timer,struct itimerspec64 *itp) { -u64 now;struct task_struct *p = timer->it.cpu.task; +u64 now; -WARN_ON_ONCE (p ==NULL ); +if (WARN_ON_ONCE (!p)) +return ;/* * Easy part: convert the reload time.
@@ -1001,12 +1004,13 @@ static void check_process_timers(struct*/ static void posix_cpu_timer_rearm (struct k_itimer *timer) { +struct task_struct *p = timer->it.cpu.task;struct sighand_struct *sighand;unsigned long flags; -struct task_struct *p = timer->it.cpu.task;u64 now; -WARN_ON_ONCE (p ==NULL ); +if (WARN_ON_ONCE (!p)) +return ;/* * Fetch the current sample and update the timer's expiry time.
@@ -1203,7 +1207,9 @@ void set_process_cpu_timer(struct task_su64 now;int ret; -WARN_ON_ONCE (clock_idx ==CPUCLOCK_SCHED ); +if (WARN_ON_ONCE (clock_idx >=CPUCLOCK_SCHED )) +return ; + ret =cpu_timer_sample_group (clock_idx, tsk, &now);if (oldval && ret != -EINVAL ) {
Re: [patch 05/44] posix-cpu-timers: Sanitize bogus WARNONS
Quoted 8 lines · Thomas Gleixner
Warning when p == NULL and then proceeding and dereferencing p does not make any sense as the kernel will crash with a NULL pointer dereference right away.
Bailing out when p == NULL and returning an error code does not cure the underlying problem which caused p to be NULL. Though it might allow to do proper debugging.
Quoted 5 lines
Same applies to the clock id check in
set_process_cpu_timer().Clean them up and make them return without trying to do further damage.
Signed-off-by Thomas Gleixner <[email protected]>
[tip: timers/core] posix-cpu-timers: Sanitize bogus WARNONS
In reply to Thomas GleixnerThe following commit has been merged into the timers/core branch of tip:
The following commit has been merged into the timers/core branch of tip:
Commit-ID: 692117c1f7a6770ed41dd8f277cd9fed1dfb16f1 Gitweb: https://git.kernel.org/tip/692117c1f7a6770ed41dd8f277cd9fed1dfb16f1 Author: Thomas Gleixner <[email protected]> AuthorDate: Mon, 19 Aug 2019 16:31:46 +02:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Wed, 21 Aug 2019 20:27:15 +02:00
posix-cpu-timers: Sanitize bogus WARNONS
Warning when p == NULL and then proceeding and dereferencing p does not make any sense as the kernel will crash with a NULL pointer dereference right away.
Bailing out when p == NULL and returning an error code does not cure the underlying problem which caused p to be NULL. Though it might allow to do proper debugging.
Same applies to the clock id check in set_process_cpu_timer().
Clean them up and make them return without trying to do further damage.
--- kernel/time/posix-cpu-timers.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
kernel/time/posix-cpu-timers.c +13 -7
@@ -375,7 +375,8 @@ static int posix_cpu_timer_del(struct k_itimer *timer)struct sighand_struct *sighand;struct task_struct *p = timer->it.cpu.task; -WARN_ON_ONCE (p ==NULL ); +if (WARN_ON_ONCE (!p)) +return -EINVAL ;/* * Protect against sighand release/switch in exit/exec and process/
@@ -581,7 +582,8 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags,u64 old_expires, new_expires, old_incr, val;int ret; -WARN_ON_ONCE (p ==NULL ); +if (WARN_ON_ONCE (!p)) +return -EINVAL ;/* * Use the to_ktime conversion because that clamps the maximum
@@ -716,10 +718,11 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags,static void posix_cpu_timer_get (struct k_itimer *timer,struct itimerspec64 *itp) { -u64 now;struct task_struct *p = timer->it.cpu.task; +u64 now; -WARN_ON_ONCE (p ==NULL ); +if (WARN_ON_ONCE (!p)) +return ;/* * Easy part: convert the reload time.
@@ -1001,12 +1004,13 @@ static void check_process_timers(struct task_struct *tsk,*/ static void posix_cpu_timer_rearm (struct k_itimer *timer) { +struct task_struct *p = timer->it.cpu.task;struct sighand_struct *sighand;unsigned long flags; -struct task_struct *p = timer->it.cpu.task;u64 now; -WARN_ON_ONCE (p ==NULL ); +if (WARN_ON_ONCE (!p)) +return ;/* * Fetch the current sample and update the timer's expiry time.
@@ -1203,7 +1207,9 @@ void set_process_cpu_timer(struct task_struct *tsk, unsigned int clock_idx,u64 now;int ret; -WARN_ON_ONCE (clock_idx ==CPUCLOCK_SCHED ); +if (WARN_ON_ONCE (clock_idx >=CPUCLOCK_SCHED )) +return ; + ret =cpu_timer_sample_group (clock_idx, tsk, &now);if (oldval && ret != -EINVAL ) {
PATCH 6/44 posix-cpu-timers: Remove tsk argument from run_posix_cpu_timers()
In reply to Thomas GleixnerIt's always current. Don't give people wrong ideas.
--- include/linux/posix-timers.h | 2 +- kernel/time/posix-cpu-timers.c | 5 +++-- kernel/time/timer.c | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-)
include/linux/posix-timers.h +1 -1
@@ -118,7 +118,7 @@ struct k_itimer {struct rcu_head rcu; }; -void run_posix_cpu_timers (struct task_struct *task); +void run_posix_cpu_timers (void );void posix_cpu_timers_exit (struct task_struct *task);void posix_cpu_timers_exit_group (struct task_struct *task); void set_process_cpu_timer(struct task_struct *task, unsigned int clock_idx,
kernel/time/posix-cpu-timers.c +3 -2
@@ -1137,11 +1137,12 @@ static inline int fastpath_timer_check(s* already updated our counts. We need to check if any timers fire now. * Interrupts are disabled. */ -void run_posix_cpu_timers (struct task_struct *tsk) +void run_posix_cpu_timers (void ) { -LIST_HEAD (firing); +struct task_struct *tsk = current;struct k_itimer *timer, *next;unsigned long flags; +LIST_HEAD (firing);lockdep_assert_irqs_disabled ();
kernel/time/timer.c +1 -1
@@ -1728,7 +1728,7 @@ void update_process_times(int user_tick)#endif scheduler_tick ();if (IS_ENABLED (CONFIG_POSIX_TIMERS )) -run_posix_cpu_timers (p); +run_posix_cpu_timers (); }/**
Re: [patch 06/44] posix-cpu-timers: Remove tsk argument from run_posix_cpu_timers()
Quoted 3 lines · Thomas Gleixner
It's always current. Don't give people wrong ideas.
Signed-off-by Thomas Gleixner <[email protected]>
[tip: timers/core] posix-cpu-timers: Remove tsk argument from run_posix_cpu_timers()
In reply to Thomas GleixnerThe following commit has been merged into the timers/core branch of tip:
The following commit has been merged into the timers/core branch of tip:
Commit-ID: dce3e8fd039cc1b62760b3ad6822cf04c262cd0e Gitweb: https://git.kernel.org/tip/dce3e8fd039cc1b62760b3ad6822cf04c262cd0e Author: Thomas Gleixner <[email protected]> AuthorDate: Mon, 19 Aug 2019 16:31:47 +02:00 Committer: Thomas Gleixner <[email protected]> CommitterDate: Wed, 21 Aug 2019 20:27:16 +02:00
posix-cpu-timers: Remove tsk argument from run_posix_cpu_timers()
It's always current. Don't give people wrong ideas.
--- include/linux/posix-timers.h | 2 +- kernel/time/posix-cpu-timers.c | 5 +++-- kernel/time/timer.c | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-)
include/linux/posix-timers.h +1 -1
@@ -118,7 +118,7 @@ struct k_itimer {struct rcu_head rcu; }; -void run_posix_cpu_timers (struct task_struct *task); +void run_posix_cpu_timers (void );void posix_cpu_timers_exit (struct task_struct *task);void posix_cpu_timers_exit_group (struct task_struct *task); void set_process_cpu_timer(struct task_struct *task, unsigned int clock_idx,
kernel/time/posix-cpu-timers.c +3 -2
@@ -1137,11 +1137,12 @@ static inline int fastpath_timer_check(struct task_struct *tsk)* already updated our counts. We need to check if any timers fire now. * Interrupts are disabled. */ -void run_posix_cpu_timers (struct task_struct *tsk) +void run_posix_cpu_timers (void ) { -LIST_HEAD (firing); +struct task_struct *tsk = current;struct k_itimer *timer, *next;unsigned long flags; +LIST_HEAD (firing);lockdep_assert_irqs_disabled ();
kernel/time/timer.c +1 -1
@@ -1728,7 +1728,7 @@ void update_process_times(int user_tick)#endif scheduler_tick ();if (IS_ENABLED (CONFIG_POSIX_TIMERS )) -run_posix_cpu_timers (p); +run_posix_cpu_timers (); }/**
PATCH 7/44 posix-cpu-timers: Simplify sighand locking in run_posix_cpu_timers()
In reply to Thomas Gleixnerrun_posix_cpu_timers() is called from the timer interrupt. The posix timer expiry always affects the current task which got interrupted.
sighand locking is only racy when done on a foreign task, which must use lock_task_sighand(). But in case of run_posix_cpu_timers() that's pointless.
sighand of a task can only be dropped or changed by the task itself. Drop happens in do_exit(), changing sighand happens in execve().
So the timer interrupt can see one of the following states of the current task which it interrupted:
- executing: sighand is set
- exiting: sighand is either set or NULL
- execing: sighand is either the original one or the new one
The 'check -> lock -> check again' logic in lock_task_sighand() is not required here at all. The state cannot change as the interrupted task context obviously is not executing instructions.
So the only interesting case here is to check whether current->sighand is NULL or not.
Add the check for sighand == NULL right at the beginning and replace lock_task_sighand() with a regular spin_lock() invocation along with proper comments why this is safe.
--- kernel/time/posix-cpu-timers.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
kernel/time/posix-cpu-timers.c +20 -6
@@ -1141,20 +1141,34 @@ void run_posix_cpu_timers(void) {struct task_struct *tsk = current;struct k_itimer *timer, *next; -unsigned long flags;LIST_HEAD (firing);lockdep_assert_irqs_disabled ();/* -* The fast path checks that there are no expired thread or thread -* group timers. If that's so, just return. +* Verify that sighand exits. If not, the task is exiting and has +* dropped sighand already. Without sighand, timer expiry is +* pointless. */ -if (!fastpath_timer_check (tsk)) +if (!tsk->sighand)return ; -if (!lock_task_sighand (tsk, &flags)) +/* +* Now do a fast check for expired task and group timers. If +* no expired timers found, return. +*/ +if (!fastpath_timer_check (tsk))return ; + +/* +* The timer interrupt hit current. It's verified that sighand +* exists, so it cannot go away before the timer interrupt returns +* as only current can remove or change its own sighand in exit() +* or exec() with sighand lock held and interrupts disabled. Ergo +* the interrupt can only observe a valid sighand or NULL. +*/ +spin_lock (&tsk->sighand->siglock); +/* * Here we take off tsk->signal->cpu_timers[N] and * tsk->cpu_timers[N] all the timers that are firing, and
@@ -1172,7 +1186,7 @@ void run_posix_cpu_timers(void)* that gets the timer lock before we do will give it up and * spin until we've taken care of that timer below. */ -unlock_task_sighand (tsk, &flags); +spin_unlock (&tsk->sighand->siglock);/* * Now that all the timers on our list have the firing flag,
Frederic Weisbecker
maintainerRe: [patch 07/44] posix-cpu-timers: Simplify sighand locking in run_posix_cpu_timers()
Quoted 9 lines · Thomas Gleixner
run_posix_cpu_timers()is called from the timer interrupt. The posix timer expiry always affects the current task which got interrupted.sighand locking is only racy when done on a foreign task, which must use
lock_task_sighand(). But in case ofrun_posix_cpu_timers()that's pointless.sighand of a task can only be dropped or changed by the task itself. Drop happens in
do_exit()
Well, that's only in case of autoreap. Otherwise this is dropped by the reaper.
Quoted 1 line
changing sighand happens in
execve().
Re: [patch 07/44] posix-cpu-timers: Simplify sighand locking in run_posix_cpu_timers()
Quoted 4 lines · Frederic Weisbecker
Quoted 3 lines · Thomas Gleixner
run_posix_cpu_timers()is called from the timer interrupt. The posix timer expiry always affects the current task which got interrupted.
Quoted 8 lines
Quoted 6 lines
sighand locking is only racy when done on a foreign task, which must use
lock_task_sighand(). But in case ofrun_posix_cpu_timers()that's pointless.sighand of a task can only be dropped or changed by the task itself. Drop happens in
do_exit()Well, that's only in case of autoreap. Otherwise this is dropped by the reaper.
Right, but in the reaper case the task cannot be on the CPU running and being interrupted by the tick. I might be missing something subtle though.
Thanks,
tglx
Frederic Weisbecker
maintainerRe: [patch 07/44] posix-cpu-timers: Simplify sighand locking in run_posix_cpu_timers()
Quoted 10 lines · Thomas Gleixner
Quoted 8 lines · Frederic Weisbecker
Quoted 7 lines · Thomas Gleixner
run_posix_cpu_timers()is called from the timer interrupt. The posix timer expiry always affects the current task which got interrupted.sighand locking is only racy when done on a foreign task, which must use
lock_task_sighand(). But in case ofrun_posix_cpu_timers()that's pointless.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
sighand of a task can only be dropped or changed by the task itself. Drop happens in
do_exit()Well, that's only in case of autoreap. Otherwise this is dropped by the reaper.
Right, but in the reaper case the task cannot be on the CPU running and being interrupted by the tick. I might be missing something subtle though.
That looks possible. After exit_notify() and until the final schedule(), the exiting task can execute concurrently with the reaper calling release_task().
Re: [patch 07/44] posix-cpu-timers: Simplify sighand locking in run_posix_cpu_timers()
Quoted 14 lines · Frederic Weisbecker
Quoted 13 lines · Thomas Gleixner
Quoted 11 lines · Frederic Weisbecker
Quoted 9 lines · Thomas Gleixner
run_posix_cpu_timers()is called from the timer interrupt. The posix timer expiry always affects the current task which got interrupted.sighand locking is only racy when done on a foreign task, which must use
lock_task_sighand(). But in case ofrun_posix_cpu_timers()that's pointless.sighand of a task can only be dropped or changed by the task itself. Drop happens in
do_exit()
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Well, that's only in case of autoreap. Otherwise this is dropped by the reaper.
Right, but in the reaper case the task cannot be on the CPU running and being interrupted by the tick. I might be missing something subtle though.
That looks possible. After
exit_notify()and until the finalschedule(), the exiting task can execute concurrently with the reaper callingrelease_task().
Duh, indeed.
We only can rely on that when we are in task context and not post state = ZOMBIE.
So for now this needs to stay unfortunately, but once we moved the expiry to task work this becomes interesting.
/me goes fix.
PATCH 8/44 posix-cpu-timers: Provide task validation functions
In reply to Thomas GleixnerThe code contains three slightly different copies of validating whether a given clock resolves to a valid task and whether the current caller has permissions to access it.
Create central functions. Replace check_clock() as a first step and rename it to something sensible.
--- kernel/time/posix-cpu-timers.c | 65 +++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 21 deletions(-)
kernel/time/posix-cpu-timers.c +44 -21
@@ -35,27 +35,52 @@ void update_rlimit_cpu(struct task_strucspin_unlock_irq (&task->sighand->siglock); } -static int check_clock (const clockid_t which_clock) +/* +* Functions for validating access to tasks. +*/ +static struct task_struct *lookup_task (const pid_t pid,bool thread) { -int error =0 ;struct task_struct *p; -const pid_t pid =CPUCLOCK_PID (which_clock); -if (CPUCLOCK_WHICH (which_clock) >=CPUCLOCK_MAX ) -return -EINVAL ; +if (!pid) +return thread ? current : current->group_leader; -if (pid ==0 ) -return 0 ; + p =find_task_by_vpid (pid); +if (!p || p == current) +return p; +if (thread) +return same_thread_group (p, current) ? p :NULL ; +if (p == current) +return p; +return has_group_leader_pid (p) ? p :NULL ; +} + +static struct task_struct *__get_task_for_clock (const clockid_t clock, +bool getref) +{ +const bool thread = !!CPUCLOCK_PERTHREAD (clock); +const pid_t pid =CPUCLOCK_PID (clock); +struct task_struct *p; + +if (CPUCLOCK_WHICH (clock) >=CPUCLOCK_MAX ) +return NULL ;rcu_read_lock (); - p =find_task_by_vpid (pid); -if (!p || !(CPUCLOCK_PERTHREAD (which_clock) ? -same_thread_group (p, current) :has_group_leader_pid (p))) { - error = -EINVAL ; - } + p =lookup_task (pid, thread); +if (p && getref) +get_task_struct (p);rcu_read_unlock (); +return p; +} -return error; +static inline struct task_struct *get_task_for_clock (const clockid_t clock) +{ +return __get_task_for_clock (clock, true); +} + +static inline int validate_clock_permissions (const clockid_t clock) +{ +return __get_task_for_clock (clock, false) ?0 : -EINVAL ; }/*
@@ -125,7 +150,8 @@ static inline u64 virt_ticks(struct taskstatic int posix_cpu_clock_getres (const clockid_t which_clock,struct timespec64 *tp) { -int error =check_clock (which_clock); +int error =validate_clock_permissions (which_clock); +if (!error) { tp->tv_sec =0 ; tp->tv_nsec = ((NSEC_PER_SEC +HZ -1 ) /HZ );
@@ -142,20 +168,17 @@ posix_cpu_clock_getres(const clockid_t w }static int -posix_cpu_clock_set (const clockid_t which_clock,const struct timespec64 *tp) +posix_cpu_clock_set (const clockid_t clock,const struct timespec64 *tp) { +int error =validate_clock_permissions (clock); +/* * You can never reset a CPU clock, but we check for other errors * in the call before failing with EPERM. */ -int error =check_clock (which_clock); -if (error ==0 ) { - error = -EPERM ; - } -return error; +return error ? : -EPERM ; } -/* * Sample a per-thread clock for the given task. */
PATCH 9/44 posix-cpu-timers: Use common permission check in posix_cpu_clock_get()
In reply to Thomas GleixnerReplace the next slightly different copy of permission checks. That also removes the necessarity to check the return value of the sample functions because the clock id is already validated.
--- kernel/time/posix-cpu-timers.c | 61 ++++++++++------------------------------- 1 file changed, 16 insertions(+), 45 deletions(-)
kernel/time/posix-cpu-timers.c +16 -45
@@ -289,53 +289,24 @@ static int cpu_clock_sample_group(constreturn 0 ; } -static int posix_cpu_clock_get_task (struct task_struct *tsk, -const clockid_t which_clock, -struct timespec64 *tp) +static int posix_cpu_clock_get (const clockid_t clock,struct timespec64 *tp) { -int err = -EINVAL ; -u64 rtn; +const clockid_t clkid =CPUCLOCK_WHICH (clock); +struct task_struct *tsk; +u64 t; + + tsk =get_task_for_clock (clock); +if (!tsk) +return -EINVAL ; + +if (CPUCLOCK_PERTHREAD (clock)) +cpu_clock_sample (clkid, tsk, &t); +else +cpu_clock_sample_group (clkid, tsk, &t); +put_task_struct (tsk); -if (CPUCLOCK_PERTHREAD (which_clock)) { -if (same_thread_group (tsk, current)) - err =cpu_clock_sample (which_clock, tsk, &rtn); - }else { -if (tsk == current ||thread_group_leader (tsk)) - err =cpu_clock_sample_group (which_clock, tsk, &rtn); - } - -if (!err) - *tp =ns_to_timespec64 (rtn); - -return err; -} - - -static int posix_cpu_clock_get (const clockid_t which_clock,struct timespec64 *tp) -{ -const pid_t pid =CPUCLOCK_PID (which_clock); -int err = -EINVAL ; - -if (pid ==0 ) { -/* -* Special case constant value for our own clocks. -* We don't have to do any lookup to find ourselves. -*/ - err =posix_cpu_clock_get_task (current, which_clock, tp); - }else { -/* -* Find the given PID, and validate that the caller -* should be able to see it. -*/ -struct task_struct *p; -rcu_read_lock (); - p =find_task_by_vpid (pid); -if (p) - err =posix_cpu_clock_get_task (p, which_clock, tp); -rcu_read_unlock (); - } - -return err; + *tp =ns_to_timespec64 (t); +return 0 ; }/*
PATCH 10/44 posix-cpu-timers: Use common permission check in posix_cpu_timer_create()
In reply to Thomas GleixnerYet another copy of the same thing gone...
--- kernel/time/posix-cpu-timers.c | 35 +++-------------------------------- 1 file changed, 3 insertions(+), 32 deletions(-)
kernel/time/posix-cpu-timers.c +3 -32
@@ -316,44 +316,15 @@ static int posix_cpu_clock_get(const clo*/ static int posix_cpu_timer_create (struct k_itimer *new_timer) { -int ret =0 ; -const pid_t pid =CPUCLOCK_PID (new_timer->it_clock); -struct task_struct *p; +struct task_struct *p =get_task_for_clock (new_timer->it_clock); -if (CPUCLOCK_WHICH (new_timer->it_clock) >=CPUCLOCK_MAX ) +if (!p)return -EINVAL ; new_timer->kclock = &clock_posix_cpu; -INIT_LIST_HEAD (&new_timer->it.cpu.entry); - -rcu_read_lock (); -if (CPUCLOCK_PERTHREAD (new_timer->it_clock)) { -if (pid ==0 ) { - p = current; - }else { - p =find_task_by_vpid (pid); -if (p && !same_thread_group (p, current)) - p =NULL ; - } - }else { -if (pid ==0 ) { - p = current->group_leader; - }else { - p =find_task_by_vpid (pid); -if (p && !has_group_leader_pid (p)) - p =NULL ; - } - } new_timer->it.cpu.task = p; -if (p) { -get_task_struct (p); - }else { - ret = -EINVAL ; - } -rcu_read_unlock (); - -return ret; +return 0 ; }/*
PATCH 11/44 posix-cpu-timers: Provide quick sample function for itimer
In reply to Thomas Gleixnerget_itimer() needs a sample of the current thread group cputime. It invokes thread_group_cputimer() - which is a misnomer. That function also starts eventually the group cputime accouting which is bogus because the accounting is already active when a timer is armed.
--- include/linux/sched/cputime.h | 2 +- kernel/time/posix-cpu-timers.c | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-)
include/linux/sched/cputime.h +1 -1
@@ -62,7 +62,7 @@ extern void cputime_adjust(struct task_c*/ void thread_group_cputime (struct task_struct *tsk,struct task_cputime *times);void thread_group_cputimer (struct task_struct *tsk,struct task_cputime *times); - +void thread_group_sample_cputime (struct task_struct *tsk,struct task_cputime *times);/* * The following are functions that support scheduler-internal time accounting.
kernel/time/posix-cpu-timers.c +21 -0
@@ -232,6 +232,27 @@ static inline void sample_cputime_atomic times->sum_exec_runtime =atomic64_read (&atomic_times->sum_exec_runtime); } +/** +* thread_group_sample_cputime - Sample cputime for a given task +* @tsk: Task for which cputime needs to be started +* @iimes: Storage for time samples +* +* Called from sys_getitimer() to calculate the expiry time of an active +* timer. That means group cputime accounting is already active. Called +* with task sighand lock held. +* +* Updates @times with an uptodate sample of the thread group cputimes. +*/ +void thread_group_sample_cputime (struct task_struct *tsk, +struct task_cputime *times) +{ +struct thread_group_cputimer *cputimer = &tsk->signal->cputimer; + +WARN_ON_ONCE (!cputimer->running); + +sample_cputime_atomic (times, &cputimer->cputime_atomic); +} +void thread_group_cputimer (struct task_struct *tsk,struct task_cputime *times) {struct thread_group_cputimer *cputimer = &tsk->signal->cputimer;
PATCH 12/44 itimers: Use quick sample function
In reply to Thomas Gleixnerget_itimer() locks sighand lock and checks whether the timer is already expired. If it is not expired then the thread group cputime accounting is already enabled. Use the sampling function not the one which is meant for starting a timer.
--- kernel/time/itimer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/time/itimer.c +1 -1
@@ -58,7 +58,7 @@ static void get_cpu_itimer(struct task_sstruct task_cputime cputime;u64 t; -thread_group_cputimer (tsk, &cputime); +thread_group_sample_cputime (tsk, &cputime);if (clock_id ==CPUCLOCK_PROF ) t = cputime.utime + cputime.stime;else
PATCH 13/44 posix-cpu-timers: Sample directly in timer check
In reply to Thomas GleixnerThe thread group accounting is active, otherwise the expiry function would not be running. Sample the thread group time directly.
--- kernel/time/posix-cpu-timers.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
kernel/time/posix-cpu-timers.c +4 -3
@@ -914,16 +914,17 @@ static void check_process_timers(structif (!READ_ONCE (tsk->signal->cputimer.running))return ; -/* +/* * Signify that a thread is checking for process timers. * Write access to this field is protected by the sighand lock. */ sig->cputimer.checking_timer = true;/* -* Collect the current process totals. +* Collect the current process totals. Group accounting is active +* so the sample can be taken directly. */ -thread_group_cputimer (tsk, &cputime); +sample_cputime_atomic (&cputime, &sig->cputimer.cputime_atomic); utime = cputime.utime; ptime = utime + cputime.stime; sum_sched_runtime = cputime.sum_exec_runtime;
PATCH 14/44 posix-cpu-timers: Rename thread_group_cputimer() and make it static
In reply to Thomas Gleixnerthread_group_cputimer() is a complete misnomer. The function does two things:
- For arming process wide timers it makes sure that the atomic time storage is up to date. If no cpu timer is armed yet, then the atomic time storage is not updated by the scheduler for performance reasons.
In that case a full summing up of all threads needs to be done and the update needs to be enabled.
- Samples the current time into the caller supplied storage.
Rename it to thread_group_start_cputime(), make it static and fixup the callsite.
--- include/linux/sched/cputime.h | 1 - kernel/time/posix-cpu-timers.c | 17 +++++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-)
include/linux/sched/cputime.h +0 -1
@@ -61,7 +61,6 @@ extern void cputime_adjust(struct task_c* Thread group CPU time accounting. */ void thread_group_cputime (struct task_struct *tsk,struct task_cputime *times); -void thread_group_cputimer (struct task_struct *tsk,struct task_cputime *times);void thread_group_sample_cputime (struct task_struct *tsk,struct task_cputime *times);/*
kernel/time/posix-cpu-timers.c +15 -2
@@ -253,7 +253,20 @@ void thread_group_sample_cputime(structsample_cputime_atomic (times, &cputimer->cputime_atomic); } -void thread_group_cputimer (struct task_struct *tsk,struct task_cputime *times) +/** +* thread_group_start_cputime - Start cputime and return a sample +* @tsk: Task for which cputime needs to be started +* @iimes: Storage for time samples +* +* The thread group cputime accouting is avoided when there are no posix +* CPU timers armed. Before starting a timer it's required to check whether +* the time accounting is active. If not, a full update of the atomic +* accounting store needs to be done and the accounting enabled. +* +* Updates @times with an uptodate sample of the thread group cputimes. +*/ +static void +thread_group_start_cputime (struct task_struct *tsk,struct task_cputime *times) {struct thread_group_cputimer *cputimer = &tsk->signal->cputimer;struct task_cputime sum;
@@ -536,7 +549,7 @@ static int cpu_timer_sample_group(const {struct task_cputime cputime; -thread_group_cputimer (p, &cputime); +thread_group_start_cputime (p, &cputime);switch (CPUCLOCK_WHICH (which_clock)) {default :return -EINVAL ;
PATCH 15/44 posix-cpu-timer: Comsolidate thread group sample code
In reply to Thomas Gleixnercpu_clock_sample_group() and cpu_timer_sample_group() are almost the same. Before the rename one called thread_group_cputimer() and the other thread_group_cputime(). Really intuitive function names.
Consolidate the functions and also avoid the thread traversal when the thread group accounting is already active.
--- kernel/time/posix-cpu-timers.c | 59 +++++++++++++---------------------------- 1 file changed, 20 insertions(+), 39 deletions(-)
kernel/time/posix-cpu-timers.c +20 -39
@@ -294,29 +294,37 @@ thread_group_start_cputime(struct task_s }/* -* Sample a process (thread group) clock for the given group_leader task. -* Must be called with task sighand lock held for safe while_each_thread() -* traversal. +* Sample a process (thread group) clock for the given task clkid. If the +* groups cputime accounting is already enabled, read the atomic +* store. Otherwise a full update is required. task sighand lock must be +* held to protect the task traversal on a full update. */ static int cpu_clock_sample_group (const clockid_t which_clock,struct task_struct *p, -u64 *sample) +u64 *sample,bool start) { +struct thread_group_cputimer *cputimer = &p->signal->cputimer;struct task_cputime cputime; +if (!READ_ONCE (cputimer->running)) { +if (start) +thread_group_start_cputime (p, &cputime); +else +thread_group_cputime (p, &cputime); + }else { +sample_cputime_atomic (&cputime, &cputimer->cputime_atomic); + } +switch (CPUCLOCK_WHICH (which_clock)) {default :return -EINVAL ;case CPUCLOCK_PROF : -thread_group_cputime (p, &cputime); *sample = cputime.utime + cputime.stime;break ;case CPUCLOCK_VIRT : -thread_group_cputime (p, &cputime); *sample = cputime.utime;break ;case CPUCLOCK_SCHED : -thread_group_cputime (p, &cputime); *sample = cputime.sum_exec_runtime;break ; }
@@ -336,7 +344,7 @@ static int posix_cpu_clock_get(const cloif (CPUCLOCK_PERTHREAD (clock))cpu_clock_sample (clkid, tsk, &t);else -cpu_clock_sample_group (clkid, tsk, &t); +cpu_clock_sample_group (clkid, tsk, &t, false);put_task_struct (tsk); *tp =ns_to_timespec64 (t);
@@ -540,33 +548,6 @@ static void cpu_timer_fire(struct k_itim }/* -* Sample a process (thread group) timer for the given group_leader task. -* Must be called with task sighand lock held for safe while_each_thread() -* traversal. -*/ -static int cpu_timer_sample_group (const clockid_t which_clock, -struct task_struct *p,u64 *sample) -{ -struct task_cputime cputime; - -thread_group_start_cputime (p, &cputime); -switch (CPUCLOCK_WHICH (which_clock)) { -default : -return -EINVAL ; -case CPUCLOCK_PROF : - *sample = cputime.utime + cputime.stime; -break ; -case CPUCLOCK_VIRT : - *sample = cputime.utime; -break ; -case CPUCLOCK_SCHED : - *sample = cputime.sum_exec_runtime; -break ; - } -return 0 ; -} - -/* * Guts of sys_timer_settime for CPU timers. * This is called with the timer locked and interrupts disabled. * If we return TIMER_RETRY, it's necessary to release the timer's lock
@@ -627,7 +608,7 @@ static int posix_cpu_timer_set(struct k_if (CPUCLOCK_PERTHREAD (timer->it_clock)) {cpu_clock_sample (timer->it_clock, p, &val); }else { -cpu_timer_sample_group (timer->it_clock, p, &val); +cpu_clock_sample_group (timer->it_clock, p, &val, true); }if (old) {
@@ -755,7 +736,7 @@ static void posix_cpu_timer_get(struct k timer->it.cpu.expires =0 ;return ; }else { -cpu_timer_sample_group (timer->it_clock, p, &now); +cpu_clock_sample_group (timer->it_clock, p, &now, false);unlock_task_sighand (p, &flags); } }
@@ -1042,7 +1023,7 @@ static void posix_cpu_timer_rearm(struct/* If the process is dying, no need to rearm */ goto unlock; } -cpu_timer_sample_group (timer->it_clock, p, &now); +cpu_clock_sample_group (timer->it_clock, p, &now, true);bump_cpu_timer (timer, now);/* Leave the sighand locked for the call below. */ }
@@ -1225,7 +1206,7 @@ void set_process_cpu_timer(struct task_sif (WARN_ON_ONCE (clock_idx >=CPUCLOCK_SCHED ))return ; - ret =cpu_timer_sample_group (clock_idx, tsk, &now); + ret =cpu_clock_sample_group (clock_idx, tsk, &now, true);if (oldval && ret != -EINVAL ) {/*
Re: [patch 15/44] posix-cpu-timer: Comsolidate thread group sample code
in the title:
s/Comsolidate
/Consolidate
Quoted 15 lines · Thomas Gleixner
cpu_clock_sample_group()andcpu_timer_sample_group()are almost the same. Before the rename one calledthread_group_cputimer()and the otherthread_group_cputime(). Really intuitive function names.Consolidate the functions and also avoid the thread traversal when the thread group accounting is already active.
Signed-off-by Thomas Gleixner <[email protected]>--- kernel/time/posix-cpu-timers.c | 59 +++++++++++++---------------------------- 1 file changed, 20 insertions(+), 39 deletions(-)
kernel/time/posix-cpu-timers.c thread_group_start_cputime(struct task_s · line 300
Quoted 10 lines
kernel/time/posix-cpu-timers.c +4
-3@@ -294,29 +294,37 @@ thread_group_start_cputime(struct task_s }/* -* Sample a process (thread group) clock for the given group_leader task. -* Must be called with task sighand lock held for safe while_each_thread() -* traversal. +* Sample a process (thread group) clock for the given task clkid. If the +* groups cputime accounting is already enabled, read the atomic +* store. Otherwise a full update is required. task sighand lock must be +* held to protect the task traversal on a full update.
s/groups
/group's
s/task sighand
/Task sighand
Thanks,
Ingo
PATCH 16/44 posix-cpu-timers: Use clock ID in posix_cpu_timer_set()
In reply to Thomas GleixnerExtract the clock ID (PROF/VIRT/SCHED) from the clock selector and use it as argument to the sample functions. That allows to simplify them once all callers are fixed.
--- kernel/time/posix-cpu-timers.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
kernel/time/posix-cpu-timers.c +6 -5
@@ -556,10 +556,11 @@ static void cpu_timer_fire(struct k_itimstatic int posix_cpu_timer_set (struct k_itimer *timer,int timer_flags,struct itimerspec64 *new,struct itimerspec64 *old) { -unsigned long flags; -struct sighand_struct *sighand; -struct task_struct *p = timer->it.cpu.task; +clockid_t clkid =CPUCLOCK_WHICH (timer->it_clock);u64 old_expires, new_expires, old_incr, val; +struct task_struct *p = timer->it.cpu.task; +struct sighand_struct *sighand; +unsigned long flags;int ret;if (WARN_ON_ONCE (!p))
@@ -606,9 +607,9 @@ static int posix_cpu_timer_set(struct k_* check if it's already passed. In short, we need a sample. */ if (CPUCLOCK_PERTHREAD (timer->it_clock)) { -cpu_clock_sample (timer->it_clock, p, &val); +cpu_clock_sample (clkid, p, &val); }else { -cpu_clock_sample_group (timer->it_clock, p, &val, true); +cpu_clock_sample_group (clkid, p, &val, true); }if (old) {
PATCH 17/44 posix-cpu-timers: Use clock ID in posix_cpu_timer_get()
In reply to Thomas GleixnerExtract the clock ID (PROF/VIRT/SCHED) from the clock selector and use it as argument to the sample functions. That allows to simplify them once all callers are fixed.
--- kernel/time/posix-cpu-timers.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
kernel/time/posix-cpu-timers.c +3 -2
@@ -699,6 +699,7 @@ static int posix_cpu_timer_set(struct k_static void posix_cpu_timer_get (struct k_itimer *timer,struct itimerspec64 *itp) { +clockid_t clkid =CPUCLOCK_WHICH (timer->it_clock);struct task_struct *p = timer->it.cpu.task;u64 now;
@@ -717,7 +718,7 @@ static void posix_cpu_timer_get(struct k* Sample the clock to take the difference with the expiry time. */ if (CPUCLOCK_PERTHREAD (timer->it_clock)) { -cpu_clock_sample (timer->it_clock, p, &now); +cpu_clock_sample (clkid, p, &now); }else {struct sighand_struct *sighand;unsigned long flags;
@@ -737,7 +738,7 @@ static void posix_cpu_timer_get(struct k timer->it.cpu.expires =0 ;return ; }else { -cpu_clock_sample_group (timer->it_clock, p, &now, false); +cpu_clock_sample_group (clkid, p, &now, false);unlock_task_sighand (p, &flags); } }
PATCH 18/44 posix-cpu-timers: Use clock ID in posix_cpu_timer_rearm()
In reply to Thomas GleixnerExtract the clock ID (PROF/VIRT/SCHED) from the clock selector and use it as argument to the sample functions. That allows to simplify them once all callers are fixed.
--- kernel/time/posix-cpu-timers.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
kernel/time/posix-cpu-timers.c +3 -2
@@ -987,6 +987,7 @@ static void check_process_timers(struct*/ static void posix_cpu_timer_rearm (struct k_itimer *timer) { +clockid_t clkid =CPUCLOCK_WHICH (timer->it_clock);struct task_struct *p = timer->it.cpu.task;struct sighand_struct *sighand;unsigned long flags;
@@ -999,7 +1000,7 @@ static void posix_cpu_timer_rearm(struct* Fetch the current sample and update the timer's expiry time. */ if (CPUCLOCK_PERTHREAD (timer->it_clock)) { -cpu_clock_sample (timer->it_clock, p, &now); +cpu_clock_sample (clkid, p, &now);bump_cpu_timer (timer, now);if (unlikely (p->exit_state))return ;
@@ -1025,7 +1026,7 @@ static void posix_cpu_timer_rearm(struct/* If the process is dying, no need to rearm */ goto unlock; } -cpu_clock_sample_group (timer->it_clock, p, &now, true); +cpu_clock_sample_group (clkid, p, &now, true);bump_cpu_timer (timer, now);/* Leave the sighand locked for the call below. */ }
PATCH 19/44 posix-cpu-timer: Remove pointless return value check
In reply to Thomas Gleixnerset_process_cpu_timer() checks already whether the clock id is valid. No point in checking the return value of the sample function. That allows to simplify the sample function later.
--- kernel/time/posix-cpu-timers.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
kernel/time/posix-cpu-timers.c +2 -3
@@ -1204,14 +1204,13 @@ void set_process_cpu_timer(struct task_s u64 *newval, u64 *oldval) {u64 now; -int ret;if (WARN_ON_ONCE (clock_idx >=CPUCLOCK_SCHED ))return ; - ret =cpu_clock_sample_group (clock_idx, tsk, &now, true); +cpu_clock_sample_group (clock_idx, tsk, &now, true); -if (oldval && ret != -EINVAL ) { +if (oldval) {/* * We are setting itimer. The *oldval is absolute and we update * it to be relative, *newval argument is relative and we update
PATCH 20/44 posix-cpu-timers: Simplify sample functions
In reply to Thomas GleixnerAll callers hand in a valdiated clock id. Remove the return value which was unchecked in most places anyway.
--- kernel/time/posix-cpu-timers.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-)
kernel/time/posix-cpu-timers.c +13 -15
@@ -180,14 +180,12 @@ posix_cpu_clock_set(const clockid_t cloc }/* -* Sample a per-thread clock for the given task. +* Sample a per-thread clock for the given task. clkid is validated. */ -static int cpu_clock_sample (const clockid_t which_clock, -struct task_struct *p,u64 *sample) +static void cpu_clock_sample (const clockid_t clkid,struct task_struct *p, +u64 *sample) { -switch (CPUCLOCK_WHICH (which_clock)) { -default : -return -EINVAL ; +switch (clkid) {case CPUCLOCK_PROF : *sample =prof_ticks (p);break ;
@@ -197,8 +195,9 @@ static int cpu_clock_sample(const clockicase CPUCLOCK_SCHED : *sample =task_sched_runtime (p);break ; +default : +WARN_ON_ONCE (1 ); } -return 0 ; }/*
@@ -297,11 +296,11 @@ thread_group_start_cputime(struct task_s* Sample a process (thread group) clock for the given task clkid. If the * groups cputime accounting is already enabled, read the atomic * store. Otherwise a full update is required. task sighand lock must be -* held to protect the task traversal on a full update. +* held to protect the task traversal on a full update. clkid is already +* validated. */ -static int cpu_clock_sample_group (const clockid_t which_clock, -struct task_struct *p, -u64 *sample,bool start) +static void cpu_clock_sample_group (const clockid_t clkid,struct task_struct *p, +u64 *sample,bool start) {struct thread_group_cputimer *cputimer = &p->signal->cputimer;struct task_cputime cputime;
@@ -315,9 +314,7 @@ static int cpu_clock_sample_group(constsample_cputime_atomic (&cputime, &cputimer->cputime_atomic); } -switch (CPUCLOCK_WHICH (which_clock)) { -default : -return -EINVAL ; +switch (clkid) {case CPUCLOCK_PROF : *sample = cputime.utime + cputime.stime;break ;
@@ -327,8 +324,9 @@ static int cpu_clock_sample_group(constcase CPUCLOCK_SCHED : *sample = cputime.sum_exec_runtime;break ; +default : +WARN_ON_ONCE (1 ); } -return 0 ; }static int posix_cpu_clock_get (const clockid_t clock,struct timespec64 *tp)
PATCH 21/44 posix-cpu-timers: Get rid of pointer indirection
In reply to Thomas GleixnerNow that the sample functions have no return value anymore, the result can simply be returned instead of using pointer indirection.
--- kernel/time/posix-cpu-timers.c | 50 ++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 28 deletions(-)
kernel/time/posix-cpu-timers.c +22 -28
@@ -182,22 +182,19 @@ posix_cpu_clock_set(const clockid_t cloc/* * Sample a per-thread clock for the given task. clkid is validated. */ -static void cpu_clock_sample (const clockid_t clkid,struct task_struct *p, -u64 *sample) +static u64 cpu_clock_sample (const clockid_t clkid,struct task_struct *p) {switch (clkid) {case CPUCLOCK_PROF : - *sample =prof_ticks (p); -break ; +return prof_ticks (p);case CPUCLOCK_VIRT : - *sample =virt_ticks (p); -break ; +return virt_ticks (p);case CPUCLOCK_SCHED : - *sample =task_sched_runtime (p); -break ; +return task_sched_runtime (p);default :WARN_ON_ONCE (1 ); } +return 0 ; }/*
@@ -299,8 +296,8 @@ thread_group_start_cputime(struct task_s* held to protect the task traversal on a full update. clkid is already * validated. */ -static void cpu_clock_sample_group (const clockid_t clkid,struct task_struct *p, -u64 *sample,bool start) +static u64 cpu_clock_sample_group (const clockid_t clkid,struct task_struct *p, +bool start) {struct thread_group_cputimer *cputimer = &p->signal->cputimer;struct task_cputime cputime;
@@ -316,17 +313,15 @@ static void cpu_clock_sample_group(constswitch (clkid) {case CPUCLOCK_PROF : - *sample = cputime.utime + cputime.stime; -break ; +return cputime.utime + cputime.stime;case CPUCLOCK_VIRT : - *sample = cputime.utime; -break ; +return cputime.utime;case CPUCLOCK_SCHED : - *sample = cputime.sum_exec_runtime; -break ; +return cputime.sum_exec_runtime;default :WARN_ON_ONCE (1 ); } +return 0 ; }static int posix_cpu_clock_get (const clockid_t clock,struct timespec64 *tp)
@@ -340,9 +335,9 @@ static int posix_cpu_clock_get(const cloreturn -EINVAL ;if (CPUCLOCK_PERTHREAD (clock)) -cpu_clock_sample (clkid, tsk, &t); + t =cpu_clock_sample (clkid, tsk);else -cpu_clock_sample_group (clkid, tsk, &t, false); + t =cpu_clock_sample_group (clkid, tsk, false);put_task_struct (tsk); *tp =ns_to_timespec64 (t);
@@ -604,11 +599,10 @@ static int posix_cpu_timer_set(struct k_* times (in arm_timer). With an absolute time, we must * check if it's already passed. In short, we need a sample. */ -if (CPUCLOCK_PERTHREAD (timer->it_clock)) { -cpu_clock_sample (clkid, p, &val); - }else { -cpu_clock_sample_group (clkid, p, &val, true); - } +if (CPUCLOCK_PERTHREAD (timer->it_clock)) + val =cpu_clock_sample (clkid, p); +else + val =cpu_clock_sample_group (clkid, p, true);if (old) {if (old_expires ==0 ) {
@@ -716,7 +710,7 @@ static void posix_cpu_timer_get(struct k* Sample the clock to take the difference with the expiry time. */ if (CPUCLOCK_PERTHREAD (timer->it_clock)) { -cpu_clock_sample (clkid, p, &now); + now =cpu_clock_sample (clkid, p); }else {struct sighand_struct *sighand;unsigned long flags;
@@ -736,7 +730,7 @@ static void posix_cpu_timer_get(struct k timer->it.cpu.expires =0 ;return ; }else { -cpu_clock_sample_group (clkid, p, &now, false); + now =cpu_clock_sample_group (clkid, p, false);unlock_task_sighand (p, &flags); } }
@@ -998,7 +992,7 @@ static void posix_cpu_timer_rearm(struct* Fetch the current sample and update the timer's expiry time. */ if (CPUCLOCK_PERTHREAD (timer->it_clock)) { -cpu_clock_sample (clkid, p, &now); + now =cpu_clock_sample (clkid, p);bump_cpu_timer (timer, now);if (unlikely (p->exit_state))return ;
@@ -1024,7 +1018,7 @@ static void posix_cpu_timer_rearm(struct/* If the process is dying, no need to rearm */ goto unlock; } -cpu_clock_sample_group (clkid, p, &now, true); + now =cpu_clock_sample_group (clkid, p, true);bump_cpu_timer (timer, now);/* Leave the sighand locked for the call below. */ }
@@ -1206,7 +1200,7 @@ void set_process_cpu_timer(struct task_sif (WARN_ON_ONCE (clock_idx >=CPUCLOCK_SCHED ))return ; -cpu_clock_sample_group (clock_idx, tsk, &now, true); + now =cpu_clock_sample_group (clock_idx, tsk, true);if (oldval) {/*
PATCH 22/44 posix-cpu-timers: Sample task times once in expiry check
In reply to Thomas GleixnerSampling the task times twice does not make sense. Do it once.
--- kernel/time/posix-cpu-timers.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
kernel/time/posix-cpu-timers.c +6 -4
@@ -785,9 +785,9 @@ static inline void check_dl_overrun(strustatic void check_thread_timers (struct task_struct *tsk,struct list_head *firing) { -struct list_head *timers = tsk->cpu_timers;struct task_cputime *tsk_expires = &tsk->cputime_expires; -u64 expires; +struct list_head *timers = tsk->cpu_timers; +u64 expires, stime, utime;unsigned long soft;if (dl_task (tsk))
@@ -800,10 +800,12 @@ static void check_thread_timers(struct tif (task_cputime_zero (&tsk->cputime_expires))return ; - expires =check_timers_list (timers, firing,prof_ticks (tsk)); +task_cputime (tsk, &utime, &stime); + + expires =check_timers_list (timers, firing, utime + stime); tsk_expires->prof_exp = expires; - expires =check_timers_list (++timers, firing,virt_ticks (tsk)); + expires =check_timers_list (++timers, firing, utime); tsk_expires->virt_exp = expires; tsk_expires->sched_exp = check_timers_list(++timers, firing,
PATCH 23/44 posix-cpu-timers: Move prof/virt_ticks into caller
In reply to Thomas GleixnerThe functions have only one caller left. No point in having them.
Move the almost duplicated code into the caller and simplify it.
--- kernel/time/posix-cpu-timers.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-)
kernel/time/posix-cpu-timers.c +9 -21
@@ -130,23 +130,6 @@ static inline int task_cputime_zero(consreturn 0 ; } -static inline u64 prof_ticks (struct task_struct *p) -{ -u64 utime, stime; - -task_cputime (p, &utime, &stime); - -return utime + stime; -} -static inline u64 virt_ticks (struct task_struct *p) -{ -u64 utime, stime; - -task_cputime (p, &utime, &stime); - -return utime; -} -static int posix_cpu_clock_getres (const clockid_t which_clock,struct timespec64 *tp) {
@@ -184,13 +167,18 @@ posix_cpu_clock_set(const clockid_t cloc*/ static u64 cpu_clock_sample (const clockid_t clkid,struct task_struct *p) { +u64 utime, stime; + +if (clkid ==CPUCLOCK_SCHED ) +return task_sched_runtime (p); + +task_cputime (p, &utime, &stime); +switch (clkid) {case CPUCLOCK_PROF : -return prof_ticks (p); +return utime + stime;case CPUCLOCK_VIRT : -return virt_ticks (p); -case CPUCLOCK_SCHED : -return task_sched_runtime (p); +return utime;default :WARN_ON_ONCE (1 ); }
PATCH 24/44 posix-cpu-timers: Create a container struct
In reply to Thomas GleixnerPer task/process data of posix CPU timers is all over the place which makes the code hard to follow and requires ifdeffery.
Create a container to hold all this information in one place, so data is consolidated and the ifdeffery can be confined to the posix timer header file and removed from places like fork.
As a first step, move the cpu_timers list head array into the new struct and clean up the initializers and simplify fork. The remaining #ifdef in fork will be removed later.
--- include/linux/init_task.h | 11 ----------- include/linux/posix-timers.h | 33 +++++++++++++++++++++++++++++++++ include/linux/sched.h | 3 ++- include/linux/sched/signal.h | 4 ++-- kernel/fork.c | 11 ++++------- kernel/time/posix-cpu-timers.c | 20 ++++++++++---------- 6 files changed, 51 insertions(+), 31 deletions(-)
include/linux/init_task.h +0 -11
@@ -36,17 +36,6 @@ extern struct cred init_cred;#define INIT_PREV_CPUTIME (x)#endif -#ifdef CONFIG_POSIX_TIMERS -#define INIT_CPU_TIMERS (s) \ - .cpu_timers = { \ - LIST_HEAD_INIT(s.cpu_timers[0]), \ - LIST_HEAD_INIT(s.cpu_timers[1]), \ - LIST_HEAD_INIT(s.cpu_timers[2]), \ - }, -#else -#define INIT_CPU_TIMERS (s) -#endif -#define INIT_TASK_COMM "swapper"/* Attach to the init_task data structure for proper alignment */
include/linux/posix-timers.h +33 -0
@@ -62,6 +62,39 @@ static inline int clockid_to_fd(const clreturn ~(clk >>3 ); } +/** +* posix_cputimers - Container for posix CPU timer related data +* @cpu_timers: List heads to queue posix CPU timers +* +* Used in task_struct and signal_struct +*/ +struct posix_cputimers { +struct list_head cpu_timers[CPUCLOCK_MAX ]; +}; + +static inline void posix_cputimers_init (struct posix_cputimers *pct) +{ +INIT_LIST_HEAD (&pct->cpu_timers[0 ]); +INIT_LIST_HEAD (&pct->cpu_timers[1 ]); +INIT_LIST_HEAD (&pct->cpu_timers[2 ]); +} + +#ifdef CONFIG_POSIX_TIMERS +/* Init task static initializer */ +#define INIT_CPU_TIMERLISTS (c) { \ + LIST_HEAD_INIT(c.cpu_timers[0]), \ + LIST_HEAD_INIT(c.cpu_timers[1]), \ + LIST_HEAD_INIT(c.cpu_timers[2]), \ +} + +#define INIT_CPU_TIMERS (s) \ + .posix_cputimers = { \ + .cpu_timers = INIT_CPU_TIMERLISTS(s.posix_cputimers), \ + }, +#else +#define INIT_CPU_TIMERS (s) +#endif +#define REQUEUE_PENDING 1/**
include/linux/sched.h +2 -1
@@ -28,6 +28,7 @@#include <linux/signal_types.h> #include <linux/mm_types_task.h> #include <linux/task_io_accounting.h> +#include <linux/posix-timers.h> #include <linux/rseq.h> /* task_struct member predeclarations (sorted alphabetically): */
@@ -878,7 +879,7 @@ struct task_struct {#ifdef CONFIG_POSIX_TIMERS struct task_cputime cputime_expires; -struct list_head cpu_timers[3 ]; +struct posix_cputimers posix_cputimers;#endif /* Process credentials: */
include/linux/sched/signal.h +2 -2
@@ -9,6 +9,7 @@#include <linux/sched/task.h> #include <linux/cred.h> #include <linux/refcount.h> +#include <linux/posix-timers.h> /* * Types defining task->signal and task->sighand and APIs using them:
@@ -151,8 +152,7 @@ struct signal_struct {/* Earliest-expiration cache. */ struct task_cputime cputime_expires; -struct list_head cpu_timers[3 ]; - +struct posix_cputimers posix_cputimers;#endif /* PID/PID hash table linkage. */
kernel/fork.c +4 -7
@@ -1523,6 +1523,7 @@ void __cleanup_sighand(struct sighand_st*/ static void posix_cpu_timers_init_group (struct signal_struct *sig) { +struct posix_cputimers *pct = &sig->posix_cputimers;unsigned long cpu_limit; cpu_limit =READ_ONCE (sig->rlim[RLIMIT_CPU ].rlim_cur);
@@ -1531,10 +1532,7 @@ static void posix_cpu_timers_init_group( sig->cputimer.running = true; } -/* The timer lists. */ -INIT_LIST_HEAD (&sig->cpu_timers[0 ]); -INIT_LIST_HEAD (&sig->cpu_timers[1 ]); -INIT_LIST_HEAD (&sig->cpu_timers[2 ]); +posix_cputimers_init (pct); }#else static inline void posix_cpu_timers_init_group (struct signal_struct *sig) { }
@@ -1649,9 +1647,8 @@ static void posix_cpu_timers_init(struct tsk->cputime_expires.prof_exp =0 ; tsk->cputime_expires.virt_exp =0 ; tsk->cputime_expires.sched_exp =0 ; -INIT_LIST_HEAD (&tsk->cpu_timers[0 ]); -INIT_LIST_HEAD (&tsk->cpu_timers[1 ]); - INIT_LIST_HEAD(&tsk->cpu_timers[2]); + + posix_cputimers_init(&tsk->posix_cputimers); }#else static inline void posix_cpu_timers_init (struct task_struct *tsk) { }
kernel/time/posix-cpu-timers.c +10 -10
@@ -407,11 +407,11 @@ static void cleanup_timers_list(struct l* * This must be called with the siglock held. */ -static void cleanup_timers (struct list_head *head) +static void cleanup_timers (struct posix_cputimers *pct) { -cleanup_timers_list (head); -cleanup_timers_list (++head); -cleanup_timers_list (++head); +cleanup_timers_list (&pct->cpu_timers[CPUCLOCK_PROF ]); +cleanup_timers_list (&pct->cpu_timers[CPUCLOCK_VIRT ]); +cleanup_timers_list (&pct->cpu_timers[CPUCLOCK_SCHED ]); }/*
@@ -421,11 +421,11 @@ static void cleanup_timers(struct list_h*/ void posix_cpu_timers_exit (struct task_struct *tsk) { -cleanup_timers (tsk->cpu_timers); +cleanup_timers (&tsk->posix_cputimers); }void posix_cpu_timers_exit_group (struct task_struct *tsk) { -cleanup_timers (tsk->signal->cpu_timers); +cleanup_timers (&tsk->signal->posix_cputimers); }static inline int expires_gt (u64 expires,u64 new_exp)
@@ -446,10 +446,10 @@ static void arm_timer(struct k_itimer *tstruct cpu_timer_list *next;if (CPUCLOCK_PERTHREAD (timer->it_clock)) { - head = p->cpu_timers; + head = p->posix_cputimers.cpu_timers; cputime_expires = &p->cputime_expires; }else { - head = p->signal->cpu_timers; + head = p->signal->posix_cputimers.cpu_timers; cputime_expires = &p->signal->cputime_expires; } head +=CPUCLOCK_WHICH (timer->it_clock);
@@ -773,8 +773,8 @@ static inline void check_dl_overrun(strustatic void check_thread_timers (struct task_struct *tsk,struct list_head *firing) { +struct list_head *timers = tsk->posix_cputimers.cpu_timers;struct task_cputime *tsk_expires = &tsk->cputime_expires; -struct list_head *timers = tsk->cpu_timers;u64 expires, stime, utime;unsigned long soft;
@@ -879,9 +879,9 @@ static void check_process_timers(struct struct list_head *firing) {struct signal_struct *const sig = tsk->signal; +struct list_head *timers = sig->posix_cputimers.cpu_timers;u64 utime, ptime, virt_expires, prof_expires;u64 sum_sched_runtime, sched_expires; -struct list_head *timers = sig->cpu_timers;struct task_cputime cputime;unsigned long soft;
PATCH 25/44 sched: Move struct task_cputime to types.h
In reply to Thomas GleixnerFor upcoming posix-timer changes to avoid include recursion hell.
--- include/linux/sched.h | 17 +---------------- include/linux/sched/types.h | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 16 deletions(-)
include/linux/sched.h +1 -16
@@ -25,6 +25,7 @@#include <linux/resource.h> #include <linux/latencytop.h> #include <linux/sched/prio.h> +#include <linux/sched/types.h> #include <linux/signal_types.h> #include <linux/mm_types_task.h> #include <linux/task_io_accounting.h>
@@ -245,22 +246,6 @@ struct prev_cputime {#endif }; -/** -* struct task_cputime - collected CPU time counts -* @utime: time spent in user mode, in nanoseconds -* @stime: time spent in kernel mode, in nanoseconds -* @sum_exec_runtime: total time spent on the CPU, in nanoseconds -* -* This structure groups together three kinds of CPU time that are tracked for -* threads and thread groups. Most things considering CPU time want to group -* these counts together and treat all three of them in parallel. -*/ -struct task_cputime { -u64 utime; -u64 stime; -unsigned long long sum_exec_runtime; -}; -/* Alternate field names when used on cache expirations: */ #define virt_exp utime#define prof_exp stime
include/linux/sched/types.h new file +21 -0
@@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_SCHED_TYPES_H +#define _LINUX_SCHED_TYPES_H + +/** +* struct task_cputime - collected CPU time counts +* @utime: time spent in user mode, in nanoseconds +* @stime: time spent in kernel mode, in nanoseconds +* @sum_exec_runtime: total time spent on the CPU, in nanoseconds +* +* This structure groups together three kinds of CPU time that are tracked for +* threads and thread groups. Most things considering CPU time want to group +* these counts together and treat all three of them in parallel. +*/ +struct task_cputime { +u64 utime; +u64 stime; +unsigned long long sum_exec_runtime; +}; + +#endif
PATCH 26/44 posix-cpu-timers: Move expiry cache into struct posix_cputimers
In reply to Thomas GleixnerThe expiry cache belongs into the posix_cputimers container where the other cpu timers information is.
--- include/linux/posix-timers.h | 31 ++++++++++++++++++++++++++++++- include/linux/sched.h | 6 ------ include/linux/sched/signal.h | 3 --- kernel/fork.c | 25 +++---------------------- kernel/sched/rt.c | 6 ++++-- kernel/time/posix-cpu-timers.c | 38 ++++++++++++++++++++------------------ 6 files changed, 57 insertions(+), 52 deletions(-)
include/linux/posix-timers.h +30 -1
@@ -62,24 +62,49 @@ static inline int clockid_to_fd(const clreturn ~(clk >>3 ); } +/* +* Alternate field names for struct task_cputime when used on cache +* expirations. Will go away soon. +*/ +#define virt_exp utime +#define prof_exp stime +#define sched_exp sum_exec_runtime + +#ifdef CONFIG_POSIX_TIMERS /** * posix_cputimers - Container for posix CPU timer related data +* @cputime_expires: Earliest-expiration cache * @cpu_timers: List heads to queue posix CPU timers * * Used in task_struct and signal_struct */ struct posix_cputimers { +struct task_cputime cputime_expires;struct list_head cpu_timers[CPUCLOCK_MAX ]; };static inline void posix_cputimers_init (struct posix_cputimers *pct) { +memset (&pct->cputime_expires,0 ,sizeof (pct->cputime_expires));INIT_LIST_HEAD (&pct->cpu_timers[0 ]);INIT_LIST_HEAD (&pct->cpu_timers[1 ]);INIT_LIST_HEAD (&pct->cpu_timers[2 ]); } -#ifdef CONFIG_POSIX_TIMERS +static inline void posix_cputimers_group_init (struct posix_cputimers *pct, +u64 cpu_limit) +{ +posix_cputimers_init (pct); +if (cpu_limit !=RLIM_INFINITY ) + pct->cputime_expires.prof_exp = cpu_limit *NSEC_PER_SEC ; +} + +static inline void posix_cputimers_rt_watchdog (struct posix_cputimers *pct, +u64 runtime) +{ + pct->cputime_expires.sched_exp = runtime; +} +/* Init task static initializer */ #define INIT_CPU_TIMERLISTS (c) { \ LIST_HEAD_INIT(c.cpu_timers[0]), \
@@ -92,7 +117,11 @@ static inline void posix_cputimers_init( .cpu_timers = INIT_CPU_TIMERLISTS(s.posix_cputimers), \ },#else +struct posix_cputimers { };#define INIT_CPU_TIMERS (s) +static inline void posix_cputimers_init (struct posix_cputimers *pct) { } +static inline void posix_cputimers_group_init (struct posix_cputimers *pct, +u64 cpu_limit) { }#endif #define REQUEUE_PENDING 1
include/linux/sched.h +0 -6
@@ -246,11 +246,6 @@ struct prev_cputime {#endif }; -/* Alternate field names when used on cache expirations: */ -#define virt_exp utime -#define prof_exp stime -#define sched_exp sum_exec_runtime -enum vtime_state {/* Task is sleeping or running in a CPU with VTIME inactive: */ VTIME_INACTIVE =0 ,
@@ -863,7 +858,6 @@ struct task_struct {unsigned long maj_flt;#ifdef CONFIG_POSIX_TIMERS -struct task_cputime cputime_expires;struct posix_cputimers posix_cputimers;#endif
include/linux/sched/signal.h +0 -3
@@ -149,9 +149,6 @@ struct signal_struct {*/ struct thread_group_cputimer cputimer; -/* Earliest-expiration cache. */ -struct task_cputime cputime_expires; -struct posix_cputimers posix_cputimers;#endif
kernel/fork.c +3 -22
@@ -1527,12 +1527,9 @@ static void posix_cpu_timers_init_group(unsigned long cpu_limit; cpu_limit =READ_ONCE (sig->rlim[RLIMIT_CPU ].rlim_cur); -if (cpu_limit !=RLIM_INFINITY ) { - sig->cputime_expires.prof_exp = cpu_limit *NSEC_PER_SEC ; +posix_cputimers_group_init (pct, cpu_limit); +if (cpu_limit !=RLIM_INFINITY ) sig->cputimer.running = true; - } - - posix_cputimers_init(pct); }#else static inline void posix_cpu_timers_init_group (struct signal_struct *sig) { }
@@ -1638,22 +1635,6 @@ static void rt_mutex_init_task(struct ta#endif } -#ifdef CONFIG_POSIX_TIMERS -/* -* Initialize POSIX timer handling for a single task. -*/ -static void posix_cpu_timers_init (struct task_struct *tsk) -{ - tsk->cputime_expires.prof_exp =0 ; - tsk->cputime_expires.virt_exp =0 ; - tsk->cputime_expires.sched_exp =0 ; - -posix_cputimers_init (&tsk->posix_cputimers); -} -#else -static inline void posix_cpu_timers_init (struct task_struct *tsk) { } -#endif -static inline void init_task_pid_links (struct task_struct *task) {enum pid_type type;
@@ -1932,7 +1913,7 @@ static __latent_entropy struct task_strutask_io_accounting_init (&p->ioac);acct_clear_integrals (p); -posix_cpu_timers_init (p); +posix_cputimers_init (&p->posix_cputimers); p->io_context =NULL ;audit_set_context (p,NULL );
kernel/sched/rt.c +4 -2
@@ -2305,8 +2305,10 @@ static void watchdog(struct rq *rq, stru } next =DIV_ROUND_UP (min (soft, hard),USEC_PER_SEC /HZ ); -if (p->rt.timeout > next) - p->cputime_expires.sched_exp = p->se.sum_exec_runtime; +if (p->rt.timeout > next) { +posix_cputimers_rt_watchdog (&p->posix_cputimers, + p->se.sum_exec_runtime); + } } }#else
kernel/time/posix-cpu-timers.c +20 -18
@@ -22,9 +22,9 @@ static void posix_cpu_timer_rearm(struct/* * Called after updating RLIMIT_CPU to run cpu timer and update -* tsk->signal->cputime_expires expiration cache if necessary. Needs -* siglock protection since other code may update expiration cache as -* well. +* tsk->signal->posix_cputimers.cputime_expires expiration cache if +* necessary. Needs siglock protection since other code may update +* expiration cache as well. */ void update_rlimit_cpu (struct task_struct *task,unsigned long rlim_new) {
@@ -447,10 +447,10 @@ static void arm_timer(struct k_itimer *tif (CPUCLOCK_PERTHREAD (timer->it_clock)) { head = p->posix_cputimers.cpu_timers; - cputime_expires = &p->cputime_expires; + cputime_expires = &p->posix_cputimers.cputime_expires; }else { head = p->signal->posix_cputimers.cpu_timers; - cputime_expires = &p->signal->cputime_expires; + cputime_expires = &p->signal->posix_cputimers.cputime_expires; } head +=CPUCLOCK_WHICH (timer->it_clock);
@@ -774,7 +774,7 @@ static void check_thread_timers(struct t struct list_head *firing) {struct list_head *timers = tsk->posix_cputimers.cpu_timers; -struct task_cputime *tsk_expires = &tsk->cputime_expires; +struct task_cputime *tsk_expires = &tsk->posix_cputimers.cputime_expires;u64 expires, stime, utime;unsigned long soft;
@@ -785,7 +785,7 @@ static void check_thread_timers(struct t* If cputime_expires is zero, then there are no active * per thread CPU timers. */ -if (task_cputime_zero (&tsk->cputime_expires)) +if (task_cputime_zero (tsk_expires))return ;task_cputime (tsk, &utime, &stime);
@@ -954,10 +954,10 @@ static void check_process_timers(struct prof_expires = x; } - sig->cputime_expires.prof_exp = prof_expires; - sig->cputime_expires.virt_exp = virt_expires; - sig->cputime_expires.sched_exp = sched_expires; -if (task_cputime_zero (&sig->cputime_expires)) + sig->posix_cputimers.cputime_expires.prof_exp = prof_expires; + sig->posix_cputimers.cputime_expires.virt_exp = virt_expires; + sig->posix_cputimers.cputime_expires.sched_exp = sched_expires; +if (task_cputime_zero (&sig->posix_cputimers.cputime_expires))stop_process_timers (sig); sig->cputimer.checking_timer = false;
@@ -1058,12 +1058,13 @@ static inline int fastpath_timer_check(s {struct signal_struct *sig; -if (!task_cputime_zero (&tsk->cputime_expires)) { +if (!task_cputime_zero (&tsk->posix_cputimers.cputime_expires)) {struct task_cputime task_sample;task_cputime (tsk, &task_sample.utime, &task_sample.stime); task_sample.sum_exec_runtime = tsk->se.sum_exec_runtime; -if (task_cputime_expired (&task_sample, &tsk->cputime_expires)) +if (task_cputime_expired (&task_sample, + &tsk->posix_cputimers.cputime_expires))return 1 ; }
@@ -1088,7 +1089,8 @@ static inline int fastpath_timer_check(ssample_cputime_atomic (&group_sample, &sig->cputimer.cputime_atomic); -if (task_cputime_expired (&group_sample, &sig->cputime_expires)) +if (task_cputime_expired (&group_sample, + &sig->posix_cputimers.cputime_expires))return 1 ; }
@@ -1218,12 +1220,12 @@ void set_process_cpu_timer(struct task_s*/ switch (clock_idx) {case CPUCLOCK_PROF : -if (expires_gt (tsk->signal->cputime_expires.prof_exp, *newval)) - tsk->signal->cputime_expires.prof_exp = *newval; +if (expires_gt (tsk->signal->posix_cputimers.cputime_expires.prof_exp, *newval)) + tsk->signal->posix_cputimers.cputime_expires.prof_exp = *newval;break ;case CPUCLOCK_VIRT : -if (expires_gt (tsk->signal->cputime_expires.virt_exp, *newval)) - tsk->signal->cputime_expires.virt_exp = *newval; +if (expires_gt (tsk->signal->posix_cputimers.cputime_expires.virt_exp, *newval)) + tsk->signal->posix_cputimers.cputime_expires.virt_exp = *newval;break ; }
PATCH 27/44 posix-cpu-timers: Provide array based access to expiry cache
In reply to Thomas GleixnerUsing struct task_cputime for the expiry cache is a pretty odd choice and comes with magic defines to rename the fields for usage in the expiry cache.
struct task_cputime is basically a u64 array with 3 members, but it has distinct members.
The expiry cache content is different than the content of task_cputime because
expiry[PROF] = task_cputime.stime + task_cputime.utime expiry[VIRT] = task_cputime.utime expiry[SCHED] = task_cputime.sum_exec_runtime
So there is no direct mapping between task_cputime and the expiry cache and the #define based remapping is just a horrible hack.
Having the expiry cache array based allows further simplification of the expiry code.
To avoid an all in one cleanup which is hard to review add a temporary anonymous union into struct task_cputime which allows array based access to it. That requires to reorder the members. Add a build time sanity check to validate that the members are at the same place.
The union and the build time checks will be removed after conversion.
--- include/linux/posix-timers.h | 26 +++++++++++++++++++------- include/linux/sched/types.h | 4 ++-- kernel/time/posix-cpu-timers.c | 10 ++++++++++ 3 files changed, 31 insertions(+), 9 deletions(-)
include/linux/posix-timers.h +19 -7
@@ -65,27 +65,39 @@ static inline int clockid_to_fd(const cl/* * Alternate field names for struct task_cputime when used on cache * expirations. Will go away soon. +* +* stime corresponds to CLOCKCPU_PROF +* utime corresponds to CLOCKCPU_VIRT +* sum_exex_runtime corresponds to CLOCKCPU_SCHED +* +* The ordering is currently enforced so struct task_cputime and the +* expiries array in struct posix_cputimers are equivalent. */ -#define virt_exp utime#define prof_exp stime +#define virt_exp utime#define sched_exp sum_exec_runtime#ifdef CONFIG_POSIX_TIMERS /** * posix_cputimers - Container for posix CPU timer related data -* @cputime_expires: Earliest-expiration cache +* @cputime_expires: Earliest-expiration cache task_cputime based +* @expiries: Earliest-expiration cache array based * @cpu_timers: List heads to queue posix CPU timers * * Used in task_struct and signal_struct */ struct posix_cputimers { -struct task_cputime cputime_expires; -struct list_head cpu_timers[CPUCLOCK_MAX ]; +/* Temporary union until all users are cleaned up */ +union { +struct task_cputime cputime_expires; +u64 expiries[CPUCLOCK_MAX ]; + }; +struct list_head cpu_timers[CPUCLOCK_MAX ]; };static inline void posix_cputimers_init (struct posix_cputimers *pct) { -memset (&pct->cputime_expires,0 ,sizeof (pct->cputime_expires)); +memset (&pct->expiries,0 ,sizeof (pct->expiries));INIT_LIST_HEAD (&pct->cpu_timers[0 ]);INIT_LIST_HEAD (&pct->cpu_timers[1 ]);INIT_LIST_HEAD (&pct->cpu_timers[2 ]);
@@ -96,13 +108,13 @@ static inline void posix_cputimers_group {posix_cputimers_init (pct);if (cpu_limit !=RLIM_INFINITY ) - pct->cputime_expires.prof_exp = cpu_limit *NSEC_PER_SEC ; + pct->expiries[CPUCLOCK_PROF ] = cpu_limit *NSEC_PER_SEC ; }static inline void posix_cputimers_rt_watchdog (struct posix_cputimers *pct,u64 runtime) { - pct->cputime_expires.sched_exp = runtime; + pct->expiries[CPUCLOCK_SCHED ] = runtime; }/* Init task static initializer */
include/linux/sched/types.h +2 -2
@@ -4,8 +4,8 @@/** * struct task_cputime - collected CPU time counts -* @utime: time spent in user mode, in nanoseconds * @stime: time spent in kernel mode, in nanoseconds +* @utime: time spent in user mode, in nanoseconds * @sum_exec_runtime: total time spent on the CPU, in nanoseconds * * This structure groups together three kinds of CPU time that are tracked for
@@ -13,8 +13,8 @@* these counts together and treat all three of them in parallel. */ struct task_cputime { -u64 utime;u64 stime; +u64 utime;unsigned long long sum_exec_runtime; };
kernel/time/posix-cpu-timers.c +10 -0
@@ -18,6 +18,16 @@#include "posix-timers.h" +static inline void temporary_check (void ) +{ +BUILD_BUG_ON (offsetof(struct task_cputime , stime) != +CPUCLOCK_PROF *sizeof (u64)); +BUILD_BUG_ON (offsetof(struct task_cputime , utime) != +CPUCLOCK_VIRT *sizeof (u64)); +BUILD_BUG_ON (offsetof(struct task_cputime , sum_exec_runtime) != +CPUCLOCK_SCHED *sizeof (u64)); +} +static void posix_cpu_timer_rearm (struct k_itimer *timer);/*
Re: [patch 27/44] posix-cpu-timers: Provide array based access to expiry cache
Quoted 18 lines · Thomas Gleixner
Using struct
task_cputimefor the expiry cache is a pretty odd choice and comes with magic defines to rename the fields for usage in the expiry cache.struct
task_cputimeis basically a u64 array with 3 members, but it has distinct members.The expiry cache content is different than the content of
task_cputimebecauseexpiry[PROF] = task_cputime.stime + task_cputime.utime expiry[VIRT] = task_cputime.utime expiry[SCHED] = task_cputime.sum_exec_runtime
So there is no direct mapping between
task_cputimeand the expiry cache and the #define based remapping is just a horrible hack.struct
posix_cputimers{
include/linux/posix-timers.h static inline int clockid_to_fd(const cl · line 96
Quoted 9 lines
include/linux/posix-timers.h +6
-2-struct task_cputime cputime_expires; -struct list_head cpu_timers[CPUCLOCK_MAX ]; +/* Temporary union until all users are cleaned up */ +union { +struct task_cputime cputime_expires; +u64 expiries[CPUCLOCK_MAX ]; + }; +struct list_head cpu_timers[CPUCLOCK_MAX ]; };
Could we please name this first_expiry[] or such, to make it clear that this is cached value of the first expiry of all timers of this process, instead of the rather vague 'expiries[]' naming?
Also, while at it, after the above temporary transition union, the final structure becomes:
struct posix_cputimers {
u64 expiries[CPUCLOCK_MAX];
struct list_head cpu_timers[CPUCLOCK_MAX];};
Wouldn't it be more natural and easier to read to have the list head and the expiry together:
struct posix_cputimer_list {
u64 first_expiry;
struct list_head list;
};
struct posix_cputimers {
struct posix_cputimer_list timers[CPUCLOCK_MAX];
};?
This makes the array structure rather clear and the first_expiry field mostly self-documenting.
Thanks,
Ingo
Re: [patch 27/44] posix-cpu-timers: Provide array based access to expiry cache
Quoted 36 lines · Ingo Molnar
include/linux/posix-timers.h static inline int clockid_to_fd(const cl · line 96
Quoted 10 lines · Thomas Gleixner
struct
posix_cputimers{include/linux/posix-timers.h +6
-2-struct task_cputime cputime_expires; -struct list_head cpu_timers[CPUCLOCK_MAX ]; +/* Temporary union until all users are cleaned up */ +union { +struct task_cputime cputime_expires; +u64 expiries[CPUCLOCK_MAX ]; + }; +struct list_head cpu_timers[CPUCLOCK_MAX ]; };Could we please name this first_expiry[] or such, to make it clear that this is cached value of the first expiry of all timers of this process, instead of the rather vague 'expiries[]' naming?
Also, while at it, after the above temporary transition union, the final structure becomes:
struct posix_cputimers { u64 expiries[CPUCLOCK_MAX]; struct list_head cpu_timers[CPUCLOCK_MAX];};
Wouldn't it be more natural and easier to read to have the list head and the expiry together:
struct posix_cputimer_list { u64 first_expiry; struct list_head list; }; struct posix_cputimers { struct posix_cputimer_list timers[CPUCLOCK_MAX]; };
Quoted 4 lines
?
This makes the array structure rather clear and the
first_expiryfield mostly self-documenting.
I kept the odd named expiries for the temporary union and then after the patch which removes the abused struct task_cputime, I applied a separate cleanup which looks similar to the above.
Just the names are a bit different and more aligned to what we have in hrtimers:
struct posix_cputimer_base {
u64 nextevt;
struct timerqueue_head tqhead;
};and then have
struct posix_cputimers {
struct posix_cputimer_base bases[CPUCLOCK_MAX];
};I'll send out a new version after doing some more testing.
Thanks,
tglx
PATCH 28/44 posix-cpu-timers: Simplify timer queueing
In reply to Thomas GleixnerNow that the expiry cache can be accessed as an array, the per clock checking can be reduced to just comparing the corresponding array elements.
--- kernel/time/posix-cpu-timers.c | 57 +++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 35 deletions(-)
kernel/time/posix-cpu-timers.c +22 -35
@@ -449,20 +449,20 @@ static inline int expires_gt(u64 expires*/ static void arm_timer (struct k_itimer *timer) { +struct cpu_timer_list *const nt = &timer->it.cpu; +int clkidx =CPUCLOCK_WHICH (timer->it_clock); +u64 *cpuexp, newexp = timer->it.cpu.expires;struct task_struct *p = timer->it.cpu.task;struct list_head *head, *listpos; -struct task_cputime *cputime_expires; -struct cpu_timer_list *const nt = &timer->it.cpu;struct cpu_timer_list *next;if (CPUCLOCK_PERTHREAD (timer->it_clock)) { - head = p->posix_cputimers.cpu_timers; - cputime_expires = &p->posix_cputimers.cputime_expires; + head = p->posix_cputimers.cpu_timers + clkidx; + cpuexp = p->posix_cputimers.expiries + clkidx; }else { - head = p->signal->posix_cputimers.cpu_timers; - cputime_expires = &p->signal->posix_cputimers.cputime_expires; + head = p->signal->posix_cputimers.cpu_timers + clkidx; + cpuexp = p->signal->posix_cputimers.expiries + clkidx; } - head +=CPUCLOCK_WHICH (timer->it_clock); listpos = head;list_for_each_entry (next, head, entry) {
@@ -472,35 +472,22 @@ static void arm_timer(struct k_itimer *t }list_add (&nt->entry, listpos); -if (listpos == head) { -u64 exp = nt->expires; +if (listpos != head) +return ; -/* -* We are the new earliest-expiring POSIX 1.b timer, hence -* need to update expiration cache. Take into account that -* for process timers we share expiration cache with itimers -* and RLIMIT_CPU and for thread timers with RLIMIT_RTTIME. -*/ - -switch (CPUCLOCK_WHICH (timer->it_clock)) { -case CPUCLOCK_PROF : -if (expires_gt (cputime_expires->prof_exp, exp)) - cputime_expires->prof_exp = exp; -break ; -case CPUCLOCK_VIRT : -if (expires_gt (cputime_expires->virt_exp, exp)) - cputime_expires->virt_exp = exp; -break ; -case CPUCLOCK_SCHED : -if (expires_gt (cputime_expires->sched_exp, exp)) - cputime_expires->sched_exp = exp; -break ; - } -if (CPUCLOCK_PERTHREAD (timer->it_clock)) -tick_dep_set_task (p,TICK_DEP_BIT_POSIX_TIMER ); -else -tick_dep_set_signal (p->signal,TICK_DEP_BIT_POSIX_TIMER ); - } +/* +* We are the new earliest-expiring POSIX 1.b timer, hence +* need to update expiration cache. Take into account that +* for process timers we share expiration cache with itimers +* and RLIMIT_CPU and for thread timers with RLIMIT_RTTIME. +*/ +if (expires_gt (*cpuexp, newexp)) + *cpuexp = newexp; + +if (CPUCLOCK_PERTHREAD (timer->it_clock)) +tick_dep_set_task (p,TICK_DEP_BIT_POSIX_TIMER ); +else +tick_dep_set_signal (p->signal,TICK_DEP_BIT_POSIX_TIMER ); }/*
PATCH 29/44 posix-cpu-timers: Simplify set_process_cpu_timer()
In reply to Thomas GleixnerThe expiry cache can now be accessed as an array. Replace the per clock checks with a simple comparison of the clock indexed array member.
--- kernel/time/posix-cpu-timers.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)
kernel/time/posix-cpu-timers.c +8 -16
@@ -1181,15 +1181,15 @@ void run_posix_cpu_timers(void)* Set one of the process-wide special case CPU timers or RLIMIT_CPU. * The tsk->sighand->siglock must be held by the caller. */ -void set_process_cpu_timer (struct task_struct *tsk,unsigned int clock_idx, +void set_process_cpu_timer (struct task_struct *tsk,unsigned int clkid,u64 *newval,u64 *oldval) { -u64 now; +u64 now, *expiry = tsk->signal->posix_cputimers.expiries + clkid; -if (WARN_ON_ONCE (clock_idx >=CPUCLOCK_SCHED )) +if (WARN_ON_ONCE (clkid >=CPUCLOCK_SCHED ))return ; - now =cpu_clock_sample_group (clock_idx, tsk, true); + now =cpu_clock_sample_group (clkid, tsk, true);if (oldval) {/*
@@ -1212,19 +1212,11 @@ void set_process_cpu_timer(struct task_s }/* -* Update expiration cache if we are the earliest timer, or eventually -* RLIMIT_CPU limit is earlier than prof_exp cpu timer expire. +* Update expiration cache if this is the earliest timer. CPUCLOCK_PROF +* expiry cache is also used by RLIMIT_CPU!. */ -switch (clock_idx) { -case CPUCLOCK_PROF : -if (expires_gt (tsk->signal->posix_cputimers.cputime_expires.prof_exp, *newval)) - tsk->signal->posix_cputimers.cputime_expires.prof_exp = *newval; -break ; -case CPUCLOCK_VIRT : -if (expires_gt (tsk->signal->posix_cputimers.cputime_expires.virt_exp, *newval)) - tsk->signal->posix_cputimers.cputime_expires.virt_exp = *newval; -break ; - } +if (expires_gt (*expiry, *newval)) + *expiry = *newval;tick_dep_set_signal (tsk->signal,TICK_DEP_BIT_POSIX_TIMER ); }
PATCH 30/44 posix-cpu-timers: Switch check_*_timers() to array cache
In reply to Thomas GleixnerUse the array based expiry cache in check_thread_timers() and convert the store in check_process_timers() for consistency.
--- kernel/time/posix-cpu-timers.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)
kernel/time/posix-cpu-timers.c +11 -15
@@ -771,8 +771,7 @@ static void check_thread_timers(struct t struct list_head *firing) {struct list_head *timers = tsk->posix_cputimers.cpu_timers; -struct task_cputime *tsk_expires = &tsk->posix_cputimers.cputime_expires; -u64 expires, stime, utime; +u64 stime, utime, *expires = tsk->posix_cputimers.expiries;unsigned long soft;if (dl_task (tsk))
@@ -782,19 +781,14 @@ static void check_thread_timers(struct t* If cputime_expires is zero, then there are no active * per thread CPU timers. */ -if (task_cputime_zero (tsk_expires)) +if (task_cputime_zero (&tsk->posix_cputimers.cputime_expires))return ;task_cputime (tsk, &utime, &stime); - expires =check_timers_list (timers, firing, utime + stime); - tsk_expires->prof_exp = expires; - - expires =check_timers_list (++timers, firing, utime); - tsk_expires->virt_exp = expires; - - tsk_expires->sched_exp =check_timers_list (++timers, firing, - tsk->se.sum_exec_runtime); + *expires++ =check_timers_list (timers, firing, utime + stime); + *expires++ =check_timers_list (++timers, firing, utime); + *expires =check_timers_list (++timers, firing, tsk->se.sum_exec_runtime);/* * Check for the special case thread timers.
@@ -832,7 +826,8 @@ static void check_thread_timers(struct t__group_send_sig_info (SIGXCPU ,SEND_SIG_PRIV , tsk); } } -if (task_cputime_zero (tsk_expires)) + +if (task_cputime_zero (&tsk->posix_cputimers.cputime_expires))tick_dep_clear_task (tsk,TICK_DEP_BIT_POSIX_TIMER ); }
@@ -951,9 +946,10 @@ static void check_process_timers(struct prof_expires = x; } - sig->posix_cputimers.cputime_expires.prof_exp = prof_expires; - sig->posix_cputimers.cputime_expires.virt_exp = virt_expires; - sig->posix_cputimers.cputime_expires.sched_exp = sched_expires; + sig->posix_cputimers.expiries[CPUCLOCK_PROF ] = prof_expires; + sig->posix_cputimers.expiries[CPUCLOCK_VIRT ] = virt_expires; + sig->posix_cputimers.expiries[CPUCLOCK_SCHED ] = sched_expires; +if (task_cputime_zero (&sig->posix_cputimers.cputime_expires))stop_process_timers (sig);
PATCH 31/44 posix-cpu-timers: Remove the odd field rename defines
In reply to Thomas GleixnerThe last users of the odd define based renaming of struct task_cputime members are gone. Good riddance.
--- include/linux/posix-timers.h | 15 --------------- 1 file changed, 15 deletions(-)
include/linux/posix-timers.h +0 -15
@@ -62,21 +62,6 @@ static inline int clockid_to_fd(const clreturn ~(clk >>3 ); } -/* -* Alternate field names for struct task_cputime when used on cache -* expirations. Will go away soon. -* -* stime corresponds to CLOCKCPU_PROF -* utime corresponds to CLOCKCPU_VIRT -* sum_exex_runtime corresponds to CLOCKCPU_SCHED -* -* The ordering is currently enforced so struct task_cputime and the -* expiries array in struct posix_cputimers are equivalent. -*/ -#define prof_exp stime -#define virt_exp utime -#define sched_exp sum_exec_runtime -#ifdef CONFIG_POSIX_TIMERS /** * posix_cputimers - Container for posix CPU timer related data
PATCH 32/44 posix-cpu-timers: Provide array based sample functions
In reply to Thomas GleixnerInstead of using task_cputime and doing the addition of utime and stime at all call sites, it's way simpler to have a sample array which allows indexed based checks against the expiry cache array.
--- kernel/time/posix-cpu-timers.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
kernel/time/posix-cpu-timers.c +26 -0
@@ -195,6 +195,32 @@ static u64 cpu_clock_sample(const clockireturn 0 ; } +static inline void store_samples (u64 *samples,u64 stime,u64 utime,u64 rtime) +{ + samples[CPUCLOCK_PROF ] = stime + utime; + samples[CPUCLOCK_VIRT ] = utime; + samples[CPUCLOCK_SCHED ] = rtime; +} + +static void task_sample_cputime (struct task_struct *p,u64 *samples) +{ +u64 stime, utime; + +task_cputime (p, &utime, &stime); +store_samples (samples, stime, utime, p->se.sum_exec_runtime); +} + +static void proc_sample_cputime_atomic (struct task_cputime_atomic *at, +u64 *samples) +{ +u64 stime, utime, rtime; + + utime =atomic64_read (&at->utime); + stime =atomic64_read (&at->stime); + rtime =atomic64_read (&at->sum_exec_runtime); +store_samples (samples, stime, utime, rtime); +} +/* * Set cputime to sum_cputime if sum_cputime > cputime. Use cmpxchg * to avoid race conditions with concurrent updates to cputime.
PATCH 33/44 posix-cpu-timers: Make expiry checks array based
In reply to Thomas GleixnerThe expiry cache is an array indexed by clock ids. The new sample functions allow to retrieve a corresponding array of samples.
Convert the fastpath expiry checks to make use of the new sample functions and do the comparisons on the sample and the expiry array.
Make the check for the expiry array being zero array based as well.
--- kernel/time/posix-cpu-timers.c | 85 +++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 49 deletions(-)
kernel/time/posix-cpu-timers.c +36 -49
@@ -32,7 +32,7 @@ static void posix_cpu_timer_rearm(struct/* * Called after updating RLIMIT_CPU to run cpu timer and update -* tsk->signal->posix_cputimers.cputime_expires expiration cache if +* tsk->signal->posix_cputimers.expiries expiration cache if * necessary. Needs siglock protection since other code may update * expiration cache as well. */
@@ -125,19 +125,9 @@ static void bump_cpu_timer(struct k_itim } } -/** -* task_cputime_zero - Check a task_cputime struct for all zero fields. -* -* @cputime: The struct to compare. -* -* Checks @cputime to see if all fields are zero. Returns true if all fields -* are zero, false if any field is nonzero. -*/ -static inline int task_cputime_zero (const struct task_cputime *cputime) +static inline bool expiry_cache_is_zero (const u64 *ec) { -if (!cputime->utime && !cputime->stime && !cputime->sum_exec_runtime) -return 1 ; -return 0 ; +return !(ec[CPUCLOCK_PROF ] | ec[CPUCLOCK_VIRT ] | ec[CPUCLOCK_SCHED ]); } static int
@@ -804,10 +794,10 @@ static void check_thread_timers(struct tcheck_dl_overrun (tsk);/* -* If cputime_expires is zero, then there are no active -* per thread CPU timers. +* If the expiry cache is zero, then there are no active per thread +* CPU timers. */ -if (task_cputime_zero (&tsk->posix_cputimers.cputime_expires)) +if (expiry_cache_is_zero (tsk->posix_cputimers.expiries))return ;task_cputime (tsk, &utime, &stime);
@@ -853,7 +843,7 @@ static void check_thread_timers(struct t } } -if (task_cputime_zero (&tsk->posix_cputimers.cputime_expires)) +if (expiry_cache_is_zero (tsk->posix_cputimers.expiries))tick_dep_clear_task (tsk,TICK_DEP_BIT_POSIX_TIMER ); }
@@ -976,7 +966,7 @@ static void check_process_timers(struct sig->posix_cputimers.expiries[CPUCLOCK_VIRT ] = virt_expires; sig->posix_cputimers.expiries[CPUCLOCK_SCHED ] = sched_expires; -if (task_cputime_zero (&sig->posix_cputimers.cputime_expires)) +if (expiry_cache_is_zero (sig->posix_cputimers.expiries))stop_process_timers (sig); sig->cputimer.checking_timer = false;
@@ -1041,26 +1031,23 @@ static void posix_cpu_timer_rearm(struct }/** -* task_cputime_expired - Compare two task_cputime entities. +* task_cputimers_expired - Compare two task_cputime entities. * -* @sample: The task_cputime structure to be checked for expiration. -* @expires: Expiration times, against which @sample will be checked. +* @samples: Array of current samples for the CPUCLOCK clocks +* @expiries: Array of expiry values for the CPUCLOCK clocks * -* Checks @sample against @expires to see if any field of @sample has expired. -* Returns true if any field of the former is greater than the corresponding -* field of the latter if the latter field is set. Otherwise returns false. +* Returns true if any mmember of @samples is greater than the corresponding +* member of @expiries if that member is non zero. False otherwise */ -static inline int task_cputime_expired (const struct task_cputime *sample, -const struct task_cputime *expires) +static inline bool task_cputimers_expired (const u64 *sample,const u64 *expiries) { -if (expires->utime && sample->utime >= expires->utime) -return 1 ; -if (expires->stime && sample->utime + sample->stime >= expires->stime) -return 1 ; -if (expires->sum_exec_runtime !=0 && - sample->sum_exec_runtime >= expires->sum_exec_runtime) -return 1 ; -return 0 ; +int i; + +for (i =0 ; i <CPUCLOCK_MAX ; i++) { +if (expiries[i] && sample[i] >= expiries[i]) +return true; + } +return false; }/**
@@ -1073,18 +1060,17 @@ static inline int task_cputime_expired(c* timers and compare them with the corresponding expiration times. Return * true if a timer has expired, else return false. */ -static inline int fastpath_timer_check (struct task_struct *tsk) +static inline bool fastpath_timer_check (struct task_struct *tsk) { +u64 *expiries = tsk->posix_cputimers.expiries;struct signal_struct *sig; -if (!task_cputime_zero (&tsk->posix_cputimers.cputime_expires)) { -struct task_cputime task_sample; +if (!expiry_cache_is_zero (expiries)) { +u64 samples[CPUCLOCK_MAX ]; -task_cputime (tsk, &task_sample.utime, &task_sample.stime); - task_sample.sum_exec_runtime = tsk->se.sum_exec_runtime; -if (task_cputime_expired (&task_sample, - &tsk->posix_cputimers.cputime_expires)) -return 1 ; +task_sample_cputime (tsk, samples); +if (task_cputimers_expired (samples, expiries)) +return true; } sig = tsk->signal;
@@ -1104,19 +1090,20 @@ static inline int fastpath_timer_check(s*/ if (READ_ONCE (sig->cputimer.running) && !READ_ONCE (sig->cputimer.checking_timer)) { -struct task_cputime group_sample; +u64 samples[CPUCLOCK_MAX ]; -sample_cputime_atomic (&group_sample, &sig->cputimer.cputime_atomic); +proc_sample_cputime_atomic (&sig->cputimer.cputime_atomic, + samples); -if (task_cputime_expired (&group_sample, - &sig->posix_cputimers.cputime_expires)) -return 1 ; +if (task_cputimers_expired (samples, + sig->posix_cputimers.expiries)) +return true; }if (dl_task (tsk) && tsk->dl.dl_overrun) -return 1 ; +return true; -return 0 ; +return false; }/*
PATCH 34/44 posix-cpu-timers: Remove cputime_expires
In reply to Thomas GleixnerThe last users of the magic struct cputime based expiry cache are gone. Remove the leftovers.
--- include/linux/posix-timers.h | 9 ++------- kernel/time/posix-cpu-timers.c | 10 ---------- 2 files changed, 2 insertions(+), 17 deletions(-)
include/linux/posix-timers.h +2 -7
@@ -65,19 +65,14 @@ static inline int clockid_to_fd(const cl#ifdef CONFIG_POSIX_TIMERS /** * posix_cputimers - Container for posix CPU timer related data -* @cputime_expires: Earliest-expiration cache task_cputime based * @expiries: Earliest-expiration cache array based * @cpu_timers: List heads to queue posix CPU timers * * Used in task_struct and signal_struct */ struct posix_cputimers { -/* Temporary union until all users are cleaned up */ -union { -struct task_cputime cputime_expires; -u64 expiries[CPUCLOCK_MAX ]; - }; -struct list_head cpu_timers[CPUCLOCK_MAX ]; +u64 expiries[CPUCLOCK_MAX ]; +struct list_head cpu_timers[CPUCLOCK_MAX ]; };static inline void posix_cputimers_init (struct posix_cputimers *pct)
kernel/time/posix-cpu-timers.c +0 -10
@@ -18,16 +18,6 @@#include "posix-timers.h" -static inline void temporary_check (void ) -{ -BUILD_BUG_ON (offsetof(struct task_cputime , stime) != -CPUCLOCK_PROF *sizeof (u64)); -BUILD_BUG_ON (offsetof(struct task_cputime , utime) != -CPUCLOCK_VIRT *sizeof (u64)); -BUILD_BUG_ON (offsetof(struct task_cputime , sum_exec_runtime) != -CPUCLOCK_SCHED *sizeof (u64)); -} -static void posix_cpu_timer_rearm (struct k_itimer *timer);/*
PATCH 35/44 posix-cpu-timers: Switch thread group sampling to array
In reply to Thomas GleixnerThat allows more simplifications in various places.
--- include/linux/sched/cputime.h | 2 kernel/time/itimer.c | 11 +--- kernel/time/posix-cpu-timers.c | 96 +++++++++++++++++------------------------ 3 files changed, 45 insertions(+), 64 deletions(-)
include/linux/sched/cputime.h +1 -1
@@ -61,7 +61,7 @@ extern void cputime_adjust(struct task_c* Thread group CPU time accounting. */ void thread_group_cputime (struct task_struct *tsk,struct task_cputime *times); -void thread_group_sample_cputime (struct task_struct *tsk,struct task_cputime *times); +void thread_group_sample_cputime (struct task_struct *tsk,u64 *samples);/* * The following are functions that support scheduler-internal time accounting.
kernel/time/itimer.c +3 -8
@@ -55,15 +55,10 @@ static void get_cpu_itimer(struct task_s val = it->expires; interval = it->incr;if (val) { -struct task_cputime cputime; -u64 t; +u64 t, samples[CPUCLOCK_MAX ]; -thread_group_sample_cputime (tsk, &cputime); -if (clock_id ==CPUCLOCK_PROF ) - t = cputime.utime + cputime.stime; -else -/* CPUCLOCK_VIRT */ - t = cputime.utime; +thread_group_sample_cputime (tsk, samples); + t = samples[clock_id];if (val < t)/* about to fire */
kernel/time/posix-cpu-timers.c +41 -55
181 lines not on this page · raw message
PATCH 36/44 posix-cpu-timers: Get rid of zero checks
In reply to Thomas GleixnerDeactivation of the expiry cache is done by setting all clock caches to
0. That requires to have a check for zero in all places which update the expiry cache:
if (cache == 0 || new < cache) cache = new;
Use U64_MAX as the deactivated value, which allows to remove the zero checks when updating the cache and reduces it to the obvious check:
if (new < cache) cache = new;
--- include/linux/posix-timers.h | 4 +++- kernel/time/posix-cpu-timers.c | 36 ++++++++++++++---------------------- 2 files changed, 17 insertions(+), 23 deletions(-)
include/linux/posix-timers.h +3 -1
@@ -77,7 +77,7 @@ struct posix_cputimers {static inline void posix_cputimers_init (struct posix_cputimers *pct) { -memset (&pct->expiries,0 ,sizeof (pct->expiries)); +memset (&pct->expiries,0xff ,sizeof (pct->expiries));INIT_LIST_HEAD (&pct->cpu_timers[0 ]);INIT_LIST_HEAD (&pct->cpu_timers[1 ]);INIT_LIST_HEAD (&pct->cpu_timers[2 ]);
@@ -106,8 +106,10 @@ static inline void posix_cputimers_rt_wa#define INIT_CPU_TIMERS (s) \ .posix_cputimers = { \ + .expiries = { U64_MAX, U64_MAX, U64_MAX }, \ .cpu_timers = INIT_CPU_TIMERLISTS(s.posix_cputimers), \ }, +#else struct posix_cputimers { };#define INIT_CPU_TIMERS (s)
kernel/time/posix-cpu-timers.c +14 -22
@@ -115,9 +115,10 @@ static void bump_cpu_timer(struct k_itim } } -static inline bool expiry_cache_is_zero (const u64 *ec) +/* Check whether all cache entries contain U64_MAX, i.e. eternal expiry time */ +static inline bool expiry_cache_is_inactive (const u64 *ec) { -return !(ec[CPUCLOCK_PROF ] | ec[CPUCLOCK_VIRT ] | ec[CPUCLOCK_SCHED ]); +return !(~ec[CPUCLOCK_PROF ] | ~ec[CPUCLOCK_VIRT ] | ~ec[CPUCLOCK_SCHED ]); } static int
@@ -434,11 +435,6 @@ void posix_cpu_timers_exit_group(structcleanup_timers (&tsk->signal->posix_cputimers); } -static inline int expires_gt (u64 expires,u64 new_exp) -{ -return expires ==0 || expires > new_exp; -} -/* * Insert the timer on the appropriate list before any timers that * expire later. This must be called with the sighand lock held.
@@ -477,7 +473,7 @@ static void arm_timer(struct k_itimer *t* for process timers we share expiration cache with itimers * and RLIMIT_CPU and for thread timers with RLIMIT_RTTIME. */ -if (expires_gt (*cpuexp, newexp)) +if (newexp < *cpuexp) *cpuexp = newexp;if (CPUCLOCK_PERTHREAD (timer->it_clock))
@@ -747,7 +743,7 @@ check_timers_list(struct list_head *timelist_move_tail (&t->entry, firing); } -return 0 ; +return U64_MAX ; }static inline void check_dl_overrun (struct task_struct *tsk)
@@ -773,11 +769,7 @@ static void check_thread_timers(struct tif (dl_task (tsk))check_dl_overrun (tsk); -/* -* If the expiry cache is zero, then there are no active per thread -* CPU timers. -*/ -if (expiry_cache_is_zero (tsk->posix_cputimers.expiries)) +if (expiry_cache_is_inactive (tsk->posix_cputimers.expiries))return ;task_cputime (tsk, &utime, &stime);
@@ -823,7 +815,7 @@ static void check_thread_timers(struct t } } -if (expiry_cache_is_zero (tsk->posix_cputimers.expiries)) +if (expiry_cache_is_inactive (tsk->posix_cputimers.expiries))tick_dep_clear_task (tsk,TICK_DEP_BIT_POSIX_TIMER ); }
@@ -854,7 +846,7 @@ static void check_cpu_itimer(struct task__group_send_sig_info (signo,SEND_SIG_PRIV , tsk); } -if (it->expires && (!*expires || it->expires < *expires)) +if (it->expires && it->expires < *expires) *expires = it->expires; }
@@ -935,14 +927,14 @@ static void check_process_timers(struct } } softns = soft *NSEC_PER_SEC ; -if (!prof_exp || softns < prof_exp) +if (softns < prof_exp) prof_exp = softns; }store_samples (sig->posix_cputimers.expiries, prof_exp, virt_exp, sched_exp); -if (expiry_cache_is_zero (sig->posix_cputimers.expiries)) +if (expiry_cache_is_inactive (sig->posix_cputimers.expiries))stop_process_timers (sig); sig->cputimer.checking_timer = false;
@@ -1013,14 +1005,14 @@ static void posix_cpu_timer_rearm(struct* @expiries: Array of expiry values for the CPUCLOCK clocks * * Returns true if any mmember of @samples is greater than the corresponding -* member of @expiries if that member is non zero. False otherwise +* member of @expiries. False otherwise */ static inline bool task_cputimers_expired (const u64 *sample,const u64 *expiries) {int i;for (i =0 ; i <CPUCLOCK_MAX ; i++) { -if (expiries[i] && sample[i] >= expiries[i]) +if (sample[i] >= expiries[i])return true; }return false;
@@ -1041,7 +1033,7 @@ static inline bool fastpath_timer_check(u64 *expiries = tsk->posix_cputimers.expiries;struct signal_struct *sig; -if (!expiry_cache_is_zero (expiries)) { +if (!expiry_cache_is_inactive (expiries)) {u64 samples[CPUCLOCK_MAX ];task_sample_cputime (tsk, samples);
@@ -1200,7 +1192,7 @@ void set_process_cpu_timer(struct task_s* Update expiration cache if this is the earliest timer. CPUCLOCK_PROF * expiry cache is also used by RLIMIT_CPU!. */ -if (expires_gt (*expiry, *newval)) +if (*newval < *expiry) *expiry = *newval;tick_dep_set_signal (tsk->signal,TICK_DEP_BIT_POSIX_TIMER );
PATCH 37/44 posix-cpu-timers: Consolidate timer expiry further
In reply to Thomas GleixnerWith the array based samples and expiry cache, the expiry function can use a loop to collect timers from the clock specific lists.
--- kernel/time/posix-cpu-timers.c | 48 +++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 23 deletions(-)
kernel/time/posix-cpu-timers.c +25 -23
@@ -746,6 +746,17 @@ check_timers_list(struct list_head *timereturn U64_MAX ; } +static void collect_posix_cputimers (struct posix_cputimers *pct, +u64 *samples,struct list_head *firing) +{ +struct list_head *timers = pct->cpu_timers; +u64 *expiries = pct->expiries; +int i; + +for (i =0 ; i <CPUCLOCK_MAX ; i++, timers++) + expiries[i] =check_timers_list (timers, firing, samples[i]); +} +static inline void check_dl_overrun (struct task_struct *tsk) {if (tsk->dl.dl_overrun) {
@@ -762,21 +773,18 @@ static inline void check_dl_overrun(strustatic void check_thread_timers (struct task_struct *tsk,struct list_head *firing) { -struct list_head *timers = tsk->posix_cputimers.cpu_timers; -u64 stime, utime, *expires = tsk->posix_cputimers.expiries; +struct posix_cputimers *pct = &tsk->posix_cputimers; +u64 samples[CPUCLOCK_MAX ];unsigned long soft;if (dl_task (tsk))check_dl_overrun (tsk); -if (expiry_cache_is_inactive (tsk->posix_cputimers.expiries)) +if (expiry_cache_is_inactive (pct->expiries))return ; -task_cputime (tsk, &utime, &stime); - - *expires++ =check_timers_list (timers, firing, utime + stime); - *expires++ =check_timers_list (++timers, firing, utime); - *expires =check_timers_list (++timers, firing, tsk->se.sum_exec_runtime); +task_sample_cputime (tsk, samples); +collect_posix_cputimers (pct, samples, firing);/* * Check for the special case thread timers.
@@ -815,7 +823,7 @@ static void check_thread_timers(struct t } } -if (expiry_cache_is_inactive (tsk->posix_cputimers.expiries)) +if (expiry_cache_is_inactive (pct->expiries))tick_dep_clear_task (tsk,TICK_DEP_BIT_POSIX_TIMER ); }
@@ -859,15 +867,15 @@ static void check_process_timers(struct struct list_head *firing) {struct signal_struct *const sig = tsk->signal; -struct list_head *timers = sig->posix_cputimers.cpu_timers; -u64 virt_exp, prof_exp, sched_exp, samples[CPUCLOCK_MAX ]; +struct posix_cputimers *pct = &sig->posix_cputimers; +u64 samples[CPUCLOCK_MAX ], *exp = pct->expiries;unsigned long soft;/* * If cputimer is not running, then there are no active * process wide timers (POSIX 1.b, itimers, RLIMIT_CPU). */ -if (!READ_ONCE (tsk->signal->cputimer.running)) +if (!READ_ONCE (sig->cputimer.running))return ;/*
@@ -881,17 +889,14 @@ static void check_process_timers(struct* so the sample can be taken directly. */ proc_sample_cputime_atomic (&sig->cputimer.cputime_atomic, samples); - - prof_exp =check_timers_list (timers, firing, samples[CPUCLOCK_PROF ]); - virt_exp =check_timers_list (++timers, firing, samples[CPUCLOCK_VIRT ]); - sched_exp =check_timers_list (++timers, firing, samples[CPUCLOCK_SCHED ]); +collect_posix_cputimers (pct, samples, firing);/* * Check for the special case process timers. */ -check_cpu_itimer (tsk, &sig->it[CPUCLOCK_PROF ], &prof_exp, +check_cpu_itimer (tsk, &sig->it[CPUCLOCK_PROF ], &exp[CPUCLOCK_PROF ], samples[CPUCLOCK_PROF ],SIGPROF ); -check_cpu_itimer (tsk, &sig->it[CPUCLOCK_VIRT ], &virt_exp, +check_cpu_itimer (tsk, &sig->it[CPUCLOCK_VIRT ], &exp[CPUCLOCK_VIRT ], samples[CPUCLOCK_VIRT ],SIGVTALRM ); soft =task_rlimit (tsk,RLIMIT_CPU );
@@ -927,13 +932,10 @@ static void check_process_timers(struct } } softns = soft *NSEC_PER_SEC ; -if (softns < prof_exp) - prof_exp = softns; +if (softns < exp[CPUCLOCK_PROF ]) + exp[CPUCLOCK_PROF ] = softns; } -store_samples (sig->posix_cputimers.expiries, prof_exp, virt_exp, - sched_exp); -if (expiry_cache_is_inactive (sig->posix_cputimers.expiries))stop_process_timers (sig);
PATCH 38/44 posix-cpu-timers: Respect INFINITY for hard RTTIME limit
In reply to Thomas GleixnerThe RTIME limit expiry code does not check the hard RTTIME limit for INFINITY, i.e. being disabled. Add it.
--- kernel/time/posix-cpu-timers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/time/posix-cpu-timers.c +1 -1
@@ -905,7 +905,7 @@ static void check_process_timers(structu64 softns, ptime = samples[CPUCLOCK_PROF ];unsigned long psecs =div_u64 (ptime,NSEC_PER_SEC ); -if (psecs >= hard) { +if (hard !=RLIM_INFINITY && psecs >= hard) {/* * At the hard limit, we just die. * No need to calculate anything else now.
Re: [patch 38/44] posix-cpu-timers: Respect INFINITY for hard RTTIME limit
Quoted 11 lines · Thomas Gleixner
The RTIME limit expiry code does not check the hard RTTIME limit for INFINITY, i.e. being disabled. Add it.
Signed-off-by Thomas Gleixner <[email protected]>--- kernel/time/posix-cpu-timers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/time/posix-cpu-timers.c static void check_process_timers(struct · line 911
Quoted 8 lines
kernel/time/posix-cpu-timers.c +1
-1@@ -905,7 +905,7 @@ static void check_process_timers(structu64 softns, ptime = samples[CPUCLOCK_PROF ];unsigned long psecs =div_u64 (ptime,NSEC_PER_SEC ); -if (psecs >= hard) { +if (hard !=RLIM_INFINITY && psecs >= hard) {/* * At the hard limit, we just die. * No need to calculate anything else now.
Might make sense to mark this as a possible ABI change in the changelog:
if some weird code learned to rely on this (arguably broken) behavior then the bug turned into an ABI.
Thanks,
Ingo
Re: [patch 38/44] posix-cpu-timers: Respect INFINITY for hard RTTIME limit
Quoted 16 lines · Ingo Molnar
Quoted 14 lines · Thomas Gleixner
The RTIME limit expiry code does not check the hard RTTIME limit for INFINITY, i.e. being disabled. Add it.
Signed-off-by Thomas Gleixner <[email protected]>--- kernel/time/posix-cpu-timers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/time/posix-cpu-timers.c +0
-0@@ -905,7 +905,7 @@ static void check_process_timers(structu64 softns, ptime = samples[CPUCLOCK_PROF ];unsigned long psecs =div_u64 (ptime,NSEC_PER_SEC );
Quoted 9 lines
kernel/time/posix-cpu-timers.c static void check_process_timers(struct · line 911
Quoted 5 lines
kernel/time/posix-cpu-timers.c +1
-1-if (psecs >= hard) { +if (hard !=RLIM_INFINITY && psecs >= hard) {/* * At the hard limit, we just die. * No need to calculate anything else now. Might make sense to mark this as a possible ABI change in the changelog:
if some weird code learned to rely on this (arguably broken) behavior then the bug turned into an ABI.
Will do, though that would be really interesting to see the offending case. That limit is in seconds and RLIM_INFINITY is at least INT_MAX which means 68 years. :)
I stumbled over it when I tried to consolidate that duplicated code in that area. I'll amend the changelog to be more clear.
Thanks,
tglx
PATCH 39/44 posix-cpu-timers: Get rid of 64bit divisions
In reply to Thomas GleixnerInstead of dividing A to match the units of B it's more efficient to multiply B to match the units of A.
--- kernel/time/posix-cpu-timers.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
kernel/time/posix-cpu-timers.c +12 -8
@@ -791,10 +791,11 @@ static void check_thread_timers(struct t*/ soft =task_rlimit (tsk,RLIMIT_RTTIME );if (soft !=RLIM_INFINITY ) { +/* Task RT timeout is accounted in jiffies. RTTIME is usec */ +unsigned long rtim = tsk->rt.timeout * (USEC_PER_SEC /HZ );unsigned long hard =task_rlimit_max (tsk,RLIMIT_RTTIME ); -if (hard !=RLIM_INFINITY && - tsk->rt.timeout >DIV_ROUND_UP (hard,USEC_PER_SEC /HZ )) { +if (hard !=RLIM_INFINITY && rtim >= hard) {/* * At the hard limit, we just die. * No need to calculate anything else now.
@@ -806,7 +807,7 @@ static void check_thread_timers(struct t__group_send_sig_info (SIGKILL ,SEND_SIG_PRIV , tsk);return ; } -if (tsk->rt.timeout >DIV_ROUND_UP (soft,USEC_PER_SEC /HZ )) { +if (rtim >= soft) {/* * At the soft limit, send a SIGXCPU every second. */
@@ -901,11 +902,13 @@ static void check_process_timers(struct soft =task_rlimit (tsk,RLIMIT_CPU );if (soft !=RLIM_INFINITY ) { +/* RLIMIT_CPU is in seconds. Samples are nanoseconds */ unsigned long hard =task_rlimit_max (tsk,RLIMIT_CPU ); -u64 softns, ptime = samples[CPUCLOCK_PROF ]; -unsigned long psecs =div_u64 (ptime,NSEC_PER_SEC ); +u64 ptime = samples[CPUCLOCK_PROF ]; +u64 softns = (u64 )soft *NSEC_PER_SEC ; +u64 hardns = (u64 )hard *NSEC_PER_SEC ; -if (hard !=RLIM_INFINITY && psecs >= hard) { +if (hard !=RLIM_INFINITY && ptime >= hardns) {/* * At the hard limit, we just die. * No need to calculate anything else now.
@@ -917,7 +920,7 @@ static void check_process_timers(struct__group_send_sig_info (SIGKILL ,SEND_SIG_PRIV , tsk);return ; } -if (psecs >= soft) { +if (ptime >= softns) {/* * At the soft limit, send a SIGXCPU every second. */
@@ -931,7 +934,8 @@ static void check_process_timers(struct sig->rlim[RLIMIT_CPU ].rlim_cur = soft; } } - softns = soft *NSEC_PER_SEC ; + +/* Update the expiry cache */ if (softns < exp[CPUCLOCK_PROF ]) exp[CPUCLOCK_PROF ] = softns; }
PATCH 40/44 posix-cpu-timers: Remove pointless comparisions
In reply to Thomas GleixnerThe soft RLIMIT expiry code checks whether the soft limit is greater than the hard limit. That's pointless because if the soft RLIMIT is greater than the hard RLIMIT then that code cannot be reached as the hard RLIMIT check is before that and already killed the process.
Remove it.
--- kernel/time/posix-cpu-timers.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
kernel/time/posix-cpu-timers.c +7 -9
@@ -807,15 +807,14 @@ static void check_thread_timers(struct t__group_send_sig_info (SIGKILL ,SEND_SIG_PRIV , tsk);return ; } +if (rtim >= soft) {/* * At the soft limit, send a SIGXCPU every second. */ -if (soft < hard) { - soft +=USEC_PER_SEC ; - tsk->signal->rlim[RLIMIT_RTTIME ].rlim_cur = - soft; - } + soft +=USEC_PER_SEC ; + tsk->signal->rlim[RLIMIT_RTTIME ].rlim_cur = soft; +if (print_fatal_signals) {pr_info ("RT Watchdog Timeout (soft): %s[%d]\n" , tsk->comm,task_pid_nr (tsk));
@@ -929,10 +928,9 @@ static void check_process_timers(struct tsk->comm, task_pid_nr(tsk)); }__group_send_sig_info (SIGXCPU ,SEND_SIG_PRIV , tsk); -if (soft < hard) { - soft++; - sig->rlim[RLIMIT_CPU ].rlim_cur = soft; - } + + soft++; + sig->rlim[RLIMIT_CPU ].rlim_cur = soft; }/* Update the expiry cache */
Re: [patch 40/44] posix-cpu-timers: Remove pointless comparisions
title:
s/comparisions
/comparisons
Thanks,
Ingo
PATCH 41/44 posix-cpu-timers: Deduplicate rlimit handling
In reply to Thomas GleixnerBoth thread and process expiry functions have the same functionality for sending signals for soft and hard RLIMITs duplicated in 4 different ways.
Split it out into a common function and cleanup the callsites.
--- kernel/time/posix-cpu-timers.c | 71 +++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 44 deletions(-)
kernel/time/posix-cpu-timers.c +27 -44
@@ -765,6 +765,20 @@ static inline void check_dl_overrun(stru } } +static bool check_rlimit (u64 time,u64 limit,int signo,bool rt,bool hard) +{ +if (time < limit) +return false; + +if (print_fatal_signals) { +pr_info ("%s Watchdog Timeout (%s): %s[%d]\n" , + rt ?"RT" :"CPU" , hard ?"hard" :"soft" , + current->comm,task_pid_nr (current)); + } +__group_send_sig_info (signo,SEND_SIG_PRIV , current); +return true; +} +/* * Check for any per-thread CPU timers that have fired and move them off * the tsk->cpu_timers[N] list onto the firing list. Here we update the
@@ -792,34 +806,18 @@ static void check_thread_timers(struct t soft =task_rlimit (tsk,RLIMIT_RTTIME );if (soft !=RLIM_INFINITY ) {/* Task RT timeout is accounted in jiffies. RTTIME is usec */ -unsigned long rtim = tsk->rt.timeout * (USEC_PER_SEC /HZ ); +unsigned long rttime = tsk->rt.timeout * (USEC_PER_SEC /HZ );unsigned long hard =task_rlimit_max (tsk,RLIMIT_RTTIME ); -if (hard !=RLIM_INFINITY && rtim >= hard) { -/* -* At the hard limit, we just die. -* No need to calculate anything else now. -*/ -if (print_fatal_signals) { -pr_info ("CPU Watchdog Timeout (hard): %s[%d]\n" , - tsk->comm,task_pid_nr (tsk)); - } -__group_send_sig_info (SIGKILL ,SEND_SIG_PRIV , tsk); +/* At the hard limit, send SIGKILL. No further action. */ +if (hard !=RLIM_INFINITY && +check_rlimit (rttime, hard,SIGKILL , true, true))return ; - } -if (rtim >= soft) { -/* -* At the soft limit, send a SIGXCPU every second. -*/ +/* At the soft limit, send a SIGXCPU every second */ +if (check_rlimit (rttime, soft,SIGXCPU , true, false)) { soft +=USEC_PER_SEC ; tsk->signal->rlim[RLIMIT_RTTIME ].rlim_cur = soft; - -if (print_fatal_signals) { -pr_info ("RT Watchdog Timeout (soft): %s[%d]\n" , - tsk->comm,task_pid_nr (tsk)); - } -__group_send_sig_info (SIGXCPU ,SEND_SIG_PRIV , tsk); } }
@@ -904,31 +902,16 @@ static void check_process_timers(struct/* RLIMIT_CPU is in seconds. Samples are nanoseconds */ unsigned long hard =task_rlimit_max (tsk,RLIMIT_CPU );u64 ptime = samples[CPUCLOCK_PROF ]; -u64 softns = (u64 )soft *NSEC_PER_SEC ; -u64 hardns = (u64 )hard *NSEC_PER_SEC ; +u64 softns = soft *NSEC_PER_SEC ; +u64 hardns = hard *NSEC_PER_SEC ; -if (hard !=RLIM_INFINITY && ptime >= hardns) { -/* -* At the hard limit, we just die. -* No need to calculate anything else now. -*/ -if (print_fatal_signals) { -pr_info ("RT Watchdog Timeout (hard): %s[%d]\n" , - tsk->comm,task_pid_nr (tsk)); - } -__group_send_sig_info (SIGKILL ,SEND_SIG_PRIV , tsk); +/* At the hard limit, send SIGKILL. No further action. */ +if (hard !=RLIM_INFINITY && +check_rlimit (ptime, hardns,SIGKILL , false, true))return ; - } -if (ptime >= softns) { -/* -* At the soft limit, send a SIGXCPU every second. -*/ -if (print_fatal_signals) { -pr_info ("CPU Watchdog Timeout (soft): %s[%d]\n" , - tsk->comm,task_pid_nr (tsk)); - } -__group_send_sig_info (SIGXCPU ,SEND_SIG_PRIV , tsk); +/* At the soft limit, send a SIGXCPU every second */ +if (check_rlimit (ptime, softns,SIGXCPU , false, false)) { soft++; sig->rlim[RLIMIT_CPU ].rlim_cur = soft; }
PATCH 42/44 posix-cpu-timers: Move state tracking to struct posix_cputimers
In reply to Thomas GleixnerPut it where it belongs and cleanup the ifdeffery in fork completely.
--- include/linux/posix-timers.h | 12 ++++++- include/linux/sched/cputime.h | 9 +++-- include/linux/sched/signal.h | 6 --- init/init_task.c | 2 - kernel/fork.c | 6 --- kernel/time/posix-cpu-timers.c | 68 +++++++++++++++++++++-------------------- 6 files changed, 53 insertions(+), 50 deletions(-)
include/linux/posix-timers.h +11 -1
@@ -66,18 +66,26 @@ static inline int clockid_to_fd(const cl/** * posix_cputimers - Container for posix CPU timer related data * @expiries: Earliest-expiration cache array based +* @timers_active: Timers are queued. +* @expiry_active: Timer expiry is active. Used for +* process wide timers to avoid multiple +* trying to handle expiry * @cpu_timers: List heads to queue posix CPU timers * * Used in task_struct and signal_struct */ struct posix_cputimers {u64 expiries[CPUCLOCK_MAX ]; +unsigned int timers_active; +unsigned int expiry_active;struct list_head cpu_timers[CPUCLOCK_MAX ]; };static inline void posix_cputimers_init (struct posix_cputimers *pct) {memset (&pct->expiries,0xff ,sizeof (pct->expiries)); + pct->timers_active =0 ; + pct->expiry_active =0 ;INIT_LIST_HEAD (&pct->cpu_timers[0 ]);INIT_LIST_HEAD (&pct->cpu_timers[1 ]);INIT_LIST_HEAD (&pct->cpu_timers[2 ]);
@@ -87,8 +95,10 @@ static inline void posix_cputimers_group u64 cpu_limit) {posix_cputimers_init (pct); -if (cpu_limit !=RLIM_INFINITY ) +if (cpu_limit !=RLIM_INFINITY ) { pct->expiries[CPUCLOCK_PROF ] = cpu_limit *NSEC_PER_SEC ; + pct->timers_active = true; + } } static inline void posix_cputimers_rt_watchdog(struct posix_cputimers *pct,
include/linux/sched/cputime.h +6 -3
@@ -70,7 +70,7 @@ void thread_group_sample_cputime(struct*/ /** -* get_running_cputimer - return &tsk->signal->cputimer if cputimer is running +* get_running_cputimer - return &tsk->signal->cputimer if cputimers are active * * @tsk: Pointer to target task. */
@@ -80,8 +80,11 @@ struct thread_group_cputimer *get_runnin {struct thread_group_cputimer *cputimer = &tsk->signal->cputimer; -/* Check if cputimer isn't running. This is accessed without locking. */ -if (!READ_ONCE (cputimer->running)) +/* +* Check whether posix cpu timers are active. If not the thread +* group accounting is not active either. Lockless check. +*/ +if (!READ_ONCE (tsk->signal->posix_cputimers.timers_active))return NULL ;/*
include/linux/sched/signal.h +0 -6
@@ -57,18 +57,12 @@ struct task_cputime_atomic {/** * struct thread_group_cputimer - thread group interval timer counts * @cputime_atomic: atomic thread group interval timers. -* @running: true when there are timers running and -* @cputime_atomic receives updates. -* @checking_timer: true when a thread in the group is in the -* process of checking for thread group timers. * * This structure contains the version of task_cputime, above, that is * used for thread group CPU timer calculations. */ struct thread_group_cputimer {struct task_cputime_atomic cputime_atomic; -bool running; -bool checking_timer; };struct multiprocess_signals {
init/init_task.c +0 -2
@@ -30,8 +30,6 @@ static struct signal_struct init_signals .posix_timers = LIST_HEAD_INIT(init_signals.posix_timers), .cputimer = { .cputime_atomic = INIT_CPUTIME_ATOMIC, - .running = false, - .checking_timer = false, },#endif INIT_CPU_TIMERS (init_signals)
kernel/fork.c +0 -6
@@ -1517,7 +1517,6 @@ void __cleanup_sighand(struct sighand_st } } -#ifdef CONFIG_POSIX_TIMERS /* * Initialize POSIX timer handling for a thread group. */
@@ -1528,12 +1527,7 @@ static void posix_cpu_timers_init_group( cpu_limit =READ_ONCE (sig->rlim[RLIMIT_CPU ].rlim_cur);posix_cputimers_group_init (pct, cpu_limit); -if (cpu_limit !=RLIM_INFINITY ) - sig->cputimer.running = true; } -#else -static inline void posix_cpu_timers_init_group (struct signal_struct *sig) { } -#endif static int copy_signal (unsigned long clone_flags,struct task_struct *tsk) {
kernel/time/posix-cpu-timers.c +36 -32
145 lines not on this page · raw message
Re: [patch 42/44] posix-cpu-timers: Move state tracking to struct posix_cputimers
Quoted 1 line · Thomas Gleixner
Put it where it belongs and cleanup the ifdeffery in fork completely.
s/cleanup
/clean up
Quoted 6 lines
*
posix_cputimers- Container for posix CPU timer related data
* @expiries: Earliest-expiration cache array based
+ * @timers_active: Timers are queued.
+ * @expiry_active: Timer expiry is active. Used for
+ * process wide timers to avoid multiple
+ * trying to handle expiry
'to avoid multiple trying to handle expiry'?
Should this be: 'to avoid multiple tasks trying to handle expiry'?
include/linux/sched/cputime.h struct thread_group_cputimer *get_runnin · line 85
Quoted 2 lines
include/linux/sched/cputime.h +2
-0+* Check whether posix cpu timers are active. If not the thread +* group accounting is not active either. Lockless check.
s/cpu timers
/CPU timers
Thanks,
Ingo
Re: [patch 42/44] posix-cpu-timers: Move state tracking to struct posix_cputimers
Quoted 15 lines · Ingo Molnar
Quoted 1 line · Thomas Gleixner
Put it where it belongs and cleanup the ifdeffery in fork completely.
s/cleanup
/clean upQuoted 6 lines
*
posix_cputimers- Container for posix CPU timer related data
* @expiries: Earliest-expiration cache array based
+ * @timers_active: Timers are queued.
+ * @expiry_active: Timer expiry is active. Used for
+ * process wide timers to avoid multiple
+ * trying to handle expiry
Quoted 3 lines
'to avoid multiple trying to handle expiry'?
Should this be: 'to avoid multiple tasks trying to handle expiry'?
Yup.
PATCH 43/44 posix-cpu-timers: Utilize timerqueue for storage
In reply to Thomas GleixnerUsing a linear O(N) search for timer insertion affects execution time and Dcache footprint badly with a larger number of timers.
Switch the storage to a timerqueue which is already used for hrtimers and alarmtimers. It does not affect the size of struct k_itimer as it.alarm is still larger.
The extra list head for the expiry list will go away later.
--- include/linux/posix-timers.h | 77 ++++++++++++------ include/linux/timerqueue.h | 10 ++ kernel/time/posix-cpu-timers.c | 174 ++++++++++++++++++++--------------------- 3 files changed, 148 insertions(+), 113 deletions(-)
include/linux/posix-timers.h +51 -26
@@ -5,17 +5,11 @@#include <linux/spinlock.h> #include <linux/list.h> #include <linux/alarmtimer.h> +#include <linux/timerqueue.h> struct kernel_siginfo ;struct task_struct ; -struct cpu_timer_list { -struct list_head entry; -u64 expires; -struct task_struct *task; -int firing; -}; -/* * Bit fields within a clockid: *
@@ -63,6 +57,51 @@ static inline int clockid_to_fd(const cl }#ifdef CONFIG_POSIX_TIMERS + +/** +* cpu_timer - Posix CPU timer representation for k_itimer +* @node: timerqueue node to queue in the task/sig +* @head: timerqueue head on which this timer is queued +* @task: Pointer to target task +* @elist: List head for the expiry list +* @firing: Timer is currently firing +*/ +struct cpu_timer { +struct timerqueue_node node; +struct timerqueue_head *head; +struct task_struct *task; +struct list_head elist; +int firing; +}; + +static inline bool cpu_timer_requeue (struct cpu_timer *ctmr) +{ +return timerqueue_add (ctmr->head, &ctmr->node); +} + +static inline bool cpu_timer_enqueue (struct timerqueue_head *head, +struct cpu_timer *ctmr) +{ + ctmr->head = head; +return timerqueue_add (head, &ctmr->node); +} + +static inline void cpu_timer_dequeue (struct cpu_timer *ctmr) +{ +if (!RB_EMPTY_NODE (&ctmr->node.node)) +timerqueue_del (ctmr->head, &ctmr->node); +} + +static inline u64 cpu_timer_getexpires (struct cpu_timer *ctmr) +{ +return ctmr->node.expires; +} + +static inline void cpu_timer_setexpires (struct cpu_timer *ctmr,u64 exp) +{ + ctmr->node.expires = exp; +} +/** * posix_cputimers - Container for posix CPU timer related data * @expiries: Earliest-expiration cache array based
@@ -78,17 +117,14 @@ struct posix_cputimers {u64 expiries[CPUCLOCK_MAX ];unsigned int timers_active;unsigned int expiry_active; -struct list_head cpu_timers[CPUCLOCK_MAX ]; +struct timerqueue_head cpu_timers[CPUCLOCK_MAX ]; };static inline void posix_cputimers_init (struct posix_cputimers *pct) {memset (&pct->expiries,0xff ,sizeof (pct->expiries)); - pct->timers_active =0 ; - pct->expiry_active =0 ; -INIT_LIST_HEAD (&pct->cpu_timers[0 ]); -INIT_LIST_HEAD (&pct->cpu_timers[1 ]); -INIT_LIST_HEAD (&pct->cpu_timers[2 ]); +memset (&pct->timers_active,0x00 ,sizeof (*pct) - + offsetof(struct posix_cputimers , timers_active)); } static inline void posix_cputimers_group_init(struct posix_cputimers *pct,
@@ -108,18 +144,7 @@ static inline void posix_cputimers_rt_wa }/* Init task static initializer */ -#define INIT_CPU_TIMERLISTS (c) { \ - LIST_HEAD_INIT(c.cpu_timers[0]), \ - LIST_HEAD_INIT(c.cpu_timers[1]), \ - LIST_HEAD_INIT(c.cpu_timers[2]), \ -} - -#define INIT_CPU_TIMERS (s) \ - .posix_cputimers = { \ - .expiries = { U64_MAX, U64_MAX, U64_MAX }, \ - .cpu_timers = INIT_CPU_TIMERLISTS(s.posix_cputimers), \ - }, - +#define INIT_CPU_TIMERS (s) .posix_cputimers = { },#else struct posix_cputimers { };#define INIT_CPU_TIMERS (s)
@@ -176,7 +201,7 @@ struct k_itimer {struct {struct hrtimer timer; } real; -struct cpu_timer_list cpu; +struct cpu_timer cpu;struct {struct alarm alarmtimer; } alarm;
include/linux/timerqueue.h +10 -0
@@ -43,6 +43,16 @@ static inline void timerqueue_init(strucRB_CLEAR_NODE (&node->node); } +static inline bool timerqueue_node_queued (struct timerqueue_node *node) +{ +return !RB_EMPTY_NODE (&node->node); +} + +static inline bool timerqueue_node_expires (struct timerqueue_node *node) +{ +return node->expires; +} +static inline void timerqueue_init_head (struct timerqueue_head *head) { head->rb_root =RB_ROOT_CACHED ;
kernel/time/posix-cpu-timers.c +87 -87
380 lines not on this page · raw message
PATCH 44/44 posix-cpu-timers: Expire timers directly
In reply to Thomas GleixnerMoving the posix cpu timers from on list to another and then expiring them from the second list is avoiding to drop and reacquire sighand lock for each timer expiry, but on the other hand it's more complicated code and suboptimal for a small number of timers.
Remove the extra list and expire them directly from the rbtree. Tests with a large number of timers did not show a difference outside of the noise range.
This also allows to switch the crude heuristics of limiting the expiry of timers to 20 for each type to a time based limitation which is way more sensible.
--- include/linux/posix-timers.h | 2 kernel/time/posix-cpu-timers.c | 85 +++++++++++++---------------------------- 2 files changed, 29 insertions(+), 58 deletions(-)
include/linux/posix-timers.h +0 -2
@@ -63,14 +63,12 @@ static inline int clockid_to_fd(const cl* @node: timerqueue node to queue in the task/sig * @head: timerqueue head on which this timer is queued * @task: Pointer to target task -* @elist: List head for the expiry list * @firing: Timer is currently firing */ struct cpu_timer {struct timerqueue_node node;struct timerqueue_head *head;struct task_struct *task; -struct list_head elist;int firing; };
kernel/time/posix-cpu-timers.c +29 -56
155 lines not on this page · raw message
Re: [patch 44/44] posix-cpu-timers: Expire timers directly
Quoted 4 lines · Thomas Gleixner
Moving the posix cpu timers from on list to another and then expiring them from the second list is avoiding to drop and reacquire sighand lock for each timer expiry, but on the other hand it's more complicated code and suboptimal for a small number of timers.
s/on list
/one list
s/cpu timers
/CPU timers
Thanks,
Ingo
Re: [patch 44/44] posix-cpu-timers: Expire timers directly
In reply to Thomas GleixnerQuoted 5 lines · Thomas Gleixner
Moving the posix cpu timers from on list to another and then expiring them from the second list is avoiding to drop and reacquire sighand lock for each timer expiry, but on the other hand it's more complicated code and suboptimal for a small number of timers.
Quoted 7 lines
Remove the extra list and expire them directly from the rbtree. Tests with a large number of timers did not show a difference outside of the noise range.
This also allows to switch the crude heuristics of limiting the expiry of timers to 20 for each type to a time based limitation which is way more sensible.
This one is buggy. I know why so don't waste your time reviewing it.
Quoted 7 lines · Thomas Gleixner
The series applies on top of:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.gittimers/coreand is available from git as well:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.gitWIP.timers/core
It seems like this basically reverts the last patch in timers/core again. Do you really want to keep that or start with a better baseline?
Quoted 11 lines · Christoph Hellwig
Quoted 7 lines · Thomas Gleixner
The series applies on top of:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.gittimers/coreand is available from git as well:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.gitWIP.timers/coreIt seems like this basically reverts the last patch in timers/core again. Do you really want to keep that or start with a better baseline?
If the callback variant is agreed on, I'm going to zap that patch which it replaces.