Messages in this thread | | | From | Patrick Bellasi <> | Subject | Re: [PATCH v3 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET flag to reset uclamp | Date | Wed, 28 Oct 2020 19:03:35 +0100 |
| |
On Wed, Oct 28, 2020 at 12:39:43 +0100, Qais Yousef <qais.yousef@arm.com> wrote...
> On 10/28/20 11:11, Patrick Bellasi wrote: >> >> >> >> /* >> >> * RT by default have a 100% boost value that could be modified >> >> * at runtime. >> >> */ >> >> if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN)) >> >> - __uclamp_update_util_min_rt_default(p); >> >> + value = sysctl_sched_uclamp_util_min_rt_default; >> >> By removing this usage of __uclamp_updadate_util_min_rt_default(p), >> the only other usage remaining is the call from: >> uclamp_udpate_util_min_rt_default(). >> >> What about an additional cleanup by in-lining the only surviving usage? > > This is not a cleanup IMO. There is special rule about updating that are > encoded and documented in this helper function. Namely: > > * p->pi_lock must be held. > * p->uclamp_req[].user_defined must be false.
Both these conditions are satisfied in the above call site: - user_defined is tested just 4 lines above - pi_lock is taken by the caller, i.e. __sched_setscheduler() Thus, there is no need to test them two times.
Moreover, the same granted pi_lock you check in __ucalmp_update_util_min_rt_default() is not checked at all in the rest of __setscheduler_uclamp().
Thus, perhaps we should have just avoided to add __uclamp_update_util_min_rt_default() since the beginning and: - have all its logic in the _only_ place where it's required - added the lockdep_assert_held() in __setscheduler_uclamp()
That's why I consider this a very good cleanup opportunity.
> I don't see open coding helps but rather makes the code harder to read and > prone to introduce bugs if anything gets reshuffled in the future.
It's not open coding IMHO, it's just adding the code that's required.
| |