lkml.org 
[lkml]   [2011]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[PATCH v2] mm: make expand_downwards symmetrical to expand_upwards
    On Sun 17-04-11 20:00:17, Hugh Dickins wrote:
    > On Fri, 14 Apr 2011, Michal Hocko wrote:
    [...]
    > > diff --git a/include/linux/mm.h b/include/linux/mm.h
    > > index 692dbae..765cf4e 100644
    > > --- a/include/linux/mm.h
    > > +++ b/include/linux/mm.h
    > > @@ -1498,8 +1498,10 @@ unsigned long ra_submit(struct file_ra_state *ra,
    > > extern int expand_stack(struct vm_area_struct *vma, unsigned long address);
    > > #if VM_GROWSUP
    > > extern int expand_upwards(struct vm_area_struct *vma, unsigned long address);
    > > + #define expand_downwards(vma, address) do { } while (0)
    >
    > I think this is wrong: doesn't the VM_GROWSUP case actually want
    > a real expand_downwards() in addition to expand_upwards()?

    Ahh, right you are. I haven't noticed that we have
    expand_stack_downwards as well and this one is called from get_arg_page
    if CONFIG_STACK_GROWSUP.

    I am wondering why we do this just for CONFIG_STACK_GROWSUP. Do we
    expand stack for GROWSDOWN automatically?

    >
    > > #else
    > > #define expand_upwards(vma, address) do { } while (0)
    > > +extern int expand_downwards(struct vm_area_struct *vma, unsigned long address);
    > > #endif
    > > extern int expand_stack_downwards(struct vm_area_struct *vma,
    > > unsigned long address);
    >
    > And if you're going for symmetry, wouldn't it be nice to add fs/exec.c
    > to the patch and remove this silly expand_stack_downwards() wrapper?

    Sounds reasonable. Thanks for the review, Hugh. I am also thinking
    whether expand_stack_{downwards,upwards} is more suitable name for those
    functions as we are more explicit that this is stack related.

    What about the updated patch bellow?

    Changes since v1
    - fixed expand_downwards case for CONFIG_STACK_GROWSUP in get_arg_page.
    - rename expand_{downwards,upwards} -> expand_stack_{downwards,upwards}
    ---

    From f3adcf40518eaf7c4ee1cb10abd16b59785a66ba Mon Sep 17 00:00:00 2001
    From: Michal Hocko <mhocko@suse.cz>
    Date: Fri, 15 Apr 2011 14:56:26 +0200
    Subject: [PATCH] mm: make expand_downwards symmetrical to expand_upwards

    Currently we have expand_upwards exported while expand_downwards is
    accessible only via expand_stack or expand_stack_downwards.

    check_stack_guard_page is a nice example of the asymmetry. It uses
    expand_stack for VM_GROWSDOWN while expand_upwards is called for
    VM_GROWSUP case.

    Let's clean this up by exporting both functions and make those name
    consistent. Let's use expand_stack_{upwards,downwards} so that we are
    explicit about stack manipulation in the name. expand_stack_downwards
    has to be defined for both CONFIG_STACK_GROWS{UP,DOWN} because
    get_arg_page calls the downwards version in the early process
    initialization phase for growsup configuration.

    Signed-off-by: Michal Hocko <mhocko@suse.cz>
    ---
    include/linux/mm.h | 13 ++++++++-----
    mm/memory.c | 4 ++--
    mm/mmap.c | 13 ++++---------
    3 files changed, 14 insertions(+), 16 deletions(-)

    diff --git a/include/linux/mm.h b/include/linux/mm.h
    index 692dbae..17f9b86 100644
    --- a/include/linux/mm.h
    +++ b/include/linux/mm.h
    @@ -1494,15 +1494,18 @@ unsigned long ra_submit(struct file_ra_state *ra,
    struct address_space *mapping,
    struct file *filp);

    -/* Do stack extension */
    +/* Generic expand stack which grows the stack according to GROWS{UP,DOWN} */
    extern int expand_stack(struct vm_area_struct *vma, unsigned long address);
    +
    +/* CONFIG_STACK_GROWSUP still needs to to grow downwards at some places */
    +extern int expand_stack_downwards(struct vm_area_struct *vma,
    + unsigned long address);
    #if VM_GROWSUP
    -extern int expand_upwards(struct vm_area_struct *vma, unsigned long address);
    +extern int expand_stack_upwards(struct vm_area_struct *vma,
    + unsigned long address);
    #else
    - #define expand_upwards(vma, address) do { } while (0)
    + #define expand_stack_upwards(vma, address) do { } while (0)
    #endif
    -extern int expand_stack_downwards(struct vm_area_struct *vma,
    - unsigned long address);

    /* Look up the first VMA which satisfies addr < vm_end, NULL if none. */
    extern struct vm_area_struct * find_vma(struct mm_struct * mm, unsigned long addr);
    diff --git a/mm/memory.c b/mm/memory.c
    index ce22a25..ba5b4d8 100644
    --- a/mm/memory.c
    +++ b/mm/memory.c
    @@ -2969,7 +2969,7 @@ static inline int check_stack_guard_page(struct vm_area_struct *vma, unsigned lo
    if (prev && prev->vm_end == address)
    return prev->vm_flags & VM_GROWSDOWN ? 0 : -ENOMEM;

    - expand_stack(vma, address - PAGE_SIZE);
    + expand_stack_downwards(vma, address - PAGE_SIZE);
    }
    if ((vma->vm_flags & VM_GROWSUP) && address + PAGE_SIZE == vma->vm_end) {
    struct vm_area_struct *next = vma->vm_next;
    @@ -2978,7 +2978,7 @@ static inline int check_stack_guard_page(struct vm_area_struct *vma, unsigned lo
    if (next && next->vm_start == address + PAGE_SIZE)
    return next->vm_flags & VM_GROWSUP ? 0 : -ENOMEM;

    - expand_upwards(vma, address + PAGE_SIZE);
    + expand_stack_upwards(vma, address + PAGE_SIZE);
    }
    return 0;
    }
    diff --git a/mm/mmap.c b/mm/mmap.c
    index e27e0cf..29c68b0 100644
    --- a/mm/mmap.c
    +++ b/mm/mmap.c
    @@ -1731,7 +1731,7 @@ static int acct_stack_growth(struct vm_area_struct *vma, unsigned long size, uns
    * 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.
    */
    -int expand_upwards(struct vm_area_struct *vma, unsigned long address)
    +int expand_stack_upwards(struct vm_area_struct *vma, unsigned long address)
    {
    int error;

    @@ -1782,7 +1782,7 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
    /*
    * vma is the first one with address < vma->vm_start. Have to extend vma.
    */
    -static int expand_downwards(struct vm_area_struct *vma,
    +int expand_stack_downwards(struct vm_area_struct *vma,
    unsigned long address)
    {
    int error;
    @@ -1829,15 +1829,10 @@ static int expand_downwards(struct vm_area_struct *vma,
    return error;
    }

    -int expand_stack_downwards(struct vm_area_struct *vma, unsigned long address)
    -{
    - return expand_downwards(vma, address);
    -}
    -
    #ifdef CONFIG_STACK_GROWSUP
    int expand_stack(struct vm_area_struct *vma, unsigned long address)
    {
    - return expand_upwards(vma, address);
    + return expand_stack_upwards(vma, address);
    }

    struct vm_area_struct *
    @@ -1859,7 +1854,7 @@ find_extend_vma(struct mm_struct *mm, unsigned long addr)
    #else
    int expand_stack(struct vm_area_struct *vma, unsigned long address)
    {
    - return expand_downwards(vma, address);
    + return expand_stack_downwards(vma, address);
    }

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


    \
     
     \ /
      Last update: 2011-04-18 12:03    [W:4.367 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site