Messages in this thread |  | | Subject | Re: [PATCH 2/3] sched, timer: Use atomics for thread_group_cputimer to improve scalability | From | Jason Low <> | Date | Wed, 15 Apr 2015 10:14:12 -0700 |
| |
On Wed, 2015-04-15 at 09:35 +0200, Ingo Molnar wrote: > * Ingo Molnar <mingo@kernel.org> wrote: > > > So after your changes we still have a separate: > > > > struct task_cputime { > > cputime_t utime; > > cputime_t stime; > > unsigned long long sum_exec_runtime; > > }; > > > > Which then weirdly overlaps with a different structure on a different > > abstraction level: > > > > struct thread_group_cputimer { > > atomic64_t utime; > > atomic64_t stime; > > atomic64_t sum_exec_runtime; > > int running; > > }; > > > > So I think it would be more obvious what's going on if we introduced > > an atomic task_cputime structure: > > > > struct task_cputime_atomic { > > atomic64_t utime; > > atomic64_t stime; > > atomic64_t sum_exec_runtime; > > }; > > > > and put that into 'struct thread_group_cputimer': > > > > struct thread_group_cputimer { > > struct task_cputime_atomic cputime_atomic; > > int running; > > }; > > > > Maybe even factor out the main update and reading methods into > > expressively named helper inlines? > > Btw., feel free to preserve your original series and turn this > factoring out into 1-2 extra patches on top of it: so that we preserve > your testing on the original series, and see the structure (and cost) > of the factoring out of the new data type.
Okay, I'll add a task_cputime_atomic.
That will convert things like:
void sample_group_cputimer(struct task_cputime *times, struct thread_group_cputimer *cputimer)
to
void sample_atomic_cputimes(struct task_cputime *times struct task_cputime_atomic *atomic_cputimes)
which makes more sense, and the new "task_cputime_atomic" can potentially be used in other places.
Thanks, Jason
|  |