Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <4cbb1eff77de1e843912267ade4686cfa1acd610.camel@kepstin.ca>
Subject: Re: [PATCH v2] tools/power turbostat: Fix RAPL summary collection
 on AMD processors
From:   Calvin Walton <calvin.walton@kepstin.ca>
To:     Chen Yu <yu.c.chen@intel.com>, Borislav Petkov <bp@suse.de>
Cc:     Terry Bowman <terry.bowman@amd.com>, lenb@kernel.org,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        wei.huang2@amd.com, aros@gmx.com
Date:   Tue, 20 Apr 2021 09:28:06 -0400
In-Reply-To: <20210420131541.GA388877@chenyu-desktop>
References: <20210419195812.147710-1-terry.bowman@amd.com>
         <20210420020336.GA386151@chenyu-desktop> <20210420080701.GA2326@zn.tnic>
         <20210420131541.GA388877@chenyu-desktop>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.2 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Tue, 2021-04-20 at 21:15 +0800, Chen Yu wrote:
> 
> Okay. I would vote for the the patch from Bas as it was a combined
> work from two
> authors and tested by several AMD users. But let me paste it here too
> for Artem to
> see if this also works for him:
> 
> 
> From 00e0622b1b693a5c7dc343aeb3aa51614a9e125e Mon Sep 17 00:00:00
> 2001
> From: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Date: Fri, 12 Mar 2021 21:27:40 +0800
> Subject: [PATCH] tools/power/turbostat: Fix turbostat for AMD Zen
> CPUs
> 
> 
> @@ -297,7 +297,10 @@ int idx_to_offset(int idx)
>  
>         switch (idx) {
>         case IDX_PKG_ENERGY:
> -               offset = MSR_PKG_ENERGY_STATUS;
> +               if (do_rapl & RAPL_AMD_F17H)
> +                       offset = MSR_PKG_ENERGY_STAT;
> +               else
> +                       offset = MSR_PKG_ENERGY_STATUS;
>                 break;
>         case IDX_DRAM_ENERGY:
>                 offset = MSR_DRAM_ENERGY_STATUS;

This patch has the same issue I noticed with the initial revision of
Terry's patch - the idx_to_offset function returns type int (32-bit
signed), but MSR_PKG_ENERGY_STAT is greater than INT_MAX (or rather,
would be interpreted as a negative number)

The end result is, as far as I can tell, that it hits the if (offset <
0) check in update_msr_sum() resulting in the timer callback for
updating the stat in the background when long durations are used to not
happen.

For short durations it still works fine since the background update
isn't used.


-- 
Calvin Walton <calvin.walton@kepstin.ca>