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.
--
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/
