Messages in this thread | | | Date | Fri, 03 Apr 2020 16:04:56 +0530 | From | "Naveen N. Rao" <> | Subject | Re: [PATCH v4 2/6] powerpc/idle: Add accessor function to always read latest idle PURR |
| |
Gautham R Shenoy wrote: > On Wed, Apr 01, 2020 at 03:12:53PM +0530, Naveen N. Rao wrote: >> Hi Gautham, >> >> Gautham R. Shenoy wrote: >> >From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> >> > >> >+ >> >+static inline u64 read_this_idle_purr(void) >> >+{ >> >+ /* >> >+ * If we are reading from an idle context, update the >> >+ * idle-purr cycles corresponding to the last idle period. >> >+ * Since the idle context is not yet over, take a fresh >> >+ * snapshot of the idle-purr. >> >+ */ >> >+ if (unlikely(get_lppaca()->idle == 1)) { >> >+ update_idle_purr_accounting(); >> >+ snapshot_purr_idle_entry(); >> >+ } >> >+ >> >+ return be64_to_cpu(get_lppaca()->wait_state_cycles); >> >+} >> >+ >> >> I think this and read_this_idle_spurr() from the next patch should be moved >> to Patch 4/6, where they are actually used. > > The reason I included this function in this patch was to justify why > we were introducing snapshotting the purr values in a global per-cpu > variable instead of on a stack variable. The reason being that someone > might want to read the PURR value from an interrupt context which had > woken up the CPU from idle. At this point, since epilog() function > wasn't called, the idle PURR count corresponding to this latest idle > period would have been accumulated in lppaca->wait_cycles. Thus, this > helper function safely reads the value by > 1) First updating the lppaca->wait_cycles with the latest idle_purr > count. > 2) Take a fresh snapshot, since the time from now to the epilog() > call is also counted under idle CPU. So the PURR cycle increment > during this short period should also be accumulated in lppaca->wait_cycles. > > > prolog() > | snapshot PURR > | > | > | > Idle > | > | <----- Interrupt . Read idle PURR ---- update idle PURR; > | snapshot PURR; > | Read idle PURR. > | > epilog() > update idle PURR >
Yes, I understand. It makes sense.
> > However, if you feel that moving this function to Patch 4 where it is > actually used makes it more readable, I can do that.
My suggestion was from a bisectability standpoint though. This is a fairly simple function, but it is generally recommended to ensure that newly added code gets exercized in the patch that it is introduced in: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/5.Posting.rst#n119
Regards, Naveen
| |