virtio-balloon: Don't wait indefinitely for guest response
v3 cover virtio-balloon: Don't wait indefinitely for guest response
Since I didn't get negative reactions to the patch I posted yesterday, here's the complete series. It adds a qerror message that mentions the machine is stopped or the guest is slow to respond, so the stats should be assumed to be old.
The error report is sent out before the stats, I don't think we do that for any other command. libvirt should be prepared to handle this. If not, please suggest a better alternative.
Differences from v2:
- Add a timeout for uncooperative/hung guests
- Add an qerror message for letting users/management know of guest stopped status.
Amit Shah (3):
balloon: Don't try fetching info if guest is unresponsive
qerror: Add a new MACHINE_STOPPED error message
balloon: Don't try fetching info if machine is stopped
balloon.c | 11 +++++++---- balloon.h | 6 ++++-- hw/virtio-balloon.c | 21 +++++++++++++++++++-- qerror.c | 4 ++++ qerror.h | 3 +++ 5 files changed, 37 insertions(+), 8 deletions(-)
Signature · 1 line
1.7.2.2
PATCH v3 1/3 balloon: Don't try fetching info if guest is unresponsive
If the guest is unresponsive and 'info balloon' is invoked, the monitor process just hangs waiting for info from the guest. Return the most recent balloon data in that case.
A new timer is added, which on expiry, just presents the old data to the monitor callback functions.
See https://bugzilla.redhat.com/show_bug.cgi?id=623903
--- hw/virtio-balloon.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
hw/virtio-balloon.c +11 -0
@@ -40,6 +40,7 @@ typedef struct VirtIOBalloonsize_t stats_vq_offset;MonitorCompletion *stats_callback;void *stats_opaque_callback_data; +QEMUTimer *timer; } VirtIOBalloon;static VirtIOBalloon *to_virtio_balloon (VirtIODevice *vdev)
@@ -137,6 +138,11 @@ static void complete_stats_request(VirtIOBalloon *vb) vb->stats_callback =NULL ; } +static void show_old_stats (void *opaque) +{ +complete_stats_request (opaque); +} +static void virtio_balloon_receive_stats (VirtIODevice *vdev,VirtQueue *vq) {VirtIOBalloon *s =DO_UPCAST (VirtIOBalloon, vdev, vdev);
@@ -148,6 +154,8 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)return ; } +qemu_del_timer (s->timer); +/* Initialize the stats to get rid of any stale values. This is only * needed to handle the case where a guest supports fewer stats than it * used to (ie. it has booted into an old kernel).
@@ -215,6 +223,7 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target, dev->stats_callback = cb; dev->stats_opaque_callback_data = cb_data;if (dev->vdev.guest_features & (1 <<VIRTIO_BALLOON_F_STATS_VQ )) { +qemu_mod_timer (dev->timer,qemu_get_clock (rt_clock) +5000 );virtqueue_push (dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);virtio_notify (&dev->vdev, dev->svq); }else {
@@ -267,6 +276,8 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev) s->dvq =virtio_add_queue (&s->vdev,128 , virtio_balloon_handle_output); s->svq =virtio_add_queue (&s->vdev,128 , virtio_balloon_receive_stats); + s->timer =qemu_new_timer (rt_clock, show_old_stats, s); +reset_stats (s);qemu_add_balloon_handler (virtio_balloon_to_target, s);
Signature · 1 line
1.7.2.2
PATCH v3 2/3 qerror: Add a new MACHINE_STOPPED error message
In reply to Amit ShahThis error message denotes some command was not successful in completing as the guest was unresponsive.
Use it in the virtio-balloon code when showing older, cached data.
--- hw/virtio-balloon.c | 1 + qerror.c | 4 ++++ qerror.h | 3 +++ 3 files changed, 8 insertions(+), 0 deletions(-)
hw/virtio-balloon.c +1 -0
@@ -140,6 +140,7 @@ static void complete_stats_request(VirtIOBalloon *vb)static void show_old_stats (void *opaque) { +qerror_report (QERR_MACHINE_STOPPED );complete_stats_request (opaque); }
qerror.c +4 -0
@@ -141,6 +141,10 @@ static const QErrorStringTable qerror_table[] = { .desc = "Using KVM without %(capability), %(feature) unavailable", }, { + .error_fmt = QERR_MACHINE_STOPPED, + .desc = "The machine is stopped or the guest is taking too long to respond", + }, + { .error_fmt = QERR_MIGRATION_EXPECTED, .desc = "An incoming migration is expected before this command can be executed", },
qerror.h +3 -0
@@ -121,6 +121,9 @@ QError *qobject_to_qerror(const QObject *obj);#define QERR_KVM_MISSING_CAP \ "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }" +#define QERR_MACHINE_STOPPED \ + "{ 'class': 'MachineStopped', 'data': {} }" +#define QERR_MIGRATION_EXPECTED \ "{ 'class': 'MigrationExpected', 'data': {} }"
Signature · 1 line
1.7.2.2
[Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 17 lines · Amit Shah
This error message denotes some command was not successful in completing as the guest was unresponsive.
Use it in the virtio-balloon code when showing older, cached data.
Signed-off-by Amit Shah <[email protected]>--- hw/virtio-balloon.c | 1 + qerror.c | 4 ++++ qerror.h | 3 +++ 3 files changed, 8 insertions(+), 0 deletions(-)hw/virtio-balloon.c +0
-0
hw/virtio-balloon.c static void complete_stats_request(VirtIOBalloon *vb) · line 145
Quoted 6 lines
hw/virtio-balloon.c +1
-0@@ -140,6 +140,7 @@ static void complete_stats_request(VirtIOBalloon *vb)static void show_old_stats (void *opaque) { +qerror_report (QERR_MACHINE_STOPPED );complete_stats_request (opaque); }
NACK. It has always been allowed & valid to call query-balloon to get the current balloon level. We must not throw an error just because the recently added mem stats can't be refreshed.
Regards,
Daniel
Signature · 4 lines
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
[Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 20 lines · Daniel P. Berrange
Quoted 18 lines · Amit Shah
This error message denotes some command was not successful in completing as the guest was unresponsive.
Use it in the virtio-balloon code when showing older, cached data.
Signed-off-by Amit Shah<[email protected]>--- hw/virtio-balloon.c | 1 + qerror.c | 4 ++++ qerror.h | 3 +++ 3 files changed, 8 insertions(+), 0 deletions(-)hw/virtio-balloon.c +0
-0@@ -140,6 +140,7 @@ static void complete_stats_request(VirtIOBalloon *vb)
Quoted 11 lines
hw/virtio-balloon.c static void complete_stats_request(VirtIOBalloon *vb) · line 146
Quoted 6 lines
hw/virtio-balloon.c +1
-0static void show_old_stats (void *opaque) { +qerror_report (QERR_MACHINE_STOPPED );complete_stats_request (opaque); }NACK. It has always been allowed& valid to call query-balloon to get the current balloon level. We must not throw an error just because the recently added mem stats can't be refreshed.
I think that's a fair comment but why even bother fixing the command. Let's introduce a new command that just gets a single piece of information instead of having a command return lots of information.
Regards,
Anthony Liguori
Quoted 3 lines
Regards,
Daniel
[Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 32 lines · Anthony Liguori
Quoted 20 lines · Daniel P. Berrange
Quoted 18 lines · Amit Shah
This error message denotes some command was not successful in completing as the guest was unresponsive.
Use it in the virtio-balloon code when showing older, cached data.
Signed-off-by Amit Shah<[email protected]>--- hw/virtio-balloon.c | 1 + qerror.c | 4 ++++ qerror.h | 3 +++ 3 files changed, 8 insertions(+), 0 deletions(-)hw/virtio-balloon.c +0
-0@@ -140,6 +140,7 @@ static void complete_stats_request(VirtIOBalloon *vb)Quoted 10 lines
hw/virtio-balloon.c static void complete_stats_request(VirtIOBalloon *vb) · line 146
Quoted 6 lines
hw/virtio-balloon.c +1
-0static void show_old_stats (void *opaque) { +qerror_report (QERR_MACHINE_STOPPED );complete_stats_request (opaque); }NACK. It has always been allowed& valid to call query-balloon to get the current balloon level. We must not throw an error just because the recently added mem stats can't be refreshed.
Quoted 3 lines
I think that's a fair comment but why even bother fixing the command. Let's introduce a new command that just gets a single piece of information instead of having a command return lots of information.
The existing query-balloon command that has been around for years & is used by all current apps has a significant regression since we added the memstats code to it: a guest can now trivially inflict a DOS on the mgmt app if it crashes or is malicious. IMHO we need to fix that regression for 0.13 so that existing apps don't suffer[1]. Adding a timeout to silently skip the stats refresh if the guest doesn't respond, but without raising an error seems the best tradeoff we can do here.
Beyond fixing that regression, I agree that this command is terminally flawed & we need to deprecate it & provide better specified new replacement(s). This seems like 0.14 work to me though.
Regards,
Daniel
[1] I know that they could already suffer if there was a bug in qemu that prevented it responding, even if the guest was not being malicious/crashed.
Signature · 4 lines
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 37 lines · Daniel P. Berrange
Quoted 32 lines · Anthony Liguori
Quoted 20 lines · Daniel P. Berrange
Quoted 18 lines · Amit Shah
This error message denotes some command was not successful in completing as the guest was unresponsive.
Use it in the virtio-balloon code when showing older, cached data.
Signed-off-by Amit Shah<[email protected]>--- hw/virtio-balloon.c | 1 + qerror.c | 4 ++++ qerror.h | 3 +++ 3 files changed, 8 insertions(+), 0 deletions(-)hw/virtio-balloon.c +0
-0@@ -140,6 +140,7 @@ static void complete_stats_request(VirtIOBalloon *vb)Quoted 10 lines
hw/virtio-balloon.c static void complete_stats_request(VirtIOBalloon *vb) · line 146
Quoted 6 lines
hw/virtio-balloon.c +1
-0static void show_old_stats (void *opaque) { +qerror_report (QERR_MACHINE_STOPPED );complete_stats_request (opaque); }NACK. It has always been allowed& valid to call query-balloon to get the current balloon level. We must not throw an error just because the recently added mem stats can't be refreshed.
Quoted 3 lines
I think that's a fair comment but why even bother fixing the command. Let's introduce a new command that just gets a single piece of information instead of having a command return lots of information.
Quoted 7 lines
The existing query-balloon command that has been around for years & is used by all current apps has a significant regression since we added the memstats code to it: a guest can now trivially inflict a DOS on the mgmt app if it crashes or is malicious. IMHO we need to fix that regression for 0.13 so that existing apps don't suffer[1]. Adding a timeout to silently skip the stats refresh if the guest doesn't respond, but without raising an error seems the best tradeoff we can do here.
I agree.
Adding a roundtrip through the guest to an existing command was a mistake.
Quoted 3 lines
Beyond fixing that regression, I agree that this command is terminally flawed & we need to deprecate it & provide better specified new replacement(s). This seems like 0.14 work to me though.
Yup.
Quoted 6 lines
Regards,
Daniel[1] I know that they could already suffer if there was a bug in qemu that prevented it responding, even if the guest was not being malicious/crashed.
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 47 lines · On Fri, 27 Aug 2010 15:59:21 +0200 Markus Armbruster <[email protected]> wrote:
Quoted 37 lines · Daniel P. Berrange
Quoted 32 lines · Anthony Liguori
Quoted 20 lines · Daniel P. Berrange
Quoted 18 lines · Amit Shah
This error message denotes some command was not successful in completing as the guest was unresponsive.
Use it in the virtio-balloon code when showing older, cached data.
Signed-off-by Amit Shah<[email protected]>--- hw/virtio-balloon.c | 1 + qerror.c | 4 ++++ qerror.h | 3 +++ 3 files changed, 8 insertions(+), 0 deletions(-)hw/virtio-balloon.c +0
-0@@ -140,6 +140,7 @@ static void complete_stats_request(VirtIOBalloon *vb)Quoted 10 lines
hw/virtio-balloon.c static void complete_stats_request(VirtIOBalloon *vb) · line 146
Quoted 6 lines
hw/virtio-balloon.c +1
-0static void show_old_stats (void *opaque) { +qerror_report (QERR_MACHINE_STOPPED );complete_stats_request (opaque); }NACK. It has always been allowed& valid to call query-balloon to get the current balloon level. We must not throw an error just because the recently added mem stats can't be refreshed.
Quoted 3 lines
I think that's a fair comment but why even bother fixing the command. Let's introduce a new command that just gets a single piece of information instead of having a command return lots of information.
Quoted 7 lines
The existing query-balloon command that has been around for years & is used by all current apps has a significant regression since we added the memstats code to it: a guest can now trivially inflict a DOS on the mgmt app if it crashes or is malicious. IMHO we need to fix that regression for 0.13 so that existing apps don't suffer[1]. Adding a timeout to silently skip the stats refresh if the guest doesn't respond, but without raising an error seems the best tradeoff we can do here.
Quoted 4 lines
I agree.
Adding a roundtrip through the guest to an existing command was a mistake.
I wondered if we could drop it for now to make it right in 0.14, but I believe it's already part of the user monitor for some time and libvirt uses the stats, right?
I think we need testing/unstable namespace in QMP, where commands can be tested for while so that we reduce the risk of nasty surprises like this one.
Quoted 14 lines
Quoted 4 lines
Beyond fixing that regression, I agree that this command is terminally flawed & we need to deprecate it & provide better specified new replacement(s). This seems like 0.14 work to me though.
Yup.
Quoted 6 lines
Regards,
Daniel[1] I know that they could already suffer if there was a bug in qemu that prevented it responding, even if the guest was not being malicious/crashed.
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 8 lines · Luiz Capitulino
I wondered if we could drop it for now to make it right in 0.14, but I believe it's already part of the user monitor for some time and libvirt uses the stats, right?
I think we need testing/unstable namespace in QMP, where commands can be tested for while so that we reduce the risk of nasty surprises like this one.
It's trying to plug a sieve with a band-aid. It's certainly an "improvement" but it's of question utility looking at the bigger picture.
Balloon is a perfect example of where what we really need to do is build interface interfaces that make sense, and then expose them in QMP.
What's a reasonable C-level API to query statistics that potentially may never return? Building in a timeout is something of a crappy API because it puts policy deep in the API that is trivial to implement elsewhere. What you'd probably do is something like:
BalloonStatsRequest *query_guest_balloon_stats(CompletionCallback *cb, void *opaque); int cancel_guest_balloon_stats(BalloonStatsRequest *req);
void release_guest_balloon_stats(BalloonStatsRequest *req);
Regards,
Anthony Liguori
Quoted 17 lines
Quoted 16 lines
Quoted 5 lines
Beyond fixing that regression, I agree that this command is terminally flawed& we need to deprecate it& provide better specified new replacement(s). This seems like 0.14 work to me though.
Yup.
Quoted 7 lines
Regards,
Daniel[1] I know that they could already suffer if there was a bug in qemu that prevented it responding, even if the guest was not being malicious/crashed.
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 22 lines · Anthony Liguori
Quoted 9 lines · Luiz Capitulino
I wondered if we could drop it for now to make it right in 0.14, but I believe it's already part of the user monitor for some time and libvirt uses the stats, right?
I think we need testing/unstable namespace in QMP, where commands can be tested for while so that we reduce the risk of nasty surprises like this one.
It's trying to plug a sieve with a band-aid. It's certainly an "improvement" but it's of question utility looking at the bigger picture.
Balloon is a perfect example of where what we really need to do is build interface interfaces that make sense, and then expose them in QMP.
What's a reasonable C-level API to query statistics that potentially may never return? Building in a timeout is something of a crappy API because it puts policy deep in the API that is trivial to implement elsewhere. What you'd probably do is something like:
Quoted 4 lines
BalloonStatsRequest *query_guest_balloon_stats(CompletionCallback *cb, void *opaque);
intcancel_guest_balloon_stats(BalloonStatsRequest *req); voidrelease_guest_balloon_stats(BalloonStatsRequest *req);
IMHO this is flawed because it does not allow you to fetch the balloon stats independantly of asking the guest OS to refresh them. So if the guest dies, it is not possible to ask QEMU what the last known stats were.
Although the current guest driver implementation requires the host to request an update before the guest will refresh stats, in theory someone could provide a guest driver impl thta does a periodic push to the host without requiring a request.
Thus for a fully generalized access mechanism you need two APIs and one event
- API to request guest balloon stats update
- API to get current known balloon stats
- Event to notify client of a balloon stats update
And for the non-stats related balloon level we need
- Event to notify client of balloon level change
Even if the API to request a guest balloon stats update is fully async and cancellable, you still need the event to notify client of the balloon stats update, because in a multiple monitor scenario the client wanting notification may be different to the client requesting the refresh.
Regards,
Daniel
Signature · 4 lines
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 29 lines · Daniel P. Berrange
Quoted 22 lines · Anthony Liguori
Quoted 9 lines · Luiz Capitulino
I wondered if we could drop it for now to make it right in 0.14, but I believe it's already part of the user monitor for some time and libvirt uses the stats, right?
I think we need testing/unstable namespace in QMP, where commands can be tested for while so that we reduce the risk of nasty surprises like this one.
It's trying to plug a sieve with a band-aid. It's certainly an "improvement" but it's of question utility looking at the bigger picture.
Balloon is a perfect example of where what we really need to do is build interface interfaces that make sense, and then expose them in QMP.
What's a reasonable C-level API to query statistics that potentially may never return? Building in a timeout is something of a crappy API because it puts policy deep in the API that is trivial to implement elsewhere. What you'd probably do is something like:
Quoted 5 lines
BalloonStatsRequest *query_guest_balloon_stats(CompletionCallback *cb, void *opaque);
intcancel_guest_balloon_stats(BalloonStatsRequest *req); voidrelease_guest_balloon_stats(BalloonStatsRequest *req);
Quoted 5 lines
IMHO this is flawed because it does not allow you to fetch the balloon stats independantly of asking the guest OS to refresh them. So if the guest dies, it is not possible to ask QEMU what the last known stats were.
The "last known" stats could be totally meaningless as they could be from 3 hours ago and there may have been a full reboot into a different OS since then.
Whether a "last known" stat is useful is a policy decision and belongs in the management tooling.
Quoted 11 lines
Although the current guest driver implementation requires the host to request an update before the guest will refresh stats, in theory someone could provide a guest driver impl thta does a periodic push to the host without requiring a request.
Thus for a fully generalized access mechanism you need two APIs and one event
- API to request guest balloon stats update
- API to get current known balloon stats
Current known balloon stats is not meaningful within qemu.
Quoted 6 lines
- Event to notify client of a balloon stats update
And for the non-stats related balloon level we need
- Event to notify client of balloon level change
An alternative API would avoid a 1-1 relationship between requests and responses. That would mean:
void balloon_set_target(BalloonInterface *b, uint64_t value); uint64_t balloon_get_STAT_NAME(BalloonInterface *b);
void balloon_request_update(BalloonInterface *b);
void balloon_add_stat_change_notifier(BalloonInterface *b, StatNotifer *notifer, uint64_t mask); void balloon_del_stat_change_notifier(BalloonInterface *b, StatNotifier *notifier);
This API allows you to set and get individual stats. Because there are individual getters/setters, it provides a better backwards/future compatibility and is pretty discoverable. It offers no real indication of the age of the stat. It's unclear to me if that's a good or bad thing.
balloon_request_update() requests that a guest update it's status but there's no specific guarantees in the interface. A mask might prove valuable here.
Then there's an interface to register notifications of when the statistics are updated. I think this probably would cover most use cases.
A main difference between this API is that the API guarantees absolutely no "freshness" to the statistics. It provides an interface to freshen them but provides no guarantees about when that will happen and even if it will happen.
Regards,
Anthony Liguori
Quoted 9 lines
Even if the API to request a guest balloon stats update is fully async and cancellable, you still need the event to notify client of the balloon stats update, because in a multiple monitor scenario the client wanting notification may be different to the client requesting the refresh.
Regards,
Daniel
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
In reply to Anthony LiguoriQuoted 6 lines · On Fri, 27 Aug 2010 09:59:55 -0500 Anthony Liguori <[email protected]>…
Quoted 5 lines · Luiz Capitulino
I wondered if we could drop it for now to make it right in 0.14, but I believe it's already part of the user monitor for some time and libvirt uses the stats, right?
Quoted 6 lines
Quoted 3 lines
I think we need testing/unstable namespace in QMP, where commands can be tested for while so that we reduce the risk of nasty surprises like this one.
It's trying to plug a sieve with a band-aid. It's certainly an "improvement" but it's of question utility looking at the bigger picture.
Are you talking about the testing namespace idea? It doesn't have anything to do with balloon or how our interfaces are built. It would be just a way to play with commands that has been converted to QMP but are not available because they're not stable yet (eg. Jan's device_show).
Quoted 2 lines
Balloon is a perfect example of where what we really need to do is build interface interfaces that make sense, and then expose them in QMP.
Main question is: can we drop the stats the way they are today to do the Right Thing for 0.14 or not?
I think we have agreed on the internal interfaces approach. My only concern is whether this will conflict when extending the wire protocol (eg. adding new arguments to existing commands). Not a problem if the C API is not stable, of course.
Quoted 8 lines
What's a reasonable C-level API to query statistics that potentially may never return? Building in a timeout is something of a crappy API because it puts policy deep in the API that is trivial to implement elsewhere. What you'd probably do is something like:
BalloonStatsRequest *query_guest_balloon_stats(CompletionCallback *cb, void *opaque);
intcancel_guest_balloon_stats(BalloonStatsRequest *req);
Shouldn't the API provide a general cancel method? All functions that talk to the guest will need one.
Quoted 24 lines
void
release_guest_balloon_stats(BalloonStatsRequest *req);Regards,
Anthony Liguori
Quoted 17 lines
Quoted 16 lines
Quoted 5 lines
Beyond fixing that regression, I agree that this command is terminally flawed& we need to deprecate it& provide better specified new replacement(s). This seems like 0.14 work to me though.
Yup.
Quoted 7 lines
Regards,
Daniel[1] I know that they could already suffer if there was a bug in qemu that prevented it responding, even if the guest was not being malicious/crashed.
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 8 lines · Luiz Capitulino
Quoted 3 lines
It's trying to plug a sieve with a band-aid. It's certainly an "improvement" but it's of question utility looking at the bigger picture.
Are you talking about the testing namespace idea? It doesn't have anything to do with balloon or how our interfaces are built. It would be just a way to play with commands that has been converted to QMP but are not available because they're not stable yet (eg. Jan's
device_show).
My point is that we shouldn't build any QMP APIs and we definitely shouldn't try to QMP-ize monitor commands.
Instead, we should design logical C APIs that we could consume within QEMU that we think we can support long term and then expose those over QMP.
Having a sandbox doesn't really solve the fundamental problem of making sure the interface is consumable.
Quoted 6 lines
Quoted 3 lines
Balloon is a perfect example of where what we really need to do is build interface interfaces that make sense, and then expose them in QMP.
Main question is: can we drop the stats the way they are today to do the Right Thing for 0.14 or not?
I don't see how 0.13.0 is going to get releases with anything but the current behavior. It's unfortunate but we're too delayed and can't afford a change like this this late in the game.
In terms of the stable branch, the least disruptive thing would be a timeout.
Quoted 5 lines
I think we have agreed on the internal interfaces approach. My only concern is whether this will conflict when extending the wire protocol (eg. adding new arguments to existing commands). Not a problem if the C API is not stable, of course.
We don't do that. It's a recipe for disaster. QEMU isn't written in Python and if we try to module our interfaces are if we were a Python library, we're destined to fail.
Quoted 5 lines
Quoted 5 lines
What's a reasonable C-level API to query statistics that potentially may never return? Building in a timeout is something of a crappy API because it puts policy deep in the API that is trivial to implement elsewhere. What you'd probably do is something like:
Quoted 7 lines
Quoted 4 lines
BalloonStatsRequest *query_guest_balloon_stats(CompletionCallback *cb, void *opaque);
intcancel_guest_balloon_stats(BalloonStatsRequest *req);Shouldn't the API provide a general cancel method? All functions that talk to the guest will need one.
See next proposal. There's no cancel but I'd argue it's not needed. You don't care if the request succeeds or fails so there's no point in cancelling it. Cancellation only works best when a request has a discrete life cycle but in the case of requesting a guest to update stats, there is not really a well define dstart and end.
Regards,
Anthony Liguori
Quoted 32 lines
Quoted 31 lines
void
release_guest_balloon_stats(BalloonStatsRequest *req);Regards,
Anthony Liguori
Quoted 23 lines
Quoted 21 lines
Quoted 7 lines
Beyond fixing that regression, I agree that this command is terminally
flawed& we need to deprecate it& provide better specified new
replacement(s). This seems like 0.14 work to me though.Yup.
Quoted 8 lines
Regards,
Daniel[1] I know that they could already suffer if there was a bug in qemu that prevented it responding, even if the guest was not being malicious/crashed.
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 10 lines · On Fri, 27 Aug 2010 14:02:45 -0500 Anthony Liguori <[email protected]>…
Quoted 8 lines · Luiz Capitulino
Quoted 3 lines
It's trying to plug a sieve with a band-aid. It's certainly an "improvement" but it's of question utility looking at the bigger picture.
Are you talking about the testing namespace idea? It doesn't have anything to do with balloon or how our interfaces are built. It would be just a way to play with commands that has been converted to QMP but are not available because they're not stable yet (eg. Jan's
device_show).
Quoted 5 lines
My point is that we shouldn't build any QMP APIs and we definitely shouldn't try to QMP-ize monitor commands.
Instead, we should design logical C APIs that we could consume within QEMU that we think we can support long term and then expose those over QMP.
Comments on the Python comment below.
Quoted 2 lines
Having a sandbox doesn't really solve the fundamental problem of making sure the interface is consumable.
It doesn't and it shouldn't. It's just a way to test commands that might not be stable yet. A very minor feature, anyway.
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
Balloon is a perfect example of where what we really need to do is build interface interfaces that make sense, and then expose them in QMP.
Main question is: can we drop the stats the way they are today to do the Right Thing for 0.14 or not?
Quoted 6 lines
I don't see how 0.13.0 is going to get releases with anything but the current behavior. It's unfortunate but we're too delayed and can't afford a change like this this late in the game.
In terms of the stable branch, the least disruptive thing would be a timeout.
Okay.
Quoted 9 lines
Quoted 5 lines
I think we have agreed on the internal interfaces approach. My only concern is whether this will conflict when extending the wire protocol (eg. adding new arguments to existing commands). Not a problem if the C API is not stable, of course.
We don't do that. It's a recipe for disaster. QEMU isn't written in Python and if we try to module our interfaces are if we were a Python library, we're destined to fail.
You mean we don't do simple protocol extensions?
So, if we need an new argument to an existing command, we add a new command instead? Just because QEMU is not written in Python?
Quoted 56 lines
Quoted 5 lines
Quoted 5 lines
What's a reasonable C-level API to query statistics that potentially may never return? Building in a timeout is something of a crappy API because it puts policy deep in the API that is trivial to implement elsewhere. What you'd probably do is something like:
Quoted 7 lines
Quoted 4 lines
BalloonStatsRequest *query_guest_balloon_stats(CompletionCallback *cb, void *opaque);
intcancel_guest_balloon_stats(BalloonStatsRequest *req);Shouldn't the API provide a general cancel method? All functions that talk to the guest will need one.
See next proposal. There's no cancel but I'd argue it's not needed. You don't care if the request succeeds or fails so there's no point in cancelling it. Cancellation only works best when a request has a discrete life cycle but in the case of requesting a guest to update stats, there is not really a well define dstart and end.
Regards,
Anthony Liguori
Quoted 32 lines
Quoted 31 lines
void
release_guest_balloon_stats(BalloonStatsRequest *req);Regards,
Anthony Liguori
Quoted 23 lines
Quoted 21 lines
Quoted 7 lines
Beyond fixing that regression, I agree that this command is terminally
flawed& we need to deprecate it& provide better specified new
replacement(s). This seems like 0.14 work to me though.Yup.
Quoted 8 lines
Regards,
Daniel[1] I know that they could already suffer if there was a bug in qemu that prevented it responding, even if the guest was not being malicious/crashed.
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 17 lines · Luiz Capitulino
Quoted 8 lines
I don't see how 0.13.0 is going to get releases with anything but the current behavior. It's unfortunate but we're too delayed and can't afford a change like this this late in the game.
In terms of the stable branch, the least disruptive thing would be a timeout.
Okay.
Quoted 6 lines
Quoted 6 lines
I think we have agreed on the internal interfaces approach. My only concern is whether this will conflict when extending the wire protocol (eg. adding new arguments to existing commands). Not a problem if the C API is not stable, of course.
Quoted 9 lines
Quoted 4 lines
We don't do that. It's a recipe for disaster. QEMU isn't written in Python and if we try to module our interfaces are if we were a Python library, we're destined to fail.
You mean we don't do simple protocol extensions?
So, if we need an new argument to an existing command, we add a new command instead? Just because QEMU is not written in Python?
Because it's too easy to get it wrong in QEMU. Here's the rationale.
If I can't trivially call a QMP function in C, then I'm not going to use QMP functions within QEMU. I'm not going to create an embedded JSON string just to call a function with three integer arguments.
Yes, if we need to do that, we can create a C API that both the QMP interface uses and we also use internally but why? All that does is introduce the chance that the C API will have more features than the QMP interface.
If we don't use these functions in QEMU, then how do we know that these functions have reasonable semantics? This is exactly the problem we suffer today. We have internal APIs that do reasonable things but everything that deals with QMP is a special case. That creates too many opportunities to get things wrong.
I think it's a vitally important requirement that all future QMP functions have direct mappings to a C interface. The long term goal should be for that interface to be used by all of the command line arguments, SDL, and the human monitor. If those things only relied on a single API and we exposed that API via QMP, than we would have an extremely useful interface.
Regards,
Anthony Liguori
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 28 lines · On Fri, 27 Aug 2010 14:37:54 -0500 Anthony Liguori <[email protected]>…
Quoted 17 lines · Luiz Capitulino
Quoted 8 lines
I don't see how 0.13.0 is going to get releases with anything but the current behavior. It's unfortunate but we're too delayed and can't afford a change like this this late in the game.
In terms of the stable branch, the least disruptive thing would be a timeout.
Okay.
Quoted 6 lines
Quoted 6 lines
I think we have agreed on the internal interfaces approach. My only concern is whether this will conflict when extending the wire protocol (eg. adding new arguments to existing commands). Not a problem if the C API is not stable, of course.
Quoted 9 lines
Quoted 4 lines
We don't do that. It's a recipe for disaster. QEMU isn't written in Python and if we try to module our interfaces are if we were a Python library, we're destined to fail.
You mean we don't do simple protocol extensions?
So, if we need an new argument to an existing command, we add a new command instead? Just because QEMU is not written in Python?
Quoted 5 lines
Because it's too easy to get it wrong in QEMU. Here's the rationale.
If I can't trivially call a QMP function in C, then I'm not going to use QMP functions within QEMU. I'm not going to create an embedded JSON string just to call a function with three integer arguments.
... And as a QMP client I don't see the point in having to use a new command just because a new argument was needed. The language QEMU is written is also unimportant. The wire format is all I see and it's language independent.
Quoted 4 lines
Yes, if we need to do that, we can create a C API that both the QMP interface uses and we also use internally but why? All that does is introduce the chance that the C API will have more features than the QMP interface.
Why? I mean, what would stop us on extending QMP when the C interface is also extended? Examples?
At first, this seems a good middle ground to me: QMP is implemented on top of the C API and the JSON stuff is limited to QMP.
Of course that C API functions returning structured data will have to create and return qobjects. But that's needed even with the direct mapping.
Quoted 6 lines
If we don't use these functions in QEMU, then how do we know that these functions have reasonable semantics? This is exactly the problem we suffer today. We have internal APIs that do reasonable things but everything that deals with QMP is a special case. That creates too many opportunities to get things wrong.
Quoted 6 lines
I think it's a vitally important requirement that all future QMP functions have direct mappings to a C interface. The long term goal should be for that interface to be used by all of the command line arguments, SDL, and the human monitor. If those things only relied on a single API and we exposed that API via QMP, than we would have an extremely useful interface.
I agree, I just think an indirect mapping would be more beneficial to QMP clients.
Should QMP be RPC to internal C interfaces? (was: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message)
In reply to Anthony LiguoriQuoted 1 line · Anthony Liguori
On 08/27/2010 02:24 PM, Luiz Capitulino wrote:
[...]
Quoted 16 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
I think we have agreed on the internal interfaces approach. My only concern is whether this will conflict when extending the wire protocol (eg. adding new arguments to existing commands). Not a problem if the C API is not stable, of course.
Quoted 9 lines
Quoted 4 lines
We don't do that. It's a recipe for disaster. QEMU isn't written in Python and if we try to module our interfaces are if we were a Python library, we're destined to fail.
You mean we don't do simple protocol extensions?
So, if we need an new argument to an existing command, we add a new command instead? Just because QEMU is not written in Python?
Quoted 5 lines
Because it's too easy to get it wrong in QEMU. Here's the rationale.
If I can't trivially call a QMP function in C, then I'm not going to use QMP functions within QEMU. I'm not going to create an embedded JSON string just to call a function with three integer arguments.
Yes, an internal interface is better done in idiomatic C, not with JSON strings.
Quoted 4 lines
Yes, if we need to do that, we can create a C API that both the QMP interface uses and we also use internally but why? All that does is introduce the chance that the C API will have more features than the QMP interface.
Why is that bad?
Internal and external interfaces have very different tradeoffs.
An internal interface should eschew backward compatibility and embrace change.
An external interface needs to be stable, yet extensible.
It's therefore advisable to separate the two. Otherwise the internal interface gets bogged down with undue compatibility considerations (backward & forward), or the external interface suffers unnecessary churn.
When we designed QMP, we took special care to make it support compatible evolution. We consciously made it a proper protocol, not RPC to internal C interfaces. Are you proposing we go back to square one and reargue the basics of QMP?
Quoted 5 lines
If we don't use these functions in QEMU, then how do we know that these functions have reasonable semantics? This is exactly the problem we suffer today. We have internal APIs that do reasonable things but everything that deals with QMP is a special case. That creates too many opportunities to get things wrong.
No, the problem we suffer today is that we didn't design the external interface properly above the level of basic protocol. We took a shortcut and converted existing monitor commands. We've since discovered we don't like that approach.
Instead of giving up on protocol design without even trying and just expose whatever internal interfaces strike us as useful via RPC, let's design the external interface properly.
Quoted 2 lines
I think it's a vitally important requirement that all future QMP functions have direct mappings to a C interface.
Why?
Quoted 5 lines
The long term goal
should be for that interface to be used by all of the command line arguments, SDL, and the human monitor. If those things only relied on a single API and we exposed that API via QMP, than we would have an extremely useful interface.
Yes, command line, human monitor and QMP should use internal interfaces to do the real work, thus separate the real work neatly from interface-specific stuff like parsing text.
No, that doesn't mean we should expose internal interfaces via RPC.
[Qemu-devel] Re: Should QMP be RPC to internal C interfaces?
Quoted 15 lines · Markus Armbruster
Quoted 6 lines
Because it's too easy to get it wrong in QEMU. Here's the rationale.
If I can't trivially call a QMP function in C, then I'm not going to use QMP functions within QEMU. I'm not going to create an embedded JSON string just to call a function with three integer arguments.
Yes, an internal interface is better done in idiomatic C, not with JSON strings.
Quoted 5 lines
Yes, if we need to do that, we can create a C API that both the QMP interface uses and we also use internally but why? All that does is introduce the chance that the C API will have more features than the QMP interface.
Quoted 9 lines
Why is that bad?
Internal and external interfaces have very different tradeoffs.
An internal interface should eschew backward compatibility and embrace change.
An external interface needs to be stable, yet extensible.
Nope.
The internal interface should be the external interface.
Otherwise, the external interface is going to rot.
Quoted 10 lines
It's therefore advisable to separate the two. Otherwise the internal interface gets bogged down with undue compatibility considerations (backward& forward), or the external interface suffers unnecessary churn.
When we designed QMP, we took special care to make it support compatible evolution. We consciously made it a proper protocol, not RPC to internal C interfaces. Are you proposing we go back to square one and reargue the basics of QMP?
All the pretty JSON interfaces don't matter if we're not exposing a useful API.
We're trying to do too much. We've been more or less completing ignoring the problem of creating a useful API for users to consume.
We need to simplify. We simplify by reducing scope. Of the things that are important, a useful API is more important than whether it maps to your favorite dynamic language in an elegant way.
Quoted 9 lines
No, the problem we suffer today is that we didn't design the external interface properly above the level of basic protocol. We took a shortcut and converted existing monitor commands. We've since discovered we don't like that approach.
Instead of giving up on protocol design without even trying and just expose whatever internal interfaces strike us as useful via RPC, let's design the external interface properly.
What does that even mean? How do you describe the external interface properly?
It's a hell of a lot simpler to design the external interface as a C API, and then to implement the external interface as a C API. Why make life harder than that?
Quoted 5 lines
Quoted 3 lines
I think it's a vitally important requirement that all future QMP functions have direct mappings to a C interface.
Why?
So that we can consume those APIs within QEMU.
Quoted 6 lines
Quoted 6 lines
The long term goal
should be for that interface to be used by all of the command line arguments, SDL, and the human monitor. If those things only relied on a single API and we exposed that API via QMP, than we would have an extremely useful interface.
Quoted 6 lines
Yes, command line, human monitor and QMP should use internal interfaces to do the real work, thus separate the real work neatly from interface-specific stuff like parsing text.
No, that doesn't mean we should expose internal interfaces via RPC.
Having two interfaces guarantees failure. What's the separation between internal and external? Is qdev internal or external?
Regards,
Anthony Liguori
[Qemu-devel] Re: Should QMP be RPC to internal C interfaces?
Quoted 1 line · Anthony Liguori
Having two interfaces guarantees failure. What's the separation between internal and external? Is qdev internal or external?
Let me put it another way, compatibility cannot be an after thought.
We need to think deeply about compatibility when we design our interfaces and we're going to have to redesign interfaces with compatibility in mind. It's a hard problem but it's solvable. Just defaulting arguments in QMP doesn't do anything to improve compatibility.
We cannot radically change our internal implementations and expect to bridge it all in some special sauce code somewhere.
This also suggests that we're going to have to practice deprecation to evolve our APIs in a reasonable fashion.
Regards,
Anthony Liguori
Quoted 3 lines
Regards,
Anthony Liguori
[Qemu-devel] Re: Should QMP be RPC to internal C interfaces?
Quoted 10 lines · On Mon, 30 Aug 2010 10:38:45 -0500 Anthony Liguori <[email protected]>…
Quoted 2 lines · Anthony Liguori
Having two interfaces guarantees failure. What's the separation between internal and external? Is qdev internal or external?
Let me put it another way, compatibility cannot be an after thought.
We need to think deeply about compatibility when we design our interfaces and we're going to have to redesign interfaces with compatibility in mind. It's a hard problem but it's solvable. Just defaulting arguments in QMP doesn't do anything to improve compatibility.
The point is: C compat sucks, QMP's doesn't. QMP will suck too if we direct map the two.
You seem to think it's worth it, we don't. How do we solve that?
Quoted 5 lines
We cannot radically change our internal implementations and expect to bridge it all in some special sauce code somewhere.
This also suggests that we're going to have to practice deprecation to evolve our APIs in a reasonable fashion.
Deprecation should be mostly used for bad defined commands, not for simple extensions.
[Qemu-devel] Re: Should QMP be RPC to internal C interfaces?
Quoted 11 lines · Luiz Capitulino
Quoted 7 lines · On Mon, 30 Aug 2010 10:38:45 -0500 Anthony Liguori<[email protected]>…
Quoted 3 lines · Anthony Liguori
Having two interfaces guarantees failure. What's the separation between internal and external? Is qdev internal or external?
Let me put it another way, compatibility cannot be an after thought.
Quoted 8 lines
Quoted 5 lines
We need to think deeply about compatibility when we design our interfaces and we're going to have to redesign interfaces with compatibility in mind. It's a hard problem but it's solvable. Just defaulting arguments in QMP doesn't do anything to improve compatibility.
The point is: C compat sucks, QMP's doesn't. QMP will suck too if we direct map the two.
Prove it because I don't believe you :-)
Here's a counter example. Let's say we have function do_something that takes three arguments. We add a forth additional argument and default it. And old client can call with three arguments and it just works because the default is "do what we used to do".
How do we discover if this new version of do_something supports four arguments instead of three? Do we ignore unknown arguments? We can't because that fourth argument may be important (but guess what, we do today and that's busted). Because I might be passing "do something new" as the fourth argument because I expressly didn't want the old behavior.
Okay, so now we have to add a mechanism to enumerate function names and their arguments. Now I can detect the fourth argument. What if the argument is a structure and we want to add a new field to the structure?
How do I know, as a client, that I can add another field to the structure and that field will be respected?
You'd end up writing a full blown schema and then enforcing the schema. As a client, you'd have to download the schema and then try to detect the features a command supported. That's an absolutely awful compat interface IMHO especially if you have different downstream schemas.
On the other hand, introducing a new function that takes a fourth argument, or a new type of structure with an added argument simplifies the problem tremendously. It means we might have four versions of the same function but that's unavoidable. Flattening this for the client makes their lives much simpler.
Quoted 9 lines
Quoted 6 lines
We cannot radically change our internal implementations and expect to bridge it all in some special sauce code somewhere.
This also suggests that we're going to have to practice deprecation to evolve our APIs in a reasonable fashion.
Deprecation should be mostly used for bad defined commands, not for simple extensions.
It's better to talk about concrete things so please give an example of how you can provide compatibility in QMP that you can't do in C and we can discuss it.
My position is that we aren't any closer to having compatible APIs then we were with the human monitor. I think we need to focus on compatibility and that that has to be solved as the QEMU interface level. I contend that it's not solvable at the QMP level.
Regards,
Anthony Liguori
Regards,
Anthony Liguori
[Qemu-devel] Re: Should QMP be RPC to internal C interfaces?
Anthony Liguori <[email protected]> writes:
[...]
Quoted 4 lines
My position is that we aren't any closer to having compatible APIs then we were with the human monitor. I think we need to focus on compatibility and that that has to be solved as the QEMU interface level. I contend that it's not solvable at the QMP level.
We've argued from day 0 every step along the way. And here we are, one year later, still arguing about the very basics.
There's a fundamental disagreement. I want to keep QMP the way it was
designed: supporting compatible evolution. You want to remake it from the ground up as RPC to internal C interfaces.
It seems exceedingly unlikely to me that we can agree on the wisdom of such a remake. We can repeat and elaborate on our arguments for a while, but let's face it: we want different things.
I'm afraid I can't build you the thing you want. The best I can offer is to step out of the way and let you build it.
[Qemu-devel] Re: Should QMP be RPC to internal C interfaces?
Quoted 11 lines · On Tue, 31 Aug 2010 14:48:51 +0200 Markus Armbruster <[email protected]> wrote:
Anthony Liguori <[email protected]> writes:
[...]
Quoted 4 lines
My position is that we aren't any closer to having compatible APIs then we were with the human monitor. I think we need to focus on compatibility and that that has to be solved as the QEMU interface level. I contend that it's not solvable at the QMP level.
We've argued from day 0 every step along the way. And here we are, one year later, still arguing about the very basics.
Quoted 7 lines
There's a fundamental disagreement. I want to keep QMP the way it was
designed: supporting compatible evolution. You want to remake it from the ground up as RPC to internal C interfaces.It seems exceedingly unlikely to me that we can agree on the wisdom of such a remake. We can repeat and elaborate on our arguments for a while, but let's face it: we want different things.
Yes, that's the fundamental problem here.
Quoted 3 lines
I'm afraid I can't build you the thing you want. The best I can offer is to step out of the way and let you build it.
[Qemu-devel] Re: Should QMP be RPC to internal C interfaces?
Quoted 20 lines · Luiz Capitulino
Quoted 16 lines · On Tue, 31 Aug 2010 14:48:51 +0200 Markus Armbruster<[email protected]> wrote:
Anthony Liguori<[email protected]> writes:
[...]
Quoted 5 lines
My position is that we aren't any closer to having compatible APIs then we were with the human monitor. I think we need to focus on compatibility and that that has to be solved as the QEMU interface level. I contend that it's not solvable at the QMP level.
We've argued from day 0 every step along the way. And here we are, one year later, still arguing about the very basics.
There's a fundamental disagreement. I want to keep QMP the way it was
designed: supporting compatible evolution. You want to remake it from the ground up as RPC to internal C interfaces.
Quoted 6 lines
Quoted 4 lines
It seems exceedingly unlikely to me that we can agree on the wisdom of such a remake. We can repeat and elaborate on our arguments for a while, but let's face it: we want different things.
Yes, that's the fundamental problem here.
Okay, so what's the path forward?
I've proposed something that I think can get us out of the rut that we're in. I've got code to support that.
Ignoring what I've proposed, what do we do differently?
Regards,
Anthony Liguori
Quoted 5 lines
Quoted 4 lines
I'm afraid I can't build you the thing you want. The best I can offer is to step out of the way and let you build it.
[Qemu-devel] Re: Should QMP be RPC to internal C interfaces?
In reply to Anthony LiguoriQuoted 20 lines · Anthony Liguori
Quoted 19 lines · Markus Armbruster
Quoted 6 lines
Because it's too easy to get it wrong in QEMU. Here's the rationale.
If I can't trivially call a QMP function in C, then I'm not going to use QMP functions within QEMU. I'm not going to create an embedded JSON string just to call a function with three integer arguments.
Yes, an internal interface is better done in idiomatic C, not with JSON strings.
Quoted 5 lines
Yes, if we need to do that, we can create a C API that both the QMP interface uses and we also use internally but why? All that does is introduce the chance that the C API will have more features than the QMP interface.
Why is that bad?
Internal and external interfaces have very different tradeoffs.
Quoted 7 lines
Quoted 5 lines
An internal interface should eschew backward compatibility and embrace change.
An external interface needs to be stable, yet extensible.
Nope.
Which part(s) do you disagree with?
(1) Internal and external interfaces have very different tradeoffs.
(2) An internal interface should eschew backward compatibility and embrace change.
(3) An external interface needs to be stable, yet extensible.
If none of the above, then how do you propose to resolve the tension between external and internal interfaces?
Quoted 18 lines
The internal interface should be the external interface.
Otherwise, the external interface is going to rot.
Quoted 10 lines
It's therefore advisable to separate the two. Otherwise the internal interface gets bogged down with undue compatibility considerations (backward& forward), or the external interface suffers unnecessary churn.
When we designed QMP, we took special care to make it support compatible evolution. We consciously made it a proper protocol, not RPC to internal C interfaces. Are you proposing we go back to square one and reargue the basics of QMP?
All the pretty JSON interfaces don't matter if we're not exposing a useful API.
Quoted 6 lines
We're trying to do too much. We've been more or less completing ignoring the problem of creating a useful API for users to consume.
We need to simplify. We simplify by reducing scope. Of the things that are important, a useful API is more important than whether it maps to your favorite dynamic language in an elegant way.
So you do propose we go back to square one and reargue the basics of QMP.
Quoted 10 lines
Quoted 9 lines
No, the problem we suffer today is that we didn't design the external interface properly above the level of basic protocol. We took a shortcut and converted existing monitor commands. We've since discovered we don't like that approach.
Instead of giving up on protocol design without even trying and just expose whatever internal interfaces strike us as useful via RPC, let's design the external interface properly.
Quoted 6 lines
What does that even mean? How do you describe the external interface properly?
It's a hell of a lot simpler to design the external interface as a C API, and then to implement the external interface as a C API. Why make life harder than that?
You define the external interace the same how you define any interface:
you specify operations, arguments, returns and so forth.
C declarations are not a specification. For instance, a C prototype is only a part of a function's specification. It says too little about possible values, nothing about error conditions, semantics and so forth.
There are "a few" successful internet protocols that aren't RPC to a C interface.
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
I think it's a vitally important requirement that all future QMP functions have direct mappings to a C interface.
Why?
So that we can consume those APIs within QEMU.
QMP functions should consume the internal APIs.
A QMP function is concerned with interfacing and compatibility, and leaves the actual work to the internal APIs. Separation of concerns.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
The long term goal
should be for that interface to be used by all of the command line arguments, SDL, and the human monitor. If those things only relied on a single API and we exposed that API via QMP, than we would have an extremely useful interface.
Quoted 8 lines
Quoted 6 lines
Yes, command line, human monitor and QMP should use internal interfaces to do the real work, thus separate the real work neatly from interface-specific stuff like parsing text.
No, that doesn't mean we should expose internal interfaces via RPC.
Having two interfaces guarantees failure.
The kernel tries hard to separate external and internal interfaces. It's been failing quite successfully.
Quoted 2 lines
What's the separation
between internal and external? Is qdev internal or external?
qdev is the device model, i.e. a generic interface to devices. qdev.h is an internal interface. It has a part facing generic code: the generic interface to devices. And it has a part facing devices: the abstract interface the devices need to implement, plus stuff to help with that.
The external interface is layered onto the internal interface. -device / device_add expose a selected part of qdev externally. info qtree exposes some more.
We could do a better job specifying either of the interfaces. But removing the distinction between internal and external interface doesn't improve the specification of the external interface one bit.
The internal interface is much richer. It contains things that are of no interest remotely, including but not limited to the part facing devices.
It's designed for local, not remote use. In particular, it makes liberal use of pointers. How would you handle those over RPC?
It can be changed easily, because the impact is limited to QEMU itself. We don't do compatibility there, we just change it and fix up the users.
[Qemu-devel] Re: Should QMP be RPC to internal C interfaces?
Quoted 39 lines · Markus Armbruster
Quoted 24 lines · Anthony Liguori
Quoted 22 lines · Markus Armbruster
Quoted 7 lines
Because it's too easy to get it wrong in QEMU. Here's the rationale.
If I can't trivially call a QMP function in C, then I'm not going to use QMP functions within QEMU. I'm not going to create an embedded JSON string just to call a function with three integer arguments.
Yes, an internal interface is better done in idiomatic C, not with JSON strings.
Quoted 6 lines
Yes, if we need to do that, we can create a C API that both the QMP interface uses and we also use internally but why? All that does is introduce the chance that the C API will have more features than the QMP interface.
Why is that bad?
Internal and external interfaces have very different tradeoffs.
Quoted 8 lines
Quoted 6 lines
An internal interface should eschew backward compatibility and embrace change.
An external interface needs to be stable, yet extensible.
Nope.
Which part(s) do you disagree with?
(1) Internal and external interfaces have very different tradeoffs.
Quoted 8 lines
(2) An internal interface should eschew backward compatibility and embrace change.
(3) An external interface needs to be stable, yet extensible.
If none of the above, then how do you propose to resolve the tension between external and internal interfaces?
What's the different between an internal an external interface?
You're "external" interface at some point needs code in QEMU that implements the interface. Why can't that interface be used by another part of QEMU? Why is it then not an internal interface?
A big part of my argument here, is that we should be eating our own dog food with respect to QMP.We need to simplify. We simplify by reducing scope. Of the things
Quoted 6 lines
Quoted 3 lines
that are important, a useful API is more important than whether it maps to your favorite dynamic language in an elegant way.
So you do propose we go back to square one and reargue the basics of QMP.
I don't even know what you mean by that. Very, very, concretely, I'm suggesting that we:
1) Declare what we have supported
2) Design useful interfaces in C that we can consume internally (see above comment about eating our dog food)
3) Bridge those useful interfaces via QMP. Ideally, we would make the QMP code dead simple such that it could be generated. If ya'll want to pretty-ify it by making things have default values or whatever type of syntactic sugar you want, that's fine.
4) I'm specifically suggesting that we don't start at the QMP level when defining new interfaces.
Quoted 7 lines
Quoted 7 lines
What does that even mean? How do you describe the external interface properly?
It's a hell of a lot simpler to design the external interface as a C API, and then to implement the external interface as a C API. Why make life harder than that?
Quoted 7 lines
You define the external interace the same how you define any interface:
you specify operations, arguments, returns and so forth.C declarations are not a specification. For instance, a C prototype is only a part of a function's specification. It says too little about possible values, nothing about error conditions, semantics and so forth.
Well here's how I've attempted to address it:
http://repo.or.cz/w/qemu/aliguori-queue.git/blob/refs/heads/qpi:/qpi-migration.h
We need our internal interfaces to be just as well specified as we need QMP to.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
I think it's a vitally important requirement that all future QMP functions have direct mappings to a C interface.
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
Why?
So that we can consume those APIs within QEMU.
QMP functions should consume the internal APIs.
A QMP function is concerned with interfacing and compatibility, and leaves the actual work to the internal APIs. Separation of concerns.
I don't think you can separate the two. I'd be happy to be proven wrong but so far, such an existence proof doesn't exist.
Quoted 9 lines
Quoted 3 lines
What's the separation
between internal and external? Is qdev internal or external?qdev is the device model, i.e. a generic interface to devices.
qdev.his an internal interface. It has a part facing generic code: the generic interface to devices. And it has a part facing devices: the abstract interface the devices need to implement, plus stuff to help with that.
Quoted 4 lines
The external interface is layered onto the internal interface. -device /
device_addexpose a selected part of qdev externally. info qtree exposes some more.
device_add is intimately tied into qdev. If you don't believe me, look at http://repo.or.cz/w/qemu/aliguori.git/shortlog/refs/heads/features/qdev-refactor to see how much work it took to separate the two.
Concretely, I'm looking at doing some significant modelling changes. To consider virtio, we have virtio-net-pci today but I'd like to move to having a virtio-pci and adding virtio-net to virtio-pci. This would totally change the -device interface and break backwards compatibility.
So how am I going to address that? I don't know yet but I'm come to believe that -device as an interface is going to be a PITA to maintain and that we should have thought much harder about compatibility.
Quoted 8 lines
We could do a better job specifying either of the interfaces. But removing the distinction between internal and external interface doesn't improve the specification of the external interface one bit.
The internal interface is much richer. It contains things that are of no interest remotely, including but not limited to the part facing devices.
Really, you're argument boils down to: if we separate 'internal' and 'external' interfaces, we can totally ignore compatibility in the 'internal' interface and implement compatibility solely in the 'external' part of the interface.
I'm calling B.S. on that. Compatibility is a hard problem and we need to think about it when introducing any interfaces.
Quoted 3 lines
It's designed for local, not remote use. In particular, it makes liberal use of pointers. How would you handle those over RPC?
A boxed type. We discussed this as KVM Forum. It would look like:
QBoxed *qboxed_new(const char *type, void *pointer); void *qboxed_get_ptr(QBoxed *obj);
It marshals to a token. Internally, there's a static global map that associates a tuple of (type, token) with pointer.
A QPI operation like:
MigrationState *qpi_migration_create(Error **err);
Maps to QMP like:
string: token qmp_migration_create(void)
Or we could use a special QMP type for handles, it's really just a detail.
Regards,
Anthony Liguori
Quoted 3 lines
It can be changed easily, because the impact is limited to QEMU itself. We don't do compatibility there, we just change it and fix up the users.
[Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
In reply to Anthony LiguoriQuoted 9 lines · Anthony Liguori
Quoted 4 lines
NACK. It has always been allowed& valid to call query-balloon to get the current balloon level. We must not throw an error just because the recently added mem stats can't be refreshed.
I think that's a fair comment but why even bother fixing the command. Let's introduce a new command that just gets a single piece of information instead of having a command return lots of information.
Instead, let's have query-balloon do the same thing as it did in 0.12 and add a new command for 0.13 (query-balloon-stats) that does the stats with timeout?
That way we won't have regressions from 0.12 and have the new behaviour only for 0.13, which we can say is 'experimental'.
Amit
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 11 lines · Amit Shah
Quoted 9 lines · Anthony Liguori
Quoted 4 lines
NACK. It has always been allowed& valid to call query-balloon to get the current balloon level. We must not throw an error just because the recently added mem stats can't be refreshed.
I think that's a fair comment but why even bother fixing the command. Let's introduce a new command that just gets a single piece of information instead of having a command return lots of information.
Quoted 6 lines
Instead, let's have query-balloon do the same thing as it did in 0.12 and add a new command for 0.13 (query-balloon-stats) that does the stats with timeout?
That way we won't have regressions from 0.12 and have the new behaviour only for 0.13, which we can say is 'experimental'.
Sounds sensible to me. Not too late for fixing 0.13? Anthony?
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 14 lines · Markus Armbruster
Quoted 11 lines · Amit Shah
Quoted 9 lines · Anthony Liguori
Quoted 4 lines
NACK. It has always been allowed& valid to call query-balloon
to get the current balloon level. We must not throw an error just because the recently added mem stats can't be refreshed.I think that's a fair comment but why even bother fixing the command. Let's introduce a new command that just gets a single piece of information instead of having a command return lots of information.
Quoted 9 lines
Quoted 7 lines
Instead, let's have query-balloon do the same thing as it did in 0.12 and add a new command for 0.13 (query-balloon-stats) that does the stats with timeout?
That way we won't have regressions from 0.12 and have the new behaviour only for 0.13, which we can say is 'experimental'.
Sounds sensible to me. Not too late for fixing 0.13? Anthony?
No new commands for 0.13. I think the only option is to revert the query-balloon behavior. A new command is too risky as -rc1 is imminent.
Regards,
Anthony Liguori
Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message
Quoted 22 lines · Anthony Liguori
Quoted 21 lines · Markus Armbruster
Quoted 18 lines · Amit Shah
Quoted 9 lines · Anthony Liguori
Quoted 4 lines
NACK. It has always been allowed& valid to call query-balloon
to get the current balloon level. We must not throw an error just because the recently added mem stats can't be refreshed.I think that's a fair comment but why even bother fixing the command. Let's introduce a new command that just gets a single piece of information instead of having a command return lots of information.
Instead, let's have query-balloon do the same thing as it did in 0.12 and add a new command for 0.13 (query-balloon-stats) that does the stats with timeout?
That way we won't have regressions from 0.12 and have the new behaviour only for 0.13, which we can say is 'experimental'.
Quoted 6 lines
Quoted 2 lines
Sounds sensible to me. Not too late for fixing 0.13? Anthony?
No new commands for 0.13. I think the only option is to revert the query-balloon behavior. A new command is too risky as -rc1 is imminent.
Anyone care to submit a patch?
PATCH 1/1 Disable virtio-balloon memory stats interface
Just got back from vacation and saw this thread. I agree with Anthony that the best thing to do is disable the memory stats interface for 0.13. We need to fix the underlying problems in qemu with respect to asynchronous commands first, then we can look at re-enabling the feature.
The virtio feature negotiation mechanism allows the feature to be disabled by commenting out a single line of code. This late in the 0.13 release cycle, this is the safest option. We can refactor/remove code in the development tree as needed.
Quoted 1 line · Markus Armbruster
Anyone care to submit a patch?
Sure...
[PATCH] Disable virtio-balloon memory stats interface
The addition of memory stats reporting to the virtio balloon causes the 'info balloon' command to become asynchronous. This is a regression because management tools that consume this command were not designed to handle lost or delayed responses.
To fix this regression, the virtio balloon memory stats feature is being disabled in qemu-0.13.
hw/virtio-balloon.c +12 -1
@@ -190,7 +190,18 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,static uint32_t virtio_balloon_get_features (VirtIODevice *vdev,uint32_t f) { - f |= (1 <<VIRTIO_BALLOON_F_STATS_VQ ); +/* +* The addition of memory stats reporting to the virtio balloon causes +* the 'info balloon' command to become asynchronous. This is a regression +* because management tools that consume this command were not designed to +* handle lost or delayed responses. +* +* To fix this regression, the virtio balloon memory stats feature is being +* disabled in qemu-0.13. +* +* -aglitke +*/ +/* f |= (1 << VIRTIO_BALLOON_F_STATS_VQ); */ return f; }
Signature · 2 lines
Thanks, Adam
[Qemu-devel] Re: [PATCH] Disable virtio-balloon memory stats interface
Quoted 15 lines · Adam Litke
The addition of memory stats reporting to the virtio balloon causes the 'info balloon' command to become asynchronous. This is a regression because management tools that consume this command were not designed to handle lost or delayed responses.
To fix this regression, the virtio balloon memory stats feature is being disabled in qemu-0.13.
Signed-off-by Adam Litke <[email protected]>hw/virtio-balloon.c +0
-0
hw/virtio-balloon.c static void virtio_balloon_set_config(VirtIODevice *vdev, · line 197
Quoted 9 lines
hw/virtio-balloon.c +5
-1@@ -190,7 +190,18 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,static uint32_t virtio_balloon_get_features (VirtIODevice *vdev,uint32_t f) { - f |= (1 <<VIRTIO_BALLOON_F_STATS_VQ ); +/* +* The addition of memory stats reporting to the virtio balloon causes +* the 'info balloon' command to become asynchronous. This is a regression +* because management tools that consume this command were not designed to +* handle lost or delayed responses.
Rather, the monitor now gets stuck when it originally didn't.
hw/virtio-balloon.c static void virtio_balloon_set_config(VirtIODevice *vdev, · line 206
Quoted 9 lines
hw/virtio-balloon.c +7
-0+* +* To fix this regression, the virtio balloon memory stats feature is being +* disabled in qemu-0.13. +* +* -aglitke +*/ +/* f |= (1 << VIRTIO_BALLOON_F_STATS_VQ); */ return f; }
Amit
[Qemu-devel] Re: [PATCH] Disable virtio-balloon memory stats interface
Quoted 17 lines · Amit Shah
Quoted 16 lines · Adam Litke
The addition of memory stats reporting to the virtio balloon causes the 'info balloon' command to become asynchronous. This is a regression because management tools that consume this command were not designed to handle lost or delayed responses.
To fix this regression, the virtio balloon memory stats feature is being disabled in qemu-0.13.
Signed-off-by Adam Litke <[email protected]>hw/virtio-balloon.c +0
-0@@ -190,7 +190,18 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
Quoted 10 lines
hw/virtio-balloon.c static void virtio_balloon_set_config(VirtIODevice *vdev, · line 197
Quoted 8 lines
hw/virtio-balloon.c +5
-1static uint32_t virtio_balloon_get_features (VirtIODevice *vdev,uint32_t f) { - f |= (1 <<VIRTIO_BALLOON_F_STATS_VQ ); +/* +* The addition of memory stats reporting to the virtio balloon causes +* the 'info balloon' command to become asynchronous. This is a regression +* because management tools that consume this command were not designed to +* handle lost or delayed responses. Rather, the monitor now gets stuck when it originally didn't.
Care to update the comment and the commit message and send as a top-level patch (and also for 0.13)?
Thanks
Amit
PATCH v3 3/3 balloon: Don't try fetching info if machine is stopped
In reply to Amit ShahIf the machine is stopped and 'info balloon' is invoked, instead of querying the guest and waiting for a timeout, just display the most recent stats available in this case and log an error report.
See https://bugzilla.redhat.com/show_bug.cgi?id=623903
--- balloon.c | 11 +++++++---- balloon.h | 6 ++++-- hw/virtio-balloon.c | 9 +++++++-- 3 files changed, 18 insertions(+), 8 deletions(-)
balloon.c +7 -4
@@ -43,17 +43,20 @@ void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)int qemu_balloon (ram_addr_t target,MonitorCompletion cb,void *opaque) {if (qemu_balloon_event) { -qemu_balloon_event (qemu_balloon_event_opaque, target, cb, opaque); +qemu_balloon_event (qemu_balloon_event_opaque, target, cb, opaque, + false);return 1 ; }else {return 0 ; } } -int qemu_balloon_status (MonitorCompletion cb,void *opaque) +int qemu_balloon_status (MonitorCompletion cb,void *opaque, +bool get_cached_data) {if (qemu_balloon_event) { -qemu_balloon_event (qemu_balloon_event_opaque,0 , cb, opaque); +qemu_balloon_event (qemu_balloon_event_opaque,0 , cb, opaque, + get_cached_data);return 1 ; }else {return 0 ;
@@ -113,7 +116,7 @@ int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque)return -1 ; } - ret =qemu_balloon_status (cb, opaque); + ret =qemu_balloon_status (cb, opaque, !vm_running);if (!ret) {qerror_report (QERR_DEVICE_NOT_ACTIVE ,"balloon" );return -1 ;
balloon.h +4 -2
@@ -17,13 +17,15 @@#include "monitor.h" typedef void (QEMUBalloonEvent )(void *opaque,ram_addr_t target, -MonitorCompletion cb,void *cb_data); +MonitorCompletion cb,void *cb_data, +bool get_cached_data);void qemu_add_balloon_handler (QEMUBalloonEvent *func,void *opaque);int qemu_balloon (ram_addr_t target,MonitorCompletion cb,void *opaque); -int qemu_balloon_status (MonitorCompletion cb,void *opaque); +int qemu_balloon_status (MonitorCompletion cb,void *opaque, +bool get_cached_data);void monitor_print_balloon (Monitor *mon,const QObject *data);int do_info_balloon (Monitor *mon,MonitorCompletion cb,void *opaque);
hw/virtio-balloon.c +7 -2
@@ -204,7 +204,8 @@ static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f) }static void virtio_balloon_to_target (void *opaque,ram_addr_t target, -MonitorCompletion cb,void *cb_data) +MonitorCompletion cb,void *cb_data, +bool get_cached_data) {VirtIOBalloon *dev = opaque;
@@ -222,8 +223,12 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,return ; } dev->stats_callback = cb; - dev->stats_opaque_callback_data = cb_data; + dev->stats_opaque_callback_data = cb_data;if (dev->vdev.guest_features & (1 <<VIRTIO_BALLOON_F_STATS_VQ )) { +if (get_cached_data) { +show_old_stats (dev); +return ; + }qemu_mod_timer (dev->timer,qemu_get_clock (rt_clock) +5000 );virtqueue_push (dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);virtio_notify (&dev->vdev, dev->svq);
Signature · 1 line
1.7.2.2