Re: [PATCH 14/16] freezer: make freezing() test freeze conditions ineffect instead of TIF_FREEZE
From: Paul Menage
Date: Fri Aug 19 2011 - 11:43:50 EST
On Fri, Aug 19, 2011 at 7:16 AM, Tejun Heo <tj@xxxxxxxxxx> wrote:
> Using TIF_FREEZE for freezing worked when there was only single
> freezing condition (the system-wide one); however, now there also is
> the cgroup_freezer and single bit flag is getting clumsy.
> thaw_processes() is already testing whether cgroup freezing in in
> effect to avoid thawing tasks which were frozen by both system and
> cgroup freezers.
>
> This is racy (nothing prevents race against cgroup freezing) and
> fragile. A much simpler way is to test actual freeze conditions from
> freezing() - ie. directly test whether system or cgroup freezing is in
> effect.
>
> This patch adds variables to indicate whether and what type of system
> freezing is in effect and reimplements freezing() such that it
> directly tests whether any of the two freezing conditions is active
> and the task should freeze. On fast path, freezing() is still very
> cheap, it only tests system_freezing_cnt.
>
> This makes the clumsy dancing aroung TIF_FREEZE unnecessary and
> freeze/thaw operations more usual - updating state variables for the
> new state and nudging target tasks so that they notice the new state
> and comply. As long as the nudging happens after state update, it's
> race-free.
>
> * This allows use of freezing() in freeze_task(). Replace the open
> coded tests with freezing().
>
> * p != current test is added to warning printing conditions in
> try_to_freeze_tasks() failure path. This is necessary as freezing()
> is now true for the task which initiated freezing too.
>
> Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
Acked-by: Paul Menage <paul@xxxxxxxxxxxxxx> (for the cgroup portions)
> ---
> include/linux/freezer.h | 33 +++++++++----------------
> kernel/cgroup_freezer.c | 8 +++++-
> kernel/fork.c | 1 -
> kernel/freezer.c | 62 ++++++++++++++++++++++++++++++----------------
> kernel/power/process.c | 15 ++++++++---
> 5 files changed, 70 insertions(+), 49 deletions(-)
>
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index bcc0637..11b2bd9 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -5,8 +5,13 @@
>
> #include <linux/sched.h>
> #include <linux/wait.h>
> +#include <linux/atomic.h>
>
> #ifdef CONFIG_FREEZER
> +extern atomic_t system_freezing_cnt; /* nr of freezing conds in effect */
> +extern bool pm_freezing; /* PM freezing in effect */
> +extern bool pm_nosig_freezing; /* PM nosig freezing in effect */
> +
> /*
> * Check if a process has been frozen
> */
> @@ -15,28 +20,16 @@ static inline int frozen(struct task_struct *p)
> return p->flags & PF_FROZEN;
> }
>
> -/*
> - * Check if there is a request to freeze a process
> - */
> -static inline int freezing(struct task_struct *p)
> -{
> - return test_tsk_thread_flag(p, TIF_FREEZE);
> -}
> +extern bool freezing_slow_path(struct task_struct *p);
>
> /*
> - * Request that a process be frozen
> - */
> -static inline void set_freeze_flag(struct task_struct *p)
> -{
> - set_tsk_thread_flag(p, TIF_FREEZE);
> -}
> -
> -/*
> - * Sometimes we may need to cancel the previous 'freeze' request
> + * Check if there is a request to freeze a process
> */
> -static inline void clear_freeze_flag(struct task_struct *p)
> +static inline bool freezing(struct task_struct *p)
> {
> - clear_tsk_thread_flag(p, TIF_FREEZE);
> + if (likely(!atomic_read(&system_freezing_cnt)))
> + return false;
> + return freezing_slow_path(p);
> }
>
> static inline bool should_send_signal(struct task_struct *p)
> @@ -164,9 +157,7 @@ static inline bool set_freezable_with_signal(void)
> })
> #else /* !CONFIG_FREEZER */
> static inline int frozen(struct task_struct *p) { return 0; }
> -static inline int freezing(struct task_struct *p) { return 0; }
> -static inline void set_freeze_flag(struct task_struct *p) {}
> -static inline void clear_freeze_flag(struct task_struct *p) {}
> +static inline bool freezing(struct task_struct *p) { return false; }
>
> static inline bool __refrigerator(bool check_kthr_stop) { return false; }
> static inline int freeze_processes(void) { BUG(); return 0; }
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 0478c35..4e82525 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -145,7 +145,11 @@ static struct cgroup_subsys_state *freezer_create(struct cgroup_subsys *ss,
> static void freezer_destroy(struct cgroup_subsys *ss,
> struct cgroup *cgroup)
> {
> - kfree(cgroup_freezer(cgroup));
> + struct freezer *freezer = cgroup_freezer(cgroup);
> +
> + if (freezer->state != CGROUP_THAWED)
> + atomic_dec(&system_freezing_cnt);
> + kfree(freezer);
> }
>
> /*
> @@ -311,9 +315,11 @@ static int freezer_change_state(struct cgroup *cgroup,
>
> switch (goal_state) {
> case CGROUP_THAWED:
> + atomic_dec(&system_freezing_cnt);
> unfreeze_cgroup(cgroup, freezer);
> break;
> case CGROUP_FROZEN:
> + atomic_inc(&system_freezing_cnt);
> retval = try_to_freeze_cgroup(cgroup, freezer);
> break;
> default:
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 8e6b6f4..fa7beb3 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1000,7 +1000,6 @@ static void copy_flags(unsigned long clone_flags, struct task_struct *p)
> new_flags |= PF_FORKNOEXEC;
> new_flags |= PF_STARTING;
> p->flags = new_flags;
> - clear_freeze_flag(p);
> }
>
> SYSCALL_DEFINE1(set_tid_address, int __user *, tidptr)
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index 7506ef3..995bddf 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -11,9 +11,41 @@
> #include <linux/freezer.h>
> #include <linux/kthread.h>
>
> +/* total number of freezing conditions in effect */
> +atomic_t system_freezing_cnt = ATOMIC_INIT(0);
> +EXPORT_SYMBOL(system_freezing_cnt);
> +
> +/* indicate whether PM freezing is in effect, protected by pm_mutex */
> +bool pm_freezing;
> +bool pm_nosig_freezing;
> +
> /* protects freezing and frozen transitions */
> static DEFINE_SPINLOCK(freezer_lock);
>
> +/**
> + * freezing_slow_path - slow path for testing whether a task needs to be frozen
> + * @p: task to be tested
> + *
> + * This function is called by freezing() if system_freezing_cnt isn't zero
> + * and tests whether @p needs to enter and stay in frozen state. Can be
> + * called under any context. The freezers are responsible for ensuring the
> + * target tasks see the updated state.
> + */
> +bool freezing_slow_path(struct task_struct *p)
> +{
> + if (p->flags & PF_NOFREEZE)
> + return false;
> +
> + if (pm_nosig_freezing || cgroup_freezing(p))
> + return true;
> +
> + if (pm_freezing && !(p->flags & PF_FREEZER_NOSIG))
> + return true;
> +
> + return false;
> +}
> +EXPORT_SYMBOL(freezing_slow_path);
> +
> /* Refrigerator is place where frozen processes are stored :-). */
> bool __refrigerator(bool check_kthr_stop)
> {
> @@ -23,16 +55,10 @@ bool __refrigerator(bool check_kthr_stop)
> long save;
>
> /*
> - * Enter FROZEN. If NOFREEZE, schedule immediate thawing by
> - * clearing freezing.
> + * No point in checking freezing() again - the caller already did.
> + * Proceed to enter FROZEN.
> */
> spin_lock_irq(&freezer_lock);
> - if (!freezing(current)) {
> - spin_unlock_irq(&freezer_lock);
> - return was_frozen;
> - }
> - if (current->flags & PF_NOFREEZE)
> - clear_freeze_flag(current);
> current->flags |= PF_FROZEN;
> spin_unlock_irq(&freezer_lock);
>
> @@ -96,18 +122,12 @@ static void fake_signal_wake_up(struct task_struct *p)
> bool freeze_task(struct task_struct *p, bool sig_only)
> {
> unsigned long flags;
> - bool ret = false;
>
> spin_lock_irqsave(&freezer_lock, flags);
> -
> - if ((p->flags & PF_NOFREEZE) ||
> - (sig_only && !should_send_signal(p)))
> - goto out_unlock;
> -
> - if (frozen(p))
> - goto out_unlock;
> -
> - set_freeze_flag(p);
> + if (!freezing(p) || frozen(p)) {
> + spin_unlock_irqrestore(&freezer_lock, flags);
> + return false;
> + }
>
> if (should_send_signal(p)) {
> fake_signal_wake_up(p);
> @@ -120,10 +140,9 @@ bool freeze_task(struct task_struct *p, bool sig_only)
> } else {
> wake_up_state(p, TASK_INTERRUPTIBLE);
> }
> - ret = true;
> -out_unlock:
> +
> spin_unlock_irqrestore(&freezer_lock, flags);
> - return ret;
> + return true;
> }
>
> void __thaw_task(struct task_struct *p)
> @@ -140,7 +159,6 @@ void __thaw_task(struct task_struct *p)
> * avoid leaving dangling TIF_SIGPENDING behind.
> */
> spin_lock_irqsave(&freezer_lock, flags);
> - clear_freeze_flag(p);
> if (frozen(p)) {
> wake_up_process(p);
> } else {
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 1bbc363..fe06ddf 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -101,7 +101,7 @@ static int try_to_freeze_tasks(bool sig_only)
> read_lock(&tasklist_lock);
> do_each_thread(g, p) {
> if (!wakeup && !freezer_should_skip(p) &&
> - freezing(p) && !frozen(p))
> + p != current && freezing(p) && !frozen(p))
> sched_show_task(p);
> } while_each_thread(g, p);
> read_unlock(&tasklist_lock);
> @@ -120,13 +120,18 @@ int freeze_processes(void)
> {
> int error;
>
> + if (!pm_freezing)
> + atomic_inc(&system_freezing_cnt);
> +
> printk("Freezing user space processes ... ");
> + pm_freezing = true;
> error = try_to_freeze_tasks(true);
> if (error)
> goto Exit;
> printk("done.\n");
>
> printk("Freezing remaining freezable tasks ... ");
> + pm_nosig_freezing = true;
> error = try_to_freeze_tasks(false);
> if (error)
> goto Exit;
> @@ -146,6 +151,11 @@ void thaw_processes(void)
> {
> struct task_struct *g, *p;
>
> + if (pm_freezing)
> + atomic_dec(&system_freezing_cnt);
> + pm_freezing = false;
> + pm_nosig_freezing = false;
> +
> oom_killer_enable();
>
> printk("Restarting tasks ... ");
> @@ -154,9 +164,6 @@ void thaw_processes(void)
>
> read_lock(&tasklist_lock);
> do_each_thread(g, p) {
> - if (cgroup_freezing(p))
> - continue;
> -
> __thaw_task(p);
> } while_each_thread(g, p);
> read_unlock(&tasklist_lock);
> --
> 1.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/