Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 12 Oct 2020 14:05:01 +0100
From:   Quentin Perret <qperret@google.com>
To:     zhuguangqing83@gmail.com
Cc:     lukasz.luba@arm.com, quentin.perret@arm.com, rjw@rjwysocki.net,
        pavel@ucw.cz, len.brown@intel.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        zhuguangqing <zhuguangqing@xiaomi.com>
Subject: Re: [PATCH] PM / EM: consult something about cpumask in
 em_dev_register_perf_domain
Message-ID: <20201012130501.GA3366383@google.com>
References: <20201012124136.4147-1-zhuguangqing83@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201012124136.4147-1-zhuguangqing83@gmail.com>
Precedence: bulk

Hi,

On Monday 12 Oct 2020 at 20:41:36 (+0800), zhuguangqing83@gmail.com wrote:
> From: zhuguangqing <zhuguangqing@xiaomi.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