Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 19 Feb 2019 15:41:38 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [RFC PATCH v1] powerpc/accounting: do not account system time on
 transition to user.
To:     Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Christophe Leroy <christophe.leroy@c-s.fr>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Paul Mackerras <paulus@samba.org>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
References: <a952f14712b90f182c0fcace434b2a0e9f21ed9f.1549636481.git.christophe.leroy@c-s.fr>
In-Reply-To: <a952f14712b90f182c0fcace434b2a0e9f21ed9f.1549636481.git.christophe.leroy@c-s.fr>
MIME-Version: 1.0
User-Agent: astroid/0.14.0 (https://github.com/astroidmail/astroid)
Message-Id: <1550554276.8eb89h3g4q.astroid@bobo.none>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.
>=20
> To do so, this patch modifies vtime_delta() to substract the
> time spent in user since the last call to vtime_delta().
>=20
> This patch gives a 2% improvment of null_syscall() selftest on a 83xx.
>=20
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>

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.

>=20
>  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(-)
>=20
> 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/as=
m/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 @@
> =20
>  #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)
> =20
>  #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 */
> =20
> 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());
> =20
>  	now =3D mftb();
> -	stime =3D now - acct->starttime;
> +	stime =3D now - acct->starttime - acct->utime_asm;
>  	acct->starttime =3D now;
> +	acct->utime +=3D acct->utime_asm;
> +	acct->utime_asm =3D 0;
> =20
>  	*stime_scaled =3D vtime_delta_scaled(acct, now, stime);
> =20
> --=20
> 2.13.3
>=20
>=20
=