2010-11-19 20:11:53

by Michael Holzheu

[permalink] [raw]
Subject: [patch 4/4] taskstats: Export "cdata_acct" with taskstats

From: Michael Holzheu <[email protected]>

With this patch the (full) cumulative CPU time is added to "struct taskstats".
The CPU time is only returned for the thread group leader.

Signed-off-by: Michael Holzheu <[email protected]>
---
include/linux/taskstats.h | 7 ++++++-
kernel/tsacct.c | 7 +++++++
2 files changed, 13 insertions(+), 1 deletion(-)

--- a/include/linux/taskstats.h
+++ b/include/linux/taskstats.h
@@ -33,7 +33,7 @@
*/


-#define TASKSTATS_VERSION 7
+#define TASKSTATS_VERSION 8
#define TS_COMM_LEN 32 /* should be >= TASK_COMM_LEN
* in linux/sched.h */

@@ -163,6 +163,11 @@ struct taskstats {
/* Delay waiting for memory reclaim */
__u64 freepages_count;
__u64 freepages_delay_total;
+ /* version 7 ends here */
+
+ /* All cumulative CPU time of dead children */
+ __u64 ac_cutime_acct; /* User CPU time [usec] */
+ __u64 ac_cstime_acct; /* System CPU time [usec] */
};


--- a/kernel/tsacct.c
+++ b/kernel/tsacct.c
@@ -31,6 +31,7 @@ void bacct_add_tsk(struct taskstats *sta
const struct cred *tcred;
struct timespec uptime, ts;
u64 ac_etime;
+ unsigned long flags;

BUILD_BUG_ON(TS_COMM_LEN < TASK_COMM_LEN);

@@ -71,6 +72,12 @@ void bacct_add_tsk(struct taskstats *sta
stats->ac_majflt = tsk->maj_flt;

strncpy(stats->ac_comm, tsk->comm, sizeof(stats->ac_comm));
+ if (tsk->tgid == tsk->pid && lock_task_sighand(tsk, &flags)) {
+ struct cdata *cd = &tsk->signal->cdata_acct;
+ stats->ac_cutime_acct = cputime_to_usecs(cd->utime);
+ stats->ac_cstime_acct = cputime_to_usecs(cd->stime);
+ unlock_task_sighand(tsk, &flags);
+ }
}



2010-11-25 13:33:56

by Oleg Nesterov

[permalink] [raw]
Subject: Re: [patch 4/4] taskstats: Export "cdata_acct" with taskstats

On 11/19, Michael Holzheu wrote:
>
> From: Michael Holzheu <[email protected]>
>
> With this patch the (full) cumulative CPU time is added to "struct taskstats".
> The CPU time is only returned for the thread group leader.
>
> ...
>
> + if (tsk->tgid == tsk->pid

thread_group_leader() ?

> && lock_task_sighand(tsk, &flags)) {

Do you really need ->siglock? Starting from 2.6.35 it is always
safe to access ->signal.

Oleg.

2010-11-25 16:57:31

by Balbir Singh

[permalink] [raw]
Subject: Re: [patch 4/4] taskstats: Export "cdata_acct" with taskstats

* Michael Holzheu <[email protected]> [2010-11-19 21:11:12]:

> From: Michael Holzheu <[email protected]>
>
> With this patch the (full) cumulative CPU time is added to "struct taskstats".
> The CPU time is only returned for the thread group leader.
>
> Signed-off-by: Michael Holzheu <[email protected]>
> ---
> include/linux/taskstats.h | 7 ++++++-
> kernel/tsacct.c | 7 +++++++
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> --- a/include/linux/taskstats.h
> +++ b/include/linux/taskstats.h
> @@ -33,7 +33,7 @@
> */
>
>
> -#define TASKSTATS_VERSION 7
> +#define TASKSTATS_VERSION 8
> #define TS_COMM_LEN 32 /* should be >= TASK_COMM_LEN
> * in linux/sched.h */
>
> @@ -163,6 +163,11 @@ struct taskstats {
> /* Delay waiting for memory reclaim */
> __u64 freepages_count;
> __u64 freepages_delay_total;
> + /* version 7 ends here */
> +
> + /* All cumulative CPU time of dead children */
> + __u64 ac_cutime_acct; /* User CPU time [usec] */
> + __u64 ac_cstime_acct; /* System CPU time [usec] */
> };
>
>
> --- a/kernel/tsacct.c
> +++ b/kernel/tsacct.c
> @@ -31,6 +31,7 @@ void bacct_add_tsk(struct taskstats *sta
> const struct cred *tcred;
> struct timespec uptime, ts;
> u64 ac_etime;
> + unsigned long flags;
>
> BUILD_BUG_ON(TS_COMM_LEN < TASK_COMM_LEN);
>
> @@ -71,6 +72,12 @@ void bacct_add_tsk(struct taskstats *sta
> stats->ac_majflt = tsk->maj_flt;
>
> strncpy(stats->ac_comm, tsk->comm, sizeof(stats->ac_comm));
> + if (tsk->tgid == tsk->pid && lock_task_sighand(tsk, &flags)) {

I don't think referring to tgid and pid is a good idea in the context
of namespaces, thread_group_leader makes more sense like Oleg pointed
out.

> + struct cdata *cd = &tsk->signal->cdata_acct;
> + stats->ac_cutime_acct = cputime_to_usecs(cd->utime);
> + stats->ac_cstime_acct = cputime_to_usecs(cd->stime);
> + unlock_task_sighand(tsk, &flags);
> + }
> }
>
>
>

--
Three Cheers,
Balbir

2010-11-25 17:21:24

by Michael Holzheu

[permalink] [raw]
Subject: Re: [patch 4/4] taskstats: Export "cdata_acct" with taskstats

Hello Oleg,

On Thu, 2010-11-25 at 14:26 +0100, Oleg Nesterov wrote:
> On 11/19, Michael Holzheu wrote:
> >
> > From: Michael Holzheu <[email protected]>
> >
> > With this patch the (full) cumulative CPU time is added to "struct taskstats".
> > The CPU time is only returned for the thread group leader.
> >
> > ...
> >
> > + if (tsk->tgid == tsk->pid
>
> thread_group_leader() ?

Yes, that's better.

> > && lock_task_sighand(tsk, &flags)) {
>
> Do you really need ->siglock? Starting from 2.6.35 it is always
> safe to access ->signal.

Hmmm, if you say that...

I just did it like it is done in e.g. fs/proc/base.c (proc_pid_limits).
Can we remove the locking there, too?

Michael

2010-11-29 16:51:02

by Oleg Nesterov

[permalink] [raw]
Subject: Re: [patch 4/4] taskstats: Export "cdata_acct" with taskstats

On 11/25, Michael Holzheu wrote:
>
> Hello Oleg,
>
> On Thu, 2010-11-25 at 14:26 +0100, Oleg Nesterov wrote:
> > On 11/19, Michael Holzheu wrote:
> > >
> > > From: Michael Holzheu <[email protected]>
> > >
> > > With this patch the (full) cumulative CPU time is added to "struct taskstats".
> > > The CPU time is only returned for the thread group leader.
> > >
> > > ...
> > >
> > > + if (tsk->tgid == tsk->pid
> >
> > thread_group_leader() ?
>
> Yes, that's better.
>
> > > && lock_task_sighand(tsk, &flags)) {
> >
> > Do you really need ->siglock? Starting from 2.6.35 it is always
> > safe to access ->signal.
>
> Hmmm, if you say that...
>
> I just did it like it is done in e.g. fs/proc/base.c (proc_pid_limits).
> Can we remove the locking there, too?

We can certainly remove more siglock's which were previously
needed to access ->signal.

This particular one is just wrong. We need task_lock(group_leader)
to read signal->rlim atomically. However, it is not trivial to do
this correctly. Probably we should ignore this minor problem.

In any case, this ->siglock buys nothing today.

Oleg.

2010-11-29 16:58:50

by Michael Holzheu

[permalink] [raw]
Subject: Re: [patch 4/4] taskstats: Export "cdata_acct" with taskstats

Hello Oleg,

On Mon, 2010-11-29 at 17:43 +0100, Oleg Nesterov wrote:
> > I just did it like it is done in e.g. fs/proc/base.c (proc_pid_limits).
> > Can we remove the locking there, too?
>
> We can certainly remove more siglock's which were previously
> needed to access ->signal.
>
> This particular one is just wrong. We need task_lock(group_leader)
> to read signal->rlim atomically. However, it is not trivial to do
> this correctly. Probably we should ignore this minor problem.
>
> In any case, this ->siglock buys nothing today.

But at least to get the two values cutime and cstime consistent we need
the siglock? There could be a parallel update for
tsk->signal->cutime/cstime, while the taskstats are created, no?

Michael

2010-11-29 18:16:29

by Oleg Nesterov

[permalink] [raw]
Subject: Re: [patch 4/4] taskstats: Export "cdata_acct" with taskstats

On 11/29, Michael Holzheu wrote:
>
> Hello Oleg,
>
> On Mon, 2010-11-29 at 17:43 +0100, Oleg Nesterov wrote:
> > > I just did it like it is done in e.g. fs/proc/base.c (proc_pid_limits).
> > > Can we remove the locking there, too?
> >
> > We can certainly remove more siglock's which were previously
> > needed to access ->signal.
> >
> > This particular one is just wrong. We need task_lock(group_leader)
> > to read signal->rlim atomically. However, it is not trivial to do
> > this correctly. Probably we should ignore this minor problem.
> >
> > In any case, this ->siglock buys nothing today.
>
> But at least to get the two values cutime and cstime consistent we need
> the siglock? There could be a parallel update for
> tsk->signal->cutime/cstime, while the taskstats are created, no?

I guess you mean 4/4 you sent. Probably you are right, I'll try
to look tomorrow.

Oleg.