[libvirt] [PATCH] qemu: Report warning if QoS is set for vhostuser interface

Michal Privoznik posted 1 patch 5 years, 4 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/4d0d2ee9c421854f465a684c06912fc1eb3eb980.1541000865.git.mprivozn@redhat.com
src/qemu/qemu_command.c | 6 ++++++
1 file changed, 6 insertions(+)
[libvirt] [PATCH] qemu: Report warning if QoS is set for vhostuser interface
Posted by Michal Privoznik 5 years, 4 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1524230

Because of historical reasons, we are not denying starting a
domain which has QoS set for unsupported type of device. We do
report just a warning instead. And even though we perhaps used to
do so for vhostuser it got lost somewhere. Bring it back.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6e3ff67660..489e8bc689 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8246,6 +8246,12 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
+    if (virDomainNetGetActualBandwidth(net)) {
+        VIR_WARN("setting bandwidth on interfaces of "
+                 "type '%s' is not implemented yet",
+                 virDomainNetTypeToString(VIR_DOMAIN_NET_TYPE_VHOSTUSER));
+    }
+
     switch ((virDomainChrType)net->data.vhostuser->type) {
     case VIR_DOMAIN_CHR_TYPE_UNIX:
         if (!(chardev = qemuBuildChrChardevStr(logManager, secManager,
-- 
2.18.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Report warning if QoS is set for vhostuser interface
Posted by Erik Skultety 5 years, 4 months ago
On Wed, Oct 31, 2018 at 04:47:45PM +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1524230
>
> Because of historical reasons, we are not denying starting a
> domain which has QoS set for unsupported type of device. We do
> report just a warning instead. And even though we perhaps used to
> do so for vhostuser it got lost somewhere. Bring it back.

So, if my blame-fu isn't flawed, then I don't think that vhostuser ever
reported a warning during machine startup, have a look at commits 4a74ccdb92f
and 0bce012d7f0 respectively - there always was a goto cleanup statement.

>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_command.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6e3ff67660..489e8bc689 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8246,6 +8246,12 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
>          goto cleanup;
>      }
>
> +    if (virDomainNetGetActualBandwidth(net)) {
> +        VIR_WARN("setting bandwidth on interfaces of "
> +                 "type '%s' is not implemented yet",
> +                 virDomainNetTypeToString(VIR_DOMAIN_NET_TYPE_VHOSTUSER));
> +    }
> +

We already do this kind of checking on line 8593 in the same source file. It's
just because of the goto cleanup jump after calling
qemuBuildVhostuserCommandLine that causes the logic to never reach the condition
(Not to mention the code is a mess). I'd suggest moving the already existing
check to the top of the qemuBuildInterfaceCommandLine function instead of
duplicating the code.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Report warning if QoS is set for vhostuser interface
Posted by Michal Privoznik 5 years, 4 months ago
On 11/01/2018 10:33 AM, Erik Skultety wrote:
> On Wed, Oct 31, 2018 at 04:47:45PM +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1524230
>>
>> Because of historical reasons, we are not denying starting a
>> domain which has QoS set for unsupported type of device. We do
>> report just a warning instead. And even though we perhaps used to
>> do so for vhostuser it got lost somewhere. Bring it back.
> 
> So, if my blame-fu isn't flawed, then I don't think that vhostuser ever
> reported a warning during machine startup, have a look at commits 4a74ccdb92f
> and 0bce012d7f0 respectively - there always was a goto cleanup statement.
> 

Ah, so we did not report warning for vhostuser, but for everything else
we do.

>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_command.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 6e3ff67660..489e8bc689 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -8246,6 +8246,12 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
>>          goto cleanup;
>>      }
>>
>> +    if (virDomainNetGetActualBandwidth(net)) {
>> +        VIR_WARN("setting bandwidth on interfaces of "
>> +                 "type '%s' is not implemented yet",
>> +                 virDomainNetTypeToString(VIR_DOMAIN_NET_TYPE_VHOSTUSER));
>> +    }
>> +
> 
> We already do this kind of checking on line 8593 in the same source file. It's
> just because of the goto cleanup jump after calling
> qemuBuildVhostuserCommandLine that causes the logic to never reach the condition
> (Not to mention the code is a mess). I'd suggest moving the already existing
> check to the top of the qemuBuildInterfaceCommandLine function instead of
> duplicating the code.

I'm not sure I understand. The code looks something like this:

  switch (actualType) {
    case VIR_DOMAIN_NET_TYPE_NETWORK:
    case VIR_DOMAIN_NET_TYPE_BRIDGE:
        createTapDevice();
        break;

        ....
    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
        ret = qemuBuildVhostuserCommandLine();
        goto cleanup;
  }

  if (bandwidth) {
    if (bandwidthSupported(actualType))
      setBandwidth();
    else
      VIR_WARN();
  }

Obviously, I can't just move 'if (bandwidth)' chunk before switch()
because setBandwidth() wouldn't have an interface to operate on (as it
is created in the switch). Or did you have something else on mind?

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Report warning if QoS is set for vhostuser interface
Posted by Erik Skultety 5 years, 4 months ago
On Thu, Nov 01, 2018 at 02:07:21PM +0100, Michal Privoznik wrote:
> On 11/01/2018 10:33 AM, Erik Skultety wrote:
> > On Wed, Oct 31, 2018 at 04:47:45PM +0100, Michal Privoznik wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1524230
> >>
> >> Because of historical reasons, we are not denying starting a
> >> domain which has QoS set for unsupported type of device. We do
> >> report just a warning instead. And even though we perhaps used to
> >> do so for vhostuser it got lost somewhere. Bring it back.
> >
> > So, if my blame-fu isn't flawed, then I don't think that vhostuser ever
> > reported a warning during machine startup, have a look at commits 4a74ccdb92f
> > and 0bce012d7f0 respectively - there always was a goto cleanup statement.
> >
>
> Ah, so we did not report warning for vhostuser, but for everything else
> we do.
>
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/qemu/qemu_command.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >> index 6e3ff67660..489e8bc689 100644
> >> --- a/src/qemu/qemu_command.c
> >> +++ b/src/qemu/qemu_command.c
> >> @@ -8246,6 +8246,12 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
> >>          goto cleanup;
> >>      }
> >>
> >> +    if (virDomainNetGetActualBandwidth(net)) {
> >> +        VIR_WARN("setting bandwidth on interfaces of "
> >> +                 "type '%s' is not implemented yet",
> >> +                 virDomainNetTypeToString(VIR_DOMAIN_NET_TYPE_VHOSTUSER));
> >> +    }
> >> +
> >
> > We already do this kind of checking on line 8593 in the same source file. It's
> > just because of the goto cleanup jump after calling
> > qemuBuildVhostuserCommandLine that causes the logic to never reach the condition
> > (Not to mention the code is a mess). I'd suggest moving the already existing
> > check to the top of the qemuBuildInterfaceCommandLine function instead of
> > duplicating the code.
>
> I'm not sure I understand. The code looks something like this:
>
>   switch (actualType) {
>     case VIR_DOMAIN_NET_TYPE_NETWORK:
>     case VIR_DOMAIN_NET_TYPE_BRIDGE:
>         createTapDevice();
>         break;
>
>         ....
>     case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
>         ret = qemuBuildVhostuserCommandLine();
>         goto cleanup;
>   }
>
>   if (bandwidth) {
>     if (bandwidthSupported(actualType))
>       setBandwidth();
>     else
>       VIR_WARN();
>   }
>
> Obviously, I can't just move 'if (bandwidth)' chunk before switch()
> because setBandwidth() wouldn't have an interface to operate on (as it
> is created in the switch). Or did you have something else on mind?

Nah, my bad, I missed that there actually was the setBandwidth call. That
basically leaves us with 2 options (if don't count your original patch) and both
of them include some refactor:

1) extract the common bits of NET_TYPE_(DIRECT|ETHERNET|BRIDGE) to a helper,
then extract the bandwidth bits to a helper and call this from the
per-interface-type helpers
2) do a massive refactor of the whole interface-cmdline generating function, so
that there wouldn't be short exit paths just like it is the case now for
vhostuser so that in the end the bandwidth snippet we have can stay and will be
applied to all the interface types without exception.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list