mpi3mr: Don't hold fwevt_lock over call to taskqueue_drain

[imp]

Summary

Holding fwevt_lock when we call taskqueue_drain can lead to deadlock
because it's draining a queue needs fwevt_lock to do work, so that other
thread will try to take out the lock and block, making the thread never
finish and taskqueue_drain never complete. There's a witness
warning/error for this which was exposed when the lock was converted to
a MTX_DEF lock from a MTX_SPIN prior to committing to the FreeBSD tree.

The lock appears to be to protect against additional items being added
to the event list while we're doing a reset. Since the taskqueue is
blocked, items can get added to the list, but won't be processed during
the reset, but there is still a (likely small) race between the
taskqueue_drain and the taskqueue_block calls where an interrupt could
fire on another CPU, resulting in a task being enqueued and started
before the block can take effect. The only way to fix that race is to
turn off interrupt processing during a reset. So we replace a deadlock
with a smaller race.

Sponsored by: Netflix

[mav]

Mutex is definitely wrong there, and I think I actually hit the panic on it on one of just several reboots, but I wonder if something needs to protect from task addition between the drain and block, if it is important.

[imp]

In D42537#972004, @mav wrote:

Mutex is definitely wrong there, and I think I actually hit the panic on it on one of just several reboots, but I wonder if something needs to protect from task addition between the drain and block, if it is important.

I don't think it is important. There's a race in adding new events to this queue, but processing them before or after the reset I don't think matters. That's one thing I was hoping to get from the authors, though: what's the intent, why do we do it and what happens if we miss it.

I hit a panic sleeping with this lock several times.

[mav]

I guess the idea could be not try to handle events while resetting. I wonder if we could block new events before calling mpi3mr_cleanup_event_taskq(), unless we do it already.

sys/dev/mpi3mr/mpi3mr.c

5832

You could remove the return while here.

[jhb]

This is certainly better. I suspect you want to do the block before the drain. The block keeps the thread from getting rescheduled so that new tasks will pile up but not schedule the thread, and the drain waits for the thread to go idle. There's no need for a separate mutex since taskqueue uses its own internal mutex to handle TQ_FLAG_BLOCKED.

[mav]

@jhb Won't the block before drain cause deadlock? At least that is how I read the man page.

[imp]

In D42537#972311, @mav wrote:

@jhb Won't the block before drain cause deadlock? At least that is how I read the man page.

I've just spend the last 20 minutes convincing myself there's not a deadlock in this case.

In taskqueue_drain, we loop while the task_ta_pending != 0 && the task is on one of the thread's workers. The worker will set ta_pending to 0 run the task and then dequeue it causing the while loop in taskqueue_drain to terminate. We won't re-enqueue the task if TQ_FLAGS_BLOCKED is set, so I think for at least threads like we have in this driver the deadlock warned about cannot happen. But I'll let John be the final arbiter of that (and I'd like to know where I went wrong in my code reading too).

[imp]

make suggested changes
Add comment about block then drain.

[jhb]

In D42537#972311, @mav wrote:

@jhb Won't the block before drain cause deadlock? At least that is how I read the man page.

No block just sets a flag to ensure that new tasks that are queued aren't passed to the runner, just placed on an internal "pending" queue.

But, bleh, the manpage is confusing. The problem it describes is that if ev_task is queued before you block, but is still pending you might sleep forever.

I can see a couple of options, but really the API here is kind of crappy. I think what you probably want is a variant of taskqueue_quiesce() or taskqueue_drain_all() that only calls taskqueue_drain_tq_active() and you want to call that after taskqueue_block().

If you know for certain that nothing else can schedule the task when this is function is called, then the old code is ok, (but then I also don't know why you need to block the taskqueue). If you don't know that for certain and want any pending task cancelled, you could do:

taskqueue_block();
if (taskqueue_cancel() != 0)
  taskqueue_drain();

[imp]

In D42537#976129, @jhb wrote:

In D42537#972311, @mav wrote:

@jhb Won't the block before drain cause deadlock? At least that is how I read the man page.

No block just sets a flag to ensure that new tasks that are queued aren't passed to the runner, just placed on an internal "pending" queue.

But, bleh, the manpage is confusing. The problem it describes is that if ev_task is queued before you block, but is still pending you might sleep forever.

I can see a couple of options, but really the API here is kind of crappy. I think what you probably want is a variant of taskqueue_quiesce() or taskqueue_drain_all() that only calls taskqueue_drain_tq_active() and you want to call that after taskqueue_block().

If you know for certain that nothing else can schedule the task when this is function is called, then the old code is ok, (but then I also don't know why you need to block the taskqueue).

The old code hits a witness assert...

If you don't know that for certain and want any pending task cancelled, you could do:

taskqueue_block();
if (taskqueue_cancel() != 0)
  taskqueue_drain();

OK. But folding it back gave me conflicts so I'm going to save a minute and just tweak it at the end... last set of conflicts I accidentally folded the dmamask and the inprogress commits

[jhb]

By old code I mean if you remove the lock only and just leave it as taskqueue_drain() followed by taskqueue_block(), sorry for the confusion.