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.
> +
> + 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).
> +
> + 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().
> +{
> + 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.
> + chain->first = chain->last_touched = NULL;
This is against the coding style AFAICT. Please do
chain->first = NULL;
chain->last_touched = NULL;
> + 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'.
> +{
> + 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;
> +
> + 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:
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;
> + }
> +
> + 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.
> +
> + 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.
> + } 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.
> +
> +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.
> +#endif
That's all. :-)
Greetings,
Rafael
--
You never change things by fighting the existing reality.
R. Buckminster Fuller
-
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]