lkml.org 
[lkml]   [2006]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH -mm] swsusp: support creating bigger images
    Date
    On Tuesday 09 May 2006 14:30, Hugh Dickins wrote:
    > On Tue, 9 May 2006, Rafael J. Wysocki wrote:
    > > On Tuesday 09 May 2006 09:33, Andrew Morton wrote:
    > > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
    > > >
    > > > I have a host of minor problems with this patch.
    > > >
    > > > > --- linux-2.6.17-rc3-mm1.orig/mm/rmap.c
    > > > > +++ linux-2.6.17-rc3-mm1/mm/rmap.c
    > > > >
    > > > > +#ifdef CONFIG_SOFTWARE_SUSPEND
    > > > > +static int page_mapped_by_task(struct page *page, struct task_struct *task)
    > > > > +{
    > > > > + struct vm_area_struct *vma;
    > > > > + int ret = 0;
    > > > > +
    > > > > + spin_lock(&task->mm->page_table_lock);
    > > > > +
    > > > > + for (vma = task->mm->mmap; vma; vma = vma->vm_next)
    > > > > + if (page_address_in_vma(page, vma) != -EFAULT) {
    > > > > + ret = 1;
    > > > > + break;
    > > > > + }
    > > > > +
    > > > > + spin_unlock(&task->mm->page_table_lock);
    > > > > +
    > > > > + return ret;
    > > > > +}
    > > >
    > > > task_struct.mm can sometimes be NULL. This function assumes that it will
    > > > never be NULL. That makes it a somewhat risky interface. Are we sure it
    > > > can never be NULL?
    > >
    > > Well, now it's only called for task == current, but I can add a check.
    >
    > Better fold it into the (renamed and recommented) page_to_copy,
    > applying only to current.
    >
    > The "use" of page_table_lock there is totally bogus. Normally you
    > need down_read(&current->mm->mmap_sem) to walk that vma chain; but
    > I'm guessing you have everything sufficiently frozen here that you
    > don't need that.

    So I think it can be changed into something like this:

    --- linux-2.6.17-rc3-mm1.orig/mm/rmap.c 2006-05-01 14:11:50.000000000 +0200
    +++ linux-2.6.17-rc3-mm1/mm/rmap.c 2006-05-10 23:10:58.000000000 +0200
    @@ -857,3 +857,38 @@ int try_to_unmap(struct page *page, int
    return ret;
    }

    +#ifdef CONFIG_SOFTWARE_SUSPEND
    +/**
    + * suspend_safe_page - determine if a page can be included in the
    + * suspend image without copying (returns true if so).
    + *
    + * It is safe to include the page in the suspend image without
    + * copying if (a) it's on the LRU and (b) it's mapped by a frozen task
    + * (all tasks except for the current task should be frozen when it's
    + * called). Otherwise the page should be copied for this purpose
    + * (compound pages are avoided for simplicity).
    + */
    +
    +int suspend_safe_page(struct page *page)
    +{
    + struct vm_area_struct *vma;
    + int ret = 0;
    +
    + if (!PageLRU(page) || PageCompound(page))
    + return 0;
    +
    + if (page_mapped(page)) {
    + ret = 1;
    + down_read(&current->mm->mmap_sem);
    + /* Return 0 if the page is mapped by the current task */
    + for (vma = current->mm->mmap; vma; vma = vma->vm_next)
    + if (page_address_in_vma(page, vma) != -EFAULT) {
    + ret = 0;
    + break;
    + }
    + up_read(&current->mm->mmap_sem);
    + }
    +
    + return ret;
    +}
    +#endif /* CONFIG_SOFTWARE_SUSPEND */
    I've left the locking, because the functions is called when we're freeing
    memory and I'm not 100% sure it's safe to drop it.

    > But if it is sufficiently frozen, I'm puzzled as to why pages mapped
    > into the current process are (potentially) unsafe, while those mapped
    > into others are safe. If the current process can get back to messing
    > with its mapped pages, what if it maps a page you earlier judged safe?

    The current task is forbidden to map anything at this point.

    Greetings,
    Rafael

    -
    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/

    \
     
     \ /
      Last update: 2006-05-11 01:01    [W:0.035 / U:0.320 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site