[PATCH] PPC64 collect and export low-level cpu usage statistics
Manish Ahuja
ahuja at austin.ibm.com
Thu Feb 16 15:44:02 EST 2006
Paulus, Stephen:
I have rebuilt this patch with suggestions and against 2.6.16-git3-rc3.
This should apply cleanly.
Stephen,
Answering some of your queries.
>Why not PACA_START_TB and PACA_DELTA_TB? Also, start_tb and delta_tb don't really
>store time base values, but PURR values.
>
>
When I dropped the earlier patch, we were tracking only purr's but since
purr was a function of timebase in a sense, one of the comments was that
I use "tb" instead of "purr". It made sense then, but now with too many
things being tracked, I would ideally like to change tb/*_cpu_util to
purr as that adds readablity quite a bit.
>>Index: linux-2.6.15-rc6/arch/powerpc/kernel/entry_64.S
>>===================================================================
>>--- linux-2.6.15-rc6.orig/arch/powerpc/kernel/entry_64.S 2005-12-18 16:36:54.000000000 -0800
>>+++ linux-2.6.15-rc6/arch/powerpc/kernel/entry_64.S 2006-01-17 15:39:03.000000000 -0800
>>@@ -520,7 +520,19 @@
>> * r13 is our per cpu area, only restore it if we are returning to
>> * userspace
>> */
>>+
>> beq 1f
>>+BEGIN_FTR_SECTION
>>+ li r10,0
>>+ stb r10,PACA_CDFLAG(r13)
>>
>>
>
>cdflag get set here but not set or used anywhere else.
>
>
>
I have a segment of code that uses this functionality. I pulled it out,
since somewhere my math wasn't adding up. I left it to be dropped as a
patch later. But if you wish, I can take this out now and add it later.
>>Index: linux-2.6.15-rc6/arch/powerpc/kernel/process.c
>>===================================================================
>>--- linux-2.6.15-rc6.orig/arch/powerpc/kernel/process.c 2005-12-18 16:36:54.000000000 -0800
>>+++ linux-2.6.15-rc6/arch/powerpc/kernel/process.c 2006-01-17 21:20:25.000000000 -0800
>>@@ -243,6 +243,7 @@
>> struct thread_struct *new_thread, *old_thread;
>> unsigned long flags;
>> struct task_struct *last;
>>+ struct paca_struct *lpaca;
>>
>>
>
>This could have been declared below (near pd)
>
>
>
Yes... But it seems fine there..
#ifdef CONFIG_SMP /* avoid complexity of lazy save/restore of fpu
>>@@ -313,19 +314,34 @@
>> new_thread = &new->thread;
>> old_thread = ¤t->thread;
>>
>>-#ifdef CONFIG_PPC64
>>- /*
>>- * Collect processor utilization data per process
>>- */
>>- if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
>>- struct cpu_usage *cu = &__get_cpu_var(cpu_usage_array);
>>- long unsigned start_tb, current_tb;
>>- start_tb = old_thread->start_tb;
>>- cu->current_tb = current_tb = mfspr(SPRN_PURR);
>>- old_thread->accum_tb += (current_tb - start_tb);
>>- new_thread->start_tb = current_tb;
>>+
>>+/* Collect cpu_util utilization data per process and per processor wise */
>>+ if (cpu_has_feature(CPU_FTR_PURR)) {
>>+ struct cpu_usage *pd = &__get_cpu_var(cpu_usage_array);
>>
>>
>
>Was there some good reason to change this variable name from cu to pd?
>
>
Not really.. except pd stood for purr data and i liked tha abbr more.
>>+ long unsigned start_cpu_util, current_cpu_util;
>>+
>>+ if ( old_thread->start_cpu_util )
>>+ pd->current_cpu_util = current_cpu_util = mfspr(SPRN_PURR);
>>+ else
>>+ old_thread->start_cpu_util = pd->current_cpu_util = current_cpu_util = mfspr(SPRN_PURR);
>>
>>
>
>Probably better would be:
> pd->current_cpu_util = current_cpu_util = mfspr(SPRN_PURR);
> if (old_thread->start_cpu_util == 0)
> old_thread->start_cpu_util = current_cpu_util;
>
>
>
Yeah, that should have been obvious. Changed as requested.
>>+
>>+ /* store delta_tb & mftb into cpu_util data array for *
>>+ * later easy access otherwise you have to do run_on_cpu *
>>+ * which is expensive */
>>
>>
>
>Comment style should be:
>
> /* store delta_tb & mftb into cpu_util data array for
> * later easy access otherwise you have to do run_on_cpu
> * which is expensive
> */
>
>
>
Changed as requested.
>>+
>>+ lpaca = get_paca();
>>+ pd->collected_krntb = lpaca->delta_tb;
>>+ pd->collected_timebase = mftb();
>>+
>>+ start_cpu_util = old_thread->start_cpu_util;
>>+ old_thread->total_dp += (current_cpu_util - start_cpu_util);
>>+
>>+ /* collect time from entry into kernel to now and account it *
>>+ * in process kernel time */
>>
>>
>
>Comment style again.
>
>
>
Changed as requested.
>>+
>>+ old_thread->proc_stime += (current_cpu_util - lpaca->start_tb);
>>+ new_thread->start_cpu_util = current_cpu_util;
>> }
>>-#endif
>>
>> local_irq_save(flags);
>> last = _switch(old_thread, new_thread);
>>Index: linux-2.6.15-rc6/arch/powerpc/kernel/setup_64.c
>>===================================================================
>>--- linux-2.6.15-rc6.orig/arch/powerpc/kernel/setup_64.c 2005-12-18 16:36:54.000000000 -0800
>>+++ linux-2.6.15-rc6/arch/powerpc/kernel/setup_64.c 2006-02-10 11:51:28.197401840 -0800
>>@@ -851,3 +851,153 @@
>>
>>
>
>
>
>>+static void collect_cpu_deltas(int cpu)
>>
>>
>
>
>
>>+static void post_cpu_deltas(int cpu)
>>
>>
>
>Should those two be #ifdef CONFIG_HOTPLUG_CPU ?
>
>
>
Yeah, they should be and are now rightly so.
>>+ /* Initialize the global variables to zero */
>>+ offline_cpu_total_tb = 0;
>>+ offline_cpu_total_cpu_util = 0;
>>+ offline_cpu_total_krncycles = 0;
>>+ offline_cpu_total_idle = 0;
>>
>>
>
>You don't need to set these to zero explicitly.
>
>
>
Ok .. But since they are done.. No harm done..
>>Index: linux-2.6.15-rc6/arch/powerpc/kernel/sysfs.c
>>===================================================================
>>--- linux-2.6.15-rc6.orig/arch/powerpc/kernel/sysfs.c 2005-12-18 16:36:54.000000000 -0800
>>+++ linux-2.6.15-rc6/arch/powerpc/kernel/sysfs.c 2006-02-10 12:36:02.375372096 -0800
>>@@ -232,8 +240,11 @@
>> if (cur_cpu_spec->num_pmcs >= 8)
>> sysdev_create_file(s, &attr_pmc8);
>>
>>- if (cpu_has_feature(CPU_FTR_SMT))
>>+ if (cpu_has_feature(CPU_FTR_PURR)) {
>> sysdev_create_file(s, &attr_purr);
>>
>>
>
>This will mean that the "purr" file doesn't exist in some cases where it
>used to (even if it was useless). Not sure if that is a problem for any
>user mode utilities.
>
>
>
I truly doubt it. But if there is such a utility, then it shouldn't
really see purr if its not a power5 system.
>>Index: linux-2.6.15-rc6/include/asm-powerpc/processor.h
>>===================================================================
>>--- linux-2.6.15-rc6.orig/include/asm-powerpc/processor.h 2005-12-18 16:36:54.000000000 -0800
>>+++ linux-2.6.15-rc6/include/asm-powerpc/processor.h 2006-01-17 21:31:17.000000000 -0800
>>@@ -177,6 +177,9 @@
>> #ifdef CONFIG_PPC64
>> unsigned long start_tb; /* Start purr when proc switched in */
>> unsigned long accum_tb; /* Total accumilated purr for process */
>>+ unsigned long start_cpu_util; /* Start cpu_util when proc switch in */
>>+ unsigned long total_dp ; /* Total delta cpu_util accum for proc */
>>+ unsigned long proc_stime; /* Was pad,Now process cpu_util stime */
>>
>>
>
>total_dp and proc_stime are not used anywhere and start_tb accum_tb are no longer used.
>
>
total_dp & proc_stime are being used.
I think I made a mistake and while porting from 2.6.11.8 to 2.6.15, I
changed things. I have gone ahead and deleted these values.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: cpu_patch-git3-rc3
Url: http://ozlabs.org/pipermail/linuxppc64-dev/attachments/20060215/4944d874/attachment.txt
More information about the Linuxppc64-dev
mailing list