[PATCH 5/8] cell: enable pause(0) in cpu_idle

[Arnd Bergmann]

Am Samstag 10 Dezember 2005 17:24 schrieb Milton Miller:
> Much better, Just a few more things.  One cut and paste error, the rest
> are more cleanups and probes of the idle code now it is in one place to
> see.

Ok. Paulus, please ignore this patch again.

> ...
>
> > +#include "pervasive.h"
> > +
> > +struct cbe_pervasive {
> > +       struct pmd_regs __iomem *regs;
> > +       int power_management_enable;
> > +};
> > +
> > +static DEFINE_PER_CPU(struct cbe_pervasive, cbe_pervasive);
> > +
> > +static void pause_zero(void)
> > +{
> > +       unsigned int multi_threading_control;
> > +
> > +       /* Reset Thread Run Latch (latch is set in idle.c) */
>
> runlatch_off is both here and in pause zero idle.   It is turned on
> below in pause_zero_idle.

ok, I will fix.

> > +       ppc64_runlatch_off();
> > +
> > +       local_irq_disable();
> > +       if (__get_cpu_var(cbe_pervasive).power_management_enable) {
> > +               /* Pause the PU */
> > +               HMT_low();
> > +               multi_threading_control = 0;
> > +               mtspr(SPRN_CTRLT,multi_threading_control);
>
> What is the purpose of the stack variable set to 0 every loop and
> only used as an argument to mtspr?

I can't see any. Max?

> > +       }
> > +       local_irq_disable();
>
> oops, local_irq_enable()

Ok. That makes it obvious that my test on the DD3 board actually did not get
here, so I also have another bug that disables all of the code in here.

> > +}
> > +
> > +static void pause_zero_idle(void)
> > +{
> > +       set_thread_flag(TIF_POLLING_NRFLAG);
> > +
>
> So your cpu keeps running, just slowly.  (This flag says don't bother
> to ipi the processor when there is work to do, it is polling waiting
> for work.   If you set this when you are sleeping then you will incurr
> wakeup latency until the next decrementer tick.

Ok, I'll investigate further. This sounds like a serious problem, but I would
expect to have observed some real performance hits as a result of it.

> > +       while (1) {
> > +               if (!need_resched()) {
> > +                       while (!need_resched()) {
> > +                               ppc64_runlatch_off();
> > +                               pause_zero();
> > +                               /*
> > +                                * Go into low thread priority and
> > possibly
> > +                                * low power mode.
> > +                                */
> > +                               HMT_low();
> > +                               HMT_very_low();
>
> Does your cpu support very_low?  Can it be disabled without disabling
> low?   If not, only one of these is needed.   The generic has both
> because some older cpus did not have very_low.  This code results in a
> periodic bump up to low on cpus that support both.
>
> And you set HMT_low in pauze_zero.

Hmm, I wonder if that might be one of the causes of the performance problems
we see on DD2 hardware (which can't do pauze_zero). Toggling between HMT_low
and HMT_very_low on one thread might cause problems in the pipeline for the
other thread in theory.
Which document would be the one telling me if HMT_very_low is supported?

	Arnd <><