Re: [PATCH] per-interrupt stacks - try 2

Re: [PATCH] per-interrupt stacks - try 2

[David Howells]

> per-CPU per-IRQ i mean, of course. It's a basic performance issue, on SMP
> we do not want dirty IRQ stacks to bounce between CPUs ...

Do you have benchmarks or something to show that is this actually a
_significant_ problem?

After all, unless you bind the interrupts to particular IRQs, loads of data -
including the irq_desc[] table - are going to be bouncing too.

I'm not convinced that it's going to be worth doing stacks on a per-CPU basis,
and not a per-IRQ basis. The only way I can see that it could be a problem is
that if it will be permitted to enter handle_IRQ_event() on two or more
different CPUs simultaneously for the same IRQ, provided that do_IRQ() itself
switches the stack just around the call to handle_IRQ_event() [see attached].

I think, however, that per-CPU softirq stacks would be a good idea, as these
are used quite capaciously by TCP/IP.

David

asmlinkage unsigned int do_IRQ(struct pt_regs regs)
{	
	/* 
	 * We ack quickly, we don't want the irq controller
	 * thinking we're snobs just because some other CPU has
	 * disabled global interrupts (we have already done the
	 * INT_ACK cycles, it's too late to try to pretend to the
	 * controller that we aren't taking the interrupt).
	 *
	 * 0 return value means that this irq is already being
	 * handled by some other CPU. (or is disabled)
	 */
	int irq = regs.orig_eax & 0xff; /* high bits used in ret_from_ code  */
	int cpu = smp_processor_id();
	irq_desc_t *desc = irq_desc + irq;
	struct thread_info *curctx;
	struct irqaction *action;
	union irq_ctx *irqctx;
	unsigned int status;

	irq_enter();
	kstat.irqs[cpu][irq]++;
	spin_lock(&desc->lock);
	desc->handler->ack(irq);
	/*
	  REPLAY is when Linux resends an IRQ that was dropped earlier
	  WAITING is used by probe to mark irqs that are being tested
	*/
	status = desc->status & ~(IRQ_REPLAY | IRQ_WAITING);
	status |= IRQ_PENDING; /* we _want_ to handle it */

	/*
	 * If the IRQ is disabled for whatever reason, we cannot
	 * use the action we have.
	 */
	action = NULL;
	if (likely(!(status & (IRQ_DISABLED | IRQ_INPROGRESS)))) {
		action = desc->action;
		status &= ~IRQ_PENDING; /* we commit to handling */
		status |= IRQ_INPROGRESS; /* we are handling it */
	}
	desc->status = status;

	/*
	 * If there is no IRQ handler or it was disabled, exit early.
	 Since we set PENDING, if another processor is handling
	 a different instance of this same irq, the other processor
	 will take care of it.
	*/
	if (unlikely(!action))
		goto out;

	/*
	 * Edge triggered interrupts need to remember
	 * pending events.
	 * This applies to any hw interrupts that allow a second
	 * instance of the same irq to arrive while we are in do_IRQ
	 * or in the handler. But the code here only handles the _second_
	 * instance of the irq, not the third or fourth. So it is mostly
	 * useful for irq hardware that does not mask cleanly in an
	 * SMP environment.
	 */
	curctx = current_thread_info();
	irqctx = desc->hardirq_stack;
	irqctx->tinfo.task = curctx->task;

	for (;;) {
		u32 *isp;

		spin_unlock(&desc->lock);

		/* build the stack frame on the IRQ stack */
		isp = (u32*) ((char*)irqctx + sizeof(*irqctx));
		*--isp = (u32) action;
		*--isp = (u32) &regs;
		*--isp = (u32) irq;

		asm volatile(
			"	movl	%1,%%eax	\n"
			"	xchgl	%%ebx,%%esp	\n"
			"	call	*%%eax		\n"
			"	xchgl	%%ebx,%%esp	\n"
			:
			: "b"(isp), "i"(handle_IRQ_event)
			: "memory", "cc", "eax", "edx", "ecx"
			);

		spin_lock(&desc->lock);
		
		if (likely(!(desc->status & IRQ_PENDING)))
			break;
		desc->status &= ~IRQ_PENDING;
	}

	irqctx->tinfo.task = NULL;
	desc->status &= ~IRQ_INPROGRESS;

 out:
	/*
	 * The ->end() handler has to deal with interrupts which got
	 * disabled while the handler was running.
	 */
	desc->handler->end(irq);
	spin_unlock(&desc->lock);

	irq_exit();

	return 1;
}

Re: [PATCH] per-interrupt stacks - try 2

[Ingo Molnar]

On Thu, 12 Sep 2002, David Howells wrote:

> > per-CPU per-IRQ i mean, of course. It's a basic performance issue, on
> > SMP we do not want dirty IRQ stacks to bounce between CPUs ...
> 
> Do you have benchmarks or something to show that is this actually a
> _significant_ problem?

you need benchmarks to tell that pure per-IRQ stacks are bad for SMP
performance?

per-IRQ+per-CPU and pure per-CPU IRQ stacks should perform rougly equally
well on SMP - with per-CPU IRQ stacks having lower runtime setup cost.

> After all, unless you bind the interrupts to particular IRQs, loads of
> data - including the irq_desc[] table - are going to be bouncing too.

there's a difference between bouncing 1-2 cachelines and bouncing a *full,
dirtied stack*. The irq_desc[] bouncing is pretty much unavoidable (IRQs
do need some global state) - the stack bouncing is just plain stupid and
perfectly avoidable.

	Ingo

Re: [PATCH] per-interrupt stacks - try 2

[David Howells]

> > Do you have benchmarks or something to show that is this actually a
> > _significant_ problem?
> 
> you need benchmarks to tell that pure per-IRQ stacks are bad for SMP
> performance?

No. I asked how _significant_ the difference is, as compared to the rest of
the accesses it makes.

> per-IRQ+per-CPU and pure per-CPU IRQ stacks should perform rougly equally
> well on SMP - with per-CPU IRQ stacks having lower runtime setup cost.

There are problems with purely per-CPU stacks if you run with interrupts
enabled. You theoretically ought to have a stack big enough to allow all
possible interrupts to be nested on one CPU.

And having non-uniform stack sizes of course introduces other
problems... notably the fact that you can no longer locate the thread_info
struct by means of AND-ing the stack pointer.

> there's a difference between bouncing 1-2 cachelines and bouncing a *full,
> dirtied stack*. The irq_desc[] bouncing is pretty much unavoidable (IRQs do
> need some global state) - the stack bouncing is just plain stupid and
> perfectly avoidable.

I wonder if it might be possible to invalidate just that bit of the
cache... though I suspect that's not worth it, even if it is.

David