Re: [patch V2 28/38] posix-cpu-timers: Restructure expiry array
On Mon, 26 Aug 2019, Frederic Weisbecker wrote:
> On Wed, Aug 21, 2019 at 09:09:15PM +0200, Thomas Gleixner wrote:
> > @@ -884,7 +888,7 @@ 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;
> > + struct posix_cputimer_base *base = sig->posix_cputimers.bases;
> > u64 utime, ptime, virt_expires, prof_expires;
> > u64 sum_sched_runtime, sched_expires;
> > struct task_cputime cputime;
> > @@ -912,9 +916,12 @@ static void check_process_timers(struct
> > ptime = utime + cputime.stime;
> > sum_sched_runtime = cputime.sum_exec_runtime;
> >
> > - prof_expires = check_timers_list(timers, firing, ptime);
> > - virt_expires = check_timers_list(++timers, firing, utime);
> > - sched_expires = check_timers_list(++timers, firing, sum_sched_runtime);
> > + prof_expires = check_timers_list([CPUCLOCK_PROF].cpu_timers,
> > +firing, ptime);
> > + virt_expires = check_timers_list([CPUCLOCK_VIRT].cpu_timers,
> > +firing, utime);
> > + sched_expires = check_timers_list([CLPCLOCK_SCHED].cpu_timers,
>
> ^^
> 0-day bot should have warned by now.
It didn't but my own testing found it and I fixed it locally already
Re: [patch V2 28/38] posix-cpu-timers: Restructure expiry array
On Wed, Aug 21, 2019 at 09:09:15PM +0200, Thomas Gleixner wrote:
> @@ -884,7 +888,7 @@ 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;
> + struct posix_cputimer_base *base = sig->posix_cputimers.bases;
> u64 utime, ptime, virt_expires, prof_expires;
> u64 sum_sched_runtime, sched_expires;
> struct task_cputime cputime;
> @@ -912,9 +916,12 @@ static void check_process_timers(struct
> ptime = utime + cputime.stime;
> sum_sched_runtime = cputime.sum_exec_runtime;
>
> - prof_expires = check_timers_list(timers, firing, ptime);
> - virt_expires = check_timers_list(++timers, firing, utime);
> - sched_expires = check_timers_list(++timers, firing, sum_sched_runtime);
> + prof_expires = check_timers_list([CPUCLOCK_PROF].cpu_timers,
> + firing, ptime);
> + virt_expires = check_timers_list([CPUCLOCK_VIRT].cpu_timers,
> + firing, utime);
> + sched_expires = check_timers_list([CLPCLOCK_SCHED].cpu_timers,
^^
0-day bot should have warned by now.
Re: [patch V2 28/38] posix-cpu-timers: Restructure expiry array
On Mon, 26 Aug 2019, Frederic Weisbecker wrote:
> On Wed, Aug 21, 2019 at 09:09:15PM +0200, Thomas Gleixner wrote:
> > /**
> > - * task_cputimers_expired - Compare two task_cputime entities.
> > + * task_cputimers_expired - Check whether posix CPU timers are expired
> > *
> > * @samples: Array of current samples for the CPUCLOCK clocks
> > - * @expiries: Array of expiry values for the CPUCLOCK clocks
> > + * @pct: Pointer to a posix_cputimers container
> > *
> > - * Returns true if any mmember of @samples is greater than the
> > corresponding
> > - * member of @expiries if that member is non zero. False otherwise
> > + * Returns true if any member of @samples is greater than the corresponding
> > + * member of @pct->bases[CLK].nextevt. False otherwise
> > */
> > -static inline bool task_cputimers_expired(const u64 *sample, const u64
> > *expiries)
> > +static inline bool
> > +task_cputimers_expired(const u64 *sample, struct posix_cputimers *pct)
> > {
> > int i;
> >
> > for (i = 0; i < CPUCLOCK_MAX; i++) {
> > - if (expiries[i] && sample[i] >= expiries[i])
> > + if (sample[i] >= pct->bases[i].nextevt)
>
> You may have false positive here if you don't check if pct->bases[i].nextevt
> is 0. Probably no big deal by the end of the series since you change that 0
> for KTIME_MAX later but right now it might hurt bisection with performance
> issues (locking sighand at every tick...).
Hrm. That should have stayed until the patch which removes that 0 state
> [...]
>
> > @@ -1176,7 +1182,7 @@ void run_posix_cpu_timers(void)
> > void set_process_cpu_timer(struct task_struct *tsk, unsigned int clkid,
> >u64 *newval, u64 *oldval)
> > {
> > - u64 now, *expiry = tsk->signal->posix_cputimers.expiries + clkid;
> > + u64 now, *nextevt = >signal->posix_cputimers.bases[clkid].nextevt;
>
> You're dereferencing the pointer before checking clkid sanity below.
Urgh. Yes.
Thanks,
tglx
Re: [patch V2 28/38] posix-cpu-timers: Restructure expiry array
On Wed, Aug 21, 2019 at 09:09:15PM +0200, Thomas Gleixner wrote:
> /**
> - * task_cputimers_expired - Compare two task_cputime entities.
> + * task_cputimers_expired - Check whether posix CPU timers are expired
> *
> * @samples: Array of current samples for the CPUCLOCK clocks
> - * @expiries:Array of expiry values for the CPUCLOCK clocks
> + * @pct: Pointer to a posix_cputimers container
> *
> - * Returns true if any mmember of @samples is greater than the corresponding
> - * member of @expiries if that member is non zero. False otherwise
> + * Returns true if any member of @samples is greater than the corresponding
> + * member of @pct->bases[CLK].nextevt. False otherwise
> */
> -static inline bool task_cputimers_expired(const u64 *sample, const u64
> *expiries)
> +static inline bool
> +task_cputimers_expired(const u64 *sample, struct posix_cputimers *pct)
> {
> int i;
>
> for (i = 0; i < CPUCLOCK_MAX; i++) {
> - if (expiries[i] && sample[i] >= expiries[i])
> + if (sample[i] >= pct->bases[i].nextevt)
You may have false positive here if you don't check if pct->bases[i].nextevt
is 0. Probably no big deal by the end of the series since you change that 0
for KTIME_MAX later but right now it might hurt bisection with performance
issues (locking sighand at every tick...).
[...]
> @@ -1176,7 +1182,7 @@ void run_posix_cpu_timers(void)
> void set_process_cpu_timer(struct task_struct *tsk, unsigned int clkid,
> u64 *newval, u64 *oldval)
> {
> - u64 now, *expiry = tsk->signal->posix_cputimers.expiries + clkid;
> + u64 now, *nextevt = >signal->posix_cputimers.bases[clkid].nextevt;
You're dereferencing the pointer before checking clkid sanity below.
>
> if (WARN_ON_ONCE(clkid >= CPUCLOCK_SCHED))
> return;
> @@ -1207,8 +1213,8 @@ 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))
> - *expiry = *newval;
> + if (expires_gt(*nextevt, *newval))
> + *nextevt = *newval;
>
> tick_dep_set_signal(tsk->signal, TICK_DEP_BIT_POSIX_TIMER);
> }
>
>
[patch V2 28/38] posix-cpu-timers: Restructure expiry array
Now that the abused struct task_cputime is gone, it's more natural to
bundle the expiry cache and the list head of each clock into a struct and
have an array of those structs.
Follow the hrtimer naming convention of 'bases' and rename the expiry cache
to 'nextevt' and adapt all usage sites.
Generates also better code .text size shrinks by 80 bytes.
Suggested-by: Ingo Molnar
Signed-off-by: Thomas Gleixner
---
V2: New patch
---
include/linux/posix-timers.h | 41 ++--
kernel/time/posix-cpu-timers.c | 104 +
2 files changed, 82 insertions(+), 63 deletions(-)
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -63,24 +63,33 @@ static inline int clockid_to_fd(const cl
}
#ifdef CONFIG_POSIX_TIMERS
+
/**
- * posix_cputimers - Container for posix CPU timer related data
- * @expiries: Earliest-expiration cache array based
+ * posix_cputimer_base - Container per posix CPU clock
+ * @nextevt: Earliest-expiration cache
* @cpu_timers:List heads to queue posix CPU timers
+ */
+struct posix_cputimer_base {
+ u64 nextevt;
+ struct list_headcpu_timers;
+};
+
+/**
+ * posix_cputimers - Container for posix CPU timer related data
+ * @bases: Base container for posix CPU clocks
*
* Used in task_struct and signal_struct
*/
struct posix_cputimers {
- u64 expiries[CPUCLOCK_MAX];
- struct list_headcpu_timers[CPUCLOCK_MAX];
+ struct posix_cputimer_base bases[CPUCLOCK_MAX];
};
static inline void posix_cputimers_init(struct posix_cputimers *pct)
{
- memset(>expiries, 0, sizeof(pct->expiries));
- INIT_LIST_HEAD(>cpu_timers[0]);
- INIT_LIST_HEAD(>cpu_timers[1]);
- INIT_LIST_HEAD(>cpu_timers[2]);
+ memset(pct->bases, 0, sizeof(pct->bases));
+ INIT_LIST_HEAD(>bases[0].cpu_timers);
+ INIT_LIST_HEAD(>bases[1].cpu_timers);
+ INIT_LIST_HEAD(>bases[2].cpu_timers);
}
void posix_cputimers_group_init(struct posix_cputimers *pct, u64 cpu_limit);
@@ -88,19 +97,23 @@ void posix_cputimers_group_init(struct p
static inline void posix_cputimers_rt_watchdog(struct posix_cputimers *pct,
u64 runtime)
{
- pct->expiries[CPUCLOCK_SCHED] = runtime;
+ pct->bases[CPUCLOCK_SCHED].nextevt = runtime;
}
/* 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_TIMERBASE(b) {
\
+ .cpu_timers = LIST_HEAD_INIT(b.cpu_timers), \
+}
+
+#define INIT_CPU_TIMERBASES(b) { \
+ INIT_CPU_TIMERBASE(b[0]), \
+ INIT_CPU_TIMERBASE(b[1]), \
+ INIT_CPU_TIMERBASE(b[2]), \
}
#define INIT_CPU_TIMERS(s) \
.posix_cputimers = {\
- .cpu_timers = INIT_CPU_TIMERLISTS(s.posix_cputimers), \
+ .bases = INIT_CPU_TIMERBASES(s.posix_cputimers.bases), \
},
#else
struct posix_cputimers { };
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -24,13 +24,13 @@ void posix_cputimers_group_init(struct p
{
posix_cputimers_init(pct);
if (cpu_limit != RLIM_INFINITY)
- pct->expiries[CPUCLOCK_PROF] = cpu_limit * NSEC_PER_SEC;
+ pct->bases[CPUCLOCK_PROF].nextevt = cpu_limit * NSEC_PER_SEC;
}
/*
* Called after updating RLIMIT_CPU to run cpu timer and update
- * tsk->signal->posix_cputimers.expiries expiration cache if
- * necessary. Needs siglock protection since other code may update
+ * tsk->signal->posix_cputimers.bases[clock].nextevt expiration cache if
+ * necessary. Needs siglock protection since other code may update the
* expiration cache as well.
*/
void update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new)
@@ -122,9 +122,11 @@ static void bump_cpu_timer(struct k_itim
}
}
-static inline bool expiry_cache_is_zero(const u64 *ec)
+static inline bool expiry_cache_is_zero(const struct posix_cputimers *pct)
{
- return !(ec[CPUCLOCK_PROF] | ec[CPUCLOCK_VIRT] | ec[CPUCLOCK_SCHED]);
+ return !(pct->bases[CPUCLOCK_PROF].nextevt |
+pct->bases[CPUCLOCK_VIRT].nextevt |
+pct->bases[CPUCLOCK_SCHED].nextevt);
}
static int
@@ -432,9 +434,9 @@ static void cleanup_timers_list(struct l
*/
static void cleanup_timers(struct posix_cputimers *pct)
{
-
