powercap/drivers/dtpm: Encapsulate even more the code
PATCH v4 1/5 powercap/drivers/dtpm: Encapsulate even more the code
In order to increase the self-encapsulation of the dtpm generic code, the following changes are adding a power update ops to the dtpm ops. That allows the generic code to call directly the dtpm backend function to update the power values.
The power update function does compute the power characteristics when the function is invoked. In the case of the CPUs, the power consumption depends on the number of online CPUs. The online CPUs mask is not up to date at CPUHP_AP_ONLINE_DYN state in the tear down callback. That is the reason why the online / offline are at separate state. As there is already an existing state for DTPM, this one is only moved to the DEAD state, so there is no addition of new state with these changes. The dtpm node is not removed when the cpu is unplugged.
That simplifies the code for the next changes and results in a more self-encapsulated code.
---
V4:
- Replaced s/sprintf/snprintf/ for the dtpm node name
V2:
- Updated the changelog with the CPU node not being removed
- Commented the cpu hotplug callbacks to explain why there are two callbacks
- Changed 'upt_power_uw' to 'update_power_uw'
- Removed unused cpumask variable
--- drivers/powercap/dtpm.c | 54 ++++++------- drivers/powercap/dtpm_cpu.c | 148 ++++++++++++++++-------------------- include/linux/cpuhotplug.h | 2 +- include/linux/dtpm.h | 3 +- 4 files changed, 97 insertions(+), 110 deletions(-)
drivers/powercap/dtpm.c +27 -27
@@ -116,8 +116,6 @@ static void __dtpm_sub_power(struct dtpm *dtpm) parent->power_limit -= dtpm->power_limit; parent = parent->parent; } - -__dtpm_rebalance_weight (root); }static void __dtpm_add_power (struct dtpm *dtpm)
@@ -130,45 +128,45 @@ static void __dtpm_add_power(struct dtpm *dtpm) parent->power_limit += dtpm->power_limit; parent = parent->parent; } +} + +static int __dtpm_update_power (struct dtpm *dtpm) +{ +int ret; + +__dtpm_sub_power (dtpm); -__dtpm_rebalance_weight (root); + ret = dtpm->ops->update_power_uw (dtpm); +if (ret) +pr_err ("Failed to update power for '%s': %d\n" , + dtpm->zone.name, ret); + +if (!test_bit (DTPM_POWER_LIMIT_FLAG , &dtpm->flags)) + dtpm->power_limit = dtpm->power_max; + +__dtpm_add_power (dtpm); + +if (root) +__dtpm_rebalance_weight (root); + +return ret; }/** * 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) +int dtpm_update_power (struct dtpm *dtpm) { -int ret =0 ; +int ret;mutex_lock (&dtpm_lock); - -if (power_min == dtpm->power_min && power_max == dtpm->power_max) - goto unlock; - -if (power_max < power_min) { - ret = -EINVAL ; - goto unlock; - } - -__dtpm_sub_power (dtpm); - - dtpm->power_min = power_min; - dtpm->power_max = power_max; -if (!test_bit (DTPM_POWER_LIMIT_FLAG , &dtpm->flags)) - dtpm->power_limit = power_max; - -__dtpm_add_power (dtpm); - -unlock: + ret =__dtpm_update_power (dtpm);mutex_unlock (&dtpm_lock);return ret;
@@ -436,6 +434,7 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)if (dtpm->ops && !(dtpm->ops->set_power_uw && dtpm->ops->get_power_uw && + dtpm->ops->update_power_uw && dtpm->ops->release))return -EINVAL ;
@@ -455,7 +454,8 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent) root = dtpm; } -__dtpm_add_power (dtpm); +if (dtpm->ops && !dtpm->ops->update_power_uw (dtpm)) +__dtpm_add_power (dtpm);pr_info ("Registered dtpm node '%s' / %llu-%llu uW, \n" , dtpm->zone.name, dtpm->power_min, dtpm->power_max);
drivers/powercap/dtpm_cpu.c +67 -81
240 lines not on this page · raw message
include/linux/cpuhotplug.h +1 -1
@@ -61,6 +61,7 @@ enum cpuhp_state {CPUHP_LUSTRE_CFS_DEAD ,CPUHP_AP_ARM_CACHE_B15_RAC_DEAD ,CPUHP_PADATA_DEAD , +CPUHP_AP_DTPM_CPU_DEAD ,CPUHP_WORKQUEUE_PREP ,CPUHP_POWER_NUMA_PREPARE , CPUHP_HRTIMERS_PREPARE,
@@ -193,7 +194,6 @@ enum cpuhp_state {CPUHP_AP_ONLINE_DYN_END =CPUHP_AP_ONLINE_DYN +30 ,CPUHP_AP_X86_HPET_ONLINE ,CPUHP_AP_X86_KVM_CLK_ONLINE , -CPUHP_AP_DTPM_CPU_ONLINE ,CPUHP_AP_ACTIVE , CPUHP_ONLINE, };
include/linux/dtpm.h +2 -1
@@ -29,6 +29,7 @@ struct dtpm {struct dtpm_ops {u64 (*set_power_uw)(struct dtpm *,u64 );u64 (*get_power_uw)(struct dtpm *); +int (*update_power_uw)(struct dtpm *);void (*release)(struct dtpm *); };
@@ -62,7 +63,7 @@ static inline struct dtpm *to_dtpm(struct powercap_zone *zone) return container_of(zone, struct dtpm, zone); } -int dtpm_update_power (struct dtpm *dtpm,u64 power_min,u64 power_max); +int dtpm_update_power (struct dtpm *dtpm);int dtpm_release_zone (struct powercap_zone *pcz);
Signature · 1 line
2.17.1
PATCH v4 2/5 powercap/drivers/dtpm: Create a registering system
A SoC can be differently structured depending on the platform and the kernel can not be aware of all the combinations, as well as the specific tweaks for a particular board.
The creation of the hierarchy must be delegated to userspace.
These changes provide a registering mechanism where the different subsystems will initialize their dtpm backends and register with a name the dtpm node in a list.
The next changes will provide an userspace interface to create hierarchically the different nodes. Those will be created by name and found via the list filled by the different subsystem.
If a specified name is not found in the list, it is assumed to be a virtual node which will have children and the default is to allocate such node.
When the node register in the list, the function will be dtpm_register where the previous semantic was to create the node. Thus, the functions are renamed to reflect their purpose.
---
V4:
- Fixed typo in the commit log
V2:
- Fixed error code path by dropping lock
--- drivers/powercap/dtpm.c | 161 ++++++++++++++++++++++++++++++++++-- drivers/powercap/dtpm_cpu.c | 4 +- include/linux/dtpm.h | 12 ++- 3 files changed, 164 insertions(+), 13 deletions(-)
drivers/powercap/dtpm.c +152 -9
@@ -20,6 +20,7 @@#include <linux/dtpm.h> #include <linux/init.h> #include <linux/kernel.h> +#include <linux/kref.h> #include <linux/powercap.h> #include <linux/slab.h> #include <linux/mutex.h>
@@ -34,6 +35,14 @@ static DEFINE_MUTEX(dtpm_lock);static struct powercap_control_type *pct;static struct dtpm *root; +struct dtpm_node { +const char *name; +struct dtpm *dtpm; +struct list_head node; +}; + +static LIST_HEAD (dtpm_list); +static int get_time_window_us (struct powercap_zone *pcz,int cid,u64 *window) {return -ENOSYS ;
@@ -152,6 +161,138 @@ static int __dtpm_update_power(struct dtpm *dtpm)return ret; } +static struct dtpm *__dtpm_lookup (const char *name) +{ +struct dtpm_node *node; + +list_for_each_entry (node, &dtpm_list, node) { +if (!strcmp (name, node->name)) +return node->dtpm; + } + +return NULL ; +} + +/** +* dtpm_get - Get a reference to a dtpm structure +* @name: the name of the dtpm device +* +* The function looks up in the list of the registered dtpm +* devices. If the dtpm device is not found, a virtual one is +* allocated. This function must be called to create a dtpm node in +* the powercap hierarchy. +* +* Return: a pointer to a dtpm structure, NULL if there is not enough +* memory +*/ +struct dtpm *dtpm_get (const char *name) +{ +struct dtpm *dtpm; + +mutex_lock (&dtpm_lock); + dtpm =__dtpm_lookup (name); +if (!dtpm) + dtpm =dtpm_alloc (NULL ); +else +kref_get (&dtpm->kref); +mutex_unlock (&dtpm_lock); + +return dtpm; +} + +static void dtpm_release (struct kref *kref) +{ + struct dtpm *dtpm = container_of(kref, struct dtpm, kref); + +kfree (dtpm); +} + +/** +* dtpm_put - Release a reference on a dtpm device +* @dtpm: a pointer to a dtpm structure +* +* Release the reference on the specified dtpm device. The last +* reference leads to a memory release. +*/ +void dtpm_put (struct dtpm *dtpm) +{ +kref_put (&dtpm->kref, dtpm_release); +} + +/** +* dtpm_register - Register the dtpm in the dtpm list +* @name: a name used as an identifier +* @dtpm: the dtpm node to be registered +* +* Stores the dtpm device in a list. +* +* Return: 0 on success, -EEXIST if the device name is already present +* in the list, -ENOMEM in case of memory allocation failure. +*/ +int dtpm_register (const char *name,struct dtpm *dtpm) +{ +struct dtpm_node *node; +int ret; + +mutex_lock (&dtpm_lock); + + ret = -EEXIST ; +if (__dtpm_lookup (name)) + goto out_unlock; + + ret = -ENOMEM ; + node =kzalloc (sizeof (*node),GFP_KERNEL ); +if (!node) + goto out_unlock; + + node->name =kstrdup (name,GFP_KERNEL ); +if (!node->name) { +kfree (node); + goto out_unlock; + } + + node->dtpm = dtpm; + +list_add (&node->node, &dtpm_list); + +pr_info ("Registered %s\n" , name); + + ret =0 ; +out_unlock: +mutex_unlock (&dtpm_lock); + +return ret; +} + +/** +* dtpm_unregister - Remove the dtpm device from the list +* @name: the dtpm device name to be removed +* +* Remove the dtpm device from the list of the registered devices. +*/ +void dtpm_unregister (const char *name) +{ +struct dtpm_node *node; + +mutex_lock (&dtpm_lock); + +list_for_each_entry (node, &dtpm_list, node) { + +if (strcmp (name, node->name)) +continue ; + +list_del (&node->node); +kfree (node->name); +kfree (node); + +pr_info ("Unregistered %s\n" , name); + +break ; + } + +mutex_unlock (&dtpm_lock); +} +/** * dtpm_update_power - Update the power on the dtpm * @dtpm: a pointer to a dtpm structure to update
@@ -208,7 +349,7 @@ int dtpm_release_zone(struct powercap_zone *pcz)if (root == dtpm) root =NULL ; -kfree (dtpm); +dtpm_put (dtpm);return 0 ; }
@@ -370,6 +511,7 @@ struct dtpm *dtpm_alloc(struct dtpm_ops *ops)if (dtpm) {INIT_LIST_HEAD (&dtpm->children);INIT_LIST_HEAD (&dtpm->sibling); +kref_init (&dtpm->kref); dtpm->weight =1024 ; dtpm->ops = ops; }
@@ -378,28 +520,29 @@ struct dtpm *dtpm_alloc(struct dtpm_ops *ops) }/** -* dtpm_unregister - Unregister a dtpm node from the hierarchy tree -* @dtpm: a pointer to a dtpm structure corresponding to the node to be removed +* dtpm_destroy - Destroy a dtpm node from the hierarchy tree +* @dtpm: a pointer to a dtpm structure corresponding to the node to be +* removed and destroyed * * Call the underlying powercap unregister function. That will call * the release callback of the powercap zone. */ -void dtpm_unregister (struct dtpm *dtpm) +void dtpm_destroy (struct dtpm *dtpm) {powercap_unregister_zone (pct, &dtpm->zone); -pr_info ("Unregistered dtpm node '%s'\n" , dtpm->zone.name); +pr_info ("Destroyed dtpm node '%s'\n" , dtpm->zone.name); }/** -* dtpm_register - Register a dtpm node in the hierarchy tree +* dtpm_create - Create a dtpm node in the hierarchy tree * @name: a string specifying the name of the node * @dtpm: a pointer to a dtpm structure corresponding to the new node * @parent: a pointer to a dtpm structure corresponding to the parent node * * Create a dtpm node in the tree. If no parent is specified, the node * is the root node of the hierarchy. If the root node already exists, -* then the registration will fail. The powercap controller must be +* then the creation will fail. The powercap controller must be * initialized before calling this function. * * The dtpm structure must be initialized with the power numbers
@@ -413,7 +556,7 @@ void dtpm_unregister(struct dtpm *dtpm)* * parent have ops which are reserved for leaves * Other negative values are reported back from the powercap framework */ -int dtpm_register (const char *name,struct dtpm *dtpm,struct dtpm *parent) +int dtpm_create (const char *name,struct dtpm *dtpm,struct dtpm *parent) {struct powercap_zone *pcz;
@@ -457,7 +600,7 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)if (dtpm->ops && !dtpm->ops->update_power_uw (dtpm))__dtpm_add_power (dtpm); -pr_info ("Registered dtpm node '%s' / %llu-%llu uW, \n" , +pr_info ("Created dtpm node '%s' / %llu-%llu uW, \n" , dtpm->zone.name, dtpm->power_min, dtpm->power_max);mutex_unlock (&dtpm_lock);
drivers/powercap/dtpm_cpu.c +2 -2
@@ -177,7 +177,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)snprintf (name,sizeof (name),"cpu%d-cpufreq" , dtpm_cpu->cpu); - ret =dtpm_register (name, dtpm,NULL ); + ret =dtpm_register (name, dtpm);if (ret) goto out_kfree_dtpm_cpu;
@@ -190,7 +190,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)return 0 ; out_dtpm_unregister: -dtpm_unregister (dtpm); +dtpm_unregister (name); dtpm_cpu =NULL ; dtpm =NULL ;
include/linux/dtpm.h +10 -2
@@ -14,6 +14,7 @@struct dtpm {struct powercap_zone zone; +struct kref kref;struct dtpm *parent;struct list_head sibling;struct list_head children;
@@ -69,10 +70,17 @@ int dtpm_release_zone(struct powercap_zone *pcz);struct dtpm *dtpm_alloc (struct dtpm_ops *ops); -void dtpm_unregister (struct dtpm *dtpm); +void dtpm_destroy (struct dtpm *dtpm); -int dtpm_register (const char *name,struct dtpm *dtpm,struct dtpm *parent); +int dtpm_create (const char *name,struct dtpm *dtpm,struct dtpm *parent);int dtpm_register_cpu (struct dtpm *parent); +int dtpm_register (const char *name,struct dtpm *dtpm); + +void dtpm_unregister (const char *name); + +struct dtpm *dtpm_get (const char *name); + +void dtpm_put (struct dtpm *dtpm);#endif
Signature · 1 line
2.17.1
Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system
Quoted 114 lines · Daniel Lezcano
A SoC can be differently structured depending on the platform and the kernel can not be aware of all the combinations, as well as the specific tweaks for a particular board.
The creation of the hierarchy must be delegated to userspace.
These changes provide a registering mechanism where the different subsystems will initialize their dtpm backends and register with a name the dtpm node in a list.
The next changes will provide an userspace interface to create hierarchically the different nodes. Those will be created by name and found via the list filled by the different subsystem.
If a specified name is not found in the list, it is assumed to be a virtual node which will have children and the default is to allocate such node.
When the node register in the list, the function will be
dtpm_registerwhere the previous semantic was to create the node. Thus, the functions are renamed to reflect their purpose.Signed-off-by Daniel Lezcano <[email protected]>Reviewed-by Lukasz Luba <[email protected]>---
V4:
- Fixed typo in the commit logV2:
- Fixed error code path by dropping lock--- drivers/powercap/dtpm.c | 161 ++++++++++++++++++++++++++++++++++-- drivers/powercap/dtpm_cpu.c | 4 +- include/linux/dtpm.h | 12 ++- 3 files changed, 164 insertions(+), 13 deletions(-)drivers/powercap/dtpm.c +55
-0@@ -20,6 +20,7 @@#include <linux/dtpm.h> #include <linux/init.h> #include <linux/kernel.h> +#include <linux/kref.h> #include <linux/powercap.h> #include <linux/slab.h> #include <linux/mutex.h> @@ -34,6 +35,14 @@ static DEFINE_MUTEX(dtpm_lock);static struct powercap_control_type *pct;static struct dtpm *root; +struct dtpm_node { +const char *name; +struct dtpm *dtpm; +struct list_head node; +}; + +static LIST_HEAD (dtpm_list); +static int get_time_window_us (struct powercap_zone *pcz,int cid,u64 *window) {return -ENOSYS ;@@ -152,6 +161,138 @@ static int __dtpm_update_power(struct dtpm *dtpm)return ret; } +static struct dtpm *__dtpm_lookup (const char *name) +{ +struct dtpm_node *node; + +list_for_each_entry (node, &dtpm_list, node) { +if (!strcmp (name, node->name)) +return node->dtpm; + } + +return NULL ; +} + +/** +* dtpm_get - Get a reference to a dtpm structure +* @name: the name of the dtpm device +* +* The function looks up in the list of the registered dtpm +* devices. If the dtpm device is not found, a virtual one is +* allocated. This function must be called to create a dtpm node in +* the powercap hierarchy. +* +* Return: a pointer to a dtpm structure, NULL if there is not enough +* memory +*/ +struct dtpm *dtpm_get (const char *name) +{ +struct dtpm *dtpm; + +mutex_lock (&dtpm_lock); + dtpm =__dtpm_lookup (name); +if (!dtpm) + dtpm =dtpm_alloc (NULL ); +else +kref_get (&dtpm->kref); +mutex_unlock (&dtpm_lock); + +return dtpm; +} + +static void dtpm_release (struct kref *kref) +{ + struct dtpm *dtpm = container_of(kref, struct dtpm, kref); + +kfree (dtpm); +} +
drivers/powercap/dtpm.c static int __dtpm_update_power(struct dtpm *dtpm) · line 219
Quoted 10 lines
drivers/powercap/dtpm.c +10
-0+/** +* dtpm_put - Release a reference on a dtpm device +* @dtpm: a pointer to a dtpm structure +* +* Release the reference on the specified dtpm device. The last +* reference leads to a memory release. +*/ +void dtpm_put (struct dtpm *dtpm) +{ +kref_put (&dtpm->kref, dtpm_release);
You forgot to also grab the dtpm_lock before calling this, right? What is preventing a get and put from being called at the same time?
You protect things at get time, but not put from what I can see :(
Quoted 29 lines
drivers/powercap/dtpm.c +29
-0+} + +/** +* dtpm_register - Register the dtpm in the dtpm list +* @name: a name used as an identifier +* @dtpm: the dtpm node to be registered +* +* Stores the dtpm device in a list. +* +* Return: 0 on success, -EEXIST if the device name is already present +* in the list, -ENOMEM in case of memory allocation failure. +*/ +int dtpm_register (const char *name,struct dtpm *dtpm) +{ +struct dtpm_node *node; +int ret; + +mutex_lock (&dtpm_lock); + + ret = -EEXIST ; +if (__dtpm_lookup (name)) + goto out_unlock; + + ret = -ENOMEM ; + node =kzalloc (sizeof (*node),GFP_KERNEL ); +if (!node) + goto out_unlock; + + node->name =kstrdup (name,GFP_KERNEL );
drivers/powercap/dtpm.c static int __dtpm_update_power(struct dtpm *dtpm) · line 258
Quoted 10 lines
drivers/powercap/dtpm.c +10
-0+if (!node->name) { +kfree (node); + goto out_unlock; + } + + node->dtpm = dtpm; + +list_add (&node->node, &dtpm_list); + +pr_info ("Registered %s\n" , name);
When kernel code works properly, it is quiet. This is debugging code a the most, never something that everyone should be seeing all the time, please remove.
Quoted 20 lines
drivers/powercap/dtpm.c +20
-0+ + ret =0 ; +out_unlock: +mutex_unlock (&dtpm_lock); + +return ret; +} + +/** +* dtpm_unregister - Remove the dtpm device from the list +* @name: the dtpm device name to be removed +* +* Remove the dtpm device from the list of the registered devices. +*/ +void dtpm_unregister (const char *name) +{ +struct dtpm_node *node; + +mutex_lock (&dtpm_lock); +
drivers/powercap/dtpm.c static int __dtpm_update_power(struct dtpm *dtpm) · line 288
Quoted 10 lines
drivers/powercap/dtpm.c +10
-0+list_for_each_entry (node, &dtpm_list, node) { + +if (strcmp (name, node->name)) +continue ; + +list_del (&node->node); +kfree (node->name); +kfree (node); + +pr_info ("Unregistered %s\n" , name);
Again, debugging code should be removed.
Quoted 37 lines
drivers/powercap/dtpm.c +12
-3+ +break ; + } + +mutex_unlock (&dtpm_lock); +} +/** * dtpm_update_power - Update the power on the dtpm * @dtpm: a pointer to a dtpm structure to update @@ -208,7 +349,7 @@ int dtpm_release_zone(struct powercap_zone *pcz)if (root == dtpm) root =NULL ; -kfree (dtpm); +dtpm_put (dtpm);return 0 ; }@@ -370,6 +511,7 @@ struct dtpm *dtpm_alloc(struct dtpm_ops *ops)if (dtpm) {INIT_LIST_HEAD (&dtpm->children);INIT_LIST_HEAD (&dtpm->sibling); +kref_init (&dtpm->kref); dtpm->weight =1024 ; dtpm->ops = ops; }@@ -378,28 +520,29 @@ struct dtpm *dtpm_alloc(struct dtpm_ops *ops) }/** -* dtpm_unregister - Unregister a dtpm node from the hierarchy tree -* @dtpm: a pointer to a dtpm structure corresponding to the node to be removed +* dtpm_destroy - Destroy a dtpm node from the hierarchy tree +* @dtpm: a pointer to a dtpm structure corresponding to the node to be +* removed and destroyed *
drivers/powercap/dtpm.c struct dtpm *dtpm_alloc(struct dtpm_ops *ops) · line 534
Quoted 10 lines
drivers/powercap/dtpm.c +2
-2* Call the underlying powercap unregister function. That will call * the release callback of the powercap zone. */ -void dtpm_unregister (struct dtpm *dtpm) +void dtpm_destroy (struct dtpm *dtpm) {powercap_unregister_zone (pct, &dtpm->zone); -pr_info ("Unregistered dtpm node '%s'\n" , dtpm->zone.name); +pr_info ("Destroyed dtpm node '%s'\n" , dtpm->zone.name);
Again, please make pr_dbg().
And any reason why you are not using "real" struct devices in this subsystem? You seem to be rolling your own infrastructure for no good reason. I imagine you want sysfs support next, right?
thanks,
greg k-h
Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system
Quoted 26 lines · Greg KH
Quoted 25 lines · Daniel Lezcano
A SoC can be differently structured depending on the platform and the kernel can not be aware of all the combinations, as well as the specific tweaks for a particular board.
The creation of the hierarchy must be delegated to userspace.
These changes provide a registering mechanism where the different subsystems will initialize their dtpm backends and register with a name the dtpm node in a list.
The next changes will provide an userspace interface to create hierarchically the different nodes. Those will be created by name and found via the list filled by the different subsystem.
If a specified name is not found in the list, it is assumed to be a virtual node which will have children and the default is to allocate such node.
When the node register in the list, the function will be
dtpm_registerwhere the previous semantic was to create the node. Thus, the functions are renamed to reflect their purpose.Signed-off-by Daniel Lezcano <[email protected]>Reviewed-by Lukasz Luba <[email protected]>---
[ ... ]
Quoted 21 lines
Quoted 7 lines
drivers/powercap/dtpm.c +7
-0+static void dtpm_release (struct kref *kref) +{ + struct dtpm *dtpm = container_of(kref, struct dtpm, kref); + +kfree (dtpm); +} +drivers/powercap/dtpm.c static int __dtpm_update_power(struct dtpm *dtpm) · line 219
Quoted 10 lines
drivers/powercap/dtpm.c +10
-0+/** +* dtpm_put - Release a reference on a dtpm device +* @dtpm: a pointer to a dtpm structure +* +* Release the reference on the specified dtpm device. The last +* reference leads to a memory release. +*/ +void dtpm_put (struct dtpm *dtpm) +{ +kref_put (&dtpm->kref, dtpm_release);You forgot to also grab the
dtpm_lockbefore calling this, right? What is preventing a get and put from being called at the same time?
Quoted 1 line
You protect things at get time, but not put from what I can see :(
Thanks for spotting this, I will send a fix for that.
[ ... ]
Quoted 7 lines
drivers/powercap/dtpm.c static int __dtpm_update_power(struct dtpm *dtpm) · line 258
Quoted 3 lines
drivers/powercap/dtpm.c +3
-0+list_add (&node->node, &dtpm_list); + +pr_info ("Registered %s\n" , name);When kernel code works properly, it is quiet. This is debugging code a the most, never something that everyone should be seeing all the time, please remove.
Initially, a comment asked for debug traces in the code. There are more traces in the code at the pr_debug level.
Is your suggestion to remove the pr_info as well as other debug traces or convert those pr_info to pr_debug ?
[ ... ]
Quoted 3 lines
And any reason why you are not using "real" struct devices in this subsystem? You seem to be rolling your own infrastructure for no good reason. I imagine you want sysfs support next, right?
Actually, the framework is on top of powercap, so it has de facto the sysfs support. On the other side, the dtpm backends are tied with the device they manage.
Signature · 5 lines
<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
Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system
Quoted 65 lines · Daniel Lezcano
Quoted 26 lines · Greg KH
Quoted 25 lines · Daniel Lezcano
A SoC can be differently structured depending on the platform and the kernel can not be aware of all the combinations, as well as the specific tweaks for a particular board.
The creation of the hierarchy must be delegated to userspace.
These changes provide a registering mechanism where the different subsystems will initialize their dtpm backends and register with a name the dtpm node in a list.
The next changes will provide an userspace interface to create hierarchically the different nodes. Those will be created by name and found via the list filled by the different subsystem.
If a specified name is not found in the list, it is assumed to be a virtual node which will have children and the default is to allocate such node.
When the node register in the list, the function will be
dtpm_registerwhere the previous semantic was to create the node. Thus, the functions are renamed to reflect their purpose.Signed-off-by Daniel Lezcano <[email protected]>Reviewed-by Lukasz Luba <[email protected]>---
[ ... ]
Quoted 21 lines
Quoted 7 lines
drivers/powercap/dtpm.c +7
-0+static void dtpm_release (struct kref *kref) +{ + struct dtpm *dtpm = container_of(kref, struct dtpm, kref); + +kfree (dtpm); +} +drivers/powercap/dtpm.c static int __dtpm_update_power(struct dtpm *dtpm) · line 219
Quoted 10 lines
drivers/powercap/dtpm.c +10
-0+/** +* dtpm_put - Release a reference on a dtpm device +* @dtpm: a pointer to a dtpm structure +* +* Release the reference on the specified dtpm device. The last +* reference leads to a memory release. +*/ +void dtpm_put (struct dtpm *dtpm) +{ +kref_put (&dtpm->kref, dtpm_release);You forgot to also grab the
dtpm_lockbefore calling this, right? What is preventing a get and put from being called at the same time?Quoted 1 line
You protect things at get time, but not put from what I can see :(
Thanks for spotting this, I will send a fix for that.
[ ... ]
Quoted 7 lines
drivers/powercap/dtpm.c static int __dtpm_update_power(struct dtpm *dtpm) · line 258
Quoted 3 lines
drivers/powercap/dtpm.c +3
-0+list_add (&node->node, &dtpm_list); + +pr_info ("Registered %s\n" , name);When kernel code works properly, it is quiet. This is debugging code a the most, never something that everyone should be seeing all the time, please remove.
Quoted 5 lines
Initially, a comment asked for debug traces in the code. There are more traces in the code at the
pr_debuglevel.Is your suggestion to remove the
pr_infoas well as other debug traces or convert thosepr_infotopr_debug?
pr_info() should not be used for "debug traces".
Use real tracepoints for debug traces if you still need them, hopefully the code is all now debugged properly, right?
Again, when code is working, it is quiet. Do not leave debugging stuff like this in a working system.
Quoted 10 lines
[ ... ]
Quoted 3 lines
And any reason why you are not using "real" struct devices in this subsystem? You seem to be rolling your own infrastructure for no good reason. I imagine you want sysfs support next, right?
Actually, the framework is on top of powercap, so it has de facto the sysfs support. On the other side, the dtpm backends are tied with the device they manage.
So why are they not a "real" device in the driver model? It looks like you almost are wanting all of that functionality and are having to implement it "by hand" instead.
thanks,
greg k-h
Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system
Hi Greg,
On 28/03/2021 08:50, Greg KH wrote:
[ ... ]
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
And any reason why you are not using "real" struct devices in this subsystem? You seem to be rolling your own infrastructure for no good reason. I imagine you want sysfs support next, right?
Actually, the framework is on top of powercap, so it has de facto the sysfs support. On the other side, the dtpm backends are tied with the device they manage.
So why are they not a "real" device in the driver model? It looks like you almost are wanting all of that functionality and are having to implement it "by hand" instead.
I'm sorry I misunderstanding your point. dtpm is the backend for the powercap subsystem which is the generic subsystem to do power limitation.
We have:
struct dtpm_cpu {
struct dtpm dtmp;
...
}struct dtpm {
struct powercap powecap;
};struct powercap {
struct device dev;
};Signature · 5 lines
<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
Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system
Quoted 29 lines · Daniel Lezcano
Hi Greg,
On 28/03/2021 08:50, Greg KH wrote:
[ ... ]
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
And any reason why you are not using "real" struct devices in this subsystem? You seem to be rolling your own infrastructure for no good reason. I imagine you want sysfs support next, right?
Actually, the framework is on top of powercap, so it has de facto the sysfs support. On the other side, the dtpm backends are tied with the device they manage.
So why are they not a "real" device in the driver model? It looks like you almost are wanting all of that functionality and are having to implement it "by hand" instead.
I'm sorry I misunderstanding your point. dtpm is the backend for the powercap subsystem which is the generic subsystem to do power limitation.
We have:
struct dtpm_cpu { struct dtpm dtmp; ... }
Quoted 7 lines
struct dtpm { struct powercap powecap; };struct powercap { struct device dev; };
Oh nice. So you can not use a kref here at all as you already have a reference counted device controling your structure. You can not have 2 references trying to control the same structure, that way lies madness and bugs :(
So why are you trying to add a kref here as the structure already has support for proper lifetimes?
thanks,
greg k-h
Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system
Quoted 38 lines · Greg KH
Quoted 29 lines · Daniel Lezcano
Hi Greg,
On 28/03/2021 08:50, Greg KH wrote:
[ ... ]
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
And any reason why you are not using "real" struct devices in this subsystem? You seem to be rolling your own infrastructure for no good reason. I imagine you want sysfs support next, right?
Actually, the framework is on top of powercap, so it has de facto the sysfs support. On the other side, the dtpm backends are tied with the device they manage.
So why are they not a "real" device in the driver model? It looks like you almost are wanting all of that functionality and are having to implement it "by hand" instead.
I'm sorry I misunderstanding your point. dtpm is the backend for the powercap subsystem which is the generic subsystem to do power limitation.
We have:
struct dtpm_cpu { struct dtpm dtmp; ... }Quoted 7 lines
struct dtpm { struct powercap powecap; };struct powercap { struct device dev; };
Quoted 7 lines
Oh nice. So you can not use a kref here at all as you already have a reference counted device controling your structure. You can not have 2 references trying to control the same structure, that way lies madness and bugs :(
So why are you trying to add a kref here as the structure already has support for proper lifetimes?
Right, I'll revisit that part. Thanks for the review.
I've a branch which is pulled by Rafael [1]. These parts are already merged in the dtpm/next branch but not yet in Rafael's tree.
I think a rebase is possible but I would like to avoid that. Would be a patch on top of the dtpm/next acceptable given your flow with Android ?
-- Daniel
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/daniel.lezcano/linux.git/log/?h=dtpm/next
Signature · 5 lines
<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
Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system
Quoted 44 lines · Daniel Lezcano
Quoted 43 lines · Greg KH
Quoted 29 lines · Daniel Lezcano
Hi Greg,
On 28/03/2021 08:50, Greg KH wrote:
[ ... ]
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
And any reason why you are not using "real" struct devices in this subsystem? You seem to be rolling your own infrastructure for no good reason. I imagine you want sysfs support next, right?
Actually, the framework is on top of powercap, so it has de facto the sysfs support. On the other side, the dtpm backends are tied with the device they manage.
So why are they not a "real" device in the driver model? It looks like you almost are wanting all of that functionality and are having to implement it "by hand" instead.
I'm sorry I misunderstanding your point. dtpm is the backend for the powercap subsystem which is the generic subsystem to do power limitation.
We have:
struct dtpm_cpu { struct dtpm dtmp; ... }Quoted 7 lines
struct dtpm { struct powercap powecap; };struct powercap { struct device dev; };Oh nice. So you can not use a kref here at all as you already have a reference counted device controling your structure. You can not have 2 references trying to control the same structure, that way lies madness and bugs :(
Quoted 7 lines
Quoted 2 lines
So why are you trying to add a kref here as the structure already has support for proper lifetimes?
Right, I'll revisit that part. Thanks for the review.
I've a branch which is pulled by Rafael [1]. These parts are already merged in the dtpm/next branch but not yet in Rafael's tree.
I would recommend fixing that up if you can rebase it. If not, you need to revert it and start over. I'll be glad to review it if you cc: me on the patches.
Quoted 2 lines
I think a rebase is possible but I would like to avoid that. Would be a patch on top of the dtpm/next acceptable given your flow with Android ?
This has nothing to do with the Android kernel workflow, sorry. I am concerned about proper kernel development and keeping bugs out of it.
thanks,
greg k-h
Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system
On 28/03/2021 19:26, Greg KH wrote:
[ ... ]
Quoted 12 lines
Quoted 7 lines
Quoted 2 lines
So why are you trying to add a kref here as the structure already has support for proper lifetimes?
Right, I'll revisit that part. Thanks for the review.
I've a branch which is pulled by Rafael [1]. These parts are already merged in the dtpm/next branch but not yet in Rafael's tree.
I would recommend fixing that up if you can rebase it. If not, you need to revert it and start over. I'll be glad to review it if you cc: me on the patches.
Quoted 5 lines
Quoted 2 lines
I think a rebase is possible but I would like to avoid that. Would be a patch on top of the dtpm/next acceptable given your flow with Android ?
This has nothing to do with the Android kernel workflow, sorry. I am concerned about proper kernel development and keeping bugs out of it.
Fair enough, I will fix it up and send a v5.
Thanks
-- Daniel
Signature · 5 lines
<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
PATCH v4 3/5 powercap/drivers/dtpm: Simplify the dtpm table
In reply to Daniel LezcanoThe dtpm table is an array of pointers, that forces the user of the table to define initdata along with the declaration of the table entry. It is more efficient to create an array of dtpm structure, so the declaration of the table entry can be done by initializing the different fields.
--- drivers/powercap/dtpm.c | 4 ++-- drivers/powercap/dtpm_cpu.c | 4 +++- include/linux/dtpm.h | 22 +++++++++------------- 3 files changed, 14 insertions(+), 16 deletions(-)
drivers/powercap/dtpm.c +2 -2
@@ -610,7 +610,7 @@ int dtpm_create(const char *name, struct dtpm *dtpm, struct dtpm *parent)static int __init dtpm_init (void ) { -struct dtpm_descr **dtpm_descr; +struct dtpm_descr *dtpm_descr; pct =powercap_register_control_type (NULL ,"dtpm" ,NULL );if (IS_ERR (pct)) {
@@ -619,7 +619,7 @@ static int __init dtpm_init(void) }for_each_dtpm_table (dtpm_descr) - (*dtpm_descr)->init (*dtpm_descr); + dtpm_descr->init ();return 0 ; }
drivers/powercap/dtpm_cpu.c +3 -1
@@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)return ret; } -int dtpm_register_cpu (struct dtpm *parent) +static int __init dtpm_cpu_init (void ) {int ret;
@@ -241,3 +241,5 @@ int dtpm_register_cpu(struct dtpm *parent)return 0 ; } + +DTPM_DECLARE (dtpm_cpu, dtpm_cpu_init);
include/linux/dtpm.h +9 -13
@@ -34,25 +34,23 @@ struct dtpm_ops {void (*release)(struct dtpm *); }; -struct dtpm_descr ; - -typedef int (*dtpm_init_t )(struct dtpm_descr *); +typedef int (*dtpm_init_t )(void );struct dtpm_descr { -struct dtpm *parent; -const char *name;dtpm_init_t init; };/* Init section thermal table */ -extern struct dtpm_descr *__dtpm_table[]; -extern struct dtpm_descr *__dtpm_table_end[]; +extern struct dtpm_descr __dtpm_table[]; +extern struct dtpm_descr __dtpm_table_end[]; -#define DTPM_TABLE_ENTRY (name) \ - static typeof(name) *__dtpm_table_entry_##name \ - __used __section("__dtpm_table") = &name +#define DTPM_TABLE_ENTRY (name, __init) \ + static struct dtpm_descr __dtpm_table_entry_##name \ + __used __section("__dtpm_table") = { \ + .init = __init, \ + } -#define DTPM_DECLARE (name) DTPM_TABLE_ENTRY(name) +#define DTPM_DECLARE (name, init) DTPM_TABLE_ENTRY(name, init)#define for_each_dtpm_table (__dtpm) \ for (__dtpm = __dtpm_table; \
@@ -74,8 +72,6 @@ void dtpm_destroy(struct dtpm *dtpm);int dtpm_create (const char *name,struct dtpm *dtpm,struct dtpm *parent); -int dtpm_register_cpu (struct dtpm *parent); -int dtpm_register (const char *name,struct dtpm *dtpm);void dtpm_unregister (const char *name);
Signature · 1 line
2.17.1
PATCH v4 4/5 powercap/drivers/dtpm: Use container_of instead of a private data field
In reply to Daniel LezcanoThe dtpm framework provides an API to allocate a dtpm node. However when a backend dtpm driver needs to allocate a dtpm node it must define its own structure and store the pointer of this structure in the private field of the dtpm structure.
It is more elegant to use the container_of macro and add the dtpm structure inside the dtpm backend specific structure. The code will be able to deal properly with the dtpm structure as a generic entity, making all this even more self-encapsulated.
The dtpm_alloc() function does no longer make sense as the dtpm structure will be allocated when allocating the device specific dtpm structure. The dtpm_init() is provided instead.
--- drivers/powercap/dtpm.c | 27 ++++++++++------------ drivers/powercap/dtpm_cpu.c | 46 ++++++++++++++++++------------------- include/linux/dtpm.h | 3 +-- 3 files changed, 35 insertions(+), 41 deletions(-)
drivers/powercap/dtpm.c +12 -15
@@ -191,10 +191,13 @@ struct dtpm *dtpm_get(const char *name)mutex_lock (&dtpm_lock); dtpm =__dtpm_lookup (name); -if (!dtpm) - dtpm =dtpm_alloc (NULL ); -else +if (!dtpm) { + dtpm =kzalloc (sizeof (*dtpm),GFP_KERNEL ); +if (dtpm) +dtpm_init (dtpm,NULL ); + }else {kref_get (&dtpm->kref); + }mutex_unlock (&dtpm_lock);return dtpm;
@@ -498,16 +501,12 @@ static struct powercap_zone_ops zone_ops = { };/** -* dtpm_alloc - Allocate and initialize a dtpm struct -* @name: a string specifying the name of the node -* -* Return: a struct dtpm pointer, NULL in case of error +* dtpm_init - Allocate and initialize a dtpm struct +* @dtpm: The dtpm struct pointer to be initialized +* @ops: The dtpm device specific ops, NULL for a virtual node */ -struct dtpm *dtpm_alloc (struct dtpm_ops *ops) +void dtpm_init (struct dtpm *dtpm,struct dtpm_ops *ops) { -struct dtpm *dtpm; - - dtpm =kzalloc (sizeof (*dtpm),GFP_KERNEL );if (dtpm) {INIT_LIST_HEAD (&dtpm->children);INIT_LIST_HEAD (&dtpm->sibling);
@@ -515,8 +514,6 @@ struct dtpm *dtpm_alloc(struct dtpm_ops *ops) dtpm->weight =1024 ; dtpm->ops = ops; } - -return dtpm; }/**
@@ -608,7 +605,7 @@ int dtpm_create(const char *name, struct dtpm *dtpm, struct dtpm *parent)return 0 ; } -static int __init dtpm_init (void ) +static int __init init_dtpm (void ) {struct dtpm_descr *dtpm_descr;
@@ -623,4 +620,4 @@ static int __init dtpm_init(void)return 0 ; } -late_initcall (dtpm_init); +late_initcall (init_dtpm);
drivers/powercap/dtpm_cpu.c +22 -24
121 lines not on this page · raw message
include/linux/dtpm.h +1 -2
@@ -24,7 +24,6 @@ struct dtpm {u64 power_max;u64 power_min;int weight; -void *private; };struct dtpm_ops {
@@ -66,7 +65,7 @@ int dtpm_update_power(struct dtpm *dtpm);int dtpm_release_zone (struct powercap_zone *pcz); -struct dtpm *dtpm_alloc (struct dtpm_ops *ops); +void dtpm_init (struct dtpm *dtpm,struct dtpm_ops *ops);void dtpm_destroy (struct dtpm *dtpm);
Signature · 1 line
2.17.1
PATCH v4 5/5 powercap/drivers/dtpm: Scale the power with the load
In reply to Daniel LezcanoCurrently the power consumption is based on the current OPP power assuming the entire performance domain is fully loaded.
That gives very gross power estimation and we can do much better by using the load to scale the power consumption.
Use the utilization to normalize and scale the power usage over the max possible power.
Tested on a rock960 with 2 big CPUS, the power consumption estimation conforms with the expected one.
Before this change:
~$ ~/dhrystone -t 1 -l 10000&
~$ cat /sys/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:1/constraint_0_max_power_uw 2260000
After this change:
~$ ~/dhrystone -t 1 -l 10000&
~$ cat /sys/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:1/constraint_0_max_power_uw 1130000
~$ ~/dhrystone -t 2 -l 10000&
~$ cat /sys/devices/virtual/powercap/dtpm/dtpm:0/dtpm:0:1/constraint_0_max_power_uw 2260000
---
V3:
- Fixed uninitialized 'cpu' in scaled_power_uw()
V2:
- Replaced cpumask by em_span_cpus
- Changed 'util' metrics variable types
- Optimized utilization scaling power computation
- Renamed parameter name for scale_pd_power_uw()
--- drivers/powercap/dtpm_cpu.c | 46 +++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 7 deletions(-)
drivers/powercap/dtpm_cpu.c +39 -7
@@ -68,27 +68,59 @@ static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)return power_limit; } +static u64 scale_pd_power_uw (struct cpumask *pd_mask,u64 power) +{ +unsigned long max =0 , sum_util =0 ; +int cpu; + +for_each_cpu_and (cpu, pd_mask, cpu_online_mask) { + +/* +* The capacity is the same for all CPUs belonging to +* the same perf domain, so a single call to +* arch_scale_cpu_capacity() is enough. However, we +* need the CPU parameter to be initialized by the +* loop, so the call ends up in this block. +* +* We can initialize 'max' with a cpumask_first() call +* before the loop but the bits computation is not +* worth given the arch_scale_cpu_capacity() just +* returns a value where the resulting assembly code +* will be optimized by the compiler. +*/ + max =arch_scale_cpu_capacity (cpu); + sum_util +=sched_cpu_util (cpu, max); + } + +/* +* In the improbable case where all the CPUs of the perf +* domain are offline, 'max' will be zero and will lead to an +* illegal operation with a zero division. +*/ +return max ? (power * ((sum_util <<10 ) / max)) >>10 :0 ; +} +static u64 get_pd_power_uw (struct dtpm *dtpm) {struct dtpm_cpu *dtpm_cpu =to_dtpm_cpu (dtpm);struct em_perf_domain *pd; -struct cpumask cpus; +struct cpumask *pd_mask;unsigned long freq; -int i, nr_cpus; +int i; pd =em_cpu_get (dtpm_cpu->cpu); - freq =cpufreq_quick_get (dtpm_cpu->cpu); -cpumask_and (&cpus, cpu_online_mask,to_cpumask (pd->cpus)); - nr_cpus =cpumask_weight (&cpus); + pd_mask =em_span_cpus (pd); + + freq =cpufreq_quick_get (dtpm_cpu->cpu);for (i =0 ; i < pd->nr_perf_states; i++) {if (pd->table[i].frequency < freq)continue ; -return pd->table[i].power * -MICROWATT_PER_MILLIWATT * nr_cpus; +return scale_pd_power_uw (pd_mask, pd->table[i].power * +MICROWATT_PER_MILLIWATT ); }return 0 ;
Signature · 1 line
2.17.1