Re: [PATCH] swsusp: separate swap-writing/reading code

Re: [PATCH] swsusp: separate swap-writing/reading code

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

Hi.

On Friday 24 March 2006 03:02, Linux Kernel Mailing List wrote:
> commit 61159a314bca6408320c3173c1282c64f5cdaa76
> tree 8e1b7627443da0fd52b2fac66366dde9f7871f1e
> parent f577eb30afdc68233f25d4d82b04102129262365
> author Rafael J. Wysocki <rjw@sisk.pl> Thu, 23 Mar 2006 19:00:00 -0800
> committer Linus Torvalds <torvalds@g5.osdl.org> Thu, 23 Mar 2006 23:38:07
> -0800
>
> [PATCH] swsusp: separate swap-writing/reading code
>
> Move the swap-writing/reading code of swsusp to a separate file.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>

I guess I missed this one somehow. Using a bitmap for allocated swap is really 
inefficient because the values are usually not fragmented much. Extents would 
have been a far better choice.

Regards,

Nigel

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] swsusp: separate swap-writing/reading code

[Rafael J. Wysocki]

Hi,

On Thursday 23 March 2006 22:13, Nigel Cunningham wrote:
> On Friday 24 March 2006 03:02, Linux Kernel Mailing List wrote:
> > commit 61159a314bca6408320c3173c1282c64f5cdaa76
> > tree 8e1b7627443da0fd52b2fac66366dde9f7871f1e
> > parent f577eb30afdc68233f25d4d82b04102129262365
> > author Rafael J. Wysocki <rjw@sisk.pl> Thu, 23 Mar 2006 19:00:00 -0800
> > committer Linus Torvalds <torvalds@g5.osdl.org> Thu, 23 Mar 2006 23:38:07
> > -0800
> >
> > [PATCH] swsusp: separate swap-writing/reading code
> >
> > Move the swap-writing/reading code of swsusp to a separate file.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: Andrew Morton <akpm@osdl.org>
> > Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> 
> I guess I missed this one somehow. Using a bitmap for allocated swap is really 
> inefficient because the values are usually not fragmented much. Extents would 
> have been a far better choice.

I agree it probably may be improved.  Still it seems to be good enough.  Further,
it's more efficient than the previous solution, so I consider it as an improvement.
Also this code has been tested for quite some time in -mm and appears to
behave properly, at least we haven't got any bug reports related to it so far.

Currently I'm not working on any better solution.  If you can provide any
patches to implement one, please submit them, but I think they'll have to be
tested for as long as this code, in -mm.

Greetings,
Rafael

Re: [PATCH] swsusp: separate swap-writing/reading code

[Mark Lord]

Rafael J. Wysocki wrote:
>
> I agree it probably may be improved.  Still it seems to be good enough.  Further,
> it's more efficient than the previous solution, so I consider it as an improvement.
> Also this code has been tested for quite some time in -mm and appears to
> behave properly, at least we haven't got any bug reports related to it so far.

I find the in-kernel swsusp to be quite slow, and it seems to use
an awful lot of memory for book-keeping.  So count that as encouragement
to improve the performance when you can.

> Currently I'm not working on any better solution.  If you can provide any
> patches to implement one, please submit them, but I think they'll have to be
> tested for as long as this code, in -mm.

It would be *really nice* if you guys could stop being so underhandedly
nasty in every single reply to anything from Nigel.

He really is trying to help, you know.

Cheers

Re: [PATCH] swsusp: separate swap-writing/reading code

[Rafael J. Wysocki]

Hi,

On Thursday 23 March 2006 23:48, Mark Lord wrote:
> Rafael J. Wysocki wrote:
> >
> > I agree it probably may be improved.  Still it seems to be good enough.  Further,
> > it's more efficient than the previous solution, so I consider it as an improvement.
> > Also this code has been tested for quite some time in -mm and appears to
> > behave properly, at least we haven't got any bug reports related to it so far.
> 
> I find the in-kernel swsusp to be quite slow, and it seems to use
> an awful lot of memory for book-keeping.  So count that as encouragement
> to improve the performance when you can.

This particular patch actually decreases the amount of memory used by swsusp.

Moreover I have _nothing_ against improvements, but it requires some time to
improve things.

> > Currently I'm not working on any better solution.  If you can provide any
> > patches to implement one, please submit them, but I think they'll have to be
> > tested for as long as this code, in -mm.
> 
> It would be *really nice* if you guys could stop being so underhandedly
> nasty in every single reply to anything from Nigel.

Well, you know, it's generally easy to say that something's done in a wrong
way, but this alone doesn't help _anyone_.

Suggestions are nice, but _someone_ has to implement them and I think
Nigel is more than capable of doing it in this particular case.  Also the code
in question is quite sensitive and such that it should be tested for a longer
time IMO.

That's what I was trying to say and it was not my intention to be nasty at all.

Greetings,
Rafael

Re: [PATCH] swsusp: separate swap-writing/reading code

[Pavel Machek]

On Čt 23-03-06 17:48:58, Mark Lord wrote:
> Rafael J. Wysocki wrote:
> >
> >I agree it probably may be improved.  Still it seems to be good enough.  
> >Further,
> >it's more efficient than the previous solution, so I consider it as an 
> >improvement.
> >Also this code has been tested for quite some time in -mm and appears to
> >behave properly, at least we haven't got any bug reports related to it so 
> >far.
> 
> I find the in-kernel swsusp to be quite slow, and it seems to use
> an awful lot of memory for book-keeping.  So count that as encouragement
> to improve the performance when you can.

Extents will provide 0.01% speedup at most, and with increase of code
complexity. Not a nice tradeoff if you ask me.

If you want faster suspend, that should be easy. You'll need *current*
2.6.16-git , and userland tools from suspend.sf.net . There's HOWTO
that explains how to set it up. We can even do LZF these days... 

> >Currently I'm not working on any better solution.  If you can provide any
> >patches to implement one, please submit them, but I think they'll have to 
> >be
> >tested for as long as this code, in -mm.
> 
> It would be *really nice* if you guys could stop being so underhandedly
> nasty in every single reply to anything from Nigel.

> He really is trying to help, you know.

Actually Rafael was *very* nice at him, I'd say. Pointing for tiny
inefficiencies, without patch attached is not really helpful.

I have repeatedly pointed him on ways how he can *really* help. There
are ways to do suspend2 in userspace these days, but Nigel refuses to
use them.
								Pavel
-- 
Picture of sleeping (Linux) penguin wanted...

Re: [PATCH] swsusp: separate swap-writing/reading code

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]

Hi Pavel.

On Monday 27 March 2006 20:26, Pavel Machek wrote:
> On Čt 23-03-06 17:48:58, Mark Lord wrote:
> > Rafael J. Wysocki wrote:
> > >I agree it probably may be improved.  Still it seems to be good enough.
> > >Further,
> > >it's more efficient than the previous solution, so I consider it as an
> > >improvement.
> > >Also this code has been tested for quite some time in -mm and appears to
> > >behave properly, at least we haven't got any bug reports related to it
> > > so far.
> >
> > I find the in-kernel swsusp to be quite slow, and it seems to use
> > an awful lot of memory for book-keeping.  So count that as encouragement
> > to improve the performance when you can.
>
> Extents will provide 0.01% speedup at most, and with increase of code
> complexity. Not a nice tradeoff if you ask me.

My point wasn't speed, but efficient use of memory. A bitmap is certainly 
better than storing n*sizeof(swap_entry_t) byes. Extents would be better 
again, but perhaps not as big after the bitmap switch.

> If you want faster suspend, that should be easy. You'll need *current*
> 2.6.16-git , and userland tools from suspend.sf.net . There's HOWTO
> that explains how to set it up. We can even do LZF these days...
>
> > >Currently I'm not working on any better solution.  If you can provide
> > > any patches to implement one, please submit them, but I think they'll
> > > have to be
> > >tested for as long as this code, in -mm.
> >
> > It would be *really nice* if you guys could stop being so underhandedly
> > nasty in every single reply to anything from Nigel.
> >
> > He really is trying to help, you know.
>
> Actually Rafael was *very* nice at him, I'd say. Pointing for tiny
> inefficiencies, without patch attached is not really helpful.

Yes, I didn't think Rafael was harsh in that email, but that's just my 
opinion. Regarding "without patch attached", could you please remember that 
comment next time you comment on my patches?

> I have repeatedly pointed him on ways how he can *really* help. There
> are ways to do suspend2 in userspace these days, but Nigel refuses to
> use them.

You know that I disagree that doing suspend in userspace is the right 
approach, and you know that current uswsusp can't do everything Suspend2 does 
without further substantial modification. Please stop painting me as the bad 
guy because I won't roll over and play dead for you. Please also stop 
encouraging people to use uswsusp when you have also warned that it might eat 
their partitions.

Regards,

Nigel

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

regular swsusp flamewar [was Re: [PATCH] swsusp: separate swap-writing/reading code]

[Pavel Machek]

Hi!

> > If you want faster suspend, that should be easy. You'll need *current*
> > 2.6.16-git , and userland tools from suspend.sf.net . There's HOWTO
> > that explains how to set it up. We can even do LZF these days...
> >
> > > >Currently I'm not working on any better solution.  If you can provide
> > > > any patches to implement one, please submit them, but I think they'll
> > > > have to be
> > > >tested for as long as this code, in -mm.
> > >
> > > It would be *really nice* if you guys could stop being so underhandedly
> > > nasty in every single reply to anything from Nigel.
> > >
> > > He really is trying to help, you know.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> > Actually Rafael was *very* nice at him, I'd say. Pointing for tiny
> > inefficiencies, without patch attached is not really helpful.
...
> > I have repeatedly pointed him on ways how he can *really* help. There
> > are ways to do suspend2 in userspace these days, but Nigel refuses to
> > use them.
> 
> You know that I disagree that doing suspend in userspace is the
> right 

You know "disagreeing" with subsystem maintainer (and everyone else
for that matter) is not exactly helpful in getting patches merged. You
are free to believe whatever you want, but if you disagree on
something as fundamental as "do not put unneccessary code to kernel"
with me, it should be no surprise that I "disagree" with your patches
(*).

> approach, and you know that current uswsusp can't do everything Suspend2 does 
> without further substantial modification. Please stop painting me as the bad 
> guy because I won't roll over and play dead for you. Please also
> stop 

I'm not trying to paint you as a bad guy. But Mark said you are trying
to help, and in that context I'd read it as "trying to help mainline
development". And you are not doing that, you are developing your own
suspend2 branch, that has nothing to do with mainline. I think we can
agree on that one...

> encouraging people to use uswsusp when you have also warned that it might eat 
> their partitions.

uswsusp is now reasonably stable. Of course, it was dangerous some
time ago. Every software is dangerous when it is young.
								Pavel

(*) Actually I can't "disagree" with your patches because you did not
submit any, in recent history.
-- 
Picture of sleeping (Linux) penguin wanted...

Re: regular swsusp flamewar [was Re: [PATCH] swsusp: separate swap-writing/reading code]

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

Hello.

I'm not playing that game again.

Instead I'm letting you know that I'm building a git tree at the moment, and 
hope to start posting patches from it shortly and seeking to merge Suspend2. 
A few weeks ago I lacked the motivation to do it, but that has since changed.

If you want to provide useful, technical comments on how I can do things 
better, I'll be happy to accept and apply them. If on the other hand you 
attempt to start another flamewar, I'll just seek to exercise my self control 
and let those comments fall to the floor.

Regards,

Nigel

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: regular swsusp flamewar

[Harald Arnesen]

Nigel Cunningham <ncunningham@cyclades.com> writes:

> Hello.
>
> I'm not playing that game again.
>
> Instead I'm letting you know that I'm building a git tree at the moment, and 
> hope to start posting patches from it shortly and seeking to merge Suspend2. 
> A few weeks ago I lacked the motivation to do it, but that has since changed.
>
> If you want to provide useful, technical comments on how I can do things 
> better, I'll be happy to accept and apply them. If on the other hand you 
> attempt to start another flamewar, I'll just seek to exercise my self control 
> and let those comments fall to the floor.

_Please_ continue, Nigel! We (at least I) need a working suspend, and it
would be a great benefit to have it in the vanilla kernel.
-- 
Hilsen Harald.

Re: regular swsusp flamewar [was Re: [PATCH] swsusp: separate swap-writing/reading code]

[Pavel Machek]

Hi!

> I'm not playing that game again.

Good.

> Instead I'm letting you know that I'm building a git tree at the moment, and 
> hope to start posting patches from it shortly and seeking to merge Suspend2. 
> A few weeks ago I lacked the motivation to do it, but that has since changed.

Please cc me when the patches are ready, and good luck with git.
								Pavel
-- 
Picture of sleeping (Linux) penguin wanted...

Re: regular swsusp flamewar [was Re: [PATCH] swsusp: separate swap-writing/reading code]

[Pavel Machek]

On Út 28-03-06 09:36:26, Nigel Cunningham wrote:
> Hello.
> 
> I'm not playing that game again.
> 
> Instead I'm letting you know that I'm building a git tree at the moment, and 
> hope to start posting patches from it shortly and seeking to merge Suspend2. 
> A few weeks ago I lacked the motivation to do it, but that has since changed.

Just out of interest... what made you motivated? (Not that it is
important and feel free to tell me I don't need to know, or reply
privately...)

								Pavel

-- 
Picture of sleeping (Linux) penguin wanted...

Re: regular swsusp flamewar [was Re: [PATCH] swsusp: separate swap-writing/reading code]

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

Hi.

On Tuesday 28 March 2006 19:16, Pavel Machek wrote:
> On Út 28-03-06 09:36:26, Nigel Cunningham wrote:
> > Hello.
> >
> > I'm not playing that game again.
> >
> > Instead I'm letting you know that I'm building a git tree at the moment,
> > and hope to start posting patches from it shortly and seeking to merge
> > Suspend2. A few weeks ago I lacked the motivation to do it, but that has
> > since changed.
>
> Just out of interest... what made you motivated? (Not that it is
> important and feel free to tell me I don't need to know, or reply
> privately...)

You did.

Regards,

Nigel

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: regular swsusp flamewar

[Harald Arnesen]

Pavel Machek <pavel@suse.cz> writes:

>> You know that I disagree that doing suspend in userspace is the
>> right 
>
> You know "disagreeing" with subsystem maintainer (and everyone else
> for that matter) is not exactly helpful in getting patches merged. You
> are free to believe whatever you want, but if you disagree on
> something as fundamental as "do not put unneccessary code to kernel"
> with me, it should be no surprise that I "disagree" with your patches
> (*).
>
>> approach, and you know that current uswsusp can't do everything Suspend2 does 
>> without further substantial modification. Please stop painting me as the bad 
>> guy because I won't roll over and play dead for you. Please also
>> stop 
>
> I'm not trying to paint you as a bad guy. But Mark said you are trying
> to help, and in that context I'd read it as "trying to help mainline
> development". And you are not doing that, you are developing your own
> suspend2 branch, that has nothing to do with mainline. I think we can
> agree on that one...

The main point for me is that suspend2 works, while mainline supspend
does not ("works": it takes less time to suspend/resume than to
shutdown/reboot).

I haven't tried uswsusp yet. Why try another out-of-kernel suspend when
suspend2 works perfectly?
-- 
Hilsen Harald.

Re: regular swsusp flamewar

[Pavel Machek]

On Út 28-03-06 03:16:22, Harald Arnesen wrote:

> The main point for me is that suspend2 works, while mainline supspend
> does not ("works": it takes less time to suspend/resume than to
> shutdown/reboot).
> 
> I haven't tried uswsusp yet. Why try another out-of-kernel suspend when
> suspend2 works perfectly?

uswsusp is not out-of-tree any more (but you'll need recent tree from
git). You should try in-kernel options before patching your kernel,
I'd say. Userland tools are at suspend.sf.net.

								Pavel
-- 
Picture of sleeping (Linux) penguin wanted...

Re: [PATCH] swsusp: separate swap-writing/reading code

[Andrew Morton]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> > I guess I missed this one somehow. Using a bitmap for allocated swap is really 
> > inefficient because the values are usually not fragmented much. Extents would 
> > have been a far better choice.
> 
> I agree it probably may be improved.  Still it seems to be good enough.  Further,
> it's more efficient than the previous solution, so I consider it as an improvement.
> Also this code has been tested for quite some time in -mm and appears to
> behave properly, at least we haven't got any bug reports related to it so far.

I think that temporarily allocating 1/32768th of total memory here is
reasonable, especially as it's not all allocated in a contiguous hunk.

> Currently I'm not working on any better solution.  If you can provide any
> patches to implement one, please submit them, but I think they'll have to be
> tested for as long as this code, in -mm.

I was a little saddened by the open-coded approach.  I'd expect that both
radix-trees and idr-trees could be used in this application.  Probably the
former.  (Radix-trees should have been designed from day one to store
`unsigned long's, not void*'s, so unless we change that, this application
will need to use typecasts when converting between void*'s and the stored
BITS_PER_LONG bitmaps).

Re: [PATCH] swsusp: separate swap-writing/reading code

[Rafael J. Wysocki]

On Friday 24 March 2006 00:58, Andrew Morton wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> >
> > > I guess I missed this one somehow. Using a bitmap for allocated swap is really 
> > > inefficient because the values are usually not fragmented much. Extents would 
> > > have been a far better choice.
> > 
> > I agree it probably may be improved.  Still it seems to be good enough.  Further,
> > it's more efficient than the previous solution, so I consider it as an improvement.
> > Also this code has been tested for quite some time in -mm and appears to
> > behave properly, at least we haven't got any bug reports related to it so far.
> 
> I think that temporarily allocating 1/32768th of total memory here is
> reasonable, especially as it's not all allocated in a contiguous hunk.
> 
> > Currently I'm not working on any better solution.  If you can provide any
> > patches to implement one, please submit them, but I think they'll have to be
> > tested for as long as this code, in -mm.
> 
> I was a little saddened by the open-coded approach.  I'd expect that both
> radix-trees and idr-trees could be used in this application.  Probably the
> former.  (Radix-trees should have been designed from day one to store
> `unsigned long's, not void*'s, so unless we change that, this application
> will need to use typecasts when converting between void*'s and the stored
> BITS_PER_LONG bitmaps).

Perhaps we can use radix-trees here.  Frankly, I haven't investigated this
possibility yet, because this part is not very high on my priority list for
improvements. :-)

First, I'd like to get the userland suspend out so people can benefit from the
userland interface.  Second, I think we can save much more memory if we
use bitmaps for the image metadata, but that will require a lot of work.
Then, there's the problem of the system responsiveness after resume,
and the "USB storage not available after resume" problem that I wouldn't
like to drop on the floor.  Not to mention device driver problems.

Greetings,
Rafael