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 == _exited)) > > > return -ESRCH; > > > - work->next = head; > > > - } while (cmpxchg(>task_works, head, work) != head); > > > + head = head->next; > > > + } while (cmpxchg(, 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 == > 2. head = head->next head == > 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(, ...) > > > > 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(). > 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 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/
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 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/
Re: [PATCH 1/2] task_work: make FIFO task_work list
在 2013-03-14四的 15:40 +0100，Oleg Nesterov写道： > On 03/14, liguang wrote: > > > > Signed-off-by: liguang > > Changelog please... > OK. > > --- > > kernel/task_work.c | 15 +++ > > 1 files changed, 3 insertions(+), 12 deletions(-) > > > > diff --git a/kernel/task_work.c b/kernel/task_work.c > > index 65bd3c9..0bf4258 100644 > > --- 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 == _exited)) > > return -ESRCH; > > - work->next = head; > > - } while (cmpxchg(>task_works, head, work) != head); > > + head = head->next; > > + } while (cmpxchg(, 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?. > 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 2. head = head->next 3. if head == NULL /* it's next node of list tail (w3->next) */ head = work else goto 1 > And cmpxchg() should be cmpxchg(>next) > > > > 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(, ...) > > 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. The motivation it make the list FIFO at task_work_add, so you don't need to reverse it at task_work_run, and it's a time-saver if the list doesn't have too many nodes I think. -- 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/
Re: [PATCH 1/2] task_work: make FIFO task_work list
On 03/14, liguang wrote: > > Signed-off-by: liguang Changelog please... > --- > kernel/task_work.c | 15 +++ > 1 files changed, 3 insertions(+), 12 deletions(-) > > diff --git a/kernel/task_work.c b/kernel/task_work.c > index 65bd3c9..0bf4258 100644 > --- 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 == _exited)) > return -ESRCH; > - work->next = head; > - } while (cmpxchg(>task_works, head, work) != head); > + head = head->next; > + } while (cmpxchg(, 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. 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? And cmpxchg() should be cmpxchg(>next) 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(, ...) can modify the freed and reused memory. Oleg. -- 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/
Re: [PATCH 1/2] task_work: make FIFO task_work list
On 03/14, liguang wrote: Signed-off-by: liguang lig.f...@cn.fujitsu.com Changelog please... --- kernel/task_work.c | 15 +++ 1 files changed, 3 insertions(+), 12 deletions(-) diff --git a/kernel/task_work.c b/kernel/task_work.c index 65bd3c9..0bf4258 100644 --- 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. 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? And cmpxchg(head) should be cmpxchg(head-next) 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. -- 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/
Re: [PATCH 1/2] task_work: make FIFO task_work list
在 2013-03-14四的 15:40 +0100，Oleg Nesterov写道： On 03/14, liguang wrote: Signed-off-by: liguang lig.f...@cn.fujitsu.com Changelog please... OK. --- kernel/task_work.c | 15 +++ 1 files changed, 3 insertions(+), 12 deletions(-) diff --git a/kernel/task_work.c b/kernel/task_work.c index 65bd3c9..0bf4258 100644 --- 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?. 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 2. head = head-next 3. if head == NULL /* it's next node of list tail (w3-next) */ head = work else goto 1 And cmpxchg(head) should be cmpxchg(head-next) 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. The motivation it make the list FIFO at task_work_add, so you don't need to reverse it at task_work_run, and it's a time-saver if the list doesn't have too many nodes I think. -- 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/
