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.
>
> > This is also part of the ground work for implementing support for
> > supporting multiple swap devices.
> >
> > Signed-off-by: Nigel Cunningham <[email protected]>
> >
> > diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> > index 38725f5..d772521 100644
> > --- a/kernel/power/Makefile
> > +++ b/kernel/power/Makefile
> > @@ -5,6 +5,6 @@ endif
> >
> > obj-y := main.o process.o console.o
> > obj-$(CONFIG_PM_LEGACY) += pm.o
> > -obj-$(CONFIG_SOFTWARE_SUSPEND) += swsusp.o disk.o snapshot.o swap.o user.o
> > +obj-$(CONFIG_SOFTWARE_SUSPEND) += swsusp.o disk.o snapshot.o swap.o user.o extent.o
> >
> > obj-$(CONFIG_MAGIC_SYSRQ) += poweroff.o
> > diff --git a/kernel/power/extent.c b/kernel/power/extent.c
> > new file mode 100644
> > index 0000000..b769956
> > --- /dev/null
> > +++ b/kernel/power/extent.c
> > @@ -0,0 +1,119 @@
> > +/*
> > + * kernel/power/extent.c
> > + *
> > + * Copyright (C) 2003-2006 Nigel Cunningham <[email protected]>
> > + * Copyright (C) 2006 Red Hat, inc.
> > + *
> > + * Distributed under GPLv2.
> > + *
> > + * These functions encapsulate the manipulation of storage metadata.
> > + */
> > +
> > +#include <linux/suspend.h>
> > +#include "extent.h"
> > +
> > +/* suspend_get_extent
> > + *
> > + * Returns a free extent. May fail, returning NULL instead.
> > + */
> > +static struct extent *suspend_get_extent(void)
> > +{
> > + struct extent *result;
>
> I'd call the type 'struct suspend_extent', for clarity.
Done, thanks for the suggestion.
> > +
> > + if (!(result = kmalloc(sizeof(struct extent), GFP_ATOMIC)))
> > + return NULL;
>
> if you use kzalloc() here, the initializations below won't be necessary and
> the function will reduce to a one-liner (in which case it can be inlined).
Done.
> > +
> > + result->minimum = result->maximum = 0;
> > + result->next = NULL;
> > +
> > + return result;
> > +}
> > +
> > +/* suspend_put_extent_chain.
> > + *
> > + * Frees a whole chain of extents.
> > + */
> > +void suspend_put_extent_chain(struct extent_chain *chain)
>
> I'd call it suspend_free_all_extents().
Renamed as suspend_free_extent_chain (I'm thinking of future use of
multiple chains for multiple swap devices).
> > +{
> > + struct extent *this;
> > +
> > + this = chain->first;
> > +
> > + while(this) {
> > + struct extent *next = this->next;
> > + kfree(this);
> > + chain->num_extents--;
> > + this = next;
> > + }
> > +
> > + BUG_ON(chain->num_extents);
>
> I'd drop this BUG_ON(). Once the code has been debugged, it's no longer
> of any use.
Done.
> > + chain->first = chain->last_touched = NULL;
>
> This is against the coding style AFAICT. Please do
>
> chain->first = NULL;
> chain->last_touched = NULL;
Done. Didn't know that, thanks.
> > + chain->size = 0;
> > +}
> > +
> > +/*
> > + * suspend_add_to_extent_chain
> > + *
> > + * Add an extent to an existing chain.
> > + */
> > +int suspend_add_to_extent_chain(struct extent_chain *chain,
> > + unsigned long minimum, unsigned long maximum)
>
> I'd use shorter names for these arguments, like 'start', 'end'.
min & max now used.
> > +{
> > + struct extent *new_extent = NULL, *start_at;
>
> The names are not the best here too, IMHO. I'd use something like
> *new_ext, *cur_ext.
>
> > +
> > + /* Find the right place in the chain */
> > + start_at = (chain->last_touched &&
> > + (chain->last_touched->minimum < minimum)) ?
> > + chain->last_touched : NULL;
> > +
> > + if (!start_at && chain->first && chain->first->minimum < minimum)
> > + start_at = chain->first;
>
> The above two statements can be simplified, like
>
> cur_ext = NULL;
> if (chain->last_touched && chain->last_touched->minimum < start)
> cur_ext = chain->last_touched;
> else if (chain->first && chain->first->minimum < start)
> cur_ext = chain->first;
Done, ta.
> > +
> > + while (start_at && start_at->next && start_at->next->minimum < minimum)
> > + start_at = start_at->next;
>
> You don't need to check if start_at is nonzero in every step. It's sufficient to
> check this once at the beginning.
>
> Moreover, the below is also only executed if start_at is nonzero,
> so you can put it under one if () along with the above loop, like this:
Done, ta.
> if (cur_ext) {
> while (cur_ext->next && cur_ext->next->minimum < start)
> cur_ext = cur_ext->next;
>
> if (cur_ext-> maximum == (start - 1)) {
> struct suspend_extent *next_ext;
>
> cur_ext->maximum = end;
> next_ext = cur_ext->next;
> if (next_ext && cur_ext->maximum == next_ext->minimum - 1) {
> cur_ext->maximum = next_ext->maximum;
> cur_ext->next = next_ext->next;
> kfree(next_ext);
> chain->num_extents--;
> }
>
> etc.
> > +
> > + if (start_at && start_at->maximum == (minimum - 1)) {
> > + start_at->maximum = maximum;
> > +
> > + /* Merge with the following one? */
> > + if (start_at->next &&
> > + start_at->maximum + 1 == start_at->next->minimum) {
> > + struct extent *to_free = start_at->next;
> > + start_at->maximum = start_at->next->maximum;
> > + start_at->next = start_at->next->next;
> > + chain->num_extents--;
> > + kfree(to_free);
> > + }
> > +
> > + chain->last_touched = start_at;
> > + chain->size+= (maximum - minimum + 1);
> > +
> > + return 0;
> > + }
> > +
> > + new_extent = suspend_get_extent();
> > + if (!new_extent) {
> > + printk("Error unable to append a new extent to the chain.\n");
> > + return 2;
>
> return -ENOMEM;
Mmm. Ta. Forgotten defunct debugging.
> > + }
> > +
> > + chain->num_extents++;
> > + chain->size+= (maximum - minimum + 1);
> > + new_extent->minimum = minimum;
> > + new_extent->maximum = maximum;
> > + new_extent->next = NULL;
>
> The last one won't be necessary if you use kzalloc() to allocate extents.
Ack.
> > +
> > + chain->last_touched = new_extent;
> > +
> > + if (start_at) {
> > + struct extent *next = start_at->next;
> > + start_at->next = new_extent;
> > + new_extent->next = next;
>
> *next is unnecessary here. You can do it like this:
>
> new_ext->next = cur_ext->next;
> cur_ext->next = new_ext;
>
> because new_ext->next is initially NULL.
k.
> > + } else {
> > + if (chain->first)
> > + new_extent->next = chain->first;
> > + chain->first = new_extent;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/kernel/power/extent.h b/kernel/power/extent.h
> > new file mode 100644
> > index 0000000..062b4c1
> > --- /dev/null
> > +++ b/kernel/power/extent.h
> > @@ -0,0 +1,50 @@
> > +/*
> > + * kernel/power/extent.h
> > + *
> > + * Copyright (C) 2004-2006 Nigel Cunningham <[email protected]>
> > + * Copyright (C) 2006 Red Hat, inc.
> > + *
> > + * This file is released under the GPLv2.
> > + *
> > + * It contains declarations related to extents. Extents are
> > + * suspend's method of storing some of the metadata for the image.
> > + * See extent.c for more info.
> > + *
> > + */
> > +
> > +#ifndef EXTENT_H
> > +#define EXTENT_H
> > +
> > +struct extent {
> > + unsigned long minimum, maximum;
>
> Well, I'd use shorter names, but whatever.
>
> > + struct extent *next;
> > +};
> > +
> > +struct extent_chain {
> > + int size; /* size of the chain ie sum (max-min+1) */
> > + int num_extents;
> > + struct extent *first, *last_touched;
> > +};
> > +
> > +/* 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.
Sort of done - expanded to two loops. 'tis more readable that way
anyway.
> > +
> > +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.
ack
> > +#endif
>
> That's all. :-)
Thanks.
> You never change things by fighting the existing reality.
> R. Buckminster Fuller
Hmm. Maybe I shouldn't bother with this then :)
Nigel
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
