Re: [PATCH] Add all thread stats for TASKSTATS_CMD_ATTR_TGID

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 2 Aug 2007 15:53:13 +0200
"Guillaume Chazarain" <guichaz@yahoo.fr> wrote:

> [Resent with different recipients as nagar@watson.ibm.com bounced with
> a User unknown]
> 
> Hi,
> 
> This patch adds all thread accounting stats for the global tgid stats.
> As a shameless plug, this fixes iotop -P (http://guichaz.free.fr/misc/iotop.py).

uhm, that's a fairly skimpy changelog for a fairly significant functional
change.

> Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>
> ---
> 
> diff -r 22708012ca6e kernel/taskstats.c
> --- a/kernel/taskstats.c	Tue Jul 31 21:12:07 2007 -0700
> +++ b/kernel/taskstats.c	Wed Aug 01 17:43:54 2007 +0200
> @@ -233,6 +233,7 @@ static int fill_tgid(pid_t tgid, struct
>  		memset(stats, 0, sizeof(*stats));
> 
>  	tsk = first;
> +	bacct_add_tsk(stats, first);
>  	do {
>  		if (tsk->exit_state)
>  			continue;
> @@ -246,6 +247,7 @@ static int fill_tgid(pid_t tgid, struct
> 
>  		stats->nvcsw += tsk->nvcsw;
>  		stats->nivcsw += tsk->nivcsw;
> +		xacct_add_tsk(stats, tsk);
>  	} while_each_thread(first, tsk);
> 
>  	unlock_task_sighand(first, &flags);
> diff -r 22708012ca6e kernel/tsacct.c
> --- a/kernel/tsacct.c	Tue Jul 31 21:12:07 2007 -0700
> +++ b/kernel/tsacct.c	Wed Aug 01 17:41:40 2007 +0200
> @@ -81,8 +81,8 @@ void xacct_add_tsk(struct taskstats *sta
>  	struct mm_struct *mm;
> 
>  	/* convert pages-jiffies to Mbyte-usec */
> -	stats->coremem = jiffies_to_usecs(p->acct_rss_mem1) * PAGE_SIZE / MB;
> -	stats->virtmem = jiffies_to_usecs(p->acct_vm_mem1) * PAGE_SIZE / MB;
> +	stats->coremem += jiffies_to_usecs(p->acct_rss_mem1) * PAGE_SIZE / MB;
> +	stats->virtmem += jiffies_to_usecs(p->acct_vm_mem1) * PAGE_SIZE / MB;
>  	mm = get_task_mm(p);
>  	if (mm) {
>  		/* adjust to KB unit */
> @@ -90,18 +90,14 @@ void xacct_add_tsk(struct taskstats *sta
>  		stats->hiwater_vm    = mm->hiwater_vm * PAGE_SIZE / KB;

How come hiwater_rss and hiwater_vm weren't similarly treated?

>  		mmput(mm);
>  	}
> -	stats->read_char	= p->rchar;
> -	stats->write_char	= p->wchar;
> -	stats->read_syscalls	= p->syscr;
> -	stats->write_syscalls	= p->syscw;
> +	stats->read_char	+= p->rchar;
> +	stats->write_char	+= p->wchar;
> +	stats->read_syscalls	+= p->syscr;
> +	stats->write_syscalls	+= p->syscw;
>  #ifdef CONFIG_TASK_IO_ACCOUNTING
> -	stats->read_bytes	= p->ioac.read_bytes;
> -	stats->write_bytes	= p->ioac.write_bytes;
> -	stats->cancelled_write_bytes = p->ioac.cancelled_write_bytes;
> -#else
> -	stats->read_bytes	= 0;
> -	stats->write_bytes	= 0;
> -	stats->cancelled_write_bytes = 0;
> +	stats->read_bytes	+= p->ioac.read_bytes;
> +	stats->write_bytes	+= p->ioac.write_bytes;
> +	stats->cancelled_write_bytes += p->ioac.cancelled_write_bytes;
>  #endif

Suitable cc's added.  Guys, please have a think about the implications of
this change?  In particular, why didn't we do this on day one?  It's so
obvious that I have a feeling we must have had a reason?