Re: [RFC PATCH v3 8/8] xen/arm: cpufreq: add xen-cpufreq driver

[Julien Grall <julien.grall@linaro.org>]

Hi Oleksandr,

On 10/24/2014 11:38 AM, Oleksandr Dmytryshyn wrote:
> On Thu, Oct 23, 2014 at 7:03 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> On 10/23/2014 04:08 PM, Oleksandr Dmytryshyn wrote:
>>> diff --git a/drivers/cpufreq/xen-cpufreq.c b/drivers/cpufreq/xen-cpufreq.c
>>> new file mode 100644
>>> index 0000000..a7f0977
>>> --- /dev/null
>>> +++ b/drivers/cpufreq/xen-cpufreq.c
>>> @@ -0,0 +1,889 @@
>>> +/*
>>> + *  Copyright (C) 2001 Russell King
>>> + *            (C) 2002 - 2003 Dominik Brodowski <linux@brodo.de>
>>> + *
>>> + *  Oct 2005 - Ashok Raj <ashok.raj@intel.com>
>>> + *   Added handling for CPU hotplug
>>> + *  Feb 2006 - Jacob Shin <jacob.shin@amd.com>
>>> + *   Fix handling for CPU hotplug -- affected CPUs
>>> + *
>>> + *           (C) 2014 GlobalLogic Inc.
>>> + *
>>> + * Based on drivers/cpufreq/cpufreq.c
>>
>> It would be interesting to explain roughly what are the major changes
>> between drivers/cpufreq/cpufreq.c and this drivers.
> Here are some changes:
>  * I've introduced xen_nr_cpus which equals to real physical CPUs number
>    and xen-cpufreq.c uses it. Original driver cpufreq.c uses nr_cpu_ids -
>    virtual CPUs number.
>  * I've removed unused parts (sreating files in the sysfs, governors, etc)
>  * I've added xen-specific functions.
>  * If we have a separate file (xen-cpufreq.c) - it is possible to
>    use both drivers (cpufreq.c and xen-cpufreq.c) at the same time and
>    start compiled kernel under Xen and under bare metal without recompilation.

Can you add this in below "Base on ..."?

>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms and conditions of the GNU General Public License,
>>> + * version 2, as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>> + * more details.
>>> + */
>>> +
>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/notifier.h>
>>> +#include <linux/types.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/irq.h>
>>> +#include <linux/workqueue.h>
>>> +#include <linux/cpufreq.h>
>>> +
>>> +#include <trace/events/power.h>
>>> +
>>> +#include <xen/xen.h>
>>> +#include <xen/events.h>
>>> +#include <xen/interface/xen.h>
>>> +#include <xen/interface/platform.h>
>>> +#include <xen/interface/sysctl.h>
>>> +#include <asm/xen/hypercall.h>
>>> +
>>> +#include "cpufreq_drv_ops.h"
>>> +
>>> +#ifdef CONFIG_CPUMASK_OFFSTACK
>>> +#error CONFIG_CPUMASK_OFFSTACK config should not be used with this driver
>>> +#endif
> 
> 
>> I remembered we talk about this check on a previous version of this
>> patch. Can't we disable the option in Kconfig rather than throwing a
>> compilation error?
>>
>> Or maybe...
> I can just remove this checking from this file. But I'm afraid if this options
> will be turned on and CPUs bit mask will correspond to the virtual CPUs number
> instead of the NR_CPUS number. And in this case some functions (like
> cpumask_setall()) will work not correctly, because physical CPUs number can be
> greater then virtual CPUs number. May be Kconfig option CONFIG_XEN_CPUFREQ
> should be depend from !CONFIG_CPUMASK_OFFSTACK option? In this case
> this checking may be safely removed from this file.
> 
>>> +static int __init xen_cpufreq_init(void)
>>
>> [..]
> I'll fix this in the next patch set.
> 
>>> +     xen_nr_cpus = op.u.physinfo.nr_cpus;
>>> +     if (xen_nr_cpus == 0 || xen_nr_cpus > NR_CPUS) {
>>
>> Improving this check by also testing nr_cpumask_bits here.
> nr_cpumask_bits corresponds to the virtual CPUs number and it should be checked
> in the another place. This driver works only with physical CPUs.

AFAIU, nr_cpumask_bits is used to dynamically allocate the size of
cpumask_var_t.

It's possible to have a platform where the number of virtual CPUs is
equal to the number of physical CPUs (It's the default on Xen).

Anyway, it looks like this option can be only enabled when
DEBUG_PER_CPU_MAPS is set. The change in Kconfig looks like the best
solution for now.

Regards,

-- 
Julien Grall