Re: [PATCH v5] powerpc32: provide VIRT_CPU_ACCOUNTING
From: Scott Wood <oss@buserror.net>
Date: 2016-02-16 21:21:15
Also in:
lkml
On Thu, 2016-02-11 at 17:16 +0100, Christophe Leroy wrote:
This patch provides VIRT_CPU_ACCOUTING to PPC32 architecture. PPC32 doesn't have the PACA structure, so we use the task_info structure to store the accounting data. In order to reuse on PPC32 the PPC64 functions, all u64 data has been replaced by 'unsigned long' so that it is u32 on PPC32 and u64 on PPC64 Signed-off-by: Christophe Leroy <redacted> --- Changes in v3: unlike previous version of the patch that was inspired from IA64 architecture, this new version tries to reuse as much as possible the PPC64 implementation. PPC32 doesn't have PACA and past discusion on v2 version has shown that it is not worth implementing a PACA in PPC32 architecture (see below benh opinion) benh: PACA is actually a data structure and you really really don't want it on ppc32 :-) Having a register point to current works, having a register point to per-cpu data instead works too (ie, change what we do today), but don't introduce a PACA *please* :-)
And Ben never replied to my reply at the time: "What is special about 64-bit that warrants doing things differently from 32 -bit? What is the difference between PACA and "per-cpu data", other than the obscure name?" I can understand wanting to avoid churn, but other than that, doing things differently on 64-bit versus 32-bit sucks.
quoted hunk ↗ jump to hunk
b/arch/powerpc/include/asm/cputime.h index e245255..c4c33be 100644--- a/arch/powerpc/include/asm/cputime.h +++ b/arch/powerpc/include/asm/cputime.h@@ -230,7 +230,11 @@ static inline cputime_t clock_t_to_cputime(constunsigned long clk) #define cputime64_to_clock_t(ct) cputime_to_clock_t((cputime_t)(ct)) +#ifdef CONFIG_PPC64 static inline void arch_vtime_task_switch(struct task_struct *tsk) { } +#else +void arch_vtime_task_switch(struct task_struct *tsk); +#endif
Add a comment explaining why this is empty on 64-bit but non-empty on 32-bit.
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm -offsets.c index 07cebc3..b04b957 100644 --- a/arch/powerpc/kernel/asm-offsets.c +++ b/arch/powerpc/kernel/asm-offsets.c@@ -256,6 +256,13 @@ int main(void) DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save)); DEFINE(PACA_NAPSTATELOST, offsetof(struct paca_struct,nap_state_lost)); DEFINE(PACA_SPRG_VDSO, offsetof(struct paca_struct, sprg_vdso)); +#else /* CONFIG_PPC64 */ +#ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE + DEFINE(PACA_STARTTIME, offsetof(struct thread_info, starttime)); + DEFINE(PACA_STARTTIME_USER, offsetof(struct thread_info, starttime_user)); + DEFINE(PACA_USER_TIME, offsetof(struct thread_info, user_time)); + DEFINE(PACA_SYSTEM_TIME, offsetof(struct thread_info, system_time)); +#endif #endif /* CONFIG_PPC64 */
Can you change the name if it's not always going to be relative to a PACA?
+#ifdef CONFIG_PPC32 +#define get_paca() task_thread_info(tsk) +#endif
Likewise, this is just going to cause confusion. Can you bundle up the time accounting fields into a struct, that you share between the paca and the 32-bit thread_info, and then have a macro to grab a pointer to that? -Scott