On Tue, Nov 15, 2016 at 03:51:22PM +0100, Martin Polednik wrote:
> 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.
Can we find out why it isn't present in glibc. Is it simply that it
is too new ? If so, IMHO desirable to just wait for glibc to support
it and make libvirt code conditional on the new version.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list