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

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

On Wed, Oct 22, 2014 at 12:34 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 22.10.14 at 10:46, <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx> wrote:
>> 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.
>
> You just restate what was known already. What you fail to do is
> explain how this can end up being correct. To me this looks like a
> flawed design (acceptable for a PoC, but certainly not for
> something proposed for upstream inclusion).
I'll try to implement it in the better way but now it is difficult to do.

>>>> --- 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.
>
> I don't follow - you just don't need the #include above; no need to
> create a new header.
The structure 'xen_sysctl_cpufreq_op_t' could not be defined in the
original file cpufreq.h
because all sysctl-related structures are defined in the
xen/include/public/sysctl.h.
And if this structure will be defined in the cpufreq.h than this file
should be included to the sysctl.h (and it is not an acceptable solution).

> Jan
>

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