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 8962ACA0EEB for ; Tue, 12 Sep 2023 14:05:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235795AbjILOFO (ORCPT ); Tue, 12 Sep 2023 10:05:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50852 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235787AbjILOFM (ORCPT ); Tue, 12 Sep 2023 10:05:12 -0400 Received: from smtpout.efficios.com (smtpout.efficios.com [167.114.26.122]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2110310D0 for ; Tue, 12 Sep 2023 07:05:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=efficios.com; s=smtpout1; t=1694527506; bh=84aZCfsC6nM8LAnqrSWKZhmE7k5tMSqBv89GRiTFvf8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=LBW+rTRFION//TLy/S2cp6YMp78QOnJAMst6Yv0CnGumdo0hemMfv/A1+zQN7RC0x o2YDQfaJdISK6CKC0EtyZzHJzG6Dt5JapTGxj2jjmcJg0l3qO3F1/hoBfW90+Jgt6h YI4hrklXFF7B/p0x1+GOU799YfxJwQEq4KF7RTIwdvRf0vm0ciDAMiLfd0qGgVMnqQ IELiNK2ZumqeZUjWuPE3VnaRzgw0XV57uuyBsUpVrRHqP/VbNAvwSxNwmgPhkFRZkA lVA6ZQpuN9Tx3QKfJnVQSoeXuBrbdpUXNXu3J5GeKW44AZ3MloTPzKbTNc4/xzMZ7M xSed9tjq/hszw== Received: from [IPV6:2606:6d00:100:4000:cacb:9855:de1f:ded2] (unknown [IPv6:2606:6d00:100:4000:cacb:9855:de1f:ded2]) by smtpout.efficios.com (Postfix) with ESMTPSA id 4RlQMQ2NLHz1PNj; Tue, 12 Sep 2023 10:05:06 -0400 (EDT) Message-ID: <244ff1b9-c51e-be9b-0755-757f969b36e1@efficios.com> Date: Tue, 12 Sep 2023 10:06:27 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.14.0 Subject: Re: [RFC PATCH 2/2] sched/fair: skip the cache hot CPU in select_idle_cpu() Content-Language: en-US To: Chen Yu Cc: Peter Zijlstra , Ingo Molnar , Vincent Guittot , Juri Lelli , Tim Chen , Aaron Lu , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Daniel Bristot de Oliveira , Valentin Schneider , K Prateek Nayak , "Gautham R . Shenoy" , linux-kernel@vger.kernel.org References: <30a7ff14-3f48-e8cf-333f-cbb7499656e3@efficios.com> From: Mathieu Desnoyers In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 9/12/23 07:53, Chen Yu wrote: > Hi Mathieu, > > thanks for the review, > > On 2023-09-11 at 11:43:27 -0400, Mathieu Desnoyers wrote: >> On 9/11/23 11:26, Mathieu Desnoyers wrote: >>> On 9/10/23 22:50, Chen Yu wrote: >> [...] >>>> --- >>>>   kernel/sched/fair.c     | 30 +++++++++++++++++++++++++++--- >>>>   kernel/sched/features.h |  1 + >>>>   kernel/sched/sched.h    |  1 + >>>>   3 files changed, 29 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >>>> index e20f50726ab8..fe3b760c9654 100644 >>>> --- a/kernel/sched/fair.c >>>> +++ b/kernel/sched/fair.c >>>> @@ -6629,6 +6629,21 @@ static void dequeue_task_fair(struct rq *rq, >>>> struct task_struct *p, int flags) >>>>       hrtick_update(rq); >>>>       now = sched_clock_cpu(cpu_of(rq)); >>>>       p->se.prev_sleep_time = task_sleep ? now : 0; >>>> +#ifdef CONFIG_SMP >>>> +    /* >>>> +     * If this rq will become idle, and dequeued task is >>>> +     * a short sleeping one, check if we can reserve >>>> +     * this idle CPU for that task for a short while. >>>> +     * During this reservation period, other wakees will >>>> +     * skip this 'idle' CPU in select_idle_cpu(), and this >>>> +     * short sleeping task can pick its previous CPU in >>>> +     * select_idle_sibling(), which brings better cache >>>> +     * locality. >>>> +     */ >>>> +    if (sched_feat(SIS_CACHE) && task_sleep && !rq->nr_running && >>>> +        p->se.sleep_avg && p->se.sleep_avg < >>>> sysctl_sched_migration_cost) >>>> +        rq->cache_hot_timeout = now + p->se.sleep_avg; >>> >>> This is really cool! >>> >>> There is one scenario that worries me with this approach: workloads >>> that sleep for a long time and then have short blocked periods. >>> Those bursts will likely bring the average to values too high >>> to stay below sysctl_sched_migration_cost. >>> >>> I wonder if changing the code above for the following would help ? >>> >>> if (sched_feat(SIS_CACHE) && task_sleep && !rq->nr_running && >>> p->se.sleep_avg) >>>     rq->cache_hot_timeout = now + min(sysctl_sched_migration_cost, >>> p->se.sleep_avg); >>> >>> For tasks that have a large sleep_avg, it would activate this rq >>> "appear as not idle for rq selection" scheme for a window of >>> sysctl_sched_migration_cost. If the sleep ends up being a long one, >>> preventing other tasks from being migrated to this rq for a tiny >>> window should not matter performance-wise. I would expect that it >>> could help workloads that come in bursts. >> >> There is perhaps a better way to handle bursts: >> >> When calculating the sleep_avg, we actually only really care about >> the sleep time for short bursts, so we could use the sysctl_sched_migration_cost >> to select which of the sleeps should be taken into account in the avg. >> >> We could rename the field "sleep_avg" to "burst_sleep_avg", and have: >> >> u64 now = sched_clock_cpu(task_cpu(p)); >> >> if ((flags & ENQUEUE_WAKEUP) && last_sleep && cpu_online(task_cpu(p)) && >> now > last_sleep && now - last_sleep < sysctl_sched_migration_cost) >> update_avg(&p->se.burst_sleep_avg, now - last_sleep); >> >> Then we can have this code is dequeue_task_fair: >> >> if (sched_feat(SIS_CACHE) && task_sleep && !rq->nr_running && p->se.busrt_sleep_avg) >> rq->cache_hot_timeout = now + p->se.burst_sleep_avg; >> >> Thoughts ? >> > > This looks reasonable, it would be fine grained to only monitor the short sleep duration > of that task. Let me try this approach to see if there is any difference. > One more tweak: given that more than one task can update the cache_hot_timeout forward one after another, and given that some tasks have larger burst_sleep_avg values than others, I suspect we want to keep the forward movement monotonic with something like: if (sched_feat(SIS_CACHE) && task_sleep && !rq->nr_running && p->se.burst_sleep_avg && rq->cache_hot_timeout < now + p->se.burst_sleep_avg) rq->cache_hot_timeout = now + p->se.burst_sleep_avg; Thanks, Mathieu > thanks, > Chenyu > >> Thanks, >> >> Mathieu >> >> -- >> Mathieu Desnoyers >> EfficiOS Inc. >> https://www.efficios.com >> -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com