Re: [PATCH V2 13/15] workqueue: also record worker in work->data if running&&queued

[Lai Jiangshan <eag0628@gmail.com>]

Hi, TJ

Thank you for reviews and comments.
First, the patchset can't be merged to 3.9 since there are under
discussion and are not shown they benefit.
(But are patch 1, 5-8 possible merged after rebased and revised?)
Second, the removal of the hash table is very possible in future with
different implementation.

On Tue, Feb 19, 2013 at 3:50 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Lai.
>
> On Tue, Feb 19, 2013 at 12:12:14AM +0800, Lai Jiangshan wrote:
>> +/**
>> + * get_work_cwq - get cwq of the work
>> + * @work: the work item of interest
>> + *
>> + * CONTEXT:
>> + * spin_lock_irq(&pool->lock), the work must be queued on this pool
>> + */
>> +static struct cpu_workqueue_struct *get_work_cwq(struct work_struct *work)
>> +{
>> +     unsigned long data = atomic_long_read(&work->data);
>> +     struct worker *worker;
>> +
>> +     if (data & WORK_STRUCT_CWQ) {
>> +             return (void *)(data & WORK_STRUCT_WQ_DATA_MASK);
>> +     } else if (data & WORK_OFFQ_REQUEUED) {
>> +             worker = worker_by_id(data >> WORK_OFFQ_WORKER_SHIFT);
>> +             BUG_ON(!worker || !worker->requeue);
>> +             return worker->current_cwq;
>> +     } else {
>> +             BUG();
>> +             return NULL;
>> +     }
>> +}
>
> So, work->data points to the last worker ID if off-queue or on-queue
> with another worker executing it and points to cwq if on-queue w/o
> another worker executing.  If on-queue w/ concurrent execution, the
> excuting worker updates work->data when it finishes execution, right?

right.

>
> Why no documentation about it at all?  The mechanism is convoluted
> with interlocking from both work and worker sides.  Lack of
> documentation makes things difficult for reviewers and later readers
> of the code.

sorry.

>
>> @@ -1296,8 +1283,16 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
>>               worklist = &cwq->delayed_works;
>>       }
>>
>> -     color_flags = work_color_to_flags(cwq->work_color);
>> -     insert_work(cwq, work, worklist, color_flags | delayed_flags);
>> +     if (worker) {
>> +             worker->requeue = true;
>> +             worker->requeue_color = cwq->work_color;
>> +             set_work_worker_and_keep_pending(work, worker->id,
>> +                             delayed_flags | WORK_OFFQ_REQUEUED);
>> +             list_add_tail(&work->entry, worklist);
>> +     } else {
>> +             color_flags = work_color_to_flags(cwq->work_color);
>> +             insert_work(cwq, work, worklist, color_flags | delayed_flags);
>> +     }
>
> I can't say I like this.  In interlocks the work being queued and the
> worker so that both have to watch out for each other.  It's kinda
> nasty.
>
>> @@ -2236,6 +2241,16 @@ __acquires(&pool->lock)
>>       worker->current_func = NULL;
>>       worker->current_cwq = NULL;
>>       cwq_dec_nr_in_flight(cwq, work_color);
>> +
>> +     if (unlikely(worker->requeue)) {
>> +             unsigned long color_flags, keep_flags;
>> +
>> +             worker->requeue = false;
>> +             keep_flags = atomic_long_read(&work->data);
>> +             keep_flags &= WORK_STRUCT_LINKED | WORK_STRUCT_DELAYED;
>> +             color_flags = work_color_to_flags(worker->requeue_color);
>> +             set_work_cwq(work, cwq, color_flags | keep_flags);
>> +     }
>
> So, what was before mostly one way "is it still executing?" query
> becomes three party handshake among the queuer, executing worker and
> try_to_grab_pending(), and we end up shifting information from the
> queuer through the executing worker because work->data can't hold both
> workqueue and worker information.
>
> I don't know, Lai.  While removal of busy_hash is nice, I'm not really
> sure whether we're ending up with better or worse code by doing this.
> It's more convoluted for sure.  Performance-wise, now that idr_find()
> for pool costs almost nothing (because we're very unlikely to have
> more than 256 pools), we're comparing one idr lookup (which can easily
> blow through 256 single layer optimization limit) against two simple
> hash table lookup.  I don't really think either would be noticeably
> better than the other in any measureable way.
>
> The trade-off, while doesn't seem too bad, doesn't seem much
> beneficial either.  It's different from what we're currently doing but
> I'm not sure we're making it better by doing this.
>

I agree your comments.

Thanks,
Lai

PS: Some small benefits of this patchset
1) work_busy() can be reimplement lockless.
2) the user can issue *reentrance" works by re-init the work.
    (w/o this patchset, the users must defer the free and alloc new
work item if they want reentrancable)
3) natural immunity of such
bugs(https://bugzilla.kernel.org/show_bug.cgi?id=51701)

but small.
need to find a better way in future.

>
> --
> tejun
> --
> 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/