Messages in this thread | | | Date | Tue, 28 Aug 2012 19:01:21 +0200 | From | Oleg Nesterov <> | Subject | Re: 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 = ¤t->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.
| |