Re: [PATCH v1 4/4] scripts: apibuild: add 'version' to variables
Hi, On Tue, Apr 05, 2022 at 04:33:27PM +, Andrea Bolognani wrote: > On Tue, Apr 05, 2022 at 01:43:23PM +0200, Victor Toso wrote: > > Differently from the previous patches, we don't parse nor export > > comments associated with variables. This isn't a big deal because we > > only export a single variable: virConnectAuthPtrDefault > > > > Nonetheless, add version field to the exported XML by checking the > > allowlist file. This way, if we add another variable in the future, we > > can simply add it to that file. Calling the function should also warn > > in case we are exporting a new Variable without adding to the file, > > e.g: > > Missing 'Since' tag for: virConnectAuthPtrDefault > > I think the fact that we're currently not parsing comments for > variables is just a mistake. > > virConnectAuthPtrDefault seems to be documented in a way that would > be appropriate for the API docs The documentation is not in the headers: https://gitlab.com/libvirt/libvirt/-/blob/master/include/libvirt/libvirt-host.h#L565 Only in the source: https://gitlab.com/libvirt/libvirt/-/blob/master/src/libvirt.c#L200 So, moving the documentation around could be an extra patch, indeed. > and the fact that it doesn't show up there means that users of > the library have no way to figure out what it's there for > without digging into the source code, or even that it exists at > all. Yes > I'd say just start treating comments for variables the same as > those for all other symbols. TBH, because it was only a single exported variable, I didn't put too much effort in parsing the docs, but you made a good point. I'll try again, to parse comments from exported variables and included it all in the XML API. Cheers, Victor signature.asc Description: PGP signature
Re: [PATCH v1 0/4] Add 'version' to other exported types
Hi,
On Tue, Apr 05, 2022 at 05:01:54PM +, Andrea Bolognani wrote:
> On Tue, Apr 05, 2022 at 04:52:10PM +0200, Michal Prívozník wrote:
> > On 4/5/22 13:43, Victor Toso wrote:
> > > As headers are already a great source of documentation for developers,
> > > I'm suggesting to add a specific comment to each of this exported types:
> > >
> > > /* ... Since */
> > >
> > > For the use case I mentioned above, I'm adding small parsing function in
> > > apibuild.py to fetch the above metadata and included it on the generated
> > > XML API.
> > >
> > > To avoid adding too much noise in the githistory, I'm proposing the
> > > addition of symbols.versions.allowlist file, that apibuild.py can use to
> > > fetch the first git tag that a given symbol appeared in.
> >
> > I like this. It's not only for Golang bindings, but other bindings might
> > benefit from this as well. And also developers reading the docs (they
> > see immediately what version was the symbol they are looking at
> > introduced in).
> >
> > Having said that, maybe we should just add 'Since ...' to every symbol
> > in include/**\.h instead of having it on a side then? If not, then I
> > think scripts/ or docs/ is better location for the allowlist file.
Sorry, missed this part when first read your reply Michal.
> Personally, I think that "since" information should be parsed
> from the docstring.
>
> Even the thing that we currently do for functions, where we look into
> the .syms file and derive the version number from there, should only
> be used as a way to validate the "since" information contained in the
> corresponding docstring.
>
> The "allowfile" you've generated could be a first step towards
> reaching the goal, but I don't think it should be something that ends
> up sticking around.
>
> Adding "Since" tags everywhere by hand would be an insane amount of
> work of course, but we should be able to hack together a script that
> does something along the lines of
>
> comment = get_comment_for(sym)
> version = grab_version_from_allowfile(sym)
> replace(comment, f"{comment} (Since {version})")
>
> and get like 90% of the way there. We could then make manual
> adjustments to the stuff that looks off.
Yes, I don't think it is too hard. IMHO, just playing with
strings at this point. My main concern was the noise it could
generate over git history. I'm the kind of person that does git
blame a lot to get the context of a something I'm reading, so
affecting that is naturally a concern of mine. If it is fine for
you all, I'm 100% okay with that. It'll take a bit longer but not
that much. (famous last words)
> Ideally, the "since" information would also be included in the HTML
> documentation. GLib does that[1] and it's very useful.
>
> To avoid cluttering things too much, we could decide to only
> show "since" information for symbols that have been introduced
> after 1.0.0.
That's 1235 out of 2144. Sounds good to me too.
> If we wanted to get *really* fancy, we could have some CSS or
> JavaScript thingy that allows you to select the libvirt version
> you're targeting and hides or marks in some way the symbols
> that are newer than that to let you know that you can't use
> them.
Where would you like to show that?
> I'm not saying that we should seriously work on that last part,
> it's just the kind of thing that you can unlock once you have
> full version information for every symbol in the API :)
There is always room for improvement, that's for sure. At the
moment, I want to unblock the code generation thing in
libvirt-go-module, somewhat my primary goal. This series is a
part of that quest 0:-)
> [1] https://docs.gtk.org/glib/func.aligned_alloc0.html
> --
> Andrea Bolognani / Red Hat / Virtualization
Thanks again for the reviews,
Victor
signature.asc
Description: PGP signature
Re: [PATCH v1 0/4] Add 'version' to other exported types
On Tue, Apr 05, 2022 at 04:52:10PM +0200, Michal Prívozník wrote:
> On 4/5/22 13:43, Victor Toso wrote:
> > As headers are already a great source of documentation for developers,
> > I'm suggesting to add a specific comment to each of this exported types:
> >
> > /* ... Since */
> >
> > For the use case I mentioned above, I'm adding small parsing function in
> > apibuild.py to fetch the above metadata and included it on the generated
> > XML API.
> >
> > To avoid adding too much noise in the githistory, I'm proposing the
> > addition of symbols.versions.allowlist file, that apibuild.py can use to
> > fetch the first git tag that a given symbol appeared in.
>
> I like this. It's not only for Golang bindings, but other bindings might
> benefit from this as well. And also developers reading the docs (they
> see immediately what version was the symbol they are looking at
> introduced in).
>
> Having said that, maybe we should just add 'Since ...' to every symbol
> in include/**\.h instead of having it on a side then? If not, then I
> think scripts/ or docs/ is better location for the allowlist file.
Personally, I think that "since" information should be parsed from
the docstring.
Even the thing that we currently do for functions, where we look into
the .syms file and derive the version number from there, should only
be used as a way to validate the "since" information contained in the
corresponding docstring.
The "allowfile" you've generated could be a first step towards
reaching the goal, but I don't think it should be something that ends
up sticking around.
Adding "Since" tags everywhere by hand would be an insane amount of
work of course, but we should be able to hack together a script that
does something along the lines of
comment = get_comment_for(sym)
version = grab_version_from_allowfile(sym)
replace(comment, f"{comment} (Since {version})")
and get like 90% of the way there. We could then make manual
adjustments to the stuff that looks off.
Ideally, the "since" information would also be included in the HTML
documentation. GLib does that[1] and it's very useful.
To avoid cluttering things too much, we could decide to only show
"since" information for symbols that have been introduced after
1.0.0.
If we wanted to get *really* fancy, we could have some CSS or
JavaScript thingy that allows you to select the libvirt version
you're targeting and hides or marks in some way the symbols that are
newer than that to let you know that you can't use them.
I'm not saying that we should seriously work on that last part, it's
just the kind of thing that you can unlock once you have full version
information for every symbol in the API :)
[1] https://docs.gtk.org/glib/func.aligned_alloc0.html
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v1 4/4] scripts: apibuild: add 'version' to variables
On Tue, Apr 05, 2022 at 01:43:23PM +0200, Victor Toso wrote: > Differently from the previous patches, we don't parse nor export > comments associated with variables. This isn't a big deal because we > only export a single variable: virConnectAuthPtrDefault > > Nonetheless, add version field to the exported XML by checking the > allowlist file. This way, if we add another variable in the future, we > can simply add it to that file. Calling the function should also warn > in case we are exporting a new Variable without adding to the file, > e.g: > Missing 'Since' tag for: virConnectAuthPtrDefault I think the fact that we're currently not parsing comments for variables is just a mistake. virConnectAuthPtrDefault seems to be documented in a way that would be appropriate for the API docs, and the fact that it doesn't show up there means that users of the library have no way to figure out what it's there for without digging into the source code, or even that it exists at all. I'd say just start treating comments for variables the same as those for all other symbols. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v1 2/4] scripts: apibuild: parse 'Since' for typedefs
On Tue, Apr 05, 2022 at 05:36:51PM +0200, Victor Toso wrote: > On Tue, Apr 05, 2022 at 04:52:09PM +0200, Michal Prívozník wrote: > > On 4/5/22 13:43, Victor Toso wrote: > > > This patch adds 'version' parameter to the generated XML API for > > > typedefs > > > > > > It'll require, for new additions, to add a comment with the version > > > that the typedef value was added. An example bellow of code diff and > > > the change in the generated XML. > > > > > > Note that the Since tag is removed from the comment as there is a > > > proper field for it in the XML. > > > > > > ```diff > > > --- a/include/libvirt/libvirt-host.h > > > +++ b/include/libvirt/libvirt-host.h > > > @@ -41,6 +41,8 @@ typedef struct _virConnect virConnect; > > > * > > > * a virConnectPtr is pointer to a virConnect private structure, this is > > > the > > > * type used to reference a connection to the Hypervisor in the API. > > > + * > > > + * Since 0.0.1 > > > */ > > > typedef virConnect *virConnectPtr; > > > ``` > > > > I'm not exactly sure why, but this diff in commit message causes my git > > to think this is a broken patch. Maybe it's trying to parse it as an > > actual patch? Funnily enough, there's no problem with 1/4. > > When I send a v2, I'll make both examples above quoted, that is, > prefixed with: " > ". Not sure why I actually used ```diff and > ```xml, gitlab does nothing with it x) I think you can leave the examples out entirely. The "Since x.y.z" comments are already a known quantity, and you're just extending their applicability to more public symbols. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] virsh: Remove any reference of KVM device assignment
On Tue, Apr 05, 2022 at 10:42:03AM +0200, Michal Privoznik wrote: > The KVM device assignment was removed in v5.7.0-rc1~103 but virsh > and its manpage still mention it. Don't do that. > > Signed-off-by: Michal Privoznik > --- > docs/manpages/virsh.rst | 7 +++ > tools/virsh-nodedev.c | 2 +- > 2 files changed, 4 insertions(+), 5 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v1 2/4] scripts: apibuild: parse 'Since' for typedefs
Hi, On Tue, Apr 05, 2022 at 04:52:09PM +0200, Michal Prívozník wrote: > On 4/5/22 13:43, Victor Toso wrote: > > This patch adds 'version' parameter to the generated XML API for > > typedefs > > > > It'll require, for new additions, to add a comment with the version > > that the typedef value was added. An example bellow of code diff and > > the change in the generated XML. > > > > Note that the Since tag is removed from the comment as there is a > > proper field for it in the XML. > > > > ```diff > > --- a/include/libvirt/libvirt-host.h > > +++ b/include/libvirt/libvirt-host.h > > @@ -41,6 +41,8 @@ typedef struct _virConnect virConnect; > > * > > * a virConnectPtr is pointer to a virConnect private structure, this is > > the > > * type used to reference a connection to the Hypervisor in the API. > > + * > > + * Since 0.0.1 > > */ > > typedef virConnect *virConnectPtr; > > ``` > > > > ```xml > > > file='libvirt-host' > > type='virConnect *' > > version='0.0.1'> > > > > > > ``` > > > > I'm not exactly sure why, but this diff in commit message causes my git > to think this is a broken patch. Maybe it's trying to parse it as an > actual patch? Funnily enough, there's no problem with 1/4. > > Michal When I send a v2, I'll make both examples above quoted, that is, prefixed with: " > ". Not sure why I actually used ```diff and ```xml, gitlab does nothing with it x) Cheers, Victor signature.asc Description: PGP signature
Re: [PATCH v1 0/4] Add 'version' to other exported types
Hi,
On Tue, Apr 05, 2022 at 04:52:10PM +0200, Michal Prívozník wrote:
> On 4/5/22 13:43, Victor Toso wrote:
> > Hi,
> >
> > This series is an extension of what Daniel did in e0e0bf6628 "scripts:
> > include function versions in API definition".
> >
> > The main motivation behind this is to help code generators to consider
> > if a given symbol is present in a given version of libvirt that they are
> > either running/building against, more specifically:
> >
> > https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/7
> >
> > As headers are already a great source of documentation for developers,
> > I'm suggesting to add a specific comment to each of this exported types:
> >
> > /* ... Since */
> >
> > For the use case I mentioned above, I'm adding small parsing function in
> > apibuild.py to fetch the above metadata and included it on the generated
> > XML API.
> >
> > To avoid adding too much noise in the githistory, I'm proposing the
> > addition of symbols.versions.allowlist file, that apibuild.py can use to
> > fetch the first git tag that a given symbol appeared in. I did a small
> > script to generate it, but it sums up to calling the bellow command for
> > any tag that starts with 'v' and has not '-rc'.
> >
> > git grep $symbol $tag ./include
>
> This gives you the latest tag that a symbol was touched in. You
> need to look at the very first commit that introduced the
> symbol.
Looking to each commit takes a few hours, that's why I took the
approach at looking per tag instead.
> For instance, in your allowlist file you have:
>
> virConnectDomainEventDeregister v0.10.0
>
> but src/libvirt_public.syms lists this symbol in 0.5.0 release:
>
> LIBVIRT_0.5.0 {
> global:
> ...
> virConnectDomainEventDeregister;
Good catch! I think the problem here is that when I run:
git tag --list 'v*' | grep -v 'rc'
the order that I got was
...
v0.1.8
v0.1.9
v0.10.0
v0.10.1
v0.10.2
v0.10.2.1
v0.10.2.2
v0.10.2.3
v0.10.2.4
v0.10.2.5
v0.10.2.6
v0.10.2.7
v0.10.2.8
v0.2.0
v0.2.1
v0.2.2
...
And I tested in that order, so v0.10.0 was tested prior to
v0.5.0.
I can see that git grep works with v0.5.0. but not with v0.4.6,
as expected.
I'll fix it and send a v2 later. Many thanks!
> > I'm trying the simples approach I could find but I'm happy to improve
> > this further based on your suggestions.
> >
> > Ah, the diff in the generated XMLs, per patch (sadly, pasteben drops it
> > after a single day). The initial reference is current master 67c77744d7
> > 'tests: Fixing compiler warning in cputest'
> >
> > 0001 (enums) ... https://paste.centos.org/view/e9137ef0
> > 0002 (typedefs) https://paste.centos.org/view/76f0b397
> > 0003 (macros) .. https://paste.centos.org/view/b68fb03a
> > 0004 (variables) https://paste.centos.org/view/4a20d9bb
> >
> > Cheers,
> > Victor
> >
> > Victor Toso (4):
> > scripts: apibuild: parse 'Since' version for enums
> > scripts: apibuild: parse 'Since' for typedefs
> > scripts: apibuild: parse 'Since' for macros
> > scripts: apibuild: add 'version' to variables
> >
> > scripts/apibuild.py| 68 +-
> > symbols.versions.allowlist | 2144
> > 2 files changed, 2199 insertions(+), 13 deletions(-)
> > create mode 100644 symbols.versions.allowlist
> >
>
> I like this. It's not only for Golang bindings, but other bindings might
> benefit from this as well. And also developers reading the docs (they
> see immediately what version was the symbol they are looking at
> introduced in).
>
> Having said that, maybe we should just add 'Since ...' to every symbol
> in include/**\.h instead of having it on a side then? If not, then I
> think scripts/ or docs/ is better location for the allowlist file.
>
> Moreover, the allowlist file now contains
>
> Michal
>
signature.asc
Description: PGP signature
Re: [PATCH v1 2/4] scripts: apibuild: parse 'Since' for typedefs
On 4/5/22 13:43, Victor Toso wrote: > This patch adds 'version' parameter to the generated XML API for > typedefs > > It'll require, for new additions, to add a comment with the version > that the typedef value was added. An example bellow of code diff and > the change in the generated XML. > > Note that the Since tag is removed from the comment as there is a > proper field for it in the XML. > > ```diff > --- a/include/libvirt/libvirt-host.h > +++ b/include/libvirt/libvirt-host.h > @@ -41,6 +41,8 @@ typedef struct _virConnect virConnect; > * > * a virConnectPtr is pointer to a virConnect private structure, this is the > * type used to reference a connection to the Hypervisor in the API. > + * > + * Since 0.0.1 > */ > typedef virConnect *virConnectPtr; > ``` > > ```xml > file='libvirt-host' > type='virConnect *' > version='0.0.1'> > > > ``` > I'm not exactly sure why, but this diff in commit message causes my git to think this is a broken patch. Maybe it's trying to parse it as an actual patch? Funnily enough, there's no problem with 1/4. Michal
Re: [PATCH v1 0/4] Add 'version' to other exported types
On 4/5/22 13:43, Victor Toso wrote:
> Hi,
>
> This series is an extension of what Daniel did in e0e0bf6628 "scripts:
> include function versions in API definition".
>
> The main motivation behind this is to help code generators to consider
> if a given symbol is present in a given version of libvirt that they are
> either running/building against, more specifically:
>
> https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/7
>
> As headers are already a great source of documentation for developers,
> I'm suggesting to add a specific comment to each of this exported types:
>
> /* ... Since */
>
> For the use case I mentioned above, I'm adding small parsing function in
> apibuild.py to fetch the above metadata and included it on the generated
> XML API.
>
> To avoid adding too much noise in the githistory, I'm proposing the
> addition of symbols.versions.allowlist file, that apibuild.py can use to
> fetch the first git tag that a given symbol appeared in. I did a small
> script to generate it, but it sums up to calling the bellow command for
> any tag that starts with 'v' and has not '-rc'.
>
> git grep $symbol $tag ./include
This gives you the latest tag that a symbol was touched in. You need to
look at the very first commit that introduced the symbol. For instance,
in your allowlist file you have:
virConnectDomainEventDeregister v0.10.0
but src/libvirt_public.syms lists this symbol in 0.5.0 release:
LIBVIRT_0.5.0 {
global:
...
virConnectDomainEventDeregister;
>
> I'm trying the simples approach I could find but I'm happy to improve
> this further based on your suggestions.
>
> Ah, the diff in the generated XMLs, per patch (sadly, pasteben drops it
> after a single day). The initial reference is current master 67c77744d7
> 'tests: Fixing compiler warning in cputest'
>
> 0001 (enums) ... https://paste.centos.org/view/e9137ef0
> 0002 (typedefs) https://paste.centos.org/view/76f0b397
> 0003 (macros) .. https://paste.centos.org/view/b68fb03a
> 0004 (variables) https://paste.centos.org/view/4a20d9bb
>
> Cheers,
> Victor
>
> Victor Toso (4):
> scripts: apibuild: parse 'Since' version for enums
> scripts: apibuild: parse 'Since' for typedefs
> scripts: apibuild: parse 'Since' for macros
> scripts: apibuild: add 'version' to variables
>
> scripts/apibuild.py| 68 +-
> symbols.versions.allowlist | 2144
> 2 files changed, 2199 insertions(+), 13 deletions(-)
> create mode 100644 symbols.versions.allowlist
>
I like this. It's not only for Golang bindings, but other bindings might
benefit from this as well. And also developers reading the docs (they
see immediately what version was the symbol they are looking at
introduced in).
Having said that, maybe we should just add 'Since ...' to every symbol
in include/**\.h instead of having it on a side then? If not, then I
think scripts/ or docs/ is better location for the allowlist file.
Moreover, the allowlist file now contains
Michal
Re: [PATCH v2 1/4] domain_conf: Added configs for RSS and Hash report.
On 1/9/22 22:07, Andrew Melnychenko wrote:
> Added "rss" and "rss_hash_report" configuration that should be used with
> qemu virtio RSS.
> Both options are triswitches. Used as "driver" options and affects only NIC
> with model type "virtio".
> In other patches - options should turn on virtio-net RSS and hash properties.
>
> Signed-off-by: Andrew Melnychenko
> ---
> docs/formatdomain.rst | 18 ++
> docs/schemas/domaincommon.rng | 10 ++
> src/conf/domain_conf.c| 31 ++-
> src/conf/domain_conf.h| 2 ++
> 4 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index d4f30bb8af..ce3e8a5dbf 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -5305,6 +5305,24 @@ following attributes are available for the
> ``"virtio"`` NIC driver:
> only for ``vhostuser`` type. :since:`Since 3.7.0 (QEMU and KVM only)`
> **In general you should leave this option alone, unless you are very
> certain
> you know what you are doing.**
> +``rss``
> + The ``rss`` option enables in-qemu/ebpf RSS for virtio NIC. RSS works with
> + virtio and tap backends only. Virtio NIC will be launched with "rss"
> + property. For now "in-qemu" RSS is supported by libvirt.
> + QEMU may load eBPF RSS if it has CAP_SYS_ADMIN permissions, which is
> + not supported by default in libvirt.
> + **In general you should leave this option alone, unless you are very
> certain
> + you know what you are doing. Proper RSS configuration depends from vcpu,
> + tap, and vhost settings.**
> +``rss_hash_report``
> + The ``rss_hash_report`` option enables in-qemu RSS hash report for virtio
> + NIC. Virtio NIC will be launched with a "hash" property. Network packets
> provided
> + to VM will contain a hash of the packet in the virt header. Usually
> enabled
> + alongside with ``rss``. Without ``rss`` option, the hash report doesn't
> affect
> + steering itself but provides vnet header with a calculated hash.
> + **In general you should leave this option alone, unless you are very
> certain
> + you know what you are doing. Proper RSS configuration depends from vcpu,
> + tap, and vhost settings.**
> virtio options
> For virtio interfaces, `Virtio-specific options <#elementsVirtio>`__ can
> also
> be set. ( :since:`Since 3.5.0` )
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 7fa5c2b8b5..9b5b94fc6c 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3595,6 +3595,16 @@
>
>
>
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 716c6d2240..762987e8a9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10271,6 +10271,8 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
> g_autofree char *vhost_path = NULL;
> g_autofree char *tap = NULL;
> g_autofree char *vhost = NULL;
> +g_autofree char *virtio_rss = NULL;
> +g_autofree char *virtio_rss_hash_report = NULL;
> const char *prefix = xmlopt ? xmlopt->config.netPrefix : NULL;
>
> if (!(def = virDomainNetDefNew(xmlopt)))
> @@ -10412,6 +10414,8 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
> queues = virXMLPropString(driver_node, "queues");
> rx_queue_size = virXMLPropString(driver_node, "rx_queue_size");
> tx_queue_size = virXMLPropString(driver_node, "tx_queue_size");
> +virtio_rss = virXMLPropString(driver_node, "rss");
> +virtio_rss_hash_report = virXMLPropString(driver_node,
> "rss_hash_report");
>
> if ((filterref_node = virXPathNode("./filterref", ctxt))) {
> filter = virXMLPropString(filterref_node, "filter");
> @@ -10822,7 +10826,24 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
> }
> def->driver.virtio.tx_queue_size = q;
> }
> -
> +if (virtio_rss) {
> +if ((val = virTristateSwitchTypeFromString(virtio_rss)) <= 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +_("'rss' attribute must be 'on'/'off'/'default':
> %s"),
> +virtio_rss);
> +goto error;
> +}
> +def->driver.virtio.rss = val;
> +}
> +if (virtio_rss_hash_report) {
> +if ((val =
> virTristateSwitchTypeFromString(virtio_rss_hash_report)) <= 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +_("'rss_hash_report' attribute must be
> 'on'/'off'/'default': %s"),
> +virtio_rss_hash_report);
> +goto error;
> +}
>
Re: [PATCH v2 4/4] test: Added xml2argv and xml2xml tests.
On 1/9/22 22:07, Andrew Melnychenko wrote:
> Added rss, hash and rss+hash xml2argv tests.
> virtio-options tests was used for xml2xml test.
>
> Signed-off-by: Andrew Melnychenko
> ---
> .../net-virtio-rss.x86_64-latest.args | 43 +++
> tests/qemuxml2argvdata/net-virtio-rss.xml | 39 +
> .../virtio-options.x86_64-latest.args | 2 +-
> tests/qemuxml2argvdata/virtio-options.xml | 2 +-
> tests/qemuxml2argvtest.c | 2 +
> 5 files changed, 86 insertions(+), 2 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/net-virtio-rss.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/net-virtio-rss.xml
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 6c67b36d5c..5227f76dce 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -3500,6 +3500,8 @@ mymain(void)
>
> DO_TEST_CAPS_LATEST("devices-acpi-index");
>
> +DO_TEST_CAPS_LATEST("net-virtio-rss");
> +
> if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
> virFileDeleteTree(fakerootdir);
>
I'd put this among with the rest of net-* test cases (couple of lines
above).
Michal
Re: [PATCH v2 0/4] VirtioNet RSS support.
On 1/9/22 22:07, Andrew Melnychenko wrote: > This series of patches add RSS property support for virtio-net-pci. > > Virtio RSS effectively works with TAP devices, it requires additional > vectors for VirtioNet, queues for TAP device, and vCPU cores. > Example of device configuration: > ``` > > > > > > > > ``` > > Capability "rss" enables RSS, "rss_hash_report" - enables hashes in vheader. > For now, "rss" property will trigger "in-qemu" RSS in most cases. > Current Qemu(6.2) supports eBPF RSS that may require additional capabilities. > In future, the helper will be provided. And this code is the base for VirtIO > RSS. > > Changes since v1: > * refactored patches > * changed docs and tests > > Changes since RFC: > * rebased and refactored > * added tests > * postponed the helper > > Andrew Melnychenko (4): > domain_conf: Added configs for RSS and Hash report. > qemu_capabilities: Added capabilites for qemu's "rss" and "hash". > qemu_command: Added "rss" and "hash" properties. > test: Added xml2argv and xml2xml tests. > > docs/formatdomain.rst | 18 > docs/schemas/domaincommon.rng | 10 + > src/conf/domain_conf.c| 31 - > src/conf/domain_conf.h| 2 + > src/qemu/qemu_capabilities.c | 2 + > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_command.c | 2 + > src/qemu/qemu_validate.c | 14 ++ > .../caps_5.1.0.x86_64.xml | 1 + > .../caps_5.2.0.aarch64.xml| 1 + > .../qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 + > .../caps_5.2.0.riscv64.xml| 1 + > .../qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 + > .../caps_5.2.0.x86_64.xml | 1 + > .../caps_6.0.0.aarch64.xml| 1 + > .../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 + > .../caps_6.0.0.x86_64.xml | 1 + > .../caps_6.1.0.x86_64.xml | 1 + > .../caps_6.2.0.aarch64.xml| 1 + > .../qemucapabilitiesdata/caps_6.2.0.ppc64.xml | 1 + > .../caps_6.2.0.x86_64.xml | 1 + > .../net-virtio-rss.x86_64-latest.args | 43 +++ > tests/qemuxml2argvdata/net-virtio-rss.xml | 39 + > .../virtio-options.x86_64-latest.args | 2 +- > tests/qemuxml2argvdata/virtio-options.xml | 2 +- > tests/qemuxml2argvtest.c | 2 + > 26 files changed, 178 insertions(+), 3 deletions(-) > create mode 100644 tests/qemuxml2argvdata/net-virtio-rss.x86_64-latest.args > create mode 100644 tests/qemuxml2argvdata/net-virtio-rss.xml > I've reworked patches a bit, adopted newer XML parsing, merged 3 and 4 together and pushed. Reviewed-by: Michal Privoznik Congratulations on your first libvirt contribution! And sorry for delayed review. Michal
[PATCH RESEND] qemu: disarm fake reboot flag on reset
From: Maxim Nestratov This is a quite an old (created at 2016) patch fixing an issue for at that time contemporary Fedora 23. virsh reboot returns success (yet after hanging for a while), VM is rebooted sucessfully too but then shutdown from inside guest causes reboot and not shutdown. VM has agent installed. So virsh reboot first tries to reboot VM thru the agent. The agent calls 'shutdown -r' command. Typically it returns instantly but on this distro for some reason it takes time. I did not investigate the cause but the command waits in dbus client code, probably waits for reply. The libvirt waits 60s for agent command to execute and then errors out. Next reboot API falls back to ACPI shutdown which returns successfully thus the reboot command return success too. Yet shutdown command in guest eventually successfull and guest is truly rebooted. So libvirt does not receive SHUTDOWN event and fake reboot flag which is armed on fallback path stays armed. Thus next shutdown from guest leads to reboot. The issue has 100% repro on Fedora 23. On modern distros I can't reproduce it at all. Shutdown command is asynchronous and returns immediately even if I start some service that ignores TERM signal and thus shutdown procedure waits for 90s (if I not mistaken) before sending KILL. Yet I guess it is nice to have this patch to be more robust. Signed-off-by: Nikolay Shirokovskiy --- src/qemu/qemu_process.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f63fc3389d..d81ed9391a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -435,6 +435,7 @@ qemuProcessHandleReset(qemuMonitor *mon G_GNUC_UNUSED, if (priv->agent) qemuAgentNotifyEvent(priv->agent, QEMU_AGENT_EVENT_RESET); +qemuDomainSetFakeReboot(vm, false); qemuDomainSaveStatus(vm); unlock: -- 2.35.1
[PATCH] qemu: disarm fake reboot flag on reset
binA9h7J9RSaP.bin Description: Binary data
[PATCH v1 2/4] scripts: apibuild: parse 'Since' for typedefs
This patch adds 'version' parameter to the generated XML API for
typedefs
It'll require, for new additions, to add a comment with the version
that the typedef value was added. An example bellow of code diff and
the change in the generated XML.
Note that the Since tag is removed from the comment as there is a
proper field for it in the XML.
```diff
--- a/include/libvirt/libvirt-host.h
+++ b/include/libvirt/libvirt-host.h
@@ -41,6 +41,8 @@ typedef struct _virConnect virConnect;
*
* a virConnectPtr is pointer to a virConnect private structure, this is the
* type used to reference a connection to the Hypervisor in the API.
+ *
+ * Since 0.0.1
*/
typedef virConnect *virConnectPtr;
```
```xml
```
Signed-off-by: Victor Toso
---
scripts/apibuild.py | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/scripts/apibuild.py b/scripts/apibuild.py
index cb68d1b970..bafde0e0ab 100755
--- a/scripts/apibuild.py
+++ b/scripts/apibuild.py
@@ -2324,9 +2324,11 @@ class docBuilder:
output.write("\n \n" %
(desc))
+(comment, since) = self.retrieve_comment_tags(name, id.extra)
+if len(since) > 0:
+output.write(" version='%s'" % escape(since))
+if comment is not None and comment != "":
+output.write(">\n \n" %
(comment))
output.write("\n")
else:
output.write("/>\n")
--
2.35.1
[PATCH v1 4/4] scripts: apibuild: add 'version' to variables
Differently from the previous patches, we don't parse nor export
comments associated with variables. This isn't a big deal because we
only export a single variable: virConnectAuthPtrDefault
Nonetheless, add version field to the exported XML by checking the
allowlist file. This way, if we add another variable in the future, we
can simply add it to that file. Calling the function should also warn
in case we are exporting a new Variable without adding to the file,
e.g:
Missing 'Since' tag for: virConnectAuthPtrDefault
Signed-off-by: Victor Toso
---
scripts/apibuild.py | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/scripts/apibuild.py b/scripts/apibuild.py
index 1235e75999..5493b3065e 100755
--- a/scripts/apibuild.py
+++ b/scripts/apibuild.py
@@ -2341,12 +2341,14 @@ class docBuilder:
def serialize_variable(self, output, name):
id = self.idx.variables[name]
+# Only a single variable exported at the moment. Comments are not
parser nor exported.
+(_, since) = self.retrieve_comment_tags(name, "")
if id.info is not None:
-output.write("\n" % (
-name, self.modulename_file(id.header), id.info))
+output.write("\n" % (
+name, self.modulename_file(id.header), id.info, since))
else:
-output.write("\n" % (
-name, self.modulename_file(id.header)))
+output.write("\n"
% (
+name, self.modulename_file(id.header), since))
def serialize_function(self, output, name):
id = self.idx.functions[name]
--
2.35.1
[PATCH v1 3/4] scripts: apibuild: parse 'Since' for macros
This patch adds 'version' parameter to the generated XML API for
macros
It'll require, for new additions, to add a comment with the version
that the macro was added. An example bellow of code diff and
the change in the generated XML.
Note that the Since tag is removed from the comment as there is a
proper field for it in the XML.
```diff
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2139,6 +2139,8 @@ int virDomainGetVcpus
(virDomainPtr domain,
* virDomainPinVcpu() APIs. VIR_COPY_CPUMAP macro extracts the cpumap of
* the specified vcpu from cpumaps array and copies it into cpumap to be used
* later by virDomainPinVcpu() API.
+ *
+ * Since 0.1.10
*/
# define VIR_COPY_CPUMAP(cpumaps, maplen, vcpu, cpumap) \
memcpy(cpumap, VIR_GET_CPUMAP(cpumaps, maplen, vcpu), maplen)
```
```xml
...
```
Signed-off-by: Victor Toso
---
scripts/apibuild.py | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/scripts/apibuild.py b/scripts/apibuild.py
index bafde0e0ab..1235e75999 100755
--- a/scripts/apibuild.py
+++ b/scripts/apibuild.py
@@ -2265,11 +2265,15 @@ class docBuilder:
output.write(" string='%s'" % strValue)
else:
output.write(" raw='%s'" % escape(rawValue))
+
+(comment, since) = self.retrieve_comment_tags(name, desc)
+if len(since) > 0:
+output.write(" version='%s'" % escape(since))
output.write(">\n")
-if desc is not None and desc != "":
-output.write(" \n" % (desc))
-self.indexString(name, desc)
+if comment is not None and comment != "":
+output.write(" \n" % (comment))
+self.indexString(name, comment)
for arg in args:
(name, desc) = arg
if desc is not None and desc != "":
--
2.35.1
[PATCH v1 0/4] Add 'version' to other exported types
Hi, This series is an extension of what Daniel did in e0e0bf6628 "scripts: include function versions in API definition". The main motivation behind this is to help code generators to consider if a given symbol is present in a given version of libvirt that they are either running/building against, more specifically: https://gitlab.com/libvirt/libvirt-go-module/-/merge_requests/7 As headers are already a great source of documentation for developers, I'm suggesting to add a specific comment to each of this exported types: /* ... Since */ For the use case I mentioned above, I'm adding small parsing function in apibuild.py to fetch the above metadata and included it on the generated XML API. To avoid adding too much noise in the githistory, I'm proposing the addition of symbols.versions.allowlist file, that apibuild.py can use to fetch the first git tag that a given symbol appeared in. I did a small script to generate it, but it sums up to calling the bellow command for any tag that starts with 'v' and has not '-rc'. git grep $symbol $tag ./include I'm trying the simples approach I could find but I'm happy to improve this further based on your suggestions. Ah, the diff in the generated XMLs, per patch (sadly, pasteben drops it after a single day). The initial reference is current master 67c77744d7 'tests: Fixing compiler warning in cputest' 0001 (enums) ... https://paste.centos.org/view/e9137ef0 0002 (typedefs) https://paste.centos.org/view/76f0b397 0003 (macros) .. https://paste.centos.org/view/b68fb03a 0004 (variables) https://paste.centos.org/view/4a20d9bb Cheers, Victor Victor Toso (4): scripts: apibuild: parse 'Since' version for enums scripts: apibuild: parse 'Since' for typedefs scripts: apibuild: parse 'Since' for macros scripts: apibuild: add 'version' to variables scripts/apibuild.py| 68 +- symbols.versions.allowlist | 2144 2 files changed, 2199 insertions(+), 13 deletions(-) create mode 100644 symbols.versions.allowlist -- 2.35.1
[PATCH v1 1/4] scripts: apibuild: parse 'Since' version for enums
This patch adds 'version' parameter to the generated XML API for
enums.
It'll require, for new additions, to add a comment with the version
that the enum value was added. An example bellow of code diff and
the change in the generated XML.
Note that the Since tag is removed from the comment as there is a
proper field for it in the XML.
```diff
- VIR_DOMAIN_STATS_DIRTYRATE = (1 << 9), /* return domain dirty rate info */
+ VIR_DOMAIN_STATS_DIRTYRATE = (1 << 9), /* return domain dirty rate info.
(Since 7.2.0) */
```
```xml
```
To easy the transition between the current state to a more documented
API, we include a file with a list of all exported keywords and the
first tag that git grep has found for them.
Signed-off-by: Victor Toso
---
scripts/apibuild.py| 40 +-
symbols.versions.allowlist | 2144
2 files changed, 2181 insertions(+), 3 deletions(-)
create mode 100644 symbols.versions.allowlist
diff --git a/scripts/apibuild.py b/scripts/apibuild.py
index bdd3077c48..cb68d1b970 100755
--- a/scripts/apibuild.py
+++ b/scripts/apibuild.py
@@ -2030,9 +2030,18 @@ class CParser:
class docBuilder:
"""A documentation builder"""
-def __init__(self, name, syms, path='.', directories=['.'], includes=[]):
+def __init__(self, name, syms, symbols_allowlist, path='.',
directories=['.'], includes=[]):
self.name = name
self.syms = syms
+
+self.allowlist = {}
+with open(symbols_allowlist) as file:
+for line in file:
+if line[0] == "#":
+continue
+(symbol, tag) = line.split()
+self.allowlist[symbol] = tag
+
self.path = path
self.directories = directories
if name == "libvirt":
@@ -2178,6 +2187,25 @@ class docBuilder:
self.scanModules()
self.scanVersions()
+# Fetch tags from the comment. Only 'Since' supported at the moment.
+# For Since, also checks at @allowlist dictionary.
+# Return the tags and the original comment without the tags.
+def retrieve_comment_tags(self, name: str, comment: str) -> (str, str):
+since = ""
+m = re.search("\(?Since v?(\d+\.\d+\.\d+)\)?", comment)
+if m:
+# Remove Since tag from the comment
+(start, end) = m.span()
+comment = comment[:start] + comment[end:]
+comment = comment.strip()
+# Only the version
+since = m.group(1)
+elif name in self.allowlist:
+since = self.allowlist[name][1:]
+else:
+self.warning("Missing 'Since' tag for: " + name)
+return (comment, since)
+
def modulename_file(self, file):
module = os.path.basename(file)
if module[-2:] == '.h':
@@ -2211,7 +2239,11 @@ class docBuilder:
if info[2] is not None and info[2] != '':
output.write(" type='%s'" % info[2])
if info[1] is not None and info[1] != '':
-output.write(" info='%s'" % escape(info[1]))
+# Search for 'Since' version tag
+(comment, since) = self.retrieve_comment_tags(name, info[1])
+if len(since) > 0:
+output.write(" version='%s'" % escape(since))
+output.write(" info='%s'" % escape(comment))
output.write("/>\n")
def serialize_macro(self, output, name):
@@ -2465,6 +2497,8 @@ class app:
self.warning("rebuild() failed, unknown module %s" % name)
return None
+symbols_allowlist = srcdir + "/../symbols.versions.allowlist"
+
builder = None
if glob.glob(srcdir + "/../src/libvirt.c") != []:
if not quiet:
@@ -2474,7 +2508,7 @@ class app:
srcdir + "/../src/util",
srcdir + "/../include/libvirt",
builddir + "/../include/libvirt"]
-builder = docBuilder(name, syms[name], builddir, dirs, [])
+builder = docBuilder(name, syms[name], symbols_allowlist,
builddir, dirs, [])
else:
self.warning("rebuild() failed, unable to guess the module")
return None
diff --git a/symbols.versions.allowlist b/symbols.versions.allowlist
new file mode 100644
index 00..a48493d34d
--- /dev/null
+++ b/symbols.versions.allowlist
@@ -0,0 +1,2144 @@
+virAdmClientClose v1.3.5
+virAdmClientFree v1.3.5
+virAdmClientGetID v1.3.5
+virAdmClientGetInfo v1.3.5
+virAdmClientGetTimestamp v1.3.5
+virAdmClientGetTransport v1.3.5
+virAdmClientPtr v1.3.5
+virAdmClient v1.3.5
+virAdmConnectCloseFunc v1.3.1
+virAdmConnectClose v1.2.17
+virAdmConnectGetLibVersion v1.3.1
+virAdmConnectGetLoggingFilters v3.0.0
+virAdmConnectGetLoggingOutputs v3.0.0
+virAdmConnectGetURI v1.3.1
+virAdmConnectIsAlive v1.3.1
+virAdmConnectListServers v1.3.2
+virAdmConnectLookupServer v1.3.3
+virAdmConnectOpen v1.2.17
+virAdmConnectPtr v1.2.17
Re: libvirt: introduce VIR_DOMAIN_DESTROY_REMOVE_LOGS flag
On 2/14/22 13:19, Nikolay Shirokovskiy wrote: > The patch series based on discussion in RFC [1]. > > I wonder if we'd better add some property like "transient logs" instead > of adding a flag to destoy API. > > Yes libguestfs uses virDomainDestroyFlags to terminate a VM and it is > intended client of this new flag but it or other clients may want to use > shutdown API or use autodestroy domains with same log behaviour. Then > for the current task we only need to support this property in domain xml > I guess. > > [1] removing VMs logs > https://listman.redhat.com/archives/libvir-list/2022-February/msg00273.html > > Nikolay Shirokovskiy (3): > libvirt: introduce VIR_DOMAIN_DESTROY_REMOVE_LOGS flag > qemu: support VIR_DOMAIN_DESTROY_REMOVE_LOGS flag > tools: support --remove-logs flag on destroing domain > > docs/manpages/virsh.rst | 7 +- > include/libvirt/libvirt-domain.h | 1 + > src/libvirt-domain.c | 6 + > src/qemu/qemu_domain.c | 41 > src/qemu/qemu_domain.h | 4 > src/qemu/qemu_driver.c | 8 ++- > tools/virsh-domain.c | 8 ++- > 7 files changed, 72 insertions(+), 3 deletions(-) > Reviewed-by: Michal Privoznik Sorry for delayed review. Michal
Re: [PATCH 01/17] ci: Drop Ubuntu 1804
On 3/2/22 10:48, Daniel P. Berrangé wrote: > On Wed, Mar 02, 2022 at 09:12:55AM +0100, Peter Krempa wrote: >> On Tue, Mar 01, 2022 at 17:46:44 +0100, Erik Skultety wrote: >>> On Tue, Feb 15, 2022 at 02:47:44PM +0100, Peter Krempa wrote: As of April 23 2022, Ubuntu 20.04 will be out for two years, which per our platform support policy means we no longer have to support Ubuntu 18.04. >>> >>> Would you mind contributing the patch to libvirt-ci and regenerating the >>> gitlab.yml config with lcitool from manifest when the time comes? :) >> >> So ... is libvirt-ci always fully mirroring what libvirt does? >> >> AFAIU lcitool is used at least within the qemu project and I didn't >> really check to see whether qemu will continue caring about Ubuntu 18.04 >> and the READMEs in libvirt-ci aren't clearing up the expectations >> either. > > Accidentally (on purpose), I proposed a platform support matrix for QEMU > that has the same rules as libvirt. So broadly speaking both projects > will target the same platforms at any given point in time. > > None the less we should *NOT* remove platforms from libvirt-ci as the > first step. We should remove the platforms from usage in all projects > first. Removing from libvirt-ci should be the last thing. > > This is because while projects broadly follow the same goals, the > timeframe in which those goals are applied may not line up exactly. > There can be constraints from the software release cycles. QEMU is > about to enter freeze, but if they encounter problems in CI they > still want to be able to pull in updates from libvirt-ci, without > Ubuntu 18.04 support being ripped out from under their feet. > But what we could do is the following: diff --git i/guests/lcitool/lcitool/ansible/playbooks/build/jobs/defaults.yml w/guests/lcitool/lcitool/ansible/playbooks/build/jobs/defaults.yml index 4f73393..7676bff 100644 --- i/guests/lcitool/lcitool/ansible/playbooks/build/jobs/defaults.yml +++ w/guests/lcitool/lcitool/ansible/playbooks/build/jobs/defaults.yml @@ -17 +16,0 @@ all_machines: - - ubuntu-1804 diff --git i/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt-dbus.yml w/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt-dbus.yml index 4603135..1ff2dd4 100644 --- i/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt-dbus.yml +++ w/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt-dbus.yml @@ -26 +25,0 @@ - - ubuntu-1804 diff --git i/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt-sandbox.yml w/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt-sandbox.yml index dee3dbe..0b74d3b 100644 --- i/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt-sandbox.yml +++ w/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt-sandbox.yml @@ -14 +13,0 @@ - - ubuntu-1804 diff --git i/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt.yml w/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt.yml index 25e5bcb..d5e0bf7 100644 --- i/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt.yml +++ w/guests/lcitool/lcitool/ansible/playbooks/build/projects/libvirt.yml @@ -26 +25,0 @@ - - ubuntu-1804 Michal
Re: [libvirt RFC] virFile: new VIR_FILE_WRAPPER_BIG_PIPE to improve performance
On 4/5/22 10:35 AM, Dr. David Alan Gilbert wrote:
> * Claudio Fontana (cfont...@suse.de) wrote:
>> On 3/28/22 10:31 AM, Daniel P. Berrangé wrote:
>>> On Sat, Mar 26, 2022 at 04:49:46PM +0100, Claudio Fontana wrote:
On 3/25/22 12:29 PM, Daniel P. Berrangé wrote:
> On Fri, Mar 18, 2022 at 02:34:29PM +0100, Claudio Fontana wrote:
>> On 3/17/22 4:03 PM, Dr. David Alan Gilbert wrote:
>>> * Claudio Fontana (cfont...@suse.de) wrote:
On 3/17/22 2:41 PM, Claudio Fontana wrote:
> On 3/17/22 11:25 AM, Daniel P. Berrangé wrote:
>> On Thu, Mar 17, 2022 at 11:12:11AM +0100, Claudio Fontana wrote:
>>> On 3/16/22 1:17 PM, Claudio Fontana wrote:
On 3/14/22 6:48 PM, Daniel P. Berrangé wrote:
> On Mon, Mar 14, 2022 at 06:38:31PM +0100, Claudio Fontana wrote:
>> On 3/14/22 6:17 PM, Daniel P. Berrangé wrote:
>>> On Sat, Mar 12, 2022 at 05:30:01PM +0100, Claudio Fontana wrote:
the first user is the qemu driver,
virsh save/resume would slow to a crawl with a default pipe
size (64k).
This improves the situation by 400%.
Going through io_helper still seems to incur in some penalty
(~15%-ish)
compared with direct qemu migration to a nc socket to a file.
Signed-off-by: Claudio Fontana
---
src/qemu/qemu_driver.c| 6 +++---
src/qemu/qemu_saveimage.c | 11 ++-
src/util/virfile.c| 12
src/util/virfile.h| 1 +
4 files changed, 22 insertions(+), 8 deletions(-)
Hello, I initially thought this to be a qemu performance issue,
so you can find the discussion about this in qemu-devel:
"Re: bad virsh save /dev/null performance (600 MiB/s max)"
https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg03142.html
>>
>>
>>> Current results show these experimental averages maximum throughput
>>> migrating to /dev/null per each FdWrapper Pipe Size (as per QEMU QMP
>>> "query-migrate", tests repeated 5 times for each).
>>> VM Size is 60G, most of the memory effectively touched before
>>> migration,
>>> through user application allocating and touching all memory with
>>> pseudorandom data.
>>>
>>> 64K: 5200 Mbps (current situation)
>>> 128K:5800 Mbps
>>> 256K: 20900 Mbps
>>> 512K: 21600 Mbps
>>> 1M: 22800 Mbps
>>> 2M: 22800 Mbps
>>> 4M: 22400 Mbps
>>> 8M: 22500 Mbps
>>> 16M:22800 Mbps
>>> 32M:22900 Mbps
>>> 64M:22900 Mbps
>>> 128M: 22800 Mbps
>>>
>>> This above is the throughput out of patched libvirt with multiple
>>> Pipe Sizes for the FDWrapper.
>>
>> Ok, its bouncing around with noise after 1 MB. So I'd suggest that
>> libvirt attempt to raise the pipe limit to 1 MB by default, but
>> not try to go higher.
>>
>>> As for the theoretical limit for the libvirt architecture,
>>> I ran a qemu migration directly issuing the appropriate QMP
>>> commands, setting the same migration parameters as per libvirt,
>>> and then migrating to a socket netcatted to /dev/null via
>>> {"execute": "migrate", "arguments": { "uri",
>>> "unix:///tmp/netcat.sock" } } :
>>>
>>> QMP:37000 Mbps
>>
>>> So although the Pipe size improves things (in particular the
>>> large jump is for the 256K size, although 1M seems a very good
>>> value),
>>> there is still a second bottleneck in there somewhere that
>>> accounts for a loss of ~14200 Mbps in throughput.
Interesting addition: I tested quickly on a system with faster cpus
and larger VM sizes, up to 200GB,
and the difference in throughput libvirt vs qemu is basically the same
~14500 Mbps.
~5 mbps qemu to netcat socket to /dev/null
~35500 mbps virsh save to /dev/null
Seems it is not proportional to cpu speed by the looks of it (not a
totally fair comparison because the VM sizes are different).
>>>
>>> It might be closer to RAM or cache bandwidth limited though; for an
>>> extra copy.
>>
>> I was thinking about sendfile(2) in iohelper, but that probably
>> can't work as the input fd is a socket, I am getting EINVAL.
>
> Yep, sendfile() requires the input to be a mmapable FD,
> and the output to be a socket.
>
> Try
Re: [PATCH 0/9] Fixes and improvements around virXMLPropTristate*()
On 3/25/22 17:26, Andrea Bolognani wrote: > I have manually audited all commits that have converted existing > code to use the new helpers (full list below) and have convinced > myself that allowReboot is indeed the only one that requires > special handling. > > Changes from [RFC]: > > * introduce a separate helper for the one scenario where we need > to accept 'default' as input instead of going for an approach > that would require updating most callers; > > * throw in a bunch of extra fixes and improvements. > > [RFC] https://listman.redhat.com/archives/libvir-list/2022-March/229506.html > > Commits that performed the conversion in a way that accurately > preserved the existing behavior: > > 14c803cb8276c564d242d4aec8f7d1f64da30321 > deac783c86009e0e828746c8c49de70f656aeb85 > e1e2e7ec46191a83eadf32be8a87901b01488a6d > cea39e95291517ff2a8ae515573bad587a7dad2d > 388cdd11f3c746690edc8e0f71289872c6180c60 > dfff3db7763f55b0157f22d816d84c71bbc2dcad > 793e71ee76acb49b215e31ee89e0c9cb52322811 > 92204134806ba4c41ba6cbc20ad2408015e7f3d5 > c348da7c4c138c108695c5f309f62e87d0101eda > 45abc1a5db60213bf94b5fb780dc6549ddd9103c > 45a61cbf68a2459202b7eda4a01a56bfcb6be048 > be63e8703c7354bdf0c767a771223b33ab2ad4f2 > b13f801d6ff6f9fe5e58aa48fc31bd25ad0b072b > b45ba35e350f2a62c6b06a637d707029bc99e559 > fa48004af5ba58cfab38cd8ab5092719a5978509 > fcc563a29bd91bd2a45b8f242eccaf37ab75b5b3 > 97cdb5be1ea84b5224f6353b425ea2503763df07 > 754a7f6c942268b2b604de072a3391ea4df91e57 > b975a8a755b192f5980a14c46e745a92d3cdd3be > e2a38216d2ff6dac8bc927603fdec1ba887b2da7 > cd4c756fd5eece6b1d7723a407ead0ed1bf8a298 > e663717cb13e49f4da21280bc2f455ddfeee782c > cacde05ad3e8428ad0e16afcfdb715cb4b3d3165 > 567efa85c2194e45ae943c01c9dffe5b44e81c96 > 54fb0b9e95c398e8ba09bc7cdceca3f588f910b8 > 3b7bc307d5fd8e7b9c618a6454d32d59b83277d2 > 2beae8273b932505888dec35ae97bd8d74893272 > 0eb42087c7907f43c114cb57b5ff2cf2a52dfea4 > b683978f1f4609e7a099ff8b36d7cac25e84cf7c > a85d553d7608bf4c9ffe74546d32afe0275d7c69 > 86cfd4d4e8de1834607e977318eee4e0c6a9a565 > 593140dabd66f01aa0d606984ff684f7cb9c1eb1 > 956373230631929dfa9a36814a283666cb290ee0 > 0f8fd4548295e3a1516939d7f3bb912a8d7e4713 > 7ae08ef3a230978510e2722fc56b61ebbae9c6b7 > > Commits that altered the existing behavior but did so safely, in > that they stopped accepting 'default' as a valid value for a > property that was defined in the schema as only taking either 'yes' > or 'no' and for which libvirt itself never produced the 'default' > value: > > 9086ae4facfb91436c1d9e7daec9285156cb4eb8 > 931afa7d99b8ff6eb18a6aa402ed64b789197d30 > 38dc25989c50dad1f6f64aa038de8c2d1c008734 > 38d76cde5e90cbb59cd8c726f35646be1740c685 > ee387289dd6c993bd2bcfdebcbad86a51fe36da8 > 550981ce9713ef82df49116c336a9f4bd0eedef4 > b127e50290383dc26e8714ba866acc9f501d7af6 > 54635ea592859d110e87d19565ffabe24d2f0e2e > 232c01ec4f650523ab5ff84cf38c4d8b55763052 > bb94b3d28db909d43d83b3f2ab73aa3f881b5c95 > 3681a5393313eba7bfa8e8f763116efb9961c686 > > Commits that altered the existing behavior in a way that required > some fixing afterwards: > > 0fe2d8dd335054fae38b46bbbac58a4662e1a1d0 > > Andrea Bolognani (9): > conf: Don't pass PROP_NONZERO to virXMLPropTristateBool() > conf: Use virTristateBoolToBool() more > qemu: Format conditionally > util: Introduce virXMLPropTristateBoolAllowDefault() > qemu: Accept > qemu: Don't ignore XMLParseAllowReboot() errors > conf: Restore error checking in VideoAccelDefParseXML() > conf: Format managed property of hostdev-pci ports correctly > network: Convert managed property of hostdev-pci ports correctly > > src/conf/domain_conf.c| 42 +++ > src/conf/interface_conf.c | 2 +- > src/conf/virnetworkportdef.c | 7 +++- > src/libvirt_private.syms | 1 + > src/network/bridge_driver.c | 2 +- > src/qemu/qemu_domain.c| 19 + > src/util/virxml.c | 18 > src/util/virxml.h | 7 > .../plug-hostdev-pci-unmanaged.xml| 12 ++ > tests/virnetworkportxml2xmltest.c | 1 + > 10 files changed, 83 insertions(+), 28 deletions(-) > create mode 100644 > tests/virnetworkportxml2xmldata/plug-hostdev-pci-unmanaged.xml > Reviewed-by: Michal Privoznik Michal
Re: [libvirt PATCH 00/10] Automatic mutex management - part 5
On 3/25/22 16:02, Tim Wiederhake wrote: > Use the recently implemented VIR_LOCK_GUARD and VIR_WITH_MUTEX_LOCK_GUARD > to simplify mutex management. > > Tim Wiederhake (10): > virnetdaemon: Use automatic mutex management > bridge_driver: Use automatic mutex management > node_device_driver: Use automatic mutex management > interface_backend_netcf: Use automatic mutex management > node_device_udev: Use automatic mutex management > qemu_agent: Use automatic mutex management > vbox_common: Use automatic mutex management > datatypes: Use automatic mutex management > ch_monitor: Use automatic mutex management > virportallocator: Use automatic mutex management > > src/ch/ch_monitor.c | 55 +++ > src/datatypes.c | 63 +++- > src/interface/interface_backend_netcf.c | 192 +++- > src/network/bridge_driver.c | 59 +++- > src/network/bridge_driver_linux.c | 4 +- > src/node_device/node_device_driver.c| 12 +- > src/node_device/node_device_udev.c | 76 +- > src/qemu/qemu_agent.c | 25 ++- > src/rpc/virnetdaemon.c | 144 ++ > src/util/virportallocator.c | 93 +--- > src/vbox/vbox_common.c | 31 ++-- > 11 files changed, 302 insertions(+), 452 deletions(-) > Reviewed-by: Michal Privoznik Michal
Re: [libvirt PATCH 08/10] datatypes: Use automatic mutex management
On 3/25/22 16:02, Tim Wiederhake wrote:
> Signed-off-by: Tim Wiederhake
> ---
> src/datatypes.c | 63 +++--
> 1 file changed, 19 insertions(+), 44 deletions(-)
>
> diff --git a/src/datatypes.c b/src/datatypes.c
> index aa614612f9..da8a9970f1 100644
> --- a/src/datatypes.c
> +++ b/src/datatypes.c
> int
> @@ -1173,14 +1153,12 @@
> virAdmConnectCloseCallbackDataRegister(virAdmConnectCloseCallbackData *cbdata,
> void *opaque,
> virFreeCallback freecb)
> {
> -int ret = -1;
> -
> -virObjectLock(cbdata);
Here, @cbdata used to be locked ...
> +VIR_LOCK_GUARD lock = virObjectLockGuard(cbdata);
>
> if (cbdata->callback) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("A close callback is already registered"));
> -goto cleanup;
> +return -1;
> }
>
> cbdata->conn = virObjectRef(conn);
> @@ -1188,10 +1166,7 @@
> virAdmConnectCloseCallbackDataRegister(virAdmConnectCloseCallbackData *cbdata,
> cbdata->opaque = opaque;
> cbdata->freeCallback = freecb;
>
> -ret = 0;
> - cleanup:
> -virObjectUnlock(conn->closeCallback);
... but ere conn->closeCallback was unlocked. I believe your change is
correct and in fact fixes this problem. But can't really verify because
this function is never called.
> -return ret;
> +return 0;
> }
>
> virAdmServerPtr
Michal
[PATCH] virsh: Remove any reference of KVM device assignment
The KVM device assignment was removed in v5.7.0-rc1~103 but virsh
and its manpage still mention it. Don't do that.
Signed-off-by: Michal Privoznik
---
docs/manpages/virsh.rst | 7 +++
tools/virsh-nodedev.c | 2 +-
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index d2e6528533..215beabd96 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -5198,10 +5198,9 @@ guests via passthrough. This is reversed with
``nodedev-reattach``, and is done automatically for managed devices.
Different backend drivers expect the device to be bound to different
-dummy devices. For example, QEMU's "kvm" backend driver (the default)
-expects the device to be bound to pci-stub, but its "vfio" backend
-driver expects the device to be bound to vfio-pci. The *--driver*
-parameter can be used to specify the desired backend driver.
+dummy devices. For example, QEMU's "vfio" backend driver expects the
+device to be bound to vfio-pci. The *--driver* parameter can be used
+to specify the desired backend driver.
nodedev-dumpxml
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index d21b2baab6..90066249af 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -615,7 +615,7 @@ static const vshCmdOptDef opts_node_device_detach[] = {
{.name = "driver",
.type = VSH_OT_STRING,
.completer = virshNodeDevicePCIBackendCompleter,
- .help = N_("pci device assignment backend driver (e.g. 'vfio' or 'kvm')")
+ .help = N_("pci device assignment backend driver (e.g. 'vfio' or 'xen')")
},
{.name = NULL}
};
--
2.35.1
Re: [libvirt RFC] virFile: new VIR_FILE_WRAPPER_BIG_PIPE to improve performance
* Claudio Fontana (cfont...@suse.de) wrote:
> On 3/28/22 10:31 AM, Daniel P. Berrangé wrote:
> > On Sat, Mar 26, 2022 at 04:49:46PM +0100, Claudio Fontana wrote:
> >> On 3/25/22 12:29 PM, Daniel P. Berrangé wrote:
> >>> On Fri, Mar 18, 2022 at 02:34:29PM +0100, Claudio Fontana wrote:
> On 3/17/22 4:03 PM, Dr. David Alan Gilbert wrote:
> > * Claudio Fontana (cfont...@suse.de) wrote:
> >> On 3/17/22 2:41 PM, Claudio Fontana wrote:
> >>> On 3/17/22 11:25 AM, Daniel P. Berrangé wrote:
> On Thu, Mar 17, 2022 at 11:12:11AM +0100, Claudio Fontana wrote:
> > On 3/16/22 1:17 PM, Claudio Fontana wrote:
> >> On 3/14/22 6:48 PM, Daniel P. Berrangé wrote:
> >>> On Mon, Mar 14, 2022 at 06:38:31PM +0100, Claudio Fontana wrote:
> On 3/14/22 6:17 PM, Daniel P. Berrangé wrote:
> > On Sat, Mar 12, 2022 at 05:30:01PM +0100, Claudio Fontana wrote:
> >> the first user is the qemu driver,
> >>
> >> virsh save/resume would slow to a crawl with a default pipe
> >> size (64k).
> >>
> >> This improves the situation by 400%.
> >>
> >> Going through io_helper still seems to incur in some penalty
> >> (~15%-ish)
> >> compared with direct qemu migration to a nc socket to a file.
> >>
> >> Signed-off-by: Claudio Fontana
> >> ---
> >> src/qemu/qemu_driver.c| 6 +++---
> >> src/qemu/qemu_saveimage.c | 11 ++-
> >> src/util/virfile.c| 12
> >> src/util/virfile.h| 1 +
> >> 4 files changed, 22 insertions(+), 8 deletions(-)
> >>
> >> Hello, I initially thought this to be a qemu performance issue,
> >> so you can find the discussion about this in qemu-devel:
> >>
> >> "Re: bad virsh save /dev/null performance (600 MiB/s max)"
> >>
> >> https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg03142.html
>
>
> > Current results show these experimental averages maximum throughput
> > migrating to /dev/null per each FdWrapper Pipe Size (as per QEMU QMP
> > "query-migrate", tests repeated 5 times for each).
> > VM Size is 60G, most of the memory effectively touched before
> > migration,
> > through user application allocating and touching all memory with
> > pseudorandom data.
> >
> > 64K: 5200 Mbps (current situation)
> > 128K:5800 Mbps
> > 256K: 20900 Mbps
> > 512K: 21600 Mbps
> > 1M: 22800 Mbps
> > 2M: 22800 Mbps
> > 4M: 22400 Mbps
> > 8M: 22500 Mbps
> > 16M:22800 Mbps
> > 32M:22900 Mbps
> > 64M:22900 Mbps
> > 128M: 22800 Mbps
> >
> > This above is the throughput out of patched libvirt with multiple
> > Pipe Sizes for the FDWrapper.
>
> Ok, its bouncing around with noise after 1 MB. So I'd suggest that
> libvirt attempt to raise the pipe limit to 1 MB by default, but
> not try to go higher.
>
> > As for the theoretical limit for the libvirt architecture,
> > I ran a qemu migration directly issuing the appropriate QMP
> > commands, setting the same migration parameters as per libvirt,
> > and then migrating to a socket netcatted to /dev/null via
> > {"execute": "migrate", "arguments": { "uri",
> > "unix:///tmp/netcat.sock" } } :
> >
> > QMP:37000 Mbps
>
> > So although the Pipe size improves things (in particular the
> > large jump is for the 256K size, although 1M seems a very good
> > value),
> > there is still a second bottleneck in there somewhere that
> > accounts for a loss of ~14200 Mbps in throughput.
> >>
> >>
> >> Interesting addition: I tested quickly on a system with faster cpus
> >> and larger VM sizes, up to 200GB,
> >> and the difference in throughput libvirt vs qemu is basically the same
> >> ~14500 Mbps.
> >>
> >> ~5 mbps qemu to netcat socket to /dev/null
> >> ~35500 mbps virsh save to /dev/null
> >>
> >> Seems it is not proportional to cpu speed by the looks of it (not a
> >> totally fair comparison because the VM sizes are different).
> >
> > It might be closer to RAM or cache bandwidth limited though; for an
> > extra copy.
>
> I was thinking about sendfile(2) in iohelper, but that probably
> can't work as the input fd is a socket, I am getting EINVAL.
> >>>
> >>> Yep, sendfile() requires the input to be a mmapable FD,
> >>> and the output to be a socket.
> >>>
> >>> Try splice() instead which merely requires 1 end to be a
>
Re: [PATCH] tests: Fixing compiler warning in cputest
On 4/4/22 20:22, Boris Fiuczynski wrote: > Found when building on Fedora 36 on s390x. > > C compiler for the host machine: gcc (gcc 12.0.1 "gcc (GCC) 12.0.1 20220308 > (Red Hat 12.0.1-0)") > C linker for the host machine: gcc ld.bfd 2.37-24 > > In function ‘cpuTestUpdateLiveCompare’, > inlined from ‘cpuTestUpdateLive’ at > ../dist-unpack/libvirt-8.2.5/tests/cputest.c:784:12: > ../dist-unpack/libvirt-8.2.5/tests/cputest.c:696:21: warning: potential null > pointer dereference [-Wnull-dereference] >696 | featAct->policy == VIR_CPU_FEATURE_REQUIRE) || >| ~~~^~~~ > > Signed-off-by: Boris Fiuczynski > --- > tests/cputest.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Michal Privoznik Michal
Re: [PATCH 08/17] qemu: Formally deprecate support for qemu < 3.1
On Mon, Apr 04, 2022 at 08:35:55 -0700, Andrea Bolognani wrote: > On Mon, Apr 04, 2022 at 10:35:25AM +0200, Peter Krempa wrote: > > As of April 23 2022, Ubuntu 20.04 will be out for two years, which means > > we no longer have to support Ubuntu 18.04 along with qemu-2.11 shipped > > with it. > > > > This then brings the minimum qemu version we have to support to > > qemu-3.1: > > > >Debian 10/Stable: 3.1 > > OpenSUSE Leap 15.3: 5.2 > >Ubuntu 20.04: 4.2 > > RHEL/Centos 8.4: 4.2 > > > > Next event in this space will be 2023/07/06 when Debian 11 will be out > > for two years. > > It's actually much earlier than that :) > > Quoting our platform support policy[1]: > > The project aims to support the most recent major version at all > times. Support for the previous major version will be dropped 2 > years after the new major version is released or when the vendor > itself drops support, whichever comes first. In this context, > third-party efforts to extend the lifetime of a distro are not > considered, even when they are endorsed by the vendor (e.g. Debian > LTS); the same is true of repositories that contain packages > backported from later releases (e.g. Debian backports). > > Looking at the Debian wiki[2] we can see > > Version Code name Release date End of life date >10 Buster 2019-07-06 ~2022-08 > > which is consistent with what's written a few lines down > > Reminder: the EOL date for the stable release is the date of the > next stable release plus one year. Oh, I didn't notice that and somehow assumed that we'd have to apply our policy of 2 years. > So come August we'll be able to bump the minimum QEMU version > further, all the way to 4.2 :) That is awesome news. I'm really looking forward to delete all pre-blockdev disk code!!
