From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Zhang Rui <rui.zhang@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Linux PM <linux-pm@vger.kernel.org>,
"Pandruvada, Srinivas" <srinivas.pandruvada@intel.com>
Subject: Re: [PATCH 04/13] intel_rapl: introduce struct rapl_private
Date: Tue, 2 Jul 2019 23:44:04 +0200 [thread overview]
Message-ID: <CAJZ5v0he3Qs_nZmTN9fcgVAnurwXOb+Dj+NnTvUqT8TCH3oNHQ@mail.gmail.com> (raw)
In-Reply-To: <1561701029-3415-5-git-send-email-rui.zhang@intel.com>
On Fri, Jun 28, 2019 at 7:50 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> Introduce a new structure, rapl_private, to save the private data
> for different RAPL Interface.
The new structure is called rapl_priv in the patch.
Besides, I would call it rapl_if_priv to indicate that this related to
an interface.
Also, IMO there should be a kerneldoc comment for it.
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/powercap/intel_rapl.c | 58 +++++++++++++++++++++----------------------
> include/linux/intel_rapl.h | 7 ++++++
> 2 files changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
> index adb35ec..a804370 100644
> --- a/drivers/powercap/intel_rapl.c
> +++ b/drivers/powercap/intel_rapl.c
> @@ -75,6 +75,8 @@ enum unit_type {
> TIME_UNIT,
> };
>
> +static struct rapl_priv rapl_msr_priv;
Please add a comment describing this variable and its role.
BTW, I'd rather call it rapl_if_msr.
> +
> /* per domain data, some are optional */
> #define NR_RAW_PRIMITIVES (NR_RAPL_PRIMITIVES - 2)
>
> @@ -155,17 +157,14 @@ static const char * const rapl_domain_names[] = {
> "psys",
> };
>
> -static struct powercap_control_type *control_type; /* PowerCap Controller */
> -static struct rapl_domain *platform_rapl_domain; /* Platform (PSys) domain */
Why do these things need to go into rapl_priv?
> -
> /* caller to ensure CPU hotplug lock is held */
> -static struct rapl_package *rapl_find_package_domain(int cpu)
> +static struct rapl_package *rapl_find_package_domain(int cpu, struct rapl_priv *priv)
> {
> int id = topology_logical_die_id(cpu);
> struct rapl_package *rp;
>
> list_for_each_entry(rp, &rapl_packages, plist) {
> - if (rp->id == id)
> + if (rp->id == id && rp->priv->control_type == priv->control_type)
> return rp;
> }
>
> @@ -1090,12 +1089,12 @@ static void rapl_update_domain_data(struct rapl_package *rp)
>
> static void rapl_unregister_powercap(void)
> {
> - if (platform_rapl_domain) {
> - powercap_unregister_zone(control_type,
> - &platform_rapl_domain->power_zone);
> - kfree(platform_rapl_domain);
> + if (&rapl_msr_priv.platform_rapl_domain) {
> + powercap_unregister_zone(rapl_msr_priv.control_type,
> + &rapl_msr_priv.platform_rapl_domain->power_zone);
> + kfree(rapl_msr_priv.platform_rapl_domain);
> }
> - powercap_unregister_control_type(control_type);
> + powercap_unregister_control_type(rapl_msr_priv.control_type);
> }
>
> static int rapl_package_register_powercap(struct rapl_package *rp)
> @@ -1113,7 +1112,7 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
> nr_pl = find_nr_power_limit(rd);
> pr_debug("register package domain %s\n", rp->name);
> power_zone = powercap_register_zone(&rd->power_zone,
> - control_type,
> + rp->priv->control_type,
> rp->name, NULL,
> &zone_ops[rd->id],
> nr_pl,
> @@ -1140,7 +1139,7 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
> /* number of power limits per domain varies */
> nr_pl = find_nr_power_limit(rd);
> power_zone = powercap_register_zone(&rd->power_zone,
> - control_type, rd->name,
> + rp->priv->control_type, rd->name,
> rp->power_zone,
> &zone_ops[rd->id], nr_pl,
> &constraint_ops);
> @@ -1161,7 +1160,7 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
> */
> while (--rd >= rp->domains) {
> pr_debug("unregister %s domain %s\n", rp->name, rd->name);
> - powercap_unregister_zone(control_type, &rd->power_zone);
> + powercap_unregister_zone(rp->priv->control_type, &rd->power_zone);
> }
>
> return ret;
> @@ -1191,9 +1190,9 @@ static int __init rapl_register_psys(void)
> rd->rpl[0].name = pl1_name;
> rd->rpl[1].prim_id = PL2_ENABLE;
> rd->rpl[1].name = pl2_name;
> - rd->rp = rapl_find_package_domain(0);
> + rd->rp = rapl_find_package_domain(0, &rapl_msr_priv);
>
> - power_zone = powercap_register_zone(&rd->power_zone, control_type,
> + power_zone = powercap_register_zone(&rd->power_zone, rapl_msr_priv.control_type,
> "psys", NULL,
> &zone_ops[RAPL_DOMAIN_PLATFORM],
> 2, &constraint_ops);
> @@ -1203,17 +1202,17 @@ static int __init rapl_register_psys(void)
> return PTR_ERR(power_zone);
> }
>
> - platform_rapl_domain = rd;
> + rapl_msr_priv.platform_rapl_domain = rd;
>
> return 0;
> }
>
> static int __init rapl_register_powercap(void)
> {
> - control_type = powercap_register_control_type(NULL, "intel-rapl", NULL);
> - if (IS_ERR(control_type)) {
> + rapl_msr_priv.control_type = powercap_register_control_type(NULL, "intel-rapl", NULL);
> + if (IS_ERR(rapl_msr_priv.control_type)) {
> pr_debug("failed to register powercap control_type.\n");
> - return PTR_ERR(control_type);
> + return PTR_ERR(rapl_msr_priv.control_type);
> }
> return 0;
> }
> @@ -1338,16 +1337,16 @@ static void rapl_remove_package(struct rapl_package *rp)
> }
> pr_debug("remove package, undo power limit on %s: %s\n",
> rp->name, rd->name);
> - powercap_unregister_zone(control_type, &rd->power_zone);
> + powercap_unregister_zone(rp->priv->control_type, &rd->power_zone);
> }
> /* do parent zone last */
> - powercap_unregister_zone(control_type, &rd_package->power_zone);
> + powercap_unregister_zone(rp->priv->control_type, &rd_package->power_zone);
> list_del(&rp->plist);
> kfree(rp);
> }
>
> /* called from CPU hotplug notifier, hotplug lock held */
> -static struct rapl_package *rapl_add_package(int cpu)
> +static struct rapl_package *rapl_add_package(int cpu, struct rapl_priv *priv)
> {
> int id = topology_logical_die_id(cpu);
> struct rapl_package *rp;
> @@ -1361,6 +1360,7 @@ static struct rapl_package *rapl_add_package(int cpu)
> /* add the new package to the list */
> rp->id = id;
> rp->lead_cpu = cpu;
> + rp->priv = priv;
>
> if (topology_max_die_per_package() > 1)
> snprintf(rp->name, PACKAGE_DOMAIN_NAME_LENGTH,
> @@ -1399,9 +1399,9 @@ static int rapl_cpu_online(unsigned int cpu)
> {
> struct rapl_package *rp;
>
> - rp = rapl_find_package_domain(cpu);
> + rp = rapl_find_package_domain(cpu, &rapl_msr_priv);
> if (!rp) {
> - rp = rapl_add_package(cpu);
> + rp = rapl_add_package(cpu, &rapl_msr_priv);
> if (IS_ERR(rp))
> return PTR_ERR(rp);
> }
> @@ -1414,7 +1414,7 @@ static int rapl_cpu_down_prep(unsigned int cpu)
> struct rapl_package *rp;
> int lead_cpu;
>
> - rp = rapl_find_package_domain(cpu);
> + rp = rapl_find_package_domain(cpu, &rapl_msr_priv);
> if (!rp)
> return 0;
>
> @@ -1427,8 +1427,6 @@ static int rapl_cpu_down_prep(unsigned int cpu)
> return 0;
> }
>
> -static enum cpuhp_state pcap_rapl_online;
> -
> static void power_limit_state_save(void)
> {
> struct rapl_package *rp;
> @@ -1538,7 +1536,7 @@ static int __init rapl_init(void)
> rapl_cpu_online, rapl_cpu_down_prep);
> if (ret < 0)
> goto err_unreg;
> - pcap_rapl_online = ret;
> + rapl_msr_priv.pcap_rapl_online = ret;
>
> /* Don't bail out if PSys is not supported */
> rapl_register_psys();
> @@ -1550,7 +1548,7 @@ static int __init rapl_init(void)
> return 0;
>
> err_unreg_all:
> - cpuhp_remove_state(pcap_rapl_online);
> + cpuhp_remove_state(rapl_msr_priv.pcap_rapl_online);
>
> err_unreg:
> rapl_unregister_powercap();
> @@ -1560,7 +1558,7 @@ static int __init rapl_init(void)
> static void __exit rapl_exit(void)
> {
> unregister_pm_notifier(&rapl_pm_notifier);
> - cpuhp_remove_state(pcap_rapl_online);
> + cpuhp_remove_state(rapl_msr_priv.pcap_rapl_online);
> rapl_unregister_powercap();
> }
>
> diff --git a/include/linux/intel_rapl.h b/include/linux/intel_rapl.h
> index 9471603..d6a8547 100644
> --- a/include/linux/intel_rapl.h
> +++ b/include/linux/intel_rapl.h
> @@ -88,6 +88,12 @@ struct rapl_domain {
> struct rapl_package *rp;
> };
>
> +struct rapl_priv {
> + struct powercap_control_type *control_type;
> + struct rapl_domain *platform_rapl_domain;
> + enum cpuhp_state pcap_rapl_online;
> +};
> +
> /* maximum rapl package domain name: package-%d-die-%d */
> #define PACKAGE_DOMAIN_NAME_LENGTH 30
>
> @@ -108,6 +114,7 @@ struct rapl_package {
> /* Track active cpus */
> struct cpumask cpumask;
> char name[PACKAGE_DOMAIN_NAME_LENGTH];
> + struct rapl_priv *priv;
> };
>
> #endif /* __INTEL_RAPL_H__ */
> --
> 2.7.4
>
next prev parent reply other threads:[~2019-07-03 0:30 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-28 5:50 [PATCH 00/13] intel_rapl: RAPL abstraction and MMIO RAPL support Zhang Rui
2019-06-28 5:50 ` [PATCH 01/13] intel_rapl: use reg instead of msr Zhang Rui
2019-06-28 5:50 ` [PATCH 02/13] intel_rapl: remove hardcoded register index Zhang Rui
2019-06-28 5:50 ` [PATCH 03/13] intel_rapl: introduce intel_rapl.h Zhang Rui
2019-07-02 22:01 ` Rafael J. Wysocki
2019-07-02 22:13 ` Pandruvada, Srinivas
2019-07-02 23:26 ` Rafael J. Wysocki
2019-06-28 5:50 ` [PATCH 04/13] intel_rapl: introduce struct rapl_private Zhang Rui
2019-07-02 21:44 ` Rafael J. Wysocki [this message]
2019-07-03 8:14 ` Zhang Rui
2019-06-28 5:50 ` [PATCH 05/13] intel_rapl: abstract register address Zhang Rui
2019-06-28 5:50 ` [PATCH 06/13] intel_rapl: abstract register access operations Zhang Rui
2019-07-02 21:56 ` Rafael J. Wysocki
2019-07-03 8:14 ` Zhang Rui
2019-06-28 5:50 ` [PATCH 07/13] intel_rapl: cleanup some functions Zhang Rui
2019-06-28 5:50 ` [PATCH 08/13] intel_rapl: cleanup hardcoded MSR access Zhang Rui
2019-06-28 5:50 ` [PATCH 09/13] intel_rapl: abstract RAPL common code Zhang Rui
2019-07-02 21:59 ` Rafael J. Wysocki
2019-07-03 8:23 ` Zhang Rui
2019-07-03 9:02 ` Rafael J. Wysocki
2019-07-02 22:45 ` Pandruvada, Srinivas
2019-06-28 5:50 ` [PATCH 10/13] intel_rapl: support 64 bit register Zhang Rui
2019-06-28 5:50 ` [PATCH 11/13] intel_rapl: support two power limits for every RAPL domain Zhang Rui
2019-06-28 5:50 ` [PATCH 12/13] int340X/processor_thermal_device: add support for MMIO RAPL Zhang Rui
2019-06-28 5:50 ` [PATCH 13/13] intel_rapl: Fix module autoloading issue Zhang Rui
-- strict thread matches above, loose matches on Subject: below --
2019-06-25 15:16 [PATCH 00/13] intel_rapl: RAPL abstraction and MMIO RAPL support Zhang Rui
2019-06-25 15:16 ` [PATCH 04/13] intel_rapl: introduce struct rapl_private Zhang Rui
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=CAJZ5v0he3Qs_nZmTN9fcgVAnurwXOb+Dj+NnTvUqT8TCH3oNHQ@mail.gmail.com \
--to=rafael@kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rjw@rjwysocki.net \
--cc=rui.zhang@intel.com \
--cc=srinivas.pandruvada@intel.com \
/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).