Freezer: Try to handle killable tasks
in [Kernel]
|
Prev: Freezer: Introduce PF_FREEZER_NOSIG
Next: [PATCH] atmel_lcdfb: fix initialization of a pre-allocated framebuffer
From: Pavel Machek on 7 May 2008 05:50 On Wed 2008-05-07 00:07:55, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rjw(a)sisk.pl> > > The introduction of TASK_KILLABLE allows the freezer to work in some situation > that it could not handle before. > > Make the freezer handle killable tasks and add try_to_freeze() in some places > where it is safe to freeze a (killable) task. Introduce the > wait_event_killable_freezable() macro to be used wherever the freezing of > a waiting killable task is desirable. > > Signed-off-by: Rafael J. Wysocki <rjw(a)sisk.pl> > --- > fs/nfs/inode.c | 2 ++ > fs/nfs/nfs3proc.c | 2 ++ > fs/nfs/nfs4proc.c | 4 ++++ > fs/nfs/pagelist.c | 8 ++++++-- > fs/smbfs/request.c | 2 ++ > include/linux/freezer.h | 20 +++++++++++++++++--- > kernel/mutex.c | 3 +++ > kernel/power/process.c | 6 ++++-- > kernel/sched.c | 2 ++ > net/sunrpc/sched.c | 2 ++ > 10 files changed, 44 insertions(+), 7 deletions(-) > > Index: linux-2.6/fs/nfs/nfs3proc.c > =================================================================== > --- linux-2.6.orig/fs/nfs/nfs3proc.c > +++ linux-2.6/fs/nfs/nfs3proc.c > @@ -17,6 +17,7 @@ > #include <linux/nfs_page.h> > #include <linux/lockd/bind.h> > #include <linux/nfs_mount.h> > +#include <linux/freezer.h> > > #include "iostat.h" > #include "internal.h" > @@ -33,6 +34,7 @@ nfs3_rpc_wrapper(struct rpc_clnt *clnt, > if (res != -EJUKEBOX) > break; > schedule_timeout_killable(NFS_JUKEBOX_RETRY_TIME); > + try_to_freeze(); > res = -ERESTARTSYS; > } while (!fatal_signal_pending(current)); > return res; > Index: linux-2.6/fs/nfs/nfs4proc.c > =================================================================== > --- linux-2.6.orig/fs/nfs/nfs4proc.c > +++ linux-2.6/fs/nfs/nfs4proc.c > @@ -48,6 +48,7 @@ > #include <linux/smp_lock.h> > #include <linux/namei.h> > #include <linux/mount.h> > +#include <linux/freezer.h> > > #include "nfs4_fs.h" > #include "delegation.h" > @@ -2788,6 +2789,7 @@ static int nfs4_wait_bit_killable(void * > if (fatal_signal_pending(current)) > return -ERESTARTSYS; > schedule(); > + try_to_freeze(); > return 0; > } I'd say try_to_freeze() belongs close to the other signal check, i.e. before schedule? > @@ -180,10 +182,12 @@ static int nfs_wait_bit_killable(void *w > { > int ret = 0; > > - if (fatal_signal_pending(current)) > + if (fatal_signal_pending(current)) { > ret = -ERESTARTSYS; > - else > + } else { > schedule(); > + try_to_freeze(); > + } Same here? > @@ -109,6 +110,7 @@ struct smb_request *smb_alloc_request(st > return ERR_PTR(-ERESTARTSYS); > current->policy = SCHED_YIELD; > schedule(); > + try_to_freeze(); And here? > @@ -182,6 +183,8 @@ __mutex_lock_common(struct mutex *lock, > /* didnt get the lock, go to sleep: */ > spin_unlock_mutex(&lock->wait_lock, flags); > schedule(); > + if (state == TASK_KILLABLE) > + try_to_freeze(); > spin_lock_mutex(&lock->wait_lock, flags); > } > I'm not comfortable with this one. Can the task be killable, but still hold some _other_ mutex? (and then release it only if it actually gets the signal?) > Index: linux-2.6/kernel/sched.c > =================================================================== > --- linux-2.6.orig/kernel/sched.c > +++ linux-2.6/kernel/sched.c > @@ -4769,6 +4769,8 @@ do_wait_for_common(struct completion *x, > __set_current_state(state); > spin_unlock_irq(&x->wait.lock); > timeout = schedule_timeout(timeout); > + if (state == TASK_KILLABLE) > + try_to_freeze(); > spin_lock_irq(&x->wait.lock); > if (!timeout) { > __remove_wait_queue(&x->wait, &wait); Same here. I can't see why we could not be holding mutexes. > @@ -227,6 +228,7 @@ static int rpc_wait_bit_killable(void *w > if (fatal_signal_pending(current)) > return -ERESTARTSYS; > schedule(); > + try_to_freeze(); > return 0; > } Swap? > @@ -426,6 +427,7 @@ static int nfs_wait_schedule(void *word) > if (signal_pending(current)) > return -ERESTARTSYS; > schedule(); > + try_to_freeze(); > return 0; > } > Swap? 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/
From: Matthew Wilcox on 7 May 2008 10:00 On Wed, May 07, 2008 at 11:41:50AM +0200, Pavel Machek wrote: > > @@ -182,6 +183,8 @@ __mutex_lock_common(struct mutex *lock, > > /* didnt get the lock, go to sleep: */ > > spin_unlock_mutex(&lock->wait_lock, flags); > > schedule(); > > + if (state == TASK_KILLABLE) > > + try_to_freeze(); > > spin_lock_mutex(&lock->wait_lock, flags); > > } > > > > I'm not comfortable with this one. Can the task be killable, but still > hold some _other_ mutex? (and then release it only if it actually gets > the signal?) Yes, that's exactly what's supposed to happen. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." -- 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: Matthew Wilcox on 7 May 2008 10:00 On Wed, May 07, 2008 at 12:07:55AM +0200, Rafael J. Wysocki wrote: > The introduction of TASK_KILLABLE allows the freezer to work in some situation > that it could not handle before. > > Make the freezer handle killable tasks and add try_to_freeze() in some places > where it is safe to freeze a (killable) task. Introduce the > wait_event_killable_freezable() macro to be used wherever the freezing of > a waiting killable task is desirable. Why do you say that TASK_KILLABLE allows the freezer to work in some situations where it couldn't before? If something's using one of the killable functions, it means that it knows how to clean up and unwind gracefully if the task receives a fatal signal. I don't understand what connection there is to the freezer. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." -- 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 7 May 2008 14:40 On Wednesday, 7 of May 2008, Matthew Wilcox wrote: > On Wed, May 07, 2008 at 11:41:50AM +0200, Pavel Machek wrote: > > > @@ -182,6 +183,8 @@ __mutex_lock_common(struct mutex *lock, > > > /* didnt get the lock, go to sleep: */ > > > spin_unlock_mutex(&lock->wait_lock, flags); > > > schedule(); > > > + if (state == TASK_KILLABLE) > > > + try_to_freeze(); > > > spin_lock_mutex(&lock->wait_lock, flags); > > > } > > > > > > > I'm not comfortable with this one. Can the task be killable, but still > > hold some _other_ mutex? (and then release it only if it actually gets > > the signal?) > > Yes, that's exactly what's supposed to happen. The question, though, is whether there is a driver that will try to lock this mutex in its .suspend() or .resume() callback. If there is one, TASK_KILLABLE won't help the freezer indeed. Thanks, Rafael -- 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 7 May 2008 14:50
On Wednesday, 7 of May 2008, Matthew Wilcox wrote: > On Wed, May 07, 2008 at 12:07:55AM +0200, Rafael J. Wysocki wrote: > > The introduction of TASK_KILLABLE allows the freezer to work in some situation > > that it could not handle before. > > > > Make the freezer handle killable tasks and add try_to_freeze() in some places > > where it is safe to freeze a (killable) task. Introduce the > > wait_event_killable_freezable() macro to be used wherever the freezing of > > a waiting killable task is desirable. > > Why do you say that TASK_KILLABLE allows the freezer to work in some > situations where it couldn't before? If something's using one of the > killable functions, it means that it knows how to clean up and unwind > gracefully if the task receives a fatal signal. I don't understand what > connection there is to the freezer. The reason why we don't freeze uninterruptible tasks is that we don't know why they are in that state. If one of tasks is uninterruptible for a relatively long time, that may indicate a non-recoverable error making it dangerous to put the system into a sleep state. If the task is killable, though, the situation is recoverable. Thanks, Rafael -- 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/ |