| Date | Mon, 19 Aug 2019 21:32:24 +0200 | From | Ingo Molnar <> | Subject | Re: [patch 27/44] posix-cpu-timers: Provide array based access to expiry cache |
| |
* Thomas Gleixner <tglx@linutronix.de> 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_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
|