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 X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A51AFC433ED for ; Wed, 21 Apr 2021 10:52:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6CE8E6144D for ; Wed, 21 Apr 2021 10:52:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237380AbhDUKxQ convert rfc822-to-8bit (ORCPT ); Wed, 21 Apr 2021 06:53:16 -0400 Received: from foss.arm.com ([217.140.110.172]:59200 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234632AbhDUKxN (ORCPT ); Wed, 21 Apr 2021 06:53:13 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 448F7113E; Wed, 21 Apr 2021 03:52:40 -0700 (PDT) Received: from e113632-lin (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CCD903F73B; Wed, 21 Apr 2021 03:52:38 -0700 (PDT) From: Valentin Schneider To: Vincent Guittot Cc: linux-kernel@vger.kernel.org, Peter Zijlstra , Ingo Molnar , Dietmar Eggemann , Morten Rasmussen , Qais Yousef , Quentin Perret , Pavan Kondeti , Rik van Riel , Lingutla Chandrasekhar Subject: Re: [PATCH 2/2] sched/fair: Relax task_hot() for misfit tasks In-Reply-To: References: <20210415175846.494385-1-valentin.schneider@arm.com> <20210415175846.494385-3-valentin.schneider@arm.com> <20210416135113.GA16445@vingu-book> <87blaakxji.mognet@arm.com> Date: Wed, 21 Apr 2021 11:52:36 +0100 Message-ID: <878s5bvrij.mognet@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 20/04/21 16:33, Vincent Guittot wrote: > On Mon, 19 Apr 2021 at 19:13, Valentin Schneider > wrote: >> >> On 16/04/21 15:51, Vincent Guittot wrote: >> > Le jeudi 15 avril 2021 � 18:58:46 (+0100), Valentin Schneider a �crit : >> >> + >> >> +/* >> >> + * What does migrating this task do to our capacity-aware scheduling criterion? >> >> + * >> >> + * Returns 1, if the task needs more capacity than the dst CPU can provide. >> >> + * Returns 0, if the task needs the extra capacity provided by the dst CPU >> >> + * Returns -1, if the task isn't impacted by the migration wrt capacity. >> >> + */ >> >> +static int migrate_degrades_capacity(struct task_struct *p, struct lb_env *env) >> >> +{ >> >> + if (!(env->sd->flags & SD_ASYM_CPUCAPACITY)) >> >> + return -1; >> >> + >> >> + if (!task_fits_capacity(p, capacity_of(env->src_cpu))) { >> >> + if (cpu_capacity_greater(env->dst_cpu, env->src_cpu)) >> >> + return 0; >> >> + else if (cpu_capacity_greater(env->src_cpu, env->dst_cpu)) >> >> + return 1; >> >> + else >> >> + return -1; >> >> + } >> > >> > Being there means that task fits src_cpu capacity so why testing p against dst_cpu ? >> > >> >> Because if p fits on src_cpu, we don't want to move it to a dst_cpu on >> which it *doesn't* fit. > > OK. I was confused because I thought that this was only to force > migration in case of group_misfit_task but you tried to extend to > other cases... I'm not convinced that you succeeded to cover all cases > > Also I found this function which returns 3 values a bit disturbing. > IIUC you tried to align to migrate_degrades_capacity but you should > have better aligned to task_hot and return only 0 or 1. -1 is not used > Ack, will do. >> >> @@ -7672,6 +7698,15 @@ int can_migrate_task(struct task_struct *p, struct lb_env *env) >> >> if (tsk_cache_hot == -1) >> >> tsk_cache_hot = task_hot(p, env); >> >> >> >> + /* >> >> + * On a (sane) asymmetric CPU capacity system, the increase in compute >> >> + * capacity should offset any potential performance hit caused by a >> >> + * migration. >> >> + */ >> >> + if ((env->dst_grp_type == group_has_spare) && >> > >> > Shouldn't it be env->src_grp_type == group_misfit_task to only care of misfit task case as >> > stated in $subject >> > >> >> Previously this was env->idle != CPU_NOT_IDLE, but I figured dst_grp_type >> could give us a better picture. Staring at this some more, this isn't so >> true when the group size goes up - there's no guarantees the dst_cpu is the >> one that has spare cycles, and the other CPUs might not be able to grant >> the capacity uplift dst_cpu can. > > yeah you have to keep checking for env->idle != CPU_NOT_IDLE > >> >> As for not using src_grp_type == group_misfit_task, this is pretty much the >> same as [1]. CPU-bound (misfit) task + some other task on the same rq >> implies group_overloaded classification when balancing at MC level (no SMT, >> so one group per CPU). > > Is it something that happens often or just a sporadic/transient state > ? I mean does it really worth the extra complexity and do you see > performance improvement ? > "Unfortunately" yes, this is a relatively common scenario when running "1 big task per CPU" types of workloads. The expected behaviour for big.LITTLE systems is to upmigrate tasks stuck on the LITTLE CPUs as soon as a big CPU becomes free, usually via newidle balance (which, since they process work faster than the LITTLEs, is bound to happen), and an extra task being enqueued at "the wrong time" can prevent this from happening. This usually means a misfit task can take a few dozen extra ms than it should to be migrated - in the tests I run (which are pretty much this 1 hog per CPU workload) this happens about ~20% of the time. > You should better focus on fixing the simple case of group_misfit_task > task. This other cases looks far more complex with lot of corner cases > >> >> [1]: http://lore.kernel.org/r/jhjblcuv2mo.mognet@arm.com