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.257 / U:0.476 seconds]
©2003-2011 Jasper Spaans. Advertise on this site