From patchwork Wed May 9 08:56:11 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Rafael J. Wysocki" X-Patchwork-Id: 10388773 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 47673602C2 for ; Wed, 9 May 2018 08:57:39 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 393FB288CD for ; Wed, 9 May 2018 08:57:39 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2E2F728E3F; Wed, 9 May 2018 08:57:39 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8B328288CD for ; Wed, 9 May 2018 08:57:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934482AbeEII4W (ORCPT ); Wed, 9 May 2018 04:56:22 -0400 Received: from mail-oi0-f67.google.com ([209.85.218.67]:38955 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934407AbeEII4N (ORCPT ); Wed, 9 May 2018 04:56:13 -0400 Received: by mail-oi0-f67.google.com with SMTP id n65-v6so30830533oig.6; Wed, 09 May 2018 01:56:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=Qpz8TzaRqVXe6hcgZRFtUE7OA0rsR8mIKzNNMVMbiLc=; b=jzUxfPbHOHm9D91OxIUadjYq55hBl5qokN75GSRQBrGviWEySEbfqhrCus39MRdOXG jt0xw5FhDA/n5OOjg7FsSdfFDvxjAESwjdelqKscOAL5jEgpEu9Gi7dpPgE3/RZi4umY eT9nL32tVtSRqR3LLGOWJ29Pn5/ezgn3JVA4YediaogfozbMINCN/DUNKFkcjVbE8d8P C0LWgM/FKe+mty0s5c9wnCqxF2204Ege3J47FvM5ovYppK/qvpigTFCbp4ZW44tu976B xGd1INHKHWdOp8za/++hsqH4mq8OzGQ2AtKqcDFWzUyAYb7CEIm/BR5r+X2dDjiZVjyX HEww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=Qpz8TzaRqVXe6hcgZRFtUE7OA0rsR8mIKzNNMVMbiLc=; b=Vt1Vj1V6WyaiqgsHL5ka6G1vnn7+Y/PbhmI0otrmKAuS+XdWrY9xVXQFWrIYwaJPip bOiD8iN5QWs7Dhw3l9VEaXM/X7GqTxRX3Os/8qWq/xWaXNy0d/YjdQCh/aH7y3jqK+H7 zY2Cr112IrtsatB1hn9dV6ipvCWdA8FaOrbxYbIqeFPDqhf3A66MQLwZC0JLK01VzcP5 6D2mPb1YJZLaCVWxnNeNjWq6lMdqDPBSe33ENedgTZ2eV4xs8lKUXy5itjGvYzrI19L/ 4nPbIBzwC0DISpc6PyCCPIGQu4R5n5BjlzhTnQx5XI4W51tbViq4FUhNjEC3PQ3XkaN2 qirA== X-Gm-Message-State: ALQs6tAuuk/w3fhTv97RugOH5KAGD7/W7h8Atox3VbMOkUosQ/LOltX3 r63umg9pU0vUL1no7cZ4QmchVwCTxRMEbMzDC4k= X-Google-Smtp-Source: AB8JxZougbbAUo/VGOQZc99D/K2r7O6FIYuRFde4rt3P2wuf8bXYP8GZXyy0YQOs1bDpG0/pk+yKjpb2nqgitf/eqJ0= X-Received: by 2002:aca:681:: with SMTP id 123-v6mr19167926oig.358.1525856172496; Wed, 09 May 2018 01:56:12 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a9d:1468:0:0:0:0:0 with HTTP; Wed, 9 May 2018 01:56:11 -0700 (PDT) In-Reply-To: <20180509084128.s3nu57njyep4tw2w@vireshk-i7> References: <872c3f8690d9362820639d91a807e535f10a9a36.1525761635.git.viresh.kumar@linaro.org> <20180509084128.s3nu57njyep4tw2w@vireshk-i7> From: "Rafael J. Wysocki" Date: Wed, 9 May 2018 10:56:11 +0200 X-Google-Sender-Auth: 48xCDnMXOnadqWQxv_Bhs4LIamw Message-ID: Subject: Re: [PATCH] sched/schedutil: Don't set next_freq to UINT_MAX To: Viresh Kumar Cc: "Rafael J. Wysocki" , Rafael Wysocki , Ingo Molnar , Peter Zijlstra , Linux PM , Vincent Guittot , Claudio Scordino , Patrick Bellasi , Juri Lelli , Joel Fernandes , "4 . 12+" , Linux Kernel Mailing List Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, May 9, 2018 at 10:41 AM, Viresh Kumar wrote: > On 08-05-18, 22:54, Rafael J. Wysocki wrote: >> On Tue, May 8, 2018 at 8:42 AM, Viresh Kumar wrote: >> > The schedutil driver sets sg_policy->next_freq to UINT_MAX on certain >> > occasions: >> > - In sugov_start(), when the schedutil governor is started for a group >> > of CPUs. >> > - And whenever we need to force a freq update before rate-limit >> > duration, which happens when: >> > - there is an update in cpufreq policy limits. >> > - Or when the utilization of DL scheduling class increases. >> > >> > In return, get_next_freq() doesn't return a cached next_freq value but >> > instead recalculates the next frequency. This has some side effects >> > though and may significantly delay a required increase in frequency. >> > >> > In sugov_update_single() we try to avoid decreasing frequency if the CPU >> > has not been idle recently. Consider this scenario, the available range >> > of frequencies for a CPU are from 800 MHz to 2.5 GHz and current >> > frequency is 800 MHz. From one of the call paths >> > sg_policy->need_freq_update is set to true and hence >> > sg_policy->next_freq is set to UINT_MAX. Now if the CPU had been busy, >> > next_f will always be less than UINT_MAX, whatever the value of next_f >> > is. And so even when we wanted to increase the frequency, we will >> > overwrite next_f with UINT_MAX and will not change the frequency >> > eventually. This will continue until the time CPU stays busy. This isn't >> > cross checked with any specific test cases, but rather based on general >> > code review. >> > >> > Fix that by not resetting the sg_policy->need_freq_update flag from >> > sugov_should_update_freq() but get_next_freq() and we wouldn't need to >> > overwrite sg_policy->next_freq anymore. >> > >> > Cc: 4.12+ # 4.12+ >> > Fixes: b7eaf1aab9f8 ("cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely") >> >> The rest of the chantelog is totally disconnected from this commit. > > I added the "Fixes" tag because this is exactly the commit after which > this problem started, isn't it? > >> So the problem is that sugov_update_single() doesn't check the special >> UNIT_MAX value before assigning sg_policy->next_freq to next_f. Fair >> enough. >> >> I don't see why the patch is the right fix for that, however. > > I thought not overwriting next_freq makes things much simpler and easy > to review. I'm kind of concerned about updating the limits via sysfs in which case the cached next frequency may be out of range, so it's better to invalidate it right away then. > What else do you have in mind to solve this problem ? Something like the below? --- kernel/sched/cpufreq_schedutil.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Index: linux-pm/kernel/sched/cpufreq_schedutil.c =================================================================== --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c +++ linux-pm/kernel/sched/cpufreq_schedutil.c @@ -305,7 +305,8 @@ static void sugov_update_single(struct u * Do not reduce the frequency if the CPU has not been idle * recently, as the reduction is likely to be premature then. */ - if (busy && next_f < sg_policy->next_freq) { + if (busy && next_f < sg_policy->next_freq && + sg_policy->next_freq != UINT_MAX) { next_f = sg_policy->next_freq; /* Reset cached freq as next_freq has changed */