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;
Date:   Fri, 21 Jul 2023 09:57:04 +0800
From:   Aaron Lu <aaron.lu@intel.com>
To:     Vincent Guittot <vincent.guittot@linaro.org>
CC:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Daniel Jordan <daniel.m.jordan@oracle.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>,
        "Mel Gorman" <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Valentin Schneider <vschneid@redhat.com>,
        Tim Chen <tim.c.chen@intel.com>,
        Nitin Tekchandani <nitin.tekchandani@intel.com>,
        Yu Chen <yu.c.chen@intel.com>,
        Waiman Long <longman@redhat.com>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH 3/4] sched/fair: delay update_tg_load_avg() for
 cfs_rq's removed load
Message-ID: <20230721015704.GA212678@ziqianlu-dell>
References: <20230718134120.81199-1-aaron.lu@intel.com>
 <20230718134120.81199-4-aaron.lu@intel.com>
 <CAKfTPtAu33AN6=X82T=yOgm40S8OXi+sPcF0QyD-bYRPV=xPEg@mail.gmail.com>
 <20230719051826.GB84059@ziqianlu-dell>
 <20230719080105.GA90441@ziqianlu-dell>
 <CAKfTPtCnvOehfrz1OSW+rmkKW+28bdGw8fU+AvVrZTxkHibL_g@mail.gmail.com>
 <20230719132914.GA91858@ziqianlu-dell>
 <CAKfTPtAqpAo8Y9BdWZ-fmnyYgA8PEtFbObqWJxc-hs2Ktqkt3Q@mail.gmail.com>
 <20230720144233.GA185317@ziqianlu-dell>
 <CAKfTPtANqtEQjv2UThb5s0TPCS2adhC_14Gfv5ayAOva9Mib8g@mail.gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAKfTPtANqtEQjv2UThb5s0TPCS2adhC_14Gfv5ayAOva9Mib8g@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?M8di6pmblHNq1nGugen37NVhvcOp24Ph4LhDnvQfS8T5cRwCBFTellMSubaQ?=
 =?us-ascii?Q?jO7Xh9B3Gcjbzv3Gh6wmBxnrf1GVG3hHbE4q5AlZKysG6ZSasor5zUz1iBUY?=
 =?us-ascii?Q?Gd8wF9mcCbyxE+1dns6YJpyKfSTYhiJS54xf7e6X/TlCkti53iR04a8VKJf2?=
 =?us-ascii?Q?dyyikTr6YKpp1st4trP8pZZpHXiuGlqfTDXq3/unsbRP+15u2jPh10LQEO4o?=
 =?us-ascii?Q?+5vI4oQgIkTv2Oi5tC8Ypzmoue+aKXr6fEI8n50cCZMxS8D/pkECrEwjN5jN?=
 =?us-ascii?Q?DrWEcDGxt62KF3ae3g3CAHK3GyNkk5hNVGt3FHXeZrOIBH6P7MFNmcmGLRs2?=
 =?us-ascii?Q?ixCO40lrF1iHTDVRE0G5OXkCdC08PUI16ApjPq+rnVPKRKZU14Aos+whmY+W?=
 =?us-ascii?Q?tMAH1OJXXoeq7s94v40YVe07MTMwIT8YGameJktjn57GZbshrfZlt1QU33uc?=
 =?us-ascii?Q?XaVV1O99FSQIwCpb8DqEma6d13pwnmwwm2i1zpgw/b2JPJ19ER9JduXYpUzz?=
 =?us-ascii?Q?HNJmoofZCaBBj9PNdhK6DN5fVVLT15Yw/p7yEUirbD12tBqB50zB8rI+Tb0z?=
 =?us-ascii?Q?EmRIn/tTXckoOqDzYGKTWrnkGY6hr5JXnkYKCqoxdTl+WBumLwDq59uWPDcy?=
 =?us-ascii?Q?wJo4i8CJlOPYARG5EMhnHnuFfqx20OB5kQZEmvK9gBBAnFK4vfvSvXb4I6Q7?=
 =?us-ascii?Q?exypkXWFnDauNAE4+StU5MRL3qDqu0oyh1p6Rc4eeEceJd8sURWCUjsmW4z4?=
 =?us-ascii?Q?/ZxDZa79mEKUjsjBvhGol3kuB+05JkfHliOcuBxDkQLYFHq8z7yHVxpLGctT?=
 =?us-ascii?Q?2acbxGoC1HXhSRnz4crTal19D1MHHshibqsFkw+PEUWFwsePFXvmcZJmMe6W?=
 =?us-ascii?Q?EaV2hatZK3YZ0mQ24N+FJizsZGqoBPHp890tUFiuiXxGrHYhqqyoA6/KUxMl?=
 =?us-ascii?Q?2N+kpQr12dVHGeMZMBcaUsn2erQlA4T0OeFAOaaS1S5Emq20ua6+tasHLPCy?=
 =?us-ascii?Q?+di+qS4FZf/OngZZ1wSEey5NtrZG0vzX34vvAHQqwfdvgFsgkZS5JT3P8vgv?=
 =?us-ascii?Q?6J+oqqmmijDVOHgMQZo0ePp8yIp8Pk21fs4l7mpRzq7hsVsXV52dO+jYbXtW?=
 =?us-ascii?Q?SIMq7a7IAjLdpUpqbXLtVmgaGP5pi+0cY+//PFURHEfyoXDyjk4gQkNAgPEY?=
 =?us-ascii?Q?ZDtsd38JK1QE1fxobojPClh67v8+u2ZmKexDe73/sfxWatsMT0JtxGkaM64D?=
 =?us-ascii?Q?3R4A2YrKyK1lCYLLpcqRuzIUAZ2GHPSNHm902kSrITlfIKJTDidobByBJIo1?=
 =?us-ascii?Q?GwfhkCA51je8PFtrxoK5GE8p0WQfudQ7JMO2qXyoSuNZvctwmJXsJ663d02v?=
 =?us-ascii?Q?rL7+A+/+5Twuk2e+jn6Sa5GrDmYNiaw5u7G0bmWsScc6WK1wx9FURh4p2Atr?=
 =?us-ascii?Q?By+mGJ2RwVTK97m5+iOjIrmqN7LmEuABqeQQxZ1Y11o/DHYl9uftCtHNc4xr?=
 =?us-ascii?Q?PntNCJqpGCt0fAfqVRMzdHDpagWPXt/8jz0lFnHMvaYn6Jo7Vss+ObFQfQBB?=
 =?us-ascii?Q?bh1H1Q7URk+cVqzTmgtE64MBmjMImTaz56ZYv38O?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4451628e-c4f9-4c07-9c4b-08db898dcf30
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3062.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jul 2023 01:57:16.5317
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: B1SGg9CfRDoNU0wEPLWbM5bTrHxh4aqrGTaKpO1QndBm/WXnOlzg5kkvyUPOcOSRpslJwPdLrH9cuqwRVsAJXg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6017
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Jul 20, 2023 at 05:02:32PM +0200, Vincent Guittot wrote:
>
> What was wrong with your proposal to limit the update inside
> update_tg_load_avg()  ? except maybe s/1000000/NSEC_PER_MSEC/ and
> computing delta after testing the time since last update

I was thinking it might be better to align the update_tg_load_avg() with
cfs_rq's decay point but that's just my feeling.

Absolutely nothing wrong with the below approach, it's straightforward
and effective. I'll fix the use of cfs_rq_clock_pelt() and collect
some data and then send out v2.

Thank you Vincent for all your comments, they're very useful to me.

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index a80a73909dc2..e48fd0e6982d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3665,6 +3665,7 @@ static inline bool cfs_rq_is_decayed(struct
> cfs_rq *cfs_rq)
>  static inline void update_tg_load_avg(struct cfs_rq *cfs_rq)
>  {
>         long delta = cfs_rq->avg.load_avg - cfs_rq->tg_load_avg_contrib;
> +       u64 now = cfs_rq_clock_pelt(cfs_rq);
> 
>         /*
>          * No need to update load_avg for root_task_group as it is not used.
> @@ -3672,9 +3673,11 @@ static inline void update_tg_load_avg(struct
> cfs_rq *cfs_rq)
>         if (cfs_rq->tg == &root_task_group)
>                 return;
> 
> -       if (abs(delta) > cfs_rq->tg_load_avg_contrib / 64) {
> +       if ((now - cfs_rq->last_update_tg_load_avg > 1000000) &&
> +           abs(delta) > cfs_rq->tg_load_avg_contrib / 64) {
>                 atomic_long_add(delta, &cfs_rq->tg->load_avg);
>                 cfs_rq->tg_load_avg_contrib = cfs_rq->avg.load_avg;
> +               cfs_rq->last_update_tg_load_avg = now;
>         }
>  }