Re: util: allow virProcessSetScheduler to set SCHED_DEADLINE

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]