2010-06-02 08:53:56

by Jiri Slaby

Subject: [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

Hi,

I addressed the comments I got on the previous RFC. I left the handles
in place, the functions in hibernate_io_ops now works on them. Further
I got rid of the memory barriers and minimized global variables as much
as possible. Comments welcome.

--

Some code, which will be moved out of swap.c, will know nothing about
swap. There will be also other than swap writers later, so that it
won't make sense at all.

So introduce a new structure called hibernate_io_handle which
currently contains only a pointer to private data, but is independent
on I/O reader/writer actually used. Private data are swap_map_handle
for now.

This structure is allocated in _start and freed in _finish. This will
correspond to the later introduction of hibernate_io_ops where users
will do handle=start->repeat{read/write(handle)}->finish(handle).
I.e. they will operate on handle instead of global variables.

Signed-off-by: Jiri Slaby <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
kernel/power/power.h | 24 ++++++++++
kernel/power/swap.c | 114 +++++++++++++++++++++++++++++++------------------
2 files changed, 96 insertions(+), 42 deletions(-)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index 006270f..7427d54 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -1,3 +1,4 @@
+#include <linux/slab.h> /* kzalloc */
#include <linux/suspend.h>
#include <linux/suspend_ioctls.h>
#include <linux/utsname.h>
@@ -115,6 +116,29 @@ struct snapshot_handle {
*/
#define data_of(handle) ((handle).buffer)

+/**
+ * struct hibernate_io_handle - handle for image I/O processing
+ *
+ * @priv: private data for each processor (swap_map_handle etc.)
+ */
+struct hibernate_io_handle {
+ void *priv;
+};
+
+/**
+ * hib_io_handle_alloc - allocate io handle with priv_size for private data
+ *
+ * @priv_size: the sie to allocate behind hibernate_io_handle for private use
+ */
+static inline struct hibernate_io_handle *hib_io_handle_alloc(size_t priv_size)
+{
+ struct hibernate_io_handle *ret;
+ ret = kzalloc(sizeof(*ret) + priv_size, GFP_KERNEL);
+ if (ret)
+ ret->priv = ret + 1;
+ return ret;
+}
+
extern unsigned int snapshot_additional_pages(struct zone *zone);
extern unsigned long snapshot_get_image_size(void);
extern int snapshot_read_next(struct snapshot_handle *handle);
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index b0bb217..7096d20 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -268,8 +268,10 @@ static void release_swap_writer(struct swap_map_handle *handle)
handle->cur = NULL;
}

-static int get_swap_writer(struct swap_map_handle *handle)
+static struct hibernate_io_handle *get_swap_writer(void)
{
+ struct hibernate_io_handle *io_handle;
+ struct swap_map_handle *handle;
int ret;

ret = swsusp_swap_check();
@@ -277,12 +279,18 @@ static int get_swap_writer(struct swap_map_handle *handle)
if (ret != -ENOSPC)
printk(KERN_ERR "PM: Cannot find swap device, try "
"swapon -a.\n");
- return ret;
+ return ERR_PTR(ret);
}
+ io_handle = hib_io_handle_alloc(sizeof(*handle));
+ if (!io_handle) {
+ ret = -ENOMEM;
+ goto err_close;
+ }
+ handle = io_handle->priv;
handle->cur = (struct swap_map_page *)get_zeroed_page(GFP_KERNEL);
if (!handle->cur) {
ret = -ENOMEM;
- goto err_close;
+ goto err_free;
}
handle->cur_swap = alloc_swapdev_block(root_swap);
if (!handle->cur_swap) {
@@ -291,17 +299,20 @@ static int get_swap_writer(struct swap_map_handle *handle)
}
handle->k = 0;
handle->first_sector = handle->cur_swap;
- return 0;
+ return io_handle;
err_rel:
release_swap_writer(handle);
err_close:
swsusp_close(FMODE_WRITE);
- return ret;
+err_free:
+ kfree(io_handle);
+ return ERR_PTR(ret);
}

-static int swap_write_page(struct swap_map_handle *handle, void *buf,
- struct bio **bio_chain)
+static int swap_write_page(struct hibernate_io_handle *io_handle, void *buf,
+ struct bio **bio_chain)
{
+ struct swap_map_handle *handle = io_handle->priv;
int error = 0;
sector_t offset;

@@ -339,9 +350,11 @@ static int flush_swap_writer(struct swap_map_handle *handle)
return -EINVAL;
}

-static int swap_writer_finish(struct swap_map_handle *handle,
+static int swap_writer_finish(struct hibernate_io_handle *io_handle,
unsigned int flags, int error)
{
+ struct swap_map_handle *handle = io_handle->priv;
+
if (!error) {
flush_swap_writer(handle);
printk(KERN_INFO "PM: S");
@@ -352,6 +365,7 @@ static int swap_writer_finish(struct swap_map_handle *handle,
if (error)
free_all_swap_pages(root_swap);
release_swap_writer(handle);
+ kfree(io_handle);
swsusp_close(FMODE_WRITE);

return error;
@@ -361,8 +375,8 @@ static int swap_writer_finish(struct swap_map_handle *handle,
* save_image - save the suspend image data
*/

-static int save_image(struct swap_map_handle *handle,
- struct snapshot_handle *snapshot,
+static int save_image(struct hibernate_io_handle *io_handle,
+ struct snapshot_handle *snapshot,
unsigned int nr_to_write)
{
unsigned int m;
@@ -385,7 +399,7 @@ static int save_image(struct swap_map_handle *handle,
ret = snapshot_read_next(snapshot);
if (ret <= 0)
break;
- ret = swap_write_page(handle, data_of(*snapshot), &bio);
+ ret = swap_write_page(io_handle, data_of(*snapshot), &bio);
if (ret)
break;
if (!(nr_pages % m))
@@ -431,17 +445,17 @@ static int enough_swap(unsigned int nr_pages)

int swsusp_write(unsigned int flags)
{
- struct swap_map_handle handle;
+ struct hibernate_io_handle *io_handle;
struct snapshot_handle snapshot;
struct swsusp_info *header;
unsigned long pages;
int error;

pages = snapshot_get_image_size();
- error = get_swap_writer(&handle);
- if (error) {
+ io_handle = get_swap_writer();
+ if (IS_ERR(io_handle)) {
printk(KERN_ERR "PM: Cannot get swap writer\n");
- return error;
+ return PTR_ERR(io_handle);
}
if (!enough_swap(pages)) {
printk(KERN_ERR "PM: Not enough free swap\n");
@@ -457,11 +471,11 @@ int swsusp_write(unsigned int flags)
goto out_finish;
}
header = (struct swsusp_info *)data_of(snapshot);
- error = swap_write_page(&handle, header, NULL);
+ error = swap_write_page(io_handle, header, NULL);
if (!error)
- error = save_image(&handle, &snapshot, pages - 1);
+ error = save_image(io_handle, &snapshot, pages - 1);
out_finish:
- error = swap_writer_finish(&handle, flags, error);
+ error = swap_writer_finish(io_handle, flags, error);
return error;
}

@@ -477,32 +491,43 @@ static void release_swap_reader(struct swap_map_handle *handle)
handle->cur = NULL;
}

-static int get_swap_reader(struct swap_map_handle *handle,
- unsigned int *flags_p)
+static struct hibernate_io_handle *get_swap_reader(unsigned int *flags_p)
{
+ struct hibernate_io_handle *io_handle;
+ struct swap_map_handle *handle;
int error;

*flags_p = swsusp_header->flags;

if (!swsusp_header->image) /* how can this happen? */
- return -EINVAL;
+ return ERR_PTR(-EINVAL);

+ io_handle = hib_io_handle_alloc(sizeof(*handle));
+ if (!io_handle)
+ return ERR_PTR(-ENOMEM);
+ handle = io_handle->priv;
handle->cur = (struct swap_map_page *)get_zeroed_page(__GFP_WAIT | __GFP_HIGH);
- if (!handle->cur)
- return -ENOMEM;
+ if (!handle->cur) {
+ error = -ENOMEM;
+ goto err_free;
+ }

error = hib_bio_read_page(swsusp_header->image, handle->cur, NULL);
- if (error) {
- release_swap_reader(handle);
- return error;
- }
+ if (error)
+ goto err_rel;
handle->k = 0;
- return 0;
+ return io_handle;
+err_rel:
+ release_swap_reader(handle);
+err_free:
+ kfree(io_handle);
+ return ERR_PTR(error);
}

-static int swap_read_page(struct swap_map_handle *handle, void *buf,
- struct bio **bio_chain)
+static int swap_read_page(struct hibernate_io_handle *io_handle, void *buf,
+ struct bio **bio_chain)
{
+ struct swap_map_handle *handle = io_handle->priv;
sector_t offset;
int error;

@@ -526,22 +551,25 @@ static int swap_read_page(struct swap_map_handle *handle, void *buf,
return error;
}

-static int swap_reader_finish(struct swap_map_handle *handle)
+static int swap_reader_finish(struct hibernate_io_handle *io_handle)
{
+ struct swap_map_handle *handle = io_handle->priv;
+
release_swap_reader(handle);
+ kfree(io_handle);

return 0;
}

/**
- * load_image - load the image using the swap map handle
+ * load_image - load the image
* @handle and the snapshot handle @snapshot
* (assume there are @nr_pages pages to load)
*/

-static int load_image(struct swap_map_handle *handle,
- struct snapshot_handle *snapshot,
- unsigned int nr_to_read)
+static int load_image(struct hibernate_io_handle *io_handle,
+ struct snapshot_handle *snapshot,
+ unsigned int nr_to_read)
{
unsigned int m;
int error = 0;
@@ -563,7 +591,7 @@ static int load_image(struct swap_map_handle *handle,
error = snapshot_write_next(snapshot);
if (error <= 0)
break;
- error = swap_read_page(handle, data_of(*snapshot), &bio);
+ error = swap_read_page(io_handle, data_of(*snapshot), &bio);
if (error)
break;
if (snapshot->sync_read)
@@ -598,7 +626,7 @@ static int load_image(struct swap_map_handle *handle,
int swsusp_read(unsigned int *flags_p)
{
int error;
- struct swap_map_handle handle;
+ struct hibernate_io_handle *io_handle;
struct snapshot_handle snapshot;
struct swsusp_info *header;

@@ -607,14 +635,16 @@ int swsusp_read(unsigned int *flags_p)
if (error < PAGE_SIZE)
return error < 0 ? error : -EFAULT;
header = (struct swsusp_info *)data_of(snapshot);
- error = get_swap_reader(&handle, flags_p);
- if (error)
+ io_handle = get_swap_reader(flags_p);
+ if (IS_ERR(io_handle)) {
+ error = PTR_ERR(io_handle);
goto end;
+ }
if (!error)
- error = swap_read_page(&handle, header, NULL);
+ error = swap_read_page(io_handle, header, NULL);
if (!error)
- error = load_image(&handle, &snapshot, header->pages - 1);
- swap_reader_finish(&handle);
+ error = load_image(io_handle, &snapshot, header->pages - 1);
+ swap_reader_finish(io_handle);
end:
if (!error)
pr_debug("PM: Image successfully loaded\n");
--
1.7.1

2010-06-02 08:53:23

by Jiri Slaby

Subject: [PATCH 4/9] PM / Hibernate: user, implement user_ops reader

Switch /dev/snapshot writer to hibernate_io_ops approach so that we
can do whatever we want with snapshot processing code. All the later
code changes will be transparent and needn't care about different
readers/writers.

In this patch only reader is implemented, writer was done previously.

It works similarly to writer, CONSUMER here is snapshot layer,
PRODUCER is fops->write.

Signed-off-by: Jiri Slaby <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
kernel/power/user.c | 76 +++++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/kernel/power/user.c b/kernel/power/user.c
index b4610c3..fb9e5c8 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -79,6 +79,7 @@ static struct {
#define TODO_FINISH 2
#define TODO_CLOSED 3
#define TODO_ERROR 4
+#define TODO_RD_RUNNING 5
DECLARE_BITMAP(flags, 10); /* TODO_* flags defined above */
} to_do;

@@ -131,12 +132,43 @@ static int user_writer_finish(struct hibernate_io_handle *io_handle,
return error;
}

+static struct hibernate_io_handle *user_reader_start(unsigned int *flags_p)
+{
+ return hib_io_handle_alloc(0) ? : ERR_PTR(-ENOMEM);
+}
+
+static int user_read_page(struct hibernate_io_handle *io_handle, void *addr,
+ struct bio **bio_chain)
+{
+ int err = 0;
+
+ mutex_lock(&to_do.lock);
+ to_do.buffer = addr;
+ mutex_unlock(&to_do.lock);
+ set_bit(TODO_WORK, to_do.flags);
+ wake_up_interruptible(&to_do.wait);
+
+ wait_event(to_do.done, !test_bit(TODO_WORK, to_do.flags) ||
+ (err = test_bit(TODO_CLOSED, to_do.flags)));
+
+ return err ? -EIO : 0;
+}
+
+static int user_reader_finish(struct hibernate_io_handle *io_handle)
+{
+ return 0;
+}
+
struct hibernate_io_ops user_ops = {
.free_space = user_free_space,

.writer_start = user_writer_start,
.writer_finish = user_writer_finish,
.write_page = user_write_page,
+
+ .reader_start = user_reader_start,
+ .reader_finish = user_reader_finish,
+ .read_page = user_read_page,
};

static void snapshot_writer(struct work_struct *work)
@@ -150,6 +182,22 @@ static void snapshot_writer(struct work_struct *work)

static DECLARE_WORK(snapshot_writer_w, snapshot_writer);

+static void snapshot_reader(struct work_struct *work)
+{
+ int ret;
+
+ set_bit(TODO_RD_RUNNING, to_do.flags);
+ ret = swsusp_read(NULL);
+ if (ret) {
+ printk(KERN_ERR "PM: read failed with %d\n", ret);
+ set_bit(TODO_ERROR, to_do.flags);
+ }
+ clear_bit(TODO_RD_RUNNING, to_do.flags);
+ wake_up_interruptible(&to_do.wait);
+}
+
+static DECLARE_WORK(snapshot_reader_w, snapshot_reader);
+
static int snapshot_open(struct inode *inode, struct file *filp)
{
struct snapshot_data *data;
@@ -300,20 +348,29 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf,

mutex_lock(&pm_mutex);

+ if (!test_bit(TODO_RD_RUNNING, to_do.flags))
+ queue_work(to_do.worker, &snapshot_reader_w);
+
data = filp->private_data;

if (!pg_offp) {
- res = snapshot_write_next(&data->handle);
- if (res <= 0)
+ res = wait_event_interruptible(to_do.wait,
+ test_bit(TODO_WORK, to_do.flags));
+ if (res)
goto unlock;
- } else {
- res = PAGE_SIZE - pg_offp;
}
+ res = PAGE_SIZE - pg_offp;

- res = simple_write_to_buffer(data_of(data->handle), res, &pg_offp,
- buf, count);
+ mutex_lock(&to_do.lock);
+ res = simple_write_to_buffer(to_do.buffer, res, &pg_offp, buf, count);
+ mutex_unlock(&to_do.lock);
if (res > 0)
*offp += res;
+
+ if (!(pg_offp & ~PAGE_MASK)) {
+ clear_bit(TODO_WORK, to_do.flags);
+ wake_up(&to_do.done);
+ }
unlock:
mutex_unlock(&pm_mutex);

@@ -398,9 +455,12 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
break;

case SNAPSHOT_ATOMIC_RESTORE:
- snapshot_write_finalize(&data->handle);
+ error = wait_event_interruptible(to_do.wait,
+ !test_bit(TODO_RD_RUNNING, to_do.flags));
+ if (error)
+ break;
if (data->mode != O_WRONLY || !data->frozen ||
- !snapshot_image_loaded(&data->handle)) {
+ test_bit(TODO_ERROR, to_do.flags)) {
error = -EPERM;
break;
}
--
1.7.1

2010-06-02 08:53:30

by Jiri Slaby

Subject: [PATCH 2/9] PM / Hibernate: add hibernate_io_ops

This is a centralized structure for actually used reader/writer. It
contains basic operations called from snapshot layer which doesn't
need to know who reads/writes the image right now -- this is
specified by current set of functions stored in hibernate_io_ops.

In other words there are more than one hibernate_io_ops which
contain different set of functions. E.g. one is for swap, one for
userspace (user.c) handler.

For now they will hold only swap operations. In next patches, user
support will be converted to ops as well to have a single layer which
allows easier transitions later.

Signed-off-by: Jiri Slaby <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
kernel/power/hibernate.c | 2 +
kernel/power/power.h | 27 +++++++++++++++++++++
kernel/power/swap.c | 57 ++++++++++++++++++++++++++++++---------------
3 files changed, 67 insertions(+), 19 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index aa9e916..7d38e6a 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -35,6 +35,8 @@ dev_t swsusp_resume_device;
sector_t swsusp_resume_block;
int in_suspend __nosavedata = 0;

+struct hibernate_io_ops *hibernate_io_ops;
+
enum {
HIBERNATION_INVALID,
HIBERNATION_PLATFORM,
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 7427d54..32a40f9 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -139,6 +139,31 @@ static inline struct hibernate_io_handle *hib_io_handle_alloc(size_t priv_size)
return ret;
}

+/**
+ * struct hibernate_io_ops - functions of a hibernation module
+ *
+ * @free_space: hom much space is available for the image
+ * @reader_start: initialize an image reader
+ * @reader_finish: deinitialize the reader
+ * @writer_start: initialize an image writer
+ * @writer_finish: deinitialize the writer
+ * @read_page: read an image page from a backing store
+ * @write_page: write an image page to a backing store
+ */
+struct hibernate_io_ops {
+ unsigned long (*free_space)(void);
+
+ struct hibernate_io_handle *(*reader_start)(unsigned int *flags_p);
+ int (*reader_finish)(struct hibernate_io_handle *io_handle);
+ struct hibernate_io_handle *(*writer_start)(void);
+ int (*writer_finish)(struct hibernate_io_handle *io_handle,
+ unsigned int flags, int error);
+ int (*read_page)(struct hibernate_io_handle *io_handle, void *addr,
+ struct bio **bio_chain);
+ int (*write_page)(struct hibernate_io_handle *io_handle, void *addr,
+ struct bio **bio_chain);
+};
+
extern unsigned int snapshot_additional_pages(struct zone *zone);
extern unsigned long snapshot_get_image_size(void);
extern int snapshot_read_next(struct snapshot_handle *handle);
@@ -237,6 +262,8 @@ enum {

extern int pm_test_level;

+extern struct hibernate_io_ops *hibernate_io_ops;
+
#ifdef CONFIG_SUSPEND_FREEZER
static inline int suspend_freeze_processes(void)
{
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 7096d20..f09494e 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -268,7 +268,7 @@ static void release_swap_writer(struct swap_map_handle *handle)
handle->cur = NULL;
}

-static struct hibernate_io_handle *get_swap_writer(void)
+static struct hibernate_io_handle *swap_writer_start(void)
{
struct hibernate_io_handle *io_handle;
struct swap_map_handle *handle;
@@ -399,7 +399,8 @@ static int save_image(struct hibernate_io_handle *io_handle,
ret = snapshot_read_next(snapshot);
if (ret <= 0)
break;
- ret = swap_write_page(io_handle, data_of(*snapshot), &bio);
+ ret = hibernate_io_ops->write_page(io_handle,
+ data_of(*snapshot), &bio);
if (ret)
break;
if (!(nr_pages % m))
@@ -419,18 +420,18 @@ static int save_image(struct hibernate_io_handle *io_handle,
}

/**
- * enough_swap - Make sure we have enough swap to save the image.
+ * enough_space - Make sure we have enough space to save the image.
*
- * Returns TRUE or FALSE after checking the total amount of swap
- * space avaiable from the resume partition.
+ * Returns TRUE or FALSE after checking the total amount of
+ * space avaiable from the resume block.
*/

-static int enough_swap(unsigned int nr_pages)
+static int enough_space(unsigned int nr_pages)
{
- unsigned int free_swap = count_swap_pages(root_swap, 1);
+ unsigned int free_pages = hibernate_io_ops->free_space();

- pr_debug("PM: Free swap pages: %u\n", free_swap);
- return free_swap > nr_pages + PAGES_FOR_IO;
+ pr_debug("PM: Free storage pages: %u\n", free_pages);
+ return free_pages > nr_pages + PAGES_FOR_IO;
}

/**
@@ -452,13 +453,13 @@ int swsusp_write(unsigned int flags)
int error;

pages = snapshot_get_image_size();
- io_handle = get_swap_writer();
+ io_handle = hibernate_io_ops->writer_start();
if (IS_ERR(io_handle)) {
printk(KERN_ERR "PM: Cannot get swap writer\n");
return PTR_ERR(io_handle);
}
- if (!enough_swap(pages)) {
- printk(KERN_ERR "PM: Not enough free swap\n");
+ if (!enough_space(pages)) {
+ printk(KERN_ERR "PM: Not enough free space for image\n");
error = -ENOSPC;
goto out_finish;
}
@@ -471,14 +472,19 @@ int swsusp_write(unsigned int flags)
goto out_finish;
}
header = (struct swsusp_info *)data_of(snapshot);
- error = swap_write_page(io_handle, header, NULL);
+ error = hibernate_io_ops->write_page(io_handle, header, NULL);
if (!error)
error = save_image(io_handle, &snapshot, pages - 1);
out_finish:
- error = swap_writer_finish(io_handle, flags, error);
+ error = hibernate_io_ops->writer_finish(io_handle, flags, error);
return error;
}

+static unsigned long swap_free_space(void)
+{
+ return count_swap_pages(root_swap, 1);
+}
+
/**
* The following functions allow us to read data using a swap map
* in a file-alike way
@@ -491,7 +497,7 @@ static void release_swap_reader(struct swap_map_handle *handle)
handle->cur = NULL;
}

-static struct hibernate_io_handle *get_swap_reader(unsigned int *flags_p)
+static struct hibernate_io_handle *swap_reader_start(unsigned int *flags_p)
{
struct hibernate_io_handle *io_handle;
struct swap_map_handle *handle;
@@ -561,6 +567,17 @@ static int swap_reader_finish(struct hibernate_io_handle *io_handle)
return 0;
}

+struct hibernate_io_ops swap_ops = {
+ .free_space = swap_free_space,
+
+ .reader_start = swap_reader_start,
+ .reader_finish = swap_reader_finish,
+ .writer_start = swap_writer_start,
+ .writer_finish = swap_writer_finish,
+ .read_page = swap_read_page,
+ .write_page = swap_write_page,
+};
+
/**
* load_image - load the image
* @handle and the snapshot handle @snapshot
@@ -591,7 +608,8 @@ static int load_image(struct hibernate_io_handle *io_handle,
error = snapshot_write_next(snapshot);
if (error <= 0)
break;
- error = swap_read_page(io_handle, data_of(*snapshot), &bio);
+ error = hibernate_io_ops->read_page(io_handle,
+ data_of(*snapshot), &bio);
if (error)
break;
if (snapshot->sync_read)
@@ -635,16 +653,16 @@ int swsusp_read(unsigned int *flags_p)
if (error < PAGE_SIZE)
return error < 0 ? error : -EFAULT;
header = (struct swsusp_info *)data_of(snapshot);
- io_handle = get_swap_reader(flags_p);
+ io_handle = hibernate_io_ops->reader_start(flags_p);
if (IS_ERR(io_handle)) {
error = PTR_ERR(io_handle);
goto end;
}
if (!error)
- error = swap_read_page(io_handle, header, NULL);
+ error = hibernate_io_ops->read_page(io_handle, header, NULL);
if (!error)
error = load_image(io_handle, &snapshot, header->pages - 1);
- swap_reader_finish(io_handle);
+ hibernate_io_ops->reader_finish(io_handle);
end:
if (!error)
pr_debug("PM: Image successfully loaded\n");
@@ -713,6 +731,7 @@ static int swsusp_header_init(void)
swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
if (!swsusp_header)
panic("Could not allocate memory for swsusp_header\n");
+ hibernate_io_ops = &swap_ops;
return 0;
}

--
1.7.1

2010-06-02 08:53:22

by Jiri Slaby

Subject: [PATCH 8/9] PM / Hibernate: dealign swsusp_info

From: Jiri Slaby <[email protected]>

Now there is no need to have swsusp_info page aligned thanks to chunk
i/o support. We may add more info after it on the very same page.
Later...

Signed-off-by: Jiri Slaby <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
kernel/power/power.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index 6e2e796..1fdbfe7 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -12,7 +12,7 @@ struct swsusp_info {
unsigned long image_pages;
unsigned long pages;
unsigned long size;
-} __attribute__((aligned(PAGE_SIZE)));
+};

#ifdef CONFIG_HIBERNATION
#ifdef CONFIG_ARCH_HIBERNATION_HEADER
--
1.7.1

2010-06-02 08:54:18

by Jiri Slaby

Subject: [PATCH 3/9] PM / Hibernate: user, implement user_ops writer

Switch /dev/snapshot writer to hibernate_io_ops approach so that we
can do whatever we want with snapshot processing code. All the later
code changes will be transparent and needn't care about different
readers/writers.

In this patch only writer is implemented -- for better bisectability
if something breaks. (The development was easier too, because one
could break only one part, not both.)

Also, when using this interface, switch to the ops on open/release,
so they are used.

There are explicit barriers protecting to_do_buffer, because it is
all done in a producer-consumer fashion:
PRODUCER (snapshot layer)
1) to_do_buffer is set
2) set TODO bit
3) wake CONSUMER
4) wait for TODO bit _clear_

CONSUMER (fops->read)
1) wait for TODO bit
2) pass to_do_buffer to userspace
3) clear TODO bit
4) wake PRODUCER

Signed-off-by: Jiri Slaby <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
kernel/power/user.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 132 insertions(+), 8 deletions(-)

diff --git a/kernel/power/user.c b/kernel/power/user.c
index e819e17..b4610c3 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -23,6 +23,8 @@
#include <linux/console.h>
#include <linux/cpu.h>
#include <linux/freezer.h>
+#include <linux/mutex.h>
+#include <linux/wait.h>
#include <scsi/scsi_scan.h>

#include <asm/uaccess.h>
@@ -61,10 +63,93 @@ static struct snapshot_data {
char frozen;
char ready;
char platform_support;
+ struct hibernate_io_ops *prev_ops;
} snapshot_state;

atomic_t snapshot_device_available = ATOMIC_INIT(1);

+static struct {
+ void *buffer; /* buffer to pass through */
+ struct workqueue_struct *worker; /* the thread initiating read/write */
+ wait_queue_head_t wait; /* wait for buffer */
+ wait_queue_head_t done; /* read/write done? */
+ struct mutex lock; /* protection for buffer */
+
+#define TODO_WORK 1
+#define TODO_FINISH 2
+#define TODO_CLOSED 3
+#define TODO_ERROR 4
+ DECLARE_BITMAP(flags, 10); /* TODO_* flags defined above */
+} to_do;
+
+static unsigned long user_free_space(void)
+{
+ return ~0UL; /* we have no idea, maybe we will fail later */
+}
+
+static struct hibernate_io_handle *user_writer_start(void)
+{
+ return hib_io_handle_alloc(0) ? : ERR_PTR(-ENOMEM);
+}
+
+static int user_write_page(struct hibernate_io_handle *io_handle, void *buf,
+ struct bio **bio_chain)
+{
+ int err = 0;
+
+ if (test_bit(TODO_CLOSED, to_do.flags))
+ return -EIO;
+
+ mutex_lock(&to_do.lock);
+ to_do.buffer = buf;
+ mutex_unlock(&to_do.lock);
+ set_bit(TODO_WORK, to_do.flags);
+ wake_up_interruptible(&to_do.wait);
+
+ wait_event(to_do.done, !test_bit(TODO_WORK, to_do.flags) ||
+ (err = test_bit(TODO_CLOSED, to_do.flags)));
+
+ return err ? -EIO : 0;
+}
+
+static int user_writer_finish(struct hibernate_io_handle *io_handle,
+ unsigned int flags, int error)
+{
+ int err = 0;
+
+ if (error)
+ set_bit(TODO_ERROR, to_do.flags);
+ set_bit(TODO_FINISH, to_do.flags);
+ wake_up_interruptible(&to_do.wait);
+
+ wait_event(to_do.done, !test_bit(TODO_FINISH, to_do.flags) ||
+ (err = test_bit(TODO_CLOSED, to_do.flags)));
+
+ if (!error && err)
+ error = -EIO;
+
+ return error;
+}
+
+struct hibernate_io_ops user_ops = {
+ .free_space = user_free_space,
+
+ .writer_start = user_writer_start,
+ .writer_finish = user_writer_finish,
+ .write_page = user_write_page,
+};
+
+static void snapshot_writer(struct work_struct *work)
+{
+ int ret;
+
+ ret = swsusp_write(0);
+ if (ret)
+ printk(KERN_ERR "PM: write failed with %d\n", ret);
+}
+
+static DECLARE_WORK(snapshot_writer_w, snapshot_writer);
+
static int snapshot_open(struct inode *inode, struct file *filp)
{
struct snapshot_data *data;
@@ -115,9 +200,17 @@ static int snapshot_open(struct inode *inode, struct file *filp)
}
if (error)
atomic_inc(&snapshot_device_available);
+ else {
+ data->prev_ops = hibernate_io_ops;
+ hibernate_io_ops = &user_ops;
+ }
data->frozen = 0;
data->ready = 0;
data->platform_support = 0;
+ memset(to_do.flags, 0, sizeof(*to_do.flags));
+ init_waitqueue_head(&to_do.wait);
+ init_waitqueue_head(&to_do.done);
+ mutex_init(&to_do.lock);

Unlock:
mutex_unlock(&pm_mutex);
@@ -131,6 +224,10 @@ static int snapshot_release(struct inode *inode, struct file *filp)

mutex_lock(&pm_mutex);

+ set_bit(TODO_CLOSED, to_do.flags);
+ wake_up(&to_do.done);
+ flush_workqueue(to_do.worker);
+
swsusp_free();
free_basic_memory_bitmaps();
data = filp->private_data;
@@ -139,6 +236,7 @@ static int snapshot_release(struct inode *inode, struct file *filp)
thaw_processes();
pm_notifier_call_chain(data->mode == O_WRONLY ?
PM_POST_HIBERNATION : PM_POST_RESTORE);
+ hibernate_io_ops = data->prev_ops;
atomic_inc(&snapshot_device_available);

mutex_unlock(&pm_mutex);
@@ -152,6 +250,7 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
struct snapshot_data *data;
ssize_t res;
loff_t pg_offp = *offp & ~PAGE_MASK;
+ int fin = 0;

mutex_lock(&pm_mutex);

@@ -161,18 +260,31 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
goto Unlock;
}
if (!pg_offp) { /* on page boundary? */
- res = snapshot_read_next(&data->handle);
- if (res <= 0)
+ res = wait_event_interruptible(to_do.wait,
+ test_bit(TODO_WORK, to_do.flags) ||
+ (fin = test_and_clear_bit(TODO_FINISH, to_do.flags)));
+ if (res)
goto Unlock;
- } else {
- res = PAGE_SIZE - pg_offp;
+ if (test_bit(TODO_ERROR, to_do.flags)) {
+ res = -EIO;
+ goto Unlock;
+ }
+ if (fin)
+ goto wake;
}
+ res = PAGE_SIZE - pg_offp;

- res = simple_read_from_buffer(buf, count, &pg_offp,
- data_of(data->handle), res);
+ mutex_lock(&to_do.lock);
+ res = simple_read_from_buffer(buf, count, &pg_offp, to_do.buffer, res);
+ mutex_unlock(&to_do.lock);
if (res > 0)
*offp += res;

+ if (!(pg_offp & ~PAGE_MASK)) {
+ clear_bit(TODO_WORK, to_do.flags);
+wake:
+ wake_up(&to_do.done);
+ }
Unlock:
mutex_unlock(&pm_mutex);

@@ -278,8 +390,11 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
error = hibernation_snapshot(data->platform_support);
if (!error)
error = put_user(in_suspend, (int __user *)arg);
- if (!error)
+ if (!error) {
+ if (in_suspend)
+ queue_work(to_do.worker, &snapshot_writer_w);
data->ready = 1;
+ }
break;

case SNAPSHOT_ATOMIC_RESTORE:
@@ -473,7 +588,16 @@ static struct miscdevice snapshot_device = {

static int __init snapshot_device_init(void)
{
- return misc_register(&snapshot_device);
+ int ret;
+
+ to_do.worker = create_singlethread_workqueue("suspend_worker");
+ if (!to_do.worker)
+ return -ENOMEM;
+
+ ret = misc_register(&snapshot_device);
+ if (ret)
+ destroy_workqueue(to_do.worker);
+ return ret;
};

device_initcall(snapshot_device_init);
--
1.7.1

2010-06-02 08:54:25

by Jiri Slaby

Subject: [PATCH 9/9] PM / Hibernate: move non-swap code to image.c

Now, when all the swap-independent code was separated, it's time to
move it into a separate file called image.c. Although snapshot.c is
image related too, we want to move the snapshot.c code to mm/,
because it is rather a memory management so that mm people won't
forget about hibernation. Hence we don't pollute snapshot.c with
image processing here.

Signed-off-by: Jiri Slaby <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
kernel/power/Makefile | 2 +-
kernel/power/image.c | 204 +++++++++++++++++++++++++++++++++++++++++++++++
kernel/power/power.h | 1 -
kernel/power/snapshot.c | 2 +-
kernel/power/swap.c | 192 --------------------------------------------
5 files changed, 206 insertions(+), 195 deletions(-)
create mode 100644 kernel/power/image.c

diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index 524e058..c8f26f0 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -9,7 +9,7 @@ obj-$(CONFIG_FREEZER) += process.o
obj-$(CONFIG_SUSPEND) += suspend.o
obj-$(CONFIG_PM_TEST_SUSPEND) += suspend_test.o
obj-$(CONFIG_HIBERNATION) += hibernate.o snapshot.o swap.o user.o \
- block_io.o
+ block_io.o image.o
obj-$(CONFIG_HIBERNATION_NVS) += hibernate_nvs.o

obj-$(CONFIG_MAGIC_SYSRQ) += poweroff.o
diff --git a/kernel/power/image.c b/kernel/power/image.c
new file mode 100644
index 0000000..7ebed6b
--- /dev/null
+++ b/kernel/power/image.c
@@ -0,0 +1,204 @@
+/*
+ * kernel/power/image.c
+ *
+ * This file provides functions for reading and writing the suspend image.
+ * It is independent of readers/writers (e.g. swap).
+ *
+ * Copyright (C) 1998,2001-2005 Pavel Machek <[email protected]>
+ * Copyright (C) 2006 Rafael J. Wysocki <[email protected]>
+ *
+ * This file is released under the GPLv2.
+ */
+
+#include <linux/kernel.h>
+#include <linux/time.h>
+
+#include "power.h"
+
+/**
+ * save_image - save the suspend image data
+ */
+static int save_image(struct hibernate_io_handle *io_handle,
+ struct snapshot_handle *snapshot,
+ unsigned int nr_to_write)
+{
+ unsigned int m;
+ int ret;
+ int nr_pages;
+ int err2;
+ struct bio *bio;
+ struct timeval start;
+ struct timeval stop;
+
+ printk(KERN_INFO "PM: Saving image data pages (%u pages) ... ",
+ nr_to_write);
+ m = nr_to_write / 100;
+ if (!m)
+ m = 1;
+ nr_pages = 0;
+ bio = NULL;
+ do_gettimeofday(&start);
+ while (1) {
+ ret = snapshot_read_next(snapshot);
+ if (ret <= 0)
+ break;
+ ret = hibernate_io_ops->write_page(io_handle,
+ data_of(*snapshot), &bio);
+ if (ret)
+ break;
+ if (!(nr_pages % m))
+ printk(KERN_CONT "\b\b\b\b%3d%%", nr_pages / m);
+ nr_pages++;
+ }
+ err2 = hib_wait_on_bio_chain(&bio);
+ do_gettimeofday(&stop);
+ if (!ret)
+ ret = err2;
+ if (!ret)
+ printk(KERN_CONT "\b\b\b\bdone\n");
+ else
+ printk(KERN_CONT "\n");
+ swsusp_show_speed(&start, &stop, nr_to_write, "Wrote");
+ return ret;
+}
+
+/**
+ * enough_space - Make sure we have enough space to save the image.
+ *
+ * Returns TRUE or FALSE after checking the total amount of
+ * space avaiable from the resume block.
+ */
+static int enough_space(unsigned int nr_pages)
+{
+ unsigned int free_pages = hibernate_io_ops->free_space();
+
+ pr_debug("PM: Free storage pages: %u\n", free_pages);
+ return free_pages > nr_pages + PAGES_FOR_IO;
+}
+
+/**
+ * swsusp_write - Write entire image and metadata.
+ * @flags: flags to pass to the "boot" kernel in the image header
+ *
+ * It is important _NOT_ to umount filesystems at this point. We want
+ * them synced (in case something goes wrong) but we DO not want to mark
+ * filesystem clean: it is not. (And it does not matter, if we resume
+ * correctly, we'll mark system clean, anyway.)
+ */
+int swsusp_write(unsigned int flags)
+{
+ struct hibernate_io_handle *io_handle;
+ struct snapshot_handle snapshot;
+ unsigned long pages;
+ int error;
+
+ pages = snapshot_get_image_size();
+ io_handle = hibernate_io_ops->writer_start();
+ if (IS_ERR(io_handle)) {
+ printk(KERN_ERR "PM: Cannot get swap writer\n");
+ return PTR_ERR(io_handle);
+ }
+ if (!enough_space(pages)) {
+ printk(KERN_ERR "PM: Not enough free space for image\n");
+ error = -ENOSPC;
+ goto out_finish;
+ }
+ memset(&snapshot, 0, sizeof(struct snapshot_handle));
+ error = snapshot_write_init(io_handle, &snapshot);
+ if (error) {
+ printk(KERN_ERR "PM: Cannot init writer\n");
+ goto out_finish;
+ }
+ error = save_image(io_handle, &snapshot, pages - 1);
+out_finish:
+ error = hibernate_io_ops->writer_finish(io_handle, flags, error);
+ return error;
+}
+
+/**
+ * load_image - load the image
+ * @handle and the snapshot handle @snapshot
+ * (assume there are @nr_pages pages to load)
+ */
+static int load_image(struct hibernate_io_handle *io_handle,
+ struct snapshot_handle *snapshot,
+ unsigned int nr_to_read)
+{
+ unsigned int m;
+ int error = 0;
+ struct timeval start;
+ struct timeval stop;
+ struct bio *bio;
+ int err2;
+ unsigned nr_pages;
+
+ printk(KERN_INFO "PM: Loading image data pages (%u pages) ... ",
+ nr_to_read);
+ m = nr_to_read / 100;
+ if (!m)
+ m = 1;
+ nr_pages = 0;
+ bio = NULL;
+ do_gettimeofday(&start);
+ for ( ; ; ) {
+ error = hibernate_io_ops->read_page(io_handle,
+ data_of(*snapshot), &bio);
+ if (error)
+ break;
+ if (snapshot->sync_read)
+ error = hib_wait_on_bio_chain(&bio);
+ if (error)
+ break;
+ error = snapshot_write_next(snapshot);
+ if (error >= 0)
+ nr_pages++;
+ if (error <= 0)
+ break;
+ if (!(nr_pages % m))
+ printk("\b\b\b\b%3d%%", nr_pages / m);
+ }
+ err2 = hib_wait_on_bio_chain(&bio);
+ do_gettimeofday(&stop);
+ if (!error)
+ error = err2;
+ if (!error) {
+ printk("\b\b\b\bdone\n");
+ snapshot_write_finalize(snapshot);
+ if (!snapshot_image_loaded(snapshot))
+ error = -ENODATA;
+ } else
+ printk("\n");
+ swsusp_show_speed(&start, &stop, nr_to_read, "Read");
+ return error;
+}
+
+/**
+ * swsusp_read - read the hibernation image.
+ * @flags_p: flags passed by the "frozen" kernel in the image header should
+ * be written into this memeory location
+ */
+int swsusp_read(unsigned int *flags_p)
+{
+ int error;
+ struct hibernate_io_handle *io_handle;
+ struct snapshot_handle snapshot;
+
+ memset(&snapshot, 0, sizeof(struct snapshot_handle));
+ io_handle = hibernate_io_ops->reader_start(flags_p);
+ if (IS_ERR(io_handle)) {
+ error = PTR_ERR(io_handle);
+ goto end;
+ }
+ error = snapshot_read_init(io_handle, &snapshot);
+ if (!error)
+ error = load_image(io_handle, &snapshot,
+ snapshot_get_image_size() - 1);
+ hibernate_io_ops->reader_finish(io_handle);
+end:
+ if (!error)
+ pr_debug("PM: Image successfully loaded\n");
+ else
+ pr_debug("PM: Error %d resuming\n", error);
+ return error;
+}
+
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 1fdbfe7..ad42a0f 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -169,7 +169,6 @@ struct hibernate_io_ops {
struct bio **bio_chain);
};

-extern unsigned int snapshot_additional_pages(struct zone *zone);
extern unsigned long snapshot_get_image_size(void);
extern int snapshot_read_init(struct hibernate_io_handle *io_handle,
struct snapshot_handle *handle);
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 9cd6931..f898820 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -790,7 +790,7 @@ void free_basic_memory_bitmaps(void)
* zone (usually the returned value is greater than the exact number)
*/

-unsigned int snapshot_additional_pages(struct zone *zone)
+static unsigned int snapshot_additional_pages(struct zone *zone)
{
unsigned int res;

diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index f7864bc..585d460 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -371,109 +371,6 @@ static int swap_writer_finish(struct hibernate_io_handle *io_handle,
return error;
}

-/**
- * save_image - save the suspend image data
- */
-
-static int save_image(struct hibernate_io_handle *io_handle,
- struct snapshot_handle *snapshot,
- unsigned int nr_to_write)
-{
- unsigned int m;
- int ret;
- int nr_pages;
- int err2;
- struct bio *bio;
- struct timeval start;
- struct timeval stop;
-
- printk(KERN_INFO "PM: Saving image data pages (%u pages) ... ",
- nr_to_write);
- m = nr_to_write / 100;
- if (!m)
- m = 1;
- nr_pages = 0;
- bio = NULL;
- do_gettimeofday(&start);
- while (1) {
- ret = snapshot_read_next(snapshot);
- if (ret <= 0)
- break;
- ret = hibernate_io_ops->write_page(io_handle,
- data_of(*snapshot), &bio);
- if (ret)
- break;
- if (!(nr_pages % m))
- printk(KERN_CONT "\b\b\b\b%3d%%", nr_pages / m);
- nr_pages++;
- }
- err2 = hib_wait_on_bio_chain(&bio);
- do_gettimeofday(&stop);
- if (!ret)
- ret = err2;
- if (!ret)
- printk(KERN_CONT "\b\b\b\bdone\n");
- else
- printk(KERN_CONT "\n");
- swsusp_show_speed(&start, &stop, nr_to_write, "Wrote");
- return ret;
-}
-
-/**
- * enough_space - Make sure we have enough space to save the image.
- *
- * Returns TRUE or FALSE after checking the total amount of
- * space avaiable from the resume block.
- */
-
-static int enough_space(unsigned int nr_pages)
-{
- unsigned int free_pages = hibernate_io_ops->free_space();
-
- pr_debug("PM: Free storage pages: %u\n", free_pages);
- return free_pages > nr_pages + PAGES_FOR_IO;
-}
-
-/**
- * swsusp_write - Write entire image and metadata.
- * @flags: flags to pass to the "boot" kernel in the image header
- *
- * It is important _NOT_ to umount filesystems at this point. We want
- * them synced (in case something goes wrong) but we DO not want to mark
- * filesystem clean: it is not. (And it does not matter, if we resume
- * correctly, we'll mark system clean, anyway.)
- */
-
-int swsusp_write(unsigned int flags)
-{
- struct hibernate_io_handle *io_handle;
- struct snapshot_handle snapshot;
- unsigned long pages;
- int error;
-
- pages = snapshot_get_image_size();
- io_handle = hibernate_io_ops->writer_start();
- if (IS_ERR(io_handle)) {
- printk(KERN_ERR "PM: Cannot get swap writer\n");
- return PTR_ERR(io_handle);
- }
- if (!enough_space(pages)) {
- printk(KERN_ERR "PM: Not enough free space for image\n");
- error = -ENOSPC;
- goto out_finish;
- }
- memset(&snapshot, 0, sizeof(struct snapshot_handle));
- error = snapshot_write_init(io_handle, &snapshot);
- if (error) {
- printk(KERN_ERR "PM: Cannot init writer\n");
- goto out_finish;
- }
- error = save_image(io_handle, &snapshot, pages - 1);
-out_finish:
- error = hibernate_io_ops->writer_finish(io_handle, flags, error);
- return error;
-}
-
static unsigned long swap_free_space(void)
{
return count_swap_pages(root_swap, 1);
@@ -573,95 +470,6 @@ struct hibernate_io_ops swap_ops = {
};

/**
- * load_image - load the image
- * @handle and the snapshot handle @snapshot
- * (assume there are @nr_pages pages to load)
- */
-
-static int load_image(struct hibernate_io_handle *io_handle,
- struct snapshot_handle *snapshot,
- unsigned int nr_to_read)
-{
- unsigned int m;
- int error = 0;
- struct timeval start;
- struct timeval stop;
- struct bio *bio;
- int err2;
- unsigned nr_pages;
-
- printk(KERN_INFO "PM: Loading image data pages (%u pages) ... ",
- nr_to_read);
- m = nr_to_read / 100;
- if (!m)
- m = 1;
- nr_pages = 0;
- bio = NULL;
- do_gettimeofday(&start);
- for ( ; ; ) {
- error = hibernate_io_ops->read_page(io_handle,
- data_of(*snapshot), &bio);
- if (error)
- break;
- if (snapshot->sync_read)
- error = hib_wait_on_bio_chain(&bio);
- if (error)
- break;
- error = snapshot_write_next(snapshot);
- if (error >= 0)
- nr_pages++;
- if (error <= 0)
- break;
- if (!(nr_pages % m))
- printk("\b\b\b\b%3d%%", nr_pages / m);
- }
- err2 = hib_wait_on_bio_chain(&bio);
- do_gettimeofday(&stop);
- if (!error)
- error = err2;
- if (!error) {
- printk("\b\b\b\bdone\n");
- snapshot_write_finalize(snapshot);
- if (!snapshot_image_loaded(snapshot))
- error = -ENODATA;
- } else
- printk("\n");
- swsusp_show_speed(&start, &stop, nr_to_read, "Read");
- return error;
-}
-
-/**
- * swsusp_read - read the hibernation image.
- * @flags_p: flags passed by the "frozen" kernel in the image header should
- * be written into this memeory location
- */
-
-int swsusp_read(unsigned int *flags_p)
-{
- int error;
- struct hibernate_io_handle *io_handle;
- struct snapshot_handle snapshot;
-
- memset(&snapshot, 0, sizeof(struct snapshot_handle));
- io_handle = hibernate_io_ops->reader_start(flags_p);
- if (IS_ERR(io_handle)) {
- error = PTR_ERR(io_handle);
- goto end;
- }
- error = snapshot_read_init(io_handle, &snapshot);
- if (!error)
- error = load_image(io_handle, &snapshot,
- snapshot_get_image_size() - 1);
- hibernate_io_ops->reader_finish(io_handle);
-end:
- if (!error)
- pr_debug("PM: Image successfully loaded\n");
- else
- pr_debug("PM: Error %d resuming\n", error);
- return error;
-}
-
-/**
* swsusp_check - Check for swsusp signature in the resume device
*/

--
1.7.1

2010-06-02 08:54:23

by Jiri Slaby

Subject: [PATCH 7/9] PM / Hibernate: split snapshot_write_next

When reading the snapshot, do the initialization and header read in
a separate function. This makes the code more readable and lowers
complexity of snapshot_write_next.

Signed-off-by: Jiri Slaby <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
kernel/power/power.h | 2 +
kernel/power/snapshot.c | 100 ++++++++++++++++++++++++++++++-----------------
kernel/power/swap.c | 20 ++++------
3 files changed, 74 insertions(+), 48 deletions(-)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index ff3f63f..6e2e796 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -171,6 +171,8 @@ struct hibernate_io_ops {

extern unsigned int snapshot_additional_pages(struct zone *zone);
extern unsigned long snapshot_get_image_size(void);
+extern int snapshot_read_init(struct hibernate_io_handle *io_handle,
+ struct snapshot_handle *handle);
extern int snapshot_write_init(struct hibernate_io_handle *io_handle,
struct snapshot_handle *handle);
extern int snapshot_read_next(struct snapshot_handle *handle);
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 4600d15..9cd6931 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -2129,10 +2129,54 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
}

/**
+ * snapshot_read_init - initialization before reading the snapshot from
+ * a backing storage
+ *
+ * This function *must* be called before snapshot_write_next to initialize
+ * @handle and read header.
+ *
+ * @io_handle: io handle used for reading
+ * @handle: snapshot handle to init
+ */
+int snapshot_read_init(struct hibernate_io_handle *io_handle,
+ struct snapshot_handle *handle)
+{
+ int ret;
+
+ /* This makes the buffer be freed by swsusp_free() */
+ buffer = get_image_page(GFP_ATOMIC, PG_ANY);
+ if (!buffer)
+ return -ENOMEM;
+
+ ret = hib_buffer_init_read(io_handle);
+ if (ret)
+ return ret;
+ ret = hib_buffer_read(io_handle, buffer, sizeof(struct swsusp_info));
+ if (ret)
+ goto finish;
+ hib_buffer_finish_read(io_handle);
+
+ ret = load_header(buffer);
+ if (ret)
+ return ret;
+
+ ret = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
+ if (ret)
+ return ret;
+
+ handle->buffer = buffer;
+ handle->sync_read = 1;
+ return 0;
+finish:
+ hib_buffer_finish_read(io_handle);
+ return ret;
+}
+
+/**
* snapshot_write_next - used for writing the system memory snapshot.
*
- * On the first call to it @handle should point to a zeroed
- * snapshot_handle structure. The structure gets updated and a pointer
+ * Before calling this function, snapshot_read_init has to be called with
+ * handle passed as @handle here. The structure gets updated and a pointer
* to it should be passed to this function every next time.
*
* On success the function returns a positive number. Then, the caller
@@ -2144,42 +2188,20 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
* structure pointed to by @handle is not updated and should not be used
* any more.
*/
-
int snapshot_write_next(struct snapshot_handle *handle)
{
static struct chain_allocator ca;
int error = 0;

- /* Check if we have already loaded the entire image */
- if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages)
- return 0;
-
handle->sync_read = 1;
-
- if (!handle->cur) {
- if (!buffer)
- /* This makes the buffer be freed by swsusp_free() */
- buffer = get_image_page(GFP_ATOMIC, PG_ANY);
-
- if (!buffer)
- return -ENOMEM;
-
- handle->buffer = buffer;
- } else if (handle->cur == 1) {
- error = load_header(buffer);
- if (error)
- return error;
-
- error = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
- if (error)
- return error;
-
- } else if (handle->cur <= nr_meta_pages + 1) {
+ if (handle->cur < nr_meta_pages) {
error = unpack_orig_pfns(buffer, &copy_bm);
if (error)
return error;

- if (handle->cur == nr_meta_pages + 1) {
+ /* well, this was the last meta page
+ prepare for ordinary pages */
+ if (handle->cur + 1 == nr_meta_pages) {
error = prepare_image(&orig_bm, &copy_bm);
if (error)
return error;
@@ -2192,16 +2214,22 @@ int snapshot_write_next(struct snapshot_handle *handle)
if (IS_ERR(handle->buffer))
return PTR_ERR(handle->buffer);
}
+ error = PAGE_SIZE;
} else {
copy_last_highmem_page();
- handle->buffer = get_buffer(&orig_bm, &ca);
- if (IS_ERR(handle->buffer))
- return PTR_ERR(handle->buffer);
- if (handle->buffer != buffer)
- handle->sync_read = 0;
+ /* prepare next unless this was the last one */
+ if (handle->cur + 1 < nr_meta_pages + nr_copy_pages) {
+ handle->buffer = get_buffer(&orig_bm, &ca);
+ if (IS_ERR(handle->buffer))
+ return PTR_ERR(handle->buffer);
+ if (handle->buffer != buffer)
+ handle->sync_read = 0;
+ error = PAGE_SIZE;
+ }
}
+
handle->cur++;
- return PAGE_SIZE;
+ return error;
}

/**
@@ -2216,7 +2244,7 @@ void snapshot_write_finalize(struct snapshot_handle *handle)
{
copy_last_highmem_page();
/* Free only if we have loaded the image entirely */
- if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages) {
+ if (handle->cur >= nr_meta_pages + nr_copy_pages) {
memory_bm_free(&orig_bm, PG_UNSAFE_CLEAR);
free_highmem_data();
}
@@ -2225,7 +2253,7 @@ void snapshot_write_finalize(struct snapshot_handle *handle)
int snapshot_image_loaded(struct snapshot_handle *handle)
{
return !(!nr_copy_pages || !last_highmem_page_copied() ||
- handle->cur <= nr_meta_pages + nr_copy_pages);
+ handle->cur < nr_meta_pages + nr_copy_pages);
}

#ifdef CONFIG_HIGHMEM
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 9b319f1..f7864bc 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -599,9 +599,6 @@ static int load_image(struct hibernate_io_handle *io_handle,
bio = NULL;
do_gettimeofday(&start);
for ( ; ; ) {
- error = snapshot_write_next(snapshot);
- if (error <= 0)
- break;
error = hibernate_io_ops->read_page(io_handle,
data_of(*snapshot), &bio);
if (error)
@@ -610,9 +607,13 @@ static int load_image(struct hibernate_io_handle *io_handle,
error = hib_wait_on_bio_chain(&bio);
if (error)
break;
+ error = snapshot_write_next(snapshot);
+ if (error >= 0)
+ nr_pages++;
+ if (error <= 0)
+ break;
if (!(nr_pages % m))
printk("\b\b\b\b%3d%%", nr_pages / m);
- nr_pages++;
}
err2 = hib_wait_on_bio_chain(&bio);
do_gettimeofday(&stop);
@@ -640,22 +641,17 @@ int swsusp_read(unsigned int *flags_p)
int error;
struct hibernate_io_handle *io_handle;
struct snapshot_handle snapshot;
- struct swsusp_info *header;

memset(&snapshot, 0, sizeof(struct snapshot_handle));
- error = snapshot_write_next(&snapshot);
- if (error < PAGE_SIZE)
- return error < 0 ? error : -EFAULT;
- header = (struct swsusp_info *)data_of(snapshot);
io_handle = hibernate_io_ops->reader_start(flags_p);
if (IS_ERR(io_handle)) {
error = PTR_ERR(io_handle);
goto end;
}
+ error = snapshot_read_init(io_handle, &snapshot);
if (!error)
- error = hibernate_io_ops->read_page(io_handle, header, NULL);
- if (!error)
- error = load_image(io_handle, &snapshot, header->pages - 1);
+ error = load_image(io_handle, &snapshot,
+ snapshot_get_image_size() - 1);
hibernate_io_ops->reader_finish(io_handle);
end:
if (!error)
--
1.7.1

2010-06-02 08:54:21

by Jiri Slaby

Subject: [PATCH 5/9] PM / Hibernate: add chunk i/o support

Chunk support is useful when not writing whole pages at once. It takes
care of joining the buffers into the pages and writing at once when
needed.

This adds functions for both reads and writes.

In the end when pages are compressed they use this interface as well
(because they are indeed shorter than PAGE_SIZE).

Signed-off-by: Jiri Slaby <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
kernel/power/block_io.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++
kernel/power/power.h | 16 ++++++
2 files changed, 142 insertions(+), 0 deletions(-)

diff --git a/kernel/power/block_io.c b/kernel/power/block_io.c
index 97024fd..5b6413d 100644
--- a/kernel/power/block_io.c
+++ b/kernel/power/block_io.c
@@ -3,6 +3,7 @@
*
* Copyright (C) 1998,2001-2005 Pavel Machek <[email protected]>
* Copyright (C) 2006 Rafael J. Wysocki <[email protected]>
+ * Copyright (C) 2004-2008 Nigel Cunningham (nigel at tuxonice net)
*
* This file is released under the GPLv2.
*/
@@ -74,6 +75,131 @@ int hib_bio_write_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
virt_to_page(addr), bio_chain);
}

+static int hib_buffer_init_rw(struct hibernate_io_handle *io_handle,
+ int writing)
+{
+ /* should never happen - it means we didn't finish properly last time */
+ BUG_ON(io_handle->chunk_buffer || io_handle->chunk_buffer_pos);
+
+ io_handle->chunk_buffer =
+ (void *)get_zeroed_page(__GFP_WAIT | __GFP_HIGH);
+ if (io_handle->chunk_buffer && !writing)
+ io_handle->chunk_buffer_pos = PAGE_SIZE;
+
+ return io_handle->chunk_buffer ? 0 : -ENOMEM;
+}
+
+int hib_buffer_init_read(struct hibernate_io_handle *io_handle)
+{
+ return hib_buffer_init_rw(io_handle, 0);
+}
+
+int hib_buffer_init_write(struct hibernate_io_handle *io_handle)
+{
+ return hib_buffer_init_rw(io_handle, 1);
+}
+
+/**
+ * hib_buffer_rw - combine smaller buffers into PAGE_SIZE I/O
+ * @io_handle: handle from reader/writer
+ * @writing: whether writing (or reading).
+ * @buffer: the start of the buffer to write or fill.
+ * @buffer_size: the size of the buffer to write or fill.
+ **/
+static int hib_buffer_rw(struct hibernate_io_handle *io_handle, int writing,
+ char *buffer, int buffer_size)
+{
+ int bytes_left = buffer_size, ret;
+
+ while (bytes_left) {
+ char *source_start = buffer + buffer_size - bytes_left;
+ char *dest_start = io_handle->chunk_buffer +
+ io_handle->chunk_buffer_pos;
+ int capacity = PAGE_SIZE - io_handle->chunk_buffer_pos;
+ char *to = writing ? dest_start : source_start;
+ char *from = writing ? source_start : dest_start;
+
+ if (bytes_left <= capacity) {
+ memcpy(to, from, bytes_left);
+ io_handle->chunk_buffer_pos += bytes_left;
+ return 0;
+ }
+
+ /* Complete this page and start a new one */
+ memcpy(to, from, capacity);
+ bytes_left -= capacity;
+
+ if (writing)
+ ret = hibernate_io_ops->write_page(io_handle,
+ io_handle->chunk_buffer, NULL);
+ else
+ ret = hibernate_io_ops->read_page(io_handle,
+ io_handle->chunk_buffer, NULL);
+
+ if (ret)
+ return ret;
+
+ io_handle->chunk_buffer_pos = 0;
+ }
+
+ return 0;
+}
+
+int hib_buffer_read(struct hibernate_io_handle *io_handle, char *buffer,
+ int buffer_size)
+{
+ return hib_buffer_rw(io_handle, 0, buffer, buffer_size);
+}
+
+int hib_buffer_write(struct hibernate_io_handle *io_handle, char *buffer,
+ int buffer_size)
+{
+ return hib_buffer_rw(io_handle, 1, buffer, buffer_size);
+}
+
+int hib_buffer_flush_page_read(struct hibernate_io_handle *io_handle)
+{
+ io_handle->chunk_buffer_pos = PAGE_SIZE;
+
+ return 0;
+}
+
+int hib_buffer_finish_read(struct hibernate_io_handle *io_handle)
+{
+ int ret;
+
+ ret = hib_buffer_flush_page_read(io_handle);
+
+ free_page((unsigned long)io_handle->chunk_buffer);
+ io_handle->chunk_buffer = NULL;
+ io_handle->chunk_buffer_pos = 0;
+
+ return ret;
+}
+
+int hib_buffer_flush_page_write(struct hibernate_io_handle *io_handle)
+{
+ int ret = 0;
+ if (io_handle->chunk_buffer_pos)
+ ret = hibernate_io_ops->write_page(io_handle,
+ io_handle->chunk_buffer, NULL);
+ io_handle->chunk_buffer_pos = 0;
+ return ret;
+}
+
+int hib_buffer_finish_write(struct hibernate_io_handle *io_handle)
+{
+ int ret = 0;
+
+ ret = hib_buffer_flush_page_write(io_handle);
+
+ free_page((unsigned long)io_handle->chunk_buffer);
+ io_handle->chunk_buffer = NULL;
+ io_handle->chunk_buffer_pos = 0;
+
+ return ret;
+}
+
int hib_wait_on_bio_chain(struct bio **bio_chain)
{
struct bio *bio;
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 32a40f9..812b66c 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -119,9 +119,14 @@ struct snapshot_handle {
/**
* struct hibernate_io_handle - handle for image I/O processing
*
+ * @chunk_buffer: temporary buffer used by chunk I/O to fill whole page
+ * @chunk_buffer_pos: position in chio_buffer
* @priv: private data for each processor (swap_map_handle etc.)
*/
struct hibernate_io_handle {
+ char *chunk_buffer;
+ unsigned long chunk_buffer_pos;
+
void *priv;
};

@@ -200,6 +205,17 @@ extern int hib_bio_write_page(pgoff_t page_off, void *addr,
struct bio **bio_chain);
extern int hib_wait_on_bio_chain(struct bio **bio_chain);

+extern int hib_buffer_init_read(struct hibernate_io_handle *io_handle);
+extern int hib_buffer_init_write(struct hibernate_io_handle *io_handle);
+extern int hib_buffer_finish_read(struct hibernate_io_handle *io_handle);
+extern int hib_buffer_finish_write(struct hibernate_io_handle *io_handle);
+extern int hib_buffer_flush_page_read(struct hibernate_io_handle *io_handle);
+extern int hib_buffer_flush_page_write(struct hibernate_io_handle *io_handle);
+extern int hib_buffer_read(struct hibernate_io_handle *io_handle, char *buffer,
+ int buffer_size);
+extern int hib_buffer_write(struct hibernate_io_handle *io_handle, char *buffer,
+ int buffer_size);
+
struct timeval;
/* kernel/power/swsusp.c */
extern void swsusp_show_speed(struct timeval *, struct timeval *,
--
1.7.1

2010-06-02 08:54:27

by Jiri Slaby

Subject: [PATCH 6/9] PM / Hibernate: split snapshot_read_next

When writing the snapshot, do the initialization and header write in
a separate function. This makes the code more readable and lowers
complexity of snapshot_read_next.

Signed-off-by: Jiri Slaby <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
kernel/power/power.h | 2 +
kernel/power/snapshot.c | 65 ++++++++++++++++++++++++++++++----------------
kernel/power/swap.c | 14 +++-------
3 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index 812b66c..ff3f63f 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -171,6 +171,8 @@ struct hibernate_io_ops {

extern unsigned int snapshot_additional_pages(struct zone *zone);
extern unsigned long snapshot_get_image_size(void);
+extern int snapshot_write_init(struct hibernate_io_handle *io_handle,
+ struct snapshot_handle *handle);
extern int snapshot_read_next(struct snapshot_handle *handle);
extern int snapshot_write_next(struct snapshot_handle *handle);
extern void snapshot_write_finalize(struct snapshot_handle *handle);
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 25ce010..4600d15 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1598,10 +1598,46 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
}

/**
+ * snapshot_write_init - initialization before writing the snapshot to
+ * a backing storage
+ *
+ * This function *must* be called before snapshot_read_next to initialize
+ * @handle and write a header.
+ *
+ * @io_handle: handle used when writing the initial info onto storage
+ * @handle: snapshot handle to init
+ */
+int snapshot_write_init(struct hibernate_io_handle *io_handle,
+ struct snapshot_handle *handle)
+{
+ int ret;
+
+ /* This makes the buffer be freed by swsusp_free() */
+ buffer = get_image_page(GFP_ATOMIC, PG_ANY);
+ if (!buffer)
+ return -ENOMEM;
+ init_header(buffer);
+ ret = hib_buffer_init_write(io_handle);
+ if (ret)
+ return ret;
+ ret = hib_buffer_write(io_handle, buffer, sizeof(struct swsusp_info));
+ if (ret)
+ goto finish;
+ hib_buffer_finish_write(io_handle);
+ memory_bm_position_reset(&orig_bm);
+ memory_bm_position_reset(&copy_bm);
+ handle->buffer = buffer;
+ return 0;
+finish:
+ hib_buffer_finish_write(io_handle);
+ return ret;
+}
+
+/**
* snapshot_read_next - used for reading the system memory snapshot.
*
- * On the first call to it @handle should point to a zeroed
- * snapshot_handle structure. The structure gets updated and a pointer
+ * Before calling this function, snapshot_write_init has to be called with
+ * handle passed as @handle here. The structure gets updated and a pointer
* to it should be passed to this function every next time.
*
* On success the function returns a positive number. Then, the caller
@@ -1613,31 +1649,12 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
* structure pointed to by @handle is not updated and should not be used
* any more.
*/
-
int snapshot_read_next(struct snapshot_handle *handle)
{
- if (handle->cur > nr_meta_pages + nr_copy_pages)
- return 0;
-
- if (!buffer) {
- /* This makes the buffer be freed by swsusp_free() */
- buffer = get_image_page(GFP_ATOMIC, PG_ANY);
- if (!buffer)
- return -ENOMEM;
- }
- if (!handle->cur) {
- int error;
-
- error = init_header((struct swsusp_info *)buffer);
- if (error)
- return error;
- handle->buffer = buffer;
- memory_bm_position_reset(&orig_bm);
- memory_bm_position_reset(&copy_bm);
- } else if (handle->cur <= nr_meta_pages) {
+ if (handle->cur < nr_meta_pages) {
memset(buffer, 0, PAGE_SIZE);
pack_pfns(buffer, &orig_bm);
- } else {
+ } else if (handle->cur < nr_meta_pages + nr_copy_pages) {
struct page *page;

page = pfn_to_page(memory_bm_next_pfn(&copy_bm));
@@ -1655,6 +1672,8 @@ int snapshot_read_next(struct snapshot_handle *handle)
} else {
handle->buffer = page_address(page);
}
+ } else {
+ return 0;
}
handle->cur++;
return PAGE_SIZE;
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index f09494e..9b319f1 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -448,7 +448,6 @@ int swsusp_write(unsigned int flags)
{
struct hibernate_io_handle *io_handle;
struct snapshot_handle snapshot;
- struct swsusp_info *header;
unsigned long pages;
int error;

@@ -464,17 +463,12 @@ int swsusp_write(unsigned int flags)
goto out_finish;
}
memset(&snapshot, 0, sizeof(struct snapshot_handle));
- error = snapshot_read_next(&snapshot);
- if (error < PAGE_SIZE) {
- if (error >= 0)
- error = -EFAULT;
-
+ error = snapshot_write_init(io_handle, &snapshot);
+ if (error) {
+ printk(KERN_ERR "PM: Cannot init writer\n");
goto out_finish;
}
- header = (struct swsusp_info *)data_of(snapshot);
- error = hibernate_io_ops->write_page(io_handle, header, NULL);
- if (!error)
- error = save_image(io_handle, &snapshot, pages - 1);
+ error = save_image(io_handle, &snapshot, pages - 1);
out_finish:
error = hibernate_io_ops->writer_finish(io_handle, flags, error);
return error;
--
1.7.1

2010-06-02 11:40:51

by Nigel Cunningham

Subject: Re: [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

Hi Jiri et al.

On 02/06/10 18:52, Jiri Slaby wrote:
> Hi,
>
> I addressed the comments I got on the previous RFC. I left the handles
> in place, the functions in hibernate_io_ops now works on them. Further
> I got rid of the memory barriers and minimized global variables as much
> as possible. Comments welcome.

Hmm. I've been working on a set of patches too!

How about if I post them in shortly. We can then look at where there's
overlap and what to take from each.

Nigel

2010-06-02 12:37:23

by Nigel Cunningham

Subject: Re: [linux-pm] [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

Hi.

On 02/06/10 18:52, Jiri Slaby wrote:
> Hi,
>
> I addressed the comments I got on the previous RFC. I left the handles
> in place, the functions in hibernate_io_ops now works on them. Further
> I got rid of the memory barriers and minimized global variables as much
> as possible. Comments welcome.
>
> --
>
> Some code, which will be moved out of swap.c, will know nothing about
> swap. There will be also other than swap writers later, so that it
> won't make sense at all.
>
> So introduce a new structure called hibernate_io_handle which
> currently contains only a pointer to private data, but is independent
> on I/O reader/writer actually used. Private data are swap_map_handle
> for now.
>
> This structure is allocated in _start and freed in _finish. This will
> correspond to the later introduction of hibernate_io_ops where users
> will do handle=start->repeat{read/write(handle)}->finish(handle).
> I.e. they will operate on handle instead of global variables.

I'm starting on appending your patch series to mine (yes, I've given you
no time to object :> I'm too keen!)

Since, in my tree, swap_map pages are history, I'll skip this patch.

Nigel

2010-06-10 13:55:54

by Pavel Machek

Subject: Re: [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

Hi!

> I addressed the comments I got on the previous RFC. I left the handles
> in place, the functions in hibernate_io_ops now works on them. Further
> I got rid of the memory barriers and minimized global variables as much
> as possible. Comments welcome.

It would be good if you carried ack-s from previous rounds, so that I
don't have to review good patches again...

> Some code, which will be moved out of swap.c, will know nothing about
> swap. There will be also other than swap writers later, so that it
> won't make sense at all.
>
> So introduce a new structure called hibernate_io_handle which
> currently contains only a pointer to private data, but is independent
> on I/O reader/writer actually used. Private data are swap_map_handle
> for now.
>
> This structure is allocated in _start and freed in _finish. This will
> correspond to the later introduction of hibernate_io_ops where users
> will do handle=start->repeat{read/write(handle)}->finish(handle).
> I.e. they will operate on handle instead of global variables.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>

ack.

> +/**
> + * hib_io_handle_alloc - allocate io handle with priv_size for private data
> + *
> + * @priv_size: the sie to allocate behind hibernate_io_handle for private use
> + */
> +static inline struct hibernate_io_handle *hib_io_handle_alloc(size_t priv_size)
> +{
> + struct hibernate_io_handle *ret;
> + ret = kzalloc(sizeof(*ret) + priv_size, GFP_KERNEL);
> + if (ret)
> + ret->priv = ret + 1;

Uhuh, why this? Aha, grabbing the pointer to priv_size-sized area at
the end of regular struct?

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2010-06-11 09:46:24

by Nigel Cunningham

Subject: Re: [linux-pm] [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

Hi.

(Sorry Jiri - unintentionally originally replied only to you).

On 02/06/10 18:52, Jiri Slaby wrote:
> Hi,
>
> I addressed the comments I got on the previous RFC. I left the handles
> in place, the functions in hibernate_io_ops now works on them. Further
> I got rid of the memory barriers and minimized global variables as much
> as possible. Comments welcome.

I would like to hear the arguments for using these handles. I understand
there may have been some previous discussion, but am unable to find it.

It seems far more sensible to me to not pass around a handle that
virtually nothing actually uses, and instead store and utilise the state
in the place where it is actually useful. If we had more than one struct
hibernate_io_handle in use at a time, I could understand going this way.
As it stands, however...

Regards,

Nigel

2010-06-21 15:21:19

by Jiri Slaby

Subject: Re: [linux-pm] [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

On 06/11/2010 11:46 AM, Nigel Cunningham wrote:
> On 02/06/10 18:52, Jiri Slaby wrote:
>> I addressed the comments I got on the previous RFC. I left the handles
>> in place, the functions in hibernate_io_ops now works on them. Further
>> I got rid of the memory barriers and minimized global variables as much
>> as possible. Comments welcome.
>
> I would like to hear the arguments for using these handles. I understand
> there may have been some previous discussion, but am unable to find it.
>
> It seems far more sensible to me to not pass around a handle that
> virtually nothing actually uses, and instead store and utilise the state
> in the place where it is actually useful. If we had more than one struct
> hibernate_io_handle in use at a time, I could understand going this way.
> As it stands, however...

Hi, it I added that based on this: http://lkml.org/lkml/2010/3/24/458

--
js

2010-06-21 15:23:29

by Jiri Slaby

Subject: Re: [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

On 06/10/2010 03:55 PM, Pavel Machek wrote:
> It would be good if you carried ack-s from previous rounds, so that I
> don't have to review good patches again...

Hi, previously ACKed patches were merged already. These were much
rewritten and their original versions were rather NACKed. Otherwise I
transfer ACKs indeed.

>> +/**
>> + * hib_io_handle_alloc - allocate io handle with priv_size for private data
>> + *
>> + * @priv_size: the sie to allocate behind hibernate_io_handle for private use
>> + */
>> +static inline struct hibernate_io_handle *hib_io_handle_alloc(size_t priv_size)
>> +{
>> + struct hibernate_io_handle *ret;
>> + ret = kzalloc(sizeof(*ret) + priv_size, GFP_KERNEL);
>> + if (ret)
>> + ret->priv = ret + 1;
>
> Uhuh, why this? Aha, grabbing the pointer to priv_size-sized area at
> the end of regular struct?

Yes, exactly, any more transparent way to do it?

--
js

2010-06-21 21:59:07

by Nigel Cunningham

Subject: Re: [linux-pm] [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

Hi Jiri.

On 22/06/10 01:21, Jiri Slaby wrote:
> On 06/11/2010 11:46 AM, Nigel Cunningham wrote:
>> On 02/06/10 18:52, Jiri Slaby wrote:
>>> I addressed the comments I got on the previous RFC. I left the handles
>>> in place, the functions in hibernate_io_ops now works on them. Further
>>> I got rid of the memory barriers and minimized global variables as much
>>> as possible. Comments welcome.
>>
>> I would like to hear the arguments for using these handles. I understand
>> there may have been some previous discussion, but am unable to find it.
>>
>> It seems far more sensible to me to not pass around a handle that
>> virtually nothing actually uses, and instead store and utilise the state
>> in the place where it is actually useful. If we had more than one struct
>> hibernate_io_handle in use at a time, I could understand going this way.
>> As it stands, however...
>
> Hi, it I added that based on this: http://lkml.org/lkml/2010/3/24/458

Okay; thanks.

Looking at Pavel's comment is confusing. The variable you were adding
isn't "global static" (that's a contradiction in terms anyway). Its
scope is the file level.

Since the data is only used in this file, your change makes perfect
sense to me.

Rafael, Pavel: care to discuss this further?

Nigel

2010-06-24 15:22:39

by Rafael J. Wysocki

Subject: Re: [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

On Wednesday, June 02, 2010, Jiri Slaby wrote:
> Hi,

Hi,

> I addressed the comments I got on the previous RFC.

Well, some of them. :-)

> I left the handles in place, the functions in hibernate_io_ops now works on
> them. Further I got rid of the memory barriers and minimized global variables
> as much as possible. Comments welcome.

One general comment first. Can we just simply assume that the kernel won't
do _any_ transformations of image data in the case when s2disk is used?

I think that's going to simplify things quite a bit.

> --
>
> Some code, which will be moved out of swap.c, will know nothing about
> swap. There will be also other than swap writers later, so that it
> won't make sense at all.
>
> So introduce a new structure called hibernate_io_handle which
> currently contains only a pointer to private data, but is independent
> on I/O reader/writer actually used. Private data are swap_map_handle
> for now.
>
> This structure is allocated in _start and freed in _finish. This will
> correspond to the later introduction of hibernate_io_ops where users
> will do handle=start->repeat{read/write(handle)}->finish(handle).
> I.e. they will operate on handle instead of global variables.

OK

This one looks good.

> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> kernel/power/power.h | 24 ++++++++++
> kernel/power/swap.c | 114 +++++++++++++++++++++++++++++++------------------
> 2 files changed, 96 insertions(+), 42 deletions(-)
>
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 006270f..7427d54 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -1,3 +1,4 @@
> +#include <linux/slab.h> /* kzalloc */
> #include <linux/suspend.h>
> #include <linux/suspend_ioctls.h>
> #include <linux/utsname.h>
> @@ -115,6 +116,29 @@ struct snapshot_handle {
> */
> #define data_of(handle) ((handle).buffer)
>
> +/**
> + * struct hibernate_io_handle - handle for image I/O processing
> + *
> + * @priv: private data for each processor (swap_map_handle etc.)
> + */
> +struct hibernate_io_handle {
> + void *priv;
> +};
> +
> +/**
> + * hib_io_handle_alloc - allocate io handle with priv_size for private data
> + *
> + * @priv_size: the sie to allocate behind hibernate_io_handle for private use
> + */
> +static inline struct hibernate_io_handle *hib_io_handle_alloc(size_t priv_size)
> +{
> + struct hibernate_io_handle *ret;
> + ret = kzalloc(sizeof(*ret) + priv_size, GFP_KERNEL);
> + if (ret)
> + ret->priv = ret + 1;
> + return ret;
> +}
> +
> extern unsigned int snapshot_additional_pages(struct zone *zone);
> extern unsigned long snapshot_get_image_size(void);
> extern int snapshot_read_next(struct snapshot_handle *handle);
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index b0bb217..7096d20 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -268,8 +268,10 @@ static void release_swap_writer(struct swap_map_handle *handle)
> handle->cur = NULL;
> }
>
> -static int get_swap_writer(struct swap_map_handle *handle)
> +static struct hibernate_io_handle *get_swap_writer(void)
> {
> + struct hibernate_io_handle *io_handle;
> + struct swap_map_handle *handle;
> int ret;
>
> ret = swsusp_swap_check();
> @@ -277,12 +279,18 @@ static int get_swap_writer(struct swap_map_handle *handle)
> if (ret != -ENOSPC)
> printk(KERN_ERR "PM: Cannot find swap device, try "
> "swapon -a.\n");
> - return ret;
> + return ERR_PTR(ret);
> }
> + io_handle = hib_io_handle_alloc(sizeof(*handle));
> + if (!io_handle) {
> + ret = -ENOMEM;
> + goto err_close;
> + }
> + handle = io_handle->priv;
> handle->cur = (struct swap_map_page *)get_zeroed_page(GFP_KERNEL);
> if (!handle->cur) {
> ret = -ENOMEM;
> - goto err_close;
> + goto err_free;
> }
> handle->cur_swap = alloc_swapdev_block(root_swap);
> if (!handle->cur_swap) {
> @@ -291,17 +299,20 @@ static int get_swap_writer(struct swap_map_handle *handle)
> }
> handle->k = 0;
> handle->first_sector = handle->cur_swap;
> - return 0;
> + return io_handle;
> err_rel:
> release_swap_writer(handle);
> err_close:
> swsusp_close(FMODE_WRITE);
> - return ret;
> +err_free:
> + kfree(io_handle);
> + return ERR_PTR(ret);
> }
>
> -static int swap_write_page(struct swap_map_handle *handle, void *buf,
> - struct bio **bio_chain)
> +static int swap_write_page(struct hibernate_io_handle *io_handle, void *buf,
> + struct bio **bio_chain)
> {
> + struct swap_map_handle *handle = io_handle->priv;
> int error = 0;
> sector_t offset;
>
> @@ -339,9 +350,11 @@ static int flush_swap_writer(struct swap_map_handle *handle)
> return -EINVAL;
> }
>
> -static int swap_writer_finish(struct swap_map_handle *handle,
> +static int swap_writer_finish(struct hibernate_io_handle *io_handle,
> unsigned int flags, int error)
> {
> + struct swap_map_handle *handle = io_handle->priv;
> +
> if (!error) {
> flush_swap_writer(handle);
> printk(KERN_INFO "PM: S");
> @@ -352,6 +365,7 @@ static int swap_writer_finish(struct swap_map_handle *handle,
> if (error)
> free_all_swap_pages(root_swap);
> release_swap_writer(handle);
> + kfree(io_handle);
> swsusp_close(FMODE_WRITE);
>
> return error;
> @@ -361,8 +375,8 @@ static int swap_writer_finish(struct swap_map_handle *handle,
> * save_image - save the suspend image data
> */
>
> -static int save_image(struct swap_map_handle *handle,
> - struct snapshot_handle *snapshot,
> +static int save_image(struct hibernate_io_handle *io_handle,
> + struct snapshot_handle *snapshot,
> unsigned int nr_to_write)
> {
> unsigned int m;
> @@ -385,7 +399,7 @@ static int save_image(struct swap_map_handle *handle,
> ret = snapshot_read_next(snapshot);
> if (ret <= 0)
> break;
> - ret = swap_write_page(handle, data_of(*snapshot), &bio);
> + ret = swap_write_page(io_handle, data_of(*snapshot), &bio);
> if (ret)
> break;
> if (!(nr_pages % m))
> @@ -431,17 +445,17 @@ static int enough_swap(unsigned int nr_pages)
>
> int swsusp_write(unsigned int flags)
> {
> - struct swap_map_handle handle;
> + struct hibernate_io_handle *io_handle;
> struct snapshot_handle snapshot;
> struct swsusp_info *header;
> unsigned long pages;
> int error;
>
> pages = snapshot_get_image_size();
> - error = get_swap_writer(&handle);
> - if (error) {
> + io_handle = get_swap_writer();
> + if (IS_ERR(io_handle)) {
> printk(KERN_ERR "PM: Cannot get swap writer\n");
> - return error;
> + return PTR_ERR(io_handle);
> }
> if (!enough_swap(pages)) {
> printk(KERN_ERR "PM: Not enough free swap\n");
> @@ -457,11 +471,11 @@ int swsusp_write(unsigned int flags)
> goto out_finish;
> }
> header = (struct swsusp_info *)data_of(snapshot);
> - error = swap_write_page(&handle, header, NULL);
> + error = swap_write_page(io_handle, header, NULL);
> if (!error)
> - error = save_image(&handle, &snapshot, pages - 1);
> + error = save_image(io_handle, &snapshot, pages - 1);
> out_finish:
> - error = swap_writer_finish(&handle, flags, error);
> + error = swap_writer_finish(io_handle, flags, error);
> return error;
> }
>
> @@ -477,32 +491,43 @@ static void release_swap_reader(struct swap_map_handle *handle)
> handle->cur = NULL;
> }
>
> -static int get_swap_reader(struct swap_map_handle *handle,
> - unsigned int *flags_p)
> +static struct hibernate_io_handle *get_swap_reader(unsigned int *flags_p)
> {
> + struct hibernate_io_handle *io_handle;
> + struct swap_map_handle *handle;
> int error;
>
> *flags_p = swsusp_header->flags;
>
> if (!swsusp_header->image) /* how can this happen? */
> - return -EINVAL;
> + return ERR_PTR(-EINVAL);
>
> + io_handle = hib_io_handle_alloc(sizeof(*handle));
> + if (!io_handle)
> + return ERR_PTR(-ENOMEM);
> + handle = io_handle->priv;
> handle->cur = (struct swap_map_page *)get_zeroed_page(__GFP_WAIT | __GFP_HIGH);
> - if (!handle->cur)
> - return -ENOMEM;
> + if (!handle->cur) {
> + error = -ENOMEM;
> + goto err_free;
> + }
>
> error = hib_bio_read_page(swsusp_header->image, handle->cur, NULL);
> - if (error) {
> - release_swap_reader(handle);
> - return error;
> - }
> + if (error)
> + goto err_rel;
> handle->k = 0;
> - return 0;
> + return io_handle;
> +err_rel:
> + release_swap_reader(handle);
> +err_free:
> + kfree(io_handle);
> + return ERR_PTR(error);
> }
>
> -static int swap_read_page(struct swap_map_handle *handle, void *buf,
> - struct bio **bio_chain)
> +static int swap_read_page(struct hibernate_io_handle *io_handle, void *buf,
> + struct bio **bio_chain)
> {
> + struct swap_map_handle *handle = io_handle->priv;
> sector_t offset;
> int error;
>
> @@ -526,22 +551,25 @@ static int swap_read_page(struct swap_map_handle *handle, void *buf,
> return error;
> }
>
> -static int swap_reader_finish(struct swap_map_handle *handle)
> +static int swap_reader_finish(struct hibernate_io_handle *io_handle)
> {
> + struct swap_map_handle *handle = io_handle->priv;
> +
> release_swap_reader(handle);
> + kfree(io_handle);
>
> return 0;
> }
>
> /**
> - * load_image - load the image using the swap map handle
> + * load_image - load the image
> * @handle and the snapshot handle @snapshot
> * (assume there are @nr_pages pages to load)
> */
>
> -static int load_image(struct swap_map_handle *handle,
> - struct snapshot_handle *snapshot,
> - unsigned int nr_to_read)
> +static int load_image(struct hibernate_io_handle *io_handle,
> + struct snapshot_handle *snapshot,
> + unsigned int nr_to_read)
> {
> unsigned int m;
> int error = 0;
> @@ -563,7 +591,7 @@ static int load_image(struct swap_map_handle *handle,
> error = snapshot_write_next(snapshot);
> if (error <= 0)
> break;
> - error = swap_read_page(handle, data_of(*snapshot), &bio);
> + error = swap_read_page(io_handle, data_of(*snapshot), &bio);
> if (error)
> break;
> if (snapshot->sync_read)
> @@ -598,7 +626,7 @@ static int load_image(struct swap_map_handle *handle,
> int swsusp_read(unsigned int *flags_p)
> {
> int error;
> - struct swap_map_handle handle;
> + struct hibernate_io_handle *io_handle;
> struct snapshot_handle snapshot;
> struct swsusp_info *header;
>
> @@ -607,14 +635,16 @@ int swsusp_read(unsigned int *flags_p)
> if (error < PAGE_SIZE)
> return error < 0 ? error : -EFAULT;
> header = (struct swsusp_info *)data_of(snapshot);
> - error = get_swap_reader(&handle, flags_p);
> - if (error)
> + io_handle = get_swap_reader(flags_p);
> + if (IS_ERR(io_handle)) {
> + error = PTR_ERR(io_handle);
> goto end;
> + }
> if (!error)
> - error = swap_read_page(&handle, header, NULL);
> + error = swap_read_page(io_handle, header, NULL);
> if (!error)
> - error = load_image(&handle, &snapshot, header->pages - 1);
> - swap_reader_finish(&handle);
> + error = load_image(io_handle, &snapshot, header->pages - 1);
> + swap_reader_finish(io_handle);
> end:
> if (!error)
> pr_debug("PM: Image successfully loaded\n");
>

2010-06-24 15:25:19

by Rafael J. Wysocki

Subject: Re: [PATCH 2/9] PM / Hibernate: add hibernate_io_ops

On Wednesday, June 02, 2010, Jiri Slaby wrote:
> This is a centralized structure for actually used reader/writer. It
> contains basic operations called from snapshot layer which doesn't
> need to know who reads/writes the image right now -- this is
> specified by current set of functions stored in hibernate_io_ops.
>
> In other words there are more than one hibernate_io_ops which
> contain different set of functions. E.g. one is for swap, one for
> userspace (user.c) handler.

I don't like that user space reader/writer idea. I'll write more on that
commenting the next patches.

> For now they will hold only swap operations. In next patches, user
> support will be converted to ops as well to have a single layer which
> allows easier transitions later.

The patch looks good to me.

> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> kernel/power/hibernate.c | 2 +
> kernel/power/power.h | 27 +++++++++++++++++++++
> kernel/power/swap.c | 57 ++++++++++++++++++++++++++++++---------------
> 3 files changed, 67 insertions(+), 19 deletions(-)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index aa9e916..7d38e6a 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -35,6 +35,8 @@ dev_t swsusp_resume_device;
> sector_t swsusp_resume_block;
> int in_suspend __nosavedata = 0;
>
> +struct hibernate_io_ops *hibernate_io_ops;
> +
> enum {
> HIBERNATION_INVALID,
> HIBERNATION_PLATFORM,
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 7427d54..32a40f9 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -139,6 +139,31 @@ static inline struct hibernate_io_handle *hib_io_handle_alloc(size_t priv_size)
> return ret;
> }
>
> +/**
> + * struct hibernate_io_ops - functions of a hibernation module
> + *
> + * @free_space: hom much space is available for the image

s/hom/how/

> + * @reader_start: initialize an image reader
> + * @reader_finish: deinitialize the reader
> + * @writer_start: initialize an image writer
> + * @writer_finish: deinitialize the writer
> + * @read_page: read an image page from a backing store
> + * @write_page: write an image page to a backing store
> + */
> +struct hibernate_io_ops {
> + unsigned long (*free_space)(void);
> +
> + struct hibernate_io_handle *(*reader_start)(unsigned int *flags_p);
> + int (*reader_finish)(struct hibernate_io_handle *io_handle);
> + struct hibernate_io_handle *(*writer_start)(void);
> + int (*writer_finish)(struct hibernate_io_handle *io_handle,
> + unsigned int flags, int error);
> + int (*read_page)(struct hibernate_io_handle *io_handle, void *addr,
> + struct bio **bio_chain);
> + int (*write_page)(struct hibernate_io_handle *io_handle, void *addr,
> + struct bio **bio_chain);
> +};
> +
> extern unsigned int snapshot_additional_pages(struct zone *zone);
> extern unsigned long snapshot_get_image_size(void);
> extern int snapshot_read_next(struct snapshot_handle *handle);
> @@ -237,6 +262,8 @@ enum {
>
> extern int pm_test_level;
>
> +extern struct hibernate_io_ops *hibernate_io_ops;
> +
> #ifdef CONFIG_SUSPEND_FREEZER
> static inline int suspend_freeze_processes(void)
> {
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 7096d20..f09494e 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -268,7 +268,7 @@ static void release_swap_writer(struct swap_map_handle *handle)
> handle->cur = NULL;
> }
>
> -static struct hibernate_io_handle *get_swap_writer(void)
> +static struct hibernate_io_handle *swap_writer_start(void)
> {
> struct hibernate_io_handle *io_handle;
> struct swap_map_handle *handle;
> @@ -399,7 +399,8 @@ static int save_image(struct hibernate_io_handle *io_handle,
> ret = snapshot_read_next(snapshot);
> if (ret <= 0)
> break;
> - ret = swap_write_page(io_handle, data_of(*snapshot), &bio);
> + ret = hibernate_io_ops->write_page(io_handle,
> + data_of(*snapshot), &bio);
> if (ret)
> break;
> if (!(nr_pages % m))
> @@ -419,18 +420,18 @@ static int save_image(struct hibernate_io_handle *io_handle,
> }
>
> /**
> - * enough_swap - Make sure we have enough swap to save the image.
> + * enough_space - Make sure we have enough space to save the image.
> *
> - * Returns TRUE or FALSE after checking the total amount of swap
> - * space avaiable from the resume partition.
> + * Returns TRUE or FALSE after checking the total amount of
> + * space avaiable from the resume block.
> */
>
> -static int enough_swap(unsigned int nr_pages)
> +static int enough_space(unsigned int nr_pages)
> {
> - unsigned int free_swap = count_swap_pages(root_swap, 1);
> + unsigned int free_pages = hibernate_io_ops->free_space();
>
> - pr_debug("PM: Free swap pages: %u\n", free_swap);
> - return free_swap > nr_pages + PAGES_FOR_IO;
> + pr_debug("PM: Free storage pages: %u\n", free_pages);
> + return free_pages > nr_pages + PAGES_FOR_IO;
> }
>
> /**
> @@ -452,13 +453,13 @@ int swsusp_write(unsigned int flags)
> int error;
>
> pages = snapshot_get_image_size();
> - io_handle = get_swap_writer();
> + io_handle = hibernate_io_ops->writer_start();
> if (IS_ERR(io_handle)) {
> printk(KERN_ERR "PM: Cannot get swap writer\n");
> return PTR_ERR(io_handle);
> }
> - if (!enough_swap(pages)) {
> - printk(KERN_ERR "PM: Not enough free swap\n");
> + if (!enough_space(pages)) {
> + printk(KERN_ERR "PM: Not enough free space for image\n");
> error = -ENOSPC;
> goto out_finish;
> }
> @@ -471,14 +472,19 @@ int swsusp_write(unsigned int flags)
> goto out_finish;
> }
> header = (struct swsusp_info *)data_of(snapshot);
> - error = swap_write_page(io_handle, header, NULL);
> + error = hibernate_io_ops->write_page(io_handle, header, NULL);
> if (!error)
> error = save_image(io_handle, &snapshot, pages - 1);
> out_finish:
> - error = swap_writer_finish(io_handle, flags, error);
> + error = hibernate_io_ops->writer_finish(io_handle, flags, error);
> return error;
> }
>
> +static unsigned long swap_free_space(void)
> +{
> + return count_swap_pages(root_swap, 1);
> +}
> +
> /**
> * The following functions allow us to read data using a swap map
> * in a file-alike way
> @@ -491,7 +497,7 @@ static void release_swap_reader(struct swap_map_handle *handle)
> handle->cur = NULL;
> }
>
> -static struct hibernate_io_handle *get_swap_reader(unsigned int *flags_p)
> +static struct hibernate_io_handle *swap_reader_start(unsigned int *flags_p)
> {
> struct hibernate_io_handle *io_handle;
> struct swap_map_handle *handle;
> @@ -561,6 +567,17 @@ static int swap_reader_finish(struct hibernate_io_handle *io_handle)
> return 0;
> }
>
> +struct hibernate_io_ops swap_ops = {
> + .free_space = swap_free_space,
> +
> + .reader_start = swap_reader_start,
> + .reader_finish = swap_reader_finish,
> + .writer_start = swap_writer_start,
> + .writer_finish = swap_writer_finish,
> + .read_page = swap_read_page,
> + .write_page = swap_write_page,
> +};
> +
> /**
> * load_image - load the image
> * @handle and the snapshot handle @snapshot
> @@ -591,7 +608,8 @@ static int load_image(struct hibernate_io_handle *io_handle,
> error = snapshot_write_next(snapshot);
> if (error <= 0)
> break;
> - error = swap_read_page(io_handle, data_of(*snapshot), &bio);
> + error = hibernate_io_ops->read_page(io_handle,
> + data_of(*snapshot), &bio);
> if (error)
> break;
> if (snapshot->sync_read)
> @@ -635,16 +653,16 @@ int swsusp_read(unsigned int *flags_p)
> if (error < PAGE_SIZE)
> return error < 0 ? error : -EFAULT;
> header = (struct swsusp_info *)data_of(snapshot);
> - io_handle = get_swap_reader(flags_p);
> + io_handle = hibernate_io_ops->reader_start(flags_p);
> if (IS_ERR(io_handle)) {
> error = PTR_ERR(io_handle);
> goto end;
> }
> if (!error)
> - error = swap_read_page(io_handle, header, NULL);
> + error = hibernate_io_ops->read_page(io_handle, header, NULL);
> if (!error)
> error = load_image(io_handle, &snapshot, header->pages - 1);
> - swap_reader_finish(io_handle);
> + hibernate_io_ops->reader_finish(io_handle);
> end:
> if (!error)
> pr_debug("PM: Image successfully loaded\n");
> @@ -713,6 +731,7 @@ static int swsusp_header_init(void)
> swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
> if (!swsusp_header)
> panic("Could not allocate memory for swsusp_header\n");
> + hibernate_io_ops = &swap_ops;
> return 0;
> }
>
>

2010-06-24 16:29:06

by Rafael J. Wysocki

Subject: Re: [PATCH 3/9] PM / Hibernate: user, implement user_ops writer

On Wednesday, June 02, 2010, Jiri Slaby wrote:
> Switch /dev/snapshot writer to hibernate_io_ops approach so that we
> can do whatever we want with snapshot processing code. All the later
> code changes will be transparent and needn't care about different
> readers/writers.

Well. I don't want image data to undergo _any_ transformations within the
kernel before going to s2disk. In fact, s2disk is supposed to do whatever it
wants with the image data (that are supposed to be _raw_ image data).

So, I don't think we need to "switch" /dev/snapshot to anything adding
complexity in the process.

> In this patch only writer is implemented -- for better bisectability
> if something breaks. (The development was easier too, because one
> could break only one part, not both.)
>
> Also, when using this interface, switch to the ops on open/release,
> so they are used.
>
> There are explicit barriers protecting to_do_buffer, because it is
> all done in a producer-consumer fashion:
> PRODUCER (snapshot layer)
> 1) to_do_buffer is set
> 2) set TODO bit
> 3) wake CONSUMER
> 4) wait for TODO bit _clear_
>
> CONSUMER (fops->read)
> 1) wait for TODO bit
> 2) pass to_do_buffer to userspace
> 3) clear TODO bit
> 4) wake PRODUCER
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> kernel/power/user.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 132 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/power/user.c b/kernel/power/user.c
> index e819e17..b4610c3 100644
> --- a/kernel/power/user.c
> +++ b/kernel/power/user.c
> @@ -23,6 +23,8 @@
> #include <linux/console.h>
> #include <linux/cpu.h>
> #include <linux/freezer.h>
> +#include <linux/mutex.h>
> +#include <linux/wait.h>
> #include <scsi/scsi_scan.h>
>
> #include <asm/uaccess.h>
> @@ -61,10 +63,93 @@ static struct snapshot_data {
> char frozen;
> char ready;
> char platform_support;
> + struct hibernate_io_ops *prev_ops;
> } snapshot_state;
>
> atomic_t snapshot_device_available = ATOMIC_INIT(1);
>
> +static struct {
> + void *buffer; /* buffer to pass through */
> + struct workqueue_struct *worker; /* the thread initiating read/write */
> + wait_queue_head_t wait; /* wait for buffer */
> + wait_queue_head_t done; /* read/write done? */
> + struct mutex lock; /* protection for buffer */
> +
> +#define TODO_WORK 1
> +#define TODO_FINISH 2
> +#define TODO_CLOSED 3
> +#define TODO_ERROR 4
> + DECLARE_BITMAP(flags, 10); /* TODO_* flags defined above */
> +} to_do;
> +
> +static unsigned long user_free_space(void)
> +{
> + return ~0UL; /* we have no idea, maybe we will fail later */
> +}
> +
> +static struct hibernate_io_handle *user_writer_start(void)
> +{
> + return hib_io_handle_alloc(0) ? : ERR_PTR(-ENOMEM);
> +}
> +
> +static int user_write_page(struct hibernate_io_handle *io_handle, void *buf,
> + struct bio **bio_chain)
> +{
> + int err = 0;
> +
> + if (test_bit(TODO_CLOSED, to_do.flags))
> + return -EIO;
> +
> + mutex_lock(&to_do.lock);
> + to_do.buffer = buf;
> + mutex_unlock(&to_do.lock);
> + set_bit(TODO_WORK, to_do.flags);
> + wake_up_interruptible(&to_do.wait);
> +
> + wait_event(to_do.done, !test_bit(TODO_WORK, to_do.flags) ||
> + (err = test_bit(TODO_CLOSED, to_do.flags)));
> +
> + return err ? -EIO : 0;
> +}
> +
> +static int user_writer_finish(struct hibernate_io_handle *io_handle,
> + unsigned int flags, int error)
> +{
> + int err = 0;
> +
> + if (error)
> + set_bit(TODO_ERROR, to_do.flags);
> + set_bit(TODO_FINISH, to_do.flags);
> + wake_up_interruptible(&to_do.wait);
> +
> + wait_event(to_do.done, !test_bit(TODO_FINISH, to_do.flags) ||
> + (err = test_bit(TODO_CLOSED, to_do.flags)));
> +
> + if (!error && err)
> + error = -EIO;
> +
> + return error;
> +}
> +
> +struct hibernate_io_ops user_ops = {
> + .free_space = user_free_space,
> +
> + .writer_start = user_writer_start,
> + .writer_finish = user_writer_finish,
> + .write_page = user_write_page,
> +};
> +
> +static void snapshot_writer(struct work_struct *work)
> +{
> + int ret;
> +
> + ret = swsusp_write(0);
> + if (ret)
> + printk(KERN_ERR "PM: write failed with %d\n", ret);
> +}
> +
> +static DECLARE_WORK(snapshot_writer_w, snapshot_writer);
> +
> static int snapshot_open(struct inode *inode, struct file *filp)
> {
> struct snapshot_data *data;
> @@ -115,9 +200,17 @@ static int snapshot_open(struct inode *inode, struct file *filp)
> }
> if (error)
> atomic_inc(&snapshot_device_available);
> + else {
> + data->prev_ops = hibernate_io_ops;
> + hibernate_io_ops = &user_ops;
> + }
> data->frozen = 0;
> data->ready = 0;
> data->platform_support = 0;
> + memset(to_do.flags, 0, sizeof(*to_do.flags));
> + init_waitqueue_head(&to_do.wait);
> + init_waitqueue_head(&to_do.done);
> + mutex_init(&to_do.lock);
>
> Unlock:
> mutex_unlock(&pm_mutex);
> @@ -131,6 +224,10 @@ static int snapshot_release(struct inode *inode, struct file *filp)
>
> mutex_lock(&pm_mutex);
>
> + set_bit(TODO_CLOSED, to_do.flags);
> + wake_up(&to_do.done);
> + flush_workqueue(to_do.worker);
> +
> swsusp_free();
> free_basic_memory_bitmaps();
> data = filp->private_data;
> @@ -139,6 +236,7 @@ static int snapshot_release(struct inode *inode, struct file *filp)
> thaw_processes();
> pm_notifier_call_chain(data->mode == O_WRONLY ?
> PM_POST_HIBERNATION : PM_POST_RESTORE);
> + hibernate_io_ops = data->prev_ops;
> atomic_inc(&snapshot_device_available);
>
> mutex_unlock(&pm_mutex);
> @@ -152,6 +250,7 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
> struct snapshot_data *data;
> ssize_t res;
> loff_t pg_offp = *offp & ~PAGE_MASK;
> + int fin = 0;
>
> mutex_lock(&pm_mutex);
>
> @@ -161,18 +260,31 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
> goto Unlock;
> }
> if (!pg_offp) { /* on page boundary? */
> - res = snapshot_read_next(&data->handle);

I really don't understand why you're willing to do this. In the s2disk case we
have an image in memory and we only want to trasfer it to user space
page-by-page, where user space decides when each page is going to be
transferred. snapshot_read_next() is for that and while you can modify it
however you like, I don't think it's really worth it.

> - if (res <= 0)
> + res = wait_event_interruptible(to_do.wait,
> + test_bit(TODO_WORK, to_do.flags) ||
> + (fin = test_and_clear_bit(TODO_FINISH, to_do.flags)));
> + if (res)
> goto Unlock;
> - } else {
> - res = PAGE_SIZE - pg_offp;
> + if (test_bit(TODO_ERROR, to_do.flags)) {
> + res = -EIO;
> + goto Unlock;
> + }
> + if (fin)
> + goto wake;
> }
> + res = PAGE_SIZE - pg_offp;
>
> - res = simple_read_from_buffer(buf, count, &pg_offp,
> - data_of(data->handle), res);
> + mutex_lock(&to_do.lock);
> + res = simple_read_from_buffer(buf, count, &pg_offp, to_do.buffer, res);
> + mutex_unlock(&to_do.lock);
> if (res > 0)
> *offp += res;
>
> + if (!(pg_offp & ~PAGE_MASK)) {
> + clear_bit(TODO_WORK, to_do.flags);
> +wake:
> + wake_up(&to_do.done);
> + }
> Unlock:
> mutex_unlock(&pm_mutex);
>
> @@ -278,8 +390,11 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
> error = hibernation_snapshot(data->platform_support);
> if (!error)
> error = put_user(in_suspend, (int __user *)arg);
> - if (!error)
> + if (!error) {
> + if (in_suspend)
> + queue_work(to_do.worker, &snapshot_writer_w);
> data->ready = 1;
> + }
> break;
>
> case SNAPSHOT_ATOMIC_RESTORE:
> @@ -473,7 +588,16 @@ static struct miscdevice snapshot_device = {
>
> static int __init snapshot_device_init(void)
> {
> - return misc_register(&snapshot_device);
> + int ret;
> +
> + to_do.worker = create_singlethread_workqueue("suspend_worker");
> + if (!to_do.worker)
> + return -ENOMEM;
> +
> + ret = misc_register(&snapshot_device);
> + if (ret)
> + destroy_workqueue(to_do.worker);
> + return ret;
> };
>
> device_initcall(snapshot_device_init);

And using a special workqueue for this purpose is seriously over the top IMnshO.

Why don't you just regard s2disk as a special case and don't touch it
(or modify it only so much to prevent it from getting in the way)?

Rafael

2010-06-25 13:38:42

by Rafael J. Wysocki

Subject: Re: [PATCH 4/9] PM / Hibernate: user, implement user_ops reader

On Wednesday, June 02, 2010, Jiri Slaby wrote:
> Switch /dev/snapshot writer to hibernate_io_ops approach so that we
> can do whatever we want with snapshot processing code. All the later
> code changes will be transparent and needn't care about different
> readers/writers.
>
> In this patch only reader is implemented, writer was done previously.
>
> It works similarly to writer, CONSUMER here is snapshot layer,
> PRODUCER is fops->write.

Here, I have pretty much the same comment as for [3/9]. I don't really think
it's necessary or even useful to change the s2disk code paths.

> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---

Rafael

2010-06-25 13:46:29

by Rafael J. Wysocki

Subject: Re: [PATCH 5/9] PM / Hibernate: add chunk i/o support

On Wednesday, June 02, 2010, Jiri Slaby wrote:
> Chunk support is useful when not writing whole pages at once. It takes
> care of joining the buffers into the pages and writing at once when
> needed.
>
> This adds functions for both reads and writes.
>
> In the end when pages are compressed they use this interface as well
> (because they are indeed shorter than PAGE_SIZE).

This one looks good up to a couple of minor things below.

> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> kernel/power/block_io.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++
> kernel/power/power.h | 16 ++++++
> 2 files changed, 142 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/power/block_io.c b/kernel/power/block_io.c
> index 97024fd..5b6413d 100644
> --- a/kernel/power/block_io.c
> +++ b/kernel/power/block_io.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) 1998,2001-2005 Pavel Machek <[email protected]>
> * Copyright (C) 2006 Rafael J. Wysocki <[email protected]>
> + * Copyright (C) 2004-2008 Nigel Cunningham (nigel at tuxonice net)

Please use the e-mail address.

> *
> * This file is released under the GPLv2.
> */
> @@ -74,6 +75,131 @@ int hib_bio_write_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
> virt_to_page(addr), bio_chain);
> }
>
> +static int hib_buffer_init_rw(struct hibernate_io_handle *io_handle,
> + int writing)
> +{
> + /* should never happen - it means we didn't finish properly last time */
> + BUG_ON(io_handle->chunk_buffer || io_handle->chunk_buffer_pos);

Please use if (WARN_ON(...)) retuirn -EINVAL; instead. The BUG_ON() will kill
an otherwise perfectly useable system just because we happen to have a bug in
the hibernation code.

Generally, please don't use BUG_ON() unless you _have_ _to_ (ie. the system
would crash anyway causing data loss and possibly other damage to happen if
we didn't do it).

Rafael

2010-06-25 13:55:06

by Rafael J. Wysocki

Subject: Re: [PATCH 6/9] PM / Hibernate: split snapshot_read_next

On Wednesday, June 02, 2010, Jiri Slaby wrote:
> When writing the snapshot, do the initialization and header write in
> a separate function. This makes the code more readable and lowers
> complexity of snapshot_read_next.

This one looks good, but it seems to depend on [3/9] and [4/9]. Does it?

> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> kernel/power/power.h | 2 +
> kernel/power/snapshot.c | 65 ++++++++++++++++++++++++++++++----------------
> kernel/power/swap.c | 14 +++-------
> 3 files changed, 48 insertions(+), 33 deletions(-)
>
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 812b66c..ff3f63f 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -171,6 +171,8 @@ struct hibernate_io_ops {
>
> extern unsigned int snapshot_additional_pages(struct zone *zone);
> extern unsigned long snapshot_get_image_size(void);
> +extern int snapshot_write_init(struct hibernate_io_handle *io_handle,
> + struct snapshot_handle *handle);
> extern int snapshot_read_next(struct snapshot_handle *handle);
> extern int snapshot_write_next(struct snapshot_handle *handle);
> extern void snapshot_write_finalize(struct snapshot_handle *handle);
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 25ce010..4600d15 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1598,10 +1598,46 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
> }
>
> /**
> + * snapshot_write_init - initialization before writing the snapshot to
> + * a backing storage
> + *
> + * This function *must* be called before snapshot_read_next to initialize
> + * @handle and write a header.
> + *
> + * @io_handle: handle used when writing the initial info onto storage
> + * @handle: snapshot handle to init
> + */
> +int snapshot_write_init(struct hibernate_io_handle *io_handle,
> + struct snapshot_handle *handle)
> +{
> + int ret;
> +
> + /* This makes the buffer be freed by swsusp_free() */
> + buffer = get_image_page(GFP_ATOMIC, PG_ANY);
> + if (!buffer)
> + return -ENOMEM;
> + init_header(buffer);
> + ret = hib_buffer_init_write(io_handle);
> + if (ret)
> + return ret;
> + ret = hib_buffer_write(io_handle, buffer, sizeof(struct swsusp_info));
> + if (ret)
> + goto finish;
> + hib_buffer_finish_write(io_handle);
> + memory_bm_position_reset(&orig_bm);
> + memory_bm_position_reset(&copy_bm);
> + handle->buffer = buffer;
> + return 0;
> +finish:
> + hib_buffer_finish_write(io_handle);
> + return ret;
> +}
> +
> +/**
> * snapshot_read_next - used for reading the system memory snapshot.
> *
> - * On the first call to it @handle should point to a zeroed
> - * snapshot_handle structure. The structure gets updated and a pointer
> + * Before calling this function, snapshot_write_init has to be called with
> + * handle passed as @handle here. The structure gets updated and a pointer
> * to it should be passed to this function every next time.
> *
> * On success the function returns a positive number. Then, the caller
> @@ -1613,31 +1649,12 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
> * structure pointed to by @handle is not updated and should not be used
> * any more.
> */
> -
> int snapshot_read_next(struct snapshot_handle *handle)
> {
> - if (handle->cur > nr_meta_pages + nr_copy_pages)
> - return 0;
> -
> - if (!buffer) {
> - /* This makes the buffer be freed by swsusp_free() */
> - buffer = get_image_page(GFP_ATOMIC, PG_ANY);
> - if (!buffer)
> - return -ENOMEM;
> - }
> - if (!handle->cur) {
> - int error;
> -
> - error = init_header((struct swsusp_info *)buffer);
> - if (error)
> - return error;
> - handle->buffer = buffer;
> - memory_bm_position_reset(&orig_bm);
> - memory_bm_position_reset(&copy_bm);
> - } else if (handle->cur <= nr_meta_pages) {
> + if (handle->cur < nr_meta_pages) {
> memset(buffer, 0, PAGE_SIZE);
> pack_pfns(buffer, &orig_bm);
> - } else {
> + } else if (handle->cur < nr_meta_pages + nr_copy_pages) {
> struct page *page;
>
> page = pfn_to_page(memory_bm_next_pfn(&copy_bm));
> @@ -1655,6 +1672,8 @@ int snapshot_read_next(struct snapshot_handle *handle)
> } else {
> handle->buffer = page_address(page);
> }
> + } else {
> + return 0;
> }
> handle->cur++;
> return PAGE_SIZE;
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index f09494e..9b319f1 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -448,7 +448,6 @@ int swsusp_write(unsigned int flags)
> {
> struct hibernate_io_handle *io_handle;
> struct snapshot_handle snapshot;
> - struct swsusp_info *header;
> unsigned long pages;
> int error;
>
> @@ -464,17 +463,12 @@ int swsusp_write(unsigned int flags)
> goto out_finish;
> }
> memset(&snapshot, 0, sizeof(struct snapshot_handle));
> - error = snapshot_read_next(&snapshot);
> - if (error < PAGE_SIZE) {
> - if (error >= 0)
> - error = -EFAULT;
> -
> + error = snapshot_write_init(io_handle, &snapshot);
> + if (error) {
> + printk(KERN_ERR "PM: Cannot init writer\n");
> goto out_finish;
> }
> - header = (struct swsusp_info *)data_of(snapshot);
> - error = hibernate_io_ops->write_page(io_handle, header, NULL);
> - if (!error)
> - error = save_image(io_handle, &snapshot, pages - 1);
> + error = save_image(io_handle, &snapshot, pages - 1);
> out_finish:
> error = hibernate_io_ops->writer_finish(io_handle, flags, error);
> return error;
>

2010-06-25 13:55:53

by Rafael J. Wysocki

Subject: Re: [PATCH 7/9] PM / Hibernate: split snapshot_write_next

On Wednesday, June 02, 2010, Jiri Slaby wrote:
> When reading the snapshot, do the initialization and header read in
> a separate function. This makes the code more readable and lowers
> complexity of snapshot_write_next.

Same as for [6/9], looks good by itself, but seems to depend on
[3/9] and [4/9].

> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> kernel/power/power.h | 2 +
> kernel/power/snapshot.c | 100 ++++++++++++++++++++++++++++++-----------------
> kernel/power/swap.c | 20 ++++------
> 3 files changed, 74 insertions(+), 48 deletions(-)
>
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index ff3f63f..6e2e796 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -171,6 +171,8 @@ struct hibernate_io_ops {
>
> extern unsigned int snapshot_additional_pages(struct zone *zone);
> extern unsigned long snapshot_get_image_size(void);
> +extern int snapshot_read_init(struct hibernate_io_handle *io_handle,
> + struct snapshot_handle *handle);
> extern int snapshot_write_init(struct hibernate_io_handle *io_handle,
> struct snapshot_handle *handle);
> extern int snapshot_read_next(struct snapshot_handle *handle);
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 4600d15..9cd6931 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -2129,10 +2129,54 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
> }
>
> /**
> + * snapshot_read_init - initialization before reading the snapshot from
> + * a backing storage
> + *
> + * This function *must* be called before snapshot_write_next to initialize
> + * @handle and read header.
> + *
> + * @io_handle: io handle used for reading
> + * @handle: snapshot handle to init
> + */
> +int snapshot_read_init(struct hibernate_io_handle *io_handle,
> + struct snapshot_handle *handle)
> +{
> + int ret;
> +
> + /* This makes the buffer be freed by swsusp_free() */
> + buffer = get_image_page(GFP_ATOMIC, PG_ANY);
> + if (!buffer)
> + return -ENOMEM;
> +
> + ret = hib_buffer_init_read(io_handle);
> + if (ret)
> + return ret;
> + ret = hib_buffer_read(io_handle, buffer, sizeof(struct swsusp_info));
> + if (ret)
> + goto finish;
> + hib_buffer_finish_read(io_handle);
> +
> + ret = load_header(buffer);
> + if (ret)
> + return ret;
> +
> + ret = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
> + if (ret)
> + return ret;
> +
> + handle->buffer = buffer;
> + handle->sync_read = 1;
> + return 0;
> +finish:
> + hib_buffer_finish_read(io_handle);
> + return ret;
> +}
> +
> +/**
> * snapshot_write_next - used for writing the system memory snapshot.
> *
> - * On the first call to it @handle should point to a zeroed
> - * snapshot_handle structure. The structure gets updated and a pointer
> + * Before calling this function, snapshot_read_init has to be called with
> + * handle passed as @handle here. The structure gets updated and a pointer
> * to it should be passed to this function every next time.
> *
> * On success the function returns a positive number. Then, the caller
> @@ -2144,42 +2188,20 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
> * structure pointed to by @handle is not updated and should not be used
> * any more.
> */
> -
> int snapshot_write_next(struct snapshot_handle *handle)
> {
> static struct chain_allocator ca;
> int error = 0;
>
> - /* Check if we have already loaded the entire image */
> - if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages)
> - return 0;
> -
> handle->sync_read = 1;
> -
> - if (!handle->cur) {
> - if (!buffer)
> - /* This makes the buffer be freed by swsusp_free() */
> - buffer = get_image_page(GFP_ATOMIC, PG_ANY);
> -
> - if (!buffer)
> - return -ENOMEM;
> -
> - handle->buffer = buffer;
> - } else if (handle->cur == 1) {
> - error = load_header(buffer);
> - if (error)
> - return error;
> -
> - error = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
> - if (error)
> - return error;
> -
> - } else if (handle->cur <= nr_meta_pages + 1) {
> + if (handle->cur < nr_meta_pages) {
> error = unpack_orig_pfns(buffer, &copy_bm);
> if (error)
> return error;
>
> - if (handle->cur == nr_meta_pages + 1) {
> + /* well, this was the last meta page
> + prepare for ordinary pages */
> + if (handle->cur + 1 == nr_meta_pages) {
> error = prepare_image(&orig_bm, &copy_bm);
> if (error)
> return error;
> @@ -2192,16 +2214,22 @@ int snapshot_write_next(struct snapshot_handle *handle)
> if (IS_ERR(handle->buffer))
> return PTR_ERR(handle->buffer);
> }
> + error = PAGE_SIZE;
> } else {
> copy_last_highmem_page();
> - handle->buffer = get_buffer(&orig_bm, &ca);
> - if (IS_ERR(handle->buffer))
> - return PTR_ERR(handle->buffer);
> - if (handle->buffer != buffer)
> - handle->sync_read = 0;
> + /* prepare next unless this was the last one */
> + if (handle->cur + 1 < nr_meta_pages + nr_copy_pages) {
> + handle->buffer = get_buffer(&orig_bm, &ca);
> + if (IS_ERR(handle->buffer))
> + return PTR_ERR(handle->buffer);
> + if (handle->buffer != buffer)
> + handle->sync_read = 0;
> + error = PAGE_SIZE;
> + }
> }
> +
> handle->cur++;
> - return PAGE_SIZE;
> + return error;
> }
>
> /**
> @@ -2216,7 +2244,7 @@ void snapshot_write_finalize(struct snapshot_handle *handle)
> {
> copy_last_highmem_page();
> /* Free only if we have loaded the image entirely */
> - if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages) {
> + if (handle->cur >= nr_meta_pages + nr_copy_pages) {
> memory_bm_free(&orig_bm, PG_UNSAFE_CLEAR);
> free_highmem_data();
> }
> @@ -2225,7 +2253,7 @@ void snapshot_write_finalize(struct snapshot_handle *handle)
> int snapshot_image_loaded(struct snapshot_handle *handle)
> {
> return !(!nr_copy_pages || !last_highmem_page_copied() ||
> - handle->cur <= nr_meta_pages + nr_copy_pages);
> + handle->cur < nr_meta_pages + nr_copy_pages);
> }
>
> #ifdef CONFIG_HIGHMEM
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 9b319f1..f7864bc 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -599,9 +599,6 @@ static int load_image(struct hibernate_io_handle *io_handle,
> bio = NULL;
> do_gettimeofday(&start);
> for ( ; ; ) {
> - error = snapshot_write_next(snapshot);
> - if (error <= 0)
> - break;
> error = hibernate_io_ops->read_page(io_handle,
> data_of(*snapshot), &bio);
> if (error)
> @@ -610,9 +607,13 @@ static int load_image(struct hibernate_io_handle *io_handle,
> error = hib_wait_on_bio_chain(&bio);
> if (error)
> break;
> + error = snapshot_write_next(snapshot);
> + if (error >= 0)
> + nr_pages++;
> + if (error <= 0)
> + break;
> if (!(nr_pages % m))
> printk("\b\b\b\b%3d%%", nr_pages / m);
> - nr_pages++;
> }
> err2 = hib_wait_on_bio_chain(&bio);
> do_gettimeofday(&stop);
> @@ -640,22 +641,17 @@ int swsusp_read(unsigned int *flags_p)
> int error;
> struct hibernate_io_handle *io_handle;
> struct snapshot_handle snapshot;
> - struct swsusp_info *header;
>
> memset(&snapshot, 0, sizeof(struct snapshot_handle));
> - error = snapshot_write_next(&snapshot);
> - if (error < PAGE_SIZE)
> - return error < 0 ? error : -EFAULT;
> - header = (struct swsusp_info *)data_of(snapshot);
> io_handle = hibernate_io_ops->reader_start(flags_p);
> if (IS_ERR(io_handle)) {
> error = PTR_ERR(io_handle);
> goto end;
> }
> + error = snapshot_read_init(io_handle, &snapshot);
> if (!error)
> - error = hibernate_io_ops->read_page(io_handle, header, NULL);
> - if (!error)
> - error = load_image(io_handle, &snapshot, header->pages - 1);
> + error = load_image(io_handle, &snapshot,
> + snapshot_get_image_size() - 1);
> hibernate_io_ops->reader_finish(io_handle);
> end:
> if (!error)
>

2010-06-25 13:56:14

by Rafael J. Wysocki

Subject: Re: [PATCH 8/9] PM / Hibernate: dealign swsusp_info

On Wednesday, June 02, 2010, Jiri Slaby wrote:
> From: Jiri Slaby <[email protected]>
>
> Now there is no need to have swsusp_info page aligned thanks to chunk
> i/o support. We may add more info after it on the very same page.
> Later...

OK

> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> kernel/power/power.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 6e2e796..1fdbfe7 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -12,7 +12,7 @@ struct swsusp_info {
> unsigned long image_pages;
> unsigned long pages;
> unsigned long size;
> -} __attribute__((aligned(PAGE_SIZE)));
> +};
>
> #ifdef CONFIG_HIBERNATION
> #ifdef CONFIG_ARCH_HIBERNATION_HEADER
>

2010-06-25 13:56:59

by Rafael J. Wysocki

Subject: Re: [PATCH 9/9] PM / Hibernate: move non-swap code to image.c

On Wednesday, June 02, 2010, Jiri Slaby wrote:
> Now, when all the swap-independent code was separated, it's time to
> move it into a separate file called image.c. Although snapshot.c is
> image related too, we want to move the snapshot.c code to mm/,
> because it is rather a memory management so that mm people won't
> forget about hibernation. Hence we don't pollute snapshot.c with
> image processing here.

Makes sense.

> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---

Rafael

2010-06-25 14:02:07

by Rafael J. Wysocki

Subject: Re: [linux-pm] [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

On Monday, June 21, 2010, Nigel Cunningham wrote:
> Hi Jiri.
>
> On 22/06/10 01:21, Jiri Slaby wrote:
> > On 06/11/2010 11:46 AM, Nigel Cunningham wrote:
> >> On 02/06/10 18:52, Jiri Slaby wrote:
> >>> I addressed the comments I got on the previous RFC. I left the handles
> >>> in place, the functions in hibernate_io_ops now works on them. Further
> >>> I got rid of the memory barriers and minimized global variables as much
> >>> as possible. Comments welcome.
> >>
> >> I would like to hear the arguments for using these handles. I understand
> >> there may have been some previous discussion, but am unable to find it.
> >>
> >> It seems far more sensible to me to not pass around a handle that
> >> virtually nothing actually uses, and instead store and utilise the state
> >> in the place where it is actually useful. If we had more than one struct
> >> hibernate_io_handle in use at a time, I could understand going this way.
> >> As it stands, however...
> >
> > Hi, it I added that based on this: http://lkml.org/lkml/2010/3/24/458
>
> Okay; thanks.
>
> Looking at Pavel's comment is confusing. The variable you were adding
> isn't "global static" (that's a contradiction in terms anyway). Its
> scope is the file level.
>
> Since the data is only used in this file, your change makes perfect
> sense to me.
>
> Rafael, Pavel: care to discuss this further?

Well. Generally speaking, I like things as they are, except for patches [3/9]
and [4/9].

So, I'd like to take [1-2/9] and [5-9/9]. Jiri, do [6-7/9] need to be changed
substantially in case [3-4/9] are dropped?

Rafael

2010-07-18 12:36:53

by Pavel Machek

Subject: Re: [PATCH 1/9] PM / Hibernate: swap, switch to hibernate_io_handle

On Mon 2010-06-21 17:23:23, Jiri Slaby wrote:
> On 06/10/2010 03:55 PM, Pavel Machek wrote:
> > It would be good if you carried ack-s from previous rounds, so that I
> > don't have to review good patches again...
>
> Hi, previously ACKed patches were merged already. These were much
> rewritten and their original versions were rather NACKed. Otherwise I
> transfer ACKs indeed.

Thanks!

> >> +/**
> >> + * hib_io_handle_alloc - allocate io handle with priv_size for private data
> >> + *
> >> + * @priv_size: the sie to allocate behind hibernate_io_handle for private use
> >> + */
> >> +static inline struct hibernate_io_handle *hib_io_handle_alloc(size_t priv_size)
> >> +{
> >> + struct hibernate_io_handle *ret;
> >> + ret = kzalloc(sizeof(*ret) + priv_size, GFP_KERNEL);
> >> + if (ret)
> >> + ret->priv = ret + 1;
> >
> > Uhuh, why this? Aha, grabbing the pointer to priv_size-sized area at
> > the end of regular struct?
>
> Yes, exactly, any more transparent way to do it?

Normally, I believe void data[]; is added at the end of structure, and
then something like ret->priv = &ret->data; is done...?
Pavel

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2010-07-19 16:42:37

by Jiri Slaby

Subject: Re: [PATCH 6/9] PM / Hibernate: split snapshot_read_next

On 06/25/2010 03:53 PM, Rafael J. Wysocki wrote:
> On Wednesday, June 02, 2010, Jiri Slaby wrote:
>> When writing the snapshot, do the initialization and header write in
>> a separate function. This makes the code more readable and lowers
>> complexity of snapshot_read_next.
>
> This one looks good, but it seems to depend on [3/9] and [4/9]. Does it?

Hi, actually I recalled, that this is exactly the reason why I wrote the
3+4/9 crap. (See below.)

>> Signed-off-by: Jiri Slaby <[email protected]>
>> Cc: "Rafael J. Wysocki" <[email protected]>
>> ---
>> kernel/power/power.h | 2 +
>> kernel/power/snapshot.c | 65 ++++++++++++++++++++++++++++++----------------
>> kernel/power/swap.c | 14 +++-------
>> 3 files changed, 48 insertions(+), 33 deletions(-)
>>
>> diff --git a/kernel/power/power.h b/kernel/power/power.h
>> index 812b66c..ff3f63f 100644
>> --- a/kernel/power/power.h
>> +++ b/kernel/power/power.h
>> @@ -171,6 +171,8 @@ struct hibernate_io_ops {
>>
>> extern unsigned int snapshot_additional_pages(struct zone *zone);
>> extern unsigned long snapshot_get_image_size(void);
>> +extern int snapshot_write_init(struct hibernate_io_handle *io_handle,
>> + struct snapshot_handle *handle);
>> extern int snapshot_read_next(struct snapshot_handle *handle);
>> extern int snapshot_write_next(struct snapshot_handle *handle);
>> extern void snapshot_write_finalize(struct snapshot_handle *handle);
>> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
>> index 25ce010..4600d15 100644
>> --- a/kernel/power/snapshot.c
>> +++ b/kernel/power/snapshot.c
>> @@ -1598,10 +1598,46 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
>> }
>>
>> /**
>> + * snapshot_write_init - initialization before writing the snapshot to
>> + * a backing storage
>> + *
>> + * This function *must* be called before snapshot_read_next to initialize
>> + * @handle and write a header.
>> + *
>> + * @io_handle: handle used when writing the initial info onto storage
>> + * @handle: snapshot handle to init
>> + */
>> +int snapshot_write_init(struct hibernate_io_handle *io_handle,
>> + struct snapshot_handle *handle)
>> +{
>> + int ret;
>> +
>> + /* This makes the buffer be freed by swsusp_free() */
>> + buffer = get_image_page(GFP_ATOMIC, PG_ANY);
>> + if (!buffer)
>> + return -ENOMEM;
>> + init_header(buffer);
>> + ret = hib_buffer_init_write(io_handle);
>> + if (ret)
>> + return ret;
>> + ret = hib_buffer_write(io_handle, buffer, sizeof(struct swsusp_info));

If I don't have 3+4/9, I neither have the io_handle and thus cannot use
chunk writer all over the code.

The possibility is to somehow do init_header and storing pfns in user.c
part separately (the same as before these patches) and then remove it
from the code with user.c some time later. The same will hold for
anything we will use chunk writer for and so on.

Is it OK this way?

thanks,
--
js