On 05/21/24 13:00, Sebastian Andrzej Siewior wrote: > On 2024-05-15 23:05:36 [+0100], Qais Yousef wrote: > > rt_task() checks if a task has RT priority. But depends on your > > dictionary, this could mean it belongs to RT class, or is a 'realtime' > > task, which includes RT and DL classes. > > > > Since this has caused some confusion already on discussion [1], it > > seemed a clean up is due. > > > > I define the usage of rt_task() to be tasks that belong to RT class. > > Make sure that it returns true only for RT class and audit the users and > > replace the ones required the old behavior with the new realtime_task() > > which returns true for RT and DL classes. Introduce similar > > realtime_prio() to create similar distinction to rt_prio() and update > > the users that required the old behavior to use the new function. > > > > Move MAX_DL_PRIO to prio.h so it can be used in the new definitions. > > > > Document the functions to make it more obvious what is the difference > > between them. PI-boosted tasks is a factor that must be taken into > > account when choosing which function to use. > > > > Rename task_is_realtime() to realtime_task_policy() as the old name is > > confusing against the new realtime_task(). > > I *think* everyone using rt_task() means to include DL tasks. And > everyone means !SCHED-people since they know when the difference matters. yes, this makes sense > > > No functional changes were intended. > > > > [1] https://lore.kernel.org/lkml/20240506100509.GL40213@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > > > Reviewed-by: Phil Auld <pauld@xxxxxxxxxx> > > Signed-off-by: Qais Yousef <qyousef@xxxxxxxxxxx> > > --- > > > > Changes since v1: > > > > * Use realtime_task_policy() instead task_has_realtime_policy() (Peter) > > * Improve commit message readability about replace some rt_task() > > users. > > > > v1 discussion: https://lore.kernel.org/lkml/20240514234112.792989-1-qyousef@xxxxxxxxxxx/ > > > > fs/select.c | 2 +- > > fs/bcachefs/six.c > six_owner_running() has rt_task(). But imho should have realtime_task() > to consider DL. But I think it is way worse that it has its own locking > rather than using what everyone else but then again it wouldn't be the > new hot thing… I think I missed this one. Converted now. Thanks! > > > include/linux/ioprio.h | 2 +- > > include/linux/sched/deadline.h | 6 ++++-- > > include/linux/sched/prio.h | 1 + > > include/linux/sched/rt.h | 27 ++++++++++++++++++++++++++- > > kernel/locking/rtmutex.c | 4 ++-- > > kernel/locking/rwsem.c | 4 ++-- > > kernel/locking/ww_mutex.h | 2 +- > > kernel/sched/core.c | 6 +++--- > > kernel/time/hrtimer.c | 6 +++--- > > kernel/trace/trace_sched_wakeup.c | 2 +- > > mm/page-writeback.c | 4 ++-- > > mm/page_alloc.c | 2 +- > > 13 files changed, 48 insertions(+), 20 deletions(-) > … > > diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c > > index 70625dff62ce..08b95e0a41ab 100644 > > --- a/kernel/time/hrtimer.c > > +++ b/kernel/time/hrtimer.c > > @@ -1996,7 +1996,7 @@ static void __hrtimer_init_sleeper(struct hrtimer_sleeper *sl, > > * expiry. > > */ > > if (IS_ENABLED(CONFIG_PREEMPT_RT)) { > > - if (task_is_realtime(current) && !(mode & HRTIMER_MODE_SOFT)) > > + if (realtime_task_policy(current) && !(mode & HRTIMER_MODE_SOFT)) > > mode |= HRTIMER_MODE_HARD; > > } > > > > @@ -2096,7 +2096,7 @@ long hrtimer_nanosleep(ktime_t rqtp, const enum hrtimer_mode mode, > > u64 slack; > > > > slack = current->timer_slack_ns; > > - if (rt_task(current)) > > + if (realtime_task(current)) > > slack = 0; > > > > hrtimer_init_sleeper_on_stack(&t, clockid, mode); > > @@ -2301,7 +2301,7 @@ schedule_hrtimeout_range_clock(ktime_t *expires, u64 delta, > > * Override any slack passed by the user if under > > * rt contraints. > > */ > > - if (rt_task(current)) > > + if (realtime_task(current)) > > delta = 0; > > I know this is just converting what is already here but… > __hrtimer_init_sleeper() looks at the policy to figure out if the task > is realtime do decide if should expire in HARD-IRQ context. This is > correct, a boosted task should not sleep. > > hrtimer_nanosleep() + schedule_hrtimeout_range_clock() is looking at > priority to decide if slack should be removed. This should also look at > policy since a boosted task shouldn't sleep. I have to admit I never dug deep enough into this code. Happy to convert these users. I'll add that as a separate patch as this is somewhat changing behavior which this patch intends to do a clean up only. > > In order to be PI-boosted you need to acquire a lock and the only lock > you can sleep while acquired without generating a warning is a mutex_t > (or equivalent sleeping lock) on PREEMPT_RT. Note we care about the behavior for !PREEMPT_RT. PI issues are important there too. I assume the fact the PREEMPT_RT changes the locks behavior is what you're referring to here and not applicable to normal case. Thanks! -- Qais Yousef > > > hrtimer_init_sleeper_on_stack(&t, clock_id, mode); > > diff --git a/kernel/trace/trace_sched_wakeup.c b/kernel/trace/trace_sched_wakeup.c > > index 0469a04a355f..19d737742e29 100644 > > --- a/kernel/trace/trace_sched_wakeup.c > > +++ b/kernel/trace/trace_sched_wakeup.c > > @@ -545,7 +545,7 @@ probe_wakeup(void *ignore, struct task_struct *p) > > * - wakeup_dl handles tasks belonging to sched_dl class only. > > */ > > if (tracing_dl || (wakeup_dl && !dl_task(p)) || > > - (wakeup_rt && !dl_task(p) && !rt_task(p)) || > > + (wakeup_rt && !realtime_task(p)) || > > (!dl_task(p) && (p->prio >= wakeup_prio || p->prio >= current->prio))) > > return; > > > > Sebastian
