Messages in this thread Patch in this message |  | | | From | "Rafael J. Wysocki" <> | | Subject | Re: [PATCH -mm] swsusp: support creating bigger images | | Date | Thu, 11 May 2006 00:26:47 +0200 |
| |
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(¤t->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(¤t->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(¤t->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/
|  |