On Wed, 2014-03-19 at 17:05 -0500, Scott Wood wrote:
> On Wed, 2014-03-19 at 22:52 +0100, Christophe Leroy wrote:
> > This patch provides VIRT_CPU_ACCOUTING to PPC32 architecture.
> > Unlike PPC64, PPC32 doesn't provide the PACA register. Therefore the
> > implementation is similar to the one done in the IA64 architecture.
> > It is based on additional information added to the Task Info structure.
>
> PACA isn't a register -- just a convention for how Linux uses a GPR.
> Maybe it's time to use it on PPC32 as well?
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* :-)
>
> > Index: b/arch/powerpc/kernel/asm-offsets.c
> > ===================================================================
> > --- b/arch/powerpc/kernel/asm-offsets.c (revision 5607)
> > +++ b/arch/powerpc/kernel/asm-offsets.c (revision 5608)
> > @@ -167,6 +167,10 @@
> > DEFINE(TI_PREEMPT, offsetof(struct thread_info, preempt_count));
> > DEFINE(TI_TASK, offsetof(struct thread_info, task));
> > DEFINE(TI_CPU, offsetof(struct thread_info, cpu));
> > + DEFINE(TI_AC_STAMP, offsetof(struct thread_info, ac_stamp));
> > + DEFINE(TI_AC_LEAVE, offsetof(struct thread_info, ac_leave));
> > + DEFINE(TI_AC_STIME, offsetof(struct thread_info, ac_stime));
> > + DEFINE(TI_AC_UTIME, offsetof(struct thread_info, ac_utime));
>
> Doesn't this need to be protected by #ifdef
> CONFIG_VIRT_CPU_ACCOUNTING_NATIVE?
>
> >
> > #ifdef CONFIG_PPC64
> > DEFINE(DCACHEL1LINESIZE, offsetof(struct ppc64_caches, dline_size));
> > Index: b/arch/powerpc/include/asm/thread_info.h
> > ===================================================================
> > --- b/arch/powerpc/include/asm/thread_info.h (revision 5607)
> > +++ b/arch/powerpc/include/asm/thread_info.h (revision 5608)
> > @@ -43,6 +43,12 @@
> > int cpu; /* cpu we're on */
> > int preempt_count; /* 0 => preemptable,
> > <0 => BUG */
> > +#ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
> > + __u32 ac_stamp;
> > + __u32 ac_leave;
> > + __u32 ac_stime;
> > + __u32 ac_utime;
> > +#endif
>
> This isn't uapi; why not use "u32"?
>
> Plus, it should be made clear that this is only used on 32-bit.
>
> > struct restart_block restart_block;
> > unsigned long local_flags; /* private flags for thread */
> >
> > @@ -58,6 +64,8 @@
> > .task = &tsk, \
> > .exec_domain = &default_exec_domain, \
> > .cpu = 0, \
> > + .ac_stime = 0, \
> > + .ac_utime = 0, \
>
> Also needs to be ifdeffed -- which isn't going to work in a macro, so
> maybe remove the ifdef from the variable declarations, or just let the
> fields be initialized to zero by default. Or add PACA to 32-bit. :-)
>
> -Scott
>
On Wed, 2014-04-30 at 14:56 +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2014-03-19 at 17:05 -0500, Scott Wood wrote:
> > On Wed, 2014-03-19 at 22:52 +0100, Christophe Leroy wrote:
> > > This patch provides VIRT_CPU_ACCOUTING to PPC32 architecture.
> > > Unlike PPC64, PPC32 doesn't provide the PACA register. Therefore the
> > > implementation is similar to the one done in the IA64 architecture.
> > > It is based on additional information added to the Task Info structure.
> >
> > PACA isn't a register -- just a convention for how Linux uses a GPR.
> > Maybe it's time to use it on PPC32 as well?
>
> 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* :-)
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?
-Scott