lkml.org 
[lkml]   [2018]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] kthread/smpboot: Serialize kthread parking against wakeup
    On Tue, Apr 24, 2018 at 08:12:49PM +0530, Kohli, Gaurav wrote:
    > @@ -157,6 +156,7 @@ static int smpboot_thread_fn(void *data)
    >
    >                 if (!ht->thread_should_run(td->cpu)) {
    >
    >                         preempt_enable_no_resched();
    >
    > +                       set_current_state(TASK_INTERRUPTIBLE);
    >
    >                         schedule();
    >
    >                 } else {
    >
    >                         __set_current_state(TASK_RUNNING);
    >
    > Please suggest if this approach is better.

    Bah, my brain isn't working... see below for the 'correct' version of
    your second patch.

    But this violates the normal pattern; see the comment near
    set_current_state(). That pattern ensures the thread either sees the
    wakeup condition or the actual wakeup.

    I'm thinking that with this patch there is a scenario where we'll miss
    both the kthread_should_park() and the actual wakeup and end up not
    doing anything.

    I do the like the end result, but I suspect it's buggy.


    ---
    kernel/smpboot.c | 7 +------
    1 file changed, 1 insertion(+), 6 deletions(-)

    diff --git a/kernel/smpboot.c b/kernel/smpboot.c
    index 5043e7433f4b..5bdf57f2ce68 100644
    --- a/kernel/smpboot.c
    +++ b/kernel/smpboot.c
    @@ -109,10 +109,8 @@ static int smpboot_thread_fn(void *data)
    struct smp_hotplug_thread *ht = td->ht;

    while (1) {
    - set_current_state(TASK_INTERRUPTIBLE);
    preempt_disable();
    if (kthread_should_stop()) {
    - __set_current_state(TASK_RUNNING);
    preempt_enable();
    /* cleanup must mirror setup */
    if (ht->cleanup && td->status != HP_THREAD_NONE)
    @@ -122,7 +120,6 @@ static int smpboot_thread_fn(void *data)
    }

    if (kthread_should_park()) {
    - __set_current_state(TASK_RUNNING);
    preempt_enable();
    if (ht->park && td->status == HP_THREAD_ACTIVE) {
    BUG_ON(td->cpu != smp_processor_id());
    @@ -139,7 +136,6 @@ static int smpboot_thread_fn(void *data)
    /* Check for state change setup */
    switch (td->status) {
    case HP_THREAD_NONE:
    - __set_current_state(TASK_RUNNING);
    preempt_enable();
    if (ht->setup)
    ht->setup(td->cpu);
    @@ -147,7 +143,6 @@ static int smpboot_thread_fn(void *data)
    continue;

    case HP_THREAD_PARKED:
    - __set_current_state(TASK_RUNNING);
    preempt_enable();
    if (ht->unpark)
    ht->unpark(td->cpu);
    @@ -156,10 +151,10 @@ static int smpboot_thread_fn(void *data)
    }

    if (!ht->thread_should_run(td->cpu)) {
    + set_current_state(TASK_IDLE);
    preempt_enable_no_resched();
    schedule();
    } else {
    - __set_current_state(TASK_RUNNING);
    preempt_enable();
    ht->thread_fn(td->cpu);
    }
    \
     
     \ /
      Last update: 2018-04-24 20:06    [W:2.949 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site