On Thursday 06 July 2006 16:26, Rafael J. Wysocki wrote:
> On Thursday 06 July 2006 16:19, David Brownell wrote:
> >
> > > > > > I have seen that before: Atomic snapshot used fpu copy in some wrong
> > > > > > variants. Symptom was exactly that -- elevated preempt count --
> > > > > > because fpu copy routine elevated it, then copied the task struct.
> > > > > >
> > > > > > But I thought we solved that problem...?
> > > > >
> > > > > We did. We don't use memcpy for precisely that reason. 3DNOW memcpy was one
> > > > > of the problem children. This would be a different creature though,
> > > > > wouldn't it?
> > > >
> > > > Hmm. Aparently we had a parting of ways on this at some point. Memcpy is being
> > > > used by swsusp, and it has been used since before 2.6.12-rc1. (This is when
> > > > doing the atomic copy, not resuming).
> >
> > And it could well be that's when this bug appeared. It's on an Athlon,
> > so that theory checks out as well as possible short of a patch.
> >
> >
> > > Do you mean the one in copy_data_pages()? Indeed, that may be a problem if
> > > the MMU-based memcpy is used.
> > >
> > > Pavel, should we fix this?
> >
> > Of course it needs fixing ... it's a bug, also a regression.
> >
> > My question is where to fix... swsusp_arch_resume() seems most
> > correct, albeit messy. There's unfortunately no exact parallel
> > on the resume side to where the bug was inserted. Those of us
> > who avoid hacking asm code might prefer restore_processor_state().
>
> Well, I meant replacing the memcpy() in copy_data_pages with an open coded
> copying loop. That should be enough to fix the problem.
To be more specific, could you please check if the appended patch (tested
on x86_64) helps?
Rafael
kernel/power/snapshot.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
Index: linux-2.6.17-mm6/kernel/power/snapshot.c
===================================================================
--- linux-2.6.17-mm6.orig/kernel/power/snapshot.c
+++ linux-2.6.17-mm6/kernel/power/snapshot.c
@@ -227,11 +227,17 @@ static void copy_data_pages(struct pbe *
for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
if (saveable(zone, &zone_pfn)) {
struct page *page;
+ long *src, *dst;
+ int n;
+
page = pfn_to_page(zone_pfn + zone->zone_start_pfn);
BUG_ON(!pbe);
pbe->orig_address = (unsigned long)page_address(page);
- /* copy_page is not usable for copying task structs. */
- memcpy((void *)pbe->address, (void *)pbe->orig_address, PAGE_SIZE);
+ /* copy_page and memcpy are not usable for copying task structs. */
+ dst = (long *)pbe->address;
+ src = (long *)pbe->orig_address;
+ for (n = PAGE_SIZE / sizeof(long); n; n--)
+ *dst++ = *src++;
pbe = pbe->next;
}
}
