From: Patrick Bellasi <patrick.bellasi@matbug.net>
To: Qais Yousef <qais.yousef@arm.com>
Cc: Yun Hsiang <hsiang023167@gmail.com>,
Dietmar Eggemann <dietmar.eggemann@arm.com>,
peterz@infradead.org, linux-kernel@vger.kernel.org
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 [thread overview]
Message-ID: <87sg9ymdmw.derkling@matbug.net> (raw)
In-Reply-To: <20201028113943.7jzxbytizrv7wsm3@e107158-lin>
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.
next prev parent reply other threads:[~2020-10-28 21:54 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-25 7:36 [PATCH v3 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET flag to reset uclamp Yun Hsiang
2020-10-26 9:47 ` Dietmar Eggemann
2020-10-26 15:45 ` Yun Hsiang
2020-10-26 19:00 ` Dietmar Eggemann
2020-10-27 15:58 ` Yun Hsiang
2020-10-28 10:11 ` Patrick Bellasi
2020-10-28 11:39 ` Qais Yousef
2020-10-28 18:03 ` Patrick Bellasi [this message]
2020-10-28 18:29 ` Qais Yousef
2020-10-28 18:41 ` Yun Hsiang
2020-10-29 12:37 ` Dietmar Eggemann
2020-10-29 11:08 ` Qais Yousef
2020-10-29 13:02 ` Yun Hsiang
2020-10-29 13:06 ` Qais Yousef
2020-10-29 15:50 ` Dietmar Eggemann
2020-10-29 17:17 ` Qais Yousef
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87sg9ymdmw.derkling@matbug.net \
--to=patrick.bellasi@matbug.net \
--cc=dietmar.eggemann@arm.com \
--cc=hsiang023167@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=qais.yousef@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).