Messages in this thread | | | Date | Mon, 26 May 2014 21:26:33 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH 1/5] irq_work: Split raised and lazy lists |
| |
On Mon, May 26, 2014 at 06:53:13PM +0200, Frederic Weisbecker wrote: > On Mon, May 26, 2014 at 05:59:44PM +0200, Peter Zijlstra wrote: > > On Sun, May 25, 2014 at 04:29:47PM +0200, Frederic Weisbecker wrote: > > > An irq work can be handled from two places: from the tick if the work > > > carries the "lazy" flag and the tick is periodic, or from a self IPI. > > > > > > We merge all these works in a single list and we use some per cpu latch > > > to avoid raising a self-IPI when one is already pending. > > > > > > Now we could do away with this ugly latch if only the list was only made of > > > non-lazy works. Just enqueueing a work on the empty list would be enough > > > to know if we need to raise an IPI or not. > > > > > > Also we are going to implement remote irq work queuing. Then the per CPU > > > latch will need to become atomic in the global scope. That's too bad > > > because, here as well, just enqueueing a work on an empty list of > > > non-lazy works would be enough to know if we need to raise an IPI or not. > > > > > > So lets take a way out of this: split the works in two distinct lists, > > > one for the works that can be handled by the next tick and another > > > one for those handled by the IPI. Just checking if the latter is empty > > > when we queue a new work is enough to know if we need to raise an IPI. > > > > That ^ > > > > > bool irq_work_queue(struct irq_work *work) > > > { > > > + unsigned long flags; > > > + > > > /* Only queue if not already pending */ > > > if (!irq_work_claim(work)) > > > return false; > > > > > > - /* Queue the entry and raise the IPI if needed. */ > > > - preempt_disable(); > > > + /* Check dynticks safely */ > > > + local_irq_save(flags); > > > > Does not mention this ^ > > > > 'sup? > > Because it's really just a technical detail. > If we enqueue before checking for tick stopped, we can avoid disabling irqs > because it's fine if we just raced with an irq in-between. > > But now that we enqueue _after_, we can't afford an IRQ in between. > > Should I update the comments maybe?
Well, yes because it was entirely non-obvious, but maybe we can write it such that we can avoid the irq disable, because they're expensive.
How about something like:
if (work->flags & IRQ_WORK_LAZY) { if (llist_add(&work->llnode, __get_cpu_var(lazy_list)) && tick_nohz_tick_stopped()) arch_irq_work_raise(); } else { if (llist_add(&work->llnode, __get_cpu_var(raise_list))) arch_irq_work_raise(); }
That way we check it after the enqueue.
| |