Re: [PATCH v3 1/3] sched/fair: remove quota/burst write-order dependency

[Benjamin Segall <bsegall@google.com>]

Zhe Liu <liuzhe1@kylinos.cn> writes:

> Burst validation currently depends on the active quota, so the same quota
> and burst values may be accepted or rejected depending on the order in
> which userspace updates them.
>
> Make burst validation independent of the current quota while preserving,
> for finite quotas, the previous maximum configurable burst at
> max_bw_runtime_us / 2. Keep the configured burst across quota updates and
> cap the effective burst to min(burst, quota) when CFS runtime is refilled.
>
> Fixes: f4183717b370 ("sched/fair: Introduce the burstable CFS controller")
> Signed-off-by: Zhe Liu <liuzhe1@kylinos.cn>
> Tested-by: Tao Cui <cuitao@kylinos.cn>
> Reviewed-by: Michal Koutný <mkoutny@suse.com>

Yeah, all the state validation that winds up requiring doing edits in a
particular order is annoying and imo not particularly valuable.

Reviewed-by: Ben Segall <bsegall@google.com>

> ---
>  kernel/sched/core.c | 3 +--
>  kernel/sched/fair.c | 3 ++-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b998ef6b87af..c399f0a59c25 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -10181,8 +10181,7 @@ static int tg_set_bandwidth(struct task_group *tg,
>  	if (quota_us != RUNTIME_INF && quota_us > max_bw_runtime_us)
>  		return -EINVAL;
>  
> -	if (quota_us != RUNTIME_INF && (burst_us > quota_us ||
> -					burst_us + quota_us > max_bw_runtime_us))
> +	if (burst_us > max_bw_runtime_us / 2)
>  		return -EINVAL;
>  
>  #ifdef CONFIG_CFS_BANDWIDTH
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index ade1eceb39b8..e6f9d9296fd2 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6627,7 +6627,8 @@ void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
>  		cfs_b->nr_burst++;
>  	}
>  
> -	cfs_b->runtime = min(cfs_b->runtime, cfs_b->quota + cfs_b->burst);
> +	cfs_b->runtime = min(cfs_b->runtime,
> +			     cfs_b->quota + min(cfs_b->burst, cfs_b->quota));
>  	cfs_b->runtime_snap = cfs_b->runtime;
>  }