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: Mon, 12 Oct 2020 14:05:01 +0100 From: Quentin Perret <qperret@...gle.com> To: zhuguangqing83@...il.com Cc: lukasz.luba@....com, quentin.perret@....com, rjw@...ysocki.net, pavel@....cz, len.brown@...el.com, linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, zhuguangqing <zhuguangqing@...omi.com> Subject: Re: [PATCH] PM / EM: consult something about cpumask in em_dev_register_perf_domain Hi, On Monday 12 Oct 2020 at 20:41:36 (+0800), zhuguangqing83@...il.com wrote: > From: zhuguangqing <zhuguangqing@...omi.com> > > Hi, Lukasz, Quentin > I have three questions to consult about cpumask in energy_model.c. OK, let's see if we can help :) > 1, The first one is about the meanings of the following two parameters, > [1] and [2]. > [1]: "cpumask_t *cpus" in function em_dev_register_perf_domain(): Pointer > to cpumask_t, which in case of a CPU device is obligatory. It can be taken > from i.e. 'policy->cpus'. > [2]: "unsigned long cpus[]" in struct em_perf_domain: Cpumask covering the > CPUs of the domain. It's here for performance reasons to avoid potential > cache misses during energy calculations in the scheduler and simplifies > allocating/freeing that memory region. > > From the current code, we see [2] is copied from [1]. But from comments, > accorinding to my understanding, [2] and [1] have different meanings. > [1] can be taken from i.e. 'policy->cpus', according to the comment in the > defination of struct cpufreq_policy, it means Online CPUs only. Actually, > 'policy->cpus' is not always Online CPUs. > [2] means each_possible_cpus in the same domain, including phycical > hotplug cpus(just possible), logically hotplug cpus(just present) and > online cpus. > > > So, the first question is, what are the meanings of [1] and [2]? > I guess maybe there are the following 4 possible choices. > A), for_each_possible_cpu in the same domain, maybe phycical hotplug > B), for_each_present_cpu in the same domain, maybe logically hotplug > C), for_each_online_cpu in the same domain, online > D), others So, if the comments are confusing we should update them, but from the EM framework perspective, all cpumasks must be the _possible_ CPUs in the domain. It's up to the clients (e.g. the scheduler) to deal with hotplug and so on, but the EM framework should cover non-online CPUs too. > 2, The second question is about the function em_dev_register_perf_domain(). > If multiple clients register the same performance domain with different > *dev or *cpus, how should we handle? > > int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states, > struct em_data_callback *cb, cpumask_t *cpus) > > For example, say cpu0 and cpu1 are in the same performance domain, > cpu0 is registered first. As part of the init process, > em_dev_register_perf_domain() is called, then *dev = cpu0_dev, > *cpus = 01b(cpu1 is initially offline). It creates a em_pd for cpu0_dev. > After a while, cpu1 is online, em_dev_register_perf_domain() is called > again as part of init process for cpu1, then *dev =cpu1_dev, > *cpus = 11b(cpu1 is online). In this case, for the current code, > cpu1_dev can not get its em_pd. As per the above, the registration should be done once, with the mask of all possible CPUs in the domain. If CPUs 0 and 1 share the same domain, a single call to em_dev_register_perf_domain() should be sufficient to register both of them at once. > 3, The third question is, how can we ensure cpu_dev as follows is not > NULL? If we can't ensure that, maybe we should add a check before using > it. > /kernel/power/energy_model.c > 174) static int em_create_pd(struct device *dev, int nr_states, > 175) struct em_data_callback *cb, cpumask_t *cpus) > 176) { > 199) if (_is_cpu_device(dev)) > 200) for_each_cpu(cpu, cpus) { > 201) cpu_dev = get_cpu_device(cpu); > 202) cpu_dev->em_pd = pd; > 203) } And that should not be necessary as we check for the !dev case at the top of em_dev_register_perf_domain(). Or were you thinking about something else? Thanks, Quentin
Powered by blists - more mailing lists