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

[Markus Armbruster]

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