Received-SPF: pass (google.com: domain of linux-kernel+bounces-58986-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Fri, 09 Feb 2024 13:36:08 +1000
Message-Id: <CZ08FF0GBT76.1FZS9YFEMKSOM@wheely>
Cc: <linux-kernel@vger.kernel.org>, <linux-s390@vger.kernel.org>,
 <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH v2 2/5] sched/vtime: get rid of generic
 vtime_task_switch() implementation
From: "Nicholas Piggin" <npiggin@gmail.com>
To: "Alexander Gordeev" <agordeev@linux.ibm.com>, "Frederic Weisbecker"
 <frederic@kernel.org>, "Ingo Molnar" <mingo@kernel.org>, "Michael Ellerman"
 <mpe@ellerman.id.au>, "Heiko Carstens" <hca@linux.ibm.com>, "Vasily Gorbik"
 <gor@linux.ibm.com>
References: <cover.1707422448.git.agordeev@linux.ibm.com>
 <2baebb95af9772959a0d85d0bd789afdfd2baf94.1707422448.git.agordeev@linux.ibm.com>
In-Reply-To: <2baebb95af9772959a0d85d0bd789afdfd2baf94.1707422448.git.agordeev@linux.ibm.com>

On Fri Feb 9, 2024 at 6:15 AM AEST, Alexander Gordeev wrote:
> The generic vtime_task_switch() implementation gets built only
> if __ARCH_HAS_VTIME_TASK_SWITCH is not defined, but requires an
> architecture to implement arch_vtime_task_switch() callback at
> the same time, which is confusing.
>
> Further, arch_vtime_task_switch() is implemented for 32-bit PowerPC
> architecture only and vtime_task_switch() generic variant is rather
> superfluous.
>
> Simplify the whole vtime_task_switch() wiring by moving the existing
> generic implementation to PowerPC.

I think this could be squashed with patch 3.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/cputime.h | 13 -------------
>  arch/powerpc/kernel/time.c         | 22 ++++++++++++++++++++++
>  kernel/sched/cputime.c             | 13 -------------
>  3 files changed, 22 insertions(+), 26 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/cputime.h b/arch/powerpc/include/as=
m/cputime.h
> index 4961fb38e438..aff858ca99c0 100644
> --- a/arch/powerpc/include/asm/cputime.h
> +++ b/arch/powerpc/include/asm/cputime.h
> @@ -32,23 +32,10 @@
>  #ifdef CONFIG_PPC64
>  #define get_accounting(tsk)	(&get_paca()->accounting)
>  #define raw_get_accounting(tsk)	(&local_paca->accounting)
> -static inline void arch_vtime_task_switch(struct task_struct *tsk) { }
> =20
>  #else
>  #define get_accounting(tsk)	(&task_thread_info(tsk)->accounting)
>  #define raw_get_accounting(tsk)	get_accounting(tsk)
> -/*
> - * Called from the context switch with interrupts disabled, to charge al=
l
> - * accumulated times to the current process, and to prepare accounting o=
n
> - * the next process.
> - */
> -static inline void arch_vtime_task_switch(struct task_struct *prev)
> -{
> -	struct cpu_accounting_data *acct =3D get_accounting(current);
> -	struct cpu_accounting_data *acct0 =3D get_accounting(prev);
> -
> -	acct->starttime =3D acct0->starttime;
> -}
>  #endif
> =20
>  /*
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index df20cf201f74..c0fdc6d94fee 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -354,6 +354,28 @@ void vtime_flush(struct task_struct *tsk)
>  	acct->hardirq_time =3D 0;
>  	acct->softirq_time =3D 0;
>  }
> +
> +/*
> + * Called from the context switch with interrupts disabled, to charge al=
l
> + * accumulated times to the current process, and to prepare accounting o=
n
> + * the next process.
> + */
> +void vtime_task_switch(struct task_struct *prev)
> +{
> +	if (is_idle_task(prev))
> +		vtime_account_idle(prev);
> +	else
> +		vtime_account_kernel(prev);
> +
> +	vtime_flush(prev);
> +
> +	if (!IS_ENABLED(CONFIG_PPC64)) {
> +		struct cpu_accounting_data *acct =3D get_accounting(current);
> +		struct cpu_accounting_data *acct0 =3D get_accounting(prev);
> +
> +		acct->starttime =3D acct0->starttime;
> +	}
> +}
>  #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
> =20
>  void __no_kcsan __delay(unsigned long loops)
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index af7952f12e6c..aa48b2ec879d 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -424,19 +424,6 @@ static inline void irqtime_account_process_tick(stru=
ct task_struct *p, int user_
>   */
>  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
> =20
> -# ifndef __ARCH_HAS_VTIME_TASK_SWITCH
> -void vtime_task_switch(struct task_struct *prev)
> -{
> -	if (is_idle_task(prev))
> -		vtime_account_idle(prev);
> -	else
> -		vtime_account_kernel(prev);
> -
> -	vtime_flush(prev);
> -	arch_vtime_task_switch(prev);
> -}
> -# endif
> -
>  void vtime_account_irq(struct task_struct *tsk, unsigned int offset)
>  {
>  	unsigned int pc =3D irq_count() - offset;