Re: [patch 27/44] posix-cpu-timers: Provide array based access to expiry cache

[Thomas Gleixner]

On Mon, 19 Aug 2019, Ingo Molnar wrote:
> * Thomas Gleixner  wrote:
> >  struct posix_cputimers {
> > -   struct task_cputime cputime_expires;
> > -   struct list_headcpu_timers[CPUCLOCK_MAX];
> > +   /* Temporary union until all users are cleaned up */
> > +   union {
> > +   struct task_cputime cputime_expires;
> > +   u64 expiries[CPUCLOCK_MAX];
> > +   };
> > +   struct list_headcpu_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_headcpu_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_headlist;
>   };
> 
>   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.

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

Re: [patch 27/44] posix-cpu-timers: Provide array based access to expiry cache

[Ingo Molnar]

* Thomas Gleixner  wrote:

> Using 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.

>  struct posix_cputimers {
> - struct task_cputime cputime_expires;
> - struct list_headcpu_timers[CPUCLOCK_MAX];
> + /* Temporary union until all users are cleaned up */
> + union {
> + struct task_cputime cputime_expires;
> + u64 expiries[CPUCLOCK_MAX];
> + };
> + struct list_headcpu_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_headcpu_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_headlist;
};

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

[patch 27/44] posix-cpu-timers: Provide array based access to expiry cache

[Thomas Gleixner]

Using 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.

Signed-off-by: Thomas Gleixner 
---
 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(-)

--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -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_headcpu_timers[CPUCLOCK_MAX];
+   /* Temporary union until all users are cleaned up */
+   union {
+   struct task_cputime cputime_expires;
+   u64 expiries[CPUCLOCK_MAX];
+   };
+   struct list_headcpu_timers[CPUCLOCK_MAX];
 };
 
 static inline void posix_cputimers_init(struct posix_cputimers *pct)
 {
-   memset(>cputime_expires, 0, sizeof(pct->cputime_expires));
+   memset(>expiries, 0, sizeof(pct->expiries));
INIT_LIST_HEAD(>cpu_timers[0]);
INIT_LIST_HEAD(>cpu_timers[1]);
INIT_LIST_HEAD(>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 */
--- a/include/linux/sched/types.h
+++ b/include/linux/sched/types.h
@@ -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;
 };
 
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -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 *