On 10/28/20 19:03, Patrick Bellasi wrote: > > On Wed, Oct 28, 2020 at 12:39:43 +0100, Qais Yousef <[email protected]> > 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().
Updating the default rt value is done from different contexts. Hence it is important to document the rules under which this update must happen and ensure the update happens through a common path. __setscheduler_uclamp() is not called from 2 different contexts. > 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 disagree. This is unnecessary churn. Thanks -- Qais Yousef > > 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.
