Re: question about disabling interrupts for workqueue pool?

[Steven Rostedt]

On Tue, 2013-06-25 at 16:01 -0700, Tejun Heo wrote:
> Hello, Steven.
> 
> On Tue, Jun 25, 2013 at 06:52:34PM -0400, Steven Rostedt wrote:
> > Now my question is, are those local_irq_*() calls just for synchronizing
> > with sched RCU? If so, can you use rcu_read_lock_sched() instead?
> 
> Hmmm... using local_irq_disable/enable() is more consistent as
> irq-safe locks are used widely in workqueue.  Also, using
> rcu_read_lock_sched() would mean that we'll have
> spin_lock_irq_save/restore() nested inside preempt_disable/enable(),
> which is a bit silly in upstream kernel.

Why is that silly? It actually makes plenty of sense. Now if
preempt_disable/enable was nested in spin_lock_irq_save/restore() now
that would be pretty silly.

What I mean is, even in mainline, we want interrupts enable as much as
possible, as that will allow devices and such to have their interrupts
delivered in a timely manner. If we can get away with just disabling
preemption then doing so would be better. That's the point of softirq.
We run those with interrupts enabled but preemption disabled, and
there's several cases that we disable interrupts in a softirq with
spin_lock_irq_save/restore.

Just looking at the first part of that function:

	local_irq_disable();
	pool = get_work_pool(work);
	if (!pool) {
		local_irq_enable();
		return false;
	}

On the case of poll == NULL, we disabled interrupts for no reason.

> 
> That said, both paths you pointed out are rather cold, so I don't
> think it'd matter.  Please feel free to send a patch with comment
> explaining why local irq flipping, which fits better there, isn't
> being used.

It may take a bit of understanding the code before I send a patch. But
I'll start looking into it.

Thanks,

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/