Re: [PATCH] Use extents for recording what swap is allocated.

["Rafael J. Wysocki" <rjw@sisk.pl>]

Hi,

On Wednesday, 25 October 2006 00:13, Nigel Cunningham wrote:
> Hi.
> 
> On Tue, 2006-10-24 at 22:08 +0200, Rafael J. Wysocki wrote:
> > On Monday, 23 October 2006 06:14, Nigel Cunningham wrote:
> > > Switch from bitmaps to using extents to record what swap is allocated;
> > > they make more efficient use of memory, particularly where the allocated
> > > storage is small and the swap space is large.
> > 
> > As I said before, I like the overall idea, but I have a bunch of comments.
> 
> Thanks for them. Just a quick reply for the moment to say they're
> appreciated and I will revise accordingly.
> 
> I should also mention that this isn't the only use of these functions in
> Suspend2.

Could we please focus on things that are on the table _now_?.  You are
submitting the patch aganist the current code and I can only review it
in this context.  I can't say if I like your _future_ patches at this moment! :-)

> There I also use extents to record the blocks to which the 
> image will be written. I hope to submit modifications to swsusp to do
> that too in the near future.
> 
> > > +/* Simplify iterating through all the values in an extent chain */
> > > +#define suspend_extent_for_each(extent_chain, extentpointer, value) \
> > > +if ((extent_chain)->first) \
> > > +	for ((extentpointer) = (extent_chain)->first, (value) = \
> > > +			(extentpointer)->minimum; \
> > > +	     ((extentpointer) && ((extentpointer)->next || (value) <= \
> > > +				 (extentpointer)->maximum)); \
> > > +	     (((value) == (extentpointer)->maximum) ? \
> > > +		((extentpointer) = (extentpointer)->next, (value) = \
> > > +		 ((extentpointer) ? (extentpointer)->minimum : 0)) : \
> > > +			(value)++))
> > 
> > This macro doesn't look very nice and is used only once, so I think you
> > can drop it and just write the loop where it belongs.
> 
> With the modifications I mentioned just above, this would also be used
> for getting the blocks which match each swap extent. I can remove the
> macro, but just want to make you aware that it does serve a purpose,
> you're just not seeing it fully yet.

Can we just assume there are no other patches and proceed under this
assumption?

Could you please remove the macro for now?  You can introduce it with the
other patches when you submit them (if it's still needed at that time).

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller