Re: [RFC PATCH 4/8] cpuidle: Set up maximum umwait time and umwait states

[Thomas Gleixner]

On Fri, 15 Jun 2018, Fenghua Yu wrote:
> By default C0.2 is enabled so user wait can save more power but wakeup
> time is slower. In some cases e.g. real time, user wants to disable C0.2
> so that user wait saves less power but wakeup time is faster.

Why is this default enabled?

> A new "/sys/devices/system/cpu/cpuidle/umwait_disable_c0_2" file is
> created to allow user to check if C0.2 is enabled or disabled and also
> allow user to enable or disable C0.2. Value "1" in the file means C0.2 is
> disabled. Value "0" means C0.2 is enabled.

Can we please use straight forward positive logic and have a enable file?

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * umwait.c - control user wait

Please remove these pointless file references. They get stale before its merged.

> + *
> + * Copyright (c) 2018, Intel Corporation.
> + * Fenghua Yu <fenghua.yu@xxxxxxxxx>
> + */
> +/*
> + * umwait.c adds control of user wait states that user enters through user wait
> + * instructions umwait or tpause.

umwait.c adds something?

> + */
> +#include <linux/cpu.h>
> +#include <asm/msr.h>
> +
> +static int umwait_disable_c0_2;
> +static DEFINE_MUTEX(umwait_lock);
> +
> +static ssize_t umwait_disable_c0_2_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	return sprintf(buf, "%d\n", umwait_disable_c0_2);
> +}
> +
> +static ssize_t umwait_disable_c0_2_store(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	int disable_c0_2, cpu, ret;
> +	u32 msr_val;
> +
> +	ret = kstrtou32(buf, 10, &disable_c0_2);
> +	if (ret)
> +		return ret;
> +	if (disable_c0_2 != 1 && disable_c0_2 != 0)
> +		return -EINVAL;
> +
> +	mutex_lock(&umwait_lock);
> +	umwait_disable_c0_2 = disable_c0_2;
> +	/*
> +	 * No global umwait maximum time limit (0 in bits 31-0).
> +	 * Enable or disable C0.2 based on global setting (bit 0) on all CPUs.
> +	 */
> +	msr_val = umwait_disable_c0_2 & UMWAIT_CONTROL_C02_MASK;

That mask is there because the variable can only have 0 and 1 as content....

> +	for_each_online_cpu(cpu)
> +		wrmsr_on_cpu(cpu, MSR_IA32_UMWAIT_CONTROL, msr_val, 0);

This lacks protection against CPU hotplug.

> +	mutex_unlock(&umwait_lock);
> +
> +	return count;

Thanks,

	tglx