Messages in this thread | | | Date | Tue, 20 Aug 2019 22:22:07 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [patch 27/44] posix-cpu-timers: Provide array based access to expiry cache |
| |
On Mon, 19 Aug 2019, Ingo Molnar wrote: > * Thomas Gleixner <tglx@linutronix.de> wrote: > > 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.
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
| |