Virtualization - Feb 2015 - [PATCH RFC v5 net-next 4/6] virtio-net: add basic interrupt coalescing support

"Michael S. Tsirkin" <mst at redhat.com>
writes:
> On Tue, Feb 10, 2015 at 12:02:37PM +1030, Rusty Russell wrote:
>> Jason Wang <jasowang at redhat.com> writes:
>> > This patch enables the interrupt coalescing setting through
ethtool.
>> 
>> The problem is that there's nothing network specific about
interrupt
>> coalescing.  I can see other devices wanting exactly the same thing,
>> which means we'd deprecate this in the next virtio standard.
>> 
>> I think the right answer is to extend like we did with
>> vring_used_event(), eg:
>> 
>> 1) Add a new feature VIRTIO_F_RING_COALESCE.
>> 2) Add another a 32-bit field after vring_used_event(), eg:
>>         #define vring_used_delay(vr) (*(u32
*)((vr)->avail->ring[(vr)->num + 2]))
>> 
>> This loses the ability to coalesce by number of frames, but we can
still
>> do number of sg entries, as we do now with used_event, and we could
>> change virtqueue_enable_cb_delayed() to take a precise number if we
>> wanted.
>
> But do we expect delay to be update dynamically?
> If not, why not stick it in config space?
Hmm, we could update it dynamically (and will, in the case of ethtool).
But it won't be common, so we could append a field to
virtio_pci_common_cfg for PCI.

I think MMIO and CCW would be easy to extend too, but CC'd to check.
>> My feeling is that this should be a v1.0-only feature though
>> (eg. feature bit 33).
>
> Yes, e.g. we can't extend config space for legacy virtio pci.
Thanks,
Rusty.

On Fri, 2015-02-13 at 02:52 +0000, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst at redhat.com> writes:
> > On Tue, Feb 10, 2015 at 12:02:37PM +1030, Rusty Russell wrote:
> >> Jason Wang <jasowang at redhat.com> writes:
> >> > This patch enables the interrupt coalescing setting through
ethtool.
> >> 
> >> The problem is that there's nothing network specific about
interrupt
> >> coalescing.  I can see other devices wanting exactly the same
thing,
> >> which means we'd deprecate this in the next virtio standard.
> >> 
> >> I think the right answer is to extend like we did with
> >> vring_used_event(), eg:
> >> 
> >> 1) Add a new feature VIRTIO_F_RING_COALESCE.
> >> 2) Add another a 32-bit field after vring_used_event(), eg:
> >>         #define vring_used_delay(vr) (*(u32
*)((vr)->avail->ring[(vr)->num + 2]))
> >> 
> >> This loses the ability to coalesce by number of frames, but we can
still
> >> do number of sg entries, as we do now with used_event, and we
could
> >> change virtqueue_enable_cb_delayed() to take a precise number if
we
> >> wanted.
> >
> > But do we expect delay to be update dynamically?
> > If not, why not stick it in config space?
> 
> Hmm, we could update it dynamically (and will, in the case of ethtool).
> But it won't be common, so we could append a field to
> virtio_pci_common_cfg for PCI.
> 
> I think MMIO and CCW would be easy to extend too, but CC'd to check.
As far as I understand the virtio_pci_common_cfg principle (just had a
look, for the first time ;-), it's now an equivalent of the MMIO control
registers block. I see no major problem with adding another one.

Or were you thinking about introducing some standard for the "real"
config space? (fine with me as well - the transport will have nothing to
do :-)

Pawe?

On Fri, 13 Feb 2015 13:22:09 +1030
Rusty Russell <rusty at rustcorp.com.au> wrote:
> "Michael S. Tsirkin" <mst at redhat.com> writes:
> > On Tue, Feb 10, 2015 at 12:02:37PM +1030, Rusty Russell wrote:
> >> Jason Wang <jasowang at redhat.com> writes:
> >> > This patch enables the interrupt coalescing setting through
ethtool.
> >> 
> >> The problem is that there's nothing network specific about
interrupt
> >> coalescing.  I can see other devices wanting exactly the same
thing,
> >> which means we'd deprecate this in the next virtio standard.
> >> 
> >> I think the right answer is to extend like we did with
> >> vring_used_event(), eg:
> >> 
> >> 1) Add a new feature VIRTIO_F_RING_COALESCE.
> >> 2) Add another a 32-bit field after vring_used_event(), eg:
> >>         #define vring_used_delay(vr) (*(u32
*)((vr)->avail->ring[(vr)->num + 2]))
> >> 
> >> This loses the ability to coalesce by number of frames, but we can
still
> >> do number of sg entries, as we do now with used_event, and we
could
> >> change virtqueue_enable_cb_delayed() to take a precise number if
we
> >> wanted.
> >
> > But do we expect delay to be update dynamically?
> > If not, why not stick it in config space?
> 
> Hmm, we could update it dynamically (and will, in the case of ethtool).
> But it won't be common, so we could append a field to
> virtio_pci_common_cfg for PCI.
> 
> I think MMIO and CCW would be easy to extend too, but CC'd to check.
If this is a simple extension of the config space, it should just work
for ccw (the Linux guest driver currently uses 0x100 as max config
space size, which I grabbed from pci at the time I wrote it).

But looking at this virtio_pci_common_cfg stuff, it seems to contain a
lot of things that are handled via ccws on virtio-ccw (like number of
queues or device status). Having an extra ccw just for changing this
delay value seems like overkill.

On the basic topic of interrupt coalescing: With adapter interrupts,
virtio-ccw already has some kind of coalescing: The summary indicator
is set just once and an interrupt is made pending, then individual
queue indicators are switched on and no further interrupt is generated
if the summary indicator has not been cleared by the guest yet. I'm not
sure how it would be different if an individual queue indicator is
switched on later. Chances are that the guest code processing the
indicators has not even yet processed to that individual indicator, so
it wouldn't matter if it was set delayed. It is probably something that
has to be tried out.

Pawel Moll <pawel.moll at arm.com> writes:
> On Fri, 2015-02-13 at 02:52 +0000, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst at redhat.com> writes:
>> > On Tue, Feb 10, 2015 at 12:02:37PM +1030, Rusty Russell wrote:
>> >> Jason Wang <jasowang at redhat.com> writes:
>> >> > This patch enables the interrupt coalescing setting
through ethtool.
>> >> 
>> >> The problem is that there's nothing network specific about
interrupt
>> >> coalescing.  I can see other devices wanting exactly the same
thing,
>> >> which means we'd deprecate this in the next virtio
standard.
>> >> 
>> >> I think the right answer is to extend like we did with
>> >> vring_used_event(), eg:
>> >> 
>> >> 1) Add a new feature VIRTIO_F_RING_COALESCE.
>> >> 2) Add another a 32-bit field after vring_used_event(), eg:
>> >>         #define vring_used_delay(vr) (*(u32
*)((vr)->avail->ring[(vr)->num + 2]))
>> >> 
>> >> This loses the ability to coalesce by number of frames, but we
can still
>> >> do number of sg entries, as we do now with used_event, and we
could
>> >> change virtqueue_enable_cb_delayed() to take a precise number
if we
>> >> wanted.
>> >
>> > But do we expect delay to be update dynamically?
>> > If not, why not stick it in config space?
>> 
>> Hmm, we could update it dynamically (and will, in the case of ethtool).
>> But it won't be common, so we could append a field to
>> virtio_pci_common_cfg for PCI.
>> 
>> I think MMIO and CCW would be easy to extend too, but CC'd to
check.
>
> As far as I understand the virtio_pci_common_cfg principle (just had a
> look, for the first time ;-), it's now an equivalent of the MMIO
control
> registers block. I see no major problem with adding another one.
OK, thanks.
> Or were you thinking about introducing some standard for the
"real"
> config space? (fine with me as well - the transport will have nothing to
> do :-)
No, that'd not be possible at this point.  I think it's a per-transport
decision.

Cheers,
Rusty.

Cornelia Huck <cornelia.huck at de.ibm.com>
writes:
> On Fri, 13 Feb 2015 13:22:09 +1030
> Rusty Russell <rusty at rustcorp.com.au> wrote:
>
>> "Michael S. Tsirkin" <mst at redhat.com> writes:
>> > On Tue, Feb 10, 2015 at 12:02:37PM +1030, Rusty Russell wrote:
>> >> Jason Wang <jasowang at redhat.com> writes:
>> >> > This patch enables the interrupt coalescing setting
through ethtool.
>> >> 
>> >> The problem is that there's nothing network specific about
interrupt
>> >> coalescing.  I can see other devices wanting exactly the same
thing,
>> >> which means we'd deprecate this in the next virtio
standard.
>> >> 
>> >> I think the right answer is to extend like we did with
>> >> vring_used_event(), eg:
>> >> 
>> >> 1) Add a new feature VIRTIO_F_RING_COALESCE.
>> >> 2) Add another a 32-bit field after vring_used_event(), eg:
>> >>         #define vring_used_delay(vr) (*(u32
*)((vr)->avail->ring[(vr)->num + 2]))
>> >> 
>> >> This loses the ability to coalesce by number of frames, but we
can still
>> >> do number of sg entries, as we do now with used_event, and we
could
>> >> change virtqueue_enable_cb_delayed() to take a precise number
if we
>> >> wanted.
>> >
>> > But do we expect delay to be update dynamically?
>> > If not, why not stick it in config space?
>> 
>> Hmm, we could update it dynamically (and will, in the case of ethtool).
>> But it won't be common, so we could append a field to
>> virtio_pci_common_cfg for PCI.
>> 
>> I think MMIO and CCW would be easy to extend too, but CC'd to
check.
>
> If this is a simple extension of the config space, it should just work
> for ccw (the Linux guest driver currently uses 0x100 as max config
> space size, which I grabbed from pci at the time I wrote it).
>
> But looking at this virtio_pci_common_cfg stuff, it seems to contain a
> lot of things that are handled via ccws on virtio-ccw (like number of
> queues or device status). Having an extra ccw just for changing this
> delay value seems like overkill.
Yes, possibly.
> On the basic topic of interrupt coalescing: With adapter interrupts,
> virtio-ccw already has some kind of coalescing: The summary indicator
> is set just once and an interrupt is made pending, then individual
> queue indicators are switched on and no further interrupt is generated
> if the summary indicator has not been cleared by the guest yet. I'm not
> sure how it would be different if an individual queue indicator is
> switched on later. Chances are that the guest code processing the
> indicators has not even yet processed to that individual indicator, so
> it wouldn't matter if it was set delayed. It is probably something that
> has to be tried out.
The network driver will do this at the virtio level too: no more rx
interrupts will be received until all packets have been processed.

But it is particularly useful for network transmit interrupts: we want
to be notified of the packet's finishing, but a little delay (hence more
batching) is better.  For rx, I can envision a case where the guest is
too fast and thus keeps getting interrupted after each packet.  A user
might decide to trade off some latency to increase batching; seems
like a bit like a benchmark hack to me, though...

Cheers,
Rusty.