Message ID | 20221028093403.6673-7-vincent.guittot@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp725924wru; Fri, 28 Oct 2022 02:46:32 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5DIxCex+BgSLYQha9pznBUcv1PTbH31Wfp8a8bjZG78VBqD1OliqkYKzUgoLIIBSo54Ll6 X-Received: by 2002:a17:907:75c6:b0:79c:d3f4:4a14 with SMTP id jl6-20020a17090775c600b0079cd3f44a14mr33051616ejc.61.1666950392040; Fri, 28 Oct 2022 02:46:32 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666950392; cv=none; d=google.com; s=arc-20160816; b=EfPRaFphKetqHAroOOgOAArn+TzV8++k1XPNT65kc5WAeqLkZhm8zmLox77M3ZOI7D hj6x8hFhrpxVpQ+y+niX0B/Wi6DTFZK2UV7CMycbdIzyd9wrozaj592escorWWQrfpT6 ZYHrqiy7wGrkJFL2gA2cX2ajuYPQc2yYH1OMSyGSfks3EIfl4cPaOItpLBBcS2ixRGJ2 zCY7+5zrwB8110SPpzWN+rk4l6r835r+xJFLWN025CQce3inhXovXJiUiZMo6ACncppi rZl2ilwDjM8Pu6jm8tBaWB6SmLqLwwyf1YXlwP69iBTrZL7YHmxFkxUIBhn4/+yHtQvO 6tOg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:references:in-reply-to:message-id:date:subject :cc:to:from:dkim-signature; bh=Ji/tbDBalDOjXwUPedhgktma27oNzTvFrvjSlmC+AaI=; b=fdRbcSIk81Bl+txE8K55e4xlkPdRMzssTG14pyFCWeQSlW5H8jc+XgCs1782zPyPOc 7txmApk4qGd2+PejTxZhWc7atsuAttLS49XIoUhOsw3f2DSiHTRy5LTxOJFlNZBrCG2d 8FGdWnIbOqWLg71qDScAq2nTFtNMXg5h3d2gJ5mqXE3oqDi6bMbYlMEyhJ8r71XXaez+ 6bnJRAi21P23KLNBKp8Fz67HwJ47ZwPKdf5hcgUFzvm8WDc6MkV42nqUAZpj/JM9poMY B1+OnJSJ5pX7P24wPkOjh+Atm0eA9OLhgfvkbDmcPrlImG+wM4dVvaHPljhdRbbICcSr U4kg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=CtUI0dFW; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id c2-20020a056402158200b00460faf7d2a3si3611563edv.277.2022.10.28.02.46.08; Fri, 28 Oct 2022 02:46:32 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=CtUI0dFW; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230062AbiJ1Jfp (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Fri, 28 Oct 2022 05:35:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33268 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230203AbiJ1JfK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 28 Oct 2022 05:35:10 -0400 Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 27AED5E669 for <linux-kernel@vger.kernel.org>; Fri, 28 Oct 2022 02:35:09 -0700 (PDT) Received: by mail-wm1-x332.google.com with SMTP id bh7-20020a05600c3d0700b003c6fb3b2052so3309102wmb.2 for <linux-kernel@vger.kernel.org>; Fri, 28 Oct 2022 02:35:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=references:in-reply-to:message-id:date:subject:cc:to:from:from:to :cc:subject:date:message-id:reply-to; bh=Ji/tbDBalDOjXwUPedhgktma27oNzTvFrvjSlmC+AaI=; b=CtUI0dFWy5iBU4N/aa/n7y2seGmPfFZZwMfMtQcUOOcdShtwp0DRIhX/ffww9WKKGN ims2UGmSqQmVN+WYwAQxOURX4MeGIidCnJwokOEHyT+lzeafGpDgo3LpLwRmRRQI6jD3 RV+udddnCPmrjsUJuWG5tPUZewk7XMwatPNoQacj3IaO60HveNqadXx4qzQkyBVHCo5L z0HwQtyT1sZM2Qjnb7/+rYt8Jr4L/Zz4uqJ/FoFMQX4p/zJNQT/qkuay3qPSkbCJsk05 P4/eBySziyTXApUa2HPfWVrk/udkx7EvXxXVJcFRg0l4At7oI/ExiJosG1PqCJmuOYo3 4BjA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=references:in-reply-to:message-id:date:subject:cc:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Ji/tbDBalDOjXwUPedhgktma27oNzTvFrvjSlmC+AaI=; b=MeJpT2cl90g5z8Vruh2PgRrtdsZiNu4lVNfNlJ1I7xs17pe4t4C8fWFB+L59Lc4laA xGg7OgRU2H760tyCEsY1MGsajAo3+GmW3tL6dGBIWJThD5cBF1D52fxaLplKCh7Gz8du gnzdvNiLbjiW60IHaHoPXMX/m0r+mvMoMw/WWmsB/weuVKkOnIrhQPuyZUMgmv7xicXe cKIwfbOS55XuwRR5rXg46KWK0NXCqXSwFBxWR0OvkEVqcZuXWmGvVry4knvXYJmEV9a6 JY6MkOOYkNV7bSHPxWaZC26N+MjaNSZqNSyOQskXRlkkO8qgHfOrvqZpieWk2Dyr1H/4 gpIQ== X-Gm-Message-State: ACrzQf14pF9aRexXIDHUBmCIRhBYxKb8JWXm2y2nFjswLOQClKraqk36 d7OQwL7/XH9Igg1dtkRuM9m3Gg== X-Received: by 2002:a5d:4d12:0:b0:236:751a:9c90 with SMTP id z18-20020a5d4d12000000b00236751a9c90mr15518471wrt.609.1666949707524; Fri, 28 Oct 2022 02:35:07 -0700 (PDT) Received: from localhost.localdomain ([2a01:e0a:f:6020:c12b:b448:f0a9:83ef]) by smtp.gmail.com with ESMTPSA id k3-20020a05600c1c8300b003c6b7f5567csm10909426wms.0.2022.10.28.02.35.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Oct 2022 02:35:06 -0700 (PDT) From: Vincent Guittot <vincent.guittot@linaro.org> To: mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com, dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com, linux-kernel@vger.kernel.org, parth@linux.ibm.com Cc: qais.yousef@arm.com, chris.hyser@oracle.com, patrick.bellasi@matbug.net, David.Laight@aculab.com, pjt@google.com, pavel@ucw.cz, tj@kernel.org, qperret@google.com, tim.c.chen@linux.intel.com, joshdon@google.com, timj@gnu.org, kprateek.nayak@amd.com, yu.c.chen@intel.com, youssefesmat@chromium.org, joel@joelfernandes.org, Vincent Guittot <vincent.guittot@linaro.org> Subject: [PATCH v7 6/9] sched/fair: Add sched group latency support Date: Fri, 28 Oct 2022 11:34:00 +0200 Message-Id: <20221028093403.6673-7-vincent.guittot@linaro.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20221028093403.6673-1-vincent.guittot@linaro.org> References: <20221028093403.6673-1-vincent.guittot@linaro.org> X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747924174052713957?= X-GMAIL-MSGID: =?utf-8?q?1747924174052713957?= |
Series |
Add latency priority for CFS class
|
|
Commit Message
Vincent Guittot
Oct. 28, 2022, 9:34 a.m. UTC
Task can set its latency priority with sched_setattr(), which is then used
to set the latency offset of its sched_enity, but sched group entities
still have the default latency offset value.
Add a latency.nice field in cpu cgroup controller to set the latency
priority of the group similarly to sched_setattr(). The latency priority
is then used to set the offset of the sched_entities of the group.
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
Documentation/admin-guide/cgroup-v2.rst | 8 ++++
kernel/sched/core.c | 52 +++++++++++++++++++++++++
kernel/sched/fair.c | 33 ++++++++++++++++
kernel/sched/sched.h | 4 ++
4 files changed, 97 insertions(+)
Comments
On 10/28/22 11:34, Vincent Guittot wrote: > Task can set its latency priority with sched_setattr(), which is then used > to set the latency offset of its sched_enity, but sched group entities > still have the default latency offset value. > > Add a latency.nice field in cpu cgroup controller to set the latency > priority of the group similarly to sched_setattr(). The latency priority > is then used to set the offset of the sched_entities of the group. > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > --- > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > kernel/sched/fair.c | 33 ++++++++++++++++ > kernel/sched/sched.h | 4 ++ > 4 files changed, 97 insertions(+) > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > index be4a77baf784..d8ae7e411f9c 100644 > --- a/Documentation/admin-guide/cgroup-v2.rst > +++ b/Documentation/admin-guide/cgroup-v2.rst > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > values similar to the sched_setattr(2). This maximum utilization > value is used to clamp the task specific maximum utilization clamp. > > + cpu.latency.nice > + A read-write single value file which exists on non-root > + cgroups. The default is "0". > + > + The nice value is in the range [-20, 19]. > + > + This interface file allows reading and setting latency using the > + same values used by sched_setattr(2). I'm still not sure about this [1]. In some scenarios we'd like to get the effective latency_nice of the task. How will the task inherit the cgroup value or be impacted by it? For example if there are tasks that belong to a latency sensitive cgroup; and I'd like to skip some searches in EAS to improve that latency sensitivity - how would I extract this info in EAS path given these tasks are using default latency_nice value? And if should happen if their latency_nice is set to something else other than default? [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ Thanks -- Qais Yousef > > > Memory > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index caf54e54a74f..3f42b1f61a7e 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -10890,6 +10890,47 @@ static int cpu_idle_write_s64(struct cgroup_subsys_state *css, > { > return sched_group_set_idle(css_tg(css), idle); > } > + > +static s64 cpu_latency_nice_read_s64(struct cgroup_subsys_state *css, > + struct cftype *cft) > +{ > + int prio, delta, last_delta = INT_MAX; > + s64 weight; > + > + weight = css_tg(css)->latency_offset * NICE_LATENCY_WEIGHT_MAX; > + weight = div_s64(weight, get_sched_latency(false)); > + > + /* Find the closest nice value to the current weight */ > + for (prio = 0; prio < ARRAY_SIZE(sched_latency_to_weight); prio++) { > + delta = abs(sched_latency_to_weight[prio] - weight); > + if (delta >= last_delta) > + break; > + last_delta = delta; > + } > + > + return LATENCY_TO_NICE(prio-1); > +} > + > +static int cpu_latency_nice_write_s64(struct cgroup_subsys_state *css, > + struct cftype *cft, s64 nice) > +{ > + s64 latency_offset; > + long weight; > + int idx; > + > + if (nice < MIN_LATENCY_NICE || nice > MAX_LATENCY_NICE) > + return -ERANGE; > + > + idx = NICE_TO_LATENCY(nice); > + idx = array_index_nospec(idx, LATENCY_NICE_WIDTH); > + weight = sched_latency_to_weight[idx]; > + > + latency_offset = weight * get_sched_latency(false); > + latency_offset = div_s64(latency_offset, NICE_LATENCY_WEIGHT_MAX); > + > + return sched_group_set_latency(css_tg(css), latency_offset); > +} > + > #endif > > static struct cftype cpu_legacy_files[] = { > @@ -10904,6 +10945,11 @@ static struct cftype cpu_legacy_files[] = { > .read_s64 = cpu_idle_read_s64, > .write_s64 = cpu_idle_write_s64, > }, > + { > + .name = "latency.nice", > + .read_s64 = cpu_latency_nice_read_s64, > + .write_s64 = cpu_latency_nice_write_s64, > + }, > #endif > #ifdef CONFIG_CFS_BANDWIDTH > { > @@ -11121,6 +11167,12 @@ static struct cftype cpu_files[] = { > .read_s64 = cpu_idle_read_s64, > .write_s64 = cpu_idle_write_s64, > }, > + { > + .name = "latency.nice", > + .flags = CFTYPE_NOT_ON_ROOT, > + .read_s64 = cpu_latency_nice_read_s64, > + .write_s64 = cpu_latency_nice_write_s64, > + }, > #endif > #ifdef CONFIG_CFS_BANDWIDTH > { > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 4299d5108dc7..9583936ce30c 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -11764,6 +11764,7 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent) > goto err; > > tg->shares = NICE_0_LOAD; > + tg->latency_offset = 0; > > init_cfs_bandwidth(tg_cfs_bandwidth(tg)); > > @@ -11862,6 +11863,9 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq, > } > > se->my_q = cfs_rq; > + > + se->latency_offset = tg->latency_offset; > + > /* guarantee group entities always have weight */ > update_load_set(&se->load, NICE_0_LOAD); > se->parent = parent; > @@ -11992,6 +11996,35 @@ int sched_group_set_idle(struct task_group *tg, long idle) > return 0; > } > > +int sched_group_set_latency(struct task_group *tg, s64 latency) > +{ > + int i; > + > + if (tg == &root_task_group) > + return -EINVAL; > + > + if (abs(latency) > sysctl_sched_latency) > + return -EINVAL; > + > + mutex_lock(&shares_mutex); > + > + if (tg->latency_offset == latency) { > + mutex_unlock(&shares_mutex); > + return 0; > + } > + > + tg->latency_offset = latency; > + > + for_each_possible_cpu(i) { > + struct sched_entity *se = tg->se[i]; > + > + WRITE_ONCE(se->latency_offset, latency); > + } > + > + mutex_unlock(&shares_mutex); > + return 0; > +} > + > #else /* CONFIG_FAIR_GROUP_SCHED */ > > void free_fair_sched_group(struct task_group *tg) { } > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h > index 99f10b4dc230..95d4be4f3af6 100644 > --- a/kernel/sched/sched.h > +++ b/kernel/sched/sched.h > @@ -407,6 +407,8 @@ struct task_group { > > /* A positive value indicates that this is a SCHED_IDLE group. */ > int idle; > + /* latency constraint of the group. */ > + int latency_offset; > > #ifdef CONFIG_SMP > /* > @@ -517,6 +519,8 @@ extern int sched_group_set_shares(struct task_group *tg, unsigned long shares); > > extern int sched_group_set_idle(struct task_group *tg, long idle); > > +extern int sched_group_set_latency(struct task_group *tg, s64 latency); > + > #ifdef CONFIG_SMP > extern void set_task_rq_fair(struct sched_entity *se, > struct cfs_rq *prev, struct cfs_rq *next); > -- > 2.17.1 >
On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > On 10/28/22 11:34, Vincent Guittot wrote: > > Task can set its latency priority with sched_setattr(), which is then used > > to set the latency offset of its sched_enity, but sched group entities > > still have the default latency offset value. > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > priority of the group similarly to sched_setattr(). The latency priority > > is then used to set the offset of the sched_entities of the group. > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > --- > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > kernel/sched/fair.c | 33 ++++++++++++++++ > > kernel/sched/sched.h | 4 ++ > > 4 files changed, 97 insertions(+) > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > index be4a77baf784..d8ae7e411f9c 100644 > > --- a/Documentation/admin-guide/cgroup-v2.rst > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > values similar to the sched_setattr(2). This maximum utilization > > value is used to clamp the task specific maximum utilization clamp. > > > > + cpu.latency.nice > > + A read-write single value file which exists on non-root > > + cgroups. The default is "0". > > + > > + The nice value is in the range [-20, 19]. > > + > > + This interface file allows reading and setting latency using the > > + same values used by sched_setattr(2). > > I'm still not sure about this [1]. I'm still not sure about what you are trying to say here ... This is about setting a latency nice prio to a group level. > > In some scenarios we'd like to get the effective latency_nice of the task. How > will the task inherit the cgroup value or be impacted by it? > > For example if there are tasks that belong to a latency sensitive cgroup; and > I'd like to skip some searches in EAS to improve that latency sensitivity - how > would I extract this info in EAS path given these tasks are using default > latency_nice value? And if should happen if their latency_nice is set to > something else other than default? > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ Hmm so you are speaking about something that is not part of the patch. Let focus on the patchset for now > > > Thanks > > -- > Qais Yousef > > > > > > > Memory > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index caf54e54a74f..3f42b1f61a7e 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -10890,6 +10890,47 @@ static int cpu_idle_write_s64(struct cgroup_subsys_state *css, > > { > > return sched_group_set_idle(css_tg(css), idle); > > } > > + > > +static s64 cpu_latency_nice_read_s64(struct cgroup_subsys_state *css, > > + struct cftype *cft) > > +{ > > + int prio, delta, last_delta = INT_MAX; > > + s64 weight; > > + > > + weight = css_tg(css)->latency_offset * NICE_LATENCY_WEIGHT_MAX; > > + weight = div_s64(weight, get_sched_latency(false)); > > + > > + /* Find the closest nice value to the current weight */ > > + for (prio = 0; prio < ARRAY_SIZE(sched_latency_to_weight); prio++) { > > + delta = abs(sched_latency_to_weight[prio] - weight); > > + if (delta >= last_delta) > > + break; > > + last_delta = delta; > > + } > > + > > + return LATENCY_TO_NICE(prio-1); > > +} > > + > > +static int cpu_latency_nice_write_s64(struct cgroup_subsys_state *css, > > + struct cftype *cft, s64 nice) > > +{ > > + s64 latency_offset; > > + long weight; > > + int idx; > > + > > + if (nice < MIN_LATENCY_NICE || nice > MAX_LATENCY_NICE) > > + return -ERANGE; > > + > > + idx = NICE_TO_LATENCY(nice); > > + idx = array_index_nospec(idx, LATENCY_NICE_WIDTH); > > + weight = sched_latency_to_weight[idx]; > > + > > + latency_offset = weight * get_sched_latency(false); > > + latency_offset = div_s64(latency_offset, NICE_LATENCY_WEIGHT_MAX); > > + > > + return sched_group_set_latency(css_tg(css), latency_offset); > > +} > > + > > #endif > > > > static struct cftype cpu_legacy_files[] = { > > @@ -10904,6 +10945,11 @@ static struct cftype cpu_legacy_files[] = { > > .read_s64 = cpu_idle_read_s64, > > .write_s64 = cpu_idle_write_s64, > > }, > > + { > > + .name = "latency.nice", > > + .read_s64 = cpu_latency_nice_read_s64, > > + .write_s64 = cpu_latency_nice_write_s64, > > + }, > > #endif > > #ifdef CONFIG_CFS_BANDWIDTH > > { > > @@ -11121,6 +11167,12 @@ static struct cftype cpu_files[] = { > > .read_s64 = cpu_idle_read_s64, > > .write_s64 = cpu_idle_write_s64, > > }, > > + { > > + .name = "latency.nice", > > + .flags = CFTYPE_NOT_ON_ROOT, > > + .read_s64 = cpu_latency_nice_read_s64, > > + .write_s64 = cpu_latency_nice_write_s64, > > + }, > > #endif > > #ifdef CONFIG_CFS_BANDWIDTH > > { > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > index 4299d5108dc7..9583936ce30c 100644 > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -11764,6 +11764,7 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent) > > goto err; > > > > tg->shares = NICE_0_LOAD; > > + tg->latency_offset = 0; > > > > init_cfs_bandwidth(tg_cfs_bandwidth(tg)); > > > > @@ -11862,6 +11863,9 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq, > > } > > > > se->my_q = cfs_rq; > > + > > + se->latency_offset = tg->latency_offset; > > + > > /* guarantee group entities always have weight */ > > update_load_set(&se->load, NICE_0_LOAD); > > se->parent = parent; > > @@ -11992,6 +11996,35 @@ int sched_group_set_idle(struct task_group *tg, long idle) > > return 0; > > } > > > > +int sched_group_set_latency(struct task_group *tg, s64 latency) > > +{ > > + int i; > > + > > + if (tg == &root_task_group) > > + return -EINVAL; > > + > > + if (abs(latency) > sysctl_sched_latency) > > + return -EINVAL; > > + > > + mutex_lock(&shares_mutex); > > + > > + if (tg->latency_offset == latency) { > > + mutex_unlock(&shares_mutex); > > + return 0; > > + } > > + > > + tg->latency_offset = latency; > > + > > + for_each_possible_cpu(i) { > > + struct sched_entity *se = tg->se[i]; > > + > > + WRITE_ONCE(se->latency_offset, latency); > > + } > > + > > + mutex_unlock(&shares_mutex); > > + return 0; > > +} > > + > > #else /* CONFIG_FAIR_GROUP_SCHED */ > > > > void free_fair_sched_group(struct task_group *tg) { } > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h > > index 99f10b4dc230..95d4be4f3af6 100644 > > --- a/kernel/sched/sched.h > > +++ b/kernel/sched/sched.h > > @@ -407,6 +407,8 @@ struct task_group { > > > > /* A positive value indicates that this is a SCHED_IDLE group. */ > > int idle; > > + /* latency constraint of the group. */ > > + int latency_offset; > > > > #ifdef CONFIG_SMP > > /* > > @@ -517,6 +519,8 @@ extern int sched_group_set_shares(struct task_group *tg, unsigned long shares); > > > > extern int sched_group_set_idle(struct task_group *tg, long idle); > > > > +extern int sched_group_set_latency(struct task_group *tg, s64 latency); > > + > > #ifdef CONFIG_SMP > > extern void set_task_rq_fair(struct sched_entity *se, > > struct cfs_rq *prev, struct cfs_rq *next); > > -- > > 2.17.1 > >
On 11/03/22 09:46, Vincent Guittot wrote: > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > > > On 10/28/22 11:34, Vincent Guittot wrote: > > > Task can set its latency priority with sched_setattr(), which is then used > > > to set the latency offset of its sched_enity, but sched group entities > > > still have the default latency offset value. > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > > priority of the group similarly to sched_setattr(). The latency priority > > > is then used to set the offset of the sched_entities of the group. > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > --- > > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > > kernel/sched/fair.c | 33 ++++++++++++++++ > > > kernel/sched/sched.h | 4 ++ > > > 4 files changed, 97 insertions(+) > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > > index be4a77baf784..d8ae7e411f9c 100644 > > > --- a/Documentation/admin-guide/cgroup-v2.rst > > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > > values similar to the sched_setattr(2). This maximum utilization > > > value is used to clamp the task specific maximum utilization clamp. > > > > > > + cpu.latency.nice > > > + A read-write single value file which exists on non-root > > > + cgroups. The default is "0". > > > + > > > + The nice value is in the range [-20, 19]. > > > + > > > + This interface file allows reading and setting latency using the > > > + same values used by sched_setattr(2). > > > > I'm still not sure about this [1]. > > I'm still not sure about what you are trying to say here ... > > This is about setting a latency nice prio to a group level. > > > > > In some scenarios we'd like to get the effective latency_nice of the task. How > > will the task inherit the cgroup value or be impacted by it? > > > > For example if there are tasks that belong to a latency sensitive cgroup; and > > I'd like to skip some searches in EAS to improve that latency sensitivity - how > > would I extract this info in EAS path given these tasks are using default > > latency_nice value? And if should happen if their latency_nice is set to > > something else other than default? > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ > > Hmm so you are speaking about something that is not part of the patch. > Let focus on the patchset for now I am focusing on this patchset. Isn't this an essential part of the design? Once the interface is out we can't change it. As it stands, I can't see how it can be used to replace prefer_idle in cgroup as used in Android. I can't see how this could happen if we don't define how the task will inherit the cgroup value. If we can, mind elaborating how please? Thanks -- Qais Yousef
On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote: > > On 11/03/22 09:46, Vincent Guittot wrote: > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > On 10/28/22 11:34, Vincent Guittot wrote: > > > > Task can set its latency priority with sched_setattr(), which is then used > > > > to set the latency offset of its sched_enity, but sched group entities > > > > still have the default latency offset value. > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > > > priority of the group similarly to sched_setattr(). The latency priority > > > > is then used to set the offset of the sched_entities of the group. > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > > --- > > > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > > > kernel/sched/fair.c | 33 ++++++++++++++++ > > > > kernel/sched/sched.h | 4 ++ > > > > 4 files changed, 97 insertions(+) > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > > > index be4a77baf784..d8ae7e411f9c 100644 > > > > --- a/Documentation/admin-guide/cgroup-v2.rst > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > > > values similar to the sched_setattr(2). This maximum utilization > > > > value is used to clamp the task specific maximum utilization clamp. > > > > > > > > + cpu.latency.nice > > > > + A read-write single value file which exists on non-root > > > > + cgroups. The default is "0". > > > > + > > > > + The nice value is in the range [-20, 19]. > > > > + > > > > + This interface file allows reading and setting latency using the > > > > + same values used by sched_setattr(2). > > > > > > I'm still not sure about this [1]. > > > > I'm still not sure about what you are trying to say here ... > > > > This is about setting a latency nice prio to a group level. > > > > > > > > In some scenarios we'd like to get the effective latency_nice of the task. How > > > will the task inherit the cgroup value or be impacted by it? > > > > > > For example if there are tasks that belong to a latency sensitive cgroup; and > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how > > > would I extract this info in EAS path given these tasks are using default > > > latency_nice value? And if should happen if their latency_nice is set to > > > something else other than default? > > > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ > > > > Hmm so you are speaking about something that is not part of the patch. > > Let focus on the patchset for now > > I am focusing on this patchset. Isn't this an essential part of the design? > Once the interface is out we can't change it. As it stands, I can't see how it So, are you speaking about the interface i.e. setting a value between [-20:19] > can be used to replace prefer_idle in cgroup as used in Android. I can't see > how this could happen if we don't define how the task will inherit the cgroup > value. If we can, mind elaborating how please? Or how to take into account the value set for a cgroup ? Regarding the behavior, the rule remains the same that a sched_entity attached to a cgroup will not get more (latency in this case) than what has been set for the group entity. > > > Thanks > > -- > Qais Yousef
On Thu, Nov 3, 2022 at 5:03 PM Vincent Guittot <vincent.guittot@linaro.org> wrote: > > On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote: > > > > On 11/03/22 09:46, Vincent Guittot wrote: > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > On 10/28/22 11:34, Vincent Guittot wrote: > > > > > Task can set its latency priority with sched_setattr(), which is then used > > > > > to set the latency offset of its sched_enity, but sched group entities > > > > > still have the default latency offset value. > > > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > > > > priority of the group similarly to sched_setattr(). The latency priority > > > > > is then used to set the offset of the sched_entities of the group. > > > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > > > --- > > > > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > > > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > > > > kernel/sched/fair.c | 33 ++++++++++++++++ > > > > > kernel/sched/sched.h | 4 ++ > > > > > 4 files changed, 97 insertions(+) > > > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > > > > index be4a77baf784..d8ae7e411f9c 100644 > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > > > > values similar to the sched_setattr(2). This maximum utilization > > > > > value is used to clamp the task specific maximum utilization clamp. > > > > > > > > > > + cpu.latency.nice > > > > > + A read-write single value file which exists on non-root > > > > > + cgroups. The default is "0". > > > > > + > > > > > + The nice value is in the range [-20, 19]. > > > > > + > > > > > + This interface file allows reading and setting latency using the > > > > > + same values used by sched_setattr(2). > > > > > > > > I'm still not sure about this [1]. > > > > > > I'm still not sure about what you are trying to say here ... > > > > > > This is about setting a latency nice prio to a group level. > > > > > > > > > > > In some scenarios we'd like to get the effective latency_nice of the task. How > > > > will the task inherit the cgroup value or be impacted by it? > > > > > > > > For example if there are tasks that belong to a latency sensitive cgroup; and > > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how > > > > would I extract this info in EAS path given these tasks are using default > > > > latency_nice value? And if should happen if their latency_nice is set to > > > > something else other than default? > > > > > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ > > > > > > Hmm so you are speaking about something that is not part of the patch. > > > Let focus on the patchset for now > > > > I am focusing on this patchset. Isn't this an essential part of the design? > > Once the interface is out we can't change it. As it stands, I can't see how it > > So, are you speaking about the interface i.e. setting a value between [-20:19] > > > can be used to replace prefer_idle in cgroup as used in Android. I can't see > > how this could happen if we don't define how the task will inherit the cgroup > > value. If we can, mind elaborating how please? > > Or how to take into account the value set for a cgroup ? > > Regarding the behavior, the rule remains the same that a sched_entity > attached to a cgroup will not get more (latency in this case) than > what has been set for the group entity. I think the interface solves a different problem which is latency of task or cgroup wrt other group. Vincent, you are setting this for a “top app” group in android in your tests, and seeing improvement correct? AFAICS, this improvement comes because of lower latency during *same CPU* competition between different groups by juggling around the wakeup-preemption window -- which maybe is good for Android. OTOH, the “prefer idle” flag in android that Qais is referring to, will need a completely different method as I cannot see how a nice value can communicate that (that can complement Vincent's changes here). And it will need to have a per-task interface as well. We have something in ChromeOS as well, which is a proc knob and also out-of-tree patch for that [1]. Without [1] we fail Android CTS testing on a recent ARM64 ChromeOS device. [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3884575 The changelog in [1] also has a detailed description of the ChromeOS usecase. Qais, any other reason you can see why Vincent's change will not be a good thing for Android? Since you 1 CGroup for the whole user-facing app (top app), you can just set that to a low "latency_nice" and get better wake-up latency for that. (Side rant about latency and CFS -- IMHO a better long term solution for lower latency is to use RT but don't throttle -- rather demote. Or break CFS into multiple tiers, and apply demotion. This is in a way what Vincent is doing, as the task becomes more CPU bound'ish, he's taking away the latency boost. Vincent/Qais, somebody was working on the RT demotion vs throttling a while back, any idea on the latest on that?). thanks, - Joel > > > > > > > Thanks > > > > -- > > Qais Yousef
On Fri, 4 Nov 2022 at 11:15, Joel Fernandes <joel@joelfernandes.org> wrote: > > On Thu, Nov 3, 2022 at 5:03 PM Vincent Guittot > <vincent.guittot@linaro.org> wrote: > > > > On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > On 11/03/22 09:46, Vincent Guittot wrote: > > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > > > On 10/28/22 11:34, Vincent Guittot wrote: > > > > > > Task can set its latency priority with sched_setattr(), which is then used > > > > > > to set the latency offset of its sched_enity, but sched group entities > > > > > > still have the default latency offset value. > > > > > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > > > > > priority of the group similarly to sched_setattr(). The latency priority > > > > > > is then used to set the offset of the sched_entities of the group. > > > > > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > > > > --- > > > > > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > > > > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > > > > > kernel/sched/fair.c | 33 ++++++++++++++++ > > > > > > kernel/sched/sched.h | 4 ++ > > > > > > 4 files changed, 97 insertions(+) > > > > > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > > > > > index be4a77baf784..d8ae7e411f9c 100644 > > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst > > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > > > > > values similar to the sched_setattr(2). This maximum utilization > > > > > > value is used to clamp the task specific maximum utilization clamp. > > > > > > > > > > > > + cpu.latency.nice > > > > > > + A read-write single value file which exists on non-root > > > > > > + cgroups. The default is "0". > > > > > > + > > > > > > + The nice value is in the range [-20, 19]. > > > > > > + > > > > > > + This interface file allows reading and setting latency using the > > > > > > + same values used by sched_setattr(2). > > > > > > > > > > I'm still not sure about this [1]. > > > > > > > > I'm still not sure about what you are trying to say here ... > > > > > > > > This is about setting a latency nice prio to a group level. > > > > > > > > > > > > > > In some scenarios we'd like to get the effective latency_nice of the task. How > > > > > will the task inherit the cgroup value or be impacted by it? > > > > > > > > > > For example if there are tasks that belong to a latency sensitive cgroup; and > > > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how > > > > > would I extract this info in EAS path given these tasks are using default > > > > > latency_nice value? And if should happen if their latency_nice is set to > > > > > something else other than default? > > > > > > > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ > > > > > > > > Hmm so you are speaking about something that is not part of the patch. > > > > Let focus on the patchset for now > > > > > > I am focusing on this patchset. Isn't this an essential part of the design? > > > Once the interface is out we can't change it. As it stands, I can't see how it > > > > So, are you speaking about the interface i.e. setting a value between [-20:19] > > > > > can be used to replace prefer_idle in cgroup as used in Android. I can't see > > > how this could happen if we don't define how the task will inherit the cgroup > > > value. If we can, mind elaborating how please? > > > > Or how to take into account the value set for a cgroup ? > > > > Regarding the behavior, the rule remains the same that a sched_entity > > attached to a cgroup will not get more (latency in this case) than > > what has been set for the group entity. > > I think the interface solves a different problem which is latency of > task or cgroup wrt other group. Vincent, you are setting this for a > “top app” group in android in your tests, and seeing improvement > correct? AFAICS, this improvement comes because of lower latency Yes Top app and display group > during *same CPU* competition between different groups by juggling > around the wakeup-preemption window -- which maybe is good for > Android. > > OTOH, the “prefer idle” flag in android that Qais is referring to, > will need a completely different method as I cannot see how a nice > value can communicate that (that can complement Vincent's changes > here). And it will need to have a per-task interface as well. We have Why a negative latency_nice value condition can't be used ? or latency -20 ? > something in ChromeOS as well, which is a proc knob and also > out-of-tree patch for that [1]. Without [1] we fail Android CTS > testing on a recent ARM64 ChromeOS device. > [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3884575 > The changelog in [1] also has a detailed description of the ChromeOS usecase. > > Qais, any other reason you can see why Vincent's change will not be a > good thing for Android? Since you 1 CGroup for the whole user-facing > app (top app), you can just set that to a low "latency_nice" and get > better wake-up latency for that. > > (Side rant about latency and CFS -- IMHO a better long term solution > for lower latency is to use RT but don't throttle -- rather demote. Or > break CFS into multiple tiers, and apply demotion. This is in a way > what Vincent is doing, as the task becomes more CPU bound'ish, he's > taking away the latency boost. Vincent/Qais, somebody was working on > the RT demotion vs throttling a while back, any idea on the latest on > that?). > > thanks, > > - Joel > > > > > > > > > > > > > Thanks > > > > > > -- > > > Qais Yousef
On Fri, Nov 4, 2022 at 10:37 AM Vincent Guittot <vincent.guittot@linaro.org> wrote: [...] > > during *same CPU* competition between different groups by juggling > > around the wakeup-preemption window -- which maybe is good for > > Android. > > > > OTOH, the “prefer idle” flag in android that Qais is referring to, > > will need a completely different method as I cannot see how a nice > > value can communicate that (that can complement Vincent's changes > > here). And it will need to have a per-task interface as well. We have > > Why a negative latency_nice value condition can't be used ? or latency -20 ? That's overloading the meaning of a value, the whole nice thing is supposed to be "relative to something". So you are being nice to something else. Here -20 means you are not being nice. But in fact you are, because you are avoiding hurting something else by going to an idle CPU. So it becomes really weird. Also, why would -19 or -18 not be a value instead to cause wakeup to prefer an idle CPU? It confuses the user on how to choose value and we should refrain from that IMHO.
> On Nov 4, 2022, at 6:37 AM, Vincent Guittot <vincent.guittot@linaro.org> wrote: > > On Fri, 4 Nov 2022 at 11:15, Joel Fernandes <joel@joelfernandes.org> wrote: >> >>> On Thu, Nov 3, 2022 at 5:03 PM Vincent Guittot >>> <vincent.guittot@linaro.org> wrote: >>> >>> On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote: >>>> >>>> On 11/03/22 09:46, Vincent Guittot wrote: >>>>> On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: >>>>>> >>>>>> On 10/28/22 11:34, Vincent Guittot wrote: >>>>>>> Task can set its latency priority with sched_setattr(), which is then used >>>>>>> to set the latency offset of its sched_enity, but sched group entities >>>>>>> still have the default latency offset value. >>>>>>> >>>>>>> Add a latency.nice field in cpu cgroup controller to set the latency >>>>>>> priority of the group similarly to sched_setattr(). The latency priority >>>>>>> is then used to set the offset of the sched_entities of the group. >>>>>>> >>>>>>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> >>>>>>> --- >>>>>>> Documentation/admin-guide/cgroup-v2.rst | 8 ++++ >>>>>>> kernel/sched/core.c | 52 +++++++++++++++++++++++++ >>>>>>> kernel/sched/fair.c | 33 ++++++++++++++++ >>>>>>> kernel/sched/sched.h | 4 ++ >>>>>>> 4 files changed, 97 insertions(+) >>>>>>> >>>>>>> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst >>>>>>> index be4a77baf784..d8ae7e411f9c 100644 >>>>>>> --- a/Documentation/admin-guide/cgroup-v2.rst >>>>>>> +++ b/Documentation/admin-guide/cgroup-v2.rst >>>>>>> @@ -1095,6 +1095,14 @@ All time durations are in microseconds. >>>>>>> values similar to the sched_setattr(2). This maximum utilization >>>>>>> value is used to clamp the task specific maximum utilization clamp. >>>>>>> >>>>>>> + cpu.latency.nice >>>>>>> + A read-write single value file which exists on non-root >>>>>>> + cgroups. The default is "0". >>>>>>> + >>>>>>> + The nice value is in the range [-20, 19]. >>>>>>> + >>>>>>> + This interface file allows reading and setting latency using the >>>>>>> + same values used by sched_setattr(2). >>>>>> >>>>>> I'm still not sure about this [1]. >>>>> >>>>> I'm still not sure about what you are trying to say here ... >>>>> >>>>> This is about setting a latency nice prio to a group level. >>>>> >>>>>> >>>>>> In some scenarios we'd like to get the effective latency_nice of the task. How >>>>>> will the task inherit the cgroup value or be impacted by it? >>>>>> >>>>>> For example if there are tasks that belong to a latency sensitive cgroup; and >>>>>> I'd like to skip some searches in EAS to improve that latency sensitivity - how >>>>>> would I extract this info in EAS path given these tasks are using default >>>>>> latency_nice value? And if should happen if their latency_nice is set to >>>>>> something else other than default? >>>>>> >>>>>> [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ >>>>> >>>>> Hmm so you are speaking about something that is not part of the patch. >>>>> Let focus on the patchset for now >>>> >>>> I am focusing on this patchset. Isn't this an essential part of the design? >>>> Once the interface is out we can't change it. As it stands, I can't see how it >>> >>> So, are you speaking about the interface i.e. setting a value between [-20:19] >>> >>>> can be used to replace prefer_idle in cgroup as used in Android. I can't see >>>> how this could happen if we don't define how the task will inherit the cgroup >>>> value. If we can, mind elaborating how please? >>> >>> Or how to take into account the value set for a cgroup ? >>> >>> Regarding the behavior, the rule remains the same that a sched_entity >>> attached to a cgroup will not get more (latency in this case) than >>> what has been set for the group entity. >> >> I think the interface solves a different problem which is latency of >> task or cgroup wrt other group. Vincent, you are setting this for a >> “top app” group in android in your tests, and seeing improvement >> correct? AFAICS, this improvement comes because of lower latency > > Yes Top app and display group > >> during *same CPU* competition between different groups by juggling >> around the wakeup-preemption window -- which maybe is good for >> Android. >> >> OTOH, the “prefer idle” flag in android that Qais is referring to, >> will need a completely different method as I cannot see how a nice >> value can communicate that (that can complement Vincent's changes >> here). And it will need to have a per-task interface as well. We have > > Why a negative latency_nice value condition can't be used ? or latency -20 ? Ah and forgot to reply about negative. Maybe, but it’s still a horrible overload of the meaning of the value. I am not terribly against choosing negative value if there is consensus among everyone. Qais? - Joel > >> something in ChromeOS as well, which is a proc knob and also >> out-of-tree patch for that [1]. Without [1] we fail Android CTS >> testing on a recent ARM64 ChromeOS device. >> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3884575 >> The changelog in [1] also has a detailed description of the ChromeOS usecase. >> >> Qais, any other reason you can see why Vincent's change will not be a >> good thing for Android? Since you 1 CGroup for the whole user-facing >> app (top app), you can just set that to a low "latency_nice" and get >> better wake-up latency for that. >> >> (Side rant about latency and CFS -- IMHO a better long term solution >> for lower latency is to use RT but don't throttle -- rather demote. Or >> break CFS into multiple tiers, and apply demotion. This is in a way >> what Vincent is doing, as the task becomes more CPU bound'ish, he's >> taking away the latency boost. Vincent/Qais, somebody was working on >> the RT demotion vs throttling a while back, any idea on the latest on >> that?). >> >> thanks, >> >> - Joel >> >> >>> >>>> >>>> >>>> Thanks >>>> >>>> -- >>>> Qais Yousef
On Fri, 4 Nov 2022 at 11:48, Joel Fernandes <joel@joelfernandes.org> wrote: > > On Fri, Nov 4, 2022 at 10:37 AM Vincent Guittot > <vincent.guittot@linaro.org> wrote: > [...] > > > during *same CPU* competition between different groups by juggling > > > around the wakeup-preemption window -- which maybe is good for > > > Android. > > > > > > OTOH, the “prefer idle” flag in android that Qais is referring to, > > > will need a completely different method as I cannot see how a nice > > > value can communicate that (that can complement Vincent's changes > > > here). And it will need to have a per-task interface as well. We have > > > > Why a negative latency_nice value condition can't be used ? or latency -20 ? > > That's overloading the meaning of a value, the whole nice thing is > supposed to be "relative to something". So you are being nice to > something else. Here -20 means you are not being nice. But in fact you > are, because you are avoiding hurting something else by going to an > idle CPU. So it becomes really weird. Looking for an idle CPU 1st is already the default behavior of CFS. Here we speak about an EAS specific behavior where we want to forgot the "full" EAS policy for some tasks and favor latency by spreading and looking for an idle cpu > > Also, why would -19 or -18 not be a value instead to cause wakeup to > prefer an idle CPU? It confuses the user on how to choose value and we > should refrain from that IMHO. IIRC, the 1st idea was to say any negative value but then using the lowest one can be seen as an addon to the wakeup preemption
On 11/03/22 18:02, Vincent Guittot wrote: > On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote: > > > > On 11/03/22 09:46, Vincent Guittot wrote: > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > On 10/28/22 11:34, Vincent Guittot wrote: > > > > > Task can set its latency priority with sched_setattr(), which is then used > > > > > to set the latency offset of its sched_enity, but sched group entities > > > > > still have the default latency offset value. > > > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > > > > priority of the group similarly to sched_setattr(). The latency priority > > > > > is then used to set the offset of the sched_entities of the group. > > > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > > > --- > > > > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > > > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > > > > kernel/sched/fair.c | 33 ++++++++++++++++ > > > > > kernel/sched/sched.h | 4 ++ > > > > > 4 files changed, 97 insertions(+) > > > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > > > > index be4a77baf784..d8ae7e411f9c 100644 > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > > > > values similar to the sched_setattr(2). This maximum utilization > > > > > value is used to clamp the task specific maximum utilization clamp. > > > > > > > > > > + cpu.latency.nice > > > > > + A read-write single value file which exists on non-root > > > > > + cgroups. The default is "0". > > > > > + > > > > > + The nice value is in the range [-20, 19]. > > > > > + > > > > > + This interface file allows reading and setting latency using the > > > > > + same values used by sched_setattr(2). > > > > > > > > I'm still not sure about this [1]. > > > > > > I'm still not sure about what you are trying to say here ... > > > > > > This is about setting a latency nice prio to a group level. > > > > > > > > > > > In some scenarios we'd like to get the effective latency_nice of the task. How > > > > will the task inherit the cgroup value or be impacted by it? > > > > > > > > For example if there are tasks that belong to a latency sensitive cgroup; and > > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how > > > > would I extract this info in EAS path given these tasks are using default > > > > latency_nice value? And if should happen if their latency_nice is set to > > > > something else other than default? > > > > > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ > > > > > > Hmm so you are speaking about something that is not part of the patch. > > > Let focus on the patchset for now > > > > I am focusing on this patchset. Isn't this an essential part of the design? > > Once the interface is out we can't change it. As it stands, I can't see how it > > So, are you speaking about the interface i.e. setting a value between [-20:19] About how the cgroup and per task interface interact. How to get the effective value of latency_nice for a task that belongs to a hierarchy? If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that has tg->cpu.latency.nice = -19 And I want to use this interface in EAS; how should I interpret these values? How should I walk up the hierarchy and decide the _effective_ latency_nice value? > > > can be used to replace prefer_idle in cgroup as used in Android. I can't see > > how this could happen if we don't define how the task will inherit the cgroup > > value. If we can, mind elaborating how please? > > Or how to take into account the value set for a cgroup ? Yes that. How to calculate effective value in a hierarchy taking parents, children and task latency_nice values into account. > > Regarding the behavior, the rule remains the same that a sched_entity > attached to a cgroup will not get more (latency in this case) than > what has been set for the group entity. So it behaves like a limit as described in cgroup-v2.rst? Is this enforced in the series? Thanks -- Qais Yousef > > > > > > > Thanks > > > > -- > > Qais Yousef
On 11/04/22 10:14, Joel Fernandes wrote: > I think the interface solves a different problem which is latency of > task or cgroup wrt other group. Vincent, you are setting this for a > “top app” group in android in your tests, and seeing improvement > correct? AFAICS, this improvement comes because of lower latency > during *same CPU* competition between different groups by juggling > around the wakeup-preemption window -- which maybe is good for > Android. > > OTOH, the “prefer idle” flag in android that Qais is referring to, > will need a completely different method as I cannot see how a nice > value can communicate that (that can complement Vincent's changes > here). And it will need to have a per-task interface as well. We have > something in ChromeOS as well, which is a proc knob and also > out-of-tree patch for that [1]. Without [1] we fail Android CTS > testing on a recent ARM64 ChromeOS device. > [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3884575 > The changelog in [1] also has a detailed description of the ChromeOS usecase. > > Qais, any other reason you can see why Vincent's change will not be a > good thing for Android? Since you 1 CGroup for the whole user-facing > app (top app), you can just set that to a low "latency_nice" and get > better wake-up latency for that. There are two things to consider here: 1. The interface and its extensibility. 2. Current use case of improving wake up latency by manipulating vruntime. (2) looks promising approach, but it's very hard to know if it can be considered a replacement. So (1) must be spot on to not block adding other consumers in EAS or anywhere else in the scheduler for what matters. > > (Side rant about latency and CFS -- IMHO a better long term solution > for lower latency is to use RT but don't throttle -- rather demote. Or > break CFS into multiple tiers, and apply demotion. This is in a way > what Vincent is doing, as the task becomes more CPU bound'ish, he's > taking away the latency boost. Vincent/Qais, somebody was working on > the RT demotion vs throttling a while back, any idea on the latest on > that?). I can see an appetite in the future for userspace to provide more hints about the characteristics of the tasks to improve performance and power. I'm starting to lean towards having a framework to encapsulate such description where latency_nice and potentially new notion of priority or something else can be part of. One aspect of prefer_idle in android for instance is that it disables the packing behavior of EAS. I've seen Prateek showing similar examples in [1] (FORK_SPREAD). I don't have a clear idea in my head, but I'm slowly starting to lean towards the need for a proper QoS/hint framework to describe the characteristic of the task or a collection of tasks working together. My pony wish for now I guess :-) [1] https://lore.kernel.org/lkml/20220910105326.1797-1-kprateek.nayak@amd.com/ Thanks -- Qais Yousef
On 11/04/22 06:55, Joel Fernandes wrote: > >> I think the interface solves a different problem which is latency of task > >> or cgroup wrt other group. Vincent, you are setting this for a “top app” > >> group in android in your tests, and seeing improvement correct? AFAICS, > >> this improvement comes because of lower latency > > > > Yes Top app and display group > > > >> during *same CPU* competition between different groups by juggling around > >> the wakeup-preemption window -- which maybe is good for Android. > >> > >> OTOH, the “prefer idle” flag in android that Qais is referring to, will > >> need a completely different method as I cannot see how a nice value can > >> communicate that (that can complement Vincent's changes here). And it will > >> need to have a per-task interface as well. We have > > > > Why a negative latency_nice value condition can't be used ? or latency -20 > > ? > > Ah and forgot to reply about negative. > > Maybe, but it’s still a horrible overload of the meaning of the value. I am > not terribly against choosing negative value if there is consensus among > everyone. Qais? TBH I think the whole notion of 'nice' is confusing. From my experience talking with some game developers they didn't know how to use nice values or what they exactly mean. Given their target audience is a large diverse range of devices with different spec, and that they can only lower it as increasing it requires privilege; how to decide the right value to work reliably everywhere? latency_nice might suffer from the same problem. But I don't have a better alternative to suggest. I think that was the worrying input from Peter and Thomas; these numbers could appear like magic crystal ball from users perspective but I haven't seen any new discussion in spite my attempt to stir it. As we brought up in another email thread; I think CFS notion of priority could be improved and it could lead to improve this problem by product. But this won't address problems like load balance search times that were brought up as part of this latency discussions in the past. So short answer is I don't know :) I think the interface and use cases should be discussed more still. Thanks -- Qais Yousef
> On Nov 4, 2022, at 7:21 AM, Qais Yousef <qyousef@layalina.io> wrote: > > On 11/03/22 18:02, Vincent Guittot wrote: >>> On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote: >>> >>> On 11/03/22 09:46, Vincent Guittot wrote: >>>> On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: >>>>> >>>>> On 10/28/22 11:34, Vincent Guittot wrote: >>>>>> Task can set its latency priority with sched_setattr(), which is then used >>>>>> to set the latency offset of its sched_enity, but sched group entities >>>>>> still have the default latency offset value. >>>>>> >>>>>> Add a latency.nice field in cpu cgroup controller to set the latency >>>>>> priority of the group similarly to sched_setattr(). The latency priority >>>>>> is then used to set the offset of the sched_entities of the group. >>>>>> >>>>>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> >>>>>> --- >>>>>> Documentation/admin-guide/cgroup-v2.rst | 8 ++++ >>>>>> kernel/sched/core.c | 52 +++++++++++++++++++++++++ >>>>>> kernel/sched/fair.c | 33 ++++++++++++++++ >>>>>> kernel/sched/sched.h | 4 ++ >>>>>> 4 files changed, 97 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst >>>>>> index be4a77baf784..d8ae7e411f9c 100644 >>>>>> --- a/Documentation/admin-guide/cgroup-v2.rst >>>>>> +++ b/Documentation/admin-guide/cgroup-v2.rst >>>>>> @@ -1095,6 +1095,14 @@ All time durations are in microseconds. >>>>>> values similar to the sched_setattr(2). This maximum utilization >>>>>> value is used to clamp the task specific maximum utilization clamp. >>>>>> >>>>>> + cpu.latency.nice >>>>>> + A read-write single value file which exists on non-root >>>>>> + cgroups. The default is "0". >>>>>> + >>>>>> + The nice value is in the range [-20, 19]. >>>>>> + >>>>>> + This interface file allows reading and setting latency using the >>>>>> + same values used by sched_setattr(2). >>>>> >>>>> I'm still not sure about this [1]. >>>> >>>> I'm still not sure about what you are trying to say here ... >>>> >>>> This is about setting a latency nice prio to a group level. >>>> >>>>> >>>>> In some scenarios we'd like to get the effective latency_nice of the task. How >>>>> will the task inherit the cgroup value or be impacted by it? >>>>> >>>>> For example if there are tasks that belong to a latency sensitive cgroup; and >>>>> I'd like to skip some searches in EAS to improve that latency sensitivity - how >>>>> would I extract this info in EAS path given these tasks are using default >>>>> latency_nice value? And if should happen if their latency_nice is set to >>>>> something else other than default? >>>>> >>>>> [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ >>>> >>>> Hmm so you are speaking about something that is not part of the patch. >>>> Let focus on the patchset for now >>> >>> I am focusing on this patchset. Isn't this an essential part of the design? >>> Once the interface is out we can't change it. As it stands, I can't see how it >> >> So, are you speaking about the interface i.e. setting a value between [-20:19] > > About how the cgroup and per task interface interact. > > How to get the effective value of latency_nice for a task that belongs to > a hierarchy? > > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that > has tg->cpu.latency.nice = -19 Just for the task placement signal, One way is to go through the se hierarchy till the root and get the minimum. Then make that the effective value. So In your example that would make it -19 so prefer idle = 1. We should need a Boolean signal. Not pretty but not the end of the world imho. > And I want to use this interface in EAS; how should I interpret these values? > How should I walk up the hierarchy and decide the _effective_ latency_nice > value Ah, using a min function? Joel >> > Thanks > > -- > Qais Yousef > >> >>> >>> >>> Thanks >>> >>> -- >>> Qais Yousef
On Fri, 4 Nov 2022 at 12:21, Qais Yousef <qyousef@layalina.io> wrote: > > On 11/03/22 18:02, Vincent Guittot wrote: > > On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > On 11/03/22 09:46, Vincent Guittot wrote: > > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > > > On 10/28/22 11:34, Vincent Guittot wrote: > > > > > > Task can set its latency priority with sched_setattr(), which is then used > > > > > > to set the latency offset of its sched_enity, but sched group entities > > > > > > still have the default latency offset value. > > > > > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > > > > > priority of the group similarly to sched_setattr(). The latency priority > > > > > > is then used to set the offset of the sched_entities of the group. > > > > > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > > > > --- > > > > > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > > > > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > > > > > kernel/sched/fair.c | 33 ++++++++++++++++ > > > > > > kernel/sched/sched.h | 4 ++ > > > > > > 4 files changed, 97 insertions(+) > > > > > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > > > > > index be4a77baf784..d8ae7e411f9c 100644 > > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst > > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > > > > > values similar to the sched_setattr(2). This maximum utilization > > > > > > value is used to clamp the task specific maximum utilization clamp. > > > > > > > > > > > > + cpu.latency.nice > > > > > > + A read-write single value file which exists on non-root > > > > > > + cgroups. The default is "0". > > > > > > + > > > > > > + The nice value is in the range [-20, 19]. > > > > > > + > > > > > > + This interface file allows reading and setting latency using the > > > > > > + same values used by sched_setattr(2). > > > > > > > > > > I'm still not sure about this [1]. > > > > > > > > I'm still not sure about what you are trying to say here ... > > > > > > > > This is about setting a latency nice prio to a group level. > > > > > > > > > > > > > > In some scenarios we'd like to get the effective latency_nice of the task. How > > > > > will the task inherit the cgroup value or be impacted by it? > > > > > > > > > > For example if there are tasks that belong to a latency sensitive cgroup; and > > > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how > > > > > would I extract this info in EAS path given these tasks are using default > > > > > latency_nice value? And if should happen if their latency_nice is set to > > > > > something else other than default? > > > > > > > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ > > > > > > > > Hmm so you are speaking about something that is not part of the patch. > > > > Let focus on the patchset for now > > > > > > I am focusing on this patchset. Isn't this an essential part of the design? > > > Once the interface is out we can't change it. As it stands, I can't see how it > > > > So, are you speaking about the interface i.e. setting a value between [-20:19] > > About how the cgroup and per task interface interact. > > How to get the effective value of latency_nice for a task that belongs to > a hierarchy? At the common parents level of the 2 entities that you want to compare and root level if there no other entity to compare with > > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that > has tg->cpu.latency.nice = -19 according to what i said above, latency_nice = 20 inside the group and -19 when comparing at the parent level > > And I want to use this interface in EAS; how should I interpret these values? > How should I walk up the hierarchy and decide the _effective_ latency_nice > value? The current use of latency_nice doesn't need to walk the hierarchy because it applies at each scheduling level so the childs automatically follow parents' latency. > > > > > > can be used to replace prefer_idle in cgroup as used in Android. I can't see > > > how this could happen if we don't define how the task will inherit the cgroup > > > value. If we can, mind elaborating how please? > > > > Or how to take into account the value set for a cgroup ? > > Yes that. How to calculate effective value in a hierarchy taking parents, > children and task latency_nice values into account. > > > > > Regarding the behavior, the rule remains the same that a sched_entity > > attached to a cgroup will not get more (latency in this case) than > > what has been set for the group entity. > > So it behaves like a limit as described in cgroup-v2.rst? Is this enforced in > the series? It's like a limit and this is enforced in this serie as we are applying the latency at each level before moving to the next one > > > Thanks > > -- > Qais Yousef > > > > > > > > > > > > Thanks > > > > > > -- > > > Qais Yousef
On Fri, Nov 04, 2022 at 03:24:23PM +0100, Vincent Guittot wrote: > On Fri, 4 Nov 2022 at 12:21, Qais Yousef <qyousef@layalina.io> wrote: > > > > On 11/03/22 18:02, Vincent Guittot wrote: > > > On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > On 11/03/22 09:46, Vincent Guittot wrote: > > > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > > > > > On 10/28/22 11:34, Vincent Guittot wrote: > > > > > > > Task can set its latency priority with sched_setattr(), which is then used > > > > > > > to set the latency offset of its sched_enity, but sched group entities > > > > > > > still have the default latency offset value. > > > > > > > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > > > > > > priority of the group similarly to sched_setattr(). The latency priority > > > > > > > is then used to set the offset of the sched_entities of the group. > > > > > > > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > > > > > --- > > > > > > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > > > > > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > > > > > > kernel/sched/fair.c | 33 ++++++++++++++++ > > > > > > > kernel/sched/sched.h | 4 ++ > > > > > > > 4 files changed, 97 insertions(+) > > > > > > > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > > > > > > index be4a77baf784..d8ae7e411f9c 100644 > > > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst > > > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > > > > > > values similar to the sched_setattr(2). This maximum utilization > > > > > > > value is used to clamp the task specific maximum utilization clamp. > > > > > > > > > > > > > > + cpu.latency.nice > > > > > > > + A read-write single value file which exists on non-root > > > > > > > + cgroups. The default is "0". > > > > > > > + > > > > > > > + The nice value is in the range [-20, 19]. > > > > > > > + > > > > > > > + This interface file allows reading and setting latency using the > > > > > > > + same values used by sched_setattr(2). > > > > > > > > > > > > I'm still not sure about this [1]. > > > > > > > > > > I'm still not sure about what you are trying to say here ... > > > > > > > > > > This is about setting a latency nice prio to a group level. > > > > > > > > > > > > > > > > > In some scenarios we'd like to get the effective latency_nice of the task. How > > > > > > will the task inherit the cgroup value or be impacted by it? > > > > > > > > > > > > For example if there are tasks that belong to a latency sensitive cgroup; and > > > > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how > > > > > > would I extract this info in EAS path given these tasks are using default > > > > > > latency_nice value? And if should happen if their latency_nice is set to > > > > > > something else other than default? > > > > > > > > > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ > > > > > > > > > > Hmm so you are speaking about something that is not part of the patch. > > > > > Let focus on the patchset for now > > > > > > > > I am focusing on this patchset. Isn't this an essential part of the design? > > > > Once the interface is out we can't change it. As it stands, I can't see how it > > > > > > So, are you speaking about the interface i.e. setting a value between [-20:19] > > > > About how the cgroup and per task interface interact. > > > > How to get the effective value of latency_nice for a task that belongs to > > a hierarchy? > > At the common parents level of the 2 entities that you want to compare > and root level if there no other entity to compare with > > > > > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that > > has tg->cpu.latency.nice = -19 > > according to what i said above, latency_nice = 20 inside the group and > -19 when comparing at the parent level > > > > > And I want to use this interface in EAS; how should I interpret these values? > > How should I walk up the hierarchy and decide the _effective_ latency_nice > > value? > > The current use of latency_nice doesn't need to walk the hierarchy > because it applies at each scheduling level so the childs > automatically follow parents' latency. Not really, I don't see how that will work that way in the wake up path. The wake up path (EAS in particular) does not walk through CPU controller group hierarchy from top level, it only cares about cpuset/affinities and the "effective" values of tasks. So when you wake up a task, how will you retrieve the attribute for 'prefer idle' in the wakeup path using this patchset? The only way is to aggregate the CGroup hierarchy information to get a per-task effective value; say using a min function. If you see uclamp_rq_util_with(), that also is using doing uclamp aggregation similarly. So I think Qais is asking about the aggregation function in the EAS wakeup path. Thanks.
On Fri, 4 Nov 2022 at 15:57, Joel Fernandes <joel@joelfernandes.org> wrote: > > On Fri, Nov 04, 2022 at 03:24:23PM +0100, Vincent Guittot wrote: > > On Fri, 4 Nov 2022 at 12:21, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > On 11/03/22 18:02, Vincent Guittot wrote: > > > > On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > > > On 11/03/22 09:46, Vincent Guittot wrote: > > > > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > > > > > > > On 10/28/22 11:34, Vincent Guittot wrote: > > > > > > > > Task can set its latency priority with sched_setattr(), which is then used > > > > > > > > to set the latency offset of its sched_enity, but sched group entities > > > > > > > > still have the default latency offset value. > > > > > > > > > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > > > > > > > priority of the group similarly to sched_setattr(). The latency priority > > > > > > > > is then used to set the offset of the sched_entities of the group. > > > > > > > > > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > > > > > > --- > > > > > > > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > > > > > > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > > > > > > > kernel/sched/fair.c | 33 ++++++++++++++++ > > > > > > > > kernel/sched/sched.h | 4 ++ > > > > > > > > 4 files changed, 97 insertions(+) > > > > > > > > > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > > > > > > > index be4a77baf784..d8ae7e411f9c 100644 > > > > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst > > > > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > > > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > > > > > > > values similar to the sched_setattr(2). This maximum utilization > > > > > > > > value is used to clamp the task specific maximum utilization clamp. > > > > > > > > > > > > > > > > + cpu.latency.nice > > > > > > > > + A read-write single value file which exists on non-root > > > > > > > > + cgroups. The default is "0". > > > > > > > > + > > > > > > > > + The nice value is in the range [-20, 19]. > > > > > > > > + > > > > > > > > + This interface file allows reading and setting latency using the > > > > > > > > + same values used by sched_setattr(2). > > > > > > > > > > > > > > I'm still not sure about this [1]. > > > > > > > > > > > > I'm still not sure about what you are trying to say here ... > > > > > > > > > > > > This is about setting a latency nice prio to a group level. > > > > > > > > > > > > > > > > > > > > In some scenarios we'd like to get the effective latency_nice of the task. How > > > > > > > will the task inherit the cgroup value or be impacted by it? > > > > > > > > > > > > > > For example if there are tasks that belong to a latency sensitive cgroup; and > > > > > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how > > > > > > > would I extract this info in EAS path given these tasks are using default > > > > > > > latency_nice value? And if should happen if their latency_nice is set to > > > > > > > something else other than default? > > > > > > > > > > > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/ > > > > > > > > > > > > Hmm so you are speaking about something that is not part of the patch. > > > > > > Let focus on the patchset for now > > > > > > > > > > I am focusing on this patchset. Isn't this an essential part of the design? > > > > > Once the interface is out we can't change it. As it stands, I can't see how it > > > > > > > > So, are you speaking about the interface i.e. setting a value between [-20:19] > > > > > > About how the cgroup and per task interface interact. > > > > > > How to get the effective value of latency_nice for a task that belongs to > > > a hierarchy? > > > > At the common parents level of the 2 entities that you want to compare > > and root level if there no other entity to compare with > > > > > > > > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that > > > has tg->cpu.latency.nice = -19 > > > > according to what i said above, latency_nice = 20 inside the group and > > -19 when comparing at the parent level > > > > > > > > And I want to use this interface in EAS; how should I interpret these values? > > > How should I walk up the hierarchy and decide the _effective_ latency_nice > > > value? > > > > The current use of latency_nice doesn't need to walk the hierarchy > > because it applies at each scheduling level so the childs > > automatically follow parents' latency. > > Not really, I don't see how that will work that way in the wake up path. The > wake up path (EAS in particular) does not walk through CPU controller group > hierarchy from top level, it only cares about cpuset/affinities and the > "effective" values of tasks. I was explaining the current use of latency_ni i.e. in this patchset, I'm not speaking about what should be done for other case like EAS In fact, it's exactly what I explained few lines above : "> > At the common parents level of the 2 entities that you want to compare > > and root level if there no other entity to compare with" > > So when you wake up a task, how will you retrieve the attribute for 'prefer > idle' in the wakeup path using this patchset? The only way is to aggregate > the CGroup hierarchy information to get a per-task effective value; say using > a min function. > > If you see uclamp_rq_util_with(), that also is using doing uclamp > aggregation similarly. > > So I think Qais is asking about the aggregation function in the EAS wakeup > path. > > Thanks.
On Fri, Nov 4, 2022 at 3:03 PM Vincent Guittot <vincent.guittot@linaro.org> wrote: [...] > > > > > > On 11/03/22 09:46, Vincent Guittot wrote: > > > > > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > > > > > > > > > On 10/28/22 11:34, Vincent Guittot wrote: > > > > > > > > > Task can set its latency priority with sched_setattr(), which is then used > > > > > > > > > to set the latency offset of its sched_enity, but sched group entities > > > > > > > > > still have the default latency offset value. > > > > > > > > > > > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > > > > > > > > priority of the group similarly to sched_setattr(). The latency priority > > > > > > > > > is then used to set the offset of the sched_entities of the group. > > > > > > > > > > > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > > > > > > > --- > > > > > > > > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > > > > > > > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > > > > > > > > kernel/sched/fair.c | 33 ++++++++++++++++ > > > > > > > > > kernel/sched/sched.h | 4 ++ > > > > > > > > > 4 files changed, 97 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > > > > > > > > index be4a77baf784..d8ae7e411f9c 100644 > > > > > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst > > > > > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > > > > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > > > > > > > > values similar to the sched_setattr(2). This maximum utilization > > > > > > > > > value is used to clamp the task specific maximum utilization clamp. > > > > > > > > > > > > > > > > > > + cpu.latency.nice [...] > > > > > > > > > > > > > > Hmm so you are speaking about something that is not part of the patch. > > > > > > > Let focus on the patchset for now > > > > > > > > > > > > I am focusing on this patchset. Isn't this an essential part of the design? > > > > > > Once the interface is out we can't change it. As it stands, I can't see how it > > > > > > > > > > So, are you speaking about the interface i.e. setting a value between [-20:19] > > > > > > > > About how the cgroup and per task interface interact. > > > > > > > > How to get the effective value of latency_nice for a task that belongs to > > > > a hierarchy? > > > > > > At the common parents level of the 2 entities that you want to compare > > > and root level if there no other entity to compare with > > > > > > > > > > > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that > > > > has tg->cpu.latency.nice = -19 > > > > > > according to what i said above, latency_nice = 20 inside the group and > > > -19 when comparing at the parent level > > > > > > > > > > > And I want to use this interface in EAS; how should I interpret these values? > > > > How should I walk up the hierarchy and decide the _effective_ latency_nice > > > > value? > > > > > > The current use of latency_nice doesn't need to walk the hierarchy > > > because it applies at each scheduling level so the childs > > > automatically follow parents' latency. > > > > Not really, I don't see how that will work that way in the wake up path. The > > wake up path (EAS in particular) does not walk through CPU controller group > > hierarchy from top level, it only cares about cpuset/affinities and the > > "effective" values of tasks. > > I was explaining the current use of latency_ni i.e. in this patchset, > I'm not speaking about what should be done for other case like EAS That's fine, but you did mention the negative value of latency_nice used to mark that a task prefers idle CPU so we should finish that discussion :-D. Since that will be one of the potential users of this patchset. > In fact, it's exactly what I explained few lines above : > "> > At the common parents level of the 2 entities that you want to compare > > > and root level if there no other entity to compare with" Yes this is a different usecase, but having more real world use cases will add more purpose to the patchset. I also want to add -- for ChromeOS, Youssef tried it and the temporary boost that latency_nice gives is not enough for latency-sensitive workloads (like the main thread of a ChromeOS web page which is both CPU bound and handles latency-sensitive user input). So we are also exploring other ways. However, I have no issue with the patchset if it helps Android and would love to review further. Thanks.
On Fri, 4 Nov 2022 at 16:12, Joel Fernandes <joel@joelfernandes.org> wrote: > > On Fri, Nov 4, 2022 at 3:03 PM Vincent Guittot > <vincent.guittot@linaro.org> wrote: > [...] > > > > > > > On 11/03/22 09:46, Vincent Guittot wrote: > > > > > > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > > > > > > > > > > > On 10/28/22 11:34, Vincent Guittot wrote: > > > > > > > > > > Task can set its latency priority with sched_setattr(), which is then used > > > > > > > > > > to set the latency offset of its sched_enity, but sched group entities > > > > > > > > > > still have the default latency offset value. > > > > > > > > > > > > > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency > > > > > > > > > > priority of the group similarly to sched_setattr(). The latency priority > > > > > > > > > > is then used to set the offset of the sched_entities of the group. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > > > > > > > > --- > > > > > > > > > > Documentation/admin-guide/cgroup-v2.rst | 8 ++++ > > > > > > > > > > kernel/sched/core.c | 52 +++++++++++++++++++++++++ > > > > > > > > > > kernel/sched/fair.c | 33 ++++++++++++++++ > > > > > > > > > > kernel/sched/sched.h | 4 ++ > > > > > > > > > > 4 files changed, 97 insertions(+) > > > > > > > > > > > > > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > > > > > > > > > > index be4a77baf784..d8ae7e411f9c 100644 > > > > > > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst > > > > > > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst > > > > > > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds. > > > > > > > > > > values similar to the sched_setattr(2). This maximum utilization > > > > > > > > > > value is used to clamp the task specific maximum utilization clamp. > > > > > > > > > > > > > > > > > > > > + cpu.latency.nice > [...] > > > > > > > > > > > > > > > > Hmm so you are speaking about something that is not part of the patch. > > > > > > > > Let focus on the patchset for now > > > > > > > > > > > > > > I am focusing on this patchset. Isn't this an essential part of the design? > > > > > > > Once the interface is out we can't change it. As it stands, I can't see how it > > > > > > > > > > > > So, are you speaking about the interface i.e. setting a value between [-20:19] > > > > > > > > > > About how the cgroup and per task interface interact. > > > > > > > > > > How to get the effective value of latency_nice for a task that belongs to > > > > > a hierarchy? > > > > > > > > At the common parents level of the 2 entities that you want to compare > > > > and root level if there no other entity to compare with > > > > > > > > > > > > > > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that > > > > > has tg->cpu.latency.nice = -19 > > > > > > > > according to what i said above, latency_nice = 20 inside the group and > > > > -19 when comparing at the parent level > > > > > > > > > > > > > > And I want to use this interface in EAS; how should I interpret these values? > > > > > How should I walk up the hierarchy and decide the _effective_ latency_nice > > > > > value? > > > > > > > > The current use of latency_nice doesn't need to walk the hierarchy > > > > because it applies at each scheduling level so the childs > > > > automatically follow parents' latency. > > > > > > Not really, I don't see how that will work that way in the wake up path. The > > > wake up path (EAS in particular) does not walk through CPU controller group > > > hierarchy from top level, it only cares about cpuset/affinities and the > > > "effective" values of tasks. > > > > I was explaining the current use of latency_ni i.e. in this patchset, > > I'm not speaking about what should be done for other case like EAS > > That's fine, but you did mention the negative value of latency_nice > used to mark that a task prefers idle CPU so we should finish that > discussion :-D. Since that will be one of the potential users of this > patchset. We should finish reviewing this patchset 1st. Then, we can discuss if we should use -19, <0 or something else. This doesn't add any value to this patchset IMO > > > In fact, it's exactly what I explained few lines above : > > "> > At the common parents level of the 2 entities that you want to compare > > > > and root level if there no other entity to compare with" > > Yes this is a different usecase, but having more real world use cases > will add more purpose to the patchset. As long as the policy is defined and I think it is defined, we are fine and it's not the purpose of discussing implementation details of potential other use. Once this patchset is merged, I will be more than happy to prepare another one to make use of latency_nice in EAS and we can discuss further based on it. > > I also want to add -- for ChromeOS, Youssef tried it and the temporary > boost that latency_nice gives is not enough for latency-sensitive > workloads (like the main thread of a ChromeOS web page which is both > CPU bound and handles latency-sensitive user input). So we are also > exploring other ways. However, I have no issue with the patchset if it > helps Android and would love to review further. > > Thanks.
On 11/04/22 14:57, Joel Fernandes wrote: > > The current use of latency_nice doesn't need to walk the hierarchy > > because it applies at each scheduling level so the childs > > automatically follow parents' latency. > > Not really, I don't see how that will work that way in the wake up path. The > wake up path (EAS in particular) does not walk through CPU controller group > hierarchy from top level, it only cares about cpuset/affinities and the > "effective" values of tasks. > > So when you wake up a task, how will you retrieve the attribute for 'prefer > idle' in the wakeup path using this patchset? The only way is to aggregate > the CGroup hierarchy information to get a per-task effective value; say using > a min function. > > If you see uclamp_rq_util_with(), that also is using doing uclamp > aggregation similarly. > > So I think Qais is asking about the aggregation function in the EAS wakeup > path. Yes that's what I was trying to say. Thanks for helping to clarify it! Thanks -- Qais Yousef
On 11/04/22 09:13, Joel Fernandes wrote: > > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup > > that has tg->cpu.latency.nice = -19 > > Just for the task placement signal, One way is to go through the se hierarchy > till the root and get the minimum. Then make that the effective value. So In > your example that would make it -19 so prefer idle = 1. We should need > a Boolean signal. Not pretty but not the end of the world imho. It is not hard to hack something. My worry is about consistency; and maintainers in the future saying that doesn't fit the current design. I'd love for this to be usable everywhere as-is. That requires the expectations for both users and consumers are being made clear from the beginning. What I was asking for is for the documentation to reflect this, and the implementation of this effective function being made available from the start. Cheers -- Qais Yousef
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst index be4a77baf784..d8ae7e411f9c 100644 --- a/Documentation/admin-guide/cgroup-v2.rst +++ b/Documentation/admin-guide/cgroup-v2.rst @@ -1095,6 +1095,14 @@ All time durations are in microseconds. values similar to the sched_setattr(2). This maximum utilization value is used to clamp the task specific maximum utilization clamp. + cpu.latency.nice + A read-write single value file which exists on non-root + cgroups. The default is "0". + + The nice value is in the range [-20, 19]. + + This interface file allows reading and setting latency using the + same values used by sched_setattr(2). Memory diff --git a/kernel/sched/core.c b/kernel/sched/core.c index caf54e54a74f..3f42b1f61a7e 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -10890,6 +10890,47 @@ static int cpu_idle_write_s64(struct cgroup_subsys_state *css, { return sched_group_set_idle(css_tg(css), idle); } + +static s64 cpu_latency_nice_read_s64(struct cgroup_subsys_state *css, + struct cftype *cft) +{ + int prio, delta, last_delta = INT_MAX; + s64 weight; + + weight = css_tg(css)->latency_offset * NICE_LATENCY_WEIGHT_MAX; + weight = div_s64(weight, get_sched_latency(false)); + + /* Find the closest nice value to the current weight */ + for (prio = 0; prio < ARRAY_SIZE(sched_latency_to_weight); prio++) { + delta = abs(sched_latency_to_weight[prio] - weight); + if (delta >= last_delta) + break; + last_delta = delta; + } + + return LATENCY_TO_NICE(prio-1); +} + +static int cpu_latency_nice_write_s64(struct cgroup_subsys_state *css, + struct cftype *cft, s64 nice) +{ + s64 latency_offset; + long weight; + int idx; + + if (nice < MIN_LATENCY_NICE || nice > MAX_LATENCY_NICE) + return -ERANGE; + + idx = NICE_TO_LATENCY(nice); + idx = array_index_nospec(idx, LATENCY_NICE_WIDTH); + weight = sched_latency_to_weight[idx]; + + latency_offset = weight * get_sched_latency(false); + latency_offset = div_s64(latency_offset, NICE_LATENCY_WEIGHT_MAX); + + return sched_group_set_latency(css_tg(css), latency_offset); +} + #endif static struct cftype cpu_legacy_files[] = { @@ -10904,6 +10945,11 @@ static struct cftype cpu_legacy_files[] = { .read_s64 = cpu_idle_read_s64, .write_s64 = cpu_idle_write_s64, }, + { + .name = "latency.nice", + .read_s64 = cpu_latency_nice_read_s64, + .write_s64 = cpu_latency_nice_write_s64, + }, #endif #ifdef CONFIG_CFS_BANDWIDTH { @@ -11121,6 +11167,12 @@ static struct cftype cpu_files[] = { .read_s64 = cpu_idle_read_s64, .write_s64 = cpu_idle_write_s64, }, + { + .name = "latency.nice", + .flags = CFTYPE_NOT_ON_ROOT, + .read_s64 = cpu_latency_nice_read_s64, + .write_s64 = cpu_latency_nice_write_s64, + }, #endif #ifdef CONFIG_CFS_BANDWIDTH { diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 4299d5108dc7..9583936ce30c 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -11764,6 +11764,7 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent) goto err; tg->shares = NICE_0_LOAD; + tg->latency_offset = 0; init_cfs_bandwidth(tg_cfs_bandwidth(tg)); @@ -11862,6 +11863,9 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq, } se->my_q = cfs_rq; + + se->latency_offset = tg->latency_offset; + /* guarantee group entities always have weight */ update_load_set(&se->load, NICE_0_LOAD); se->parent = parent; @@ -11992,6 +11996,35 @@ int sched_group_set_idle(struct task_group *tg, long idle) return 0; } +int sched_group_set_latency(struct task_group *tg, s64 latency) +{ + int i; + + if (tg == &root_task_group) + return -EINVAL; + + if (abs(latency) > sysctl_sched_latency) + return -EINVAL; + + mutex_lock(&shares_mutex); + + if (tg->latency_offset == latency) { + mutex_unlock(&shares_mutex); + return 0; + } + + tg->latency_offset = latency; + + for_each_possible_cpu(i) { + struct sched_entity *se = tg->se[i]; + + WRITE_ONCE(se->latency_offset, latency); + } + + mutex_unlock(&shares_mutex); + return 0; +} + #else /* CONFIG_FAIR_GROUP_SCHED */ void free_fair_sched_group(struct task_group *tg) { } diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index 99f10b4dc230..95d4be4f3af6 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -407,6 +407,8 @@ struct task_group { /* A positive value indicates that this is a SCHED_IDLE group. */ int idle; + /* latency constraint of the group. */ + int latency_offset; #ifdef CONFIG_SMP /* @@ -517,6 +519,8 @@ extern int sched_group_set_shares(struct task_group *tg, unsigned long shares); extern int sched_group_set_idle(struct task_group *tg, long idle); +extern int sched_group_set_latency(struct task_group *tg, s64 latency); + #ifdef CONFIG_SMP extern void set_task_rq_fair(struct sched_entity *se, struct cfs_rq *prev, struct cfs_rq *next);