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] [day] [month] [year] [list]

Message-ID: <20130625232657.GC30407@mtj.dyndns.org>
Date:	Tue, 25 Jun 2013 16:26:57 -0700
From:	Tejun Heo <tj@...nel.org>
To:	Steven Rostedt <rostedt@...dmis.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?

Hello, Steven.

On Tue, Jun 25, 2013 at 07:19:04PM -0400, Steven Rostedt wrote:
> 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.

If you know you're gonna be disabling irq pretty soon, you don't need
to do that, so...

> 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.

It's much more likely that get_work_pool() there returns !NULL.  I
didn't think it'd matter enough to put likely().  Sure, it's nice to
not disable interrupts but really, in upstream, I don't think the
above matters in the upstream kernel.  The extra coverage is at the
worst idr_find() into single level idr.

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

Wrapping from local_irq_disable() to spin_unlock_irq() with RCU sched
read lock/unlock should do, I think.

Thanks!

-- 
tejun
--
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/