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

[Tim Chen]

On Thu, 2016-10-06 at 13:13 +0200, Thomas Gleixner wrote:
> On Wed, 5 Oct 2016, Tim Chen wrote:
> > 
> > On Wed, 2016-10-05 at 16:35 +0200, Thomas Gleixner wrote:
> > > 
> > > > 
> > > > +	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.
> With a sysctl which failed to be installed. Good luck with that.

I misunderstood your earlier comment.
You are talking about the case where we fail to register the sysctl?

In this case, the system is in a state that indicates it isÂ
ITMT capable but cannot be enabled. ÂSo we return and do not turn on ITMT
scheduling. ÂThe system operator should always have the capability
to enable/disable ITMT via sysctl. ÂSo we do not turn on ITMT if operator has
no control over it, even if the system is capable of ITMT.


> Â
> > 
> > > 
> > > > 
> > > > +		}
> > > > +		/*
> > > > +		Â* 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.
> Then please make this two functions (set/clear) so one can actually follow
> the logic. The above is just too convoluted.

Sure, I will add a clear function and move the clearing logic there.

Thanks.

Tim