lkml.org 
[lkml]   [2012]   [Aug]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: lockdep trace from posix timers
On 08/28, Peter Zijlstra wrote:
>
> On Fri, 2012-08-24 at 20:56 +0200, Oleg Nesterov wrote:
> >
> > Peter, if you think it can work for you and if you agree with
> > the implementation I will be happy to send the patch.
>
> Yeah I think it would work, but I'm not sure why you're introducing the
> cmp_xchg helper just for this..

Please look at 1-4 the patches I sent (only 1-2 are relevant), I removed
this helper. Although I still think it makes sense, but of course not in
task_work.c.

> struct callback_head *
> task_work_cancel(struct task_struct *task, task_work_func_t func)
> {
> - unsigned long flags;
> - struct callback_head *last, *res = NULL;
> -
> - raw_spin_lock_irqsave(&task->pi_lock, flags);
> - last = task->task_works;
> - if (last) {
> - struct callback_head *q = last, *p = q->next;
> - while (1) {
> - if (p->func == func) {
> - q->next = p->next;
> - if (p == last)
> - task->task_works = q == p ? NULL : q;
> - res = p;
> - break;
> - }
> - if (p == last)
> - break;
> - q = p;
> - p = q->next;
> + struct callback_head **workp, *work;
> +
> +again:
> + workp = &task->task_works;
> + work = *workp;
> + while (work) {
> + if (work->func == func) {

But you can't dereference this pointer. Without some locking this
can race with another task_work_cancel() or task_work_run(), this
work can be free/unmapped/reused.

> + if (cmpxchg(workp, work, work->next) == work)
> + return work;

Or this can race with task_work_cancel(work) + task_work_add(work).
cmpxchg() can succeed even if work->func is already different.

> +static callback_head *task_work_pop(void)
> {
> - struct task_struct *task = current;
> - struct callback_head *p, *q;
> -
> - while (1) {
> - raw_spin_lock_irq(&task->pi_lock);
> - p = task->task_works;
> - task->task_works = NULL;
> - raw_spin_unlock_irq(&task->pi_lock);
> -
> - if (unlikely(!p))
> - return;
> -
> - q = p->next; /* head */
> - p->next = NULL; /* cut it */
> - while (q) {
> - p = q->next;
> - q->func(q);
> - q = p;
> - }
> + struct callback_head **head = &current->task_work;
> + struct callback_head *entry, *old_entry;
> +
> + entry = *head;
> + for (;;) {
> + if (!entry || entry == &dead)
> + return NULL;
> +
> + old_entry = entry;
> + entry = cmpxchg(head, entry, entry->next);

Well, this obviously means cmpxchg() for each entry...

> ( And yeah, I know, its not FIFO ;-)

Cough. akpm didn't like fifo, Linus disliked it too...

And now you! Whats going on??? ;)

Oleg.



\
 
 \ /
  Last update: 2012-08-28 19:41    [W:0.161 / U:0.576 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site