assign-dev: Purpose of interrupt_work
Hi,
I was starring at the IRQ delivery path of assigned devices for a while, wondering why we have a work queue there. Now, after looking at some prehistoric versions, I think the reason is that there once was a mutex involved while we now use RCU. Am I right that we could actually drop this indirection today?
Jan
Quoted 8 lines · Jan Kiszka
Hi,
I was starring at the IRQ delivery path of assigned devices for a while, wondering why we have a work queue there. Now, after looking at some prehistoric versions, I think the reason is that there once was a mutex involved while we now use RCU. Am I right that we could actually drop this indirection today?
ioapic/pic path still has mutex. If MSIX is used (like it should) we can drop work queue.
--
Gleb.
Quoted 11 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
Hi,
I was starring at the IRQ delivery path of assigned devices for a while, wondering why we have a work queue there. Now, after looking at some prehistoric versions, I think the reason is that there once was a mutex involved while we now use RCU. Am I right that we could actually drop this indirection today?
ioapic/pic path still has mutex. If MSIX is used (like it should) we can drop work queue.
I see. Wouldn't it be feasible to convert the fast paths of [io]apic to spinlocks?
Jan
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
Hi,
I was starring at the IRQ delivery path of assigned devices for a while, wondering why we have a work queue there. Now, after looking at some prehistoric versions, I think the reason is that there once was a mutex involved while we now use RCU. Am I right that we could actually drop this indirection today?
Quoted 6 lines
Quoted 2 lines
ioapic/pic path still has mutex. If MSIX is used (like it should) we can drop work queue.
I see. Wouldn't it be feasible to convert the fast paths of [io]apic to spinlocks?
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
--
Gleb.
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Jan Kiszka
Quoted 11 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
Hi,
I was starring at the IRQ delivery path of assigned devices for a while, wondering why we have a work queue there. Now, after looking at some prehistoric versions, I think the reason is that there once was a mutex involved while we now use RCU. Am I right that we could actually drop this indirection today?
ioapic/pic path still has mutex. If MSIX is used (like it should) we can drop work queue.
Quoted 7 lines
Quoted 3 lines
I see. Wouldn't it be feasible to convert the fast paths of [io]apic to spinlocks?
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Well, lockless is another thing.
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.
Jan
Quoted 22 lines · Jan Kiszka
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Jan Kiszka
Quoted 11 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
Hi,
I was starring at the IRQ delivery path of assigned devices for a while, wondering why we have a work queue there. Now, after looking at some prehistoric versions, I think the reason is that there once was a mutex involved while we now use RCU. Am I right that we could actually drop this indirection today?
ioapic/pic path still has mutex. If MSIX is used (like it should) we can drop work queue.
Quoted 7 lines
Quoted 3 lines
I see. Wouldn't it be feasible to convert the fast paths of [io]apic to spinlocks?
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Quoted 7 lines
Well, lockless is another thing.
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.
Avi's point in favor of mutex is: they are as fast as spinlocks when congested and allows preemption when held.
--
Gleb.
Quoted 24 lines · Gleb Natapov
Quoted 23 lines · Jan Kiszka
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Jan Kiszka
Quoted 11 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
Hi,
I was starring at the IRQ delivery path of assigned devices for a while, wondering why we have a work queue there. Now, after looking at some prehistoric versions, I think the reason is that there once was a mutex involved while we now use RCU. Am I right that we could actually drop this indirection today?
ioapic/pic path still has mutex. If MSIX is used (like it should) we can drop work queue.
Quoted 7 lines
Quoted 3 lines
I see. Wouldn't it be feasible to convert the fast paths of [io]apic to spinlocks?
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Well, lockless is another thing.
Quoted 7 lines
Quoted 5 lines
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.Avi's point in favor of mutex is: they are as fast as spinlocks when congested and allows preemption when held.
...but require scheduler activity in case of dev-passthrough, which is surely playing in a different league.
Jan
Quoted 11 lines · Jan Kiszka
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Well, lockless is another thing.
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.
Quoted 6 lines
Quoted 2 lines
Avi's point in favor of mutex is: they are as fast as spinlocks when congested and allows preemption when held.
...but require scheduler activity in case of dev-passthrough, which is surely playing in a different league.
I'd rather remove dev-passthrough completely than continue adding hack upon hack upon hack to make is some times kinda sorta work :)
--
Gleb.
Quoted 12 lines · Gleb Natapov
Quoted 11 lines · Jan Kiszka
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Well, lockless is another thing.
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Avi's point in favor of mutex is: they are as fast as spinlocks when congested and allows preemption when held.
...but require scheduler activity in case of dev-passthrough, which is surely playing in a different league.
I'd rather remove dev-passthrough completely than continue adding hack upon hack upon hack to make is some times kinda sorta work :)
Hmm, is this code not needed for the VT-d & Co. case? Or what is the alternative?
Jan
Quoted 4 lines · Jan Kiszka
Quoted 3 lines
I'd rather remove dev-passthrough completely than continue adding hack upon hack upon hack to make is some times kinda sorta work :)
Hmm, is this code not needed for the VT-d& Co. case?
It is needed.
Quoted 3 lines
Or what is the
alternative?
irqfd (which only supports edge-triggered interrupts now). Note irqfd needs the same love, it uses a workqueue as well.
The theory is:
fd1 = eventfd()
give fd1 to kvm as irqfd, vhost-net as trigger fd fd2 = eventfd()
give fd2 to kvm as irqfd, uio as trigger fd fd3 = evenfd()
give fd3 to kvm as irqfd, another kvm as ioeventfd
One interface, multiple users (in the kernel, userspace, or other processes)
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 24 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 4 lines
I'd rather remove dev-passthrough completely than continue adding hack upon hack
upon hack to make is some times kinda sorta work :)Hmm, is this code not needed for the VT-d& Co. case?
It is needed.
Quoted 3 lines
Or what is the
alternative?irqfd (which only supports edge-triggered interrupts now). Note irqfd needs the same love, it uses a workqueue as well.
The theory is:
fd1 =eventfd()
give fd1 to kvm as irqfd, vhost-net as trigger fd fd2 =eventfd()
give fd2 to kvm as irqfd, uio as trigger fd fd3 =evenfd()
give fd3 to kvm as irqfd, another kvm as ioeventfd
Quoted 2 lines
One interface, multiple users (in the kernel, userspace, or other processes)
I see to overall gain, but likely only worsens my objective (low latency), at least it doesn't improve it.
Jan
Quoted 15 lines · Jan Kiszka
Quoted 15 lines
Quoted 4 lines
Or what is the
alternative?irqfd (which only supports edge-triggered interrupts now). Note irqfd needs the same love, it uses a workqueue as well.
The theory is:
fd1 =eventfd()
give fd1 to kvm as irqfd, vhost-net as trigger fd fd2 =eventfd()
give fd2 to kvm as irqfd, uio as trigger fd fd3 =evenfd()
give fd3 to kvm as irqfd, another kvm as ioeventfd
Quoted 6 lines
Quoted 3 lines
One interface, multiple users (in the kernel, userspace, or other processes)
I see to overall gain, but likely only worsens my objective (low latency), at least it doesn't improve it.
irqfd can work from interrupt context, so if we only use the workqueue for the many-vcpu cases, we should be fine.
I think we can do it cleanly, too: in ioapic code, if we're talking to one vcpu, wake it immediately; otherwise schedule work to continue in process context. This was the common code can always work in interrupt context and not worry about the work queue.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 20 lines · Avi Kivity
Quoted 14 lines · Jan Kiszka
Quoted 14 lines
Quoted 3 lines
Or what is the
alternative?irqfd (which only supports edge-triggered interrupts now). Note irqfd needs the same love, it uses a workqueue as well.
The theory is:
fd1 =eventfd()
give fd1 to kvm as irqfd, vhost-net as trigger fd fd2 =eventfd()
give fd2 to kvm as irqfd, uio as trigger fd fd3 =evenfd()
give fd3 to kvm as irqfd, another kvm as ioeventfdQuoted 4 lines
Quoted 2 lines
One interface, multiple users (in the kernel, userspace, or other processes)
I see to overall gain, but likely only worsens my objective (low latency), at least it doesn't improve it.
Quoted 8 lines
irqfd can work from interrupt context, so if we only use the workqueue for the many-vcpu cases, we should be fine.
I think we can do it cleanly, too: in ioapic code, if we're talking to one vcpu, wake it immediately; otherwise schedule work to continue in process context. This was the common code can always work in interrupt context and not worry about the work queue.
Even if we are talking to one cpu locking is still needed, so unless we make it spinlock we can't inject from irq context. If we change mutex to spinlock what is the point of work queue?
--
Gleb.
Quoted 10 lines · Gleb Natapov
Quoted 10 lines
irqfd can work from interrupt context, so if we only use the workqueue for the many-vcpu cases, we should be fine.
I think we can do it cleanly, too: in ioapic code, if we're talking to one vcpu, wake it immediately; otherwise schedule work to continue in process context. This was the common code can always work in interrupt context and not worry about the work queue.
Quoted 4 lines
Even if we are talking to one cpu locking is still needed, so unless we make it spinlock we can't inject from irq context. If we change mutex to spinlock what is the point of work queue?
Not to loop over 4096 vcpus with interrupts disabled, triggered by guest action.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 10 lines · Avi Kivity
Quoted 9 lines · Gleb Natapov
Quoted 9 lines
irqfd can work from interrupt context, so if we only use the workqueue for the many-vcpu cases, we should be fine.
I think we can do it cleanly, too: in ioapic code, if we're talking to one vcpu, wake it immediately; otherwise schedule work to continue in process context. This was the common code can always work in interrupt context and not worry about the work queue.
Quoted 7 lines
Quoted 3 lines
Even if we are talking to one cpu locking is still needed, so unless we make it spinlock we can't inject from irq context. If we change mutex to spinlock what is the point of work queue?
Not to loop over 4096 vcpus with interrupts disabled, triggered by guest action.
But in a work queue you'll do this anyway. You can't access ioapic without the lock.
--
Gleb.
Quoted 15 lines · Jan Kiszka
Quoted 14 lines · Gleb Natapov
Quoted 13 lines · Jan Kiszka
Quoted 13 lines
Quoted 11 lines
Quoted 4 lines
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Well, lockless is another thing.
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.Avi's point in favor of mutex is: they are as fast as spinlocks when congested and allows preemption when held.
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
...but require scheduler activity in case of dev-passthrough, which is surely playing in a different league.
I'd rather remove dev-passthrough completely than continue adding hack upon hack upon hack to make is some times kinda sorta work :)
Hmm, is this code not needed for the VT-d & Co. case? Or what is the alternative?
The smile there is for a reason. My dislike of dev-passthrough will not make it go away. Alternatives? I don't know what HW makers plans are, but with interrupt remapping + APIC vitalization in HW theoretically it is possible to inject interrupts directly from device to virtual machine.
--
Gleb.
Quoted 5 lines · Jan Kiszka
Quoted 5 lines
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Quoted 7 lines
Well, lockless is another thing.
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.
I'm worried about disabling irqs for non-device-assignment cases. It would be more palatable if ioapic was completely O(1) (there are some per-vcpu loops in there, shouldn't be too bad for 16 vcpus, but we want to scale).
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 12 lines · Avi Kivity
Quoted 10 lines · Jan Kiszka
Quoted 4 lines
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Well, lockless is another thing.
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.
Quoted 5 lines
I'm worried about disabling irqs for non-device-assignment cases. It would be more palatable if ioapic was completely O(1) (there are some per-vcpu loops in there, shouldn't be too bad for 16 vcpus, but we want to scale).
We can change it to hash for directed irqs (still not O(1)), but for broadcast irq the loop will be required.
--
Gleb.
Quoted 8 lines · Gleb Natapov
Quoted 7 lines
I'm worried about disabling irqs for non-device-assignment cases. It would be more palatable if ioapic was completely O(1) (there are some per-vcpu loops in there, shouldn't be too bad for 16 vcpus, but we want to scale).
We can change it to hash for directed irqs (still not O(1)),
Or cache the vcpu pointer in the redirection table entry.
Quoted 3 lines
but for
broadcast irq the loop will be required.
That's fine, those are rare.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 14 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 5 lines
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Quoted 7 lines
Well, lockless is another thing.
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.
Quoted 4 lines
I'm worried about disabling irqs for non-device-assignment cases. It would be more palatable if ioapic was completely O(1) (there are some per-vcpu loops in there, shouldn't be too bad for 16 vcpus, but we want to scale).
Yeah, what a pity. That's likely not solvable in a generic way, given that the guest finally decided how many VCPUs may listen to a line.
OK, but dropping interrupt_work from the MSI path is still worthwhile, and probably more future-proof anyway.
Jan
Quoted 11 lines · Jan Kiszka
Quoted 5 lines
I'm worried about disabling irqs for non-device-assignment cases. It would be more palatable if ioapic was completely O(1) (there are some per-vcpu loops in there, shouldn't be too bad for 16 vcpus, but we want to scale).
Yeah, what a pity. That's likely not solvable in a generic way, given that the guest finally decided how many VCPUs may listen to a line.
OK, but dropping
interrupt_workfrom the MSI path is still worthwhile, and probably more future-proof anyway.
Yes - we can extend the irq routing table to contain this information. It means we need to invalidate it when the guest reprograms the interrupt controllers, but I think it should work out fine.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 20 lines · Jan Kiszka
Quoted 14 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 5 lines
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Quoted 7 lines
Well, lockless is another thing.
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.Quoted 4 lines
I'm worried about disabling irqs for non-device-assignment cases. It would be more palatable if ioapic was completely O(1) (there are some per-vcpu loops in there, shouldn't be too bad for 16 vcpus, but we want to scale).
Quoted 5 lines
Yeah, what a pity. That's likely not solvable in a generic way, given that the guest finally decided how many VCPUs may listen to a line.
OK, but dropping
interrupt_workfrom the MSI path is still worthwhile, and probably more future-proof anyway.
Seems appropriate to convert the process context work to threaded interrupt (instead of workqueue). That should help latency.
Quoted 19 lines · Marcelo Tosatti
Quoted 18 lines · Jan Kiszka
Quoted 17 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 5 lines
Apic is lockless. For ioapic/pic I used spinlocks initially, but Avi prefers mutexes. Theoretically it is possible to make them lockless, but code will be complex and eventually more slow, since more then two atomic operation will be used on irq injection path.
Quoted 7 lines
Well, lockless is another thing.
But also converting to spinlocks would indeed add some overhead:
irqsave/restore. But I wonder if this isn't worth it, at least when looking at the (supposed to be fast) device passthrough scenario which would be simpler and faster.I'm worried about disabling irqs for non-device-assignment cases. It would be more palatable if ioapic was completely O(1) (there are some per-vcpu loops in there, shouldn't be too bad for 16 vcpus, but we want to scale).
Quoted 9 lines
Quoted 5 lines
Yeah, what a pity. That's likely not solvable in a generic way, given that the guest finally decided how many VCPUs may listen to a line.
OK, but dropping
interrupt_workfrom the MSI path is still worthwhile, and probably more future-proof anyway.Seems appropriate to convert the process context work to threaded interrupt (instead of workqueue). That should help latency.
Not for the trivial case (I want to avoid scheduling as far as possible).
Jan