[v2] hwmon: coretemp: avoid RDMSR interruptions to isolated CPUs

[PATCH v2] hwmon: coretemp: avoid RDMSR interruptions to isolated CPUs

Posted by Marcelo Tosatti 2 years, 9 months ago


The coretemp driver uses rdmsr_on_cpu calls to read
MSR_IA32_PACKAGE_THERM_STATUS/MSR_IA32_THERM_STATUS registers,
which contain information about current core temperature.

For certain low latency applications, the RDMSR interruption exceeds
the applications requirements.

So disallow reading of crit_alarm and temp files via /sys, returning 
-EINVAL, in case CPU isolation is enabled.

Temperature information from the housekeeping cores should be
sufficient to infer die temperature.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

---
v2: improve changelog to mention that an error is returned,
    and sysfs file is not disabled (Guenter Roeck)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 9bee4d33fbdf..30a35f4130d5 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -27,6 +27,7 @@
 #include <asm/msr.h>
 #include <asm/processor.h>
 #include <asm/cpu_device_id.h>
+#include <linux/sched/isolation.h>
 
 #define DRVNAME	"coretemp"
 
@@ -121,6 +122,10 @@ static ssize_t show_crit_alarm(struct device *dev,
 	struct platform_data *pdata = dev_get_drvdata(dev);
 	struct temp_data *tdata = pdata->core_data[attr->index];
 
+
+	if (!housekeeping_cpu(tdata->cpu, HK_TYPE_MISC))
+		return -EINVAL;
+
 	mutex_lock(&tdata->update_lock);
 	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
 	mutex_unlock(&tdata->update_lock);
@@ -158,6 +163,8 @@ static ssize_t show_temp(struct device *dev,
 
 	/* Check whether the time interval has elapsed */
 	if (!tdata->valid || time_after(jiffies, tdata->last_updated + HZ)) {
+		if (!housekeeping_cpu(tdata->cpu, HK_TYPE_MISC))
+			return -EINVAL;
 		rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
 		/*
 		 * Ignore the valid bit. In all observed cases the register

Re: [PATCH v2] hwmon: coretemp: avoid RDMSR interruptions to isolated CPUs

Posted by Guenter Roeck 2 years, 9 months ago

On 12/16/22 06:07, Marcelo Tosatti wrote:
> 
> The coretemp driver uses rdmsr_on_cpu calls to read
> MSR_IA32_PACKAGE_THERM_STATUS/MSR_IA32_THERM_STATUS registers,
> which contain information about current core temperature.
> 
> For certain low latency applications, the RDMSR interruption exceeds
> the applications requirements.
> 
> So disallow reading of crit_alarm and temp files via /sys, returning
> -EINVAL, in case CPU isolation is enabled.
> 
> Temperature information from the housekeeping cores should be
> sufficient to infer die temperature.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> ---
> v2: improve changelog to mention that an error is returned,
>      and sysfs file is not disabled (Guenter Roeck)
> 

You did not address my feedback. I requested a code change.
Returning -EINVAL is unacceptable, and a solution not creating
the sysfs attributes to start with would be preferred.

Guenter

> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 9bee4d33fbdf..30a35f4130d5 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -27,6 +27,7 @@
>   #include <asm/msr.h>
>   #include <asm/processor.h>
>   #include <asm/cpu_device_id.h>
> +#include <linux/sched/isolation.h>
>   
>   #define DRVNAME	"coretemp"
>   
> @@ -121,6 +122,10 @@ static ssize_t show_crit_alarm(struct device *dev,
>   	struct platform_data *pdata = dev_get_drvdata(dev);
>   	struct temp_data *tdata = pdata->core_data[attr->index];
>   
> +
> +	if (!housekeeping_cpu(tdata->cpu, HK_TYPE_MISC))
> +		return -EINVAL;
> +
>   	mutex_lock(&tdata->update_lock);
>   	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
>   	mutex_unlock(&tdata->update_lock);
> @@ -158,6 +163,8 @@ static ssize_t show_temp(struct device *dev,
>   
>   	/* Check whether the time interval has elapsed */
>   	if (!tdata->valid || time_after(jiffies, tdata->last_updated + HZ)) {
> +		if (!housekeeping_cpu(tdata->cpu, HK_TYPE_MISC))
> +			return -EINVAL;
>   		rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
>   		/*
>   		 * Ignore the valid bit. In all observed cases the register
> 
> 
> 
>

[PATCH v3] hwmon: coretemp: avoid RDMSR interruptions to isolated CPUs

Posted by Marcelo Tosatti 2 years, 9 months ago


The coretemp driver uses rdmsr_on_cpu calls to read
MSR_IA32_PACKAGE_THERM_STATUS/MSR_IA32_THERM_STATUS registers,
which contain information about current core temperature.

For certain low latency applications, the RDMSR interruption exceeds
the applications requirements.

So do not create core files in sysfs, for CPUs which have
isolation and nohz_full enabled.

Temperature information from the housekeeping cores should be
sufficient to infer die temperature.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

---
v3: do not create sysfs files for isolated CPUs (Guenter Roeck)
v2: improve changelog to mention that an error is returned,
     and sysfs file is not disabled (Guenter Roeck)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 9bee4d33fbdf..619dfde7a712 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -27,6 +27,7 @@
 #include <asm/msr.h>
 #include <asm/processor.h>
 #include <asm/cpu_device_id.h>
+#include <linux/sched/isolation.h>
 
 #define DRVNAME	"coretemp"
 
@@ -458,6 +459,9 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 	u32 eax, edx;
 	int err, index, attr_no;
 
+	if (!housekeeping_cpu(cpu, HK_TYPE_MISC))
+		return 0;
+
 	/*
 	 * Find attr number for sysfs:
 	 * We map the attr number to core id of the CPU

Re: [PATCH v3] hwmon: coretemp: avoid RDMSR interruptions to isolated CPUs

Posted by Guenter Roeck 2 years, 8 months ago

On Fri, Dec 16, 2022 at 05:24:08PM -0300, Marcelo Tosatti wrote:
> The coretemp driver uses rdmsr_on_cpu calls to read
> MSR_IA32_PACKAGE_THERM_STATUS/MSR_IA32_THERM_STATUS registers,
> which contain information about current core temperature.
> 
> For certain low latency applications, the RDMSR interruption exceeds
> the applications requirements.
> 
> So do not create core files in sysfs, for CPUs which have
> isolation and nohz_full enabled.
> 
> Temperature information from the housekeeping cores should be
> sufficient to infer die temperature.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
> v3: do not create sysfs files for isolated CPUs (Guenter Roeck)
> v2: improve changelog to mention that an error is returned,
>      and sysfs file is not disabled (Guenter Roeck)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 9bee4d33fbdf..619dfde7a712 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -27,6 +27,7 @@
>  #include <asm/msr.h>
>  #include <asm/processor.h>
>  #include <asm/cpu_device_id.h>
> +#include <linux/sched/isolation.h>
>  
>  #define DRVNAME	"coretemp"
>  
> @@ -458,6 +459,9 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>  	u32 eax, edx;
>  	int err, index, attr_no;
>  
> +	if (!housekeeping_cpu(cpu, HK_TYPE_MISC))
> +		return 0;
> +
>  	/*
>  	 * Find attr number for sysfs:
>  	 * We map the attr number to core id of the CPU