lkml.org 
[lkml]   [2010]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] proc: pagemap: Hold mmap_sem during page walk
    Date
    > > I just recommend simply do double buffering.
    > >
    > Like this ?
    > CC'ed Horiguchi, he touched hugepage part of this code recently.

    I like this patch. but I have few comment.

    >
    > ==
    > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    >
    > In initial design, walk_page_range() was designed just for walking page table and
    > it didn't require mmap_sem. Now, find_vma() etc.. are used in walk_page_range()
    > and we need mmap_sem around it.
    >
    > This patch adds mmap_sem around walk_page_range().
    >
    > Because /proc/<pid>/pagemap's callback routine use put_user(), we have to get
    > rid of it to do sane fix.
    >
    > Reported-by: San Mehat <san@google.com>
    > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    > ---
    > fs/proc/task_mmu.c | 89 ++++++++++++++++++++++-------------------------------
    > 1 file changed, 38 insertions(+), 51 deletions(-)
    >
    > Index: linux-2.6.34-rc3/fs/proc/task_mmu.c
    > ===================================================================
    > --- linux-2.6.34-rc3.orig/fs/proc/task_mmu.c
    > +++ linux-2.6.34-rc3/fs/proc/task_mmu.c
    > @@ -406,8 +406,11 @@ static int show_smap(struct seq_file *m,
    >
    > memset(&mss, 0, sizeof mss);
    > mss.vma = vma;
    > - if (vma->vm_mm && !is_vm_hugetlb_page(vma))
    > + if (vma->vm_mm && !is_vm_hugetlb_page(vma)) {
    > + down_read(&vma->vm_mm->mmap_sem);
    > walk_page_range(vma->vm_start, vma->vm_end, &smaps_walk);
    > + up_read(&vma->vm_mm->mmap_sem);
    > + }

    m_start already take mmap_sem. then I don't think this part is necessary.


    >
    > show_map_vma(m, vma);
    >
    > @@ -552,7 +555,8 @@ const struct file_operations proc_clear_
    > };
    >
    > struct pagemapread {
    > - u64 __user *out, *end;
    > + int pos, len;
    > + u64 *buffer;
    > };
    >
    > #define PM_ENTRY_BYTES sizeof(u64)
    > @@ -575,10 +579,8 @@ struct pagemapread {
    > static int add_to_pagemap(unsigned long addr, u64 pfn,
    > struct pagemapread *pm)
    > {
    > - if (put_user(pfn, pm->out))
    > - return -EFAULT;
    > - pm->out++;
    > - if (pm->out >= pm->end)
    > + pm->buffer[pm->pos++] = pfn;
    > + if (pm->pos >= pm->len)
    > return PM_END_OF_BUFFER;
    > return 0;
    > }
    > @@ -720,21 +722,20 @@ static int pagemap_hugetlb_range(pte_t *
    > * determine which areas of memory are actually mapped and llseek to
    > * skip over unmapped regions.
    > */
    > +#define PAGEMAP_WALK_SIZE (PMD_SIZE)
    > static ssize_t pagemap_read(struct file *file, char __user *buf,
    > size_t count, loff_t *ppos)
    > {
    > struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
    > - struct page **pages, *page;
    > - unsigned long uaddr, uend;
    > struct mm_struct *mm;
    > struct pagemapread pm;
    > - int pagecount;
    > int ret = -ESRCH;
    > struct mm_walk pagemap_walk = {};
    > unsigned long src;
    > unsigned long svpfn;
    > unsigned long start_vaddr;
    > unsigned long end_vaddr;
    > + int copied = 0;
    >
    > if (!task)
    > goto out;
    > @@ -757,34 +758,10 @@ static ssize_t pagemap_read(struct file
    > if (!mm)
    > goto out_task;
    >
    > -
    > - uaddr = (unsigned long)buf & PAGE_MASK;
    > - uend = (unsigned long)(buf + count);
    > - pagecount = (PAGE_ALIGN(uend) - uaddr) / PAGE_SIZE;
    > - ret = 0;
    > - if (pagecount == 0)
    > + pm.len = PM_ENTRY_BYTES * (PAGEMAP_WALK_SIZE >> PAGE_SHIFT);
    > + pm.buffer = kmalloc(pm.len, GFP_KERNEL);

    In /proc interface, GFP_TEMPORARY is recommened rather than GFP_KERNEL.
    it help to prevent unnecessary external fragmentation.


    > + if (!pm.buffer)
    > goto out_mm;
    > - pages = kcalloc(pagecount, sizeof(struct page *), GFP_KERNEL);
    > - ret = -ENOMEM;
    > - if (!pages)
    > - goto out_mm;
    > -
    > - down_read(&current->mm->mmap_sem);
    > - ret = get_user_pages(current, current->mm, uaddr, pagecount,
    > - 1, 0, pages, NULL);
    > - up_read(&current->mm->mmap_sem);
    > -
    > - if (ret < 0)
    > - goto out_free;
    > -
    > - if (ret != pagecount) {
    > - pagecount = ret;
    > - ret = -EFAULT;
    > - goto out_pages;
    > - }
    > -
    > - pm.out = (u64 __user *)buf;
    > - pm.end = (u64 __user *)(buf + count);
    >
    > pagemap_walk.pmd_entry = pagemap_pte_range;
    > pagemap_walk.pte_hole = pagemap_pte_hole;
    > @@ -807,23 +784,33 @@ static ssize_t pagemap_read(struct file
    > * user buffer is tracked in "pm", and the walk
    > * will stop when we hit the end of the buffer.
    > */
    > - ret = walk_page_range(start_vaddr, end_vaddr, &pagemap_walk);
    > - if (ret == PM_END_OF_BUFFER)
    > - ret = 0;
    > - /* don't need mmap_sem for these, but this looks cleaner */
    > - *ppos += (char __user *)pm.out - buf;
    > - if (!ret)
    > - ret = (char __user *)pm.out - buf;
    > -
    > -out_pages:
    > - for (; pagecount; pagecount--) {
    > - page = pages[pagecount-1];
    > - if (!PageReserved(page))
    > - SetPageDirty(page);
    > - page_cache_release(page);
    > + while (count && (start_vaddr < end_vaddr)) {
    > + int len;
    > + unsigned long end;
    > + pm.pos = 0;
    > + start_vaddr += PAGEMAP_WALK_SIZE;
    > + end = start_vaddr + PAGEMAP_WALK_SIZE;
    > + down_read(&mm->mmap_sem);
    > + ret = walk_page_range(start_vaddr, end, &pagemap_walk);
    > + up_read(&mm->mmap_sem);
    > +
    > + len = PM_ENTRY_BYTES * pm.pos;
    > + if (len > count)
    > + len = count;
    > + if (copy_to_user((char __user *)buf, pm.buffer, len) < 0) {
    > + ret = -EFAULT;
    > + goto out_free;
    > + }
    > + copied += len;
    > + buf += len;
    > + count -= len;
    > }
    > + *ppos += copied;
    > + if (!ret || ret == PM_END_OF_BUFFER)
    > + ret = copied;
    > +
    > out_free:
    > - kfree(pages);
    > + kfree(pm.buffer);
    > out_mm:
    > mmput(mm);
    > out_task:
    >





    \
     
     \ /
      Last update: 2010-04-01 08:07    [W:0.033 / U:0.108 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site