Skip to contentSkip to conversation

Introduce a new fields "gtime" and "cgtime" in task_struct and signal_struct

PATCH Superseded Laurent Vivier 3 participants 7 messages Aug 20, 2007 – Aug 5, 2009

Subsystems (6)
KVM MM core Scheduler virtio core +2 more subsystems
lore
Share & tools
lore
#0 lore raw

PATCH 2/4 Introduce a new fields "gtime" and "cgtime" in task_struct and signal_struct

[PATCH 2/4] like for cpustat, introduce the "gtime" (guest time of the task) and "cgtime" (guest time of the task children) fields for the
tasks. Modify signal_struct and task_struct. Modify /proc/<pid>/stat to display these new fields.

Signature · 2 lines
------------- Laurent.Vivier-6ktuUTfB/[email protected]  --------------
          "Software is hard" - Donald Knuth
fs/proc/array.c +42 -3
Index: kvm/fs/proc/array.c
===================================================================
@@ -354,6 +354,15 @@ static clock_t task_stime(struct task_st
 	return stime;
 }
 
+#ifdef CONFIG_GUEST_ACCOUNTING
+static clock_t task_gtime(struct task_struct *p)
+{
+	clock_t gtime = cputime_to_clock_t(p->gtime);
+
+	return gtime;
+}
+#endif
+
 static int do_task_stat(struct task_struct *task, char *buffer, int whole)
 {
 	unsigned long vsize, eip, esp, wchan = ~0UL;
@@ -369,6 +378,10 @@ static int do_task_stat(struct task_stru
 	unsigned long cmin_flt = 0, cmaj_flt = 0;
 	unsigned long  min_flt = 0,  maj_flt = 0;
 	cputime_t cutime, cstime;
+#ifdef CONFIG_GUEST_ACCOUNTING
+	cputime_t cgtime;
+	clock_t gtime;
+#endif
 	clock_t utime, stime;
 	unsigned long rsslim = 0;
 	char tcomm[sizeof(task->comm)];
@@ -388,6 +401,10 @@ static int do_task_stat(struct task_stru
 	sigemptyset(&sigign);
 	sigemptyset(&sigcatch);
 	cutime = cstime = cputime_zero;
+#ifdef CONFIG_GUEST_ACCOUNTING
+	cgtime = cputime_zero;
+	gtime = 0;
+#endif
 	utime = stime = 0;
 
 	rcu_read_lock();
@@ -406,6 +423,9 @@ static int do_task_stat(struct task_stru
 		cmaj_flt = sig->cmaj_flt;
 		cutime = sig->cutime;
 		cstime = sig->cstime;
+#ifdef CONFIG_GUEST_ACCOUNTING
+		cgtime = sig->cgtime;
+#endif
 		rsslim = sig->rlim[RLIMIT_RSS].rlim_cur;
 
 		/* add up live thread stats at the group level */
@@ -416,6 +436,9 @@ static int do_task_stat(struct task_stru
 				maj_flt += t->maj_flt;
 				utime += task_utime(t);
 				stime += task_stime(t);
+#ifdef CONFIG_GUEST_ACCOUNTING
+				gtime += task_gtime(t);
+#endif
 				t = next_thread(t);
 			} while (t != task);
 
@@ -423,6 +446,9 @@ static int do_task_stat(struct task_stru
 			maj_flt += sig->maj_flt;
 			utime += cputime_to_clock_t(sig->utime);
 			stime += cputime_to_clock_t(sig->stime);
+#ifdef CONFIG_GUEST_ACCOUNTING
+			gtime += cputime_to_clock_t(sig->gtime);
+#endif
 		}
 
 		sid = signal_session(sig);
@@ -440,6 +466,9 @@ static int do_task_stat(struct task_stru
 		maj_flt = task->maj_flt;
 		utime = task_utime(task);
 		stime = task_stime(task);
+#ifdef CONFIG_GUEST_ACCOUNTING
+		gtime = task_gtime(task);
+#endif
 	}
 
 	/* scale priority and nice values from timeslices to -20..20 */
@@ -455,9 +484,13 @@ static int do_task_stat(struct task_stru
 	/* convert nsec -> ticks */
 	start_time = nsec_to_clock_t(start_time);
 
-	res = sprintf(buffer, "%d (%s) %c %d %d %d %d %d %u %lu \
-%lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
-%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu\n",
+	res = sprintf(buffer, "%d (%s) %c %d %d %d %d %d %u %lu "
+"%lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu "
+#ifdef CONFIG_GUEST_ACCOUNTING
+"%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld\n",
+#else
+"%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu\n",
+#endif
 		task->pid,
 		tcomm,
 		state,
@@ -502,7 +535,13 @@ static int do_task_stat(struct task_stru
 		task_cpu(task),
 		task->rt_priority,
 		task->policy,
+#ifdef CONFIG_GUEST_ACCOUNTING
+		(unsigned long long)delayacct_blkio_ticks(task),
+		gtime,
+		cputime_to_clock_t(cgtime));
+#else
 		(unsigned long long)delayacct_blkio_ticks(task));
+#endif
 	if (mm)
 		mmput(mm);
 	return res;
include/linux/sched.h +7 -0
Index: kvm/include/linux/sched.h
===================================================================
@@ -515,6 +515,10 @@ struct signal_struct {
 	 * in __exit_signal, except for the group leader.
 	 */
 	cputime_t utime, stime, cutime, cstime;
+#ifdef CONFIG_GUEST_ACCOUNTING
+	cputime_t gtime;
+	cputime_t cgtime;
+#endif
 	unsigned long nvcsw, nivcsw, cnvcsw, cnivcsw;
 	unsigned long min_flt, maj_flt, cmin_flt, cmaj_flt;
 	unsigned long inblock, oublock, cinblock, coublock;
@@ -1019,6 +1023,9 @@ struct task_struct {
 
 	unsigned int rt_priority;
 	cputime_t utime, stime;
+#ifdef CONFIG_GUEST_ACCOUNTING
+	cputime_t gtime;
+#endif
 	unsigned long nvcsw, nivcsw; /* context switch counts */
 	struct timespec start_time; 		/* monotonic time */
 	struct timespec real_start_time;	/* boot based time */
kernel/exit.c +10 -0
Index: kvm/kernel/exit.c
===================================================================
@@ -120,6 +120,9 @@ static void __exit_signal(struct task_st
 		 */
 		sig->utime = cputime_add(sig->utime, tsk->utime);
 		sig->stime = cputime_add(sig->stime, tsk->stime);
+#ifdef CONFIG_GUEST_ACCOUNTING
+		sig->gtime = cputime_add(sig->gtime, tsk->gtime);
+#endif
 		sig->min_flt += tsk->min_flt;
 		sig->maj_flt += tsk->maj_flt;
 		sig->nvcsw += tsk->nvcsw;
@@ -1255,6 +1258,13 @@ static int wait_task_zombie(struct task_
 			cputime_add(p->stime,
 			cputime_add(sig->stime,
 				    sig->cstime)));
+#ifdef CONFIG_GUEST_ACCOUNTING
+		psig->cgtime =
+			cputime_add(psig->cgtime,
+			cputime_add(p->gtime,
+			cputime_add(sig->gtime,
+				    sig->cgtime)));
+#endif
 		psig->cmin_flt +=
 			p->min_flt + sig->min_flt + sig->cmin_flt;
 		psig->cmaj_flt +=
kernel/fork.c +7 -0
Index: kvm/kernel/fork.c
===================================================================
@@ -877,6 +877,10 @@ static inline int copy_signal(unsigned l
 	sig->tty_old_pgrp = NULL;
 
 	sig->utime = sig->stime = sig->cutime = sig->cstime = cputime_zero;
+#ifdef CONFIG_GUEST_ACCOUNTING
+	sig->gtime = cputime_zero;
+	sig->cgtime = cputime_zero;
+#endif
 	sig->nvcsw = sig->nivcsw = sig->cnvcsw = sig->cnivcsw = 0;
 	sig->min_flt = sig->maj_flt = sig->cmin_flt = sig->cmaj_flt = 0;
 	sig->inblock = sig->oublock = sig->cinblock = sig->coublock = 0;
@@ -1045,6 +1049,9 @@ static struct task_struct *copy_process(
 
 	p->utime = cputime_zero;
 	p->stime = cputime_zero;
+#ifdef CONFIG_GUEST_ACCOUNTING
+	p->gtime = cputime_zero;
+#endif
 
 #ifdef CONFIG_TASK_XACCT
 	p->rchar = 0;		/* I/O counter: bytes read */

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

#1 lore raw

Am Montag, 20. August 2007 schrieb Laurent Vivier:

fs/proc/array.c 2007-08-20 13:04:03.000000000 +0200

Quoted 4 lines

Index: kvm/fs/proc/array.c
===================================================================

fs/proc/array.c 2007-08-20 13:04:03.000000000 +0200 renamed from fs/proc/array.c    2007-08-20 11:11:30.000000000 +0200 +0 -0

Just a heads up, this patch collides with this fix in mm:
http://marc.info/?l=linux-mm-commits&m=118737949222362&w=2

If Ingo accepts this fix, your patch should be adopted in array.c to use cputime_t for gtime as well. Lets see what Ingo thinks.

Christian

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/

#2 lore raw
Quoted 11 lines · Christian Borntraeger

Am Montag, 20. August 2007 schrieb Laurent Vivier:

fs/proc/array.c 2007-08-20 13:04:03.000000000 +0200

Quoted 4 lines

Index: kvm/fs/proc/array.c
===================================================================

fs/proc/array.c 2007-08-20 13:04:03.000000000 +0200 renamed from fs/proc/array.c 2007-08-20 11:11:30.000000000 +0200 +0 -0

Just a heads up, this patch collides with this fix in mm:
http://marc.info/?l=linux-mm-commits&m=118737949222362&w=2

If Ingo accepts this fix, your patch should be adopted in array.c to use cputime_t for gtime as well. Lets see what Ingo thinks.

Thank you for the comment. No problem for me to rewrite the patch according the fix in the mm.

Laurent

Signature · 2 lines
------------- Laurent.Vivier-6ktuUTfB/[email protected]  --------------
          "Software is hard" - Donald Knuth

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

#3 lore raw
In reply to Christian Borntraeger
Quoted 11 lines · Christian Borntraeger

Am Montag, 20. August 2007 schrieb Laurent Vivier:

fs/proc/array.c 2007-08-20 13:04:03.000000000 +0200

Quoted 4 lines

Index: kvm/fs/proc/array.c
===================================================================

fs/proc/array.c 2007-08-20 13:04:03.000000000 +0200 renamed from fs/proc/array.c 2007-08-20 11:11:30.000000000 +0200 +0 -0

Just a heads up, this patch collides with this fix in mm:
http://marc.info/?l=linux-mm-commits&m=118737949222362&w=2

If Ingo accepts this fix, your patch should be adopted in array.c to use cputime_t for gtime as well. Lets see what Ingo thinks.

Thank you for the comment. No problem for me to rewrite the patch according the fix in the mm.

Laurent

Signature · 2 lines
------------- [email protected]  --------------
          "Software is hard" - Donald Knuth

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization

#4 lore raw
In reply to Laurent Vivier

Am Montag, 20. August 2007 schrieb Laurent Vivier:

fs/proc/array.c 2007-08-20 13:04:03.000000000 +0200

Quoted 4 lines

Index: kvm/fs/proc/array.c
===================================================================

fs/proc/array.c 2007-08-20 13:04:03.000000000 +0200 renamed from fs/proc/array.c    2007-08-20 11:11:30.000000000 +0200 +0 -0

Just a heads up, this patch collides with this fix in mm:
http://marc.info/?l=linux-mm-commits&m=118737949222362&w=2

If Ingo accepts this fix, your patch should be adopted in array.c to use cputime_t for gtime as well. Lets see what Ingo thinks.

Christian

#5 lore raw
In reply to Laurent Vivier
Quoted 8 lines · Laurent Vivier

[PATCH 2/4] like for cpustat, introduce the "gtime" (guest time of the task) and "cgtime" (guest time of the task children) fields for the tasks. Modify signal_struct and task_struct. Modify /proc/<pid>/stat to display these new fields.

include/linux/sched.h struct signal_struct { · line 521

Quoted 7 lines
include/linux/sched.h +4 -0
@@ -515,6 +515,10 @@ struct signal_struct {
 	 * in __exit_signal, except for the group leader.
 	 */
 	cputime_t utime, stime, cutime, cstime;
+#ifdef CONFIG_GUEST_ACCOUNTING
+	cputime_t gtime;
+	cputime_t cgtime;
+#endif

A handful of general (and less general) observations about these patches:

1- The code is very ugly due to being an #ifdef fest. Please
always try to avoid them.

2- cputime_t is very coarse on x86: measured in jiffies. This means
that with a default HZ of 250 we'll have units of 4 msecs. That's almost useless to rely on in new instrumentation: an irq can come in and out without accounting noticing it, etc. If we do some new statistics then it should be a lot better than jiffies granular.

3- stime of vcpu tasks/threads already approximates 'guest time'
adequately. (as Jeremy observed it as well) Yes, it mixes 'true guest mode' and 'host mode' system time, but then again due to the jiffies granularity we have a _far_ bigger skew going on already.

4- namespace collision: 'gtime' is already used as 'group time' in
a few places. One of the two things needs to be renamed.

5- tracepoints and perfcounters could be used to measure guest time
precisely, in a low-overhead mode.

These issues need to be addressed in a meaningful way. #2 probably means a revamping of cputime_t handling on x86 - of not just the gtime. But #3 is worth keeping in mind as well.

I think #5 is the most capable solution by a wide margin - we need just a single tracepoint to emit 'nsecs spent in guest mode' information and that's it. It would be a far smaller patch.

The tracepoint might even sample the guest RIP and hence could be used as a VM-exit profiler and 'perf record -e kvm:vm_exit + perf report' could be used to examine/profile/trace guest exit reasons.

Ingo

#6 lore raw
In reply to Laurent Vivier
Quoted 8 lines · Laurent Vivier

[PATCH 2/4] like for cpustat, introduce the "gtime" (guest time of the task) and "cgtime" (guest time of the task children) fields for the tasks. Modify signal_struct and task_struct. Modify /proc/<pid>/stat to display these new fields.

include/linux/sched.h struct signal_struct { · line 521

Quoted 7 lines
include/linux/sched.h +4 -0
@@ -515,6 +515,10 @@ struct signal_struct {
 	 * in __exit_signal, except for the group leader.
 	 */
 	cputime_t utime, stime, cutime, cstime;
+#ifdef CONFIG_GUEST_ACCOUNTING
+	cputime_t gtime;
+	cputime_t cgtime;
+#endif

A handful of general (and less general) observations about these patches:

1- The code is very ugly due to being an #ifdef fest. Please
always try to avoid them.

2- cputime_t is very coarse on x86: measured in jiffies. This means
that with a default HZ of 250 we'll have units of 4 msecs. That's almost useless to rely on in new instrumentation: an irq can come in and out without accounting noticing it, etc. If we do some new statistics then it should be a lot better than jiffies granular.

3- stime of vcpu tasks/threads already approximates 'guest time'
adequately. (as Jeremy observed it as well) Yes, it mixes 'true guest mode' and 'host mode' system time, but then again due to the jiffies granularity we have a _far_ bigger skew going on already.

4- namespace collision: 'gtime' is already used as 'group time' in
a few places. One of the two things needs to be renamed.

5- tracepoints and perfcounters could be used to measure guest time
precisely, in a low-overhead mode.

These issues need to be addressed in a meaningful way. #2 probably means a revamping of cputime_t handling on x86 - of not just the gtime. But #3 is worth keeping in mind as well.

I think #5 is the most capable solution by a wide margin - we need just a single tracepoint to emit 'nsecs spent in guest mode' information and that's it. It would be a far smaller patch.

The tracepoint might even sample the guest RIP and hence could be used as a VM-exit profiler and 'perf record -e kvm:vm_exit + perf report' could be used to examine/profile/trace guest exit reasons.

Ingo