lkml.org 
[lkml]   [2015]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 2/4] smpboot: Make cleanup to mirror setup
    Date
    The per-cpu kthread cleanup() callback is the mirror of the setup()
    callback. When the per-cpu kthread is started, it first calls setup()
    to initialize the resources which are then released by cleanup() when
    the kthread exits.

    Now since the introduction of a per-cpu kthread cpumask, the kthreads
    excluded by the cpumask on boot may happen to be parked immediately
    after their creation without taking the setup() stage, waiting to be
    asked to unpark to do so. Then when smpboot_unregister_percpu_thread()
    is later called, the kthread is stopped without having ever called
    setup().

    But this triggers a bug as the kthread unconditionally calls cleanup()
    on exit but this doesn't mirror any setup(). Thus the kernel crashes
    because we try to free resources that haven't been initialized, as in
    the watchdog case:

    [ 112.645556] WATCHDOG disable 0
    [ 112.648765] WATCHDOG disable 1
    [ 112.651891] WATCHDOG disable 2
    [ 112.654953] BUG: unable to handle kernel NULL pointer dereference at (null)
    [ 112.662808] IP: [<ffffffff8111ea16>] hrtimer_active+0x26/0x60
    [...]
    [ 112.815078] Call Trace:
    [ 112.817523] [<ffffffff8111fe7c>] hrtimer_try_to_cancel+0x1c/0x280
    [ 112.823697] [<ffffffff811200fd>] hrtimer_cancel+0x1d/0x30
    [ 112.829172] [<ffffffff8115d846>] watchdog_disable+0x56/0x70
    [ 112.834818] [<ffffffff8115d86e>] watchdog_cleanup+0xe/0x10
    [ 112.840381] [<ffffffff810ca05c>] smpboot_thread_fn+0x23c/0x2c0
    [ 112.846296] [<ffffffff810c9e20>] ? sort_range+0x30/0x30
    [ 112.851596] [<ffffffff810c6478>] kthread+0xf8/0x110
    [ 112.856550] [<ffffffff810c6380>] ? kthread_create_on_node+0x210/0x210
    [ 112.863065] [<ffffffff8191501f>] ret_from_fork+0x3f/0x70
    [ 112.868460] [<ffffffff810c6380>] ? kthread_create_on_node+0x210/0x210

    This bug is currently masked with explicit kthread unparking before
    kthread_stop() on smpboot_destroy_threads(). This forces a call to
    setup() and then unpark().

    We could fix this by unconditionally calling setup() on kthread entry.
    But setup() isn't always cheap. In the case of watchdog it launches
    hrtimer, perf events, etc... So we may as well like to skip it if there
    are chances the kthread will never be used, as in a reduced cpumask
    value.

    So let's simply do a state machine check before calling cleanup() that
    makes sure setup() has been called before mirroring it.

    And remove the nasty hack workaround.

    Reviewed-by: Chris Metcalf <cmetcalf@ezchip.com>
    Cc: Andrew Morton <akpm@linux-foundation.org>
    Cc: Chris Metcalf <cmetcalf@ezchip.com>
    Cc: Don Zickus <dzickus@redhat.com>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Thomas Gleixner <tglx@linutronix.de>
    Cc: Ulrich Obergfell <uobergfe@redhat.com>
    Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
    ---
    kernel/smpboot.c | 12 ++----------
    1 file changed, 2 insertions(+), 10 deletions(-)

    diff --git a/kernel/smpboot.c b/kernel/smpboot.c
    index 71aa90b..60aa858 100644
    --- a/kernel/smpboot.c
    +++ b/kernel/smpboot.c
    @@ -113,7 +113,8 @@ static int smpboot_thread_fn(void *data)
    if (kthread_should_stop()) {
    __set_current_state(TASK_RUNNING);
    preempt_enable();
    - if (ht->cleanup)
    + /* cleanup must mirror setup */
    + if (ht->cleanup && td->status != HP_THREAD_NONE)
    ht->cleanup(td->cpu, cpu_online(td->cpu));
    kfree(td);
    return 0;
    @@ -259,15 +260,6 @@ static void smpboot_destroy_threads(struct smp_hotplug_thread *ht)
    {
    unsigned int cpu;

    - /* Unpark any threads that were voluntarily parked. */
    - for_each_cpu_not(cpu, ht->cpumask) {
    - if (cpu_online(cpu)) {
    - struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu);
    - if (tsk)
    - kthread_unpark(tsk);
    - }
    - }
    -
    /* We need to destroy also the parked threads of offline cpus */
    for_each_possible_cpu(cpu) {
    struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu);
    --
    2.1.4


    \
     
     \ /
      Last update: 2015-07-11 18:21    [W:3.159 / U:0.848 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site