Messages in this thread | | | Date | Fri, 19 Feb 2016 16:51:51 +0100 | From | Radim Krčmář <> | Subject | Re: [PATCH v2 02/14] KVM: x86: simplify atomics in kvm_pit_ack_irq |
| |
2016-02-18 19:04+0100, Paolo Bonzini: > On 17/02/2016 20:14, Radim Krčmář wrote: >> - value = atomic_dec_return(&ps->pending); >> - if (value < 0) >> - /* spurious acks can be generated if, for example, the >> - * PIC is being reset. Handle it gracefully here >> - */ >> - atomic_inc(&ps->pending); >> - else if (value > 0 && ps->reinject) >> - /* in this case, we had multiple outstanding pit interrupts >> - * that we needed to inject. Reinject >> - */ >> + if (atomic_dec_if_positive(&ps->pending) > 0 && ps->reinject) >> queue_kthread_work(&ps->pit->worker, &ps->pit->expired); > > Here it would have made sense to do already > > if (!ps->reinject) { > WARN_ON_ONCE(ps->pending || !ps->irq_ack); > return; > }
I will add the WARN_ON when removing discard notifiers.
> spin_lock(...) > if (atomic_dec_if_positive(&ps->pending) > 0) > queue_kthread_work(...); > ps->irq_ack = 1; > spin_unlock(...) > > because ps->pending is only ever nonzero, and irq_ack is only ever zero, > if ps->reinject.
(Well, userspace can switch between policies at runtime.)
> Not a big deal since the ack notifier is going to > disappear altogether for the discard policy, but the nice thing is that > it lets you remove the ack notifier earlier and disentangle a bit more > discard mode. > > So if you want for v3 you can reorder the patches like this:
The end result is going to be identical. I had a version that did something similar and it was pretty tangled as well -- I wanted to remove useless locks before re-using one for the ioctls. (We need the protection earlier, because userspace can control notifiers while PIT is still being initialized. And removing the lock had dependencies.)
> > - patch 1, same > > - patch 2, what is outlined above > > - patch 3, remove ack notifier for discard
I agree that current ordering looks weird. The dependency tree looked like this in my mind:
-[1/14] ,-[2/14] -[4/14] ,-[3/14] | ,-[5/14] | ,-[6/14] +-[7/14] -[8/14] -[9-14/14]
I added [2-4/14] early (and a bit out of order), because it made diffs shorter. Dependency on [7/14] can dropped with correct mutexing inside initialization, so the v3 order would be:
-[1/14] ,-[3/14] -[8/14] ,-[2/14] -[4/14] ,-[5/14] ,-[6/14] -[7/14] -[9-14/14]
With [8/14] (remove ack notifier for discard) as third.
Would that be ok?
> - patch 4..14 the rest > > Paolo
| |