lkml.org 
[lkml]   [2007]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop
    On Sun, Apr 22, 2007 at 09:40:59PM +0200, Rafael J. Wysocki wrote:
    > From: Rafael J. Wysocki <rjw@sisk.pl>
    >
    > Fix the problem with kthread_stop() that causes the freezer to fail if a
    > freezable thread is attempting to stop a frozen one and that may cause the
    > freezer to fail if the thread being stopped is freezable and
    > try_to_freeze_tasks() is running concurrently with kthread_stop().
    >
    > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
    > ---
    > kernel/kthread.c | 9 +++++++++
    > 1 file changed, 9 insertions(+)
    >
    > Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
    > ===================================================================
    > --- linux-2.6.21-rc6-mm1.orig/kernel/kthread.c 2007-04-09 15:23:48.000000000 +0200
    > +++ linux-2.6.21-rc6-mm1/kernel/kthread.c 2007-04-22 19:05:29.000000000 +0200
    > @@ -13,6 +13,7 @@
    > #include <linux/file.h>
    > #include <linux/module.h>
    > #include <linux/mutex.h>
    > +#include <linux/freezer.h>
    > #include <asm/semaphore.h>
    >
    > /*
    > @@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
    >
    > /* Now set kthread_should_stop() to true, and wake it up. */
    > kthread_stop_info.k = k;
    > + if (!freezer_should_exempt(current)) {
    > + /* We are freezable, so we must make sure that the thread being
    > + * stopped is not frozen and will not be frozen until it dies
    > + */
    > + freezer_exempt(k);
    > + if (frozen(k))
    > + clear_frozen_flag(k);
    > + }

    I'm trying hard to convince myself that this will work. May be I am
    missing something here, but I find a potential race window (very small though)
    when k is entering the refrigerator.

    Here's how.

    kthread_stop(k) k->refrigerator()
    ---------------------------------------------------------------------
    task_lock(k);
    1) check_if_exempted();
    /* not exempted. So
    * we will freeze
    * ourself.
    */
    2) freezer_exempt(k);

    3) if(frozen(k))
    /* No, we're not yet frozen. So we
    * don't clear_frozen_flag(k) here
    */
    4) frozen_process(k);
    task_unlock(k);

    5) for(;;) {
    set_current_state(UNINTERRUPTIBLE);
    if(!frozen_process(k))
    /* k is frozen. We
    * don't break :(
    */

    schedule();
    }

    > wake_up_process(k);
    > put_task_struct(k);
    >

    Thus the freezer can still fail, no?
    IMO, we need the to take the task_lock for k here. Something like

    > + if (!freezer_should_exempt(current)) {
    task_lock(k);
    > + /* We are freezable, so we must make sure that the thread being
    > + * stopped is not frozen and will not be frozen until it dies
    > + */
    > + freezer_exempt(k);
    > + if (frozen(k))
    > + clear_frozen_flag(k);
    task_unlock(k);
    > + }


    Thanks and Regards
    gautham.
    --
    Gautham R Shenoy
    Linux Technology Center
    IBM India.
    "Freedom comes with a price tag of responsibility, which is still a bargain,
    because Freedom is priceless!"
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-04-23 14:41    [W:0.069 / U:90.376 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site