Messages in this thread Patch in this message | | | Date | Tue, 19 Feb 2013 23:04:31 +0800 | Subject | Re: [PATCH V2 13/15] workqueue: also record worker in work->data if running&&queued | From | Lai Jiangshan <> |
| |
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/
| |