Re: [PATCH] Re: /proc/uptime idle counter remains at 0

[Michael Abbott]

On Mon, 17 Aug 2009, Amerigo Wang wrote:
> On Mon, Aug 17, 2009 at 07:12:36AM +0100, Michael Abbott wrote:
> >On Mon, 17 Aug 2009, Amerigo Wang wrote:
> >> On Fri, Aug 14, 2009 at 01:18:08PM +0100, Michael Abbott wrote:
> >> >commit 6d67e34f45a92f347388e35bd84bf0361e660d3b
> >> >Author: Michael Abbott <michael.abbott@xxxxxxxxxxxxx>
> >> >Date:   Mon May 11 07:14:19 2009 +0100
> >> >
> >> >    Fix idle time field in /proc/uptime
> >> >    
> >> >    Git commit 79741dd changes idle cputime accounting, but unfortunately
> >> >    the /proc/uptime file hasn't caught up.  Here the idle time calculation
> >> >    from /proc/stat is copied over.  Further changes from commit e1c8053
> >> >    are also included in this fix.
> >> >    
> >> >    Signed-off-by: Michael Abbott <michael.abbott@xxxxxxxxxxxxx>
> >> >
> >> >diff --git a/fs/proc/uptime.c b/fs/proc/uptime.c
> >> >index 0c10a0b..be286b4 100644
> >> >--- a/fs/proc/uptime.c
> >> >+++ b/fs/proc/uptime.c
> >> >@@ -4,22 +4,32 @@
> >> > #include <linux/sched.h>
> >> > #include <linux/seq_file.h>
> >> > #include <linux/time.h>
> >> >+#include <linux/kernel_stat.h>
> >> > #include <asm/cputime.h>
> >> >+#include <asm/div64.h>
> >> > 
> >> > static int uptime_proc_show(struct seq_file *m, void *v)
> >> > {
> >> > 	struct timespec uptime;
> >> >-	struct timespec idle;
> >> >-	cputime_t idletime = cputime_add(init_task.utime, init_task.stime);
> >> >+	int i;
> >> >+	cputime64_t idle = cputime64_zero;
> >> >+	unsigned long int idle_mod;
> >> >+
> >> >+	for_each_possible_cpu(i) {
> >> >+		idle = cputime64_add(idle, kstat_cpu(i).cpustat.idle);
> >> >+#ifdef arch_idle_time
> >> >+		idle = cputime64_add(idle, arch_idle_time(i));
> >> >+#endif
> >> 
> >> 
> >> This ugly #ifdef can be removed, check fs/proc/stat.c.
> >
> >I'm sorry?  Are you sure?  Here is what fs/proc/stat.c has to say:
> >
> >#ifndef arch_idle_time
> >#define arch_idle_time(cpu) 0
> >#endif
> >...
> >		idle = cputime64_add(idle, arch_idle_time(i));
> >
> >
> >I think what you're actually saying is the #ifdef can be moved to 
> >somewhere where it can be easily missed.  For this very reason, I'd rather 
> >be more explicit about it.
> 
> Yes, indeed.
> 
> I was suggesting to move that #ifdef into some header so that
> both two files can use it. For me, this is the better fix.

Yes, you're right, this is a much better idea.  In fact, the definition of 
the arch_idle_time symbol itself should be fixed.

My problem here is I'm not getting much traction getting little patches 
like this in;  your suggestion is almost to make this patch larger!


Oh, that's nasty.  Looks as if arch_idle_time only exists in 
	arch/s390/include/asm/cputime.h
and there are 20 other architecture specific cputime.h files as well as 
asm-generic/cputime.h.  I guess asm-generic/cputime.h is only included if 
an architecture specific cputime.h doesn't exist, so it would seem that 
the direct solution would be to add
	#define arch_idle_time(cpu) 0
to *all* the other cputime.h files.  Ewww.

I'm sorry, I really don't know how detailed cpu time accounting works, and 
I have no confidence of getting a patch in after the necessary work has 
been done.


The thing that seems to be killing this patch is all the extra associated 
issues.  It looks as if detailed CPU accounting is actually in a bit of a 
mess ... but I am wholly unqualified to work on this (for a start, all my 
work is uniprocessor!)

Could we just get this patch in, please, and then maybe use this as a 
trigger to clean up everything else?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/