Messages in this thread | | | Date | Fri, 15 Mar 2013 15:34:12 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 1/2] task_work: make FIFO task_work list |
| |
On 03/15, li guang wrote: > > 在 2013-03-14四的 15:40 +0100,Oleg Nesterov写道: > > > --- a/kernel/task_work.c > > > +++ b/kernel/task_work.c > > > @@ -13,11 +13,12 @@ task_work_add(struct task_struct *task, struct callback_head *work, bool notify) > > > head = ACCESS_ONCE(task->task_works); > > > if (unlikely(head == &work_exited)) > > > return -ESRCH; > > > - work->next = head; > > > - } while (cmpxchg(&task->task_works, head, work) != head); > > > + head = head->next; > > > + } while (cmpxchg(&head, NULL, work) == head); > > > > I simply can't understand how this can work... The patch assumes > > that head->next == NULL after head = head->next, why? And then > > compares the result with head and succeeds if not equal. > > > > then ->next filed was not initialized, so I think it will > be 0'ed by compiler, is it unreliable?.
work->next is not necessarily initialized, but this is not the main problem...
> > Could you please explain how it was supposed to work? If nothing > > else, Suppose we have task->task_works -> W1 -> W2 -> W3. How this > > code can add W4 after W3? > > > > 1. head = task_works
head == &W1
> 2. head = head->next
head == &W2
> 3. if head == NULL > /* it's next node of list tail (w3->next) */ > head = work
No,
> else > goto 1
And? You restart from ->task_works again.
> > Anyway, whatever I missed this is racy. > > > > head = head->next; > > > > nothing protects "head" after this. Say, it can be task_work_cancel'ed > > and freed. So, > > > > cmpxchg(&head, ...) > > > > can modify the freed and reused memory. > > > > Oleg. > > Thanks Oleg, > Hmm, at first, I think even it was changed, it can't happened to be > NULL, but ... maybe it need more deliberation.
My point was, even if it is not NULL nothing protects this element. It can be freed/reused before you do cmpxchg(&head).
> The motivation it make the list FIFO at task_work_add, so you don't > need to reverse it at task_work_run,
I understand, but this is not easy and unlikely possible without the locking.
> and it's a time-saver if the list
Yes, but compared to the next loop which does do/while again _and_ calls the work->func() "Reverse the list" doesn't add too much.
Oleg.
-- 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/
| |