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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Jun 2018 11:03:22 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     Ingo Molnar <mingo@...hat.com>, H Peter Anvin <hpa@...or.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Alan Cox <alan@...ux.intel.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [RFC PATCH 4/8] cpuidle: Set up maximum umwait time and umwait
 states

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@...el.com>
> + */
> +/*
> + * 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