From: Giovanni Gherdovich <ggherdovich@suse.cz>
To: Huang Rui <ray.huang@amd.com>,
"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
Viresh Kumar <viresh.kumar@linaro.org>,
Shuah Khan <skhan@linuxfoundation.org>,
Borislav Petkov <bp@suse.de>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@kernel.org>,
linux-pm@vger.kernel.org
Cc: Deepak Sharma <deepak.sharma@amd.com>,
Alex Deucher <alexander.deucher@amd.com>,
Mario Limonciello <mario.limonciello@amd.com>,
Nathan Fontenot <nathan.fontenot@amd.com>,
Jinzhou Su <Jinzhou.Su@amd.com>,
Xiaojian Du <Xiaojian.Du@amd.com>,
linux-kernel@vger.kernel.org, x86@kernel.org
Subject: Re: [PATCH v2 08/21] cpufreq: amd: add trace for amd-pstate module
Date: Wed, 06 Oct 2021 10:12:33 +0200 [thread overview]
Message-ID: <3ec0387f2ecf0121eb59c054e2633afa2503ef6b.camel@suse.cz> (raw)
In-Reply-To: <20210926090605.3556134-9-ray.huang@amd.com>
On Sun, 2021-09-26 at 17:05 +0800, Huang Rui wrote:
> Add trace event to monitor the performance value changes which is
> controlled by cpu governors.
>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> drivers/cpufreq/Makefile | 6 +-
> drivers/cpufreq/amd-pstate-trace.c | 2 +
> drivers/cpufreq/amd-pstate-trace.h | 96 ++++++++++++++++++++++++++++++
> drivers/cpufreq/amd-pstate.c | 11 ++++
> 4 files changed, 114 insertions(+), 1 deletion(-)
> create mode 100644 drivers/cpufreq/amd-pstate-trace.c
> create mode 100644 drivers/cpufreq/amd-pstate-trace.h
>
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 5c9a2a1ee8dc..04882bc4b145 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -17,6 +17,10 @@ obj-$(CONFIG_CPU_FREQ_GOV_ATTR_SET) += cpufreq_governor_attr_set.o
> obj-$(CONFIG_CPUFREQ_DT) += cpufreq-dt.o
> obj-$(CONFIG_CPUFREQ_DT_PLATDEV) += cpufreq-dt-platdev.o
>
> +# Traces
> +CFLAGS_amd-pstate-trace.o := -I$(src)
> +amd_pstate-y := amd-pstate.o amd-pstate-trace.o
> +
> ##################################################################################
> # x86 drivers.
> # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
> @@ -25,7 +29,7 @@ obj-$(CONFIG_CPUFREQ_DT_PLATDEV) += cpufreq-dt-platdev.o
> # speedstep-* is preferred over p4-clockmod.
>
> obj-$(CONFIG_X86_ACPI_CPUFREQ) += acpi-cpufreq.o
> -obj-$(CONFIG_X86_AMD_PSTATE) += amd-pstate.o
> +obj-$(CONFIG_X86_AMD_PSTATE) += amd_pstate.o
> obj-$(CONFIG_X86_POWERNOW_K8) += powernow-k8.o
> obj-$(CONFIG_X86_PCC_CPUFREQ) += pcc-cpufreq.o
> obj-$(CONFIG_X86_POWERNOW_K6) += powernow-k6.o
> diff --git a/drivers/cpufreq/amd-pstate-trace.c b/drivers/cpufreq/amd-pstate-trace.c
> new file mode 100644
> index 000000000000..891b696dcd69
> --- /dev/null
> +++ b/drivers/cpufreq/amd-pstate-trace.c
> @@ -0,0 +1,2 @@
> +#define CREATE_TRACE_POINTS
> +#include "amd-pstate-trace.h"
> diff --git a/drivers/cpufreq/amd-pstate-trace.h b/drivers/cpufreq/amd-pstate-trace.h
> new file mode 100644
> index 000000000000..50c85e150f30
> --- /dev/null
> +++ b/drivers/cpufreq/amd-pstate-trace.h
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * amd-pstate-trace.h - AMD Processor P-state Frequency Driver Tracer
> + *
> + * Copyright (C) 2021 Advanced Micro Devices, Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + *
> + * Author: Huang Rui <ray.huang@amd.com>
> + */
> +
> +#if !defined(_AMD_PSTATE_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _AMD_PSTATE_TRACE_H
> +
> +#include <linux/cpufreq.h>
> +#include <linux/tracepoint.h>
> +#include <linux/trace_events.h>
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM amd_cpu
Hello Ray,
I'd prefer if TRACE_SYSTEM was set to "power". In that way the tracepoint is easier
to find, since it'd be together with other power-related tracepoints. I often do
perf list | grep "power:"
to find all that's available, or equivalently
ls $TRACEFS/events/power/
and if your tracepoint is somewhere else, I wouldn't find it.
> +
> +#undef TRACE_INCLUDE_FILE
> +#define TRACE_INCLUDE_FILE amd-pstate-trace
> +
> +#define TPS(x) tracepoint_string(x)
> +
> +TRACE_EVENT(amd_pstate_perf,
> +
> + TP_PROTO(unsigned long min_perf,
> + unsigned long target_perf,
> + unsigned long capacity,
> + unsigned int cpu_id,
> + u64 prev,
> + u64 value,
> + int type
> + ),
> +
> + TP_ARGS(min_perf,
> + target_perf,
> + capacity,
> + cpu_id,
> + prev,
> + value,
> + type
> + ),
> +
> + TP_STRUCT__entry(
> + __field(unsigned long, min_perf)
> + __field(unsigned long, target_perf)
> + __field(unsigned long, capacity)
> + __field(unsigned int, cpu_id)
> + __field(u64, prev)
> + __field(u64, value)
> + __field(int, type)
> + ),
> +
> + TP_fast_assign(
> + __entry->min_perf = min_perf;
> + __entry->target_perf = target_perf;
> + __entry->capacity = capacity;
> + __entry->cpu_id = cpu_id;
> + __entry->prev = prev;
> + __entry->value = value;
> + __entry->type = type;
> + ),
> +
> + TP_printk("amd_min_perf=%lu amd_des_perf=%lu amd_max_perf=%lu cpu_id=%u prev=0x%llx value=0x%llx type=0x%d",
> + (unsigned long)__entry->min_perf,
> + (unsigned long)__entry->target_perf,
> + (unsigned long)__entry->capacity,
> + (unsigned int)__entry->cpu_id,
> + (u64)__entry->prev,
> + (u64)__entry->value,
> + (int)__entry->type
> + )
> +);
> +
> +#endif /* _AMD_PSTATE_TRACE_H */
> +
> +/* This part must be outside protection */
> +#undef TRACE_INCLUDE_PATH
> +#define TRACE_INCLUDE_PATH .
> +
> +#include <trace/define_trace.h>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index c9bee7b1698a..0c9f9c0c8928 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -45,10 +45,17 @@
> #include <asm/processor.h>
> #include <asm/cpufeature.h>
> #include <asm/cpu_device_id.h>
> +#include "amd-pstate-trace.h"
>
> #define AMD_PSTATE_TRANSITION_LATENCY 0x20000
> #define AMD_PSTATE_TRANSITION_DELAY 500
>
> +enum switch_type
> +{
> + AMD_TARGET = 0,
> + AMD_ADJUST_PERF
> +};
> +
> static struct cpufreq_driver amd_pstate_driver;
>
> struct amd_cpudata {
> @@ -183,6 +190,7 @@ amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
> {
> u64 prev = READ_ONCE(cpudata->cppc_req_cached);
> u64 value = prev;
> + enum switch_type type = fast_switch ? AMD_ADJUST_PERF : AMD_TARGET;
>
> value &= ~REQ_MIN_PERF(~0L);
> value |= REQ_MIN_PERF(min_perf);
> @@ -193,6 +201,9 @@ amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
> value &= ~REQ_MAX_PERF(~0L);
> value |= REQ_MAX_PERF(max_perf);
>
> + trace_amd_pstate_perf(min_perf, des_perf, max_perf,
> + cpudata->cpu, prev, value, type);
Two things here:
1. the field "value" seems redundant, as you're already showing me {min,des,max}_perf.
Maybe you can remove "value" from the output of the trace?
One reason I can think why you're showing me "value", is to let me see if it's the
same as "prev", in which case I'd know the request isn't passed to the hardware.
Is that so? If that's the reason, maybe it would be clear to remove "value", "prev"
and just show a field like "changed={true,false}".
2. the field "type" is a little obscure for someone reading the trace. It can be
0 or 1, and to know what that means one has to read the code. I would suggest
replacing it with a field "fast_switch={true,false}", which is more telling.
What do you think?
Giovanni
next prev parent reply other threads:[~2021-10-06 8:12 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-26 9:05 [PATCH v2 00/21] cpufreq: introduce a new AMD CPU frequency control mechanism Huang Rui
2021-09-26 9:05 ` [PATCH v2 01/21] x86/cpufreatures: add AMD Collaborative Processor Performance Control feature flag Huang Rui
2021-09-26 9:05 ` [PATCH v2 02/21] x86/msr: add AMD CPPC MSR definitions Huang Rui
2021-09-26 9:05 ` [PATCH v2 03/21] ACPI: CPPC: Check online CPUs for determining _CPC is valid Huang Rui
2021-10-19 16:52 ` Rafael J. Wysocki
2021-10-20 11:15 ` Huang, Ray
2021-09-26 9:05 ` [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function Huang Rui
2021-09-28 17:06 ` Fontenot, Nathan
2021-10-13 12:28 ` Huang, Ray
2021-10-19 16:59 ` Rafael J. Wysocki
2021-10-20 11:13 ` Huang, Ray
2021-10-20 13:32 ` Rafael J. Wysocki
2021-10-21 3:41 ` Huang, Ray
2021-09-26 9:05 ` [PATCH v2 05/21] cpufreq: amd: introduce a new amd pstate driver to support future processors Huang Rui
2021-09-28 20:40 ` Fontenot, Nathan
2021-10-14 10:14 ` Huang, Ray
2021-09-26 9:05 ` [PATCH v2 06/21] cpufreq: amd: add fast switch function for amd-pstate module Huang Rui
2021-09-26 9:05 ` [PATCH v2 07/21] cpufreq: amd: add acpi cppc function as the backend for legacy processors Huang Rui
2021-09-26 9:05 ` [PATCH v2 08/21] cpufreq: amd: add trace for amd-pstate module Huang Rui
2021-10-06 8:12 ` Giovanni Gherdovich [this message]
2021-10-21 10:19 ` Huang, Ray
2021-09-26 9:05 ` [PATCH v2 09/21] cpufreq: amd: add boost mode support for amd-pstate Huang Rui
2021-09-26 9:05 ` [PATCH v2 10/21] cpufreq: amd: add amd-pstate checking support check attribute Huang Rui
2021-09-28 21:24 ` Fontenot, Nathan
2021-10-14 10:26 ` Huang, Ray
2021-09-26 9:05 ` [PATCH v2 11/21] cpufreq: amd: add amd-pstate frequencies attributes Huang Rui
2021-09-28 21:35 ` Fontenot, Nathan
2021-10-14 10:35 ` Huang, Ray
2021-09-26 9:05 ` [PATCH v2 12/21] cpufreq: amd: add amd-pstate performance attributes Huang Rui
2021-09-26 9:05 ` [PATCH v2 13/21] cpupower: add AMD P-state capability flag Huang Rui
2021-09-26 9:05 ` [PATCH v2 14/21] cpupower: add the function to check amd-pstate enabled Huang Rui
2021-09-26 9:05 ` [PATCH v2 15/21] cpupower: initial AMD P-state capability Huang Rui
2021-09-26 9:06 ` [PATCH v2 16/21] cpupower: add the function to get the sysfs value from specific table Huang Rui
2021-09-26 9:06 ` [PATCH v2 17/21] cpupower: add amd-pstate sysfs definition and access helper Huang Rui
2021-09-26 9:06 ` [PATCH v2 18/21] cpupower: enable boost state support for amd-pstate module Huang Rui
2021-09-26 9:06 ` [PATCH v2 19/21] cpupower: move print_speed function into misc helper Huang Rui
2021-09-26 9:06 ` [PATCH v2 20/21] cpupower: print amd-pstate information on cpupower Huang Rui
2021-09-26 9:06 ` [PATCH v2 21/21] Documentation: amd-pstate: add amd-pstate driver introduction Huang Rui
2021-10-13 16:23 ` Giovanni Gherdovich
2021-10-14 11:30 ` Huang, Ray
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3ec0387f2ecf0121eb59c054e2633afa2503ef6b.camel@suse.cz \
--to=ggherdovich@suse.cz \
--cc=Jinzhou.Su@amd.com \
--cc=Xiaojian.Du@amd.com \
--cc=alexander.deucher@amd.com \
--cc=bp@suse.de \
--cc=deepak.sharma@amd.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mario.limonciello@amd.com \
--cc=mingo@kernel.org \
--cc=nathan.fontenot@amd.com \
--cc=peterz@infradead.org \
--cc=rafael.j.wysocki@intel.com \
--cc=ray.huang@amd.com \
--cc=skhan@linuxfoundation.org \
--cc=viresh.kumar@linaro.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).