* [PATCH 09/30] x86_64: ifdef out struct thread_struct::ip
@ 2009-04-10 2:35 Alexey Dobriyan
2009-04-10 3:53 ` Matt Helsley
0 siblings, 1 reply; 8+ messages in thread
From: Alexey Dobriyan @ 2009-04-10 2:35 UTC (permalink / raw)
To: akpm, containers
Cc: xemul, serue, dave, mingo, orenl, hch, torvalds, linux-kernel
struct thread_struct::ip isn't used on x86_64, struct pt_regs::ip is used
instead.
kgdb should be reading 0, but I can't check it.
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
arch/x86/include/asm/processor.h | 2 ++
arch/x86/kernel/kgdb.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -421,7 +421,9 @@ struct thread_struct {
unsigned short fsindex;
unsigned short gsindex;
#endif
+#ifdef CONFIG_X86_32
unsigned long ip;
+#endif
#ifdef CONFIG_X86_64
unsigned long fs;
#endif
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -141,7 +141,7 @@ void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
gdb_regs32[GDB_PS] = *(unsigned long *)(p->thread.sp + 8);
gdb_regs32[GDB_CS] = __KERNEL_CS;
gdb_regs32[GDB_SS] = __KERNEL_DS;
- gdb_regs[GDB_PC] = p->thread.ip;
+ gdb_regs[GDB_PC] = 0;
gdb_regs[GDB_R8] = 0;
gdb_regs[GDB_R9] = 0;
gdb_regs[GDB_R10] = 0;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 09/30] x86_64: ifdef out struct thread_struct::ip
2009-04-10 2:35 [PATCH 09/30] x86_64: ifdef out struct thread_struct::ip Alexey Dobriyan
@ 2009-04-10 3:53 ` Matt Helsley
2009-04-10 4:10 ` Jaswinder Singh Rajput
2009-04-10 9:19 ` Ingo Molnar
0 siblings, 2 replies; 8+ messages in thread
From: Matt Helsley @ 2009-04-10 3:53 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: akpm, containers, xemul, linux-kernel, dave, hch, mingo, torvalds
On Fri, Apr 10, 2009 at 06:35:22AM +0400, Alexey Dobriyan wrote:
> struct thread_struct::ip isn't used on x86_64, struct pt_regs::ip is used
> instead.
>
> kgdb should be reading 0, but I can't check it.
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
> arch/x86/include/asm/processor.h | 2 ++
> arch/x86/kernel/kgdb.c | 2 +-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -421,7 +421,9 @@ struct thread_struct {
> unsigned short fsindex;
> unsigned short gsindex;
> #endif
> +#ifdef CONFIG_X86_32
> unsigned long ip;
> +#endif
> #ifdef CONFIG_X86_64
> unsigned long fs;
> #endif
Do these make struct thread_struct behave better in cachelines (smaller,
less aliasing)? Can we really fit more in the slab du jour?
Otherwise it seems like we're littering these structs with #ifdefs
and not really saving anything. If these #ifdefs don't save any space why not
just put in a comment:
> unsigned long ip; /* Used only on i386 */
Or maybe even:
union {
unsigned long ip; /* Used only on i386 */
unsigned long fs; /* Used only on x86_64 */
};
Cheers,
-Matt Helsley
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 09/30] x86_64: ifdef out struct thread_struct::ip
2009-04-10 3:53 ` Matt Helsley
@ 2009-04-10 4:10 ` Jaswinder Singh Rajput
2009-04-10 9:20 ` Ingo Molnar
2009-04-10 9:19 ` Ingo Molnar
1 sibling, 1 reply; 8+ messages in thread
From: Jaswinder Singh Rajput @ 2009-04-10 4:10 UTC (permalink / raw)
To: Matt Helsley
Cc: Alexey Dobriyan, akpm, containers, xemul, linux-kernel, dave,
hch, mingo, torvalds
On Thu, 2009-04-09 at 20:53 -0700, Matt Helsley wrote:
> On Fri, Apr 10, 2009 at 06:35:22AM +0400, Alexey Dobriyan wrote:
> > struct thread_struct::ip isn't used on x86_64, struct pt_regs::ip is used
> > instead.
> >
> > kgdb should be reading 0, but I can't check it.
> >
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > ---
> >
> > arch/x86/include/asm/processor.h | 2 ++
> > arch/x86/kernel/kgdb.c | 2 +-
> > 2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > --- a/arch/x86/include/asm/processor.h
> > +++ b/arch/x86/include/asm/processor.h
> > @@ -421,7 +421,9 @@ struct thread_struct {
> > unsigned short fsindex;
> > unsigned short gsindex;
> > #endif
> > +#ifdef CONFIG_X86_32
> > unsigned long ip;
> > +#endif
> > #ifdef CONFIG_X86_64
> > unsigned long fs;
> > #endif
>
> Do these make struct thread_struct behave better in cachelines (smaller,
> less aliasing)? Can we really fit more in the slab du jour?
>
> Otherwise it seems like we're littering these structs with #ifdefs
> and not really saving anything. If these #ifdefs don't save any space why not
> just put in a comment:
>
> > unsigned long ip; /* Used only on i386 */
>
> Or maybe even:
>
> union {
> unsigned long ip; /* Used only on i386 */
> unsigned long fs; /* Used only on x86_64 */
> };
>
Can we do it like this:
unsigned long ip_fs; /* ip: i386, fs: x86_64 */
I am using same variable for both cases, or we can use some better name than ip_fs.
I am assuming either it is i386 or x86_64 machine ;-)
--
JSR
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 09/30] x86_64: ifdef out struct thread_struct::ip
2009-04-10 3:53 ` Matt Helsley
2009-04-10 4:10 ` Jaswinder Singh Rajput
@ 2009-04-10 9:19 ` Ingo Molnar
2009-04-10 9:47 ` Ingo Molnar
2009-04-10 11:17 ` Alexey Dobriyan
1 sibling, 2 replies; 8+ messages in thread
From: Ingo Molnar @ 2009-04-10 9:19 UTC (permalink / raw)
To: Matt Helsley, Tejun Heo, Jeremy Fitzhardinge
Cc: Alexey Dobriyan, akpm, containers, xemul, linux-kernel, dave,
hch, torvalds
* Matt Helsley <matthltc@us.ibm.com> wrote:
> On Fri, Apr 10, 2009 at 06:35:22AM +0400, Alexey Dobriyan wrote:
> > struct thread_struct::ip isn't used on x86_64, struct pt_regs::ip is used
> > instead.
> >
> > kgdb should be reading 0, but I can't check it.
> >
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > ---
> >
> > arch/x86/include/asm/processor.h | 2 ++
> > arch/x86/kernel/kgdb.c | 2 +-
> > 2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > --- a/arch/x86/include/asm/processor.h
> > +++ b/arch/x86/include/asm/processor.h
> > @@ -421,7 +421,9 @@ struct thread_struct {
> > unsigned short fsindex;
> > unsigned short gsindex;
> > #endif
> > +#ifdef CONFIG_X86_32
> > unsigned long ip;
> > +#endif
> > #ifdef CONFIG_X86_64
> > unsigned long fs;
> > #endif
>
> Do these make struct thread_struct behave better in cachelines
> (smaller, less aliasing)? Can we really fit more in the slab du
> jour?
>
> Otherwise it seems like we're littering these structs with #ifdefs
> and not really saving anything. [...]
Removing fields always saves memory (even if it does not show up
currently due to allocators cache-aligning sizes).
But the #ifdef ugliness is a real worry.
> [...] If these #ifdefs don't save any
> space why not just put in a comment:
>
> > unsigned long ip; /* Used only on i386 */
Yes.
> Or maybe even:
>
> union {
> unsigned long ip; /* Used only on i386 */
> unsigned long fs; /* Used only on x86_64 */
> };
Maybe. If this ever gets misunderstood somewhere in platform code we
will get ugly failure modes and zero compiler help.
Ingo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 09/30] x86_64: ifdef out struct thread_struct::ip
2009-04-10 4:10 ` Jaswinder Singh Rajput
@ 2009-04-10 9:20 ` Ingo Molnar
2009-04-10 10:52 ` Jaswinder Singh Rajput
0 siblings, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2009-04-10 9:20 UTC (permalink / raw)
To: Jaswinder Singh Rajput
Cc: Matt Helsley, Alexey Dobriyan, akpm, containers, xemul,
linux-kernel, dave, hch, torvalds
* Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:
> On Thu, 2009-04-09 at 20:53 -0700, Matt Helsley wrote:
> > On Fri, Apr 10, 2009 at 06:35:22AM +0400, Alexey Dobriyan wrote:
> > > struct thread_struct::ip isn't used on x86_64, struct pt_regs::ip is used
> > > instead.
> > >
> > > kgdb should be reading 0, but I can't check it.
> > >
> > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > > ---
> > >
> > > arch/x86/include/asm/processor.h | 2 ++
> > > arch/x86/kernel/kgdb.c | 2 +-
> > > 2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > --- a/arch/x86/include/asm/processor.h
> > > +++ b/arch/x86/include/asm/processor.h
> > > @@ -421,7 +421,9 @@ struct thread_struct {
> > > unsigned short fsindex;
> > > unsigned short gsindex;
> > > #endif
> > > +#ifdef CONFIG_X86_32
> > > unsigned long ip;
> > > +#endif
> > > #ifdef CONFIG_X86_64
> > > unsigned long fs;
> > > #endif
> >
> > Do these make struct thread_struct behave better in cachelines (smaller,
> > less aliasing)? Can we really fit more in the slab du jour?
> >
> > Otherwise it seems like we're littering these structs with #ifdefs
> > and not really saving anything. If these #ifdefs don't save any space why not
> > just put in a comment:
> >
> > > unsigned long ip; /* Used only on i386 */
> >
> > Or maybe even:
> >
> > union {
> > unsigned long ip; /* Used only on i386 */
> > unsigned long fs; /* Used only on x86_64 */
> > };
> >
>
> Can we do it like this:
> unsigned long ip_fs; /* ip: i386, fs: x86_64 */
>
> I am using same variable for both cases, or we can use some better
> name than ip_fs. I am assuming either it is i386 or x86_64 machine
> ;-)
This is the least clean variant amongst all the suggestions.
Ingo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 09/30] x86_64: ifdef out struct thread_struct::ip
2009-04-10 9:19 ` Ingo Molnar
@ 2009-04-10 9:47 ` Ingo Molnar
2009-04-10 11:17 ` Alexey Dobriyan
1 sibling, 0 replies; 8+ messages in thread
From: Ingo Molnar @ 2009-04-10 9:47 UTC (permalink / raw)
To: Matt Helsley, Tejun Heo, Jeremy Fitzhardinge
Cc: Alexey Dobriyan, akpm, containers, xemul, linux-kernel, dave,
hch, torvalds
* Ingo Molnar <mingo@elte.hu> wrote:
>
> * Matt Helsley <matthltc@us.ibm.com> wrote:
>
> > On Fri, Apr 10, 2009 at 06:35:22AM +0400, Alexey Dobriyan wrote:
> > > struct thread_struct::ip isn't used on x86_64, struct pt_regs::ip is used
> > > instead.
> > >
> > > kgdb should be reading 0, but I can't check it.
> > >
> > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > > ---
> > >
> > > arch/x86/include/asm/processor.h | 2 ++
> > > arch/x86/kernel/kgdb.c | 2 +-
> > > 2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > --- a/arch/x86/include/asm/processor.h
> > > +++ b/arch/x86/include/asm/processor.h
> > > @@ -421,7 +421,9 @@ struct thread_struct {
> > > unsigned short fsindex;
> > > unsigned short gsindex;
> > > #endif
> > > +#ifdef CONFIG_X86_32
> > > unsigned long ip;
> > > +#endif
> > > #ifdef CONFIG_X86_64
> > > unsigned long fs;
> > > #endif
> >
> > Do these make struct thread_struct behave better in cachelines
> > (smaller, less aliasing)? Can we really fit more in the slab du
> > jour?
> >
> > Otherwise it seems like we're littering these structs with #ifdefs
> > and not really saving anything. [...]
>
> Removing fields always saves memory (even if it does not show up
> currently due to allocators cache-aligning sizes).
I mean: we always try to consider structure size reductions as if
they saved real memory, even if they dont do so in the current
layout and allocation patterns.
In reality only every 8th 8-byte reduction will result in a true
64-byte cacheline reduction - but still we have to continue small
reductions otherwise we wont ever get to the larger reductions in
the first place.
I.e. Alexey's patch shows a real item worth checking, regardless of
the current size and alignment scenario.
Ingo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 09/30] x86_64: ifdef out struct thread_struct::ip
2009-04-10 9:20 ` Ingo Molnar
@ 2009-04-10 10:52 ` Jaswinder Singh Rajput
0 siblings, 0 replies; 8+ messages in thread
From: Jaswinder Singh Rajput @ 2009-04-10 10:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Matt Helsley, Alexey Dobriyan, akpm, containers, xemul,
linux-kernel, dave, hch, torvalds
On Fri, 2009-04-10 at 11:20 +0200, Ingo Molnar wrote:
> * Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:
>
> > On Thu, 2009-04-09 at 20:53 -0700, Matt Helsley wrote:
> > > On Fri, Apr 10, 2009 at 06:35:22AM +0400, Alexey Dobriyan wrote:
> > > > struct thread_struct::ip isn't used on x86_64, struct pt_regs::ip is used
> > > > instead.
> > > >
> > > > kgdb should be reading 0, but I can't check it.
> > > >
> > > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > > > ---
> > > >
> > > > arch/x86/include/asm/processor.h | 2 ++
> > > > arch/x86/kernel/kgdb.c | 2 +-
> > > > 2 files changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > --- a/arch/x86/include/asm/processor.h
> > > > +++ b/arch/x86/include/asm/processor.h
> > > > @@ -421,7 +421,9 @@ struct thread_struct {
> > > > unsigned short fsindex;
> > > > unsigned short gsindex;
> > > > #endif
> > > > +#ifdef CONFIG_X86_32
> > > > unsigned long ip;
> > > > +#endif
> > > > #ifdef CONFIG_X86_64
> > > > unsigned long fs;
> > > > #endif
> > >
> > > Do these make struct thread_struct behave better in cachelines (smaller,
> > > less aliasing)? Can we really fit more in the slab du jour?
> > >
> > > Otherwise it seems like we're littering these structs with #ifdefs
> > > and not really saving anything. If these #ifdefs don't save any space why not
> > > just put in a comment:
> > >
> > > > unsigned long ip; /* Used only on i386 */
> > >
> > > Or maybe even:
> > >
> > > union {
> > > unsigned long ip; /* Used only on i386 */
> > > unsigned long fs; /* Used only on x86_64 */
> > > };
> > >
> >
> > Can we do it like this:
> > unsigned long ip_fs; /* ip: i386, fs: x86_64 */
> >
> > I am using same variable for both cases, or we can use some better
> > name than ip_fs. I am assuming either it is i386 or x86_64 machine
> > ;-)
>
> This is the least clean variant amongst all the suggestions.
>
Yes, this was a wakeful call for you.
I send dozen of emails in last 24 hours to you for your feedback.
I do not need reply for this email. Please send reply for other emails.
Thanks,
--
JSR
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 09/30] x86_64: ifdef out struct thread_struct::ip
2009-04-10 9:19 ` Ingo Molnar
2009-04-10 9:47 ` Ingo Molnar
@ 2009-04-10 11:17 ` Alexey Dobriyan
1 sibling, 0 replies; 8+ messages in thread
From: Alexey Dobriyan @ 2009-04-10 11:17 UTC (permalink / raw)
To: Ingo Molnar
Cc: Matt Helsley, Tejun Heo, Jeremy Fitzhardinge, akpm, containers,
xemul, linux-kernel, dave, hch, torvalds
On Fri, Apr 10, 2009 at 11:19:31AM +0200, Ingo Molnar wrote:
>
> * Matt Helsley <matthltc@us.ibm.com> wrote:
>
> > On Fri, Apr 10, 2009 at 06:35:22AM +0400, Alexey Dobriyan wrote:
> > > struct thread_struct::ip isn't used on x86_64, struct pt_regs::ip is used
> > > instead.
> > >
> > > kgdb should be reading 0, but I can't check it.
> > >
> > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > > ---
> > >
> > > arch/x86/include/asm/processor.h | 2 ++
> > > arch/x86/kernel/kgdb.c | 2 +-
> > > 2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > --- a/arch/x86/include/asm/processor.h
> > > +++ b/arch/x86/include/asm/processor.h
> > > @@ -421,7 +421,9 @@ struct thread_struct {
> > > unsigned short fsindex;
> > > unsigned short gsindex;
> > > #endif
> > > +#ifdef CONFIG_X86_32
> > > unsigned long ip;
> > > +#endif
> > > #ifdef CONFIG_X86_64
> > > unsigned long fs;
> > > #endif
> >
> > Do these make struct thread_struct behave better in cachelines
> > (smaller, less aliasing)? Can we really fit more in the slab du
> > jour?
> >
> > Otherwise it seems like we're littering these structs with #ifdefs
> > and not really saving anything. [...]
>
> Removing fields always saves memory (even if it does not show up
> currently due to allocators cache-aligning sizes).
>
> But the #ifdef ugliness is a real worry.
You should have thought about it when i386/x86_64 unification was
introduced.
pagefault code was full of ifdefs (it's less now), and this is trivial
ifdef in a header.
> > [...] If these #ifdefs don't save any
> > space why not just put in a comment:
> >
> > > unsigned long ip; /* Used only on i386 */
>
> Yes.
>
> > Or maybe even:
> >
> > union {
> > unsigned long ip; /* Used only on i386 */
> > unsigned long fs; /* Used only on x86_64 */
> > };
>
> Maybe. If this ever gets misunderstood somewhere in platform code we
> will get ugly failure modes and zero compiler help.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-04-10 11:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-04-10 2:35 [PATCH 09/30] x86_64: ifdef out struct thread_struct::ip Alexey Dobriyan
2009-04-10 3:53 ` Matt Helsley
2009-04-10 4:10 ` Jaswinder Singh Rajput
2009-04-10 9:20 ` Ingo Molnar
2009-04-10 10:52 ` Jaswinder Singh Rajput
2009-04-10 9:19 ` Ingo Molnar
2009-04-10 9:47 ` Ingo Molnar
2009-04-10 11:17 ` Alexey Dobriyan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).