Re: Which is simpler? (Was Re: [Suspend2-devel] Re: [ 00/10] [Suspend2] Modules support.)

[Nigel Cunningham <ncunningham@cyclades.com>]

Hi.

On Thursday 23 February 2006 04:49, Rafael J. Wysocki wrote:
> > > Frankly, I didn't think of dropping PBEs right now, but in the long run
> > > that's worth considering, IMO.  The advantage of PBEs is that they are
> > > easy to handle in the assembly parts, but apart from this they are a
> > > bit wasteful (not very much, though).
> >
> > Fully agree. That's why I've sought to keep the copying in c - it makes
> > it simpler to read and maintain (although at the expense of a little bit
> > of ugliness with that if in the stack page allocation
>
> Well, that's a bit too much ugliness for me, sorry.
>
> > or (old way) working hard to make the C not use stack).
>
> I'd rather not get rid of the assembly parts.  Instead, I'd modify them to
> handle bitmaps.  I'm not going to drop them.

Well, if you do this, and I can, I will start using the code too. I don't know 
x86/x86_64/ppc/... assembly enough that I could help, but I would be willing 
to drop the current code if yours was usable. Come to that, (as I already 
said), I'm willing to work on switching to pbes prior to that and seeing if 
we can share that code earlier, if you want me to and I can find the time.

> > > The fact that we use page flags to store some suspend/resume-related
> > > information is a big disadvantage in my view, and I'd like to get rid
> > > of that in the future.  In principle we could use a bitmap, or rather
> > > two of them, to store the same information independently of the page
> > > flags, and if we use bitmaps for this purpose, we can use them also
> > > instead of PBEs.
> >
> > If you use the 'dynamically allocated pageflags' code (sure, pick a
> > better name if you want), these changes will be pretty trivial - you can
> > #define macros that could make the transition just a matter of switching
> > PageNosave (eg) to PageSomethingElse. (Ditto for setting and clearing
> > flags).
>
> I think it could be done without that code and I'd prefer to do so.  In
> fact, we only need to remember:
> (a) saveable pages
> (b) pages used to store the data from (a)
> (c) pages allocated by us that we should release eventually
> (generally that may be a broader set than just (b)).
> That's 3 bitmaps total and no need for using any more sophisticated stuff,
> if I remember everything correctly.

Maybe I have tunnel vision, but I'd be surprised if you didn't end up with 
something similar - I've tried to make it as simple as possible, and am 
basically doing the same thing (even if I'm using different terms for some of 
the concepts). I'd certainly be willing to interact on "Why did you do it 
this way?" questions and make changes if a better way is shown to me.

> > > At this point I'd have to look at your snapshot-related code and see if
> > > it's suitable for snapshot.c (in -mm now) somehow.  If you could point
> > > me to the specific parts of the suspend2 patch where this code is, I'd
> > > be grateful.
> >
> > Sure. The bulk is in kernel/power/atomic_copy.c. Arch specific routines
> > are include/asm-<arch>/suspend2.h.
>
> OK, thanks.

Thank you! I much prefer this kind of interaction. It's far more constructive.

Nigel

Attachment: pgpzKsdhUHx5O.pgp
Description: PGP signature