[PATCH resend] mm: get rid of CONFIG_STACK_GROWSUP || CONFIG_IA64

[PATCH resend] mm: get rid of CONFIG_STACK_GROWSUP || CONFIG_IA64

[Michal Hocko]

Hi Andrew,
the patch bellow probably got lost in the huge "parisc crashes with slub"
thread triggered by my earlier clean up in this area so I am resending
it standalone.
---
>From 2e79c7e73a39a09389a84a8f37eb2a2f2f2859f5 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Tue, 19 Apr 2011 11:11:41 +0200
Subject: [PATCH] mm: get rid of CONFIG_STACK_GROWSUP || CONFIG_IA64

IA64 needs some trickery for Register Backing Store so we have to
export expand_stack_upwards for it even though the architecture expands
its stack downwards normally.
We have defined VM_GROWSUP which is defined only for the above
configuration so let's use it everywhere rather than hardcoded
CONFIG_STACK_GROWSUP || CONFIG_IA64

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/mmap.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 29c68b0..3ff9edf 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1726,7 +1726,7 @@ static int acct_stack_growth(struct vm_area_struct *vma, unsigned long size, uns
 	return 0;
 }
 
-#if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
+#if VM_GROWSUP
 /*
  * PA-RISC uses this for its stack; IA64 for its Register Backing Store.
  * vma is the last one with address > vma->vm_end.  Have to extend vma.
@@ -1777,7 +1777,7 @@ int expand_stack_upwards(struct vm_area_struct *vma, unsigned long address)
 	khugepaged_enter_vma_merge(vma);
 	return error;
 }
-#endif /* CONFIG_STACK_GROWSUP || CONFIG_IA64 */
+#endif /* VM_GROWSUP */
 
 /*
  * vma is the first one with address < vma->vm_start.  Have to extend vma.
-- 
1.7.4.4

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH resend] mm: get rid of CONFIG_STACK_GROWSUP || CONFIG_IA64

[Hugh Dickins]

Hi Michal,

On Tue, 3 May 2011, Michal Hocko wrote:

> Hi Andrew,
> the patch bellow probably got lost in the huge "parisc crashes with slub"
> thread triggered by my earlier clean up in this area so I am resending
> it standalone.
> ---
> From 2e79c7e73a39a09389a84a8f37eb2a2f2f2859f5 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Tue, 19 Apr 2011 11:11:41 +0200
> Subject: [PATCH] mm: get rid of CONFIG_STACK_GROWSUP || CONFIG_IA64
> 
> IA64 needs some trickery for Register Backing Store so we have to
> export expand_stack_upwards for it even though the architecture expands
> its stack downwards normally.
> We have defined VM_GROWSUP which is defined only for the above
> configuration so let's use it everywhere rather than hardcoded
> CONFIG_STACK_GROWSUP || CONFIG_IA64
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

Sorry to be negative, but this seems more clever than helpful to me:
it does not optimize anything (apart from saving a few bytes in mm/mmap.c
itself), obscures the special IA64 case, and relies upon the ways in which
we happen to define VM_GROWSUP elsewhere.

Not a nack: others may well disagree with me.

And, though I didn't find time to comment on your later "symmetrical"
patch before it went into mmotm, I didn't see how renaming expand_downwards
and expand_upwards to expand_stack_downwards and expand_stack_upwards was
helpful either - needless change, and you end up using expand_stack_upwards
on something which is not (what we usually call) the stack.

Now, if you're looking to make a nice cleanup, how about getting rid
of find_vma_prev(), which Linus made redundant when he suddenly added
vm_prev in 2.6.36?  There's at least one place where I apologize for
its expense in a BUG_ON, I'd be glad to see that killed off.

Hey, but it's certainly not for me to assign work to you!

Hugh

> ---
>  mm/mmap.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 29c68b0..3ff9edf 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1726,7 +1726,7 @@ static int acct_stack_growth(struct vm_area_struct *vma, unsigned long size, uns
>  	return 0;
>  }
>  
> -#if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
> +#if VM_GROWSUP
>  /*
>   * PA-RISC uses this for its stack; IA64 for its Register Backing Store.
>   * vma is the last one with address > vma->vm_end.  Have to extend vma.
> @@ -1777,7 +1777,7 @@ int expand_stack_upwards(struct vm_area_struct *vma, unsigned long address)
>  	khugepaged_enter_vma_merge(vma);
>  	return error;
>  }
> -#endif /* CONFIG_STACK_GROWSUP || CONFIG_IA64 */
> +#endif /* VM_GROWSUP */
>  
>  /*
>   * vma is the first one with address < vma->vm_start.  Have to extend vma.
> -- 
> 1.7.4.4

Re: [PATCH resend] mm: get rid of CONFIG_STACK_GROWSUP || CONFIG_IA64

[Michal Hocko]

Hi Hugh,

On Tue 03-05-11 12:11:28, Hugh Dickins wrote:
> On Tue, 3 May 2011, Michal Hocko wrote:
[...]
> > IA64 needs some trickery for Register Backing Store so we have to
> > export expand_stack_upwards for it even though the architecture expands
> > its stack downwards normally.
> > We have defined VM_GROWSUP which is defined only for the above
> > configuration so let's use it everywhere rather than hardcoded
> > CONFIG_STACK_GROWSUP || CONFIG_IA64
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> 
> Sorry to be negative, but this seems more clever than helpful to me:
> it does not optimize anything (apart from saving a few bytes in mm/mmap.c
> itself), 

The patch doesn't aim at optimizing anything. It is just a cleanup.

> obscures the special IA64 case, and relies upon the ways in which
> we happen to define VM_GROWSUP elsewhere.

This case is obscure enough already because we are using VM_GROWSUP to
declare expand_stack_upwards in include/linux/mm.h while definition is
guarded by CONFIG_STACK_GROWSUP||CONFIG_IA64. 
What the patch does is just "make it consistent" thing. I think we
should at least use CONFIG_STACK_GROWSUP||CONFIG_IA64 at both places if
you do not like VM_GROWSUP misuse.

> Not a nack: others may well disagree with me.
> 
> And, though I didn't find time to comment on your later "symmetrical"
> patch before it went into mmotm, I didn't see how renaming expand_downwards
> and expand_upwards to expand_stack_downwards and expand_stack_upwards was
> helpful either - needless change, and you end up using expand_stack_upwards
> on something which is not (what we usually call) the stack.

OK, I see your point. expand_stack_upwards in ia64_do_page_fault can be
confusing as well. Maybe if we stick with the original expand_upwards
and just make expand_downwards symmetrical without renameing to
"_stack_" like the patch does? I can rework that patch if there is an
interest. I would like to have it symmetrical, though, because the
original code was rather confusing.

> Now, if you're looking to make a nice cleanup, 

Originally I didn't plan to do a big cleanup. I just needed to backport
some stack gap fixes and I found the code confusing...

> how about getting rid of find_vma_prev(), which Linus made redundant
> when he suddenly added vm_prev in 2.6.36?  There's at least one place
> where I apologize for its expense in a BUG_ON, I'd be glad to see that
> killed off.

Good thing to do, let's see if I find some time...

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH resend] mm: get rid of CONFIG_STACK_GROWSUP || CONFIG_IA64

[Hugh Dickins]

On Wed, 4 May 2011, Michal Hocko wrote:
> 
> This case is obscure enough already because we are using VM_GROWSUP to
> declare expand_stack_upwards in include/linux/mm.h

Ah yes, I didn't notice that it was already done that way there
(closer to the definitions of VM_GROWSUP so not as bad).

> while definition is guarded by CONFIG_STACK_GROWSUP||CONFIG_IA64. 
> What the patch does is just "make it consistent" thing. I think we
> should at least use CONFIG_STACK_GROWSUP||CONFIG_IA64 at both places if
> you do not like VM_GROWSUP misuse.

If it's worth changing anything, yes, that would be better.

> 
> > Not a nack: others may well disagree with me.
> > 
> > And, though I didn't find time to comment on your later "symmetrical"
> > patch before it went into mmotm, I didn't see how renaming expand_downwards
> > and expand_upwards to expand_stack_downwards and expand_stack_upwards was
> > helpful either - needless change, and you end up using expand_stack_upwards
> > on something which is not (what we usually call) the stack.
> 
> OK, I see your point. expand_stack_upwards in ia64_do_page_fault can be
> confusing as well. Maybe if we stick with the original expand_upwards
> and just make expand_downwards symmetrical without renameing to
> "_stack_" like the patch does? I can rework that patch if there is an
> interest. I would like to have it symmetrical, though, because the
> original code was rather confusing.

Yes, what I suggested before was an expand_upwards, an expand_downwards
and an expand_stack (with mod to fs/exec.c to replace its call to
expand_stack_downwards by direct call to expand_downwards).

But it's always going to be somewhat confusing and asymmetrical
because of the ia64 register backing store case.

Hugh

Re: [PATCH resend] mm: get rid of CONFIG_STACK_GROWSUP || CONFIG_IA64

[Michal Hocko]

On Wed 04-05-11 10:28:36, Hugh Dickins wrote:
> On Wed, 4 May 2011, Michal Hocko wrote:
> > 
> > This case is obscure enough already because we are using VM_GROWSUP to
> > declare expand_stack_upwards in include/linux/mm.h
> 
> Ah yes, I didn't notice that it was already done that way there
> (closer to the definitions of VM_GROWSUP so not as bad).
> 
> > while definition is guarded by CONFIG_STACK_GROWSUP||CONFIG_IA64. 
> > What the patch does is just "make it consistent" thing. I think we
> > should at least use CONFIG_STACK_GROWSUP||CONFIG_IA64 at both places if
> > you do not like VM_GROWSUP misuse.
> 
> If it's worth changing anything, yes, that would be better.

I have looked into the history again and the current VM_GROWSUP usage
for the CONFIG_STACK_GROWSUP||CONFIG_IA64 has been introduced by
commit 8ca3eb08097f6839b2206e2242db4179aee3cfb3
Author: Luck, Tony <tony.luck@intel.com>
Date:   Tue Aug 24 11:44:18 2010 -0700

    guard page for stacks that grow upwards
    
    pa-risc and ia64 have stacks that grow upwards. Check that
    they do not run into other mappings. By making VM_GROWSUP
    0x0 on architectures that do not ever use it, we can avoid
    some unpleasant #ifdefs in check_stack_guard_page().
    
    Signed-off-by: Tony Luck <tony.luck@intel.com>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

So I think the flag should be used that way. If we ever going to add a
new architecture like IA64 which uses both ways of expanding we should
make it easier by minimizing the places which have to be examined.

> > > Not a nack: others may well disagree with me.
> > > 
> > > And, though I didn't find time to comment on your later "symmetrical"
> > > patch before it went into mmotm, I didn't see how renaming expand_downwards
> > > and expand_upwards to expand_stack_downwards and expand_stack_upwards was
> > > helpful either - needless change, and you end up using expand_stack_upwards
> > > on something which is not (what we usually call) the stack.
> > 
> > OK, I see your point. expand_stack_upwards in ia64_do_page_fault can be
> > confusing as well. Maybe if we stick with the original expand_upwards
> > and just make expand_downwards symmetrical without renameing to
> > "_stack_" like the patch does? I can rework that patch if there is an
> > interest. I would like to have it symmetrical, though, because the
> > original code was rather confusing.
> 
> Yes, what I suggested before was an expand_upwards, an expand_downwards
> and an expand_stack (with mod to fs/exec.c to replace its call to
> expand_stack_downwards by direct call to expand_downwards).

OK, now, with the cleanup patch, we have expand_stack and
expand_stack_{downwards,upwards}. I will repost the patch to Andrew with
up and down cases renamed. Does it work for you?

> But it's always going to be somewhat confusing and asymmetrical
> because of the ia64 register backing store case.

How come? We would have expand_stack which is pretty much clear that it
is expanding stack in the architecture specific way. And then we would
have expand_{upwards,downward} which are clear about way how we expand
whatever VMA, right?
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH resend] mm: get rid of CONFIG_STACK_GROWSUP || CONFIG_IA64

[Hugh Dickins]

On Wed, May 4, 2011 at 11:30 PM, Michal Hocko <mhocko@suse.cz> wrote:

> So I think the flag should be used that way. If we ever going to add a
> new architecture like IA64 which uses both ways of expanding we should
> make it easier by minimizing the places which have to be examined.

If, yes.  Let's just agree to disagree.  It looks like I'm preferring
to think of the ia64 case as exceptional, and I want to be reminded of
that peculiar case; whereas you are wanting to generalize and make it
not stand out.  Both valid.

> OK, now, with the cleanup patch, we have expand_stack and
> expand_stack_{downwards,upwards}. I will repost the patch to Andrew with
> up and down cases renamed. Does it work for you?

Sounds right.

>
>> But it's always going to be somewhat confusing and asymmetrical
>> because of the ia64 register backing store case.
>
> How come? We would have expand_stack which is pretty much clear that it
> is expanding stack in the architecture specific way. And then we would
> have expand_{upwards,downward} which are clear about way how we expand
> whatever VMA, right?

Right.  I'm preferring to be reminded of the confusion and asymmetry,
you're preferring to smooth over it.

Hugh

Re: [PATCH resend] mm: get rid of CONFIG_STACK_GROWSUP || CONFIG_IA64

[Michal Hocko]

On Thu 05-05-11 18:12:12, Hugh Dickins wrote:
> On Wed, May 4, 2011 at 11:30 PM, Michal Hocko <mhocko@suse.cz> wrote:
> 
> > So I think the flag should be used that way. If we ever going to add a
> > new architecture like IA64 which uses both ways of expanding we should
> > make it easier by minimizing the places which have to be examined.
> 
> If, yes.  Let's just agree to disagree.  It looks like I'm preferring
> to think of the ia64 case as exceptional, and I want to be reminded of
> that peculiar case; whereas you are wanting to generalize and make it
> not stand out.  Both valid.

Probably a call for Andrew?
Anyway, whatever is the way we go I think that both declaration and
definition should be guarded by the same ifdefs.

> > OK, now, with the cleanup patch, we have expand_stack and
> > expand_stack_{downwards,upwards}. I will repost the patch to Andrew with
> > up and down cases renamed. Does it work for you?
> 
> Sounds right.

OK, I will repost the updated patch.

> >> But it's always going to be somewhat confusing and asymmetrical
> >> because of the ia64 register backing store case.
> >
> > How come? We would have expand_stack which is pretty much clear that it
> > is expanding stack in the architecture specific way. And then we would
> > have expand_{upwards,downward} which are clear about way how we expand
> > whatever VMA, right?
> 
> Right.  I'm preferring to be reminded of the confusion and asymmetry,
> you're preferring to smooth over it.

OK

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic