Re: [PATCH] powerpc/entry: Fix double accounting of user time on interrupt entry

On Wed, 2026-09-02 at 07:36 +0200, Christophe Leroy (CS GROUP) wrote:

Hi Christophe,

Thanks for reviewing the patch.

Le 02/09/2026 à 07:06, Aboorva Devarajan a écrit :

quoted

Since the switch to generic entry, an interrupt taken from user mode
accounts user time twice: once in arch_interrupt_enter_prepare() and
again in arch_enter_from_user_mode(), which irqentry_enter() invokes
for the same interrupt:

   arch_interrupt_enter_prepare()
       account_cpu_user_entry()
   irqentry_enter()
     arch_enter_from_user_mode()
       account_cpu_user_entry()

account_cpu_user_entry() accumulates the time spent in user mode
since the last return to user space, so the second call charges the
same interval again.

One of them was added by commit 09a9d3a8499d ("powerpc: introduce 
arch_enter_from_user_mode"), the other one by commit 893082ac769b 
("powerpc: Prepare for IRQ entry exit"). 

Would be good to mention it and explain how it went wrong.

Sure, I'll explain this in the commit message.

quoted

With CONFIG_VIRT_CPU_ACCOUNTING_NATIVE=y this roughly doubles the
reported user time of any workload that takes interrupts. On a
pseries LPAR, ps/top show ~200% CPU for a single-threaded CPU-bound
loop, and time(1) reports user time about twice the elapsed time.

Remove the accounting from arch_interrupt_enter_prepare() and rely on
arch_enter_from_user_mode(), which runs for both syscalls and
interrupts. The duplicate account_stolen_time() call is removed the
same way.

Fixes: bee25f97ad24 ("powerpc: Enable GENERIC_ENTRY feature")

Not sure it is the correct commit. Should probably be one of the ones
that introduced the wrong call.

The history is as follows:

Commit 09a9d3a8499d ("powerpc: introduce arch_enter_from_user_mode")
introduced the arch_enter_from_user_mode() function with the accounting,
but the function was not called at that point. Commit 893082ac769b
("powerpc: Prepare for IRQ entry exit") introduced arch_interrupt_enter_prepare()
and copied the accounting from interrupt_enter_prepare() into it, but the
new helper was not used either. Both changes were preparatory and did not
change the accounting behavior.

Commit bee25f97ad24 ("powerpc: Enable GENERIC_ENTRY feature") made both paths
active. The syscall path removed its open-coded account_cpu_user_entry() and
started accounting through arch_enter_from_user_mode(). But the interrupt path
started using arch_interrupt_enter_prepare() followed by irqentry_enter(),
which also invokes arch_enter_from_user_mode(), while the accounting in
arch_interrupt_enter_prepare() remained. This resulted in the same user-time
interval being accounted twice for interrupts taken from user mode.

I used bee25f97ad24 ("powerpc: Enable GENERIC_ENTRY feature") as the Fixes
tag because 893082ac769b only introduced the accounting call while
arch_interrupt_enter_prepare() was unused. bee25f97ad24 is where the duplicate
accounting actually became functional.

Would it be ok to keep bee25f97ad24 as the Fixes tag? Please let me know if
you think 893082ac769b would be more appropriate.

quoted

Signed-off-by: Aboorva Devarajan <redacted>
---
Verified on a pseries LPAR (CONFIG_VIRT_CPU_ACCOUNTING_NATIVE=y),
7.3.0-rc1, single-threaded CPU-bound loop:

Before:

   $ python3 -c 'while True: pass' &
   $ sleep 3; ps -p $! -o pid,etime,time,pcpu
       PID     ELAPSED     TIME %CPU
      4980       00:03 00:00:06  210

After:

   $ python3 -c 'while True: pass' &
   $ sleep 3; ps -p $! -o pid,etime,time,pcpu
       PID     ELAPSED     TIME %CPU
      4951       00:03 00:00:03  105

  arch/powerpc/include/asm/entry-common.h | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/entry-common.h b/arch/powerpc/include/asm/entry-common.h
index c5adb5006361..984294e62568 100644
--- a/arch/powerpc/include/asm/entry-common.h
+++ b/arch/powerpc/include/asm/entry-common.h

@@ -222,8 +222,6 @@ static inline void arch_interrupt_enter_prepare(struct pt_regs *regs)

  
  	if (user_mode(regs)) {
  		kuap_lock();
-		account_cpu_user_entry();
-		account_stolen_time();
  	} else {
  		kuap_save_and_lock(regs);
  		/*

@@ -426,6 +424,10 @@ static __always_inline void arch_enter_from_user_mode(struct pt_regs *regs)

  #endif
  	kuap_assert_locked();
  	booke_restore_dbcr0();
+	/*
+	 * User and stolen time is accounted here for every entry from
+	 * user mode. The interrupt prepare hooks must not account again.
+	 */

Don't polute the code with such a comment. This is to be explained in
the commit message. First a comment shall tell what code does, not what 
it doesn't do. And the comment has no added value, when you see 
account_cpu_user_entry() is called you already know that.

Here you feel the need to add such comment, but if you had implemented 
this function yourself and done it right at the first time, would you
have felt the need for this comment ?

Have a look at https://docs.kernel.org/process/coding-style.html#commenting

Agreed, thanks for pointing this out. I'll drop the comment in v2.

quoted

  	account_cpu_user_entry();
  	account_stolen_time();
  

base-commit: fb442a6673ff1046bf67754957d95880fdb394b5

Regards,
Aboorva