| Message ID | 20210413054733.36363-2-mst@redhat.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | virtio net: spurious interrupt related fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 3 of 3 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/kdoc | success | Errors and warnings before: 1 this patch: 1 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: 'interupt' may be misspelled - perhaps 'interrupt'? WARNING: line length of 88 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/header_inline | success | Link |
在 2021/4/13 下午1:47, Michael S. Tsirkin 写道: > virtio_disable_cb is currently a nop for split ring with event index. > This is because it used to be always called from a callback when we know > device won't trigger more events until we update the index. However, > now that we run with interrupts enabled a lot we also poll without a > callback so that is different: disabling callbacks will help reduce the > number of spurious interrupts. > Further, if using event index with a packed ring, and if being called > from a callback, we actually do disable interrupts which is unnecessary. > > Fix both issues by tracking whenever we get a callback. If that is > the case disabling interrupts with event index can be a nop. > If not the case disable interrupts. Note: with a split ring > there's no explicit "no interrupts" value. For now we write > a fixed value so our chance of triggering an interupt > is 1/ring size. It's probably better to write something > related to the last used index there to reduce the chance > even further. For now I'm keeping it simple. So I wonder whether last_used_idx is better, it looks to me the device will never move used index "across" that: https://lore.kernel.org/patchwork/patch/946475/ And it looks to me it's better to move the optimization (event_triggered) into a separated patch. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > drivers/virtio/virtio_ring.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 71e16b53e9c1..88f0b16b11b8 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -113,6 +113,9 @@ struct vring_virtqueue { > /* Last used index we've seen. */ > u16 last_used_idx; > > + /* Hint for event idx: already triggered no need to disable. */ > + bool event_triggered; > + > union { > /* Available for split ring */ > struct { > @@ -739,7 +742,10 @@ static void virtqueue_disable_cb_split(struct virtqueue *_vq) > > if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) { > vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; > - if (!vq->event) > + if (vq->event) > + /* TODO: this is a hack. Figure out a cleaner value to write. */ > + vring_used_event(&vq->split.vring) = 0x0; > + else > vq->split.vring.avail->flags = > cpu_to_virtio16(_vq->vdev, > vq->split.avail_flags_shadow); > @@ -1605,6 +1611,7 @@ static struct virtqueue *vring_create_virtqueue_packed( > vq->weak_barriers = weak_barriers; > vq->broken = false; > vq->last_used_idx = 0; > + vq->event_triggered = false; > vq->num_added = 0; > vq->packed_ring = true; > vq->use_dma_api = vring_use_dma_api(vdev); > @@ -1919,6 +1926,12 @@ void virtqueue_disable_cb(struct virtqueue *_vq) > { > struct vring_virtqueue *vq = to_vvq(_vq); > > + /* If device triggered an event already it won't trigger one again: > + * no need to disable. > + */ > + if (vq->event_triggered) > + return; I guess we nee to check vq->event as well. Thanks > + > if (vq->packed_ring) > virtqueue_disable_cb_packed(_vq); > else > @@ -1942,6 +1955,9 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq) > { > struct vring_virtqueue *vq = to_vvq(_vq); > > + if (vq->event_triggered) > + vq->event_triggered = false; > + > return vq->packed_ring ? virtqueue_enable_cb_prepare_packed(_vq) : > virtqueue_enable_cb_prepare_split(_vq); > } > @@ -2005,6 +2021,9 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq) > { > struct vring_virtqueue *vq = to_vvq(_vq); > > + if (vq->event_triggered) > + vq->event_triggered = false; > + > return vq->packed_ring ? virtqueue_enable_cb_delayed_packed(_vq) : > virtqueue_enable_cb_delayed_split(_vq); > } > @@ -2044,6 +2063,10 @@ irqreturn_t vring_interrupt(int irq, void *_vq) > if (unlikely(vq->broken)) > return IRQ_HANDLED; > > + /* Just a hint for performance: so it's ok that this can be racy! */ > + if (vq->event) > + vq->event_triggered = true; > + > pr_debug("virtqueue callback for %p (%p)\n", vq, vq->vq.callback); > if (vq->vq.callback) > vq->vq.callback(&vq->vq); > @@ -2083,6 +2106,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, > vq->weak_barriers = weak_barriers; > vq->broken = false; > vq->last_used_idx = 0; > + vq->event_triggered = false; > vq->num_added = 0; > vq->use_dma_api = vring_use_dma_api(vdev); > #ifdef DEBUG
On Tue, Apr 13, 2021 at 1:47 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > virtio_disable_cb is currently a nop for split ring with event index. > This is because it used to be always called from a callback when we know > device won't trigger more events until we update the index. However, > now that we run with interrupts enabled a lot we also poll without a > callback so that is different: disabling callbacks will help reduce the > number of spurious interrupts. The device may poll for transmit completions as a result of an interrupt from virtnet_poll_tx. As well as asynchronously to this transmit interrupt, from start_xmit or from virtnet_poll_cleantx as a result of a receive interrupt. As of napi-tx, transmit interrupts are left enabled to operate in standard napi mode. While previously they would be left disabled for most of the time, enabling only when the queue as low on descriptors. (in practice, for the at the time common case of split ring with event index, little changed, as that mode does not actually enable/disable the interrupt, but looks at the consumer index in the ring to decide whether to interrupt) Combined, this may cause the following: 1. device sends a packet and fires transmit interrupt 2. driver cleans interrupts using virtnet_poll_cleantx 3. driver handles transmit interrupt using vring_interrupt, detects that the vring is empty: !more_used(vq), and records a spurious interrupt. I don't quite follow how suppressing interrupt suppression, i.e., skipping disable_cb, helps avoid this. I'm probably missing something. Is this solving a subtly different problem from the one as I understand it? > Further, if using event index with a packed ring, and if being called > from a callback, we actually do disable interrupts which is unnecessary. > > Fix both issues by tracking whenever we get a callback. If that is > the case disabling interrupts with event index can be a nop. > If not the case disable interrupts. Note: with a split ring > there's no explicit "no interrupts" value. For now we write > a fixed value so our chance of triggering an interupt > is 1/ring size. It's probably better to write something > related to the last used index there to reduce the chance > even further. For now I'm keeping it simple. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
On Tue, Apr 13, 2021 at 10:01:11AM -0400, Willem de Bruijn wrote: > On Tue, Apr 13, 2021 at 1:47 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > virtio_disable_cb is currently a nop for split ring with event index. > > This is because it used to be always called from a callback when we know > > device won't trigger more events until we update the index. However, > > now that we run with interrupts enabled a lot we also poll without a > > callback so that is different: disabling callbacks will help reduce the > > number of spurious interrupts. > > The device may poll for transmit completions as a result of an interrupt > from virtnet_poll_tx. > > As well as asynchronously to this transmit interrupt, from start_xmit or > from virtnet_poll_cleantx as a result of a receive interrupt. > > As of napi-tx, transmit interrupts are left enabled to operate in standard > napi mode. While previously they would be left disabled for most of the > time, enabling only when the queue as low on descriptors. > > (in practice, for the at the time common case of split ring with event index, > little changed, as that mode does not actually enable/disable the interrupt, > but looks at the consumer index in the ring to decide whether to interrupt) > > Combined, this may cause the following: > > 1. device sends a packet and fires transmit interrupt > 2. driver cleans interrupts using virtnet_poll_cleantx > 3. driver handles transmit interrupt using vring_interrupt, > detects that the vring is empty: !more_used(vq), > and records a spurious interrupt. > > I don't quite follow how suppressing interrupt suppression, i.e., > skipping disable_cb, helps avoid this. > I'm probably missing something. Is this solving a subtly different > problem from the one as I understand it? I was thinking of this one: 1. device is sending packets 2. driver cleans them at the same time using virtnet_poll_cleantx 3. device fires transmit interrupts 4. driver handles transmit interrupts using vring_interrupt, detects that the vring is empty: !more_used(vq), and records spurious interrupts. but even yours is also fixed I think. The common point is that a single spurious interrupt is not a problem. The problem only exists if there are tons of spurious interrupts with no real ones. For this to trigger, we keep polling the ring and while we do device keeps firing interrupts. So just disable interrupts while we poll. > > Further, if using event index with a packed ring, and if being called > > from a callback, we actually do disable interrupts which is unnecessary. > > > > Fix both issues by tracking whenever we get a callback. If that is > > the case disabling interrupts with event index can be a nop. > > If not the case disable interrupts. Note: with a split ring > > there's no explicit "no interrupts" value. For now we write > > a fixed value so our chance of triggering an interupt > > is 1/ring size. It's probably better to write something > > related to the last used index there to reduce the chance > > even further. For now I'm keeping it simple. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
On Tue, Apr 13, 2021 at 3:54 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Apr 13, 2021 at 10:01:11AM -0400, Willem de Bruijn wrote: > > On Tue, Apr 13, 2021 at 1:47 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > virtio_disable_cb is currently a nop for split ring with event index. > > > This is because it used to be always called from a callback when we know > > > device won't trigger more events until we update the index. However, > > > now that we run with interrupts enabled a lot we also poll without a > > > callback so that is different: disabling callbacks will help reduce the > > > number of spurious interrupts. > > > > The device may poll for transmit completions as a result of an interrupt > > from virtnet_poll_tx. > > > > As well as asynchronously to this transmit interrupt, from start_xmit or > > from virtnet_poll_cleantx as a result of a receive interrupt. > > > > As of napi-tx, transmit interrupts are left enabled to operate in standard > > napi mode. While previously they would be left disabled for most of the > > time, enabling only when the queue as low on descriptors. > > > > (in practice, for the at the time common case of split ring with event index, > > little changed, as that mode does not actually enable/disable the interrupt, > > but looks at the consumer index in the ring to decide whether to interrupt) > > > > Combined, this may cause the following: > > > > 1. device sends a packet and fires transmit interrupt > > 2. driver cleans interrupts using virtnet_poll_cleantx > > 3. driver handles transmit interrupt using vring_interrupt, > > detects that the vring is empty: !more_used(vq), > > and records a spurious interrupt. > > > > I don't quite follow how suppressing interrupt suppression, i.e., > > skipping disable_cb, helps avoid this. > > I'm probably missing something. Is this solving a subtly different > > problem from the one as I understand it? > > I was thinking of this one: > > 1. device is sending packets > 2. driver cleans them at the same time using virtnet_poll_cleantx > 3. device fires transmit interrupts > 4. driver handles transmit interrupts using vring_interrupt, > detects that the vring is empty: !more_used(vq), > and records spurious interrupts. I think that's the same scenario > > > but even yours is also fixed I think. > > The common point is that a single spurious interrupt is not a problem. > The problem only exists if there are tons of spurious interrupts with no > real ones. For this to trigger, we keep polling the ring and while we do > device keeps firing interrupts. So just disable interrupts while we > poll. But the main change in this patch is to turn some virtqueue_disable_cb calls into no-ops. I don't understand how that helps reduce spurious interrupts, as if anything, it keeps interrupts enabled for longer. Another patch in the series disable callbacks* before starting to clean the descriptors from the rx interrupt. That I do understand will suppress additional tx interrupts that might see no work to be done. I just don't entire follow this patch on its own. *(I use interrupt and callback as a synonym in this context, correct me if I'm glancing over something essential)
On Tue, Apr 13, 2021 at 05:44:42PM -0400, Willem de Bruijn wrote: > On Tue, Apr 13, 2021 at 3:54 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Apr 13, 2021 at 10:01:11AM -0400, Willem de Bruijn wrote: > > > On Tue, Apr 13, 2021 at 1:47 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > virtio_disable_cb is currently a nop for split ring with event index. > > > > This is because it used to be always called from a callback when we know > > > > device won't trigger more events until we update the index. However, > > > > now that we run with interrupts enabled a lot we also poll without a > > > > callback so that is different: disabling callbacks will help reduce the > > > > number of spurious interrupts. > > > > > > The device may poll for transmit completions as a result of an interrupt > > > from virtnet_poll_tx. > > > > > > As well as asynchronously to this transmit interrupt, from start_xmit or > > > from virtnet_poll_cleantx as a result of a receive interrupt. > > > > > > As of napi-tx, transmit interrupts are left enabled to operate in standard > > > napi mode. While previously they would be left disabled for most of the > > > time, enabling only when the queue as low on descriptors. > > > > > > (in practice, for the at the time common case of split ring with event index, > > > little changed, as that mode does not actually enable/disable the interrupt, > > > but looks at the consumer index in the ring to decide whether to interrupt) > > > > > > Combined, this may cause the following: > > > > > > 1. device sends a packet and fires transmit interrupt > > > 2. driver cleans interrupts using virtnet_poll_cleantx > > > 3. driver handles transmit interrupt using vring_interrupt, > > > detects that the vring is empty: !more_used(vq), > > > and records a spurious interrupt. > > > > > > I don't quite follow how suppressing interrupt suppression, i.e., > > > skipping disable_cb, helps avoid this. > > > I'm probably missing something. Is this solving a subtly different > > > problem from the one as I understand it? > > > > I was thinking of this one: > > > > 1. device is sending packets > > 2. driver cleans them at the same time using virtnet_poll_cleantx > > 3. device fires transmit interrupts > > 4. driver handles transmit interrupts using vring_interrupt, > > detects that the vring is empty: !more_used(vq), > > and records spurious interrupts. > > I think that's the same scenario Not a big difference I agree. > > > > > > but even yours is also fixed I think. > > > > The common point is that a single spurious interrupt is not a problem. > > The problem only exists if there are tons of spurious interrupts with no > > real ones. For this to trigger, we keep polling the ring and while we do > > device keeps firing interrupts. So just disable interrupts while we > > poll. > > But the main change in this patch is to turn some virtqueue_disable_cb > calls into no-ops. Well this was not the design. This is the main change: @@ -739,7 +742,10 @@ static void virtqueue_disable_cb_split(struct virtqueue *_vq) if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) { vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; - if (!vq->event) + if (vq->event) + /* TODO: this is a hack. Figure out a cleaner value to write. */ + vring_used_event(&vq->split.vring) = 0x0; + else vq->split.vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->split.avail_flags_shadow); IIUC previously when event index was enabled (vq->event) virtqueue_disable_cb_split was a nop. Now it sets index to 0x0 (which is a hack, but good enough for testing I think). > I don't understand how that helps reduce spurious > interrupts, as if anything, it keeps interrupts enabled for longer. > > Another patch in the series disable callbacks* before starting to > clean the descriptors from the rx interrupt. That I do understand will > suppress additional tx interrupts that might see no work to be done. I > just don't entire follow this patch on its own. > > *(I use interrupt and callback as a synonym in this context, correct > me if I'm glancing over something essential) It's the same for the pci transport.
> > > > > > > > > but even yours is also fixed I think. > > > > > > The common point is that a single spurious interrupt is not a problem. > > > The problem only exists if there are tons of spurious interrupts with no > > > real ones. For this to trigger, we keep polling the ring and while we do > > > device keeps firing interrupts. So just disable interrupts while we > > > poll. > > > > But the main change in this patch is to turn some virtqueue_disable_cb > > calls into no-ops. > > Well this was not the design. This is the main change: > > > @@ -739,7 +742,10 @@ static void virtqueue_disable_cb_split(struct virtqueue *_vq) > > if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) { > vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; > - if (!vq->event) > + if (vq->event) > + /* TODO: this is a hack. Figure out a cleaner value to write. */ > + vring_used_event(&vq->split.vring) = 0x0; > + else > vq->split.vring.avail->flags = > cpu_to_virtio16(_vq->vdev, > vq->split.avail_flags_shadow); > > > IIUC previously when event index was enabled (vq->event) virtqueue_disable_cb_split > was a nop. Now it sets index to 0x0 (which is a hack, but good enough > for testing I think). So now tx interrupts will really be suppressed even in event-idx mode. And what is the purpose of suppressing this operation if event_triggered, i.e., after an interrupt occurred? You mention " if using event index with a packed ring, and if being called from a callback, we actually do disable interrupts which is unnecessary." Can you elaborate? Also, even if unnecessary, does it matter? The operation itself seems fairly cheap. These should probably be two separate patches. There is also a third case, split ring without event index. That behaves more like packed ring, I suppose. > > I don't understand how that helps reduce spurious > > interrupts, as if anything, it keeps interrupts enabled for longer.
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 71e16b53e9c1..88f0b16b11b8 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -113,6 +113,9 @@ struct vring_virtqueue { /* Last used index we've seen. */ u16 last_used_idx; + /* Hint for event idx: already triggered no need to disable. */ + bool event_triggered; + union { /* Available for split ring */ struct { @@ -739,7 +742,10 @@ static void virtqueue_disable_cb_split(struct virtqueue *_vq) if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) { vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; - if (!vq->event) + if (vq->event) + /* TODO: this is a hack. Figure out a cleaner value to write. */ + vring_used_event(&vq->split.vring) = 0x0; + else vq->split.vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->split.avail_flags_shadow); @@ -1605,6 +1611,7 @@ static struct virtqueue *vring_create_virtqueue_packed( vq->weak_barriers = weak_barriers; vq->broken = false; vq->last_used_idx = 0; + vq->event_triggered = false; vq->num_added = 0; vq->packed_ring = true; vq->use_dma_api = vring_use_dma_api(vdev); @@ -1919,6 +1926,12 @@ void virtqueue_disable_cb(struct virtqueue *_vq) { struct vring_virtqueue *vq = to_vvq(_vq); + /* If device triggered an event already it won't trigger one again: + * no need to disable. + */ + if (vq->event_triggered) + return; + if (vq->packed_ring) virtqueue_disable_cb_packed(_vq); else @@ -1942,6 +1955,9 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq) { struct vring_virtqueue *vq = to_vvq(_vq); + if (vq->event_triggered) + vq->event_triggered = false; + return vq->packed_ring ? virtqueue_enable_cb_prepare_packed(_vq) : virtqueue_enable_cb_prepare_split(_vq); } @@ -2005,6 +2021,9 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq) { struct vring_virtqueue *vq = to_vvq(_vq); + if (vq->event_triggered) + vq->event_triggered = false; + return vq->packed_ring ? virtqueue_enable_cb_delayed_packed(_vq) : virtqueue_enable_cb_delayed_split(_vq); } @@ -2044,6 +2063,10 @@ irqreturn_t vring_interrupt(int irq, void *_vq) if (unlikely(vq->broken)) return IRQ_HANDLED; + /* Just a hint for performance: so it's ok that this can be racy! */ + if (vq->event) + vq->event_triggered = true; + pr_debug("virtqueue callback for %p (%p)\n", vq, vq->vq.callback); if (vq->vq.callback) vq->vq.callback(&vq->vq); @@ -2083,6 +2106,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, vq->weak_barriers = weak_barriers; vq->broken = false; vq->last_used_idx = 0; + vq->event_triggered = false; vq->num_added = 0; vq->use_dma_api = vring_use_dma_api(vdev); #ifdef DEBUG
virtio_disable_cb is currently a nop for split ring with event index. This is because it used to be always called from a callback when we know device won't trigger more events until we update the index. However, now that we run with interrupts enabled a lot we also poll without a callback so that is different: disabling callbacks will help reduce the number of spurious interrupts. Further, if using event index with a packed ring, and if being called from a callback, we actually do disable interrupts which is unnecessary. Fix both issues by tracking whenever we get a callback. If that is the case disabling interrupts with event index can be a nop. If not the case disable interrupts. Note: with a split ring there's no explicit "no interrupts" value. For now we write a fixed value so our chance of triggering an interupt is 1/ring size. It's probably better to write something related to the last used index there to reduce the chance even further. For now I'm keeping it simple. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- drivers/virtio/virtio_ring.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)