Re: [libvirt] util: allow virProcessSetScheduler to set SCHED_DEADLINE

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.

># 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.

Otherwise I like it. >VIR_ENUM_DECL(virProcessSchedPolicy); > >char * >@@ -93,6 +116,9 @@ int virProcessRunInMountNamespace(pid_t pid, > >int virProcessSetScheduler(pid_t pid, > virProcessSchedPolicy policy, >- int priority); >+ int priority, >+ unsigned long long runtime, >+ unsigned long long deadline, >+ unsigned long long period); > >#endif /* __VIR_PROCESS_H__ */ >-- >2.8.1 > >-- >libvir-list mailing list &gt;libvir-list(a)redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list