Messages in this thread | | | Date | Tue, 02 Mar 2010 03:00:27 +0900 | From | Tejun Heo <> | Subject | Re: [PATCH 20/43] workqueue: reimplement work flushing using linked works |
| |
Hello,
On 03/01/2010 11:53 PM, Oleg Nesterov wrote: > On 02/26, Tejun Heo wrote: >> >> +static void move_linked_works(struct work_struct *work, struct list_head *head, >> + struct work_struct **nextp) >> +{ >> ... >> + work = list_entry(work->entry.prev, struct work_struct, entry); >> + list_for_each_entry_safe_continue(work, n, NULL, entry) { > > list_for_each_entry_safe_from(work) ? It doesn't need to move this > work back.
Yeap, that will be prettier. I used _continue there thinking continue will step from the current one and after finding out that it didn't, I rewound work not knowing about _from. Will update.
>> @@ -680,7 +734,27 @@ static int worker_thread(void *__worker) >> if (kthread_should_stop()) >> break; >> >> - run_workqueue(worker); >> + spin_lock_irq(&cwq->lock); >> + >> + while (!list_empty(&cwq->worklist)) { >> + struct work_struct *work = >> + list_first_entry(&cwq->worklist, >> + struct work_struct, entry); >> + >> + if (likely(!(*work_data_bits(work) & >> + WORK_STRUCT_LINKED))) { >> + /* optimization path, not strictly necessary */ >> + process_one_work(worker, work); >> + if (unlikely(!list_empty(&worker->scheduled))) >> + process_scheduled_works(worker); >> + } else { >> + move_linked_works(work, &worker->scheduled, >> + NULL); >> + process_scheduled_works(worker); >> + } >> + } > > So. If the next pending work W doesn't have WORK_STRUCT_LINKED, > it will be executed first, then we flush ->scheduled. > > But, > >> static void insert_wq_barrier(struct cpu_workqueue_struct *cwq, >> - struct wq_barrier *barr, struct list_head *head) >> + struct wq_barrier *barr, >> + struct work_struct *target, struct worker *worker) >> { >> ... >> + /* >> + * If @target is currently being executed, schedule the >> + * barrier to the worker; otherwise, put it after @target. >> + */ >> + if (worker) >> + head = worker->scheduled.next; > > this is the "target == current_work" case, > >> - insert_work(cwq, &barr->work, head, work_color_to_flags(WORK_NO_COLOR)); >> + insert_work(cwq, &barr->work, head, >> + work_color_to_flags(WORK_NO_COLOR) | linked); >> } > > and in this case we put this barrier at the head of ->scheduled list. > > This means, this barrier will run after that work W, not before it?
Yes, the barrier will run after the target work as it should.
> Hmm. And what if there are no pending works but ->current_work == target ? > Again, we add the barrier to ->scheduled, but in this case worker_thread() > can't even notice ->scheduled is not empty because it only checks ->worklist?
A worker always checks ->scheduled after a work is finished. IOW, if someone saw worker->current_work == target while holding the lock, the worker will check the scheduled queue after finishing the target. If it's not doing it somewhere, it's a bug.
> insert_wq_barrier() also does: > > unsigned long *bits = work_data_bits(target); > ... > *bits |= WORK_STRUCT_LINKED; > > perhaps this needs atomic_long_set(), although I am not sure this really > matters.
Yeah, well, work->data access is pretty messed up. At this point, there's no reason for atomic_long_t to begin with. No real atomic_long operations are used anyway. Maybe work->data started as proper atomic_long_t and lost its properness as it got overloaded with multiple things. I'm thinking about just making it a unsigned long and killing work_data_bits().
Thanks.
-- tejun
| |