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=-5.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, HK_RANDOM_FROM,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS 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 018C4C38A2A for ; Fri, 8 May 2020 16:59:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D3868218AC for ; Fri, 8 May 2020 16:59:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727122AbgEHQ7e (ORCPT ); Fri, 8 May 2020 12:59:34 -0400 Received: from sender2-pp-o92.zoho.com.cn ([163.53.93.251]:25314 "EHLO sender2-pp-o92.zoho.com.cn" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726750AbgEHQ7d (ORCPT ); Fri, 8 May 2020 12:59:33 -0400 X-Greylist: delayed 6383 seconds by postgrey-1.27 at vger.kernel.org; Fri, 08 May 2020 12:59:29 EDT ARC-Seal: i=1; a=rsa-sha256; t=1588957119; cv=none; d=zoho.com.cn; s=zohoarc; b=TkaJmbwyr5Dih3GcfdQAITPF5nlWXDEKZkjOeOpNQaR9GPsGlvvcMD5v1A6uMKjVANc7kF+FiR4GhM9GYD6foG2ZvZGlxOzHM0DrjQti3RURB92y3whH6NNtEgkv3Fz5YE3bsivaJTo7nE2l4kJEmRmvANCYZ0OuO+4Ul5dUAmg= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zoho.com.cn; s=zohoarc; t=1588957119; h=Content-Type:Cc:Date:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:To; bh=hinQtspvNNJvWFC1mFI4rvIKuoVuJBJGKGSRpNj179s=; b=NptcGeiyQbTx+oRETd2ZICyJ7G1D74eiFtRwH0HFQN7r3LwExAk+ycf7NiyoS5GslSXD9j8XDnk9JQj7zwKzF5D1KUQbxfLv5lIwC6P0yXGstKnffMBb+ve2R1K6IH9cLn2gFSSKYiGqD1CAw73tnYwO6F+BUIF6BaurJ8qdpTI= ARC-Authentication-Results: i=1; mx.zoho.com.cn; spf=pass smtp.mailfrom=zohooouoto@zoho.com.cn; dmarc=pass header.from= header.from= Received: from localhost (122.194.88.39 [122.194.88.39]) by mx.zoho.com.cn with SMTPS id 1588957117227398.5930646405061; Sat, 9 May 2020 00:58:37 +0800 (CST) Date: Sat, 9 May 2020 01:02:13 +0800 From: Tao Zhou To: Vincent Guittot Cc: Phil Auld , Peter Zijlstra , linux-kernel , Ingo Molnar , Juri Lelli , Tao Zhou Subject: Re: [PATCH v2] sched/fair: Fix enqueue_task_fair warning some more Message-ID: <20200508170213.GA27353@geo.homenetwork> References: <20200506141821.GA9773@lorien.usersys.redhat.com> <20200507203612.GF19331@lorien.usersys.redhat.com> <20200508151515.GA25974@geo.homenetwork> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-ZohoCNMailClient: External Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, May 08, 2020 at 05:27:44PM +0200, Vincent Guittot wrote: > On Fri, 8 May 2020 at 17:12, Tao Zhou wrote: > > > > Hi Phil, > > > > On Thu, May 07, 2020 at 04:36:12PM -0400, Phil Auld wrote: > > > sched/fair: Fix enqueue_task_fair warning some more > > > > > > The recent patch, fe61468b2cb (sched/fair: Fix enqueue_task_fair warning) > > > did not fully resolve the issues with the rq->tmp_alone_branch != > > > &rq->leaf_cfs_rq_list warning in enqueue_task_fair. There is a case where > > > the first for_each_sched_entity loop exits due to on_rq, having incompletely > > > updated the list. In this case the second for_each_sched_entity loop can > > > further modify se. The later code to fix up the list management fails to do > > > what is needed because se no longer points to the sched_entity which broke > > > out of the first loop. > > > > > > > > Address this by calling leaf_add_rq_list if there are throttled parents while > > > doing the second for_each_sched_entity loop. > > > > Thanks for your trace imformation and explanation. I > > truely have learned from this and that. > > > > s/leaf_add_rq_list/list_add_leaf_cfs_rq/ > > > > > > > > Suggested-by: Vincent Guittot > > > Signed-off-by: Phil Auld > > > Cc: Peter Zijlstra (Intel) > > > Cc: Vincent Guittot > > > Cc: Ingo Molnar > > > Cc: Juri Lelli > > > --- > > > kernel/sched/fair.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > index 02f323b85b6d..c6d57c334d51 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -5479,6 +5479,13 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags) > > > /* end evaluation on encountering a throttled cfs_rq */ > > > if (cfs_rq_throttled(cfs_rq)) > > > goto enqueue_throttle; > > > + > > > + /* > > > + * One parent has been throttled and cfs_rq removed from the > > > + * list. Add it back to not break the leaf list. > > > + */ > > > + if (throttled_hierarchy(cfs_rq)) > > > + list_add_leaf_cfs_rq(cfs_rq); > > > } > > > > I was confused by why the throttled cfs rq can be on list. > > It is possible when enqueue a task and thanks to the 'threads'. > > But I think the above comment does not truely put the right > > intention, right ? > > If throttled parent is onlist, the child cfs_rq is ignored > > to be added to the leaf cfs_rq list me think. > > > > unthrottle_cfs_rq() follows the same logic if i am not wrong. > > Is it necessary to add the above to it ? > > When a cfs_rq is throttled, its sched group is dequeued and all child > cfs_rq are removed from leaf_cfs_rq list. But the sched group of the > child cfs_rq stay enqueued in the throttled cfs_rq so child sched > group->on_rq might be still set. If there is a throttle of throttle, and unthrottle the child throttled cfs_rq(ugly): ... | cfs_rq throttled (parent A) | | cfs_rq in hierarchy (B) | | cfs_rq throttled (C) | ... Then unthrottle the child throttled cfs_rq C, now the A is on the leaf_cfs_rq list. sched_group entity of C is enqueued to B, and sched_group entity of B is on_rq and is ignored by enqueue but in the throttled hierarchy and not add to leaf_cfs_rq list. The above may be absolutely wrong that I miss something. Another thing : In enqueue_task_fair(): for_each_sched_entity(se) { cfs_rq = cfs_rq_of(se); if (list_add_leaf_cfs_rq(cfs_rq)) break; } In unthrottle_cfs_rq(): for_each_sched_entity(se) { cfs_rq = cfs_rq_of(se); list_add_leaf_cfs_rq(cfs_rq); } The difference between them is that if condition, add if condition to unthrottle_cfs_rq() may be an optimization and keep the same. > > > > Thanks, > > Tau > > > > > > > > enqueue_throttle: > > > -- > > > 2.18.0 > > > > > > V2 rework the fix based on Vincent's suggestion. Thanks Vincent. > > > > > > > > > Cheers, > > > Phil > > > > > > -- > > >