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;
Message-ID: <c086b5fb-681e-d104-1e11-873ed5444c5c@bytedance.com>
Date:   Thu, 8 Dec 2022 17:07:52 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.5.1
Subject: Re: [PATCH] Reduce rq lock contention in load_balance()
To:     chenying <chenying.kernel@bytedance.com>, mingo@redhat.com,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Benjamin Segall <bsegall@google.com>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>
References: <ef54b130-e727-5ed1-1a4e-1e3c0a713b98@bytedance.com>
Content-Language: en-US
From:   Abel Wu <wuyun.abel@bytedance.com>
In-Reply-To: <ef54b130-e727-5ed1-1a4e-1e3c0a713b98@bytedance.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Ying,

On 11/24/22 5:07 PM, chenying wrote:
> ...
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index a4a20046e586..384690bda8c3 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -954,6 +954,7 @@ struct balance_callback {
>   struct rq {
>       /* runqueue lock: */
>       raw_spinlock_t        __lock;
> +    raw_spinlock_t          lb_lock;

Do we really need a new lock for doing this? I may suggest the
following:

diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 87522c3de7b2..30d84e066a9a 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1048,6 +1048,7 @@ struct rq {

         struct balance_callback *balance_callback;

+       unsigned char           balancing;
         unsigned char           nohz_idle_balance;
         unsigned char           idle_balance;

and skip in-balancing runqueues early when find_busiest_queue().

Thanks,
	Abel

> 
>       /*
>        * nr_running and cpu_load should be in the same cacheline because