Re: [PATCH] Add all thread stats for TASKSTATS_CMD_ATTR_TGID

[Balbir Singh <balbir@linux.vnet.ibm.com>]

Guillaume Chazarain wrote:
> TASKSTATS_CMD_ATTR_TGID used to return only the delay accounting stats,
> not the basic and extended accounting. With this patch,
> TASKSTATS_CMD_ATTR_TGID returns also the min, max or sum (depending on
> the semantic of the field) of the accounting info for all threads of a
> thread group. This makes TASKSTATS_CMD_ATTR_TGID usable in a similar
> fashion to TASKSTATS_CMD_ATTR_PID, for commands like iotop -P
> (http://guichaz.free.fr/misc/iotop.py).
> 
> Changelog since V1 (http://lkml.org/lkml/2007/8/2/185):
> - Update combined stats of exited threads in fill_tgid_exit() as
> suggested by Balbir Singh.
> - Very light cleanup of fill_tgid_exit() by the way.
> - bacct fields are also combined for all threads.
> - Instead of assuming memory stats are identical for all threads, we
> take the max of all threads (hiwater_rss and hiwater_vm).
> 
> Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>
> Cc: Balbir Singh <balbir@in.ibm.com>
> Cc: Jay Lan <jlan@engr.sgi.com>
> Cc: Jonathan Lim <jlim@sgi.com>
> ---
> 
>  kernel/taskstats.c |   15 ++++++++++-----
>  kernel/tsacct.c    |   43 ++++++++++++++++++++++---------------------
>  2 files changed, 32 insertions(+), 26 deletions(-)
> 
> diff -r 73eff559debc kernel/taskstats.c
> --- a/kernel/taskstats.c	Sat Aug 18 17:15:17 2007 -0700
> +++ b/kernel/taskstats.c	Sun Aug 19 17:20:15 2007 +0200
> @@ -246,6 +246,8 @@ static int fill_tgid(pid_t tgid, struct 
> 
>  		stats->nvcsw += tsk->nvcsw;
>  		stats->nivcsw += tsk->nivcsw;
> +		bacct_add_tsk(stats, tsk);
> +		xacct_add_tsk(stats, tsk);

I'm afraid this is still not good enough. bacct_add_tsk() will assign
values and do nothing in the loop (HINT: no summation). Ideally,
we should split up bacct_add_tsk(), so that the initialization
code is split up from values that can summed.

>  	} while_each_thread(first, tsk);
> 
>  	unlock_task_sighand(first, &flags);
> @@ -265,21 +267,24 @@ static void fill_tgid_exit(struct task_s
>  static void fill_tgid_exit(struct task_struct *tsk)
>  {
>  	unsigned long flags;
> +	struct taskstats *tg_stats;
> 
>  	spin_lock_irqsave(&tsk->sighand->siglock, flags);
> -	if (!tsk->signal->stats)
> +	tg_stats = tsk->signal->stats;
> +	if (!tg_stats)
>  		goto ret;
> 
>  	/*
>  	 * Each accounting subsystem calls its functions here to
>  	 * accumalate its per-task stats for tsk, into the per-tgid structure
>  	 *
> -	 *	per-task-foo(tsk->signal->stats, tsk);
> -	 */
> -	delayacct_add_tsk(tsk->signal->stats, tsk);
> +	 *	per-task-foo(tg_stats, tsk);
> +	 */
> +	bacct_add_tsk(tg_stats, tsk);

Ditto.. see above

> +	xacct_add_tsk(tg_stats, tsk);
> +	delayacct_add_tsk(tg_stats, tsk);
>  ret:
>  	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
> -	return;
>  }
> 
>  static int add_del_listener(pid_t pid, cpumask_t *maskp, int isadd)
> diff -r 73eff559debc kernel/tsacct.c
> --- a/kernel/tsacct.c	Sat Aug 18 17:15:17 2007 -0700
> +++ b/kernel/tsacct.c	Sun Aug 19 17:20:43 2007 +0200
> @@ -38,8 +38,9 @@ void bacct_add_tsk(struct taskstats *sta
>  	/* rebase elapsed time to usec */
>  	ac_etime = timespec_to_ns(&ts);
>  	do_div(ac_etime, NSEC_PER_USEC);
> -	stats->ac_etime = ac_etime;
> -	stats->ac_btime = get_seconds() - ts.tv_sec;
> +	stats->ac_etime = max_t(u64, stats->ac_etime, ac_etime);
> +	stats->ac_btime = min_t(u32, stats->ac_btime,
> +				get_seconds() - ts.tv_sec);

I am not sure why we get the max value, ideally we should
be summing up utime, stime into the stats structure.


Could you please write a simple test case and ensure that the
summed up stats are indeed correct for TGID's?

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL