[PATCH 3/3] domain_conf: add SCHED_DEADLINE support in the XML configuration

[Michal Prívozník]

On 8/1/22 19:11, Sasha Algisi wrote:
> Users can set SCHED_DEADLINE as a scheduling policy.
> 
> For example, for setting runtime = 10000000, deadline = 15000000
> and period = 20000000 for vcpus 0-2:
> 
> <cputune>
>   ...
>   <vcpusched vcpus="0-2" scheduler="deadline" runtime="10000000"
>    deadline="15000000" period="20000000"/>
>   ...
> </cputune>
> 
> Update release notes accordingly.
> 
> Signed-off-by: Sasha Algisi <sasha.algisi at edu.unito.it>
> Signed-off-by: Dario Faggioli <dfaggioli at suse.com>
> ---
>  NEWS.rst                          |  5 +++
>  docs/formatdomain.rst             | 16 +++++++---
>  src/conf/domain_conf.c            | 52 ++++++++++++++++++++++++++++---
>  src/conf/schemas/domaincommon.rng | 16 ++++++++++
>  4 files changed, 80 insertions(+), 9 deletions(-)
> 
> diff --git a/NEWS.rst b/NEWS.rst
> index ef298da539..23484afdc2 100644
> --- a/NEWS.rst
> +++ b/NEWS.rst
> @@ -17,6 +17,11 @@ v8.7.0 (unreleased)
>  
>  * **New features**
>  
> +  * qemu: support for SCHED_DEADLINE scheduling
> +
> +    Users can now use the SCHED_DEADLINE scheduling policy for tasks
> +    associated to virtual CPUs, IO Threads and Emulator processes.
> +
>  * **Improvements**
>  

Bonus points for remembering to update NEWS.rst, but we tend to do that
in a separate patch. The reason being: easier backports. I mean, when a
downstream maintainer decides to backport these patches, they would get
a conflict in the NEWS file instantly.

>  * **Bug fixes**
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 1ed969ac3e..216262b79d 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -910,10 +910,11 @@ CPU Tuning
>     support since 2.1.0`
>  ``vcpusched``, ``iothreadsched`` and ``emulatorsched``
>     The optional ``vcpusched``, ``iothreadsched`` and ``emulatorsched`` elements
> -   specify the scheduler type (values ``batch``, ``idle``, ``fifo``, ``rr``) for
> -   particular vCPU, IOThread and emulator threads respectively. For ``vcpusched``
> -   and ``iothreadsched`` the attributes ``vcpus`` and ``iothreads`` select which
> -   vCPUs/IOThreads this setting applies to, leaving them out sets the default.
> +   specify the scheduler type (values ``batch``, ``idle``, ``fifo``, ``rr``,
> +   ``deadline`` :since:`Since 8.7.0`) for particular vCPU, IOThread and emulator
> +   threads respectively. For ``vcpusched`` and ``iothreadsched`` the attributes
> +   ``vcpus`` and ``iothreads`` select which vCPUs/IOThreads this setting applies
> +   to, leaving them out sets the default.
>     The element ``emulatorsched`` does not have that attribute. Valid ``vcpus``
>     values start at 0 through one less than the number of vCPU's defined for the
>     domain. Valid ``iothreads`` values are described in the `IOThreads Allocation`_
> @@ -923,6 +924,13 @@ CPU Tuning
>     priority must be specified as well (and is ignored for non-real-time ones).
>     The value range for the priority depends on the host kernel (usually 1-99).
>     :since:`Since 1.2.13` ``emulatorsched`` :since:`since 5.3.0`
> +   For SCHED_DEADLINE (``deadline``), runtime , deadline and period must also
> +   be specified (they are ignored in other schedulers). It must always be true
> +   that: runtime <= deadline <= period.
> +   The values are specified in nanoseconds. The valid range for the parameters
> +   is [1024, 2^63-1] (but a smaller one can be put in place via sysctl). The
> +   period can be set to 0, in which case, a period equal to the deadline is
> +   used.

I wonder whether we should make the @period attribute optional then and
if not provided in the XML then fill in the value provided to @deadline.
Just a suggestion though.

>  ``cachetune`` :since:`Since 4.1.0`
>     Optional ``cachetune`` element can control allocations for CPU caches using
>     the resctrl on the host. Whether or not is this supported can be gathered
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e85cc1f809..86ada8f147 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16693,7 +16693,10 @@ virDomainLoaderDefParseXML(virDomainLoaderDef *loader,
>  static int
>  virDomainSchedulerParseCommonAttrs(xmlNodePtr node,
>                                     virProcessSchedPolicy *policy,
> -                                   int *priority)
> +                                   int *priority,
> +                                   uint64_t *runtime,
> +                                   uint64_t *deadline,
> +                                   uint64_t *period)

Again, why not use ull instead? I'm just going to stop picking on this.
I'm sure you get the idea. The rest of the patch looks correct.


What I'm missing in this patch is a test case. We tend to either
introduce a new one or extend an existing one whenever we touch XML
parser/formatter (tests/qemuxml2xmltest.c).

Quick git grep scheduler= -- tests/   shows some test cases that could
be extended, or where inspiration for a new one can be taken from.

Michal