On Wed, 2015-04-15 at 09:35 +0200, Ingo Molnar wrote: > * Ingo Molnar <mi...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/