From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B6235C433F5 for ; Wed, 23 Feb 2022 15:48:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242443AbiBWPst (ORCPT ); Wed, 23 Feb 2022 10:48:49 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38158 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238190AbiBWPsr (ORCPT ); Wed, 23 Feb 2022 10:48:47 -0500 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 7088E6C907 for ; Wed, 23 Feb 2022 07:48:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1645631295; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=yme7e5g8TGrkrXhF+dr6q/ZQouPgxuhcTIm1Orj8V9k=; b=PVYbLDJVt0Dc8Vn61aiqWx3rkVoJn0qhW2Jx0QiaiUTa6/5xlImm5kH4hMpALgq+XC6byo aqBcCtOl1x4VTW4jkyAEKEyxwnW2sZA7PtgfU8goQZFKitEmBTZ4f92oMxnv40/tLofDrA UsIbPz5DzEkyBkrFXIECZfrb1iv5za4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-255-gPQdpIRbMcWDgTVZX0rpcw-1; Wed, 23 Feb 2022 10:48:11 -0500 X-MC-Unique: gPQdpIRbMcWDgTVZX0rpcw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 762191854E2A; Wed, 23 Feb 2022 15:48:09 +0000 (UTC) Received: from lorien.usersys.redhat.com (unknown [10.22.10.120]) by smtp.corp.redhat.com (Postfix) with ESMTPS id F3DB71077D44; Wed, 23 Feb 2022 15:47:39 +0000 (UTC) Date: Wed, 23 Feb 2022 10:47:38 -0500 From: Phil Auld To: Carlos Bilbao Cc: Peter Zijlstra , juri.lelli@redhat.com, vincent.guittot@linaro.org, mingo@redhat.com, dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de, bristot@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] kernel/sched: Update schedstats when migrating threads Message-ID: References: <20220126152222.5429-1-carlos.bilbao@amd.com> <0e42c46a-ccc4-e793-00b8-ae407e06846f@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0e42c46a-ccc4-e793-00b8-ae407e06846f@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 23, 2022 at 09:33:59AM -0600 Carlos Bilbao wrote: > On 2/23/2022 9:28 AM, Phil Auld wrote: > > On Wed, Feb 23, 2022 at 09:14:45AM -0600 Carlos Bilbao wrote: > >> On 2/23/2022 4:19 AM, Peter Zijlstra wrote: > >>> On Wed, Jan 26, 2022 at 09:22:23AM -0600, Carlos Bilbao wrote: > >>>> The kernel manages per-task scheduler statistics or schedstats. Such > >>>> counters should be reinitialized when the thread is migrated to a > >>>> different core rq, except for the values recording number of migrations. > >>> > >>> I'm confused, why should we reset schedstats on migrate? I'm thinking > >>> this breaks per-task, since tasks tend to bounce around quite a lot. > >>> > >> > >> Thanks for your comments, Peter. > >> > >> Looking at the documentation of schedstats I see that most values are > >> actually linked to the particular CPU: time spent on the cpu, timeslices > >> run on this cpu, number of times _something_ was called when the cpu was > >> idle, and so forth. Those values lose their meaning after migration and we > >> should reinitialize their counters. However, reviewing sched_statistics I > >> identify two fields that we should definitely keep increasing even after > >> migration (nr_migrations_cold, nr_forced_migrations). > >> > > > > The documentation is a little off. I think it should say "any cpu" instead > > of "this cpu". If you reset these per task counters (time on cpu, number > > of timeslices etc) on every migration then they become meaningless (and > > useless). > > > > > > Cheers, > > Phil > > > > Well that clarifies it! Then, let me ask the opposite question... What > fields of schedstats should we clear when migrating? If there isn't any, > I will just increase the number of migrations. > I don't think any should be cleared on migration. They're per task and should be monotically increasing. If they ever reset it becomes hard to know what they mean when you read them. Cheers, Phil > >> So this patch will have to be upgraded if there's some other value(s) in > >> schedstats that we do not want to reinitialize either. > >> > >>>> Signed-off-by: Carlos Bilbao > >>>> --- > >>>> kernel/sched/core.c | 10 +++++++++- > >>>> 1 file changed, 9 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c > >>>> index fe53e510e711..d64c2a290176 100644 > >>>> --- a/kernel/sched/core.c > >>>> +++ b/kernel/sched/core.c > >>>> @@ -8757,6 +8757,7 @@ bool sched_smp_initialized __read_mostly; > >>>> int migrate_task_to(struct task_struct *p, int target_cpu) > >>>> { > >>>> struct migration_arg arg = { p, target_cpu }; > >>>> + uint64_t forced_migrations, migrations_cold; > >>>> int curr_cpu = task_cpu(p); > >>>> > >>>> if (curr_cpu == target_cpu) > >>>> @@ -8765,7 +8766,14 @@ int migrate_task_to(struct task_struct *p, int target_cpu) > >>>> if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) > >>>> return -EINVAL; > >>>> > >>>> - /* TODO: This is not properly updating schedstats */ > >>>> + if (schedstat_enabled()) { > >>>> + forced_migrations = schedstat_val(p->stats.nr_forced_migrations); > >>>> + migrations_cold = schedstat_val(p->stats.nr_migrations_cold); > >>>> + memset(&p->stats, 0, sizeof(p->stats)); > >>>> + schedstat_set(p->stats.nr_forced_migrations, forced_migrations); > >>>> + schedstat_set(p->stats.nr_migrations_cold, migrations_cold); > >>>> + schedstat_inc(p->stats.nr_migrations_cold); > >>>> + } > >>>> > >>>> trace_sched_move_numa(p, curr_cpu, target_cpu); > >>>> return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); > >>>> -- > >>>> 2.27.0 > >>>> > >> > >> Thanks, > >> Carlos > >> > > > > Thanks, > Carlos > --