Re: [PATCH v5 20/29] cgroup/cpuset: Give back the bandwidth an attach did not move

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

Hi Juri,

> Humm, this is identity-blind: it cannot tell a task that can_attach()
> counted (its bandwidth really is inside sum_migrate_dl_bw) from one that
> became a deadline task in the can_attach() -> attach() gap, for which
> nothing was reserved, can it?

It cannot, and your example happens. There was no test for it.

I wrote a reproducer for it, in a 4-CPU guest. P is a one-CPU partition
root, which admits 95%. A process starts in the root cgroup, in the
root domain of the three other CPUs. One write to cgroup.procs moves
the process and all its threads into P. The process has:
- a main thread, the thread group leader. It is SCHED_OTHER and asleep.
- two sleeping deadline threads of 30% each.
- 2048 other sleeping threads, to make the move long.

During the move, another process calls sched_setattr() on the leader
and makes it a deadline task of 40%. cpuset_attach() handles the
leader first.

Without the changes below, each sched_setattr() that landed between
cpuset_can_attach() and cpuset_attach() gave the same result:
- P counts 40%, for the leader.
- The root domain the process left still counts the 60% of the two
   threads.
- When the two threads run, their bandwidth follows them, with no
   admission test. P then counts 100%.

> Not sure if we can consider tracking the counted tasks per task? The
> taskset is pinned between can_attach() and attach(), so we could build a
> list of (task, bw) pairs at can_attach() and let the gate check
> membership?

Yes. The planned next version does that, with a field in the task
instead of a list. It needs no allocation in cpuset_can_attach(), and
no lookup. Three things change:

1. This patch. cpuset_can_attach() records, with each task it counts,
    that room was made for it. cpuset_attach_task() moves the bandwidth
    of a task only if the task has that record. In the test above, P
    then counts the 60% of the two threads. The leader's 40% stays in
    the root domain it was admitted in. The patch has your Suggested-by.

2. Patch 21 of v5. The record is then the CPU the room was reserved on.
    cpuset_attach_task() moves the bandwidth to that CPU, and does not
    compute the CPU again. cpuset_can_attach() picks that CPU from the
    one the task is on. This matters for a sleeping task that wakes up
    during the attach and runs on another CPU. Computed again, the CPU
    differs, and the task misses its reservation.

    A wakeup cannot wait for the attach, and cannot fail. It does not
    need to. Between cpuset_can_attach() and cpuset_attach(), the
    bandwidth of each counted task is held in two places, as it is
    today. The root domain it leaves still counts it, and the
    destination has room reserved for it. A task that wakes up in
    between still has its old mask, so it runs in the root domain it
    leaves, which counts it. cpuset_attach_task() then moves the
    bandwidth to the reserved CPU and releases it in the root domain the
    task left.

3. A new patch. With 1 and 2 alone, the leader is in P after the move,
    and its bandwidth is still counted in the root domain it left. The
    cause is that sched_setattr() lands between the two steps at all.
    __sched_setscheduler() takes cpuset_mutex for a change that involves
    SCHED_DEADLINE, but the attach drops that mutex between
    cpuset_can_attach() and cpuset_attach(). So such a sched_setattr()
    now waits for an attach in progress, and so does sched_setaffinity()
    of a deadline task.

With the three, the move and the sched_setattr() never both succeed in
the test:
- If the sched_setattr() comes first, the move counts the three tasks
   and is refused with EBUSY.
- If it comes after, the leader has its new CPU and has not run on it.
   The call is refused with EPERM, as for any sleeping task moved to
   another partition.

A thread can still die during an attach. This patch already gives back
its part of the reservation.

Your finding led to one more change. In the test above, the bandwidth
of the two threads reached P with no admission test. Patch 23 of v5
does that on purpose: it moves the bandwidth of a task to the root
domain the task runs in. With isolcpus="domain", a cpuset that is not a
partition root can hold CPUs of two root domains. A deadline task in it
could then change root domain with no cpuset attach at all, and patch 23
moved its bandwidth unchecked.

Three new patches keep a deadline task in the root domain that counts
its bandwidth:
- Where a CPU is picked for the task, a CPU of that root domain is
   tried first. This covers a CPU offline, a mask change, a wakeup and
   a rebuild.
- A CPU offline fails with EBUSY if it takes the last CPU the task has
   in that root domain.
- A write to cpuset.cpus fails with EBUSY if it leaves the task no CPU
   in that root domain. cpuset makes a partition root that would do the
   same invalid.

The bandwidth of a task then goes to another root domain only by a
cpuset attach, which asks for room.

With admission control off, nothing is refused, and patch 23 still
moves the bandwidth. So an administrator gets past a refusal by writing
-1 to sched_rt_runtime_us. The two refusals are new behaviour for
user-space. I would like your opinion on them, and on whether that knob
is enough or a dedicated one is wanted.

The planned next version adds cases to deadline_bw_test for the changes
to the attach. Each is about a move between cpusets:
- a policy change during the move, with and without room in P;
- an affinity change during the move;
- a wakeup during the move, with and without room to spare;
- a thread that dies during the move;
- a move that another controller refuses;
- a move of a frozen task.
Four more cases cover the two refusals and the choice of a CPU.

About your sched-deadline-tests: I went through the branch you pointed
to. Four reproducers had no equivalent in deadline_bw_test, so I added
cases for them:
- a task stopped by SIGSTOP that leaves the class;
- a sleeping task that leaves the class after its CPU went offline;
- admission control off, with a task pinned to a CPU that goes offline;
- a deadline waiter that boosts two SCHED_OTHER owners through a chain
   of two mutexes.

They pass with the series. On v7.4-rc1, the first fails, and the third
failed in 6 guests of 8. The patch that was 11 in v5 fixes the first.
The one that was 13 fixes the third.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com