linux-kernel - Re: question about disabling interrupts for workqueue pool?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 25 Jun 2013 19:19:04 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Tejun Heo <tj@...nel.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	RT <linux-rt-users@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Clark Williams <clark@...hat.com>,
	"Paul E. McKenney" <paulmck@...ibm.com>
Subject: Re: question about disabling interrupts for workqueue pool?

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/