Re: [Xen-devel] [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

On Fri, Oct 17, 2014 at 4:21 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx> wrote:
>> +int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op)
>> +{
>> +    int ret = 0;
>> +    switch ( op->cmd )
>> +    {
>> +    case XEN_SYSCTL_CPUFREQ_get_target:
>> +        spin_lock(&sysctl_cpufreq_lock);
>> +        op->u.target.cpu = sysctl_cpufreq_data.cpu;
>> +        op->u.target.freq = sysctl_cpufreq_data.freq;
>> +        op->u.target.relation = sysctl_cpufreq_data.relation;
>> +        spin_unlock(&sysctl_cpufreq_lock);
>> +        break;
>> +
>> +    case XEN_SYSCTL_CPUFREQ_set_result:
>> +        spin_lock(&sysctl_cpufreq_lock);
>> +        sysctl_cpufreq_data.result = op->u.result;
>> +        spin_unlock(&sysctl_cpufreq_lock);
>> +        break;
>
> Seeing this and taking it together with the previous patch I still
> can't really see how all this is intended to fit together, and
> where the frequency change really takes place. I'm particularly
> lacking understanding of how the result of the operation is
> intended to be made available for the right .target handler
> invocation - right now it looks like the next invocation will get
> to see the result of the previous one.
Some requests to change frequency can be issued from the hypercall.
All hypercalls are locked by spinlocks. Thus the frequency changing
should be done in the atomic contex. But we can not send command to the
hwdom and wait answer in the atomic context. So we may use the result
of the previous command and send current command.

>> +    default:
>> +        return -ENOSYS;
>
> -EINVAL or -EOPNOTSUPP please.
I'll fix this in the next patch-set

>> --- a/xen/include/xen/cpufreq.h
>> +++ b/xen/include/xen/cpufreq.h
>> @@ -20,6 +20,7 @@
>>  #include <xen/spinlock.h>
>>  #include <xen/errno.h>
>>  #include <xen/cpumask.h>
>> +#include <public/sysctl.h>
>
> Please don't add new dependencies that aren't needed (not even
> on ARM).
I'll create a separate file hwdom-cpufreq.h and I'll use it.

>> @@ -264,4 +265,8 @@ int write_userspace_scaling_setspeed(unsigned int cpu, 
>> unsigned int freq);
>>  void cpufreq_dbs_timer_suspend(void);
>>  void cpufreq_dbs_timer_resume(void);
>>
>> +#ifdef HAS_DOM0_CPUFREQ
>> +int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op);
>> +#endif
>
> Simply forward declare and use struct xen_sysctl_cpufreq_op here.
>
> And framing a declaration in #ifdef is relatively pointless too.
I'll create a separate file hwdom-cpufreq.h and I'll use it (as I've
written above).

> Jan
>

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