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

[Tim Chen <tim.c.chen@linux.intel.com>]

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