[PATCH] Shrink thread_info a bit

[PATCH] Shrink thread_info a bit

[Russell King - ARM Linux]

Thread info comes in on Versatile Express at 752 bytes in size, which
is quite large.  Of this, the crunch state is 184 bytes, which is only
used on Cirrus Logic devices.

It is wasteful to have this in every kernel when Cirrus Logic CPUs
are not that widely used.  So make this conditional.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/thread_info.h |    2 ++
 arch/arm/kernel/asm-offsets.c      |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 7b5cc8d..a030be7 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -59,7 +59,9 @@ struct thread_info {
 	__u32			syscall;	/* syscall number */
 	__u8			used_cp[16];	/* thread used copro */
 	unsigned long		tp_value;
+#ifdef CONFIG_CRUNCH
 	struct crunch_state	crunchstate;
+#endif
 	union fp_state		fpstate __attribute__((aligned(8)));
 	union vfp_state		vfpstate;
 #ifdef CONFIG_ARM_THUMBEE

[PATCH] Shrink thread_info a bit

[Baruch Siach]

Hi Russell,

On Mon, Oct 24, 2011 at 01:48:18PM +0100, Russell King - ARM Linux wrote:
> Thread info comes in on Versatile Express at 752 bytes in size, which
> is quite large.  Of this, the crunch state is 184 bytes, which is only
> used on Cirrus Logic devices.
> 
> It is wasteful to have this in every kernel when Cirrus Logic CPUs
> are not that widely used.  So make this conditional.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/include/asm/thread_info.h |    2 ++
>  arch/arm/kernel/asm-offsets.c      |    2 ++
>  2 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> index 7b5cc8d..a030be7 100644
> --- a/arch/arm/include/asm/thread_info.h
> +++ b/arch/arm/include/asm/thread_info.h
> @@ -59,7 +59,9 @@ struct thread_info {
>  	__u32			syscall;	/* syscall number */
>  	__u8			used_cp[16];	/* thread used copro */
>  	unsigned long		tp_value;
> +#ifdef CONFIG_CRUNCH
>  	struct crunch_state	crunchstate;
> +#endif
>  	union fp_state		fpstate __attribute__((aligned(8)));
>  	union vfp_state		vfpstate;
>  #ifdef CONFIG_ARM_THUMBEE

Where is the arch/arm/kernel/asm-offsets.c hunk?

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH] Shrink thread_info a bit

[Russell King - ARM Linux]

On Mon, Oct 24, 2011 at 03:06:19PM +0200, Baruch Siach wrote:
> Hi Russell,
> 
> On Mon, Oct 24, 2011 at 01:48:18PM +0100, Russell King - ARM Linux wrote:
> > Thread info comes in on Versatile Express at 752 bytes in size, which
> > is quite large.  Of this, the crunch state is 184 bytes, which is only
> > used on Cirrus Logic devices.
> > 
> > It is wasteful to have this in every kernel when Cirrus Logic CPUs
> > are not that widely used.  So make this conditional.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  arch/arm/include/asm/thread_info.h |    2 ++
> >  arch/arm/kernel/asm-offsets.c      |    2 ++
> >  2 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> > index 7b5cc8d..a030be7 100644
> > --- a/arch/arm/include/asm/thread_info.h
> > +++ b/arch/arm/include/asm/thread_info.h
> > @@ -59,7 +59,9 @@ struct thread_info {
> >  	__u32			syscall;	/* syscall number */
> >  	__u8			used_cp[16];	/* thread used copro */
> >  	unsigned long		tp_value;
> > +#ifdef CONFIG_CRUNCH
> >  	struct crunch_state	crunchstate;
> > +#endif
> >  	union fp_state		fpstate __attribute__((aligned(8)));
> >  	union vfp_state		vfpstate;
> >  #ifdef CONFIG_ARM_THUMBEE
> 
> Where is the arch/arm/kernel/asm-offsets.c hunk?

Have you looked there?

[PATCH] Shrink thread_info a bit

[Will Deacon]

Hi Russell,

On Mon, Oct 24, 2011 at 01:48:18PM +0100, Russell King - ARM Linux wrote:
> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> index 7b5cc8d..a030be7 100644
> --- a/arch/arm/include/asm/thread_info.h
> +++ b/arch/arm/include/asm/thread_info.h
> @@ -59,7 +59,9 @@ struct thread_info {
>  	__u32			syscall;	/* syscall number */
>  	__u8			used_cp[16];	/* thread used copro */
>  	unsigned long		tp_value;
> +#ifdef CONFIG_CRUNCH
>  	struct crunch_state	crunchstate;
> +#endif
>  	union fp_state		fpstate __attribute__((aligned(8)));

Can we also shrink the contents of fp_state when AEABI && !OABI_COMPAT?
It's slightly more involved as ptrace and core-dumping would need some
similar treatment. We probably need to keep the union kicking around
though, since the iwmmxt state is held in there.

Will

[PATCH] Shrink thread_info a bit

[Russell King - ARM Linux]

On Mon, Oct 24, 2011 at 02:18:53PM +0100, Will Deacon wrote:
> Hi Russell,
> 
> On Mon, Oct 24, 2011 at 01:48:18PM +0100, Russell King - ARM Linux wrote:
> > diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> > index 7b5cc8d..a030be7 100644
> > --- a/arch/arm/include/asm/thread_info.h
> > +++ b/arch/arm/include/asm/thread_info.h
> > @@ -59,7 +59,9 @@ struct thread_info {
> >  	__u32			syscall;	/* syscall number */
> >  	__u8			used_cp[16];	/* thread used copro */
> >  	unsigned long		tp_value;
> > +#ifdef CONFIG_CRUNCH
> >  	struct crunch_state	crunchstate;
> > +#endif
> >  	union fp_state		fpstate __attribute__((aligned(8)));
> 
> Can we also shrink the contents of fp_state when AEABI && !OABI_COMPAT?
> It's slightly more involved as ptrace and core-dumping would need some
> similar treatment. We probably need to keep the union kicking around
> though, since the iwmmxt state is held in there.

This patch is the nice easy bit - doing the same with the fpstate would
be much more involved because thre's also bits of assembly which would
be impacted as well.

[PATCH] Shrink thread_info a bit

[Tim Bird]

On 10/24/2011 5:48 AM, Russell King - ARM Linux wrote:
> Thread info comes in on Versatile Express at 752 bytes in size, which
> is quite large.  Of this, the crunch state is 184 bytes, which is only
> used on Cirrus Logic devices.
>
> It is wasteful to have this in every kernel when Cirrus Logic CPUs
> are not that widely used.  So make this conditional.
>
> Signed-off-by: Russell King<rmk+kernel@arm.linux.org.uk>
> ---
>   arch/arm/include/asm/thread_info.h |    2 ++
>   arch/arm/kernel/asm-offsets.c      |    2 ++
>   2 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> index 7b5cc8d..a030be7 100644
> --- a/arch/arm/include/asm/thread_info.h
> +++ b/arch/arm/include/asm/thread_info.h
> @@ -59,7 +59,9 @@ struct thread_info {
>   	__u32			syscall;	/* syscall number */
>   	__u8			used_cp[16];	/* thread used copro */
>   	unsigned long		tp_value;
> +#ifdef CONFIG_CRUNCH
>   	struct crunch_state	crunchstate;
> +#endif
>   	union fp_state		fpstate __attribute__((aligned(8)));
>   	union vfp_state		vfpstate;
>   #ifdef CONFIG_ARM_THUMBEE
>
>
Since the asm-offsets.h code already has the offset definitions under
#ifdef CONFIG_CRUNCH, this qualifies as a code consistency cleanup,
as well as a memory savings.

I looked at this, and can find no code outside of #ifdef CONFIG_CRUNCH
that references this - so FWIW
Reviewed-by Tim Bird <tim.bird@am.sony.com>

  -- Tim

[PATCH] Shrink thread_info a bit

[Dave Martin]

On Mon, Oct 24, 2011 at 02:18:53PM +0100, Will Deacon wrote:
> Hi Russell,
> 
> On Mon, Oct 24, 2011 at 01:48:18PM +0100, Russell King - ARM Linux wrote:
> > diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> > index 7b5cc8d..a030be7 100644
> > --- a/arch/arm/include/asm/thread_info.h
> > +++ b/arch/arm/include/asm/thread_info.h
> > @@ -59,7 +59,9 @@ struct thread_info {
> >  	__u32			syscall;	/* syscall number */
> >  	__u8			used_cp[16];	/* thread used copro */
> >  	unsigned long		tp_value;
> > +#ifdef CONFIG_CRUNCH
> >  	struct crunch_state	crunchstate;
> > +#endif
> >  	union fp_state		fpstate __attribute__((aligned(8)));
> 
> Can we also shrink the contents of fp_state when AEABI && !OABI_COMPAT?

Is this the right config combination to indicate that the FPA ABI will
not be supported?  I'm assuming so, since that's the condition which
currently disallows CONFIG_NWFPE.

I'm not saying it's not ... I'm just unsure.

Note that the FPA state (35 words) is not that much smaller than the
iwmmxt state (38 words), so not much space would be saved by shrinking
that union.

I wonder whether fpstate can go away completely on platforms which have
neither fpa nor iwmmxt however.  That would be a more significant saving.
Is it used for anything else?

> It's slightly more involved as ptrace and core-dumping would need some
> similar treatment. We probably need to keep the union kicking around
> though, since the iwmmxt state is held in there.

We would need some sensible error return from the ptrace calls to access
the FPA registers.  PTRACE_GETREGSET (not currently working on ARM since
the relevant generic code is #ifdef'd on CONFIG_HAVE_ARCH_TRACEHOOK for
some reason) returns -EINVAL when requesting a regset that isn't there.

Fixing coredumps should be relatively easy -- I think we just need to
change arm_regsets[] in arch/arm/kernel/ptrace.c appropriately.

Cheers
---Dave

[PATCH] Shrink thread_info a bit

[Will Deacon]

Hi Dave,

On Tue, Oct 25, 2011 at 03:41:11PM +0100, Dave Martin wrote:
> On Mon, Oct 24, 2011 at 02:18:53PM +0100, Will Deacon wrote:
> > 
> > Can we also shrink the contents of fp_state when AEABI && !OABI_COMPAT?
> 
> Is this the right config combination to indicate that the FPA ABI will
> not be supported?  I'm assuming so, since that's the condition which
> currently disallows CONFIG_NWFPE.
> 
> I'm not saying it's not ... I'm just unsure.

Well, we basically just want to check whether or not we might have OABI
executables in userspace so I think this is the right way of doing that.
I toyed with doing something like !CONFIG_VFP but that doesn't help with the
compat stuff.

> Note that the FPA state (35 words) is not that much smaller than the
> iwmmxt state (38 words), so not much space would be saved by shrinking
> that union.

Right, but the IWMMXT part of the union is already predicated on
CONFIG_IWMMXT so the goal is to have an empty union for the common case
(EABI and no IWMMXT).

> I wonder whether fpstate can go away completely on platforms which have
> neither fpa nor iwmmxt however.  That would be a more significant saving.
> Is it used for anything else?

That's kind of the idea, but I was just going to leave it empty.

> > It's slightly more involved as ptrace and core-dumping would need some
> > similar treatment. We probably need to keep the union kicking around
> > though, since the iwmmxt state is held in there.
> 
> We would need some sensible error return from the ptrace calls to access
> the FPA registers.  PTRACE_GETREGSET (not currently working on ARM since
> the relevant generic code is #ifdef'd on CONFIG_HAVE_ARCH_TRACEHOOK for
> some reason) returns -EINVAL when requesting a regset that isn't there.
> 
> Fixing coredumps should be relatively easy -- I think we just need to
> change arm_regsets[] in arch/arm/kernel/ptrace.c appropriately.

Russell also pointed out that we need to hack some assembly code for this to
work, but I can't say I've looked into it yet.

Will