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
| ||
|
Date: Sat, 04 Feb 2023 19:01:15 -0800 From: srinivas pandruvada <srinivas.pandruvada@...ux.intel.com> To: "Zhang, Rui" <rui.zhang@...el.com>, "rafael@...nel.org" <rafael@...nel.org> Cc: "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org> Subject: Re: [PATCH v5 4/4] thermal/drivers/intel_powerclamp: Add additional module params Hi Rui, > [...] > #ifdef CONFIG_CPUMASK_OFFSTACK > typedef struct cpumask *cpumask_var_t; > #else > typedef struct cpumask cpumask_var_t[1]; > fi > > I happened to build with CONFIG_CPUMASK_OFFSTACK cleared, and got > following errors > > Try v2 for the module parameters Thanks, Srinivas > $ make > CALL scripts/checksyscalls.sh > DESCEND objtool > AR drivers/thermal/intel/built-in.a > CC [M] drivers/thermal/intel/intel_powerclamp.o > drivers/thermal/intel/intel_powerclamp.c: In function > ‘allocate_idle_injection_mask’: > drivers/thermal/intel/intel_powerclamp.c:122:6: error: the address of > ‘idle_injection_cpu_mask’ will always evaluate as ‘true’ [- > Werror=address] > 122 | if (!idle_injection_cpu_mask) { > | ^ > drivers/thermal/intel/intel_powerclamp.c: In function ‘cpumask_get’: > drivers/thermal/intel/intel_powerclamp.c:183:6: error: the address of > ‘idle_injection_cpu_mask’ will always evaluate as ‘true’ [- > Werror=address] > 183 | if (!idle_injection_cpu_mask) > | ^ > drivers/thermal/intel/intel_powerclamp.c: In function ‘max_idle_set’: > drivers/thermal/intel/intel_powerclamp.c:220:6: error: the address of > ‘idle_injection_cpu_mask’ will always evaluate as ‘true’ [- > Werror=address] > 220 | if (idle_injection_cpu_mask && > cpumask_equal(cpu_present_mask, idle_injection_cpu_mask) && > | ^~~~~~~~~~~~~~~~~~~~~~~ > drivers/thermal/intel/intel_powerclamp.c: In function > ‘powerclamp_exit’: > drivers/thermal/intel/intel_powerclamp.c:825:6: error: the address of > ‘idle_injection_cpu_mask’ will always evaluate as ‘true’ [- > Werror=address] > 825 | if (idle_injection_cpu_mask) > | ^~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > make[4]: *** [scripts/Makefile.build:252: > drivers/thermal/intel/intel_powerclamp.o] Error 1 > make[3]: *** [scripts/Makefile.build:504: drivers/thermal/intel] > Error 2 > make[2]: *** [scripts/Makefile.build:504: drivers/thermal] Error 2 > make[1]: *** [scripts/Makefile.build:504: drivers] Error 2 > make: *** [Makefile:2021: .] Error 2 > > > + > > +static int allocate_idle_injection_mask(void) > > +{ > > + /* This mask is allocated only one time and freed during > > module > > exit */ > > + if (!idle_injection_cpu_mask) { > > + if (!zalloc_cpumask_var(&idle_injection_cpu_mask, > > GFP_KERNEL)) > > + return -ENOMEM; > > + > > + cpumask_copy(idle_injection_cpu_mask, > > cpu_present_mask); > > + } > > + > > + return 0; > > +} > > + > > +static int cpumask_set(const char *arg, const struct kernel_param > > *kp) > > +{ > > + int ret; > > + > > + mutex_lock(&powerclamp_lock); > > + > > + /* Can't set mask when cooling device is in use */ > > + if (powerclamp_data.clamping) { > > + ret = -EAGAIN; > > + goto skip_cpumask_set; > > + } > > + > > + /* > > + * When module parameters are passed from kernel command > > line > > + * during insmod, the module parameter callback is called > > + * before powerclamp_init(), so we can't assume that some > > + * cpumask can be allocated before here. > > + */ > > + ret = allocate_idle_injection_mask(); > > + if (ret) > > + goto skip_cpumask_set; > > + > > Just a suggestion, can we have a quick path for restoring to all cpu > mode? > > Say, echo > /sys/module/intel_powerclamp/parameters/cpumask > > > + ret = bitmap_parse(arg, strlen(arg), > > cpumask_bits(idle_injection_cpu_mask), > > + nr_cpumask_bits); > > + if (ret) > > + goto skip_cpumask_set; > > + > > + if (cpumask_empty(idle_injection_cpu_mask)) { > > + ret = -EINVAL; > > + goto skip_cpumask_set; > > + } > > + > > + if (cpumask_equal(cpu_present_mask, > > idle_injection_cpu_mask) && > > + max_idle > MAX_ALL_CPU_IDLE) { > > + ret = -EINVAL; > > + goto skip_cpumask_set; > > + } > > + > > + mutex_unlock(&powerclamp_lock); > > + > > + return 0; > > + > > +skip_cpumask_set: > > + mutex_unlock(&powerclamp_lock); > > + > > + return ret; > > +} > > + > > +static int cpumask_get(char *buf, const struct kernel_param *kp) > > +{ > > + if (!idle_injection_cpu_mask) > > + return -EINVAL; > > + > > + return bitmap_print_to_pagebuf(false, buf, > > cpumask_bits(idle_injection_cpu_mask), > > + nr_cpumask_bits); > > +} > > + > > +static const struct kernel_param_ops cpumask_ops = { > > + .set = cpumask_set, > > + .get = cpumask_get, > > +}; > > + > > +module_param_cb(cpumask, &cpumask_ops, NULL, 0644); > > +MODULE_PARM_DESC(cpumask, "Mask of CPUs to use for idle > > injection."); > > + > > +static int max_idle_set(const char *arg, const struct kernel_param > > *kp) > > +{ > > + u8 _max_idle; > > + int ret = 0; > > + > > + mutex_lock(&powerclamp_lock); > > + > > + /* Can't set mask when cooling device is in use */ > > + if (powerclamp_data.clamping) { > > + ret = -EAGAIN; > > + goto skip_limit_set; > > + } > > + > > + ret = kstrtou8(arg, 10, &_max_idle); > > + if (ret) > > + goto skip_limit_set; > > + > > + if (_max_idle > MAX_TARGET_RATIO) { > > + ret = -EINVAL; > > + goto skip_limit_set; > > + } > > + > > + if (idle_injection_cpu_mask && > > cpumask_equal(cpu_present_mask, > > idle_injection_cpu_mask) && > > + _max_idle > MAX_ALL_CPU_IDLE) { > > + ret = -EINVAL; > > + goto skip_limit_set; > > + } > > + > > + max_idle = _max_idle; > > + > > +skip_limit_set: > > + mutex_unlock(&powerclamp_lock); > > + > > + return ret; > > +} > > + > > +static const struct kernel_param_ops max_idle_ops = { > > + .set = max_idle_set, > > + .get = param_get_int, > > +}; > > + > > +module_param_cb(max_idle, &max_idle_ops, &max_idle, 0644); > > +MODULE_PARM_DESC(max_idle, "maximum injected idle time to the > > total > > CPU time ratio in percent range:1-100"); > > + > > struct powerclamp_calibration_data { > > unsigned long confidence; /* used for calibration, > > basically a > > counter > > * gets incremented each time a > > clamping > > @@ -342,6 +471,10 @@ static unsigned int get_run_time(void) > > unsigned int compensated_ratio; > > unsigned int runtime; > > > > + /* No compensation for non systemwide idle injection */ > > + if (max_idle > MAX_ALL_CPU_IDLE) > > + return (duration * 100 / > > powerclamp_data.target_ratio - > > duration); > > The comment and the code is not aligned. > we can still set max_idle to a value lower than MAX_ALL_CPU_IDLE for > non systemwide idle injection, right? > > thanks, > rui > > > + > > /* > > * make sure user selected ratio does not take effect until > > * the next round. adjust target_ratio if user has changed > > @@ -460,21 +593,11 @@ static void trigger_idle_injection(void) > > */ > > static int powerclamp_idle_injection_register(void) > > { > > - /* > > - * The idle inject core will only inject for online CPUs, > > - * So we can register for all present CPUs. In this way > > - * if some CPU goes online/offline while idle inject > > - * is registered, nothing additional calls are required. > > - * The same runtime and idle time is applicable for > > - * newly onlined CPUs if any. > > - * > > - * Here cpu_present_mask can be used as is. > > - * cast to (struct cpumask *) is required as the > > - * cpu_present_mask is const struct cpumask *, otherwise > > - * there will be compiler warnings. > > - */ > > - ii_dev = idle_inject_register_full((struct cpumask > > *)cpu_present_mask, > > - idle_inject_update); > > + if (cpumask_equal(cpu_present_mask, > > idle_injection_cpu_mask)) > > + ii_dev = > > idle_inject_register_full(idle_injection_cpu_mask, > > idle_inject_update); > > + else > > + ii_dev = > > idle_inject_register(idle_injection_cpu_mask); > > + > > if (!ii_dev) { > > pr_err("powerclamp: idle_inject_register > > failed\n"); > > return -EAGAIN; > > @@ -510,7 +633,7 @@ static int start_power_clamp(void) > > ret = powerclamp_idle_injection_register(); > > if (!ret) { > > trigger_idle_injection(); > > - if (poll_pkg_cstate_enable) > > + if (poll_pkg_cstate_enable && max_idle < > > MAX_ALL_CPU_IDLE) > > schedule_delayed_work(&poll_pkg_cstate_work > > , > > 0); > > } > > > > @@ -565,7 +688,7 @@ static int powerclamp_set_cur_state(struct > > thermal_cooling_device *cdev, > > mutex_lock(&powerclamp_lock); > > > > new_target_ratio = clamp(new_target_ratio, 0UL, > > - (unsigned long) (MAX_TARGET_RATIO - > > 1)); > > + (unsigned long) (max_idle - 1)); > > if (!powerclamp_data.target_ratio && new_target_ratio > 0) > > { > > pr_info("Start idle injection to reduce power\n"); > > powerclamp_data.target_ratio = new_target_ratio; > > @@ -656,6 +779,13 @@ static int __init powerclamp_init(void) > > > > /* probe cpu features and ids here */ > > retval = powerclamp_probe(); > > + if (retval) > > + return retval; > > + > > + mutex_lock(&powerclamp_lock); > > + retval = allocate_idle_injection_mask(); > > + mutex_unlock(&powerclamp_lock); > > + > > if (retval) > > return retval; > > > > @@ -689,6 +819,9 @@ static void __exit powerclamp_exit(void) > > > > cancel_delayed_work_sync(&poll_pkg_cstate_work); > > debugfs_remove_recursive(debug_dir); > > + > > + if (idle_injection_cpu_mask) > > + free_cpumask_var(idle_injection_cpu_mask); > > } > > module_exit(powerclamp_exit); > >
Powered by blists - more mailing lists