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

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

On Thu, Oct 23, 2014 at 7:03 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Oleksandr,
>
> 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.

>> + *
>> + * 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.

> Regards,
>
> --
> Julien Grall

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