[PATCH v2 0/4] hwmon: improvements to amd_energy driver

[PATCH v2 0/4] hwmon: improvements to amd_energy driver

[Naveen Krishna Chatradhi]

Hi Guenter,

Would like to know your feedback on the following feature
for the amd_energy driver.

1) Accumulator Interval change based on reported resolution
    - Frequency of the accumulator thread can be set during
      the probe based on fine grain (1.625 micro J) or course
      grain (0.125 milli J) resolutions.

Akshay Gupta (1):
  hwmon: amd_energy: Move label out of accumulation structure

Naveen Krishna Chatradhi (3):
  hwmon: amd_energy: optimize accumulation interval
  hwmon: amd_energy: Improve the accumulation logic
  hwmon: (amd_energy) Update driver documentation

 Documentation/hwmon/amd_energy.rst |   5 +
 drivers/hwmon/amd_energy.c         | 163 ++++++++++++-----------------
 2 files changed, 72 insertions(+), 96 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] hwmon: amd_energy: Move label out of accumulation structure

[Naveen Krishna Chatradhi]

From: Akshay Gupta <Akshay.Gupta@amd.com>

At present, core & socket labels are defined in struct sensor_accumulator
This patch moves it to the amd_energy_data structure, which will
help in calling memset on struct sensor_accumulator to optimize the code.

Signed-off-by: Akshay Gupta <Akshay.Gupta@amd.com>
---
Changes since v1:
None

 drivers/hwmon/amd_energy.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index 29603742c858..9580a16185b8 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -35,7 +35,6 @@
 struct sensor_accumulator {
 	u64 energy_ctr;
 	u64 prev_value;
-	char label[10];
 };
 
 struct amd_energy_data {
@@ -52,6 +51,7 @@ struct amd_energy_data {
 	int nr_cpus;
 	int nr_socks;
 	int core_id;
+	char (*label)[10];
 };
 
 static int amd_energy_read_labels(struct device *dev,
@@ -61,7 +61,7 @@ static int amd_energy_read_labels(struct device *dev,
 {
 	struct amd_energy_data *data = dev_get_drvdata(dev);
 
-	*str = data->accums[channel].label;
+	*str = data->label[channel];
 	return 0;
 }
 
@@ -253,6 +253,7 @@ static int amd_create_sensor(struct device *dev,
 	struct sensor_accumulator *accums;
 	int i, num_siblings, cpus, sockets;
 	u32 *s_config;
+	char (*label_l)[10];
 
 	/* Identify the number of siblings per core */
 	num_siblings = ((cpuid_ebx(0x8000001e) >> 8) & 0xff) + 1;
@@ -276,21 +277,25 @@ static int amd_create_sensor(struct device *dev,
 	if (!accums)
 		return -ENOMEM;
 
+	label_l = devm_kcalloc(dev, cpus + sockets,
+			       sizeof(*label_l), GFP_KERNEL);
+	if (!label_l)
+		return -ENOMEM;
+
 	info->type = type;
 	info->config = s_config;
 
 	data->nr_cpus = cpus;
 	data->nr_socks = sockets;
 	data->accums = accums;
+	data->label = label_l;
 
 	for (i = 0; i < cpus + sockets; i++) {
 		s_config[i] = config;
 		if (i < cpus)
-			scnprintf(accums[i].label, 10,
-				  "Ecore%03u", i);
+			scnprintf(label_l[i], 10, "Ecore%03u", i);
 		else
-			scnprintf(accums[i].label, 10,
-				  "Esocket%u", (i - cpus));
+			scnprintf(label_l[i], 10, "Esocket%u", (i - cpus));
 	}
 
 	return 0;
-- 
2.17.1

[PATCH v2 2/4] hwmon: amd_energy: optimize accumulation interval

[Naveen Krishna Chatradhi]

On a system with course grain resolution of energy unit (milli J) the
accumulation thread can be executed less frequently than on the system
with fine grain resolution(micro J).

This patch sets the accumulation thread interval to an optimum value
calculated based on the (energy unit) resolution supported by the
hardware (assuming a peak wattage of 240W).

Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
Changes since v1:
1. Generalized formula as suggested by Guenter.
2. Use milli seconds for interval

 drivers/hwmon/amd_energy.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index 9580a16185b8..c413adfc6a73 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -46,8 +46,9 @@ struct amd_energy_data {
 	struct mutex lock;
 	/* An accumulator for each core and socket */
 	struct sensor_accumulator *accums;
+	unsigned int timeout_ms;
 	/* Energy Status Units */
-	u64 energy_units;
+	int energy_units;
 	int nr_cpus;
 	int nr_socks;
 	int core_id;
@@ -215,6 +216,7 @@ static umode_t amd_energy_is_visible(const void *_data,
 static int energy_accumulator(void *p)
 {
 	struct amd_energy_data *data = (struct amd_energy_data *)p;
+	unsigned int timeout = data->timeout_ms;
 
 	while (!kthread_should_stop()) {
 		/*
@@ -227,14 +229,7 @@ static int energy_accumulator(void *p)
 		if (kthread_should_stop())
 			break;
 
-		/*
-		 * On a 240W system, with default resolution the
-		 * Socket Energy status register may wrap around in
-		 * 2^32*15.3 e-6/240 = 273.8041 secs (~4.5 mins)
-		 *
-		 * let us accumulate for every 100secs
-		 */
-		schedule_timeout(msecs_to_jiffies(100000));
+		schedule_timeout(msecs_to_jiffies(timeout));
 	}
 	return 0;
 }
@@ -331,6 +326,13 @@ static int amd_energy_probe(struct platform_device *pdev)
 	if (IS_ERR(hwmon_dev))
 		return PTR_ERR(hwmon_dev);
 
+	/*
+	 * On a system with peak wattage of 250W
+	 * timeout = 2 ^ 32 / 2 ^ energy_units / 250 secs
+	 */
+	data->timeout_ms = 1000 *
+			   BIT(min(28, 31 - data->energy_units)) / 250;
+
 	data->wrap_accumulate = kthread_run(energy_accumulator, data,
 					    "%s", dev_name(hwmon_dev));
 	if (IS_ERR(data->wrap_accumulate))
-- 
2.17.1

[PATCH v2 3/4] hwmon: amd_energy: Improve the accumulation logic

[Naveen Krishna Chatradhi]

Factor out the common code in the accumulation functions for core and
socket accumulation.

While at it, handle the return value of the amd_create_sensor() function.

Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
Changes since v1:
1. Return values are handled in the probe.

 drivers/hwmon/amd_energy.c | 126 +++++++++++++------------------------
 1 file changed, 45 insertions(+), 81 deletions(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index c413adfc6a73..cbc6a6e466c5 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -74,108 +74,67 @@ static void get_energy_units(struct amd_energy_data *data)
 	data->energy_units = (rapl_units & AMD_ENERGY_UNIT_MASK) >> 8;
 }
 
-static void accumulate_socket_delta(struct amd_energy_data *data,
-				    int sock, int cpu)
+static void accumulate_delta(struct amd_energy_data *data,
+			     int channel, int cpu, u32 reg)
 {
-	struct sensor_accumulator *s_accum;
+	struct sensor_accumulator *accum;
 	u64 input;
 
 	mutex_lock(&data->lock);
-	rdmsrl_safe_on_cpu(cpu, ENERGY_PKG_MSR, &input);
+	rdmsrl_safe_on_cpu(cpu, reg, &input);
 	input &= AMD_ENERGY_MASK;
 
-	s_accum = &data->accums[data->nr_cpus + sock];
-	if (input >= s_accum->prev_value)
-		s_accum->energy_ctr +=
-			input - s_accum->prev_value;
+	accum = &data->accums[channel];
+	if (input >= accum->prev_value)
+		accum->energy_ctr +=
+			input - accum->prev_value;
 	else
-		s_accum->energy_ctr += UINT_MAX -
-			s_accum->prev_value + input;
+		accum->energy_ctr += UINT_MAX -
+			accum->prev_value + input;
 
-	s_accum->prev_value = input;
+	accum->prev_value = input;
 	mutex_unlock(&data->lock);
 }
 
-static void accumulate_core_delta(struct amd_energy_data *data)
+static void read_accumulate(struct amd_energy_data *data)
 {
-	struct sensor_accumulator *c_accum;
-	u64 input;
-	int cpu;
+	int sock, scpu, cpu;
+
+	for (sock = 0; sock < data->nr_socks; sock++) {
+		scpu = cpumask_first_and(cpu_online_mask,
+					 cpumask_of_node(sock));
+
+		accumulate_delta(data, data->nr_cpus + sock,
+				 scpu, ENERGY_PKG_MSR);
+	}
 
-	mutex_lock(&data->lock);
 	if (data->core_id >= data->nr_cpus)
 		data->core_id = 0;
 
 	cpu = data->core_id;
+	if (cpu_online(cpu))
+		accumulate_delta(data, cpu, cpu, ENERGY_CORE_MSR);
 
-	if (!cpu_online(cpu))
-		goto out;
-
-	rdmsrl_safe_on_cpu(cpu, ENERGY_CORE_MSR, &input);
-	input &= AMD_ENERGY_MASK;
-
-	c_accum = &data->accums[cpu];
-
-	if (input >= c_accum->prev_value)
-		c_accum->energy_ctr +=
-			input - c_accum->prev_value;
-	else
-		c_accum->energy_ctr += UINT_MAX -
-			c_accum->prev_value + input;
-
-	c_accum->prev_value = input;
-
-out:
 	data->core_id++;
-	mutex_unlock(&data->lock);
-}
-
-static void read_accumulate(struct amd_energy_data *data)
-{
-	int sock;
-
-	for (sock = 0; sock < data->nr_socks; sock++) {
-		int cpu;
-
-		cpu = cpumask_first_and(cpu_online_mask,
-					cpumask_of_node(sock));
-
-		accumulate_socket_delta(data, sock, cpu);
-	}
-
-	accumulate_core_delta(data);
 }
 
 static void amd_add_delta(struct amd_energy_data *data, int ch,
-			  int cpu, long *val, bool is_core)
+			  int cpu, long *val, u32 reg)
 {
-	struct sensor_accumulator *s_accum, *c_accum;
+	struct sensor_accumulator *accum;
 	u64 input;
 
 	mutex_lock(&data->lock);
-	if (!is_core) {
-		rdmsrl_safe_on_cpu(cpu, ENERGY_PKG_MSR, &input);
-		input &= AMD_ENERGY_MASK;
-
-		s_accum = &data->accums[ch];
-		if (input >= s_accum->prev_value)
-			input += s_accum->energy_ctr -
-				  s_accum->prev_value;
-		else
-			input += UINT_MAX - s_accum->prev_value +
-				  s_accum->energy_ctr;
-	} else {
-		rdmsrl_safe_on_cpu(cpu, ENERGY_CORE_MSR, &input);
-		input &= AMD_ENERGY_MASK;
+	rdmsrl_safe_on_cpu(cpu, reg, &input);
+	input &= AMD_ENERGY_MASK;
 
-		c_accum = &data->accums[ch];
-		if (input >= c_accum->prev_value)
-			input += c_accum->energy_ctr -
-				 c_accum->prev_value;
-		else
-			input += UINT_MAX - c_accum->prev_value +
-				 c_accum->energy_ctr;
-	}
+	accum = &data->accums[ch];
+	if (input >= accum->prev_value)
+		input += accum->energy_ctr -
+				accum->prev_value;
+	else
+		input += UINT_MAX - accum->prev_value +
+				accum->energy_ctr;
 
 	/* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
 	*val = div64_ul(input * 1000000UL, BIT(data->energy_units));
@@ -188,20 +147,22 @@ static int amd_energy_read(struct device *dev,
 			   u32 attr, int channel, long *val)
 {
 	struct amd_energy_data *data = dev_get_drvdata(dev);
+	u32 reg;
 	int cpu;
 
 	if (channel >= data->nr_cpus) {
 		cpu = cpumask_first_and(cpu_online_mask,
 					cpumask_of_node
 					(channel - data->nr_cpus));
-		amd_add_delta(data, channel, cpu, val, false);
+		reg = ENERGY_PKG_MSR;
 	} else {
 		cpu = channel;
 		if (!cpu_online(cpu))
 			return -ENODEV;
 
-		amd_add_delta(data, channel, cpu, val, true);
+		reg = ENERGY_CORE_MSR;
 	}
+	amd_add_delta(data, channel, cpu, val, reg);
 
 	return 0;
 }
@@ -242,7 +203,7 @@ static const struct hwmon_ops amd_energy_ops = {
 
 static int amd_create_sensor(struct device *dev,
 			     struct amd_energy_data *data,
-			     u8 type, u32 config)
+			     enum hwmon_sensor_types type, u32 config)
 {
 	struct hwmon_channel_info *info = &data->energy_info;
 	struct sensor_accumulator *accums;
@@ -301,6 +262,7 @@ static int amd_energy_probe(struct platform_device *pdev)
 	struct device *hwmon_dev;
 	struct amd_energy_data *data;
 	struct device *dev = &pdev->dev;
+	int ret;
 
 	data = devm_kzalloc(dev,
 			    sizeof(struct amd_energy_data), GFP_KERNEL);
@@ -313,8 +275,10 @@ static int amd_energy_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, data);
 	/* Populate per-core energy reporting */
 	data->info[0] = &data->energy_info;
-	amd_create_sensor(dev, data, hwmon_energy,
-			  HWMON_E_INPUT | HWMON_E_LABEL);
+	ret = amd_create_sensor(dev, data, hwmon_energy,
+				HWMON_E_INPUT | HWMON_E_LABEL);
+	if (ret)
+		return ret;
 
 	mutex_init(&data->lock);
 	get_energy_units(data);
@@ -338,7 +302,7 @@ static int amd_energy_probe(struct platform_device *pdev)
 	if (IS_ERR(data->wrap_accumulate))
 		return PTR_ERR(data->wrap_accumulate);
 
-	return PTR_ERR_OR_ZERO(data->wrap_accumulate);
+	return 0;
 }
 
 static int amd_energy_remove(struct platform_device *pdev)
-- 
2.17.1

[PATCH v2 4/4] hwmon: (amd_energy) Update driver documentation

[Naveen Krishna Chatradhi]

Update the documentation with the newly added features
* Set the accumulation interval based on resolution

Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
Changes since v1:
Dropped the accumulator status control and debugfs feature updates

 Documentation/hwmon/amd_energy.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/hwmon/amd_energy.rst b/Documentation/hwmon/amd_energy.rst
index f8288edff664..86e4ebc5cbc2 100644
--- a/Documentation/hwmon/amd_energy.rst
+++ b/Documentation/hwmon/amd_energy.rst
@@ -84,6 +84,11 @@ per run to a respective 64-bit counter. The kernel thread starts
 running during probe, wakes up every 100secs and stops running
 when driver is removed.
 
+Frequency of the accumulator thread is set during the probe
+based on the chosen energy unit resolution. For example
+A. fine grain (1.625 micro J)
+B. course grain (0.125 milli J)
+
 A socket and core energy read would return the current register
 value added to the respective energy accumulator.
 
-- 
2.17.1

Re: [PATCH v2 1/4] hwmon: amd_energy: Move label out of accumulation structure

[Guenter Roeck]

On Tue, Sep 29, 2020 at 04:23:19PM +0530, Naveen Krishna Chatradhi wrote:
> From: Akshay Gupta <Akshay.Gupta@amd.com>
> 
> At present, core & socket labels are defined in struct sensor_accumulator
> This patch moves it to the amd_energy_data structure, which will
> help in calling memset on struct sensor_accumulator to optimize the code.
> 
> Signed-off-by: Akshay Gupta <Akshay.Gupta@amd.com>

Series applied.

Thanks,
Guenter

> ---
> Changes since v1:
> None
> 
>  drivers/hwmon/amd_energy.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> index 29603742c858..9580a16185b8 100644
> --- a/drivers/hwmon/amd_energy.c
> +++ b/drivers/hwmon/amd_energy.c
> @@ -35,7 +35,6 @@
>  struct sensor_accumulator {
>  	u64 energy_ctr;
>  	u64 prev_value;
> -	char label[10];
>  };
>  
>  struct amd_energy_data {
> @@ -52,6 +51,7 @@ struct amd_energy_data {
>  	int nr_cpus;
>  	int nr_socks;
>  	int core_id;
> +	char (*label)[10];
>  };
>  
>  static int amd_energy_read_labels(struct device *dev,
> @@ -61,7 +61,7 @@ static int amd_energy_read_labels(struct device *dev,
>  {
>  	struct amd_energy_data *data = dev_get_drvdata(dev);
>  
> -	*str = data->accums[channel].label;
> +	*str = data->label[channel];
>  	return 0;
>  }
>  
> @@ -253,6 +253,7 @@ static int amd_create_sensor(struct device *dev,
>  	struct sensor_accumulator *accums;
>  	int i, num_siblings, cpus, sockets;
>  	u32 *s_config;
> +	char (*label_l)[10];
>  
>  	/* Identify the number of siblings per core */
>  	num_siblings = ((cpuid_ebx(0x8000001e) >> 8) & 0xff) + 1;
> @@ -276,21 +277,25 @@ static int amd_create_sensor(struct device *dev,
>  	if (!accums)
>  		return -ENOMEM;
>  
> +	label_l = devm_kcalloc(dev, cpus + sockets,
> +			       sizeof(*label_l), GFP_KERNEL);
> +	if (!label_l)
> +		return -ENOMEM;
> +
>  	info->type = type;
>  	info->config = s_config;
>  
>  	data->nr_cpus = cpus;
>  	data->nr_socks = sockets;
>  	data->accums = accums;
> +	data->label = label_l;
>  
>  	for (i = 0; i < cpus + sockets; i++) {
>  		s_config[i] = config;
>  		if (i < cpus)
> -			scnprintf(accums[i].label, 10,
> -				  "Ecore%03u", i);
> +			scnprintf(label_l[i], 10, "Ecore%03u", i);
>  		else
> -			scnprintf(accums[i].label, 10,
> -				  "Esocket%u", (i - cpus));
> +			scnprintf(label_l[i], 10, "Esocket%u", (i - cpus));
>  	}
>  
>  	return 0;