Patch: cpu utilization monitor.

[Dave Hansen]

On Tue, 2004-03-16 at 15:38, ahuja at austin.ibm.com wrote:
> This patch adds the framework required by performace team and on demand
> computing. At this point only the important bits/framework are covered.
>
> All the kobjects/calculations are yet to be written.
>
> We are still contiunuing to disscuss methods for performace monitoring.
>
> Purr is a vcpu performance counter. This collects purr/tb
> periodically to be used later in computations from any given
> cpu without needing to acquire a cpu exclusively.

Your code is very tidy, but I do have a few questions.

How does this complement the existing oprofile performance counter
interface which is already included in the kernel?

You probably want to have your own config symbol for these

        obj-$(CONFIG_LPARCFG)          += lparcfg.o profile.o

For instance, since your code appears to only work on certain CPUs, you
might want to do this in arch/ppc64/Kconfig:

        config POWER5_PROFILE...
                bool
                depends on LPARCFG && YOUR_CHIP_MODEL_HERE...

Also, do you really want to call your files profile.[ch]?  Isn't it just
for specific CPU models?

Is the built-in timer functionality in the kernel accurate enough for
what you want?  Are you sure you really want a new timer, instead of
just plugging into the existing timer interrupt like ppc64_do_profile()?

Instead of having the entire functional part of a function body
indented, like in start_util_timer(), I like seeing something like this:
	if (tl->function != NULL)
		return;
It saves a lot of indenting mess, and I think it looks better.

Also, how can start_util_timer() get called twice?  Shouldn't there
simply be a BUG() if that happens?  Should you really be compensating
for that?

I think this code really needs to go out into another function
 int __devinit __cpu_up(unsigned int cpu)
...

      /* Collect starting purr */
       if (PVR_VER(systemcfg->processor) == PV_POWER5) {
               cus->start_purr = mfspr(PURR);
               cus->tb = mftb();
       }
There's no need to put performance counter initialization inline with
the rest of the cpu bringup code.  I'd at least put that code into a
function that goes into your profile.c, and is called from the CPU
bringup code.

Are these
        struct cpu_util_store {
                struct timer_list my_timer;
                unsigned long long start_purr;
                unsigned long long current_purr;
                unsigned long long tb;
        };
really "unsigned long long", or are they always a particular size?  If
they're a constant size in the hardware, it's probably best to use the
u64 notation.  Also, my_timer is probably a bad variable name.  You
might try something like "timer" or cpu_util_timer.  The 'my_'
definitely needs to go.

Also, some other questions to think about as you develop this further:
How easy will it be to expand this code to sample other performance
counters?  Can this code be used with other CPUs types?  Is this code
compatible with hotplug CPUs?   Can you get the CPU to generate
interrupts when the counter overflows, instead of sampling?

-- dave


** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/