Re: [RFC][PATCH] vmscan: report vm_flags in page_referenced()

[Minchan Kim <minchan.kim@gmail.com>]

On Fri, May 8, 2009 at 9:15 PM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> On Fri, May 08, 2009 at 08:09:24PM +0800, Minchan Kim wrote:
>> On Fri, May 8, 2009 at 1:17 PM, Wu Fengguang <fengguang.wu@intel.com> wrote:
>> > On Thu, May 07, 2009 at 11:17:46PM +0800, Peter Zijlstra wrote:
>> >> On Thu, 2009-05-07 at 17:10 +0200, Johannes Weiner wrote:
>> >>
>> >> > > @@ -1269,8 +1270,15 @@ static void shrink_active_list(unsigned
>> >> > >
>> >> > >           /* page_referenced clears PageReferenced */
>> >> > >           if (page_mapping_inuse(page) &&
>> >> > > -             page_referenced(page, 0, sc->mem_cgroup))
>> >> > > +             page_referenced(page, 0, sc->mem_cgroup)) {
>> >> > > +                 struct address_space *mapping = page_mapping(page);
>> >> > > +
>> >> > >                   pgmoved++;
>> >> > > +                 if (mapping && test_bit(AS_EXEC, &mapping->flags)) {
>> >> > > +                         list_add(&page->lru, &l_active);
>> >> > > +                         continue;
>> >> > > +                 }
>> >> > > +         }
>> >> >
>> >> > Since we walk the VMAs in page_referenced anyway, wouldn't it be
>> >> > better to check if one of them is executable?  This would even work
>> >> > for executable anon pages.  After all, there are applications that cow
>> >> > executable mappings (sbcl and other language environments that use an
>> >> > executable, run-time modified core image come to mind).
>> >>
>> >> Hmm, like provide a vm_flags mask along to page_referenced() to only
>> >> account matching vmas... seems like a sensible idea.
>> >
>> > Here is a quick patch for your opinions. Compile tested.
>> >
>> > With the added vm_flags reporting, the mlock=>unevictable logic can
>> > possibly be made more straightforward.
>> >
>> > Thanks,
>> > Fengguang
>> > ---
>> > vmscan: report vm_flags in page_referenced()
>> >
>> > This enables more informed reclaim heuristics, eg. to protect executable
>> > file pages more aggressively.
>> >
>> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
>> > ---
>> >  include/linux/rmap.h |    5 +++--
>> >  mm/rmap.c            |   30 +++++++++++++++++++++---------
>> >  mm/vmscan.c          |    7 +++++--
>> >  3 files changed, 29 insertions(+), 13 deletions(-)
>> >
>> > --- linux.orig/include/linux/rmap.h
>> > +++ linux/include/linux/rmap.h
>> > @@ -83,7 +83,8 @@ static inline void page_dup_rmap(struct
>> >  /*
>> >  * Called from mm/vmscan.c to handle paging out
>> >  */
>> > -int page_referenced(struct page *, int is_locked, struct mem_cgroup *cnt);
>> > +int page_referenced(struct page *, int is_locked,
>> > +                       struct mem_cgroup *cnt, unsigned long *vm_flags);
>> >  int try_to_unmap(struct page *, int ignore_refs);
>> >
>> >  /*
>> > @@ -128,7 +129,7 @@ int page_wrprotect(struct page *page, in
>> >  #define anon_vma_prepare(vma)  (0)
>> >  #define anon_vma_link(vma)     do {} while (0)
>> >
>> > -#define page_referenced(page,l,cnt) TestClearPageReferenced(page)
>> > +#define page_referenced(page, locked, cnt, flags) TestClearPageReferenced(page)
>> >  #define try_to_unmap(page, refs) SWAP_FAIL
>> >
>> >  static inline int page_mkclean(struct page *page)
>> > --- linux.orig/mm/rmap.c
>> > +++ linux/mm/rmap.c
>> > @@ -333,7 +333,8 @@ static int page_mapped_in_vma(struct pag
>> >  * repeatedly from either page_referenced_anon or page_referenced_file.
>> >  */
>> >  static int page_referenced_one(struct page *page,
>> > -       struct vm_area_struct *vma, unsigned int *mapcount)
>> > +                              struct vm_area_struct *vma,
>> > +                              unsigned int *mapcount)
>> >  {
>> >        struct mm_struct *mm = vma->vm_mm;
>> >        unsigned long address;
>> > @@ -385,7 +386,8 @@ out:
>> >  }
>> >
>> >  static int page_referenced_anon(struct page *page,
>> > -                               struct mem_cgroup *mem_cont)
>> > +                               struct mem_cgroup *mem_cont,
>> > +                               unsigned long *vm_flags)
>> >  {
>> >        unsigned int mapcount;
>> >        struct anon_vma *anon_vma;
>> > @@ -406,6 +408,7 @@ static int page_referenced_anon(struct p
>> >                if (mem_cont && !mm_match_cgroup(vma->vm_mm, mem_cont))
>> >                        continue;
>> >                referenced += page_referenced_one(page, vma, &mapcount);
>> > +               *vm_flags |= vma->vm_flags;
>>
>> Sometime this vma don't contain the anon page.
>> That's why we need page_check_address.
>> For such a case, wrong *vm_flag cause be harmful to reclaim.
>> It can be happen in your first class citizen patch, I think.
>
> Yes I'm aware of that - the VMA area covers that page, but have no pte
> actually installed for that page. That should be OK - the presentation
> of such VMA is a good indication of it being some executable text.
>

Sorry but I can't understand your point.

This is general interface but not only executable text.
Sometime, The information of vma which don't really have the page can
be passed to caller.
ex) It can be happen by COW, mremap, non-linear mapping and so on.
but I am not sure.
I doubt  vm_flag information is useful.
-- 
Kinds regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>