On 23-Jan 10:16, Peter Zijlstra wrote:

On Tue, Jan 22, 2019 at 03:33:15PM +0000, Patrick Bellasi wrote:

quoted

On 22-Jan 15:57, Peter Zijlstra wrote:

quoted

On Tue, Jan 22, 2019 at 02:01:15PM +0000, Patrick Bellasi wrote:

quoted

quoted

quoted

Yes, I would say we have two options:

 1) SCHED_FLAG_KEEP_POLICY enforces all the scheduling class specific
    attributes, but cross class attributes (e.g. uclamp)

 2) add SCHED_KEEP_NICE, SCHED_KEEP_PRIO, and SCED_KEEP_PARAMS
    and use them in the if conditions in D)

So the current KEEP_POLICY basically provides sched_setparam(), and

But it's not exposed user-space.

Correct; not until your first patch indeed.

quoted

quoted

given we have that as a syscall, that is supposedly a useful
functionality.

For uclamp is definitively useful to change clamps without the need to
read beforehand the current policy params and use them in a following
set syscall... which is racy pattern.

Right; but my argument was mostly that if sched_setparam() is a useful
interface, a 'pure' KEEP_POLICY would be too and your (1) looses that.

Ok, that's an argument in favour of option (2).

quoted

quoted

And I suppose the UTIL_CLAMP is !KEEP_UTIL; we could go either way
around with that flag.

What about getting rid of the racy case above by exposing userspace
only the new UTIL_CLAMP and, on:

  sched_setscheduler(flags: UTIL_CLAMP)

we enforce the other two flags from the syscall:

---8<---
        SYSCALL_DEFINE3(sched_setattr)
            if (attr.sched_flags & SCHED_FLAG_KEEP_POLICY) {
                attr.sched_policy = SETPARAM_POLICY;
                attr.sched_flags |= (KEEP_POLICY|KEEP_PARAMS);
            }
---8<---

This will not make possible to change class and set flags in one go,
but honestly that's likely a very limited use-case, isn't it ?

So I must admit to not seeing much use for sched_setparam() (and its
equivalents) myself, but given it is an existing interface, I also think
it would be nice to cover that functionality in the sched_setattr()
call.

Which will make them sort-of equivalent... meaning: both the POSIX
sched_setparam() and the !POSIX sched_setattr() will allow to change
params/attributes without changing the policy.

That is; I know of userspace priority-ceiling implementations using
sched_setparam(), but I don't see any reason why that wouldn't also work
with sched_setscheduler() (IOW always also set the policy).

The sched_setscheduler() requires a policy to be explicitely defined,
it's a mandatory parameter and has to be specified.

Unless a RT task could be blocked by a FAIR one and you need
sched_setscheduler() to boost both prio and class (which looks like a
poor RT design to begin with) why would you use sched_setscheduler()
instead of sched_setparam()?

They are both POSIX calls and, AFAIU, sched_setparam() seems to be
designed exactly for those kind on use cases.

quoted

quoted

quoted

In both cases the goal should be to return from code block D).

I don't think so; we really do want to 'goto change' for util changes
too I think. Why duplicate part of that logic?

But that will force a dequeue/enqueue... isn't too much overhead just
to change a clamp value?

These syscalls aren't what I consider fast paths anyway. However, there
are people that rely on the scheduler syscalls not to schedule
themselves, or rather be non-blocking (see for example that prio-ceiling
implementation).

And in that respect the newly introduced uclamp_mutex does appear to be
a problem.

Mmm... could be... I'll look better into it. Could be that that mutex
is not really required. We don't need to serialize task specific
clamp changes and anyway the protected code never sleeps and uses
atomic instruction.

Also; do you expect these clamp values to be changed often?

Not really, the most common use cases are:
 a) a resource manager (e.g. the Android run-time) set clamps for a
    bunch of tasks whenever you switch for example from one app to
    antoher... but that will be done via cgroups (i.e. different path)
 b) a task can relax his constraints to save energy (something
    conceptually similar to use a deferrable timers)

In both cases I expect a limited call frequency.

quoted

Perhaps we can also end up with some wired

s/wired/weird/, right?

Right :)

quoted

side-effects like the task being preempted ?

Nothing worse than any other random reschedule would cause.

quoted

Consider also that the uclamp_task_update_active() added by this patch
not only has lower overhead but it will be use also by cgroups where
we want to force update all the tasks on a cgroup's clamp change.

I haven't gotten that far; but I would prefer not to have two different
'change' paths in __sched_setscheduler().

Yes, I agree that two paths in __sched_setscheduler() could be
confusing. Still we have to consider that here we are adding
"not class specific" attributes.

What if we keep "not class specific" code completely outside of
__sched_setscheduler() and do something like:

---8<---
    int sched_setattr(struct task_struct *p, const struct sched_attr *attr)
    {
            retval = __sched_setattr(p, attr);
            if (retval)
                return retval;
            return __sched_setscheduler(p, attr, true, true);
    }
    EXPORT_SYMBOL_GPL(sched_setattr);
---8<---

where __sched_setattr() will collect all the tunings which do not
require an enqueue/dequeue, so far only the new uclamp settings, while
the rest remains under __sched_setscheduler().

Thoughts ?

-- 
#include <best/regards.h>

Patrick Bellasi