Received: by 2002:a25:ad19:0:0:0:0:0 with SMTP id y25csp997087ybi; Fri, 2 Aug 2019 07:42:06 -0700 (PDT) X-Google-Smtp-Source: APXvYqx5uplNs7vHRkeVE41OT4mUgfds0KeyyFrYzq6IJOFIRM8tm6ynqZDctqfu3/vyuv3b0oDa X-Received: by 2002:a63:7a01:: with SMTP id v1mr127360349pgc.310.1564756926765; Fri, 02 Aug 2019 07:42:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1564756926; cv=none; d=google.com; s=arc-20160816; b=TznieRl4E7LyMBKfOMiQdV3vQzw06rM9cEHf5Cjc/c1yvp6RaBaISpdwDM6WdkKGrU q5C213j1utPvc/RU5hgaIj4gXaBrNFk+He3OHdbh08+jvYGj6p/bGM4sEz0wQ/TX71Qk dmVEoWmjaDWr5YTwUYItxZUmlSccbsrOorHc42nk5oRlqDdHG8eeZaxlFHUhHzcLcMqC bYwasxR5axBDpWsE4j4AWgo3ONJVe+/S8b2KzSWmyonsTBFSWmVt0l4D6kI9Wu7fGlzr QzWJSEhkuXOekfkoiF0Gr5NQ+ZYot6cyiBU+WVRH3whu9wkVAWcVu5/XLaU311XNaPXs vZ/g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=HohU5S6eQLDs9ZSN7DgWxFPUQxsN3mdikxSrJnjO95c=; b=U8hLZng7oNNGHYP68VbqeLSWwNhYR+D7qxyk7//yfDStIMUrbJAnj1ElRj/W/CIeiD 1GfflvETLoeRwdhuGgSpjLAgu6IGKpkh7fulu0BNRYyhEL7JSrLWvYQvLCi03KWyAa/h KWRUSiWcuTy/DBgeF2hDdOVu4It/k9taBVQn+IBrlNaBnBICtXr38aHa78LGEr33sF3g vTx2hxqZu4/Tm6P5KzkBX4yfm3mNyKTmhPUPDv+Y7Nw447EVSGg7s1me/O14puwitt4o Gaf5LNB+wPPgkkbwTLI07PFd/p8AyiBYl82bai3mjd77ks7DV/GfNR+9vELOjTscDQqX xT8w== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id q32si6843450pjc.2.2019.08.02.07.41.51; Fri, 02 Aug 2019 07:42:06 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731486AbfHBJLK (ORCPT + 99 others); Fri, 2 Aug 2019 05:11:10 -0400 Received: from cloudserver094114.home.pl ([79.96.170.134]:45246 "EHLO cloudserver094114.home.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728211AbfHBJLJ (ORCPT ); Fri, 2 Aug 2019 05:11:09 -0400 Received: from 79.184.255.110.ipv4.supernova.orange.pl (79.184.255.110) (HELO kreacher.localnet) by serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer 0.83.275) id 1e8abed56eccc6b2; Fri, 2 Aug 2019 11:11:02 +0200 From: "Rafael J. Wysocki" To: Viresh Kumar Cc: Doug Smythies , "'Rafael J. Wysocki'" , 'Ingo Molnar' , 'Peter Zijlstra' , 'Linux PM' , 'Vincent Guittot' , 'Joel Fernandes' , "'v4 . 18+'" , 'Linux Kernel Mailing List' Subject: Re: [PATCH] cpufreq: schedutil: Don't skip freq update when limits change Date: Fri, 02 Aug 2019 11:11:01 +0200 Message-ID: <1599417.3YyTWY6lWL@kreacher> In-Reply-To: <20190802034819.vywlces7rxzfy33f@vireshk-i7> References: <000001d54892$a25b86b0$e7129410$@net> <20190802034819.vywlces7rxzfy33f@vireshk-i7> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Friday, August 2, 2019 5:48:19 AM CEST Viresh Kumar wrote: > On 01-08-19, 10:57, Doug Smythies wrote: > > On 2019.07.31 23:17 Viresh Kumar wrote: > > > On 31-07-19, 17:20, Doug Smythies wrote: > > >> Summary: > > >> > > >> The old way, using UINT_MAX had two purposes: first, > > >> as a "need to do a frequency update" flag; but also second, to > > >> force any subsequent old/new frequency comparison to NOT be "the same, > > >> so why bother actually updating" (see: sugov_update_next_freq). All > > >> patches so far have been dealing with the flag, but only partially > > >> the comparisons. In a busy system, and when schedutil.c doesn't actually > > >> know the currently set system limits, the new frequency is dominated by > > >> values the same as the old frequency. So, when sugov_fast_switch calls > > >> sugov_update_next_freq, false is usually returned. > > > > > > And finally we know "Why" :) > > > > > > Good work Doug. Thanks for taking it to the end. > > > > > >> However, if we move the resetting of the flag and add another condition > > >> to the "no need to actually update" decision, then perhaps this patch > > >> version 1 will be O.K. It seems to be. (see way later in this e-mail). > > > > > >> With all this new knowledge, how about going back to > > >> version 1 of this patch, and then adding this: > > >> > > >> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > > >> index 808d32b..f9156db 100644 > > >> --- a/kernel/sched/cpufreq_schedutil.c > > >> +++ b/kernel/sched/cpufreq_schedutil.c > > >> @@ -100,7 +100,12 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time) > > >> static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time, > > >> unsigned int next_freq) > > >> { > > >> - if (sg_policy->next_freq == next_freq) > > >> + /* > > >> + * Always force an update if the flag is set, regardless. > > >> + * In some implementations (intel_cpufreq) the frequency is clamped > > >> + * further downstream, and might not actually be different here. > > >> + */ > > >> + if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update) > > >> return false; > > > > > > This is not correct because this is an optimization we have in place > > > to make things more efficient. And it was working by luck earlier and > > > my patch broke it for good :) > > > > Disagree. > > All I did was use a flag where it used to be set to UNIT_MAX, to basically > > implement the same thing. > > And the earlier code wasn't fully correct as well, that's why we tried > to fix it earlier. Your argument seems to be "There was an earlier problem related to this, which was fixed, so it is fragile and I'd rather avoid it". Still, you are claiming that the code was in fact incorrect and you are not giving convincing arguments to support that. > So introducing the UINT_MAX thing again would be > wrong, even if it fixes the problem for you. Would it be wrong, because it would reintroduce the fragile code, or would it be wrong, because it would re-introduce a bug? What bug if so? > Also this won't fix the issue for rest of the governors but just > schedutil. Because this is a driver only problem and there is no point > trying to fix that in a governor. Well, I'm not convinced that this is a driver problem yet.