swsusp: code separation continued

This patch makes only the functions in swsusp.c call functions in snapshot.c and not both ways.  Basically, it moves the code without changing its functionality.

Hi Rafael,

Quoted 3 lines · Rafael J. Wysocki

This patch makes only the functions in swsusp.c call functions in snapshot.c and not both ways.  Basically, it moves the code without changing its functionality.

This is not quite true.

kernel/power/snapshot.c line 126

Quoted 5 lines

#else

kernel/power/snapshot.c +1 -1

-static int save_highmem(void) { return 0; }
+int save_highmem(void) { return 0; }
 int restore_highmem(void) { return 0; }
 #endif /* CONFIG_HIGHMEM */

Here you change code, which will be optimized completely away to an empty function, which bloats the kernel.

Please put these two functions into a local header like this:

#ifdef CONFIG_HIGHMEM
int save_highmem(void);
int restore_highmem(void);
#else
static inline int save_highmem(void) { return 0; } static inline int restore_highmem(void) { return 0; } #endif

That way no having no highmem means, this code is not used at all and everything using the return code and expecting != 0 is going to be optimized away.

I think everyone CCed will agree here :-)

This patch moves the snapshot-handling functions remaining in swsusp.c to snapshot.c (ie. it moves the code without changing the functionality).

Hi!

Quoted 3 lines

This patch moves the snapshot-handling functions remaining in swsusp.c to snapshot.c (ie. it moves the code without changing the functionality).

I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where it is, and define "must not collide" as a part of interface.

That will keep snapshot.c smaller/simpler, and I plan to eventually put responsibility for relocation to userspace.

That should simplify error handling at least: data structures needed for relocation can be kept in userspace memory,

and therefore we do not risk memory leak in case something goes wrong.

Quoted 5 lines

Quoted 3 lines

This patch moves the snapshot-handling functions remaining in swsusp.c to snapshot.c (ie. it moves the code without changing the functionality).

I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where it is, and define "must not collide" as a part of interface.

That's doable, but frankly I don't like the idea.

Quoted 2 lines

That will keep snapshot.c smaller/simpler, and I plan to eventually put responsibility for relocation to userspace.

Please note that the relocating code uses the page flags to mark the allocated pages as well as to avoid the pages that should not be used. In my opinion no userspace process should be allowed to fiddle with the page flags.

Moreover, get_safe_page() is called directly by the arch code on x86-64, so it has to stay in the kernel and hence it should be in snapshot.c. OTOH the relocating code is nothing more than "if the page is not safe, use get_safe_page() to allocate one" kind of thing, so I don't see a point in taking it out of the kernel (in the future) too.

Quoted 2 lines

That should simplify error handling at least: data structures needed for relocation can be kept in userspace memory,

Well, after the patches that are already in -mm we don't use any additional data structures for this purpose, so that's not a problem, I think. ;-)

Hi!

Quoted 4 lines

Please note that the relocating code uses the page flags to mark the allocated pages as well as to avoid the pages that should not be used. In my opinion no userspace process should be allowed to fiddle with the page flags.

Of course, userspace would have to use separate data structure. [Hash table?]

Quoted 5 lines

Moreover, get_safe_page() is called directly by the arch code on x86-64, so it has to stay in the kernel and hence it should be in snapshot.c. OTOH the relocating code is nothing more than "if the page is not safe, use get_safe_page() to allocate one" kind of thing, so I don't see a point in taking it out of the kernel (in the future) too.

Well... for resume. If userspace does the allocation, it is:

userspace reads image
userspace relocates it
sys_atomic_restore(image)
if something goes wrong, userspace is clearly responsible for freeing it.

How would you propose kernel<->user interface?

userspace reads pagedir sys_these_pages_are_forbidden(pagedir)
userspace reads rest
sys_atomic_restore(image)
if something goes wrong, userspace must dealocate pages _and_ clear forbidden flags?

Quoted 6 lines

Quoted 2 lines

That should simplify error handling at least: data structures needed for relocation can be kept in userspace memory,

Well, after the patches that are already in -mm we don't use any additional data structures for this purpose, so that's not a problem, I think. ;-)

Until someone will want to get page flag bits back ;-), ok.

Quoted 6 lines

Quoted 4 lines

Please note that the relocating code uses the page flags to mark the allocated pages as well as to avoid the pages that should not be used. In my opinion no userspace process should be allowed to fiddle with the page flags.

Of course, userspace would have to use separate data structure. [Hash table?]

IMO a bitmap could be used. Anyway in that case the x86-64 arch code would need to have access either to this structure or to the image metadata, because it must figure out which pages are not safe. I don't see any simple way of making this work ...

Quoted 14 lines

Quoted 5 lines

Moreover, get_safe_page() is called directly by the arch code on x86-64, so it has to stay in the kernel and hence it should be in snapshot.c. OTOH the relocating code is nothing more than "if the page is not safe, use get_safe_page() to allocate one" kind of thing, so I don't see a point in taking it out of the kernel (in the future) too.

Well... for resume. If userspace does the allocation, it is:

userspace reads image
userspace relocates it
sys_atomic_restore(image)
if something goes wrong, userspace is clearly responsible for freeing it.

Quoted 8 lines

How would you propose kernel<->user interface?

userspace reads pagedir sys_these_pages_are_forbidden(pagedir)
userspace reads rest
sys_atomic_restore(image)
if something goes wrong, userspace must dealocate pages _and_ clear forbidden flags?

Well, you have taken these things out of context. Namely, the userspace process cannot freeze the other tasks, suspend devices etc., so it has to

call the kernel for these purposes anyway. Of course if something goes wrong it has to call the kernel to revert these steps too. Similarly it can call the kernel to allocate the image memory and to free it in case something's wrong. For example, if the userspace initiates the resume:

- if (image not found)
exit
- sys_freeze_processes /* this one will be tricky ;-) */

- sys_create_pagedir

- while (image data) {
	sys_put_this_stuff_where_appropriate(image data);
	/* Here the kernel will do the relocation etc. if necessary */
	if (something's wrong)
		goto Cleanup; }

- sys_atomic_restore /* suspend devices, disable IRQs, restore */

Cleanup: /* certainly something's gone wrong */
- sys_destroy_pagedir /* that's it */
- sys_resume_devices

Hi!

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Please note that the relocating code uses the page flags to mark the allocated pages as well as to avoid the pages that should not be used. In my opinion no userspace process should be allowed to fiddle with the page flags.

Quoted 6 lines

Quoted 1 line

Of course, userspace would have to use separate data structure. [Hash table?]

IMO a bitmap could be used. Anyway in that case the x86-64 arch code would need to have access either to this structure or to the image metadata, because it must figure out which pages are not safe. I don't see any simple way of making this work ...

Can you elaborate? resume is certainly going to get list of pbes...

Quoted 4 lines

Well, you have taken these things out of context. Namely, the userspace process cannot freeze the other tasks, suspend devices etc., so it has to

Yes, process freezing probably needs to be separate. Suspending devices can well be part of atomic_snapshot operation; userspace does not need to care.

Quoted 5 lines

call the kernel for these purposes anyway. Of course if something goes wrong it has to call the kernel to revert these steps too. Similarly it can call the kernel to allocate the image memory and to free it in case something's wrong. For example, if the userspace initiates the resume:

Quoted 3 lines

- if (image not found)
exit
- sys_freeze_processes /* this one will be tricky ;-) */

Why, I have it implemented? Just do not freeze the process calling you.

Quoted 1 line

- sys_create_pagedir

Ugly...

Quoted 6 lines

- while (image data) {
	sys_put_this_stuff_where_appropriate(image data);
	/* Here the kernel will do the relocation etc. if necessary */
	if (something's wrong)
		goto Cleanup; }

- sys_atomic_restore /* suspend devices, disable IRQs, restore */

Exactly. I'd like to go a

Quoted 3 lines

Cleanup: /* certainly something's gone wrong */
- sys_destroy_pagedir /* that's it */
- sys_resume_devices

You should not need to do this one. resuming devices is going to be integrated in atomic_restore, because suspending devices is there, too.

Here's how it looks... additionaly, I have ioctl for getting one usable page. It is true that I did not solve error paths, yet; I'll certainly need some way to free memory, too.

Quoted 1 line

Can you elaborate? resume is certainly going to get list of pbes...

OK
On x86-64 we have to allocate a few safe pages to put the temporary page tables on them. In principle I can imagine the following code for this:

do {
	get a page;
	walk the list of pbes to verify that the page is safe;
	if (the page is not safe)
		keep track of it;
} while (the page is not safe)

but I'd rather not like to propose Andi to merge it. ;-) Currently the x86-64 arch code uses the same method of marking non-safe pages that is used by the rest of swsusp for efficiency and I think it should stay this
way.

Quoted 3 lines

Quoted 1 line

- sys_create_pagedir

Ugly...

Oh, it can be done on-the-fly in sys_put_this_stuff_where_appropriate(image data) (at the expense of one redundant check per call).

Quoted 6 lines

Quoted 3 lines

Cleanup: /* certainly something's gone wrong */
- sys_destroy_pagedir /* that's it */
- sys_resume_devices

You should not need to do this one. resuming devices is going to be integrated in atomic_restore, because suspending devices is there, too.

Yes, but I need to thaw processes anyway, so I can release memory as well. OTOH, if sys_atomic_restore fails because of the lack of memory, the memory should be freed _before_ resuming devices, since otherwise subsequent failures are almost certain to appear (I've seen what happens in that case). Now, if the memory is allocated by the kernel, I can easily put an emergency memory-freeing call in sys_atomic_restore (in that case sys_destroy_pagedir will be redundant, but so what?).

Quoted 3 lines

Here's how it looks... additionaly, I have ioctl for getting one usable page. It is true that I did not solve error paths, yet; I'll certainly need some way to free memory, too.

IMHO, these are important issues.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

- sys_create_pagedir

Ugly...

Oh, it can be done on-the-fly in sys_put_this_stuff_where_appropriate(image data) (at the expense of one redundant check per call).

Yes, but it is still ugly, as you keep some context across the syscalls.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

Cleanup: /* certainly something's gone wrong */
- sys_destroy_pagedir /* that's it */
- sys_resume_devices

You should not need to do this one. resuming devices is going to be integrated in atomic_restore, because suspending devices is there, too.

Quoted 7 lines

Yes, but I need to thaw processes anyway, so I can release memory as well. OTOH, if sys_atomic_restore fails because of the lack of memory, the memory should be freed _before_ resuming devices, since otherwise subsequent failures are almost certain to appear (I've seen what happens in that case). Now, if the memory is allocated by the kernel, I can easily put an emergency memory-freeing call in sys_atomic_restore (in that case sys_destroy_pagedir will be redundant, but so what?).

Ugh, I'd say "don't care about this one too much". If resume is failing, we have bad problems anyway.

Quoted 6 lines

Quoted 3 lines

Oh, it can be done on-the-fly in sys_put_this_stuff_where_appropriate(image data) (at the expense of one redundant check per call).

Yes, but it is still ugly, as you keep some context across the syscalls.

That depends on how you implement the interface. If you insist on using ioctls then yes, it's ugly. However, if it is a file in sysfs, for example, then you have well-defined open(), close(), read() and write() operations and it is assumed you will keep some context accross eg. write()s.

Hi!

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

Oh, it can be done on-the-fly in sys_put_this_stuff_where_appropriate(image data) (at the expense of one redundant check per call).

Yes, but it is still ugly, as you keep some context across the syscalls.

Quoted 4 lines

That depends on how you implement the interface. If you insist on using ioctls then yes, it's ugly. However, if it is a file in sysfs, for example, then you have well-defined open(), close(), read() and write() operations and it is assumed you will keep some context accross eg. write()s.

I was trying to keep kernel code simple. Yes, if we do it sysfs based, that's probably not a problem. I'm not sure if nice sysfs interface can be done without excessive ammount of code.

Quoted 8 lines

Quoted 4 lines

That depends on how you implement the interface. If you insist on using ioctls then yes, it's ugly. However, if it is a file in sysfs, for example, then you have well-defined open(), close(), read() and write() operations and it is assumed you will keep some context accross eg. write()s.

I was trying to keep kernel code simple. Yes, if we do it sysfs based, that's probably not a problem. I'm not sure if nice sysfs interface can be done without excessive ammount of code.

I'm not sure either, but I'm going to try.

Still first I'd like to make swsusp free only as much memory as needed and not as much as possible which should improve its performance quite a bit.

Hi!

Quoted 3 lines

This patch moves the snapshot-handling functions remaining in swsusp.c to snapshot.c (ie. it moves the code without changing the functionality).

I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where it is, and define "must not collide" as a part of interface. That will keep snapshot.c smaller/simpler,

Quoted 7 lines

Quoted 3 lines

This patch moves the snapshot-handling functions remaining in swsusp.c to snapshot.c (ie. it moves the code without changing the functionality).

I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where it is, and define "must not collide" as a part of interface. That will keep snapshot.c smaller/simpler,

Speaking of simplifications and having seen your code I hope you will agree with the appended patch against vanilla 2.6.14-git3 (it reduces the duplication of code, and replaces swsusp_pagedir_relocate with a simpler mechanism).

kernel/power/swsusp.c +1 -1

@@ -997,20 +870,22 @@
 	int error = 0;
 	struct pbe *p;
 
-	if (!(p = alloc_pagedir(nr_copy_pages)))
+	if (!(p = alloc_pagedir(nr_copy_pages, 0)))
 		return -ENOMEM;

Hi!

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

This patch moves the snapshot-handling functions remaining in swsusp.c to snapshot.c (ie. it moves the code without changing the functionality).

I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where it is, and define "must not collide" as a part of interface. That will keep snapshot.c smaller/simpler,

Speaking of simplifications and having seen your code I hope you will agree with the appended patch against vanilla 2.6.14-git3 (it reduces the duplication of code, and replaces swsusp_pagedir_relocate with a simpler mechanism).

...and also moves stuff around in a way

a) I don't like

and

b) is almost impossible to review

:-). Can you keep "relocate" code in swsusp.c, just making it simpler?

Quoted 9 lines

kernel/power/swsusp.c +1 -1

@@ -997,20 +870,22 @@
 	int error = 0;
 	struct pbe *p;
 
-	if (!(p = alloc_pagedir(nr_copy_pages)))
+	if (!(p = alloc_pagedir(nr_copy_pages, 0)))
 		return -ENOMEM;
 

kernel/power/swsusp.c line 881

Quoted 9 lines

kernel/power/swsusp.c +2 -3

 	if ((error = read_pagedir(p)))
 		return error;
-
 	create_pbe_list(p, nr_copy_pages);
-
-	if (!(pagedir_nosave = swsusp_pagedir_relocate(p)))
+	mark_unsafe_pages(p);
+	if (!(pagedir_nosave = alloc_pagedir(nr_copy_pages, 1)))
 		return -ENOMEM;

Okay, this is probably better approach than copying pagedir around...

Quoted 6 lines

Quoted 3 lines

Speaking of simplifications and having seen your code I hope you will agree with the appended patch against vanilla 2.6.14-git3 (it reduces the duplication of code, and replaces swsusp_pagedir_relocate with a simpler mechanism).

...and also moves stuff around in a way

Quoted 5 lines

a) I don't like

and

b) is almost impossible to review

OK, I'll try to split it into two patches to make it cleaner.

The appended patch simplifies the relocation of the page backup list (aka pagedir) during resume.

Quoted 3 lines

The appended patch simplifies the relocation of the page backup list (aka pagedir) during resume.

ACK and thanks for patience.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Speaking of simplifications and having seen your code I hope you will agree with the appended patch against vanilla 2.6.14-git3 (it reduces the duplication of code, and replaces swsusp_pagedir_relocate with a simpler mechanism).

...and also moves stuff around in a way

Quoted 7 lines

Quoted 5 lines

a) I don't like

and

b) is almost impossible to review

OK, I'll try to split it into two patches to make it cleaner.

The first patch is appended, the next one will be in the reply to this message.

The changes made by the appended patch are necessary for the relocation simplification in the next patch. Still, the changes allow us to drop check_pagedir() and make get_safe_page() be a one-line wrapper around alloc_image_page() (get_safe_page() goes to snapshot.c, because alloc_image_page() is static and it does not make sense to export it).