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=-6.9 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 D3F3DC4363A for ; Tue, 27 Oct 2020 18:03:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 76EA220723 for ; Tue, 27 Oct 2020 18:03:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=joelfernandes.org header.i=@joelfernandes.org header.b="bsIQ5abh" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1824151AbgJ0SDh (ORCPT ); Tue, 27 Oct 2020 14:03:37 -0400 Received: from mail-qt1-f195.google.com ([209.85.160.195]:33063 "EHLO mail-qt1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757394AbgJ0OTO (ORCPT ); Tue, 27 Oct 2020 10:19:14 -0400 Received: by mail-qt1-f195.google.com with SMTP id j62so1075858qtd.0 for ; Tue, 27 Oct 2020 07:19:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=joelfernandes.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=bxh1Do9gB2L1qAcESsHXkjxOIjeM0Npe6oBM5FfnnCY=; b=bsIQ5abhxIx4Kuz1KfVERGK1ug+bmNb8gWArGIWJRq4uLEOGl8+9v8dKOJO+LXEpbi nnpSu/+OiU5GZqcVoUkt4PVykOBtgfvYoS4lounvL/Dtz2FHPnHA3gYZAZDketrQcyjR F8j9bWg7MZJ1RhEGhuw6A+LRKyYIoI0XBjSn0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=bxh1Do9gB2L1qAcESsHXkjxOIjeM0Npe6oBM5FfnnCY=; b=eX819eD2P1FfOctvOyP+x+Rr7KO9SIF6/bLHLJQGcXIpwHCkvwEhXoS4kbop+gvD8d +v3IBNc+ao9mR6ZCO6KXYyUVEZ0Y7X7soW55ztERXd6+/O8+MiJ65YzDnrJuUkxaULnS QQVa7g2GeQdV0ZpIKxXHgJX6H2AJe+1Vd8rqfZKWnMupXnracffD1cxMlJaFzOnswcUz Crw1WEfZ1BebIgxBdswik/sgtwM8tgR7QWx6oPWiot64oQoDs2KOOCRS+EkZgdQFaZR0 tM3jtez3n6E7db7HmZfI9yafSwbR2+UTznqZ5Y6ac8gwQiALXTDKZl2dKbVbs/WoDfJe 4tpA== X-Gm-Message-State: AOAM5323plPaje5AUNNtP5yYWhY1gssueN5weXITNti/e41ZAWQYwBtu An6nHf+8RrZmGhqO4O+YMR+GsQ== X-Google-Smtp-Source: ABdhPJya1MYpLPelnzTrpfjK5t/oGZBp8gdcvGnyABV2WaYubYYFygCM3HswoL6/iwzEyRm4xj67ag== X-Received: by 2002:ac8:6f53:: with SMTP id n19mr2321027qtv.282.1603808352539; Tue, 27 Oct 2020 07:19:12 -0700 (PDT) Received: from localhost ([2620:15c:6:411:cad3:ffff:feb3:bd59]) by smtp.gmail.com with ESMTPSA id q16sm871284qtw.9.2020.10.27.07.19.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Oct 2020 07:19:11 -0700 (PDT) Date: Tue, 27 Oct 2020 10:19:11 -0400 From: Joel Fernandes To: Peter Zijlstra Cc: Vineeth Pillai , "Li, Aubrey" , Nishanth Aravamudan , Julien Desfossez , Tim Chen , Aaron Lu , Aubrey Li , Thomas Glexiner , LKML , Ingo Molnar , Linus Torvalds , Frederic Weisbecker , Kees Cook , Greg Kerr , Phil Auld , Valentin Schneider , Mel Gorman , Pawan Gupta , Paolo Bonzini , vineeth@bitbyteword.org, Chen Yu , Christian Brauner , Agata Gruza , Antonio Gomez Iglesias , graf@amazon.com, konrad.wilk@oracle.com, Dario Faggioli , Paul Turner , Steven Rostedt , Patrick Bellasi , =?utf-8?B?YmVuYmppYW5nKOiSi+W9qik=?= , Alexandre Chartre , James.Bottomley@hansenpartnership.com, OWeisse@umich.edu, Dhaval Giani , Junaid Shahid , Jesse Barnes , "Hyser,Chris" , "Paul E. McKenney" , Tim Chen , "Ning, Hongyu" Subject: Re: [PATCH v8 -tip 02/26] sched: Introduce sched_class::pick_task() Message-ID: <20201027141911.GC771372@google.com> References: <20201020014336.2076526-1-joel@joelfernandes.org> <20201020014336.2076526-3-joel@joelfernandes.org> <8ea1aa61-4a1c-2687-9f15-1062d37606c7@linux.intel.com> <20201023214702.GA3603399@google.com> <4241e5ac-ecdf-8634-fa0d-dd6759e477e1@linux.microsoft.com> <8230ada7-839f-2335-9a55-b09f6a813e91@linux.microsoft.com> <20201026090131.GE2628@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20201026090131.GE2628@hirez.programming.kicks-ass.net> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 26, 2020 at 10:01:31AM +0100, Peter Zijlstra wrote: > On Sat, Oct 24, 2020 at 08:27:16AM -0400, Vineeth Pillai wrote: > > > > > > On 10/24/20 7:10 AM, Vineeth Pillai wrote: > > > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > index 93a3b874077d..4cae5ac48b60 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -4428,12 +4428,14 @@ pick_next_entity(struct cfs_rq *cfs_rq, struct > > > sched_entity *curr) > > >                         se = second; > > >         } > > > > > > -       if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < > > > 1) { > > > +       if (left && cfs_rq->next && > > > +                       wakeup_preempt_entity(cfs_rq->next, left) < 1) { > > >                 /* > > >                  * Someone really wants this to run. If it's not unfair, > > > run it. > > >                  */ > > >                 se = cfs_rq->next; > > > -       } else if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, > > > left) < 1) { > > > +       } else if (left && cfs_rq->last && > > > +                       wakeup_preempt_entity(cfs_rq->last, left) < 1) { > > >                 /* > > >                  * Prefer last buddy, try to return the CPU to a > > > preempted task. > > > > > > > > > There reason for left being NULL needs to be investigated. This was > > > there from v1 and we did not yet get to it. I shall try to debug later > > > this week. > > > > Thinking more about it and looking at the crash, I think that > > 'left == NULL' can happen in pick_next_entity for core scheduling. > > If a cfs_rq has only one task that is running, then it will be > > dequeued and 'left = __pick_first_entity()' will be NULL as the > > cfs_rq will be empty. This would not happen outside of coresched > > because we never call pick_tack() before put_prev_task() which > > will enqueue the task back. > > > > With core scheduling, a cpu can call pick_task() for its sibling while > > the sibling is still running the active task and put_prev_task has yet > > not been called. This can result in 'left == NULL'. > > Quite correct. Hurmph.. the reason we do this is because... we do the > update_curr() the wrong way around. And I can't seem to remember why we > do that (it was in my original patches). > > Something like so seems the obvious thing to do, but I can't seem to > remember why we're not doing it :-( The code below is just a refactor and not a functional change though, right? i.e. pick_next_entity() is already returning se = curr, if se == NULL. But the advantage of your refactor is it doesn't crash the kernel. So your change appears safe to me unless I missed something. thanks, - Joel > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -6950,15 +6950,10 @@ static struct task_struct *pick_task_fai > do { > struct sched_entity *curr = cfs_rq->curr; > > - se = pick_next_entity(cfs_rq, NULL); > + if (curr && curr->on_rq) > + update_curr(cfs_rq); > > - if (curr) { > - if (se && curr->on_rq) > - update_curr(cfs_rq); > - > - if (!se || entity_before(curr, se)) > - se = curr; > - } > + se = pick_next_entity(cfs_rq, curr); > > cfs_rq = group_cfs_rq(se); > } while (cfs_rq);