Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 18 Jan 2022 12:32:48 -0500
From:   Daniel Jordan <daniel.m.jordan@oracle.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Alexander Duyck <alexanderduyck@fb.com>,
        Alex Williamson <alex.williamson@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Ben Segall <bsegall@google.com>,
        Cornelia Huck <cohuck@redhat.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Ingo Molnar <mingo@redhat.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Josh Triplett <josh@joshtriplett.org>,
        Michal Hocko <mhocko@suse.com>, Nico Pache <npache@redhat.com>,
        Pasha Tatashin <pasha.tatashin@soleen.com>,
        Steffen Klassert <steffen.klassert@secunet.com>,
        Steve Sistare <steven.sistare@oracle.com>,
        Tejun Heo <tj@kernel.org>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        linux-mm@kvack.org, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org
Subject: Re: [RFC 15/16] sched/fair: Account kthread runtime debt for CFS
 bandwidth
Message-ID: <20220118173248.amqd3qwyuqc33egk@oracle.com>
References: <20220106004656.126790-1-daniel.m.jordan@oracle.com>
 <20220106004656.126790-16-daniel.m.jordan@oracle.com>
 <YeFDC0mV3yurUFbl@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YeFDC0mV3yurUFbl@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?LuSWc2/GFn3h6tZFXdHHTNiuBkGOcNfC3TYwV3r2gfllAA8Gz0vJzx45tpRk?=
 =?us-ascii?Q?dYiM0QAoG9AuM88AvirnMDhJvH8aldDB8dxefRIXC3uYfroZTvwk3r2S4hEJ?=
 =?us-ascii?Q?X4ANBRyCc9WihYXoOiGVFyrPf/fqoXYt7SZ1YwFzXbDRjQzTtpO9rrv3KdEB?=
 =?us-ascii?Q?+EPO5nt8Ta6orekZegc0nJYKMpr40aBTDTq6JgAM581PFhgAQ/xG1pszrKsL?=
 =?us-ascii?Q?o9EoQ6ZTvX1IsTWCv1LLpzaj28cCzp0SZpugFPnwt6oqcKCo1Hd1dEV2xY4y?=
 =?us-ascii?Q?2aU+/R2Hz684vTDqEDaLSW6bJ0ZYnnnd0uktbYl7+XiquPm6yGhwpEz6NKFD?=
 =?us-ascii?Q?vrFt0HR4H4pYtVQrZQiV7TwmVwUsW7i3IuGK0uULVwCmwFpcIPveh6X9mJl9?=
 =?us-ascii?Q?MAr0nZ9O0if4Fc5xoC/8giSZgaxKFaXHRnkkX5zwLF4hAxAc6GyKzt33BynL?=
 =?us-ascii?Q?9WtSBP4uXFp7UjN4yjgjLFULVf4odUWhLFOpcymAJPs7Tc0Hjm9EzeHXZPsR?=
 =?us-ascii?Q?CnDgz6yqSfl3XV2AZ5Yaz4GCTYEk3NDDIC0boOt3sRpA1FrVsU8vPpHv1K/L?=
 =?us-ascii?Q?0j/vhycUuJBG/e0vdhG5hpXDc1I7X6/uQpYzONLFK5RtyyCqHXzwD2OEfreS?=
 =?us-ascii?Q?vjC2iah2qGUdPkBPos6UUV0OvUQaUyLjNvX2w3caiLtByMIzkVPSiUcVM2vM?=
 =?us-ascii?Q?OhArxnxilcmNqsB2Z6VlFe4AC9qulYm+A7bZYGBbTj19RufRXGstitChTrhZ?=
 =?us-ascii?Q?FgfZvkIiJpIOuabJjn12TR30bsjUgtDWUASYcLcQAHsR6jhExsBs51ejjOLl?=
 =?us-ascii?Q?Iwe1dM/5PIfknSUE1asmuWMEisZTe8a++UiM5C2wcyhmiKHDAqddELAjzRdp?=
 =?us-ascii?Q?LkVRBiCRLSkYRT60cLQN+l7IvaSxU5TrgwI+5gmxhfUa9yj3p+67hlcWt4TL?=
 =?us-ascii?Q?8QP143lX1ZDHe3srvcyj7jUwvPgGOYzwYcFstmfhn7l/e1sp0NR/5K9sX2qh?=
 =?us-ascii?Q?Xn74qbLGmkWPtU14cW3qkD3r1G3AGUdnfnIIJPkblyQ4QJabjBI2/aom/mzn?=
 =?us-ascii?Q?VYMCD5aRkmqUX8V+WR7Qge4nNlkIx885QVhh+XPZvCF3fAJCcowi9lGsibqs?=
 =?us-ascii?Q?iWaROHFyyvn5hSaY8R/seoNirEu1+nw6TN2/Q5Czzv1sLoCTJU3lTzrVncbr?=
 =?us-ascii?Q?nh6+OA851ss6CUOO4LTdIeIVXme9Dh6yMcvpY1P2f/EmpWgm2QgCTlfjNkyZ?=
 =?us-ascii?Q?tjfs9zQckah23PcoPyasFxeHa33sX3p4UrAuQSu/Zsh2NEL6MH8OqpBYciVh?=
 =?us-ascii?Q?qiiuIMlM/c8dvWmhCKdlXShRleJN0nLcQN8HpdUZHY2Ys1tUtAkWOBmcZCJJ?=
 =?us-ascii?Q?zLDnMqfbyKMN2M6htQNoTTBS6ixKVICifOqwTABK7rGqZjr9SCXiuCgBKGOt?=
 =?us-ascii?Q?4QSkhyYF36WCUcyEn0Pfb9sDp4GBSZlFiC9fLQwDsHwdTRyDGAR5W+5r07qQ?=
 =?us-ascii?Q?tFEMcKSAwnO/4zbVKrpcMrQmVzhmOAihjTCTxcm+aNRIr+cTUOXPh+iC5rRN?=
 =?us-ascii?Q?if5disxZl+ze63R4DexeR8c6LaNPmojT4q9htcfggZ/HLJyjquu3vxeEHXLb?=
 =?us-ascii?Q?+VS0K9kcqPQ7o+Fk/OSOQIlsaFoSLYwH+kyuZlR/c3h6zqUWBnKbXWrnLgj+?=
 =?us-ascii?Q?cXfvfQ=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9415b92e-334a-48c8-a7f4-08d9daa88e81
X-MS-Exchange-CrossTenant-AuthSource: SA1PR10MB5711.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jan 2022 17:32:53.4076
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: c5QQpR2WxBJ/+RU5EcjtJaky8cC7mpPlNIQnjTCdEbZUkj62PK4H2128stnOLoiRhcxSS9KkY2u9TX1G5OUCkEVtPACr7fHETwaW3KoB1IQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB3737
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10231 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 bulkscore=0
 malwarescore=0 adultscore=0 phishscore=0 mlxscore=0 spamscore=0
 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2110150000 definitions=main-2201180105
X-Proofpoint-GUID: PYFz-HiM05rExjYRUWUzg3U34vB4yhCj
X-Proofpoint-ORIG-GUID: PYFz-HiM05rExjYRUWUzg3U34vB4yhCj
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On Fri, Jan 14, 2022 at 10:31:55AM +0100, Peter Zijlstra wrote:
> On Wed, Jan 05, 2022 at 07:46:55PM -0500, Daniel Jordan wrote:
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 44c452072a1b..3c2d7f245c68 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4655,10 +4655,19 @@ static inline u64 sched_cfs_bandwidth_slice(void)
> >   */
> >  void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
> >  {
> > -	if (unlikely(cfs_b->quota == RUNTIME_INF))
> > +	u64 quota = cfs_b->quota;
> > +	u64 payment;
> > +
> > +	if (unlikely(quota == RUNTIME_INF))
> >  		return;
> >  
> > -	cfs_b->runtime += cfs_b->quota;
> > +	if (cfs_b->debt) {
> > +		payment = min(quota, cfs_b->debt);
> > +		cfs_b->debt -= payment;
> > +		quota -= payment;
> > +	}
> > +
> > +	cfs_b->runtime += quota;
> >  	cfs_b->runtime = min(cfs_b->runtime, cfs_b->quota + cfs_b->burst);
> >  }
> 
> It might be easier to make cfs_bandwidth::runtime an s64 and make it go
> negative.

Yep, nice, no need for a new field in cfs_bandwidth.

> > @@ -5406,6 +5415,32 @@ static void __maybe_unused unthrottle_offline_cfs_rqs(struct rq *rq)
> >  	rcu_read_unlock();
> >  }
> >  
> > +static void incur_cfs_debt(struct rq *rq, struct sched_entity *se,
> > +			   struct task_group *tg, u64 debt)
> > +{
> > +	if (!cfs_bandwidth_used())
> > +		return;
> > +
> > +	while (tg != &root_task_group) {
> > +		struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)];
> > +
> > +		if (cfs_rq->runtime_enabled) {
> > +			struct cfs_bandwidth *cfs_b = &tg->cfs_bandwidth;
> > +			u64 payment;
> > +
> > +			raw_spin_lock(&cfs_b->lock);
> > +
> > +			payment = min(cfs_b->runtime, debt);
> > +			cfs_b->runtime -= payment;
> 
> At this point it might hit 0 (or go negative if/when you do the above)
> and you'll need to throttle the group.

I might not be following you, but there could be cfs_rq's with local
runtime_remaining, so even if it goes 0 or negative, the group might
still have quota left and so shouldn't be throttled right away.

I was thinking the throttling would happen as normal, when a cfs_rq ran
out of runtime_remaining and failed to refill it from
cfs_bandwidth::runtime.

> > +			cfs_b->debt += debt - payment;
> > +
> > +			raw_spin_unlock(&cfs_b->lock);
> > +		}
> > +
> > +		tg = tg->parent;
> > +	}
> > +}
> 
> So part of the problem I have with this is that these external things
> can consume all the bandwidth and basically indefinitely starve the
> group.
> 
> This is doulby so if you're going to account things like softirq network
> processing.

Yes.  As Tejun points out, I'll make sure remote charging doesn't run
away.

> Also, why does the whole charging API have a task argument? It either is
> current or NULL in case of things like softirq, neither really make
> sense as an argument.

@task distinguishes between NULL for softirq and current for everybody
else.

It's possible to detect this difference internally though, if that's
what you're saying, so @task can go away.

> Also, by virtue of this being a start-stop annotation interface, the
> accrued time might be arbitrarily large and arbitrarily delayed. I'm not
> sure that's sensible.

Yes, that is a risk.  With start-stop, users need to be careful to
account often enough and have a "reasonable" upper bound on period
length, however that's defined.  Multithreaded jobs are probably the
worst offender since these threads charge a sizable amount at once
compared to the other use cases.

> For tasks it might be better to mark the task and have the tick DTRT
> instead of later trying to 'migrate' the time.

Ok, I'll try that.  The start-stop approach keeps remote charging from
adding overhead in the tick for non-remote-charging things, far and away
the common case, but I'll see how expensive the tick-based approach is.

Can hide it behind a static branch for systems not using the cpu
contoller.