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

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

On Fri, Oct 24, 2014 at 2:29 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx> 
>> 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@xxxxxxxx>
>>>> + *
>>>> + *  Oct 2005 - Ashok Raj <ashok.raj@xxxxxxxxx>
>>>> + *   Added handling for CPU hotplug
>>>> + *  Feb 2006 - Jacob Shin <jacob.shin@xxxxxxx>
>>>> + *   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 ..."?
I'll do this in the next patch-set.

>>>> + *
>>>> + * 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.
I'll do this in the next patch-set.

> Regards,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel