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

Message-ID: <1372200754.18733.236.camel@gandalf.local.home>
Date:	Tue, 25 Jun 2013 18:52:34 -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: question about disabling interrupts for workqueue pool?

Hi Tejun,

We've been playing with porting -rt to 3.10, and one of the stumbling
blocks we are dealing with happens to be with commit fa1b54e69bc
"workqueue: update synchronization rules on worker_pool_idr".

The issue is with the disabling of interrupts, and specifically this
sequence of events from start_flush_work():

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

        spin_lock(&pool->lock);
        /* see the comment in try_to_grab_pending() with the same code */
        pwq = get_work_pwq(work);
        if (pwq) {
                if (unlikely(pwq->pool != pool))
                        goto already_gone;
        } else {
                worker = find_worker_executing_work(pool, work);
                if (!worker)
                        goto already_gone;
                pwq = worker->current_pwq;
        }

        insert_wq_barrier(pwq, barr, work, worker);
        spin_unlock_irq(&pool->lock);

As -rt does not have spin_lock_irq() disable interrupts, it means that
spin_unlock_irq() will not enable them either. But that's not all:

        local_irq_save(flags);
        pool = get_work_pool(work);
        if (pool) {
                spin_lock(&pool->lock);
                if (find_worker_executing_work(pool, work))
                        ret |= WORK_BUSY_RUNNING;
                spin_unlock(&pool->lock);
        }
        local_irq_restore(flags);

Basically, anywhere you disable interrupts, as -rt converts spin_lock()
into a mutex.

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?

This wont solve our problems, as we will need to deal with
rcu_read_lock_sched() critical sections sleeping, but it will at least
change the problem area away from workqueues, and will document the
workqueue code better because stand alone "local_irq_disable()"s tend to
bring up a lot of questions to "why do we need to disable interrupts
here?".

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