From: Thomas Renninger <trenn@suse.de>
To: "Natarajan, Janakarajan" <Janakarajan.Natarajan@amd.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
Pu Wen <puwen@hygon.com>, Shuah Khan <shuah@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Kate Stewart <kstewart@linuxfoundation.org>,
Allison Randal <allison@lohutok.net>,
Richard Fontana <rfontana@redhat.com>,
Borislav Petkov <bp@suse.de>
Subject: Re: [PATCH 1/2] Modify cpupower to schedule itself on cores it is reading MSRs from
Date: Thu, 10 Oct 2019 13:22:21 +0200 [thread overview]
Message-ID: <3292474.drSXM59XT9@skinner.arch.suse.de> (raw)
In-Reply-To: <f2dc183f-68a5-d98d-7758-bad224578737@amd.com>
On Monday, October 7, 2019 11:11:30 PM CEST Natarajan, Janakarajan wrote:
> On 10/5/2019 7:40 AM, Thomas Renninger wrote:
>
...
> >>
> >> APERF/MPERF from CPL > 0) and avoid using the msr module (patch 2).
> >
> > And this one only exists on latest AMD cpus, right?
>
> Yes. The RDPRU instruction exists only on AMD cpus.
> >
> >> However, for systems that provide an instruction to get register values
> >> from userspace, would a command-line parameter be acceptable?
> >
> > Parameter sounds like a good idea. In fact, there already is such a
> > paramter.
cpupower monitor --help
> >
> > -c
> >
> > Schedule the process on every core before starting and
> > ending
> >
> > measuring. This could be needed for the Idle_Stats monitor when no other
> > MSR based monitor (has to be run on the core that is measured) is run in
> > parallel. This is to wake up the processors from deeper sleep states and
> > let the kernel reaccount its cpuidle (C-state) information before reading
> > the cpuidle timings from sysfs.
> >
> > Best is you exchange the order of your patches. The 2nd looks rather
> > straight forward and you can add my reviewed-by.
>
> The RDPRU instruction reads the APERF/MPERF of the cpu on which it is
> running. If we do not schedule it on each cpu specifically, it will read the APERF/MPERF
> of the cpu in which it runs/might happen to run on, which will not be the correct behavior.
Got it. And I also didn't fully read -c. I now remember.. For C-states accounting
you want to have each CPU woken up at measure start and end for accurate measuring.
It's a pity that the monitors do the per_cpu calls themselves.
So a general idle-monitor param is not possible or can only done by for example by
adding a flag to the cpuidle_monitor struct:
struct cpuidle_monitor
unsigned int needs_root:1
unsigned int per_cpu_schedule:1
not sure whether a:
struct {
unsigned int needs_root:1
unsigned int per_cpu_schedule:1
} flags
should/must be put around in a separate cleanup patch (and needs_root users adjusted).
You (and other monitors for which this might make sense) can then implement
the per_cpu_schedule flag. In AMD case you might want (you have to)
directly set it.
All around a -b/-u (--bind-measure-to-cpu, --unbind-measure-to-cpu)
parameter could be added at some point of time if it matters. And monitors
having this could bind or not.
This possibly could nuke out -c param. Or at least the idle state counter
monitor could do it itself. But don't mind about this.
What do you think?
And you should be able to re-use the bind_cpu function used in -c case?
Thomas
next prev parent reply other threads:[~2019-10-10 11:22 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-18 16:34 [PATCH 1/2] Modify cpupower to schedule itself on cores it is reading MSRs from Natarajan, Janakarajan
2019-09-18 16:34 ` [PATCH 2/2] Update cpupower to use the RDPRU instruction Natarajan, Janakarajan
2019-09-27 16:07 ` [PATCH 1/2] Modify cpupower to schedule itself on cores it is reading MSRs from Natarajan, Janakarajan
2019-09-27 21:48 ` Thomas Renninger
2019-10-02 14:45 ` Natarajan, Janakarajan
2019-10-05 12:40 ` Thomas Renninger
2019-10-07 21:11 ` Natarajan, Janakarajan
2019-10-10 11:22 ` Thomas Renninger [this message]
2019-10-11 16:58 ` Natarajan, Janakarajan
2019-09-27 18:59 ` shuah
2019-09-30 15:34 ` Natarajan, Janakarajan
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=3292474.drSXM59XT9@skinner.arch.suse.de \
--to=trenn@suse.de \
--cc=Janakarajan.Natarajan@amd.com \
--cc=allison@lohutok.net \
--cc=bp@suse.de \
--cc=gregkh@linuxfoundation.org \
--cc=kstewart@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=puwen@hygon.com \
--cc=rfontana@redhat.com \
--cc=shuah@kernel.org \
--cc=tglx@linutronix.de \
/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).