[lkml]   [2008]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [patch, minor] workqueue: consistently use 'err' in __create_workqueue_key()
    2008/7/29 Oleg Nesterov <>:
    > 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

    > 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

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

    > Oleg.

    Best regards,
    Dmitry Adamushko

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