Re: [PATCH v3 14/14] vdpa: Add x-svq to NetdevVhostVDPAOptions

[Eugenio Perez Martin]

On Thu, Mar 3, 2022 at 1:00 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Eugenio Perez Martin <eperezma@redhat.com> writes:
>
> > On Thu, Mar 3, 2022 at 7:09 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Eugenio Pérez <eperezma@redhat.com> writes:
> >>
> >> > Finally offering the possibility to enable SVQ from the command line.
> >> >
> >> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> > ---
> >> >  qapi/net.json    |  5 ++++-
> >> >  net/vhost-vdpa.c | 48 ++++++++++++++++++++++++++++++++++++++++--------
> >> >  2 files changed, 44 insertions(+), 9 deletions(-)
> >> >
> >> > diff --git a/qapi/net.json b/qapi/net.json
> >> > index 7fab2e7cd8..d243701527 100644
> >> > --- a/qapi/net.json
> >> > +++ b/qapi/net.json
> >> > @@ -445,12 +445,15 @@
> >> >  # @queues: number of queues to be created for multiqueue vhost-vdpa
> >> >  #          (default: 1)
> >> >  #
> >> > +# @x-svq: Start device with (experimental) shadow virtqueue. (Since 7.0)
> >> > +#
> >> >  # Since: 5.1
> >> >  ##
> >> >  { 'struct': 'NetdevVhostVDPAOptions',
> >> >    'data': {
> >> >      '*vhostdev':     'str',
> >> > -    '*queues':       'int' } }
> >> > +    '*queues':       'int',
> >> > +    '*x-svqs':        'bool' } }
> >>
> >> Experimental members *must* be tagged with feature @unstable.  Their
> >> name *may* start with 'x-' to help human users, at the cost of renames
> >> when the member becomes stable.
> >>
> >
> > Hi Markus,
> >
> > Thank you very much for the warning. I'll add the unstable feature tag.
> >
> > If I understood correctly this needs to be done as x-perf at
> > BackupCommon struct. Could you confirm to me that it marks only the
> > x-perf member as unstable? Without reading the actual comment it might
> > seem as if it marks all the whole BackupCommon struct as unstable.
> >
> > # ...
> > # @filter-node-name: the node name that should be assigned to the
> > #                    filter driver that the backup job inserts into the 
> > graph
> > #                    above node specified by @drive. If this option is
> > not given,
> > #                    a node name is autogenerated. (Since: 4.2)
> > #
> > # @x-perf: Performance options. (Since 6.0)
> > #
> > # Features:
> > # @unstable: Member @x-perf is experimental.
> > #
> > # Note: @on-source-error and @on-target-error only affect background
> > #       I/O.  If an error occurs during a guest write request, the device's
> > #       rerror/werror actions will be used.
> > #
> > # Since: 4.2
> > ##
> > { 'struct': 'BackupCommon',
> >   'data': { ...
> >             '*filter-node-name': 'str',
> >             '*x-perf': { 'type': 'BackupPerf',
> >                          'features': [ 'unstable' ] } } }
>
> This tacks features to member @x-perf, i.e. they apply just to member
> @x-perf.
>
> Features can also be tacked to the struct type, like this:
>
>   { 'struct': 'BackupCommon',
>     'data': { ...
>               '*filter-node-name': 'str',
>               '*x-perf': 'BackupPerf' },
>     'features': [ 'unstable' ] }
>
> Now they apply to type BackupCommon as a whole.
>
> BlockdevOptionsFile in block-core.json actually makes use of both ways:
>
> { 'struct': 'BlockdevOptionsFile',
>   'data': { 'filename': 'str',
>             '*pr-manager': 'str',
>             '*locking': 'OnOffAuto',
>             '*aio': 'BlockdevAioOptions',
>             '*aio-max-batch': 'int',
>             '*drop-cache': {'type': 'bool',
>                             'if': 'CONFIG_LINUX'},
>             '*x-check-cache-dropped': { 'type': 'bool',
>                                         'features': [ 'unstable' ] } },
>   'features': [ { 'name': 'dynamic-auto-read-only',
>                   'if': 'CONFIG_POSIX' } ] }
>
> Feature @dynamic-auto-read-only applies to the type, and feature
> @unstable applies to member @x-check-cache-dropped.
>
> Questions?
>

Yes, that's right. I expressed my point poorly actually, I'll go the reverse.

qapi-gen.py forces me to write a comment in the doc:
qapi/block-core.json:2971: feature 'unstable' lacks documentation

When I add the documentation line, it's enough to add @unstable. But
there is no way to tell if this tag is because the whole struct is
unstable or if it's because individual members are unstable unless the
reader either checks the tag or the struct code.

I was mostly worried about doc generators, I would not like to make
NetdevVhostVDPAOptions unstable at this point. But I see that there is
no problem with that.

Thanks!