Prev: Q: sys_futex() && timespec_valid()
Next: Fwd: fs/exec.c core dumping on NFS mounted directory
From: Andi Kleen on 25 Jun 2010 15:40
On Fri, Jun 25, 2010 at 08:30:25PM +0200, Peter Zijlstra wrote:

I'm not sure what all the logic for entry enqueued by someone
else is good for? Is that for the case you don't have enough
entries preallocated and you share them with someone else?

Normally if the sharing is per cpu that would be difficult
to recover from because if it's due to a nest situation (for example)
you would deadlock.

For me it would seem simpler to simply not share.

> + struct irq_work *list;
> +
> + BUG_ON(!in_irq());
> + BUG_ON(!irqs_disabled());
> +
> + list = xchg(&__get_cpu_var(irq_work_list), NULL);
> + while (list != NULL) {
> + struct irq_work *entry = list;
> +
> + list = irq_work_next(list);
> +
> + /*
> + * Clear the PENDING bit, after this point the @entry
> + * can be re-used.
> + */
> + entry->next = next_flags(NULL, IRQ_WORK_BUSY);
> + entry->func(entry);

Needs compiler memory barrier here I think.

> + /*
> + * Clear the BUSY bit and return to the free state if
> + * no-one else claimed it meanwhile.
> + */
> + cmpxchg(&entry->next, next_flags(NULL, IRQ_WORK_BUSY), NULL);
> + }
> +}

-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 25 Jun 2010 15:50
On Fri, 2010-06-25 at 21:30 +0200, Andi Kleen wrote:
> > + entry->next = next_flags(NULL, IRQ_WORK_BUSY);
> > + entry->func(entry);
>
> Needs compiler memory barrier here I think.
>
> > + /*
> > + * Clear the BUSY bit and return to the free state if
> > + * no-one else claimed it meanwhile.
> > + */
> > + cmpxchg(&entry->next, next_flags(NULL, IRQ_WORK_BUSY), NULL);
> > + }

Both the (indirect) function call and the cmpxchg imply a compiler
barrier.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andi Kleen on 25 Jun 2010 18:40
> perf has two different reasons to for the callback, what I do is set the
> state and enqueue, if its already enqueued the pending callback will
> handle both.
>
> Its cheaper than having two callback structures per event.

Again it sounds like you just need a bit...
>
> We can expose the claim/enqueue thing separately so that users can
> choose.

Yes it would be good to separate that, because I doubt other users
will require similar hacks.

-Andi

--
ak(a)linux.intel.com -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: huang ying on 25 Jun 2010 21:30
On Sat, Jun 26, 2010 at 2:30 AM, Peter Zijlstra <peterz(a)infradead.org> wrote:
> +
> +static DEFINE_PER_CPU(struct irq_work *, irq_work_list);
> +
> +/*
> + * Claim the entry so that no one else will poke at it.
> + */
> +static bool irq_work_claim(struct irq_work *entry)
> +{
> +       unsigned long flags;
> +
> +       do {
> +               flags = (unsigned long)entry->next;
> +               if (flags & IRQ_WORK_PENDING)
> +                       return false;
> +       } while (cmpxchg(&entry->next, flags, flags | IRQ_WORK_FLAGS) != flags);
> +
> +       return true;
> +}
> +
> +
> +void __weak arch_irq_work_raise(void)
> +{
> +       /*
> +        * Lame architectures will get the timer tick callback
> +        */
> +}
> +
> +/*
> + * Queue the entry and raise the IPI if needed.
> + */
> +static void __irq_work_queue(struct irq_work *entry)
> +{
> +       struct irq_work **head;
> +
> +       head = &get_cpu_var(irq_work_list);
> +
> +       do {
> +               /*
> +                * Can assign non-atomic because we keep the flags set.
> +                */
> +               entry->next = next_flags(*head, IRQ_WORK_FLAGS);
> +       } while (cmpxchg(head, entry->next, entry) != entry->next);

*head & IRQ_WORK_FLAGS == 0, but entry->next & IRQ_WORK_FLAGS ==
IRQ_WORK_FLAGS. So the cmpxchg will never succeed.

> +
> +       /*
> +        * The list was empty, raise self-interrupt to start processing.
> +        */
> +       if (!irq_work_next(entry))
> +               arch_irq_work_raise();
> +
> +       put_cpu_var(irq_work_list);
> +}

Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 26 Jun 2010 04:40
On Sat, 2010-06-26 at 00:29 +0200, Andi Kleen wrote:

> Yes it would be good to separate that, because I doubt other users
> will require similar hacks.

You're such a constructive critic..

I would think every NMI user would need them since NMI can interrupt at
any time, and if you have a limited number of irq_work structs (like 1
per cpu) you'll end up with wanting to enqueue an already enqueued one
at some point.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: Q: sys_futex() && timespec_valid()
Next: Fwd: fs/exec.c core dumping on NFS mounted directory