Bug 616772 - feature request: virtio-serial needs a callback to notify device that to guest queue is not full
feature request: virtio-serial needs a callback to notify device that to gues...
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: qemu-kvm-rhev (Show other bugs)
7.0
All Linux
low Severity low
: beta
: ---
Assigned To: Amit Shah
Virtualization Bugs
: FutureFeature
Depends On:
Blocks: 580953
  Show dependency treegraph
 
Reported: 2010-07-21 08:01 EDT by Alon Levy
Modified: 2015-12-04 11:12 EST (History)
16 users (show)

See Also:
Fixed In Version: qemu-2.3
Doc Type: Enhancement
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-12-04 11:12:51 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Proposed patch (2.60 KB, patch)
2014-07-17 10:39 EDT, Amit Shah
amit.shah: review? (alevy)
Details | Diff

  None (edit)
Description Alon Levy 2010-07-21 08:01:29 EDT
Description of problem:
when a port is fully connected (host and guest) and after virtio_serial_write returns 0, there is no way that doesn't require polling to find out when the host to guest queue (ivq) is not full and the next write will succeed.
Comment 2 RHEL Product and Program Management 2010-07-21 08:17:15 EDT
This feature request has been proposed after Feature Freeze and we
are unable to resolve it in time for the current Red Hat Enterprise
Linux release. It has been denied for the current release and
proposed for the next Red Hat Enterprise Linux release.
Comment 3 Amit Shah 2010-10-11 07:59:47 EDT
With the current version of virtio, there doesn't seem to be a way to do this -- the guest can fill up buffers for the host to send data in, but this may not be conveyed to the host at all.

A timer with a few milli-seconds worth of sleep time perhaps is better than polling.

I'll let this bug remain open for now and see if other alternatives come up.
Comment 5 Ronen Hod 2013-11-14 07:28:33 EST
Taking the suggestion to OASIS
Comment 6 Amit Shah 2014-07-17 10:39:51 EDT
Created attachment 918736 [details]
Proposed patch

Hey Alon,

Please take a look and let me know if this is what you wanted.

Thanks,
Comment 7 Amit Shah 2014-07-21 08:17:43 EDT
I discussed that patch in attachment 918736 [details] with Alon, and he thinks it's sufficient as well.  We will need to expose the callback via CharDriverState so that spice-qemu-kvm.c can use it.  Once this is done, spice_server can get rid of polls in favour of the signal it'll receive via the callback.

More details:
 in spice-qemu-char.c, vmc_write() calls

        int can_write = qemu_chr_be_can_write(scd->chr);

If the value of can_write is 0 here, it could mean the vq is full, and spice can't write anything for the guest to consume right now.  In this case, vmc_write() can register the new ->guest_writable() callback, and it will then get a notification when the vq is writable again.

The virtio-serial part of this is done, via the patch above.

The chardev work is still to be done.

Once both of those are upstream, spice-server can then be modified to use the new functionality.
Comment 8 Alon Levy 2014-07-21 08:28:17 EDT
Note for future spice implementation:
* No need for new api, simply retry writing on spice_server_char_device_wakeup.
* Then we can remove the write_to_dev_timer timer in spice/server/char_device.c

Two options for handling backwards compatibility:
* don't remove the timer. So for new implementations we will still do redundant attempts to write but since it's the same process the overhead should be negligble.
* add a capability bit. We don't have capabilities in the spice server interface, so this could be done either by first adding capabilities and then using them, or adding a new symbol to be called by qemu setting the state (defaults to false) of a wakeup on guest write availability.
Comment 9 Marc-Andre Lureau 2014-07-21 10:53:22 EDT
(In reply to Alon Levy from comment #8)
> Note for future spice implementation:
> * No need for new api, simply retry writing on
> spice_server_char_device_wakeup.

Ok, so it will need to retry write as well (read only atm)

> * Then we can remove the write_to_dev_timer timer in
> spice/server/char_device.c
> 
> Two options for handling backwards compatibility:
> * don't remove the timer. So for new implementations we will still do
> redundant attempts to write but since it's the same process the overhead
> should be negligble.
> * add a capability bit. We don't have capabilities in the spice server
> interface, so this could be done either by first adding capabilities and
> then using them, or adding a new symbol to be called by qemu setting the
> state (defaults to false) of a wakeup on guest write availability.

I would add a SpiceCharDeviceInterface.flags = SPICE_CHAR_DEVICE_FLAG_DISABLE_TIMER

that sounds simple, but should go upstream/fedora first.

Amit, any chance you propose your patch to qemu upstream soon?

I think we should open a duplicate for tracking spice-server patch.
Comment 10 Amit Shah 2014-07-21 12:56:30 EDT
(In reply to Marc-Andre Lureau from comment #9)

> Amit, any chance you propose your patch to qemu upstream soon?

Without the chardev part of it, there's not much utility to it, I suppose?  It does make a good standalone patch, but I'd rather have a proper patchset that uses the functionality here.

Can't really say when the chardev work will happen, though.  I'll discuss with Amos and either re-assign to myself or Amos will comment here.
Comment 12 Amit Shah 2014-11-10 12:00:11 EST
Marc-Andre posted spice patches upstream, and they look fine.  All the patches are upstream, and will likely land in early in the qemu-2.3 cycle.
Comment 13 Amit Shah 2015-05-15 07:11:23 EDT
The patch for virtio-serial is now upstream.  Marc-Andre's series for using this API in spice will go in 2.4.  Marc-Andre, please open a bug for spice if you'd like to backport them.
Comment 25 juzhang 2015-08-10 01:27:51 EDT
According to comment23 and comment24, set this issue as verified.
Comment 27 errata-xmlrpc 2015-12-04 11:12:51 EST
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2015-2546.html

Note You need to log in before you can comment on or make changes to this bug.