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?

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

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

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

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.

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

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.

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 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)),

but for
broadcast irq the loop will be required.

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 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 :)

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

Or what is the
alternative?

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

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_work from the MSI path is still worthwhile, and probably more future-proof anyway.

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

One interface, multiple users (in the kernel, userspace, or other processes)

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

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 ioeventfd

Quoted 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.

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

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?

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

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.

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 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_work from 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.