linux-kernel - Re: [PATCH v5 5/9] x86/sysctl: Add sysctl for ITMT scheduling feature

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]

Message-ID: <1475684673.3916.299.camel@linux.intel.com>
Date:   Wed, 05 Oct 2016 09:24:33 -0700
From:   Tim Chen <tim.c.chen@...ux.intel.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc:     rjw@...ysocki.net, mingo@...hat.com, bp@...e.de, x86@...nel.org,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org, peterz@...radead.org, jolsa@...hat.com
Subject: Re: [PATCH v5 5/9] x86/sysctl: Add sysctl for ITMT scheduling
 feature

On Wed, 2016-10-05 at 16:35 +0200, Thomas Gleixner wrote:
> On Sat, 1 Oct 2016, Srinivas Pandruvada wrote:
> > 
> > +static int sched_itmt_update_handler(struct ctl_table *table, int write,
> > +			      void __user *buffer, size_t *lenp, loff_t *ppos)
> > +{
> > +	int ret;
> > +	unsigned int old_sysctl;
> > +
> > +	mutex_lock(&itmt_update_mutex);
> > +
> > +	if (!sched_itmt_capable) {
> > +		mutex_unlock(&itmt_update_mutex);
> > +		return 0;
> This should return a proper error code.

Okay. Will return EINVAL instead.

> 
> > 
> >  void sched_set_itmt_support(bool itmt_supported)
> >  {
> >  	mutex_lock(&itmt_update_mutex);
> >  
> > -	if (itmt_supported != sched_itmt_capable)
> > -		sched_itmt_capable = itmt_supported;
> > +	if (itmt_supported == sched_itmt_capable) {
> > +		mutex_unlock(&itmt_update_mutex);
> > +		return;
> > +	}
> > +	sched_itmt_capable = itmt_supported;
> > +
> > +	if (itmt_supported) {
> > +		itmt_sysctl_header =
> > +			register_sysctl_table(itmt_root_table);
> > +		if (!itmt_sysctl_header) {
> > +			mutex_unlock(&itmt_update_mutex);
> > +			return;
> So you now have a state of capable which cannot be enabled. Whats the
> point?

For multi-socket system where ITMT is not enabled by default, the operator
can still decide to enable it via sysctl.

> 
> > 
> > +		}
> > +		/*
> > +		 * ITMT capability automatically enables ITMT
> > +		 * scheduling for small systems (single node).
> > +		 */
> > +		if (topology_num_packages() == 1)
> > +			sysctl_sched_itmt_enabled = 1;
> > +	} else {
> > +		if (itmt_sysctl_header)
> > +			unregister_sysctl_table(itmt_sysctl_header);
> > +	}
> > +
> > +	if (sysctl_sched_itmt_enabled) {
> > +		/* disable sched_itmt if we are no longer ITMT capable */
> > +		if (!itmt_supported)
> 
> How do you get here if itmt is not supported? 

If the OS decides to turn off ITMT for any reason, (i.e. invoke 
sched_set_itmt_support(false) after it has turned on itmt_support
before), this is the logic to do it.  We don't turn off ITMT support
after it has been turned on today, in the future the OS may.

If you prefer, I can change things to sched_set_itmt_support(void) so
we can only turn on ITMT support. And once the support is on, we
don't revoke it.

Thanks.

Tim