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

[Fenghua Yu <fenghua.yu@intel.com>]

On Tue, Jun 19, 2018 at 11:03:22AM +0200, Thomas Gleixner wrote:
> 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?

Current hardware implementation enables C0.2 by default. But I'm
not sure future every hardware always enables it by default.

In kernel, init code enforces to enable C0.2 globally by default.
Each umwait or tpause instruction will specify which state (C0.1 or C0.2)
to use during wait depending on user's decision. And a sysfs interface
allows user to disable C0.2 globally during run time.

Is this right arrangement?

> 
> > 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?

Sure. I will do that.

> 
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * umwait.c - control user wait
> 
> Please remove these pointless file references. They get stale before its merged.
Sure. I will remove these.

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

I will change to "umwait.c controls user wait states that user enters..."?

> 
> > + */
> > +#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....

You are right, the variable can only have 0 or 1. Is it ok to have the mask?

> 
> > +	for_each_online_cpu(cpu)
> > +		wrmsr_on_cpu(cpu, MSR_IA32_UMWAIT_CONTROL, msr_val, 0);
> 
> This lacks protection against CPU hotplug.

You are right. I will add protection.

> 
> > +	mutex_unlock(&umwait_lock);
> > +
> > +	return count;
> 
> Thanks,
> 
> 	tglx

Thanks.

-Fenghua