Prev: [PATCHv2 09/11] OMAP2: Devkit8000: Using gpio_is_valid macro
Next: [PATCHv2 07/11] OMAP2: Devkit8000: Remove en-/disable for tv panel
From: Nigel Cunningham on 2 Jun 2010 07:50
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Pavel Machek on 10 Jun 2010 10:00
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 <jslaby(a)suse.cz>
> Cc: "Rafael J. Wysocki" <rjw(a)sisk.pl>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Slaby on 21 Jun 2010 11:30
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Rafael J. Wysocki on 24 Jun 2010 11:30
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 <jslaby(a)suse.cz>
> Cc: "Rafael J. Wysocki" <rjw(a)sisk.pl>
> ---
> 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");
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Pavel Machek on 18 Jul 2010 08:40
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [PATCHv2 09/11] OMAP2: Devkit8000: Using gpio_is_valid macro
Next: [PATCHv2 07/11] OMAP2: Devkit8000: Remove en-/disable for tv panel