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);
+ }
}
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.
* 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
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
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.
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
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.