On 19-Mar 17:57, Daniel Lezcano wrote:
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.
The mainlining path is already quite different then this code. Hopefully you will like it better once is posted.
Moreover, the stack size in the kernel is limited to 16KB. This bloated structure will one day kill the stack.
And that's why I'm totally ok with one of the following patches where Leo move this struct into a per-cpu variable...
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 ?
It could, in case you need to read the same information somewhere else... which however you right is not the case of the current code.
Using next_idx as a return value is also another design decision, since it makes the core more streamline to read.
Really ?
Yes, considering other patches which are not currently merged, exactly like this one. However, I agree that this cached value should/could be added once the other usages are added.
It's likely like that now because when we forward ported patches from v3.18 we skipped the patches with the usages, with the idea to reduce the overall code footprint. Nevertheless, when you are required to do these forward/backward porting, sometimes it makes things simpler to keep patches almost similar.
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.
At the current status of the code I would say yes, however, again I'm not against refactoring/cleanups... there are many to do... we should just avoid creating many different code favours.
I can also agree that this patch makes sense... but I would like better to see this patch aligned with the most updated cleanup we already have.
-- #include <best/regards.h>
Patrick Bellasi