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.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 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 CA83FC4727D for ; Thu, 24 Sep 2020 16:10:32 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7F0E9206C3 for ; Thu, 24 Sep 2020 16:10:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728619AbgIXQKb (ORCPT ); Thu, 24 Sep 2020 12:10:31 -0400 Received: from foss.arm.com ([217.140.110.172]:50086 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728448AbgIXQKb (ORCPT ); Thu, 24 Sep 2020 12:10:31 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 28D881396; Thu, 24 Sep 2020 09:10:30 -0700 (PDT) Received: from [10.57.51.181] (unknown [10.57.51.181]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4AC1B3F718; Thu, 24 Sep 2020 09:10:28 -0700 (PDT) Subject: Re: [PATCH V2 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition() To: Viresh Kumar Cc: "Rafael J. Wysocki" , Rafael Wysocki , Linux PM , Vincent Guittot , cristian.marussi@arm.com, Sudeep Holla , Linux Kernel Mailing List References: <31999d801bfb4d8063dc1ceec1234b6b80b4ae68.1600238586.git.viresh.kumar@linaro.org> <20200924123921.iiaqw2ufe2utnjtg@vireshk-i7> From: Lukasz Luba Message-ID: Date: Thu, 24 Sep 2020 17:10:26 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <20200924123921.iiaqw2ufe2utnjtg@vireshk-i7> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 9/24/20 1:39 PM, Viresh Kumar wrote: > On 24-09-20, 13:07, Rafael J. Wysocki wrote: >> On Thu, Sep 24, 2020 at 1:00 PM Lukasz Luba wrote: >>> On 9/24/20 11:24 AM, Rafael J. Wysocki wrote: >>>> On Thu, Sep 24, 2020 at 11:25 AM Lukasz Luba wrote: > >>>>> I wonder if we could just drop the reset feature. Is there a tool >>>>> which uses this file? The 'reset' sysfs would probably have to stay >>>>> forever, but an empty implementation is not an option? >>>> >>>> Well, having an empty sysfs attr would be a bit ugly, but the >>>> implementation of it could be simplified. >>>> >>>>> The documentation states: >>>>> 'This can be useful for evaluating system behaviour under different >>>>> governors without the need for a reboot.' >>>>> With the scenario of fast-switch this resetting complicates the >>>>> implementation and the justification of having it just for experiments >>>>> avoiding reboot is IMO weak. The real production code would have to pay >>>>> extra cycles every time. Also, we would probably not experiment with >>>>> cpufreq different governors, since the SchedUtil is considered the best >>>>> option. >>>> >>>> It would still be good to have a way to test it against the other >>>> available options, though. >>>> >>> >>> Experimenting with different governors would still be possible, just >>> the user-space would have to take a snapshot of the stats when switching >>> to a new governor. Then the values presented in the stats would just >>> need to be calculated in this user tool against the snapshot. >>> >>> The resetting is also not that bad, since nowadays more components >>> maintain some kind of local statistics/history (scheduler, thermal). >>> I would recommend to reset the whole system and repeat the same tests >>> with different governor, just to be sure that everything starts from >>> similar state (utilization, temperature, other devfreq devices >>> frequencies etc). >> >> Well, if everyone agrees on removing the reset feature, let's drop the >> sysfs attr too, as it would be useless going forward. >> >> Admittedly, I don't have a strong opinion and since intel_pstate >> doesn't use a frequency table, this is not relevant for systems using >> that driver anyway. > > I added this file sometime back as it made my life a lot easier while testing > some scheduler related changes and see how they affect cpufreq updates. IMO this > is a useful feature and we don't really need to get rid of it. > > Lets see where the discussion goes about the feedback you gave. > Because of supporting this reset file, the code is going to be a bit complex and also visited from the scheduler. I don't know if the config for stats is enabled for production kernels but if yes, then forcing all to keep that reset code might be too much. For the engineering kernel version is OK. I would say for most normal checks these sysfs stats are very useful. If there is a need for investigation like you described, the trace event is there, just have to be enabled. Tools like LISA would help with parsing the trace and mapping to some plots or even merging with scheduler context. From time to time some engineers are asking why the stats don't show the values (missing fast-switch tracking). I think they are interested in a simple use case, otherwise they would use the tracing. Regards, Lukasz