[libvirt] [PATCH] qemu: don't error out when cgroups don't exist

[Ján Tomko]

On 07/09/2014 02:30 PM, Martin Kletzander wrote:
> On Wed, Jul 09, 2014 at 01:07:47PM +0200, Ján Tomko wrote:
>> On 07/09/2014 10:15 AM, Martin Kletzander wrote:
>>> When creating cgroups for vcpu and emulator threads whilst starting a
>>> domain, we explicitly skip creating those cgroups in case priv->cgroup
>>> is NULL (cgroups not supported) because SetAffinity() serves the same
>>> purpose.  If the host supports only some cgroups (the ones we need are
>>> either unmounted or disabled in qemu.conf), we error out with weird
>>> message even though we could continue starting the domain.
>>
>> Yet this patch does not change the error message.
>>
> 
> Yes, because there should be no error.
> 
>>>
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1097028
>>>
>>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>>> ---
>>>  src/qemu/qemu_cgroup.c | 12 ++++++++++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>>> index 3394c68..0af6ac5 100644
>>> --- a/src/qemu/qemu_cgroup.c
>>> +++ b/src/qemu/qemu_cgroup.c
>>> @@ -949,7 +949,11 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
>>>          virCgroupFree(&cgroup_vcpu);
>>>      }
>>>
>>> -    return -1;
>>> +    if (period || quota)
>>> +        return -1;
>>> +
>>> +    virResetLastError();
>>> +    return 0;
>>>  }
>>>
>>
>> This also resets OOM errors and errors that happen when these controllers are
>> mounted.
>>
>> Can't we just check upfront if the needed controllers are available?
>>
> 
> There might be more problems than the controllers not being mounted,
> but OK, the others are corner cases anyway.  Would the following diff
> be more suitable?

Yes, that is much nicer.

> 
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 3394c68..d4c969b 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -893,6 +893,15 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
>         return -1;
>     }
> 
> +    /*
> +     * If CPU cgroup controller is not initialized here, than we need
> +     * neither period nor quota settings.  And if CPUSET controller is
> +     * not initialized either, than there's nothing to do anyway.
> +     */
> +    if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) &&
> +        !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET))
> +        return 0;
> +
>     /* We are trying to setup cgroups for CPU pinning, which can also
>     be done
>      * with virProcessSetAffinity, thus the lack of cgroups is not
>      fatal here.
>      */
> @@ -972,6 +981,15 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver,
>         return -1;
>     }
> 
> +    /*
> +     * If CPU cgroup controller is not initialized here, than we need
> +     * neither period nor quota settings.  And if CPUSET controller is
> +     * not initialized either, than there's nothing to do anyway.
> +     */
> +    if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) &&
> +        !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET))
> +        return 0;
> +
>     if (priv->cgroup == NULL)
>         return 0; /* Not supported, so claim success */
> 

s/than/then/ in the comments.

ACK with that fixed.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140709/f44d960f/attachment-0001.sig>