On 10/11/16 16:21 +0100, Martin Kletzander wrote:
> On Mon, Nov 07, 2016 at 10:01:13AM +0100, Martin Polednik wrote:
> > As sched_deadline is linux specific, it is not set through
> > sched_setscheduler but rather the sched_setattr syscall. Additionally,
> > the scheduler has new set of parameters: runtime, deadline and period.
> >
> > In this part of the series, we extend virProcessSetScheduler to
> > accommodate the additional parameters and use sched_setattr syscall to
> > set SCHED_DEADLINE. Another small addition is sched_attr struct, which
> > is required for sched_setattr and not exposed from the kernel or
> > libraries.
> > ---
> > src/qemu/qemu_process.c | 3 ++-
> > src/util/virprocess.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++--
> > src/util/virprocess.h | 28 +++++++++++++++++++-
> > 3 files changed, 97 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 1b67aee..91a635c 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -2395,7 +2395,8 @@ qemuProcessSetupPid(virDomainObjPtr vm,
> >
> > /* Set scheduler type and priority. */
> > if (sched &&
> > - virProcessSetScheduler(pid, sched->policy, sched->priority) < 0)
> > + virProcessSetScheduler(pid, sched->policy, sched->priority,
> > + sched->runtime, sched->deadline, sched->period) < 0)
> > goto cleanup;
> >
> > ret = 0;
> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index d68800b..cd1cc9b 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -70,6 +70,7 @@
> > VIR_LOG_INIT("util.process");
> >
> > #ifdef __linux__
> > +# include <sys/syscall.h>
> > /*
> > * Workaround older glibc. While kernel may support the setns
> > * syscall, the glibc wrapper might not exist. If that's the
> > @@ -91,9 +92,14 @@ VIR_LOG_INIT("util.process");
> > # endif
> > # endif
> >
> > +# ifndef __NR_sched_setattr
> > +# if defined(__x86_64__)
> > +# define __NR_sched_setattr 314
> > +# endif
> > +# endif
> > +
>
> At least ARMs, POWERs and i386 should be defined from the start so that
> tests in CI don't fail right away. And other platforms should error out
> right away. But honestly, I'd drop this compatibility code. What
> you're adding here is not a bleeding-edge feature.
It maybe makes sense to virReportSystemError in case it's not defined
-- although the feature isn't cutting edge, older kernels (such as
3.10 on CentOS) do not have the syscall (or the number)
unless they're rebases with RT_PREEMPT patch.
> > # ifndef HAVE_SETNS
> > # if defined(__NR_setns)
> > -# include <sys/syscall.h>
> >
> > static inline int setns(int fd, int nstype)
> > {
> > @@ -103,6 +109,13 @@ static inline int setns(int fd, int nstype)
> > # error Please determine the syscall number for setns on your architecture
> > # endif
> > # endif
> > +
> > +static inline int sched_setattr(pid_t pid,
> > + const struct sched_attr *attr,
> > + unsigned int flags)
> > +{
> > + return syscall(__NR_sched_setattr, pid, attr, flags);
> > +}
> > #else /* !__linux__ */
> > static inline int setns(int fd ATTRIBUTE_UNUSED, int nstype ATTRIBUTE_UNUSED)
> > {
> > @@ -110,6 +123,15 @@ static inline int setns(int fd ATTRIBUTE_UNUSED, int nstype ATTRIBUTE_UNUSED)
> > _("Namespaces are not supported on this platform."));
> > return -1;
> > }
> > +
> > +static inline int sched_setattr(pid_t pid,
> > + const struct sched_attr *attr,
> > + unsigned int flags)
> > +{
> > + virReportSystemError(ENOSYS, "%s",
> > + _("Deadline scheduler is not supported on this platform."));
> > + return -1;
> > +}
> > #endif
> >
> > VIR_ENUM_IMPL(virProcessSchedPolicy, VIR_PROC_POLICY_LAST,
> > @@ -1225,7 +1247,10 @@ virProcessSchedTranslatePolicy(virProcessSchedPolicy policy)
> > int
> > virProcessSetScheduler(pid_t pid,
> > virProcessSchedPolicy policy,
> > - int priority)
> > + int priority,
> > + unsigned long long runtime,
> > + unsigned long long deadline,
> > + unsigned long long period)
> > {
> > struct sched_param param = {0};
> > int pol = virProcessSchedTranslatePolicy(policy);
> > @@ -1235,6 +1260,47 @@ virProcessSetScheduler(pid_t pid,
> > if (!policy)
> > return 0;
> >
> > + if (pol == SCHED_DEADLINE) {
> > + int ret = 0;
> > +
> > + if (runtime < 1024 || deadline < 1024 || period < 24) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("Scheduler runtime, deadline and period must be "
> > + "higher or equal to 1024 (1 us) (runtime(%llu), "
> > + "deadline(%llu), period(%llu))"), runtime, deadline, period);
> > + return -1;
> > + }
> > +
> > + if (!(runtime <= deadline && deadline <= period)) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("Scheduler configuration does not satisfy "
> > + "(runtime(%llu) <= deadline(%llu) <= period(%llu))"),
> > + runtime, deadline, period);
> > + return -1;
> > + }
> > +
> > + struct sched_attr attrs = {
> > + .size = sizeof(attrs),
> > + .sched_policy = SCHED_DEADLINE,
> > + .sched_flags = 0,
> > + .sched_nice = 0,
> > + .sched_priority = 0,
> > + .sched_runtime = runtime,
> > + .sched_deadline = deadline,
> > + .sched_period = period,
> > + };
> > +
>
> C99 initialization is forbidden in libvirt (but we have no syntax-check
> rule for it), move it to the top of the code block.
>
> > + ret = sched_setattr(pid, &attrs, 0);
> > + if (ret != 0) {
> > + virReportSystemError(errno,
> > + _("Cannot set scheduler parameters for pid %d"),
> > + pid);
> > + return -1;
> > + }
> > +
> > + return 0;
> > + }
> > +
> > if (pol == SCHED_FIFO || pol == SCHED_RR) {
> > int min = 0;
> > int max = 0;
> > diff --git a/src/util/virprocess.h b/src/util/virprocess.h
> > index 1eac3e6..e6914e7 100644
> > --- a/src/util/virprocess.h
> > +++ b/src/util/virprocess.h
> > @@ -23,6 +23,9 @@
> > # define __VIR_PROCESS_H__
> >
> > # include <sys/types.h>
> > +# ifdef __linux__
> > +# include <linux/types.h>
> > +# endif
> >
> > # include "internal.h"
> > # include "virbitmap.h"
> > @@ -39,6 +42,26 @@ typedef enum {
> > VIR_PROC_POLICY_LAST
> > } virProcessSchedPolicy;
> >
> > +# ifdef __linux__
> > +struct sched_attr {
> > + __u32 size;
> > +
> > + __u32 sched_policy;
> > + __u64 sched_flags;
> > +
> > + /* SCHED_NORMAL, SCHED_BATCH */
> > + __s32 sched_nice;
> > +
> > + /* SCHED_FIFO, SCHED_RR */
> > + __u32 sched_priority;
> > +
> > + /* SCHED_DEADLINE (nsec) */
> > + __u64 sched_runtime;
> > + __u64 sched_deadline;
> > + __u64 sched_period;
> > +};
> > +# endif
> > +
>
> I don't like this redefinition. Not only because it uses __uXX instead
> of uint##_t. I think we should have a configure.ac check for
> HAVE_STRUCT_SCHED_ATTR (as we do with some other structs) and just error
> out if it's not available. If we're doing that, we can safely do the
> same thing with SCHED_DEADLINE and not have to carry this code on for
> decades.
The struct doesn't seem to be present in (g)libc or any other
library. I'm afraid we can't therefore simply error out.
