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=-11.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 A8441C2D0E2 for ; Thu, 24 Sep 2020 09:25:50 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5F47F2376F for ; Thu, 24 Sep 2020 09:25:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727380AbgIXJZt (ORCPT ); Thu, 24 Sep 2020 05:25:49 -0400 Received: from foss.arm.com ([217.140.110.172]:39694 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727322AbgIXJZr (ORCPT ); Thu, 24 Sep 2020 05:25:47 -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 1D9AF11D4; Thu, 24 Sep 2020 02:25:46 -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 6F26B3F73B; Thu, 24 Sep 2020 02:25:44 -0700 (PDT) Subject: Re: [PATCH V2 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition() To: "Rafael J. Wysocki" , Viresh Kumar Cc: Rafael Wysocki , Linux PM , Vincent Guittot , cristian.marussi@arm.com, Sudeep Holla , Linux Kernel Mailing List References: <31999d801bfb4d8063dc1ceec1234b6b80b4ae68.1600238586.git.viresh.kumar@linaro.org> From: Lukasz Luba Message-ID: Date: Thu, 24 Sep 2020 10:25:42 +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: 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 Hi Rafael, On 9/23/20 2:48 PM, Rafael J. Wysocki wrote: > On Wed, Sep 16, 2020 at 8:45 AM Viresh Kumar wrote: >> >> In order to prepare for lock-less stats update, add support to defer any >> updates to it until cpufreq_stats_record_transition() is called. > > This is a bit devoid of details. > > I guess you mean reset in particular, but that's not clear from the above. > > Also, it would be useful to describe the design somewhat. > >> Signed-off-by: Viresh Kumar >> --- >> drivers/cpufreq/cpufreq_stats.c | 75 ++++++++++++++++++++++++--------- >> 1 file changed, 56 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c >> index 94d959a8e954..3e7eee29ee86 100644 >> --- a/drivers/cpufreq/cpufreq_stats.c >> +++ b/drivers/cpufreq/cpufreq_stats.c >> @@ -22,17 +22,22 @@ struct cpufreq_stats { >> spinlock_t lock; >> unsigned int *freq_table; >> unsigned int *trans_table; >> + >> + /* Deferred reset */ >> + unsigned int reset_pending; >> + unsigned long long reset_time; >> }; >> >> -static void cpufreq_stats_update(struct cpufreq_stats *stats) >> +static void cpufreq_stats_update(struct cpufreq_stats *stats, >> + unsigned long long time) >> { >> unsigned long long cur_time = get_jiffies_64(); >> >> - stats->time_in_state[stats->last_index] += cur_time - stats->last_time; >> + stats->time_in_state[stats->last_index] += cur_time - time; >> stats->last_time = cur_time; >> } >> >> -static void cpufreq_stats_clear_table(struct cpufreq_stats *stats) >> +static void cpufreq_stats_reset_table(struct cpufreq_stats *stats) >> { >> unsigned int count = stats->max_state; >> >> @@ -41,42 +46,67 @@ static void cpufreq_stats_clear_table(struct cpufreq_stats *stats) >> memset(stats->trans_table, 0, count * count * sizeof(int)); >> stats->last_time = get_jiffies_64(); >> stats->total_trans = 0; >> + >> + /* Adjust for the time elapsed since reset was requested */ >> + WRITE_ONCE(stats->reset_pending, 0); > > What if this runs in parallel with store_reset()? > > The latter may update reset_pending to 1 before the below runs. > Conversely, this may clear reset_pending right after store_reset() has > set it to 1, but before it manages to set reset_time. Is that not a > problem? 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? 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. Regards, Lukasz