Re: [PATCH -mm] swsusp: support creating bigger images

[Rafael J. Wysocki]

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@xxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/