Hi Patrick,
On Mon, Mar 19, 2018 at 12:33:01PM +0000, Patrick Bellasi wrote:
On 19-Mar 07:07, Daniel Lezcano wrote:
On Sat, Mar 17, 2018 at 08:05:15PM +0800, Leo Yan wrote:
select_energy_cpu_idx() returns the target CPU index, and eenv->next_idx is used to define target CPU index as well; it's pointless to use two methods to return exactly same value.
This patch is to change the definition for return value of select_energy_cpu_idx(), errors return negative value and success returns 0. And later use eenv->next_idx for target CPU index.
Yes there is a duplication of the information. Actually, the change could be the contrary: make select_energy_cpu_idx return the cpu_idx and kill the next_idx field in the structure.
next_idx field is used only in the calling function, it could be stored in a local variable.
Unfortunately it's not merged (yet) in eas-dev, but there is an energy_diff tracepoint which gets eenv only data.
There is no real usage of this transient value for a field in a structure.
That's the reason why we store next_idx in the eenv, moreover it makes the code more self-contained from a functional standpoint since everywhere you know that the eenv contains all the information which defined a certain energy aware wakeup decision.
I disagree with the "self-contained approach is a good thing": that keeps the entire code out of the upstreaming path.
Moreover, the stack size in the kernel is limited to 16KB. This bloated structure will one day kill the stack.
Do you really think storing a value in a structure passed in parameter and returning it from the same function and then using it in one place does really make sense ?
Using next_idx as a return value is also another design decision, since it makes the core more streamline to read.
Really ?
Thus, overall I don't see a big point on this patch if you consider the above two "design decisions".
So the eenv is passed to the function, this one fills eenv->next_idx and returns next_idx and the eenv->next_idx is used by the caller only on this place only. That is not a design decision, it is an implementation and it makes sense to do this cleanup.