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

[Nigel Cunningham <ncunningham@linuxmail.org>]

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. 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.

> > +
> > +void suspend_put_extent_chain(struct extent_chain *chain);
> > +int suspend_add_to_extent_chain(struct extent_chain *chain, 
> > +		unsigned long minimum, unsigned long maximum);
> > +
> > +/* swap_entry_to_extent_val & extent_val_to_swap_entry: 
> > + * We are putting offset in the low bits so consecutive swap entries
> > + * make consecutive extent values */
> > +#define swap_entry_to_extent_val(swp_entry) (swp_entry.val)
> > +#define extent_val_to_swap_entry(val) (swp_entry_t) { (val) }
> 
> These two macros are also used only once each.  I'd just use the values
> directly.

Ok. Thanks. I think they're a leftover from 2.4 support :)

Nigel