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=-15.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, 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 6EBCBC433FE for ; Fri, 17 Sep 2021 15:25:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 570E661212 for ; Fri, 17 Sep 2021 15:25:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233015AbhIQP0i (ORCPT ); Fri, 17 Sep 2021 11:26:38 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49538 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229663AbhIQP0g (ORCPT ); Fri, 17 Sep 2021 11:26:36 -0400 Received: from mail-qk1-x734.google.com (mail-qk1-x734.google.com [IPv6:2607:f8b0:4864:20::734]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EFC00C061574 for ; Fri, 17 Sep 2021 08:25:13 -0700 (PDT) Received: by mail-qk1-x734.google.com with SMTP id bk29so18521786qkb.8 for ; Fri, 17 Sep 2021 08:25:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Th9Dectw0xwMPChqk9pLJzZcuXtG5lCvKTCrxYFTnPc=; b=Zh3W3Da0kbNcul9/QjTY8/GOdUPlHVJfwPwUB1inXpRaRf2uVc8uJcpZ7xnSgztj+s kcTxi0c+md7rzAoqgWmFCsaR/KQMsljf2yFeKfWLIAY7dxQmo4Nev5uc7/YbmTlsN7x0 oesDVqfNzM0vERyFjSwyxWaaiCnpb5ca3IOIvGp3ylUV79SflMZgIcvpEk8UCUTvI5HO XkfN1812wdixqPtIrJ5d62Ha04GecIyaId6y5wBdb1sIe+Wx+zLYdWh37aFHbXkpqMhW ADGYCo8r2JpmPWR9wFsP+dLU7KklHTbAyBYqKJxTI1QYKpsdA/R81nqIuApohRMTNrLa Xttw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Th9Dectw0xwMPChqk9pLJzZcuXtG5lCvKTCrxYFTnPc=; b=N4hRyJZHsi3DlootHZETqulh5efvnXtLk/sOYDcbe6zyUmtd7qKnhK+E8p8HqksmBS hspI1qxeNM8UQj8MujuISFZkD4zgVavWqGlSk7c+GnmPSkO/G5UYY94wJWsZRTYOx/mF hjCVnySZxKDk+icitQpcUCNw8NIaC0fPfKxt/KGSbTTsaKKUL1RUr1J51bjp0egWnJIq vBqBpd/IF8GfE5FzFzUV0GGwBFiN8PcZBJK3itRhiNBeGV2OkAkyIi4XM2Cmu7UjwReg s6B5tpuxqAzTBBFaC1LY5/Mm18ydOj+8ZZqgO9eIaOWxoTTnXMRRZX/0Wq6z4UnPuvAG 6LGg== X-Gm-Message-State: AOAM531MimQa7ypxLILBVuJdHDRigMtaMhblTRRg9g++mx4570nHxPgc vcOeTwE343r3g/PX1EIzi0D9rqIdUf+VD4P1NEIb+Q== X-Google-Smtp-Source: ABdhPJzi+ecvVDZHy16eTUV4B931Trwh6EgK/voL8hMSSwIkuUjyW0X15/qP2d5cA0l+DPQ8R0OiJeElYN/SoNclYEU= X-Received: by 2002:a25:9001:: with SMTP id s1mr13884940ybl.191.1631892313004; Fri, 17 Sep 2021 08:25:13 -0700 (PDT) MIME-Version: 1.0 References: <20210911011819.12184-1-ricardo.neri-calderon@linux.intel.com> <20210911011819.12184-7-ricardo.neri-calderon@linux.intel.com> <20210917010044.GA23727@ranerica-svr.sc.intel.com> In-Reply-To: <20210917010044.GA23727@ranerica-svr.sc.intel.com> From: Vincent Guittot Date: Fri, 17 Sep 2021 17:25:02 +0200 Message-ID: Subject: Re: [PATCH v5 6/6] sched/fair: Consider SMT in ASYM_PACKING load balance To: Ricardo Neri Cc: "Peter Zijlstra (Intel)" , Ingo Molnar , Juri Lelli , Srikar Dronamraju , Nicholas Piggin , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Len Brown , Srinivas Pandruvada , Tim Chen , Aubrey Li , "Ravi V. Shankar" , Ricardo Neri , Quentin Perret , "Joel Fernandes (Google)" , linuxppc-dev@lists.ozlabs.org, linux-kernel , Aubrey Li , Daniel Bristot de Oliveira , "Rafael J . Wysocki" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 17 Sept 2021 at 03:01, Ricardo Neri wrote: > > On Wed, Sep 15, 2021 at 05:43:44PM +0200, Vincent Guittot wrote: > > On Sat, 11 Sept 2021 at 03:19, Ricardo Neri > > wrote: > > > > > > When deciding to pull tasks in ASYM_PACKING, it is necessary not only to > > > check for the idle state of the destination CPU, dst_cpu, but also of > > > its SMT siblings. > > > > > > If dst_cpu is idle but its SMT siblings are busy, performance suffers > > > if it pulls tasks from a medium priority CPU that does not have SMT > > > siblings. > > > > > > Implement asym_smt_can_pull_tasks() to inspect the state of the SMT > > > siblings of both dst_cpu and the CPUs in the candidate busiest group. > > > > > > Cc: Aubrey Li > > > Cc: Ben Segall > > > Cc: Daniel Bristot de Oliveira > > > Cc: Dietmar Eggemann > > > Cc: Mel Gorman > > > Cc: Quentin Perret > > > Cc: Rafael J. Wysocki > > > Cc: Srinivas Pandruvada > > > Cc: Steven Rostedt > > > Cc: Tim Chen > > > Reviewed-by: Joel Fernandes (Google) > > > Reviewed-by: Len Brown > > > Signed-off-by: Ricardo Neri > > > --- > > > Changes since v4: > > > * Use sg_lb_stats::sum_nr_running the idle state of a scheduling group. > > > (Vincent, Peter) > > > * Do not even idle CPUs in asym_smt_can_pull_tasks(). (Vincent) > > > * Updated function documentation and corrected a typo. > > > > > > Changes since v3: > > > * Removed the arch_asym_check_smt_siblings() hook. Discussions with the > > > powerpc folks showed that this patch should not impact them. Also, more > > > recent powerpc processor no longer use asym_packing. (PeterZ) > > > * Removed unnecessary local variable in asym_can_pull_tasks(). (Dietmar) > > > * Removed unnecessary check for local CPUs when the local group has zero > > > utilization. (Joel) > > > * Renamed asym_can_pull_tasks() as asym_smt_can_pull_tasks() to reflect > > > the fact that it deals with SMT cases. > > > * Made asym_smt_can_pull_tasks() return false for !CONFIG_SCHED_SMT so > > > that callers can deal with non-SMT cases. > > > > > > Changes since v2: > > > * Reworded the commit message to reflect updates in code. > > > * Corrected misrepresentation of dst_cpu as the CPU doing the load > > > balancing. (PeterZ) > > > * Removed call to arch_asym_check_smt_siblings() as it is now called in > > > sched_asym(). > > > > > > Changes since v1: > > > * Don't bailout in update_sd_pick_busiest() if dst_cpu cannot pull > > > tasks. Instead, reclassify the candidate busiest group, as it > > > may still be selected. (PeterZ) > > > * Avoid an expensive and unnecessary call to cpumask_weight() when > > > determining if a sched_group is comprised of SMT siblings. > > > (PeterZ). > > > --- > > > kernel/sched/fair.c | 94 +++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 94 insertions(+) > > > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > index 26db017c14a3..8d763dd0174b 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -8597,10 +8597,98 @@ group_type group_classify(unsigned int imbalance_pct, > > > return group_has_spare; > > > } > > > > > > +/** > > > + * asym_smt_can_pull_tasks - Check whether the load balancing CPU can pull tasks > > > + * @dst_cpu: Destination CPU of the load balancing > > > + * @sds: Load-balancing data with statistics of the local group > > > + * @sgs: Load-balancing statistics of the candidate busiest group > > > + * @sg: The candidate busiest group > > > + * > > > + * Check the state of the SMT siblings of both @sds::local and @sg and decide > > > + * if @dst_cpu can pull tasks. > > > + * > > > + * If @dst_cpu does not have SMT siblings, it can pull tasks if two or more of > > > + * the SMT siblings of @sg are busy. If only one CPU in @sg is busy, pull tasks > > > + * only if @dst_cpu has higher priority. > > > + * > > > + * If both @dst_cpu and @sg have SMT siblings, and @sg has exactly one more > > > + * busy CPU than @sds::local, let @dst_cpu pull tasks if it has higher priority. > > > + * Bigger imbalances in the number of busy CPUs will be dealt with in > > > + * update_sd_pick_busiest(). > > > + * > > > + * If @sg does not have SMT siblings, only pull tasks if all of the SMT siblings > > > + * of @dst_cpu are idle and @sg has lower priority. > > > + */ > > > +static bool asym_smt_can_pull_tasks(int dst_cpu, struct sd_lb_stats *sds, > > > + struct sg_lb_stats *sgs, > > > + struct sched_group *sg) > > > +{ > > > +#ifdef CONFIG_SCHED_SMT > > > + bool local_is_smt, sg_is_smt; > > > + int sg_busy_cpus; > > > + > > > + local_is_smt = sds->local->flags & SD_SHARE_CPUCAPACITY; > > > + sg_is_smt = sg->flags & SD_SHARE_CPUCAPACITY; > > > + > > > + sg_busy_cpus = sgs->group_weight - sgs->idle_cpus; > > > + > > > + if (!local_is_smt) { > > > + /* > > > + * If we are here, @dst_cpu is idle and does not have SMT > > > + * siblings. Pull tasks if candidate group has two or more > > > + * busy CPUs. > > > + */ > > > + if (sg_is_smt && sg_busy_cpus >= 2) > > > > Do you really need to test sg_is_smt ? if sg_busy_cpus >= 2 then > > sd_is_smt must be true ? > > Thank you very much for your feedback Vincent! > > Yes, it is true that sg_busy_cpus >=2 is only true if @sg is SMT. I will > remove this check. > > > > > Also, This is the default behavior where we want to even the number of > > busy cpu. Shouldn't you return false and fall back to the default > > behavior ? > > This is also true. > > > > > That being said, the default behavior tries to even the number of idle > > cpus which is easier to compute and is equal to even the number of > > busy cpus in "normal" system with the same number of cpus in groups > > but this is not the case here. It could be good to change the default > > behavior to even the number of busy cpus and that you use the default > > behavior here. Additional condition will be used to select the busiest > > group like more busy cpu or more number of running tasks > > That is a very good observation. Checking the number of idle CPUs > assumes that both groups have the same number of CPUs. I'll look into > modifying the default behavior. Because this change will impact default smt/smp system, we might prefer to do that in a separate step With the removal of the condition !sds->local_stat.sum_nr_running which seems useless because dst_cpu is idle and not SMT, this patch looks good to me > > > > > > + return true; > > > + > > > + /* > > > + * @dst_cpu does not have SMT siblings. @sg may have SMT > > > + * siblings and only one is busy. In such case, @dst_cpu > > > + * can help if it has higher priority and is idle (i.e., > > > + * it has no running tasks). > > > > The previous comment above assume that "@dst_cpu is idle" but now you > > need to check that sds->local_stat.sum_nr_running == 0 > > But we already know that, right? We are here because in > update_sg_lb_stats() we determine that dst CPU is idle (env->idle != > CPU_NOT_IDLE). > > Thanks and BR, > Ricardo 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=-13.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 66CD8C433FE for ; Fri, 17 Sep 2021 15:26:01 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9E06961164 for ; Fri, 17 Sep 2021 15:26:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 9E06961164 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4H9yTL6sxwz303F for ; Sat, 18 Sep 2021 01:25:58 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=Zh3W3Da0; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=linaro.org (client-ip=2607:f8b0:4864:20::731; helo=mail-qk1-x731.google.com; envelope-from=vincent.guittot@linaro.org; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=Zh3W3Da0; dkim-atps=neutral Received: from mail-qk1-x731.google.com (mail-qk1-x731.google.com [IPv6:2607:f8b0:4864:20::731]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4H9ySZ1Sq6z2yNM for ; Sat, 18 Sep 2021 01:25:15 +1000 (AEST) Received: by mail-qk1-x731.google.com with SMTP id c7so13055906qka.2 for ; Fri, 17 Sep 2021 08:25:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Th9Dectw0xwMPChqk9pLJzZcuXtG5lCvKTCrxYFTnPc=; b=Zh3W3Da0kbNcul9/QjTY8/GOdUPlHVJfwPwUB1inXpRaRf2uVc8uJcpZ7xnSgztj+s kcTxi0c+md7rzAoqgWmFCsaR/KQMsljf2yFeKfWLIAY7dxQmo4Nev5uc7/YbmTlsN7x0 oesDVqfNzM0vERyFjSwyxWaaiCnpb5ca3IOIvGp3ylUV79SflMZgIcvpEk8UCUTvI5HO XkfN1812wdixqPtIrJ5d62Ha04GecIyaId6y5wBdb1sIe+Wx+zLYdWh37aFHbXkpqMhW ADGYCo8r2JpmPWR9wFsP+dLU7KklHTbAyBYqKJxTI1QYKpsdA/R81nqIuApohRMTNrLa Xttw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Th9Dectw0xwMPChqk9pLJzZcuXtG5lCvKTCrxYFTnPc=; b=sZM9XZXFneo8Q+Ph7eF7PyZeoc7x1HTiwY2GEhUawlma9lfwVuztHgu1aDtCPaXUI1 yw8A4f5JsBX7Oi62Nz5SsqQptoMgnvclFtmiNowED015fTbEVMUxo0QFPRQDJcfQLrwV fpX1ABE+5I70PT/u3aaAPR1A1NLVA+Zu/TDgSffoSLbSpl7I3fIrbtn6+yOeNqVegasi S/9/VHLfNOF8WPsAvJJZ2n9cEQ90dRYPqB2bghuuNqLYawIVJGp/MCf8mXnWqrPu7mfU qsNSpNeZ1dcms1Bu1cJRGU4Vps7glZAmzpVjcDM9iW1jU684QTrIjood06EpfOMIFKn7 fM1A== X-Gm-Message-State: AOAM5325jk+ZEmUQ6748Hay9kVK3S7PPr9cQL/4K501BRb5RIzvWzot0 kl2q4+KTAB8fcJNiABWCnVDGe/gDSEHdVguMRwqRaQ== X-Google-Smtp-Source: ABdhPJzi+ecvVDZHy16eTUV4B931Trwh6EgK/voL8hMSSwIkuUjyW0X15/qP2d5cA0l+DPQ8R0OiJeElYN/SoNclYEU= X-Received: by 2002:a25:9001:: with SMTP id s1mr13884940ybl.191.1631892313004; Fri, 17 Sep 2021 08:25:13 -0700 (PDT) MIME-Version: 1.0 References: <20210911011819.12184-1-ricardo.neri-calderon@linux.intel.com> <20210911011819.12184-7-ricardo.neri-calderon@linux.intel.com> <20210917010044.GA23727@ranerica-svr.sc.intel.com> In-Reply-To: <20210917010044.GA23727@ranerica-svr.sc.intel.com> From: Vincent Guittot Date: Fri, 17 Sep 2021 17:25:02 +0200 Message-ID: Subject: Re: [PATCH v5 6/6] sched/fair: Consider SMT in ASYM_PACKING load balance To: Ricardo Neri Content-Type: text/plain; charset="UTF-8" X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Juri Lelli , Aubrey Li , Srikar Dronamraju , "Ravi V. Shankar" , "Peter Zijlstra \(Intel\)" , Ricardo Neri , Ben Segall , Srinivas Pandruvada , "Joel Fernandes \(Google\)" , Ingo Molnar , "Rafael J . Wysocki" , Steven Rostedt , Mel Gorman , Len Brown , Nicholas Piggin , Aubrey Li , Dietmar Eggemann , Tim Chen , Quentin Perret , Daniel Bristot de Oliveira , linux-kernel , linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Fri, 17 Sept 2021 at 03:01, Ricardo Neri wrote: > > On Wed, Sep 15, 2021 at 05:43:44PM +0200, Vincent Guittot wrote: > > On Sat, 11 Sept 2021 at 03:19, Ricardo Neri > > wrote: > > > > > > When deciding to pull tasks in ASYM_PACKING, it is necessary not only to > > > check for the idle state of the destination CPU, dst_cpu, but also of > > > its SMT siblings. > > > > > > If dst_cpu is idle but its SMT siblings are busy, performance suffers > > > if it pulls tasks from a medium priority CPU that does not have SMT > > > siblings. > > > > > > Implement asym_smt_can_pull_tasks() to inspect the state of the SMT > > > siblings of both dst_cpu and the CPUs in the candidate busiest group. > > > > > > Cc: Aubrey Li > > > Cc: Ben Segall > > > Cc: Daniel Bristot de Oliveira > > > Cc: Dietmar Eggemann > > > Cc: Mel Gorman > > > Cc: Quentin Perret > > > Cc: Rafael J. Wysocki > > > Cc: Srinivas Pandruvada > > > Cc: Steven Rostedt > > > Cc: Tim Chen > > > Reviewed-by: Joel Fernandes (Google) > > > Reviewed-by: Len Brown > > > Signed-off-by: Ricardo Neri > > > --- > > > Changes since v4: > > > * Use sg_lb_stats::sum_nr_running the idle state of a scheduling group. > > > (Vincent, Peter) > > > * Do not even idle CPUs in asym_smt_can_pull_tasks(). (Vincent) > > > * Updated function documentation and corrected a typo. > > > > > > Changes since v3: > > > * Removed the arch_asym_check_smt_siblings() hook. Discussions with the > > > powerpc folks showed that this patch should not impact them. Also, more > > > recent powerpc processor no longer use asym_packing. (PeterZ) > > > * Removed unnecessary local variable in asym_can_pull_tasks(). (Dietmar) > > > * Removed unnecessary check for local CPUs when the local group has zero > > > utilization. (Joel) > > > * Renamed asym_can_pull_tasks() as asym_smt_can_pull_tasks() to reflect > > > the fact that it deals with SMT cases. > > > * Made asym_smt_can_pull_tasks() return false for !CONFIG_SCHED_SMT so > > > that callers can deal with non-SMT cases. > > > > > > Changes since v2: > > > * Reworded the commit message to reflect updates in code. > > > * Corrected misrepresentation of dst_cpu as the CPU doing the load > > > balancing. (PeterZ) > > > * Removed call to arch_asym_check_smt_siblings() as it is now called in > > > sched_asym(). > > > > > > Changes since v1: > > > * Don't bailout in update_sd_pick_busiest() if dst_cpu cannot pull > > > tasks. Instead, reclassify the candidate busiest group, as it > > > may still be selected. (PeterZ) > > > * Avoid an expensive and unnecessary call to cpumask_weight() when > > > determining if a sched_group is comprised of SMT siblings. > > > (PeterZ). > > > --- > > > kernel/sched/fair.c | 94 +++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 94 insertions(+) > > > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > index 26db017c14a3..8d763dd0174b 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -8597,10 +8597,98 @@ group_type group_classify(unsigned int imbalance_pct, > > > return group_has_spare; > > > } > > > > > > +/** > > > + * asym_smt_can_pull_tasks - Check whether the load balancing CPU can pull tasks > > > + * @dst_cpu: Destination CPU of the load balancing > > > + * @sds: Load-balancing data with statistics of the local group > > > + * @sgs: Load-balancing statistics of the candidate busiest group > > > + * @sg: The candidate busiest group > > > + * > > > + * Check the state of the SMT siblings of both @sds::local and @sg and decide > > > + * if @dst_cpu can pull tasks. > > > + * > > > + * If @dst_cpu does not have SMT siblings, it can pull tasks if two or more of > > > + * the SMT siblings of @sg are busy. If only one CPU in @sg is busy, pull tasks > > > + * only if @dst_cpu has higher priority. > > > + * > > > + * If both @dst_cpu and @sg have SMT siblings, and @sg has exactly one more > > > + * busy CPU than @sds::local, let @dst_cpu pull tasks if it has higher priority. > > > + * Bigger imbalances in the number of busy CPUs will be dealt with in > > > + * update_sd_pick_busiest(). > > > + * > > > + * If @sg does not have SMT siblings, only pull tasks if all of the SMT siblings > > > + * of @dst_cpu are idle and @sg has lower priority. > > > + */ > > > +static bool asym_smt_can_pull_tasks(int dst_cpu, struct sd_lb_stats *sds, > > > + struct sg_lb_stats *sgs, > > > + struct sched_group *sg) > > > +{ > > > +#ifdef CONFIG_SCHED_SMT > > > + bool local_is_smt, sg_is_smt; > > > + int sg_busy_cpus; > > > + > > > + local_is_smt = sds->local->flags & SD_SHARE_CPUCAPACITY; > > > + sg_is_smt = sg->flags & SD_SHARE_CPUCAPACITY; > > > + > > > + sg_busy_cpus = sgs->group_weight - sgs->idle_cpus; > > > + > > > + if (!local_is_smt) { > > > + /* > > > + * If we are here, @dst_cpu is idle and does not have SMT > > > + * siblings. Pull tasks if candidate group has two or more > > > + * busy CPUs. > > > + */ > > > + if (sg_is_smt && sg_busy_cpus >= 2) > > > > Do you really need to test sg_is_smt ? if sg_busy_cpus >= 2 then > > sd_is_smt must be true ? > > Thank you very much for your feedback Vincent! > > Yes, it is true that sg_busy_cpus >=2 is only true if @sg is SMT. I will > remove this check. > > > > > Also, This is the default behavior where we want to even the number of > > busy cpu. Shouldn't you return false and fall back to the default > > behavior ? > > This is also true. > > > > > That being said, the default behavior tries to even the number of idle > > cpus which is easier to compute and is equal to even the number of > > busy cpus in "normal" system with the same number of cpus in groups > > but this is not the case here. It could be good to change the default > > behavior to even the number of busy cpus and that you use the default > > behavior here. Additional condition will be used to select the busiest > > group like more busy cpu or more number of running tasks > > That is a very good observation. Checking the number of idle CPUs > assumes that both groups have the same number of CPUs. I'll look into > modifying the default behavior. Because this change will impact default smt/smp system, we might prefer to do that in a separate step With the removal of the condition !sds->local_stat.sum_nr_running which seems useless because dst_cpu is idle and not SMT, this patch looks good to me > > > > > > + return true; > > > + > > > + /* > > > + * @dst_cpu does not have SMT siblings. @sg may have SMT > > > + * siblings and only one is busy. In such case, @dst_cpu > > > + * can help if it has higher priority and is idle (i.e., > > > + * it has no running tasks). > > > > The previous comment above assume that "@dst_cpu is idle" but now you > > need to check that sds->local_stat.sum_nr_running == 0 > > But we already know that, right? We are here because in > update_sg_lb_stats() we determine that dst CPU is idle (env->idle != > CPU_NOT_IDLE). > > Thanks and BR, > Ricardo