lkml.org 
[lkml]   [2008]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch, minor] workqueue: consistently use 'err' in __create_workqueue_key()
    2008/7/29 Oleg Nesterov <oleg@tv-sign.ru>:
    > On 07/28, Dmitry Adamushko wrote:
    >>
    >> I guess error handling is a bit illogical in __create_workqueue_key()
    >
    > Please see below,
    >
    >> for_each_possible_cpu(cpu) {
    >> cwq = init_cpu_workqueue(wq, cpu);
    >> - if (err || !cpu_online(cpu))
    >> + if (!cpu_online(cpu))
    >> continue;
    >> err = create_workqueue_thread(cwq, cpu);
    >> + if (err)
    >> + break;
    >
    > This was done on purpose. The code above does init_cpu_workqueue(cpu)
    > for each possible cpu, even if we fail to create cwq->thread for some
    > cpu. This way destroy_workqueue() (called below) shouldn't worry about
    > the partially initialized workqueues.
    >
    > The patch above should work, but it assumes that destroy_workqueue()
    > must do nothing with cwq if cwq->thread == NULL, this is not very
    > robust.

    Yes, I saw this test and that's why I decided that destroy_workqueue()
    is able (designed) to deal with partially-initialized objects.

    Note, for the race scenario with cpu-hotplug (which I've overlooked
    indeed) which you describe below, we also seem to depend on the same
    "cwq->thread == NULL" test in cleanup_workqueue_thread() as follows:

    assume, cpu_down(cpu) -> CPU_POST_DEAD -> cleanup_workqueue_thread()
    gets called for a partially initialized workqueue for 'cpu' for which
    create_workqueue_thread() has previously failed in
    create_worqueue_key().

    >
    > And, more importantly. Let's suppose __create_workqueue_key() does
    > "break" and drops cpu_add_remove_lock. Then we race with cpu-hotplug
    > which can hit the uninitialized cwq. This is fixable, but needs other
    > complication.

    And I'd say this behavior (of having a partially-created object
    visible to the outside world) is not that robust. e.g. the
    aforementioned race would be eliminated if we place a wq on the global
    list only when it's been successfully initialized.

    For this goal, the cleanup path in __create_workqueue_key() would need
    to be altered but overall, I think it'd make the code a bit more
    straightforward.

    [ just my 0.02, maybe I'm missing something again ;-) ]



    >
    > Oleg.
    >


    --
    Best regards,
    Dmitry Adamushko


    \
     
     \ /
      Last update: 2008-07-29 14:01    [W:0.022 / U:42.004 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site