| Message ID | 1475322326-160112-6-git-send-email-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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. > 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? > + } > + /* > + * 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? > + sysctl_sched_itmt_enabled = 0; > + x86_topology_update = true; > + rebuild_sched_domains(); > + } > > mutex_unlock(&itmt_update_mutex); Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > > + } > > > + /* > > > + * 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. Thanks, tglx
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h index 637d847..e45151f 100644 --- a/arch/x86/include/asm/topology.h +++ b/arch/x86/include/asm/topology.h @@ -155,6 +155,7 @@ extern bool x86_topology_update; #include <asm/percpu.h> DECLARE_PER_CPU_READ_MOSTLY(int, sched_core_priority); +extern unsigned int __read_mostly sysctl_sched_itmt_enabled; /* Interface to set priority of a cpu */ void sched_set_itmt_core_prio(int prio, int core_cpu); @@ -164,6 +165,7 @@ void sched_set_itmt_support(bool itmt_supported); #else /* CONFIG_SCHED_ITMT */ +#define sysctl_sched_itmt_enabled 0 static inline void sched_set_itmt_core_prio(int prio, int core_cpu) { } diff --git a/arch/x86/kernel/itmt.c b/arch/x86/kernel/itmt.c index f485b49..ab0ae2a 100644 --- a/arch/x86/kernel/itmt.c +++ b/arch/x86/kernel/itmt.c @@ -33,6 +33,67 @@ static DEFINE_MUTEX(itmt_update_mutex); /* Boolean to track if system has ITMT capabilities */ static bool __read_mostly sched_itmt_capable; +/* + * Boolean to control whether we want to move processes to cpu capable + * of higher turbo frequency for cpus supporting Intel Turbo Boost Max + * Technology 3.0. + * + * It can be set via /proc/sys/kernel/sched_itmt_enabled + */ +unsigned int __read_mostly sysctl_sched_itmt_enabled; + +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; + } + + old_sysctl = sysctl_sched_itmt_enabled; + ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos); + + if (!ret && write && old_sysctl != sysctl_sched_itmt_enabled) { + x86_topology_update = true; + rebuild_sched_domains(); + } + + mutex_unlock(&itmt_update_mutex); + + return ret; +} + +static unsigned int zero; +static unsigned int one = 1; +static struct ctl_table itmt_kern_table[] = { + { + .procname = "sched_itmt_enabled", + .data = &sysctl_sched_itmt_enabled, + .maxlen = sizeof(unsigned int), + .mode = 0644, + .proc_handler = sched_itmt_update_handler, + .extra1 = &zero, + .extra2 = &one, + }, + {} +}; + +static struct ctl_table itmt_root_table[] = { + { + .procname = "kernel", + .mode = 0555, + .child = itmt_kern_table, + }, + {} +}; + +static struct ctl_table_header *itmt_sysctl_header; + /** * sched_set_itmt_support - Indicate platform support ITMT * @itmt_supported: indicate platform's CPU has ITMT capability @@ -45,13 +106,46 @@ static bool __read_mostly sched_itmt_capable; * * This must be done only after sched_set_itmt_core_prio * has been called to set the cpus' priorities. + * + * It must not be called with cpu hot plug lock + * held as we need to acquire the lock to rebuild sched domains + * later. */ 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; + } + /* + * 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) + sysctl_sched_itmt_enabled = 0; + x86_topology_update = true; + rebuild_sched_domains(); + } mutex_unlock(&itmt_update_mutex); }