Re: [PATCH 1/2] Add scaled time to taskstats based process accounting

[Michael Neuling <mikey@neuling.org>]

In message <46C3FC41.4000609@linux.vnet.ibm.com> you wrote:
> Hi, Michael,
> 
> Thanks for doing this, this is really useful.
> 
> Michael Neuling wrote:
> > This adds two items to the taststats struct to account for user and
> > system time based on scaling the CPU frequency and instruction issue
> > rates.
> > 
> > Adds account_(user|system)_time_scaled callbacks which architectures
> > can use to account for time using this mechanism.
> > 
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > 
> > ---
> > 
> >  include/linux/kernel_stat.h |    2 ++
> >  include/linux/sched.h       |    2 +-
> >  include/linux/taskstats.h   |    6 +++++-
> >  kernel/fork.c               |    2 ++
> >  kernel/sched.c              |   21 +++++++++++++++++++++
> >  kernel/timer.c              |    7 +++++--
> >  kernel/tsacct.c             |    4 ++++
> >  7 files changed, 40 insertions(+), 4 deletions(-)
> > 
> > Index: linux-2.6-ozlabs/include/linux/kernel_stat.h
> > ===================================================================
> > --- linux-2.6-ozlabs.orig/include/linux/kernel_stat.h
> > +++ linux-2.6-ozlabs/include/linux/kernel_stat.h
> > @@ -52,7 +52,9 @@ static inline int kstat_irqs(int irq)
> >  }
> > 
> >  extern void account_user_time(struct task_struct *, cputime_t);
> > +extern void account_user_time_scaled(struct task_struct *, cputime_t);
> >  extern void account_system_time(struct task_struct *, int, cputime_t);
> > +extern void account_system_time_scaled(struct task_struct *, cputime_t);
> >  extern void account_steal_time(struct task_struct *, cputime_t);
> > 
> >  #endif /* _LINUX_KERNEL_STAT_H */
> > Index: linux-2.6-ozlabs/include/linux/sched.h
> > ===================================================================
> > --- linux-2.6-ozlabs.orig/include/linux/sched.h
> > +++ linux-2.6-ozlabs/include/linux/sched.h
> > @@ -1020,7 +1020,7 @@ struct task_struct {
> >  	int __user *clear_child_tid;		/* CLONE_CHILD_CLEARTID */
> > 
> >  	unsigned int rt_priority;
> > -	cputime_t utime, stime;
> > +	cputime_t utime, stime, utimescaled, stimescaled;
> >  	unsigned long nvcsw, nivcsw; /* context switch counts */
> >  	struct timespec start_time; 		/* monotonic time */
> >  	struct timespec real_start_time;	/* boot based time */
> > Index: linux-2.6-ozlabs/include/linux/taskstats.h
> > ===================================================================
> > --- linux-2.6-ozlabs.orig/include/linux/taskstats.h
> > +++ linux-2.6-ozlabs/include/linux/taskstats.h
> > @@ -31,7 +31,7 @@
> >   */
> > 
> > 
> > -#define TASKSTATS_VERSION	5
> > +#define TASKSTATS_VERSION	6
> >  #define TS_COMM_LEN		32	/* should be >= TASK_COMM_LEN
> >  					 * in linux/sched.h */
> > 
> > @@ -142,6 +142,10 @@ struct taskstats {
> >  	__u64	write_char;		/* bytes written */
> >  	__u64	read_syscalls;		/* read syscalls */
> >  	__u64	write_syscalls;		/* write syscalls */
> > +
> > +	/* time accounting for SMT machines */
> > +	__u64	ac_utimescaled;		/* utime scaled on frequency etc */
> > +	__u64	ac_stimescaled;		/* stime scaled on frequency etc */
> >  	/* Extended accounting fields end */
> > 
> 
> I'd also request for you to add a cpu_scaled_run_real_total for use
> by delay accounting. cpu_scaled_run_real_total should be similar in
> functionality to cpu_run_real_total.

Will do.  Should I add cpu_scaled_run_real_total to the end of the
struct taskstat, or next to cpu_run_real_total?

> 
> >  #define TASKSTATS_HAS_IO_ACCOUNTING
> > Index: linux-2.6-ozlabs/kernel/fork.c
> > ===================================================================
> > --- linux-2.6-ozlabs.orig/kernel/fork.c
> > +++ linux-2.6-ozlabs/kernel/fork.c
> > @@ -1045,6 +1045,8 @@ static struct task_struct *copy_process(
> > 
> >  	p->utime = cputime_zero;
> >  	p->stime = cputime_zero;
> > +	p->utimescaled = cputime_zero;
> > +	p->stimescaled = cputime_zero;
> > 
> >  #ifdef CONFIG_TASK_XACCT
> >  	p->rchar = 0;		/* I/O counter: bytes read */
> > Index: linux-2.6-ozlabs/kernel/sched.c
> > ===================================================================
> > --- linux-2.6-ozlabs.orig/kernel/sched.c
> > +++ linux-2.6-ozlabs/kernel/sched.c
> > @@ -3249,6 +3249,16 @@ void account_user_time(struct task_struc
> >  }
> > 
> >  /*
> > + * Account scaled user cpu time to a process.
> > + * @p: the process that the cpu time gets accounted to
> > + * @cputime: the cpu time spent in user space since the last update
> > + */
> > +void account_user_time_scaled(struct task_struct *p, cputime_t cputime)
> > +{
> > +	p->utimescaled = cputime_add(p->utimescaled, cputime);
> > +}
> > +
> > +/*
> >   * Account system cpu time to a process.
> >   * @p: the process that the cpu time gets accounted to
> >   * @hardirq_offset: the offset to subtract from hardirq_count()
> > @@ -3280,6 +3290,17 @@ void account_system_time(struct task_str
> >  }
> > 
> >  /*
> > + * Account scaled system cpu time to a process.
> > + * @p: the process that the cpu time gets accounted to
> > + * @hardirq_offset: the offset to subtract from hardirq_count()
> > + * @cputime: the cpu time spent in kernel space since the last update
> > + */
> > +void account_system_time_scaled(struct task_struct *p, cputime_t cputime)
> > +{
> > +	p->stimescaled = cputime_add(p->stimescaled, cputime);
> > +}
> > +
> > +/*
> >   * Account for involuntary wait time.
> >   * @p: the process from which the cpu time has been stolen
> >   * @steal: the cpu time spent in involuntary wait
> > Index: linux-2.6-ozlabs/kernel/timer.c
> > ===================================================================
> > --- linux-2.6-ozlabs.orig/kernel/timer.c
> > +++ linux-2.6-ozlabs/kernel/timer.c
> > @@ -826,10 +826,13 @@ void update_process_times(int user_tick)
> >  	int cpu = smp_processor_id();
> > 
> >  	/* Note: this timer irq context must be accounted for as well. */
> > -	if (user_tick)
> > +	if (user_tick) {
> >  		account_user_time(p, jiffies_to_cputime(1));
> > -	else
> > +		account_user_time_scaled(p, jiffies_to_cputime(1));
> > +	} else {
> >  		account_system_time(p, HARDIRQ_OFFSET, jiffies_to_cputime(1));
> > +		account_system_time_scaled(p, jiffies_to_cputime(1));
> > +	}
> 
> I am a little confused here, scaled accounting and regular accounting
> go hand in hand?

We need to account for scaled and normal time in this generic code.
All other calls to account_(user|system)_time are in arch code.  

> 
> >  	run_local_timers();
> >  	if (rcu_pending(cpu))
> >  		rcu_check_callbacks(cpu, user_tick);
> > Index: linux-2.6-ozlabs/kernel/tsacct.c
> > ===================================================================
> > --- linux-2.6-ozlabs.orig/kernel/tsacct.c
> > +++ linux-2.6-ozlabs/kernel/tsacct.c
> > @@ -62,6 +62,10 @@ void bacct_add_tsk(struct taskstats *sta
> >  	rcu_read_unlock();
> >  	stats->ac_utime	 = cputime_to_msecs(tsk->utime) * USEC_PER_MSEC;
> >  	stats->ac_stime	 = cputime_to_msecs(tsk->stime) * USEC_PER_MSEC;
> > +	stats->ac_utimescaled =
> > +		cputime_to_msecs(tsk->utimescaled) * USEC_PER_MSEC;
> > +	stats->ac_stimescaled =
> > +		cputime_to_msecs(tsk->stimescaled) * USEC_PER_MSEC;
> >  	stats->ac_minflt = tsk->min_flt;
> >  	stats->ac_majflt = tsk->maj_flt;
> > 
> 
> 
> -- 
> 	Warm Regards,
> 	Balbir Singh
> 	Linux Technology Center
> 	IBM, ISTL
>