Hi Patrick On 06/30/20 16:55, Patrick Bellasi wrote: > > Hi Qais, > sorry for commenting on v5 with a v6 already posted, but... > ... I cannot keep up with your re-spinning rate ;)
I classified that as a nit really and doesn't affect correctness. We have different subjective view on what is better here. I did all the work in the past 2 weeks and I think as the author of this patch I have the right to keep my preference on subjective matters. I did consider your feedback and didn't ignore it and improved the naming and added a comment to make sure there's no confusion. We could nitpick the best name forever, but is it really that important? I really don't see any added value for one approach or another here to start a long debate about it. The comments were small enough that I didn't see any controversy that warrants holding the patches longer. I agreed with your proposal to use uc_se->active and clarified why your other suggestions don't hold. You pointed that uclamp_is_enabled() confused you; and I responded that I'll change the name. Sorry for not being explicit about answering the below, but I thought my answer implied that I don't prefer it. > > >> Thus, perhaps we can just use the same pattern used by the > >> sched_numa_balancing static key: > >> > >> $ git grep sched_numa_balancing > >> kernel/sched/core.c:DEFINE_STATIC_KEY_FALSE(sched_numa_balancing); > >> kernel/sched/core.c: > >> static_branch_enable(&sched_numa_balancing); > >> kernel/sched/core.c: > >> static_branch_disable(&sched_numa_balancing); > >> kernel/sched/core.c: int state = > >> static_branch_likely(&sched_numa_balancing); > >> kernel/sched/fair.c: if (!static_branch_likely(&sched_numa_balancing)) > >> kernel/sched/fair.c: if (!static_branch_likely(&sched_numa_balancing)) > >> kernel/sched/fair.c: if (!static_branch_likely(&sched_numa_balancing)) > >> kernel/sched/fair.c: if > >> (static_branch_unlikely(&sched_numa_balancing)) > >> kernel/sched/sched.h:extern struct static_key_false sched_numa_balancing; > >> > >> IOW: unconditionally define sched_uclamp_used as non static in core.c, > >> and use it directly on schedutil too. > > So, what about this instead of adding the (renamed) method above? I am sorry there's no written rule that says one should do it in a specific way. And AFAIK both way are implemented in the kernel. I appreciate your suggestion but as the person who did all the hard work, I think my preference matters here too. And actually with my approach when uclamp is not compiled in there's no need to define an extra variable; and since uclamp_is_used() is defined as false for !CONFIG_UCLAMP_TASK, it'll help with DCE, so less likely to end up with dead code that'll never run in the final binary. Thanks a lot for all of your comments and feedback anyway! -- Qais Yousef
