Re: [PATCH 2/3] binfmt_elf_fdpic: Convert initial stack alignment to arch_align_stack().

[Paul Mundt <lethal@linux-sh.org>]

On Sun, Aug 03, 2008 at 11:24:21PM -0400, Mike Frysinger wrote:
> On Fri, Aug 1, 2008 at 5:44 PM, Paul Mundt wrote:
> > On Fri, Aug 01, 2008 at 03:01:19PM +0100, David Howells wrote:
> >> Paul Mundt <lethal@linux-sh.org> wrote:
> >> > +    * In some cases (e.g. Hyper-Threading), we want to avoid L1
> >> > +    * evictions by the processes running on the same package. One
> >> > +    * thing we can do is to shuffle the initial stack for them, so
> >> > +    * we give the architecture an opportunity to do so here.
> >> > +    */
> >> >  #ifdef CONFIG_MMU
> >> > -   sp = bprm->p;
> >> > +   sp = arch_align_stack(bprm->p);
> >> >  #else
> >> > -   sp = mm->start_stack;
> >> > +   sp = arch_align_stack(mm->start_stack);
> >>
> >> Ummm...  You're calling arch_align_stack() under NOMMU...  Is that really a
> >> good idea?
> >
> > Not particularly, no.
> >
> >> You can't necessarily move the stack pointer without exiting the allocated
> >> region or shrinking the amount of stack space the executable asked for.  If
> >> you want to do this sort of thing, you need to tell the memory allocator what
> >> you're up to - or at the very least allocate some slack.
> >
> > Yes, that's a good point, and one that probably ought to be documented
> > alongside the initial alignment. I'll send an updated patch.
> 
> fs/binfmt_elf_fdpic.c: In function 'create_elf_fdpic_tables':
> fs/binfmt_elf_fdpic.c:497: error: implicit declaration of function
> 'arch_align_stack'
> make[1]: *** [fs/binfmt_elf_fdpic.o] Error 1
> make: *** [fs] Error 2
> 
> Blackfin lacks a arch_align_stack(x) in asm-blackfin/system.h ... i
> imagine a stub will work for us like every other arch though
> 
> feel like doing that Bryan ?  i'm guessing Paul doesnt want to ... ;)

There's no need, as David pointed out, that was the wrong thing to do for
the nommu case anyways. Here's an updated patch to replace the previous
one:

Signed-off-by: Paul Mundt <lethal@linux-sh.org>

---

diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 80c1f95..10c6cb8 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -472,9 +472,16 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
 	int loop;
 	int nr;	/* reset for each csp adjustment */
 
-	/* we're going to shovel a whole load of stuff onto the stack */
 #ifdef CONFIG_MMU
-	sp = bprm->p;
+	/*
+	 * we're going to shovel a whole load of stuff onto the stack
+	 *
+	 * In some cases (e.g. Hyper-Threading), we want to avoid L1
+	 * evictions by the processes running on the same package. One
+	 * thing we can do is to shuffle the initial stack for them, so
+	 * we give the architecture an opportunity to do so here.
+	 */
+	sp = arch_align_stack(bprm->p);
 #else
 	sp = mm->start_stack;
 
@@ -499,20 +506,6 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
 			return -EFAULT;
 	}
 
-#if defined(__i386__) && defined(CONFIG_SMP)
-	/* in some cases (e.g. Hyper-Threading), we want to avoid L1 evictions
-	 * by the processes running on the same package. One thing we can do is
-	 * to shuffle the initial stack for them.
-	 *
-	 * the conditionals here are unneeded, but kept in to make the code
-	 * behaviour the same as pre change unless we have hyperthreaded
-	 * processors. This keeps Mr Marcelo Person happier but should be
-	 * removed for 2.5
-	 */
-	if (smp_num_siblings > 1)
-		sp = sp - ((current->pid % 64) << 7);
-#endif
-
 	sp &= ~7UL;
 
 	/* stack the load map(s) */