| Message ID | 20220302203012.3476835-15-eperezma@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vDPA shadow virtqueue | expand |
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-svq': '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. Documentation is in docs/devel/qapi-code-gen.rst: Features -------- Syntax:: FEATURES = [ FEATURE, ... ] FEATURE = STRING | { 'name': STRING, '*if': COND } Sometimes, the behaviour of QEMU changes compatibly, but without a change in the QMP syntax (usually by allowing values or operations that previously resulted in an error). QMP clients may still need to know whether the extension is available. For this purpose, a list of features can be specified for a command or struct type. Each list member can either be ``{ 'name': STRING, '*if': COND }``, or STRING, which is shorthand for ``{ 'name': STRING }``. The optional 'if' member specifies a conditional. See `Configuring the schema`_ below for more on this. Example:: { 'struct': 'TestType', 'data': { 'number': 'int' }, 'features': [ 'allow-negative-numbers' ] } The feature strings are exposed to clients in introspection, as explained in section `Client JSON Protocol introspection`_. Intended use is to have each feature string signal that this build of QEMU shows a certain behaviour. Special features ~~~~~~~~~~~~~~~~ [...] Feature "unstable" marks a command, event, enum value, or struct member as unstable. It is not supported elsewhere so far. Interfaces so marked may be withdrawn or changed incompatibly in future releases. Naming rules and reserved names ------------------------------- [...] Names beginning with ``x-`` used to signify "experimental". This convention has been replaced by special feature "unstable". Rationale is in the commit message: commit a3c45b3e62962f99338716b1347cfb0d427cea44 Author: Markus Armbruster <armbru@redhat.com> Date: Thu Oct 28 12:25:12 2021 +0200 qapi: New special feature flag "unstable" By convention, names starting with "x-" are experimental. The parts of external interfaces so named may be withdrawn or changed incompatibly in future releases. The naming convention makes unstable interfaces easy to recognize. Promoting something from experimental to stable involves a name change. Client code needs to be updated. Occasionally bothersome. Worse, the convention is not universally observed: * QOM type "input-barrier" has properties "x-origin", "y-origin". Looks accidental, but it's ABI since 4.2. * QOM types "memory-backend-file", "memory-backend-memfd", "memory-backend-ram", and "memory-backend-epc" have a property "x-use-canonical-path-for-ramblock-id" that is documented to be stable despite its name. We could document these exceptions, but documentation helps only humans. We want to recognize "unstable" in code, like "deprecated". So support recognizing it the same way: introduce new special feature flag "unstable". It will be treated specially by the QAPI generator, like the existing feature flag "deprecated", and unlike regular feature flags. This commit updates documentation and prepares tests. The next commit updates the QAPI schema. The remaining patches update the QAPI generator and wire up -compat policy checking. Management applications can then use query-qmp-schema and -compat to manage or guard against use of unstable interfaces the same way as for deprecated interfaces. docs/devel/qapi-code-gen.txt no longer mandates the naming convention. Using it anyway might help writers of programs that aren't full-fledged management applications. Not using it can save us bothersome renames. We'll see how that shakes out. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-Id: <20211028102520.747396-2-armbru@redhat.com> > > ## > # @NetClientDriver: [...]
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' ] } } }
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?
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!
Eugenio Perez Martin <eperezma@redhat.com> writes: > 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! Yes, the doc generator insists on features being documented, and it doesn't provide for documenting them next to members, only top-level. The common solution is to phrase the comment like we do in BlockdevOptionsFile: # @unstable: Member x-check-cache-dropped is meant for debugging. If there were multiple members so flagged, we'd enumerate them all. The generator doesn't check you do this right. The existing check guards against *forgetting* to do it, not against doing it wrong. More questions?
On Fri, Mar 4, 2022 at 7:30 AM Markus Armbruster <armbru@redhat.com> wrote: > > Eugenio Perez Martin <eperezma@redhat.com> writes: > > > 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! > > Yes, the doc generator insists on features being documented, and it > doesn't provide for documenting them next to members, only top-level. > The common solution is to phrase the comment like we do in > BlockdevOptionsFile: > > # @unstable: Member x-check-cache-dropped is meant for debugging. > > If there were multiple members so flagged, we'd enumerate them all. > > The generator doesn't check you do this right. The existing check > guards against *forgetting* to do it, not against doing it wrong. > > More questions? > Got it, it matches my impression. Thank you very much!
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-svq': 'bool' } } ## # @NetClientDriver: diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 1e9fe47c03..def738998b 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -127,7 +127,11 @@ err_init: static void vhost_vdpa_cleanup(NetClientState *nc) { VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc); + struct vhost_dev *dev = s->vhost_vdpa.dev; + if (dev && dev->vq_index + dev->nvqs == dev->vq_index_end) { + g_clear_pointer(&s->vhost_vdpa.iova_tree, vhost_iova_tree_delete); + } if (s->vhost_net) { vhost_net_cleanup(s->vhost_net); g_free(s->vhost_net); @@ -187,13 +191,23 @@ static NetClientInfo net_vhost_vdpa_info = { .check_peer_type = vhost_vdpa_check_peer_type, }; +static int vhost_vdpa_get_iova_range(int fd, + struct vhost_vdpa_iova_range *iova_range) +{ + int ret = ioctl(fd, VHOST_VDPA_GET_IOVA_RANGE, iova_range); + + return ret < 0 ? -errno : 0; +} + static NetClientState *net_vhost_vdpa_init(NetClientState *peer, - const char *device, - const char *name, - int vdpa_device_fd, - int queue_pair_index, - int nvqs, - bool is_datapath) + const char *device, + const char *name, + int vdpa_device_fd, + int queue_pair_index, + int nvqs, + bool is_datapath, + bool svq, + VhostIOVATree *iova_tree) { NetClientState *nc = NULL; VhostVDPAState *s; @@ -211,6 +225,8 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, s->vhost_vdpa.device_fd = vdpa_device_fd; s->vhost_vdpa.index = queue_pair_index; + s->vhost_vdpa.shadow_vqs_enabled = svq; + s->vhost_vdpa.iova_tree = iova_tree; ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa, queue_pair_index, nvqs); if (ret) { qemu_del_net_client(nc); @@ -266,6 +282,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, g_autofree NetClientState **ncs = NULL; NetClientState *nc; int queue_pairs, i, has_cvq = 0; + g_autoptr(VhostIOVATree) iova_tree = NULL; assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA); opts = &netdev->u.vhost_vdpa; @@ -285,29 +302,44 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, qemu_close(vdpa_device_fd); return queue_pairs; } + if (opts->x_svq) { + struct vhost_vdpa_iova_range iova_range; + + if (has_cvq) { + error_setg(errp, "vdpa svq does not work with cvq"); + goto err_svq; + } + vhost_vdpa_get_iova_range(vdpa_device_fd, &iova_range); + iova_tree = vhost_iova_tree_new(iova_range.first, iova_range.last); + } ncs = g_malloc0(sizeof(*ncs) * queue_pairs); for (i = 0; i < queue_pairs; i++) { ncs[i] = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, - vdpa_device_fd, i, 2, true); + vdpa_device_fd, i, 2, true, opts->x_svq, + iova_tree); if (!ncs[i]) goto err; } if (has_cvq) { nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, - vdpa_device_fd, i, 1, false); + vdpa_device_fd, i, 1, false, opts->x_svq, + iova_tree); if (!nc) goto err; } + iova_tree = NULL; return 0; err: if (i) { qemu_del_net_client(ncs[0]); } + +err_svq: qemu_close(vdpa_device_fd); return -1;
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(-)