| 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
| ||
|
Message-ID: <59013edba75633f2f1afec284c7aaa0baf36f06e.camel@linux.intel.com>
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