| Message ID | 20210519162903.1172366-5-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vDPA software assisted live migration | expand |
Eugenio Pérez <eperezma@redhat.com> writes: > Command to enable shadow virtqueue looks like: > > { "execute": "x-vhost-enable-shadow-vq", > "arguments": { "name": "dev0", "enable": true } } > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > qapi/net.json | 22 ++++++++++++++++++++++ > hw/virtio/vhost.c | 6 ++++++ > 2 files changed, 28 insertions(+) > > diff --git a/qapi/net.json b/qapi/net.json > index c31748c87f..660feafdd2 100644 > --- a/qapi/net.json > +++ b/qapi/net.json > @@ -77,6 +77,28 @@ > ## > { 'command': 'netdev_del', 'data': {'id': 'str'} } > > +## > +# @x-vhost-enable-shadow-vq: > +# > +# Use vhost shadow virtqueue. > +# > +# @name: the device name of the VirtIO device > +# > +# @enable: true to use he alternate shadow VQ notification path Typo "he". What's a "notification path", and why should I care? Maybe # @enable: Enable alternate shadow VQ notification > +# > +# Returns: Error if failure, or 'no error' for success. Not found if vhost is not enabled. This is confusing. What do you mean by "Not found"? If you mean DeviceNotFound: 1. Not actually true: qmp_x_vhost_enable_shadow_vq() always fails with GenericError. Perhaps later patches will change that. 2. Do you really need to distinguish "vhost is not enabled" from other errors? > +# > +# Since: 6.1 > +# > +# Example: > +# > +# -> { "execute": "x-vhost-enable-shadow-vq", "arguments": { "name": "virtio-net", "enable": false } } Please break the long line, e.g. like this: # -> { "execute": "x-vhost-enable-shadow-vq", # "arguments": { "name": "virtio-net", "enable": false } } We normally show output in examples, too. > +# > +## > +{ 'command': 'x-vhost-enable-shadow-vq', > + 'data': {'name': 'str', 'enable': 'bool'}, > + 'if': 'defined(CONFIG_VHOST_KERNEL)' } > + > ## > # @NetLegacyNicOptions: > # > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 40f9f64ebd..c4c1f80661 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -15,6 +15,7 @@ > > #include "qemu/osdep.h" > #include "qapi/error.h" > +#include "qapi/qapi-commands-net.h" > #include "hw/virtio/vhost.h" > #include "qemu/atomic.h" > #include "qemu/range.h" > @@ -1831,3 +1832,8 @@ int vhost_net_set_backend(struct vhost_dev *hdev, > > return -1; > } > + > +void qmp_x_vhost_enable_shadow_vq(const char *name, bool enable, Error **errp) > +{ > + error_setg(errp, "Shadow virtqueue still not implemented"); > +}
On Fri, May 21, 2021 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote: > > Eugenio Pérez <eperezma@redhat.com> writes: > > > Command to enable shadow virtqueue looks like: > > > > { "execute": "x-vhost-enable-shadow-vq", > > "arguments": { "name": "dev0", "enable": true } } > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > qapi/net.json | 22 ++++++++++++++++++++++ > > hw/virtio/vhost.c | 6 ++++++ > > 2 files changed, 28 insertions(+) > > > > diff --git a/qapi/net.json b/qapi/net.json > > index c31748c87f..660feafdd2 100644 > > --- a/qapi/net.json > > +++ b/qapi/net.json > > @@ -77,6 +77,28 @@ > > ## > > { 'command': 'netdev_del', 'data': {'id': 'str'} } > > > > +## > > +# @x-vhost-enable-shadow-vq: > > +# > > +# Use vhost shadow virtqueue. > > +# > > +# @name: the device name of the VirtIO device > > +# > > +# @enable: true to use he alternate shadow VQ notification path > > Typo "he". > Thanks, I will fix it! > What's a "notification path", and why should I care? > > Maybe > > # @enable: Enable alternate shadow VQ notification > Your description is more accurate at some point of the series, I will fix it. > > +# > > +# Returns: Error if failure, or 'no error' for success. Not found if vhost is not enabled. > > This is confusing. What do you mean by "Not found"? > > If you mean DeviceNotFound: > > 1. Not actually true: qmp_x_vhost_enable_shadow_vq() always fails with > GenericError. Perhaps later patches will change that. > Right, I left the documentation in an intermediate state. At this point it will always return failure, and in future ones it depends on some conditions as you may have seen. If I carry the QMP command to future series, I will update the doc accordly to every commit. > 2. Do you really need to distinguish "vhost is not enabled" from other > errors? > SVQ cannot work if the device backend is not vhost, like qemu VirtIO dev. What I meant is that "qemu will only look for its name in the set of vhost devices, so you will have a device not found if the device is not a vhost one", which may not be 100% clear at first glance. Maybe this wording is better? > > +# > > +# Since: 6.1 > > +# > > +# Example: > > +# > > +# -> { "execute": "x-vhost-enable-shadow-vq", "arguments": { "name": "virtio-net", "enable": false } } > > Please break the long line, e.g. like this: > > # -> { "execute": "x-vhost-enable-shadow-vq", > # "arguments": { "name": "virtio-net", "enable": false } } > > We normally show output in examples, too. > Ok, I will fix both issues. Thanks! > > +# > > +## > > +{ 'command': 'x-vhost-enable-shadow-vq', > > + 'data': {'name': 'str', 'enable': 'bool'}, > > + 'if': 'defined(CONFIG_VHOST_KERNEL)' } > > + > > ## > > # @NetLegacyNicOptions: > > # > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index 40f9f64ebd..c4c1f80661 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -15,6 +15,7 @@ > > > > #include "qemu/osdep.h" > > #include "qapi/error.h" > > +#include "qapi/qapi-commands-net.h" > > #include "hw/virtio/vhost.h" > > #include "qemu/atomic.h" > > #include "qemu/range.h" > > @@ -1831,3 +1832,8 @@ int vhost_net_set_backend(struct vhost_dev *hdev, > > > > return -1; > > } > > + > > +void qmp_x_vhost_enable_shadow_vq(const char *name, bool enable, Error **errp) > > +{ > > + error_setg(errp, "Shadow virtqueue still not implemented"); > > +} >
Eugenio Perez Martin <eperezma@redhat.com> writes: > On Fri, May 21, 2021 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> Eugenio Pérez <eperezma@redhat.com> writes: >> >> > Command to enable shadow virtqueue looks like: >> > >> > { "execute": "x-vhost-enable-shadow-vq", >> > "arguments": { "name": "dev0", "enable": true } } >> > >> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >> > --- >> > qapi/net.json | 22 ++++++++++++++++++++++ >> > hw/virtio/vhost.c | 6 ++++++ >> > 2 files changed, 28 insertions(+) >> > >> > diff --git a/qapi/net.json b/qapi/net.json >> > index c31748c87f..660feafdd2 100644 >> > --- a/qapi/net.json >> > +++ b/qapi/net.json >> > @@ -77,6 +77,28 @@ >> > ## >> > { 'command': 'netdev_del', 'data': {'id': 'str'} } >> > >> > +## >> > +# @x-vhost-enable-shadow-vq: >> > +# >> > +# Use vhost shadow virtqueue. >> > +# >> > +# @name: the device name of the VirtIO device >> > +# >> > +# @enable: true to use he alternate shadow VQ notification path [...] >> > +# >> > +# Returns: Error if failure, or 'no error' for success. Not found if vhost is not enabled. >> >> This is confusing. What do you mean by "Not found"? >> >> If you mean DeviceNotFound: >> >> 1. Not actually true: qmp_x_vhost_enable_shadow_vq() always fails with >> GenericError. Perhaps later patches will change that. [...] >> 2. Do you really need to distinguish "vhost is not enabled" from other >> errors? >> > > SVQ cannot work if the device backend is not vhost, like qemu VirtIO > dev. What I meant is that "qemu will only look for its name in the set > of vhost devices, so you will have a device not found if the device is > not a vhost one", which may not be 100% clear at first glance. Maybe > this wording is better? We might be talking past each other. Let me try again :) The following question is *not* about the doc comment, it's about the *code*: what practical problem is solved by using DeviceNotFound instead of GenericError for some errors? [...]
On Tue, Jun 8, 2021 at 4:23 PM Markus Armbruster <armbru@redhat.com> wrote: > > Eugenio Perez Martin <eperezma@redhat.com> writes: > > > On Fri, May 21, 2021 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote: > >> > >> Eugenio Pérez <eperezma@redhat.com> writes: > >> > >> > Command to enable shadow virtqueue looks like: > >> > > >> > { "execute": "x-vhost-enable-shadow-vq", > >> > "arguments": { "name": "dev0", "enable": true } } > >> > > >> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > >> > --- > >> > qapi/net.json | 22 ++++++++++++++++++++++ > >> > hw/virtio/vhost.c | 6 ++++++ > >> > 2 files changed, 28 insertions(+) > >> > > >> > diff --git a/qapi/net.json b/qapi/net.json > >> > index c31748c87f..660feafdd2 100644 > >> > --- a/qapi/net.json > >> > +++ b/qapi/net.json > >> > @@ -77,6 +77,28 @@ > >> > ## > >> > { 'command': 'netdev_del', 'data': {'id': 'str'} } > >> > > >> > +## > >> > +# @x-vhost-enable-shadow-vq: > >> > +# > >> > +# Use vhost shadow virtqueue. > >> > +# > >> > +# @name: the device name of the VirtIO device > >> > +# > >> > +# @enable: true to use he alternate shadow VQ notification path > > [...] > > >> > +# > >> > +# Returns: Error if failure, or 'no error' for success. Not found if vhost is not enabled. > >> > >> This is confusing. What do you mean by "Not found"? > >> > >> If you mean DeviceNotFound: > >> > >> 1. Not actually true: qmp_x_vhost_enable_shadow_vq() always fails with > >> GenericError. Perhaps later patches will change that. > > [...] > > >> 2. Do you really need to distinguish "vhost is not enabled" from other > >> errors? > >> > > > > SVQ cannot work if the device backend is not vhost, like qemu VirtIO > > dev. What I meant is that "qemu will only look for its name in the set > > of vhost devices, so you will have a device not found if the device is > > not a vhost one", which may not be 100% clear at first glance. Maybe > > this wording is better? > > We might be talking past each other. Let me try again :) > > The following question is *not* about the doc comment, it's about the > *code*: what practical problem is solved by using DeviceNotFound instead > of GenericError for some errors? > Sorry, I'm not sure if I follow you :). At risk of being circular in this topic, the only use case I can think is to actually tell the difference between "the device does not exists, or is not a vhost device" and "the device does not support SVQ because X", where X can be "it uses a packed ring", "it uses event idx", ... I can only think of one practical use case, "if you see this error, you probably forgot to set vhost=on in the command line, or something is forbidding this device to be a vhost one". Having said that, I'm totally fine with using GenericError always, but I see the more fine-grained the error the better. What would be the advantage of also using GenericError here? Just to be sure that we are on the same page, I think this is better seen from PATCH 07/39: vhost: Route guest->host notification through shadow virtqueue. > [...] >
Eugenio Perez Martin <eperezma@redhat.com> writes: > On Tue, Jun 8, 2021 at 4:23 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> Eugenio Perez Martin <eperezma@redhat.com> writes: >> >> > On Fri, May 21, 2021 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> >> >> Eugenio Pérez <eperezma@redhat.com> writes: >> >> >> >> > Command to enable shadow virtqueue looks like: >> >> > >> >> > { "execute": "x-vhost-enable-shadow-vq", >> >> > "arguments": { "name": "dev0", "enable": true } } >> >> > >> >> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >> >> > --- >> >> > qapi/net.json | 22 ++++++++++++++++++++++ >> >> > hw/virtio/vhost.c | 6 ++++++ >> >> > 2 files changed, 28 insertions(+) >> >> > >> >> > diff --git a/qapi/net.json b/qapi/net.json >> >> > index c31748c87f..660feafdd2 100644 >> >> > --- a/qapi/net.json >> >> > +++ b/qapi/net.json >> >> > @@ -77,6 +77,28 @@ >> >> > ## >> >> > { 'command': 'netdev_del', 'data': {'id': 'str'} } >> >> > >> >> > +## >> >> > +# @x-vhost-enable-shadow-vq: >> >> > +# >> >> > +# Use vhost shadow virtqueue. >> >> > +# >> >> > +# @name: the device name of the VirtIO device >> >> > +# >> >> > +# @enable: true to use he alternate shadow VQ notification path >> >> [...] >> >> >> > +# >> >> > +# Returns: Error if failure, or 'no error' for success. Not found if vhost is not enabled. >> >> >> >> This is confusing. What do you mean by "Not found"? >> >> >> >> If you mean DeviceNotFound: >> >> >> >> 1. Not actually true: qmp_x_vhost_enable_shadow_vq() always fails with >> >> GenericError. Perhaps later patches will change that. >> >> [...] >> >> >> 2. Do you really need to distinguish "vhost is not enabled" from other >> >> errors? >> >> >> > >> > SVQ cannot work if the device backend is not vhost, like qemu VirtIO >> > dev. What I meant is that "qemu will only look for its name in the set >> > of vhost devices, so you will have a device not found if the device is >> > not a vhost one", which may not be 100% clear at first glance. Maybe >> > this wording is better? >> >> We might be talking past each other. Let me try again :) >> >> The following question is *not* about the doc comment, it's about the >> *code*: what practical problem is solved by using DeviceNotFound instead >> of GenericError for some errors? >> > > Sorry, I'm not sure if I follow you :). At risk of being circular in > this topic, the only use case I can think is to actually tell the > difference between "the device does not exists, or is not a vhost > device" and "the device does not support SVQ because X", where X can > be "it uses a packed ring", "it uses event idx", ... > > I can only think of one practical use case, "if you see this error, > you probably forgot to set vhost=on in the command line, or something > is forbidding this device to be a vhost one". Having said that, I'm > totally fine with using GenericError always, but I see the more > fine-grained the error the better. What would be the advantage of also > using GenericError here? In the initial design of the Error API, every error had its own distinct class. This provided for fine-grained errors. Adding a new error was bothersome: you had to define a new class, in qerror.h. Recompile the world. Conflict magnet. Constant temptation to reuse an existing error even when its error message is suboptimal, and the reuse of the class for another error conflates errors. After a bit under three years, we had 70 classes, used in almost 400 places. Management applications actually cared for just six classes. Bad error messages and development friction had turned out to be a real problem. Conflating errors pretty much not. We concluded that providing for fine-grained errors when next to nothing uses them was not worth the pain. So we ditched them: https://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg01958.html Commit ac839ccd8c3..adb2072ed0f Since them, we recommend to use GenericError unless there is a compelling reason not to. "Something might care someday" doesn't qualify. Learning by doing the wrong thing is painful and expensive, but at least the lessons tends to stick ;) Today, we have more than 4000 callers of error_setg(), and less than 40 of error_set(). > Just to be sure that we are on the same page, I think this is better > seen from PATCH 07/39: vhost: Route guest->host notification through > shadow virtqueue. > >> [...] >>
On Wed, Jun 9, 2021 at 1:46 PM Markus Armbruster <armbru@redhat.com> wrote: > > Eugenio Perez Martin <eperezma@redhat.com> writes: > > > On Tue, Jun 8, 2021 at 4:23 PM Markus Armbruster <armbru@redhat.com> wrote: > >> > >> Eugenio Perez Martin <eperezma@redhat.com> writes: > >> > >> > On Fri, May 21, 2021 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote: > >> >> > >> >> Eugenio Pérez <eperezma@redhat.com> writes: > >> >> > >> >> > Command to enable shadow virtqueue looks like: > >> >> > > >> >> > { "execute": "x-vhost-enable-shadow-vq", > >> >> > "arguments": { "name": "dev0", "enable": true } } > >> >> > > >> >> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > >> >> > --- > >> >> > qapi/net.json | 22 ++++++++++++++++++++++ > >> >> > hw/virtio/vhost.c | 6 ++++++ > >> >> > 2 files changed, 28 insertions(+) > >> >> > > >> >> > diff --git a/qapi/net.json b/qapi/net.json > >> >> > index c31748c87f..660feafdd2 100644 > >> >> > --- a/qapi/net.json > >> >> > +++ b/qapi/net.json > >> >> > @@ -77,6 +77,28 @@ > >> >> > ## > >> >> > { 'command': 'netdev_del', 'data': {'id': 'str'} } > >> >> > > >> >> > +## > >> >> > +# @x-vhost-enable-shadow-vq: > >> >> > +# > >> >> > +# Use vhost shadow virtqueue. > >> >> > +# > >> >> > +# @name: the device name of the VirtIO device > >> >> > +# > >> >> > +# @enable: true to use he alternate shadow VQ notification path > >> > >> [...] > >> > >> >> > +# > >> >> > +# Returns: Error if failure, or 'no error' for success. Not found if vhost is not enabled. > >> >> > >> >> This is confusing. What do you mean by "Not found"? > >> >> > >> >> If you mean DeviceNotFound: > >> >> > >> >> 1. Not actually true: qmp_x_vhost_enable_shadow_vq() always fails with > >> >> GenericError. Perhaps later patches will change that. > >> > >> [...] > >> > >> >> 2. Do you really need to distinguish "vhost is not enabled" from other > >> >> errors? > >> >> > >> > > >> > SVQ cannot work if the device backend is not vhost, like qemu VirtIO > >> > dev. What I meant is that "qemu will only look for its name in the set > >> > of vhost devices, so you will have a device not found if the device is > >> > not a vhost one", which may not be 100% clear at first glance. Maybe > >> > this wording is better? > >> > >> We might be talking past each other. Let me try again :) > >> > >> The following question is *not* about the doc comment, it's about the > >> *code*: what practical problem is solved by using DeviceNotFound instead > >> of GenericError for some errors? > >> > > > > Sorry, I'm not sure if I follow you :). At risk of being circular in > > this topic, the only use case I can think is to actually tell the > > difference between "the device does not exists, or is not a vhost > > device" and "the device does not support SVQ because X", where X can > > be "it uses a packed ring", "it uses event idx", ... > > > > I can only think of one practical use case, "if you see this error, > > you probably forgot to set vhost=on in the command line, or something > > is forbidding this device to be a vhost one". Having said that, I'm > > totally fine with using GenericError always, but I see the more > > fine-grained the error the better. What would be the advantage of also > > using GenericError here? > > In the initial design of the Error API, every error had its own distinct > class. This provided for fine-grained errors. > > Adding a new error was bothersome: you had to define a new class, in > qerror.h. Recompile the world. Conflict magnet. Constant temptation > to reuse an existing error even when its error message is suboptimal, > and the reuse of the class for another error conflates errors. > > After a bit under three years, we had 70 classes, used in almost 400 > places. Management applications actually cared for just six classes. > > Bad error messages and development friction had turned out to be a real > problem. Conflating errors pretty much not. > > We concluded that providing for fine-grained errors when next to nothing > uses them was not worth the pain. So we ditched them: > > https://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg01958.html > Commit ac839ccd8c3..adb2072ed0f > > Since them, we recommend to use GenericError unless there is a > compelling reason not to. "Something might care someday" doesn't > qualify. > > Learning by doing the wrong thing is painful and expensive, but at least > the lessons tends to stick ;) > > Today, we have more than 4000 callers of error_setg(), and less than 40 > of error_set(). > So let's do it with GenericError then :). Thanks for pointing it out, it will be fixed in the next revision! > > Just to be sure that we are on the same page, I think this is better > > seen from PATCH 07/39: vhost: Route guest->host notification through > > shadow virtqueue. > > > >> [...] > >> >
diff --git a/qapi/net.json b/qapi/net.json index c31748c87f..660feafdd2 100644 --- a/qapi/net.json +++ b/qapi/net.json @@ -77,6 +77,28 @@ ## { 'command': 'netdev_del', 'data': {'id': 'str'} } +## +# @x-vhost-enable-shadow-vq: +# +# Use vhost shadow virtqueue. +# +# @name: the device name of the VirtIO device +# +# @enable: true to use he alternate shadow VQ notification path +# +# Returns: Error if failure, or 'no error' for success. Not found if vhost is not enabled. +# +# Since: 6.1 +# +# Example: +# +# -> { "execute": "x-vhost-enable-shadow-vq", "arguments": { "name": "virtio-net", "enable": false } } +# +## +{ 'command': 'x-vhost-enable-shadow-vq', + 'data': {'name': 'str', 'enable': 'bool'}, + 'if': 'defined(CONFIG_VHOST_KERNEL)' } + ## # @NetLegacyNicOptions: # diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 40f9f64ebd..c4c1f80661 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -15,6 +15,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" +#include "qapi/qapi-commands-net.h" #include "hw/virtio/vhost.h" #include "qemu/atomic.h" #include "qemu/range.h" @@ -1831,3 +1832,8 @@ int vhost_net_set_backend(struct vhost_dev *hdev, return -1; } + +void qmp_x_vhost_enable_shadow_vq(const char *name, bool enable, Error **errp) +{ + error_setg(errp, "Shadow virtqueue still not implemented"); +}
Command to enable shadow virtqueue looks like: { "execute": "x-vhost-enable-shadow-vq", "arguments": { "name": "dev0", "enable": true } } Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- qapi/net.json | 22 ++++++++++++++++++++++ hw/virtio/vhost.c | 6 ++++++ 2 files changed, 28 insertions(+)