Re: [PATCH] cpufreq: Add scaling frequency range support

[Rafael J. Wysocki]

On Wednesday, July 29, 2015 05:59:14 PM Pan Xinhui wrote:
> hi, Rafael
> 	thanks for you reply.
> 
> On 2015å07æ29æ 08:18, Rafael J. Wysocki wrote:
> > On Tuesday, July 28, 2015 12:53:33 PM Pan Xinhui wrote:
> >> hi, Viresh
> >> 	thanks for your reply :)
> >> On 2015å07æ28æ 12:29, Viresh Kumar wrote:
> >>> On 28-07-15, 11:32, Pan Xinhui wrote:
> >>>> From: Pan Xinhui <xinhuix.pan@xxxxxxxxx>
> >>>>
> >>>> Userspace at most time do cpufreq tests very much inconveniently.
> >>>> Currently they have to echo min and max cpu freq separately like below:
> >>>> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> >>>> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> >>>>
> >>>> Add scaling_freq_range cpufreq attr to support userspace's demand.
> >>>> Therefore it's easier for testers to write readable scripts like below: 
> >>>> echo 480000-2240000 >
> >>>> /sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range
> >>>
> >>> I don't think this brings any good change, we already have support for
> >>> that with min/max freqs and I don't see how scripts can be less
> >>> readable with that.
> >>>
> >> yes, min/max are supported, however it is inconvenient. sometime it's very easy to cause obscure bugs.
> >> For example, some one might write a script like below.
> >> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> >> echo 960000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> >> .....//other works
> >> echo 1120000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> >> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> >> ...//other works
> >>
> >> But it did not work when we echo 112000 to min-freq, as the current max freq is smaller than it.
> >> It's hard to figure it out in a big script... we have many such scripts.
> > 
> > Fix them, then, pretty please.
> > 
> of course we will fix them. :)
> 
> > And adding this attribute is not going to magically fix them, is it?
> > 
> yes, this patch can not fix them without changing the script. BUT I have another patch which could magically fix them. :)
> 
> These two attribute files are very tricky. they are related with each other.
> Not like some other attribute file in other part of kernel, for example, proc/sys/fs/file-max.
> As the file-min is always zero. It's very reasonable to only support file-max attribute file.
> 
> The sequence we echoing value to min/max_freq is very important. Maybe we can also assume they have *state*.
> Just like a developer writes a buf to a file. he should do in this way below.
> fp = fopen(..)
>  => fwrite(...)
>   => fclose(...)
> 
> The script I mentioned above did not follow the right sequence. when script wants to set the min higher, we need set the max first to avoid min > max issue...
> So max/min_freq have *state*. just like TCP Three-way handshake, SYN, ACK&SYN, ACK. the sequence(this is so-called state) is very important.

No, this isn't like that.  The rule is simple: whatever is in one of the
attributes needs to be a smaller value than the one from the other attribute
at any time.  So there is a correlation between them, but the only "state" is
those numbers written to them previously (which they preserve quite as expected).

And the algorithm is: look for what's in min and write a number which is not
less then that to max.  And the other way around.

Again, please fix your scripts and don't litter the kernel with stuff which
only is needed because user space developers can't get their act together.

Case dismissed.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/