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 <t...@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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
