Re: [RFC v3 04/29] vhost: Add x-vhost-enable-shadow-vq qmp

[Eugenio Perez Martin <eperezma@redhat.com>]

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.
> >
> >> [...]
> >>
>