Re: [PATCH 1/1] pagemap clear_refs: modify to specify anon or mapped vma clearing

[Moussa Ba <moussa.a.ba@gmail.com>]

On Fri, Jul 24, 2009 at 1:39 AM, Amerigo Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Thu, Jul 23, 2009 at 08:57:22PM -0700, Moussa A. Ba wrote:
> >
> > Signed-off-by: Jared E. Hulbert <jaredeh@gmail.com>
> > Signed-off-by: Moussa Ba <moussa.a.ba@gmail.com>
> > --- a/fs/proc/task_mmu.c      2009-07-21 14:30:01.000000000 -0700
> > +++ b/fs/proc/task_mmu.c      2009-07-21 14:32:56.000000000 -0700
> > @@ -461,6 +461,33 @@
> >       cond_resched();
> >       return 0;
> >  }
> > +static void walk_vma_area(struct mm_walk *this_walk, struct
> > vm_area_struct *vma,
> > +                                             int type)
> > +{
> > +     switch (type) {
> > +             /* Clear Anon VMAs only */
> > +     case 1:
> > +             if (!is_vm_hugetlb_page(vma) && !vma->vm_file)
> > +                     walk_page_range(vma->vm_start,
> > +                                     vma->vm_end,
> > +                                     this_walk);
> > +             break;
> > +             /* Clear Mapped VMAs only */
> > +     case 2:
> > +             if (!is_vm_hugetlb_page(vma) && vma->vm_file)
> > +                     walk_page_range(vma->vm_start,
> > +                                     vma->vm_end,
> > +                                     this_walk);
> > +             break;
> > +             /* Clear All VMAs */
> > +     default:
> > +             if (!is_vm_hugetlb_page(vma))
> > +                     walk_page_range(vma->vm_start,
> > +                                     vma->vm_end,
> > +                                     this_walk);
> > +             break;
> > +     }
> > +}
> >
>
>
> First, can you make these plain numbers as macros or enum types?
>
> Second, I believe the above code can be simiplified, how about below?


>
> {
>    if (!is_vm_hugetlb_page(vma)) {
>        if (type == 1 && vma->vm_file)
>             return;
>        if (type == 2 && !vma->vm_file)
>             return;
>        walk_page_range(vma->vm_start, vma->end, this_walk);
>    }
> }
>


It would indeed work, it can probably be written as:

{
	if (is_vm_hugetlb_page(vma))
    	    return;
       	if (type == 2 && vma->vm_file)
	    return;
	if (type == 3 && !vma->vm_file)
	    return;
	walk_page_range(vma->vm_start, vma->vm_end, this_walk);

}

The change in numbers also addresses Matt's concern that writing a 1 is the
default behavior where all vmas are cleared, hence it is better to not break
that and make the selective clearing options 2 and 3. There is no
documentation for clear_refs beyond a 1 liner that states "Clears page
referenced bits shown in smaps output".  I will also update the Documentation
and add it to the patch.




>
>
> >  static ssize_t clear_refs_write(struct file *file, const char __user *
> > buf,
> >                               size_t count, loff_t * ppos)
> > @@ -469,13 +496,15 @@
> >       char buffer[PROC_NUMBUF], *end;
> >       struct mm_struct *mm;
> >       struct vm_area_struct *vma;
> > +     int type;
> >
> >       memset(buffer, 0, sizeof(buffer));
> >       if (count > sizeof(buffer) - 1)
> >               count = sizeof(buffer) - 1;
> >       if (copy_from_user(buffer, buf, count))
> >               return -EFAULT;
> > -     if (!simple_strtol(buffer, &end, 0))
> > +     type = simple_strtol(buffer, &end, 0);
> > +     if (!type)
> >               return -EINVAL;
> >       if (*end == '\n')
> >               end++;
> > @@ -491,9 +520,7 @@
> >               down_read(&mm->mmap_sem);
> >               for (vma = mm->mmap; vma; vma = vma->vm_next) {
> >                       clear_refs_walk.private = vma;
> > -                     if (!is_vm_hugetlb_page(vma))
> > -                             walk_page_range(vma->vm_start, vma->vm_end,
> > -                                             &clear_refs_walk);
> > +                     walk_vma_area(&clear_refs_walk, vma, type);
> >               }
> >               flush_tlb_mm(mm);
> >               up_read(&mm->mmap_sem);
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/