Re: [linux-pm] what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

Previous thread: [RFC 08/15] PM / Hibernate: add user module_ops by Jiri Slaby on Tuesday, March 23, 2010 - 9:17 am. (3 messages)

Next thread: [RFC 11/15] PM / Hibernate: add chunk i/o support by Jiri Slaby on Tuesday, March 23, 2010 - 9:17 am. (4 messages)
From: Jiri Slaby
Date: Tuesday, March 23, 2010 - 9:17 am

Switch /dev/snapshot reader to sws_module_ops approach so that we
can transparently rewrite the rest of the snapshot from pages pulling
to their pushing through layers.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/user.c |   73 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/kernel/power/user.c b/kernel/power/user.c
index 748567d..1b5d2e1 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -76,6 +76,7 @@ static DECLARE_BITMAP(to_do_flags, 10);
 #define TODO_FINISH	2
 #define TODO_CLOSED	3
 #define TODO_ERROR	4
+#define TODO_RD_RUNNING	5
 
 static unsigned long user_storage_available(void)
 {
@@ -123,12 +124,41 @@ static int put_user_writer(unsigned int flags, int error)
 	return error;
 }
 
+static int get_user_reader(unsigned int *flags_p)
+{
+	return 0;
+}
+
+static int user_read_page(void *addr, struct bio **bio_chain)
+{
+	int err = 0;
+
+	to_do_buf = addr;
+	wmb();
+	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 put_user_reader(void)
+{
+	return 0;
+}
+
 struct sws_module_ops user_ops = {
 	.storage_available = user_storage_available,
 
 	.get_writer = get_user_writer,
 	.put_writer = put_user_writer,
 	.write_page = user_write_page,
+
+	.get_reader = get_user_reader,
+	.put_reader = put_user_reader,
+	.read_page = user_read_page,
 };
 
 static void snapshot_writer(struct work_struct *work)
@@ -142,6 +172,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) ...
From: Pavel Machek
Date: Wednesday, March 24, 2010 - 10:30 pm

Ok, I guess that now I see what you are doing.... adding interface
layer between /dev/snapshot and core hibernation code.

To recap, 2.6.33 hibernation looks like:

			core hibernation
				/\
			       /  \
			swsusp	  /dev/snapshot
			swap            \
			writing      -------- read/write/ioctl interface
                                          \
					 s2disk

and after your patches, we'd get

			core hibernation
				/\
                            ---------- sws_module_ops interface
			       /  \
			swsusp	  /dev/snapshot
			swap            \
			writing      -------- read/write/ioctl interface
                                          \
					 s2disk

(Right? Did I understand the patches correctly?)

I have some problems with sws_module_ops interface (handcoded locking
is too ugly to live), but it is better than I expected. But there may
be better solution available, one that does not need two interfaces to
maintain (we can't really get rid of userland interface). What about
this?



			core hibernation
				 \
			          \
			      	  /dev/snapshot
		                      / \
			           ---------- read/write/ioctl interface
                                    /     \
				swsusp	 s2disk
				swap
				writing

? That way, we have just one interface, and still keep the advantages
of modularity / defined interfaces.

(You could literary call sys_read() from inside the kernel -- after
set_fs() -- but going to that extreme is probably not neccessary. But
having interface very similar to what /dev/snapshot provides -- with
the same locking rules -- should result in better code.)

									Pavel 

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

From: Nigel Cunningham
Date: Wednesday, March 24, 2010 - 10:42 pm

Hi.

On 25/03/10 16:30, Pavel Machek wrote:

Just picking up on that bracketed part: Can we flag the userland 
interface (and uswsusp) as being planned for eventual removal now... or 
at least agree to work toward that?

I'm asking because if we're going to make a go of getting the in-kernel 
code in much better shape, and we have Rafael, Jiri and I - and you? - 
all pulling in the same direction to improve it, there's going to come a 
point (hopefully not too far away) where uswsusp is just making life too 
difficult, and getting rid of it will be a big help.

Regards,

Nigel
--

From: Nigel Cunningham
Date: Wednesday, March 24, 2010 - 11:12 pm

Hi again.


I realised after sending this that the " - and you? - " was ambiguous. I 
wasn't meaning to suggest that you might pull in a different direction, 
but rather uncertainty as to whether you might help with the effort - 
it's been a while (AFAIR) since you've done any hibernaton patches.

Humble apologies for the ambiguity!

Nigel
--

From: Rafael J. Wysocki
Date: Thursday, March 25, 2010 - 1:14 pm

We're not dropping user space interfaces used by every distro I know of.

Rafael
--

From: Nigel Cunningham
Date: Thursday, March 25, 2010 - 1:21 pm

Hi.


So what's your long term plan then?

Regards,

Nigel
--

From: Rafael J. Wysocki
Date: Thursday, March 25, 2010 - 1:29 pm

First, improve the in-kernel thing, second, switch people to it, _then_ remove
the s2disk interface (after we're reasonably sure it's not used by any major
distro) and _finally_ simplify things after it's been removed.

Does that sound reasonable?

Rafael
--

From: Nigel Cunningham
Date: Thursday, March 25, 2010 - 1:35 pm

Hi.


Well, that's pretty much what I was thinking too - improve then remove. 
I was just suggesting that we flag now that this is our plan, so it 
doesn't come as a surprise to anyone later and we can proceed more 
quickly than might otherwise be the case. I'm imagining it won't take 
long to get uswsusp features into the kernel code.

Regards,

Nigel
--

From: Rafael J. Wysocki
Date: Thursday, March 25, 2010 - 2:13 pm

I think it's too early for that at this point.

Rafael
--

From: Pavel Machek
Date: Thursday, March 25, 2010 - 2:26 pm

I'd really prefer to keep s2disk interface. It allows advanced stuff
like internet suspend resume (yep someone is doing it), crazy stuff
like multiple resumes from same image for fast booting (yep, some
embedded people are doing that) and might-be-useful stuff like
s2both...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--

From: Nigel Cunningham
Date: Thursday, March 25, 2010 - 2:49 pm

Hi.


Neither of those are impossible with in-kernel code, so I'd argue that 
there's no need to keep the s2disk interface long-term. Userspace 
helpers might be necessary for the first one (to manage the network 
interface), but I already have multiple-resumes-from-the-same-image 
support in TuxOnice (and more 'crazy stuff' like support for resuming a 
different image after writing one - that can be used to switch to an 
initramfs containing the binaries needed to power down a UPS).

Regards,

Nigel
--

From: Pavel Machek
Date: Thursday, April 1, 2010 - 11:36 pm

The fact that it can be done in kernel -- anything can -- but whether
it should. I'd very much like to keep the 'crazy stuff' in userspace.
									Pavel

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

From: Rafael J. Wysocki
Date: Friday, April 2, 2010 - 10:03 am

On Friday 02 April 2010, Pavel Machek wrote:

IMO the question really boils down to what's the benefit of doing these things
in the kernel vs doing them in the user space.

If doing them in the kernel reduces the overall complexity of design (including
the user space interface involved etc.) I don't really see why not to.

Rafael
--

From: Rafael J. Wysocki
Date: Thursday, March 25, 2010 - 3:21 pm

The user space interface does things that the in-kernel one doesn't really care
for, so I don't think that would be a good thing to do.  I admit it's a bit
like this right now (snapshot_[read|write]_next() do some things to satisfy
the user space interface's needs), but I don't really think it should go any
further than that.

Moreover, the Jiri's approach allows us to handle other types of storage as
well as swap using uniform interface.

Rafael
--

Previous thread: [RFC 08/15] PM / Hibernate: add user module_ops by Jiri Slaby on Tuesday, March 23, 2010 - 9:17 am. (3 messages)

Next thread: [RFC 11/15] PM / Hibernate: add chunk i/o support by Jiri Slaby on Tuesday, March 23, 2010 - 9:17 am. (4 messages)