Messages in this thread Patch in this message | | | From | "Rafael J. Wysocki" <> | Subject | Re: [linux-pm] PM: cannot hibernate -- BUG at kernel/workqueue.c:3659 | Date | Thu, 26 Jan 2012 00:51:44 +0100 |
| |
Hi,
On Wednesday, January 25, 2012, Srivatsa S. Bhat wrote: > > > > > Commit 2aede851 (PM / Hibernate: Freeze kernel threads after preallocating > > memory) split up the freezing phase and moved the freezing of kernel threads > > to hibernation_snapshot() function. > > (This is a pretty old commit btw, I think it came in sometime around 3.2). > > > > And there is a BUG_ON() in freeze_workqueues_begin() which complains if > > this function is called when the kernel threads are already frozen. > > > > Now, coming to the userspace utility: > > > >> In suspend.c inside the suspend-utils userspace package, I see a loop such > >> as: > >> > >> error = freeze(snapshot_fd); > >> ... > >> attempts = 2; > >> do { > >> if (set_image_size(snapshot_fd, image_size)) { > >> error = errno; > >> break; > >> } > >> if (atomic_snapshot(snapshot_fd, &in_suspend)) { > > ^^^ > > This ends up in the call to hibernation_snapshot(). > > > >> error = errno; > >> break; > >> } > >> if (!in_suspend) { > >> /* first unblank the console, see console_codes(4) */ > >> printf("\e[13]"); > >> printf("%s: returned to userspace\n", my_name); > >> free_snapshot(snapshot_fd); > >> break; > >> } > >> > >> error = write_image(snapshot_fd, resume_fd, -1); > >> if (error) { > > > > Since swap space is limited, we come here. > > > >> free_swap_pages(snapshot_fd); > >> free_snapshot(snapshot_fd); > > ^^^ > This function makes the ioctl call SNAPSHOT_FREE. > > >> image_size = 0; > >> error = -error; > >> if (error != ENOSPC) > >> break; > > > > > > > > > > And because of the above 'if' condition, we don't break here. > > IOW, we will run the loop a second time. > > > >> } else { > >> splash.progress(100); > >> #ifdef CONFIG_BOTH > >> if (s2ram_kms || s2ram) { > >> /* If we die (and allow system to continue) > >> * between now and reset_signature(), very bad > >> * things will happen. */ > >> error = suspend_to_ram(snapshot_fd); > >> if (error) > >> goto Shutdown; > >> reset_signature(resume_fd); > >> free_swap_pages(snapshot_fd); > >> free_snapshot(snapshot_fd); > >> if (!s2ram_kms) > >> s2ram_resume(); > >> goto Unfreeze; > >> } > >> Shutdown: > >> #endif > >> close(resume_fd); > >> suspend_shutdown(snapshot_fd); > >> } > >> } while (--attempts); > >> > >> ... > >> Unfreeze: > >> unfreeze(snapshot_fd); > >> > >> > > > > > > So, since the loop is run a second time if not enough space was available, > > we end up calling hibernation_snapshot(), IOW freeze_workqueues_begin() a > > second time - and this is what makes the BUG_ON() to fire! > > > > > SNAPSHOT_CREATE_IMAGE has a check for data->ready such as: > > if (data->mode != O_RDONLY || !data->frozen || data->ready) { > error = -EPERM; > break; > } > > data->ready would be set to 1 only under SNAPSHOT_CREATE_IMAGE. However, > SNAPSHOT_FREE (invoked at the place shown above) will reset the value to 0. > This makes it possible for hibernation_snapshot() and hence > freeze_workqueues_begin() to be called a second time, which is unfortunate.
Yes, I obviously forgot about that code path when I was working on the commit that introduced the problem. :-(
Thanks a lot for the great analysis, it's really helpful!
> And actually, the patch I posted in my previous mail is not really the right > long-term fix, though it might fix the particular issue that Jiri is facing.. > > Because, allowing hibernation_snapshot() to get called a second time while > kernel threads are still frozen brings us to the same situation that commit > 2aede851 (PM / Hibernate: Freeze kernel threads after preallocating memory) > tried to prevent! IOW, a call to hibernate_preallocate_memory() would be > done inside hibernation_snapshot(), when kernel threads are frozen.. which > is known to break XFS, to give one example as mentioned in the changelog > of the above commit.
That's exactly right.
> So, the right way to fix this IMHO, would be to split up thaw_processes() > just like freezing phase: > > /* freezes or thaws user space processes */ > freeze_processes() - thaw_processes() > > /* freezes or thaws kernel threads */ > freeze_kernel_threads() - thaw_kernel_threads() > > We have to insert this thaw_kernel_threads() at appropriate places in such a > way as to not require another ioctl if possible... Then things would be > more symmetric (and hence more easy to understand) and we can avoid getting > into strange situations as discussed here. > > But before we venture into that, it would be good to know if the patch posted > in the previous mail fixes the particular problem reported in this thread, > atleast just to see if there are other problems lurking that we aren't aware > of yet..
Jiri has already said that the patch works.
I think we could avoid the issue entirely by introducing thaw_kernel_threads and making SNAPSHOT_FREE call it. No other changes should be necessary.
IOW, Jiri, does the patch below help?
[BTW, the freeze_tasks()'s kerneldoc seems to be outdated. Tejun?]
--- include/linux/freezer.h | 2 ++ kernel/power/process.c | 19 +++++++++++++++++++ kernel/power/user.c | 1 + 3 files changed, 22 insertions(+)
Index: linux/include/linux/freezer.h =================================================================== --- linux.orig/include/linux/freezer.h +++ linux/include/linux/freezer.h @@ -39,6 +39,7 @@ extern bool __refrigerator(bool check_kt extern int freeze_processes(void); extern int freeze_kernel_threads(void); extern void thaw_processes(void); +extern void thaw_kernel_threads(void); static inline bool try_to_freeze(void) { @@ -174,6 +175,7 @@ static inline bool __refrigerator(bool c static inline int freeze_processes(void) { return -ENOSYS; } static inline int freeze_kernel_threads(void) { return -ENOSYS; } static inline void thaw_processes(void) {} +static inline void thaw_kernel_threads(void) {} static inline bool try_to_freeze(void) { return false; } Index: linux/kernel/power/process.c =================================================================== --- linux.orig/kernel/power/process.c +++ linux/kernel/power/process.c @@ -188,3 +188,22 @@ void thaw_processes(void) printk("done.\n"); } +void thaw_kernel_threads(void) +{ + struct task_struct *g, *p; + + pm_nosig_freezing = false; + printk("Restarting kernel threads ... "); + + thaw_workqueues(); + + read_lock(&tasklist_lock); + do_each_thread(g, p) { + if (p->flags & (PF_KTHREAD | PF_WQ_WORKER)) + __thaw_task(p); + } while_each_thread(g, p); + read_unlock(&tasklist_lock); + + schedule(); + printk("done.\n"); +} Index: linux/kernel/power/user.c =================================================================== --- linux.orig/kernel/power/user.c +++ linux/kernel/power/user.c @@ -274,6 +274,7 @@ static long snapshot_ioctl(struct file * swsusp_free(); memset(&data->handle, 0, sizeof(struct snapshot_handle)); data->ready = 0; + thaw_kernel_threads(); break; case SNAPSHOT_PREF_IMAGE_SIZE:
| |