Re: [RFC PATCH v1] powerpc/accounting: do not account system time on transition to user.

[Nicholas Piggin]

Christophe Leroy's on February 9, 2019 12:40 am:
> Time spent in kernel mode don't need to be accounted on transition
> to user space. As far as the time spent in user is known, it
> is possible to calculate the time spent in kernel by substracting
> the time spent in user.
> 
> To do so, this patch modifies vtime_delta() to substract the
> time spent in user since the last call to vtime_delta().
> 
> This patch gives a 2% improvment of null_syscall() selftest on a 83xx.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>

This looks like a clever little optimization, although I don't know
this time accounting code very well.

> ---
> But surprisingly, this patch degrades the null_syscall selftest by 20% on the 8xx. Any idea of the reason ?

I don't know microarchitecture of any of those CPUs I'm afraid.
On the 64s CPUs, mftb is what hurts.

> 
>  arch/powerpc/include/asm/accounting.h | 1 +
>  arch/powerpc/include/asm/ppc_asm.h    | 8 +-------
>  arch/powerpc/kernel/asm-offsets.c     | 8 ++------
>  arch/powerpc/kernel/time.c            | 4 +++-
>  4 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/accounting.h b/arch/powerpc/include/asm/accounting.h
> index c607c5d835cc..2f1ff5f9fd7a 100644
> --- a/arch/powerpc/include/asm/accounting.h
> +++ b/arch/powerpc/include/asm/accounting.h
> @@ -27,6 +27,7 @@ struct cpu_accounting_data {
>  	/* Internal counters */
>  	unsigned long starttime;	/* TB value snapshot */
>  	unsigned long starttime_user;	/* TB value on exit to usermode */
> +	unsigned long utime_asm;
>  #ifdef CONFIG_ARCH_HAS_SCALED_CPUTIME
>  	unsigned long startspurr;	/* SPURR value snapshot */
>  	unsigned long utime_sspurr;	/* ->user_time when ->startspurr set */
> diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
> index e0637730a8e7..be17d570d484 100644
> --- a/arch/powerpc/include/asm/ppc_asm.h
> +++ b/arch/powerpc/include/asm/ppc_asm.h
> @@ -28,9 +28,8 @@
>  #define ACCOUNT_STOLEN_TIME
>  #else
>  #define ACCOUNT_CPU_USER_ENTRY(ptr, ra, rb)				\
> -	MFTB(ra);			/* get timebase */		\
>  	PPC_LL	rb, ACCOUNT_STARTTIME_USER(ptr);			\
> -	PPC_STL	ra, ACCOUNT_STARTTIME(ptr);				\
> +	MFTB(ra);			/* get timebase */		\
>  	subf	rb,rb,ra;		/* subtract start value */	\
>  	PPC_LL	ra, ACCOUNT_USER_TIME(ptr);				\
>  	add	ra,ra,rb;		/* add on to user time */	\
> @@ -38,12 +37,7 @@
>  
>  #define ACCOUNT_CPU_USER_EXIT(ptr, ra, rb)				\
>  	MFTB(ra);			/* get timebase */		\
> -	PPC_LL	rb, ACCOUNT_STARTTIME(ptr);				\
>  	PPC_STL	ra, ACCOUNT_STARTTIME_USER(ptr);			\
> -	subf	rb,rb,ra;		/* subtract start value */	\
> -	PPC_LL	ra, ACCOUNT_SYSTEM_TIME(ptr);				\
> -	add	ra,ra,rb;		/* add on to system time */	\
> -	PPC_STL	ra, ACCOUNT_SYSTEM_TIME(ptr)
>  
>  #ifdef CONFIG_PPC_SPLPAR
>  #define ACCOUNT_STOLEN_TIME						\
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 7a1b93c5af63..f2ba7735f56f 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -260,19 +260,15 @@ int main(void)
>  	OFFSET(PACAHWCPUID, paca_struct, hw_cpu_id);
>  	OFFSET(PACAKEXECSTATE, paca_struct, kexec_state);
>  	OFFSET(PACA_DSCR_DEFAULT, paca_struct, dscr_default);
> -	OFFSET(ACCOUNT_STARTTIME, paca_struct, accounting.starttime);
>  	OFFSET(ACCOUNT_STARTTIME_USER, paca_struct, accounting.starttime_user);
> -	OFFSET(ACCOUNT_USER_TIME, paca_struct, accounting.utime);
> -	OFFSET(ACCOUNT_SYSTEM_TIME, paca_struct, accounting.stime);
> +	OFFSET(ACCOUNT_USER_TIME, paca_struct, accounting.utime_asm);
>  	OFFSET(PACA_TRAP_SAVE, paca_struct, trap_save);
>  	OFFSET(PACA_NAPSTATELOST, paca_struct, nap_state_lost);
>  	OFFSET(PACA_SPRG_VDSO, paca_struct, sprg_vdso);
>  #else /* CONFIG_PPC64 */
>  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
> -	OFFSET(ACCOUNT_STARTTIME, thread_info, accounting.starttime);
>  	OFFSET(ACCOUNT_STARTTIME_USER, thread_info, accounting.starttime_user);
> -	OFFSET(ACCOUNT_USER_TIME, thread_info, accounting.utime);
> -	OFFSET(ACCOUNT_SYSTEM_TIME, thread_info, accounting.stime);
> +	OFFSET(ACCOUNT_USER_TIME, thread_info, accounting.utime_asm);
>  #endif
>  #endif /* CONFIG_PPC64 */
>  
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index bc0503ef9c9c..79420643b45f 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -331,8 +331,10 @@ static unsigned long vtime_delta(struct task_struct *tsk,
>  	WARN_ON_ONCE(!irqs_disabled());
>  
>  	now = mftb();
> -	stime = now - acct->starttime;
> +	stime = now - acct->starttime - acct->utime_asm;
>  	acct->starttime = now;
> +	acct->utime += acct->utime_asm;
> +	acct->utime_asm = 0;
>  
>  	*stime_scaled = vtime_delta_scaled(acct, now, stime);
>  
> -- 
> 2.13.3
> 
>