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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

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

Jan


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