Re: [PATCH] -mm swsusp: copy_page is harmfull

Re: [PATCH] -mm swsusp: copy_page is harmfull

[Benjamin Herrenschmidt]

On Sat, 2004-09-25 at 10:27, Linux Kernel Mailing List wrote:
> ChangeSet 1.1983.1.3, 2004/09/24 17:27:41-07:00, akpm@osdl.org
> 
> 	[PATCH] -mm swsusp: copy_page is harmfull
> 	
> 	From: Pavel Machek <pavel@ucw.cz>
> 	
> 	This is my fault from long time ago: copy_page can't be used for copying
> 	task struct, therefore we can't use it in swsusp.

Hi !

Just curious, but why ?

It would be useful to have this in platform code, I don't see why I couldn't
use copy_page() on ppc and I suspect it will be more efficient than memcpy
since it has specific optimisations due to the fact that we are known to be
fully aligned and the size of the copy is a constant aligned power of 2.

Ben.

Re: [PATCH] -mm swsusp: copy_page is harmfull

[Nigel Cunningham]

Hi.

On Mon, 2004-10-04 at 09:50, Benjamin Herrenschmidt wrote:
> On Sat, 2004-09-25 at 10:27, Linux Kernel Mailing List wrote:
> > ChangeSet 1.1983.1.3, 2004/09/24 17:27:41-07:00, akpm@osdl.org
> > 
> > 	[PATCH] -mm swsusp: copy_page is harmfull
> > 	
> > 	From: Pavel Machek <pavel@ucw.cz>
> > 	
> > 	This is my fault from long time ago: copy_page can't be used for copying
> > 	task struct, therefore we can't use it in swsusp.
> 
> Hi !
> 
> Just curious, but why ?
> 
> It would be useful to have this in platform code, I don't see why I couldn't
> use copy_page() on ppc and I suspect it will be more efficient than memcpy
> since it has specific optimisations due to the fact that we are known to be
> fully aligned and the size of the copy is a constant aligned power of 2.

I think I can answer that one, seeing as Pavel seems to be asleep at the
mo :>.

On x86 at least (perhaps your platform is different), copy_page can
resolve to different implementations. If you have 3D_NOW, the preempt
count will be incremented prior to copying the page via a fpu_begin
call. Sooner or later in doing the atomic copy of memory, you'll be
copying the page containing the preempt count (the task struct) for the
process doing the suspending. At that stage, you'll copy the preempt
count being one too high, and at resume time it will still be one too
high (we definitely can't use copy_page then). The simple solution would
be to use copy_page and have something like

#ifdef 3D_NOW
	dec_preempt_count();
#endif

But I'm sure you'll agree that even if it's faster, it's less clear and
uglier.

Regards,

Nigel

Re: [PATCH] -mm swsusp: copy_page is harmfull

[Pavel Machek]

Hi!

> > 	[PATCH] -mm swsusp: copy_page is harmfull
> > 	
> > 	From: Pavel Machek <pavel@ucw.cz>
> > 	
> > 	This is my fault from long time ago: copy_page can't be used for copying
> > 	task struct, therefore we can't use it in swsusp.
> 
> Hi !
> 
> Just curious, but why ?

Nigel already provided right explanation.

> It would be useful to have this in platform code, I don't see why I couldn't
> use copy_page() on ppc and I suspect it will be more efficient than memcpy
> since it has specific optimisations due to the fact that we are known to be
> fully aligned and the size of the copy is a constant aligned power of 2.

Hmm but does it matter? Is that operation really that slow on ppc?
This whole copy takes maybe second on x86, other operations are way
slower...
									Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [PATCH] -mm swsusp: copy_page is harmfull

[Benjamin Herrenschmidt]

On Mon, 2004-10-04 at 19:33, Pavel Machek wrote:

> Hmm but does it matter? Is that operation really that slow on ppc?
> This whole copy takes maybe second on x86, other operations are way
> slower...

No, you are right, it probably doesn't matter.

Ben.