linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: rafael@kernel.org, srinivas.pandruvada@linux.intel.com,
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	rui.zhang@intel.com, "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Arnd Bergmann <arnd@arndb.de>,
	"open list:GENERIC INCLUDE/ASM HEADER FILES" 
	<linux-arch@vger.kernel.org>
Subject: Re: [PATCH 3/4] powercap/drivers/dtpm: Add API for dynamic thermal power management
Date: Tue, 3 Nov 2020 19:42:55 +0100	[thread overview]
Message-ID: <18d2393a-2954-f271-817f-f9f9bf651f25@linaro.org> (raw)
In-Reply-To: <4484e771-9011-0928-e961-cd3a53be55e9@arm.com>


Hi Lukasz,

thanks for the review and the comments.

On 23/10/2020 12:29, Lukasz Luba wrote:
> Hi Daniel,

[ ... ]

>> +
>> +config DTPM
>> +    bool "Power capping for dynamic thermal power management"
> 
> Maybe starting with capital letters: Dynamic Thermal Power Management?

Ok, noted.

[ ... ]

>> +++ b/drivers/powercap/dtpm.c
>> @@ -0,0 +1,430 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright 2020 Linaro Limited
>> + *
>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>> + *
>> + * The powercap based Dynamic Thermal Power Management framework
>> + * provides to the userspace a consistent API to set the power limit
>> + * on some devices.
>> + *
>> + * DTPM defines the functions to create a tree of constraints. Each
>> + * parent node is a virtual description of the aggregation of the
>> + * children. It propagates the constraints set at its level to its
>> + * children and collect the children power infomation. The leaves of
> 
> s/infomation/information/

Ok, thanks

[ ... ]

>> +static struct powercap_control_type *pct;
>> +static struct dtpm *root;
> 
> I wonder if it safe to have the tree without a global lock for it, like
> mutex tree_lock ?
> I have put some comments below when the code traverses the tree.

The mutex is a heavy lock and the its purpose is to allow the current
process to be preempted while the spinlock is very fast without preemption.

Putting in place a single lock will simplify the code but I'm not sure
it is worth as it could be a contention. It would be simpler to switch
to a big lock than the opposite.

[ ... ]

>> +static void dtpm_rebalance_weight(void)
>> +{
>> +    __dtpm_rebalance_weight(root);
>> +}
>> +
>> +static void dtpm_sub_power(struct dtpm *dtpm)
>> +{
>> +    struct dtpm *parent = dtpm->parent;
>> +
>> +    while (parent) {
> 
> I am not sure if it is safe for a corner case when the
> nodes are removing from bottom to top. We don't hold a tree
> lock, so these two (above line and below) operations might
> be split/preempted and 'parent' freed before taking the lock.
> Is it possible? (Note: I might missed something like double
> locking using this local node spinlock).

The parent can not be freed until it has children, the check is done in
the release node function.

>> +        spin_lock(&parent->lock);
>> +        parent->power_min -= dtpm->power_min;
>> +        parent->power_max -= dtpm->power_max;
>> +        spin_unlock(&parent->lock);
>> +        parent = parent->parent;
>> +    }
>> +
>> +    dtpm_rebalance_weight();
>> +}
>> +
>> +static void dtpm_add_power(struct dtpm *dtpm)
>> +{
>> +    struct dtpm *parent = dtpm->parent;
>> +
>> +    while (parent) {
> 
> Similar here?
> 
>> +        spin_lock(&parent->lock);
>> +        parent->power_min += dtpm->power_min;
>> +        parent->power_max += dtpm->power_max;
>> +        spin_unlock(&parent->lock);
>> +        parent = parent->parent;
>> +    }
>> +
>> +    dtpm_rebalance_weight();
>> +}
>> +
>> +/**
>> + * dtpm_update_power - Update the power on the dtpm
>> + * @dtpm: a pointer to a dtpm structure to update
>> + * @power_min: a u64 representing the new power_min value
>> + * @power_max: a u64 representing the new power_max value
>> + *
>> + * Function to update the power values of the dtpm node specified in
>> + * parameter. These new values will be propagated to the tree.
>> + *
>> + * Return: zero on success, -EINVAL if the values are inconsistent
>> + */
>> +int dtpm_update_power(struct dtpm *dtpm, u64 power_min, u64 power_max)
>> +{
>> +    if (power_min == dtpm->power_min && power_max == dtpm->power_max)
>> +        return 0;
>> +
>> +    if (power_max < power_min)
>> +        return -EINVAL;
>> +
>> +    dtpm_sub_power(dtpm);
>> +    spin_lock(&dtpm->lock);
>> +    dtpm->power_min = power_min;
>> +    dtpm->power_max = power_max;
>> +    spin_unlock(&dtpm->lock);
>> +    dtpm_add_power(dtpm);
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * dtpm_release_zone - Cleanup when the node is released
>> + * @pcz: a pointer to a powercap_zone structure
>> + *
>> + * Do some housecleaning and update the weight on the tree. The
>> + * release will be denied if the node has children. This function must
>> + * be called by the specific release callback of the different
>> + * backends.
>> + *
>> + * Return: 0 on success, -EBUSY if there are children
>> + */
>> +int dtpm_release_zone(struct powercap_zone *pcz)
>> +{
>> +    struct dtpm *dtpm = to_dtpm(pcz);
>> +    struct dtpm *parent = dtpm->parent;
>> +
> 
> I would lock the whole tree, just to play safe.
> What do you think?

I would like to keep the fine grain locking to prevent a potential
contention. If it appears we hit a locking incorrectness or a race
putting in question the fine grain locking scheme, then we can consider
switching to a tree lock.

>> +    if (!list_empty(&dtpm->children))
>> +        return -EBUSY;
>> +
>> +    if (parent) {
>> +        spin_lock(&parent->lock);
>> +        list_del(&dtpm->sibling);
>> +        spin_unlock(&parent->lock);
>> +    }
>> +
>> +    dtpm_sub_power(dtpm);
>> +
>> +    kfree(dtpm);
>> +
>> +    return 0;
>> +}

[ ... ]

>> +struct dtpm *dtpm_alloc(void)
>> +{
>> +    struct dtpm *dtpm;
>> +
>> +    dtpm = kzalloc(sizeof(*dtpm), GFP_KERNEL);
>> +    if (dtpm) {
>> +        INIT_LIST_HEAD(&dtpm->children);
>> +        INIT_LIST_HEAD(&dtpm->sibling);
>> +        spin_lock_init(&dtpm->lock);
> 
> Why do we use spinlock and not mutex?

The mutex will force the calling process to be preempted, that is useful
when the critical sections contains blocking calls.

Here we are just changing values without blocking calls, so using the
spinlock is more adequate as they are faster.

[ ... ]

>> +static int __init dtpm_init(void)
>> +{
>> +    struct dtpm_descr **dtpm_descr;
>> +
>> +    pct = powercap_register_control_type(NULL, "dtpm", NULL);
>> +    if (!pct) {
>> +        pr_err("Failed to register control type\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    for_each_dtpm_table(dtpm_descr)
>> +        (*dtpm_descr)->init(*dtpm_descr);
> 
> We don't check the returned value here. It is required that the
> devices should already be up and running (like cpufreq).
> But if for some reason the init() failed, maybe it's worth to add a
> field inside the dtpm_desc or dtpm struct like 'bool ready' ?
> It could be retried to init later.

It would be make sense to check the init return value if we want to
rollback what we have done. Here we don't want to do that. If one
subsystem fails to insert itself in the tree, it will log an error but
the tree should continue to give access to what have been successfully
initialized.

The rollback is important in the init() ops, not in dtpm_init().

>> +
>> +    return 0;
>> +}
>> +late_initcall(dtpm_init);
> 
> The framework would start operating at late boot. We don't control
> the thermal/power issues in earier stages.
> Although, at this late stage all other things like cpufreq should be
> ready, so the ->init() on them is likely to success.

Right, the dtpm is accessible through sysfs for an userspace thermal
daemon doing the smart mitigation. So do the initcall can be really late.

[ ... ]

Thanks for the review.

  -- Daniel


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

  reply	other threads:[~2020-11-03 18:43 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-06 12:20 [PATCH 0/4] powercap/dtpm: Add the DTPM framework Daniel Lezcano
2020-10-06 12:20 ` [PATCH 1/4] units: Add Watt units Daniel Lezcano
2020-11-10 10:02   ` Lukasz Luba
2020-10-06 12:20 ` [PATCH 2/4] Documentation/powercap/dtpm: Add documentation for dtpm Daniel Lezcano
2020-10-13 22:01   ` Ram Chandrasekar
2020-10-06 12:20 ` [PATCH 3/4] powercap/drivers/dtpm: Add API for dynamic thermal power management Daniel Lezcano
2020-10-06 16:42   ` kernel test robot
2020-10-06 18:05   ` kernel test robot
2020-10-23 10:29   ` Lukasz Luba
2020-11-03 18:42     ` Daniel Lezcano [this message]
2020-11-10  9:59   ` Lukasz Luba
2020-11-10 11:05     ` Lukasz Luba
2020-11-10 14:59       ` Daniel Lezcano
2020-11-10 15:04         ` Lukasz Luba
2020-11-10 12:55     ` Daniel Lezcano
2020-10-06 12:20 ` [PATCH 4/4] powercap/drivers/dtpm: Add CPU energy model based support Daniel Lezcano
2020-10-23 13:27   ` Lukasz Luba
2020-11-04 10:47     ` Daniel Lezcano
2020-11-04 10:57       ` Lukasz Luba
2020-11-04 11:15         ` Daniel Lezcano
2020-11-10 12:50   ` Lukasz Luba
2020-10-07 10:43 ` [PATCH 0/4] powercap/dtpm: Add the DTPM framework Hans de Goede
2020-10-12 10:30   ` Daniel Lezcano
2020-10-12 11:46     ` Hans de Goede
2020-10-12 16:02       ` Daniel Lezcano
2020-10-13 12:47         ` Hans de Goede
2020-10-12 16:37       ` Rafael J. Wysocki
2020-10-13 13:04         ` Hans de Goede
2020-10-14 13:33           ` Rafael J. Wysocki
2020-10-14 14:06             ` Hans de Goede
2020-10-14 15:42               ` Rafael J. Wysocki
2020-10-16 11:10                 ` [RFC] Documentation: Add documentation for new performance_profile sysfs class (Also Re: [PATCH 0/4] powercap/dtpm: Add the DTPM framework) Hans de Goede
2020-10-16 14:26                   ` Elia Devito
     [not found]                     ` <HK2PR0302MB2449214B28438ADC1790D468BD030@HK2PR0302MB2449.apcprd03.prod.outlook.com>
2020-10-16 14:43                       ` Fw: [External] " Mark Pearson
2020-10-16 15:16                         ` Elia Devito
2020-10-16 14:51                   ` Rafael J. Wysocki
2020-10-18  9:41                     ` Hans de Goede
2020-10-18 12:31                       ` Rafael J. Wysocki
2020-10-19 18:43                         ` Hans de Goede
     [not found]                           ` <HK2PR0302MB24494037019FBC7720976735BD1E0@HK2PR0302MB2449.apcprd03.prod.outlook.com>
2020-10-19 18:49                             ` Fw: [External] " Mark Pearson
2020-10-25 10:13                               ` Hans de Goede
2020-10-20 12:34                           ` Rafael J. Wysocki

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=18d2393a-2954-f271-817f-f9f9bf651f25@linaro.org \
    --to=daniel.lezcano@linaro.org \
    --cc=arnd@arndb.de \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lukasz.luba@arm.com \
    --cc=rafael@kernel.org \
    --cc=rjw@rjwysocki.net \
    --cc=rui.zhang@intel.com \
    --cc=srinivas.pandruvada@linux.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).