Re: [PATCH] sched/fair: Add missing unthrottle_cfs_rq()

From: Valentin Schneider
Date: Thu Aug 22 2019 - 16:41:00 EST

On 22/08/2019 19:48, bsegall@xxxxxxxxxx wrote:> Having now seen the rest of the thread:
> Could you send the repro, as it doesn't seem to have reached lkml, so
> that I can confirm my guess as to what's going on?

Huh, odd. Here's the thing:

#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <pthread.h>

unsigned long NUM_LOOPS=2500000*250;

/* simple loop based delay: */
static void delay_loop(unsigned long loops)
asm volatile(
" test %0,%0 \n"
" jz 3f \n"
" jmp 1f \n"

".align 16 \n"
"1: jmp 2f \n"

".align 16 \n"
"2: dec %0 \n"
" jnz 2b \n"
"3: dec %0 \n"

: /* we don't need output */
:"a" (loops)

void __const_udelay(unsigned long xloops)
int d0;

xloops *= 4;
asm("mull %%edx"
:"=d" (xloops), "=&a" (d0)
:"1" (xloops), "0"


void __udelay(unsigned long usecs)
__const_udelay(usecs * 0x000010c7); /* 2**32 / 1000000 (rounded up) */

static void *thread_start(void *arg)
while(1) {
__udelay((unsigned long)arg);

int main(int argc, char*argv[])
int i;
int thread;
unsigned long timeout;
pthread_t new_th;

if (argc != 3) {
printf("./delay nr_thread work_loop\n");

thread = atoi(argv[1]);
timeout = (unsigned long)atoi(argv[2]);

for (i = 0; i < thread; i++) {
pthread_create(&new_th, NULL, thread_start, (void *)timeout);

while(1) {

mkdir /sys/fs/cgroup/cpu/test1
echo 100000 > /sys/fs/cgroup/cpu/cpu.cfs_period_us
echo 1600000 > /sys/fs/cgroup/cpu/test1/cpu.cfs_quota_us
./delay 500 1000 &
echo $! > /sys/fs/cgroup/cpu/test1/cgroup.procs
mkdir /sys/fs/cgroup/cpu/test2
echo 100000 > /sys/fs/cgroup/cpu/test2/cpu.cfs_period_us
echo 800000 > /sys/fs/cgroup/cpu/test2/cpu.cfs_quota_us
./delay 500 1000 &
echo $! > /sys/fs/cgroup/cpu/test2/cgroup.procs
prev=0;while true; do curr=`cat /sys/fs/cgroup/cpu/test1/cpuacct.usage` && echo $(($curr-$prev)) && prev=$curr && sleep 1; done

I ran the thing on a dual-socket x86 test box and could trigger the issue
quite rapidly (w/ the WARN_ON in distribute_cfs_runtime()).

> It seems most likely we throttle during one of the remove-change-adds in
> set_cpus_allowed and friends or during the put half of pick_next_task
> followed by idle balance to drop the lock. Then distribute races with a
> later assign_cfs_rq_runtime so that the account finds runtime in the
> cfs_b.

I should still have a trace laying around, let me have a look.

> Re clock_task, it's only frozen for the purposes of pelt, not delta_exec

Noted, thanks. But then we shouldn't expect throttled rq's to call into
update_curr(), right? Maybe just right after they've been throttled, but not
beyond that. Otherwise I fail to see how that would make sense.

> The other possible way to fix this would be to skip assign if throttled,
> since the only time it could succeed is if we're racing with a
> distribute that will unthrottle use anyways.

So pretty much the change Liangyan originally proposed? (so much for trying
to help :p)

> The main advantage of that is the risk of screwy behavior due to unthrottling
> in the middle of pick_next/put_prev. The disadvantage is that we already
> have the lock, if it works we don't need an ipi to trigger a preempt,
> etc. (But I think one of the issues is that we may trigger the preempt
> on the previous task, not the next, and I'm not 100% sure that will
> carry over correctly)