question about disabling interrupts for workqueue pool?

question about disabling interrupts for workqueue pool?

[Steven Rostedt]

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

Re: question about disabling interrupts for workqueue pool?

[Tejun Heo]

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.

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.

Thanks.

-- 
tejun

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

Re: question about disabling interrupts for workqueue pool?

[Tejun Heo]

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