Messages in this thread | ![/](/images/icornerl.gif) | | From | Vincent Guittot <> | Date | Mon, 8 Apr 2024 09:14:46 +0200 | Subject | Re: [PATCH] sched/pi: Reweight fair_policy() tasks when inheriting prio |
| |
On Fri, 5 Apr 2024 at 19:16, Qais Yousef <qyousef@layalina.io> wrote: > > On 04/05/24 14:15, Vincent Guittot wrote: > > On Fri, 5 Apr 2024 at 00:05, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > On 04/03/24 15:11, Vincent Guittot wrote: > > > > On Wed, 3 Apr 2024 at 02:59, Qais Yousef <qyousef@layalina.io> wrote: > > > > > > > > > > For fair tasks inheriting the priority (nice) without reweighting is > > > > > a NOP as the task's share won't change. > > > > > > > > AFAICT, there is no nice priority inheritance with rt_mutex; All nice > > > > > > Hmm from what I see there is > > > > > > > tasks are sorted with the same "default prio" in the rb waiter tree. > > > > This means that the rt top waiter is not the cfs with highest prio but > > > > the 1st cfs waiting for the mutex. > > > > > > This is about the order on which tasks contending for the lock more than the > > > effective priority the task holding the lock should run at though, no? > > > > No, they are ordered by priority in the rb tree so you can get the > > priority of the top waiter and apply it to the owner of the lock > > I think I see what you're getting at now. There's no guarantee the top waiter > is the higher priority fair task. Yes. > > > > > > > > > > > > > > > > > > > > This is visible when running with PTHREAD_PRIO_INHERIT where fair tasks > > > > > with low priority values are susceptible to starvation leading to PI > > > > > like impact on lock contention. > > > > > > > > > > The logic in rt_mutex will reset these low priority fair tasks into nice > > > > > 0, but without the additional reweight operation to actually update the > > > > > weights, it doesn't have the desired impact of boosting them to allow > > > > > them to run sooner/longer to release the lock. > > > > > > > > > > Apply the reweight for fair_policy() tasks to achieve the desired boost > > > > > for those low nice values tasks. Note that boost here means resetting > > > > > their nice to 0; as this is what the current logic does for fair tasks. > > > > > > > > But you can at the opposite decrease the cfs prio of a task > > > > and even worse with the comment : > > > > /* XXX used to be waiter->prio, not waiter->task->prio */ > > > > > > > > we use the prio of the top cfs waiter (ie the one waiting for the > > > > lock) not the default 0 so it can be anything in the range [-20:19] > > > > > > > > Then, a task with low prio (i.e. nice > 0) can get a prio boost even > > > > if this task and the waiter are low priority tasks > > > > > > I don't see this effect. The only change I am doing here > > > is that when we set the prio that we are supposed to be inheriting, instead of > > > simply changing prio, I also ensure we reweight so that we run at the inherited > > > nice value. I am not changing how the waiter logic works. > > > > But if you look more deeply in the code, you will see that all cfs are > > sorted with the same default prio so cfs tasks are not sorted and are > > considered to be the same. > > Yes I saw that. We can potentially revert 715f7f9ece46 ("locking/rtmutex: > Squash !RT tasks to DEFAULT_PRIO") ;-) > > /hides > > > > > All that to say that I think the weight is not applied on purpose. > > This might work for your particular case but there are more changes to > > be done if you want to apply prio inheritance between cfs tasks. > > > > As an example, what about the impact of cgroup on the actual weight > > and the inherited priority of a task ? If the owner and the waiter > > don't belong to the same cgroup their own prio is meaningless... task > > nice -20 in a group with a weight equal to nice 19 vs a task nice 19 > > in a group with a weight equals to nice -20 > > That is on my mind actually. But I thought it's a separate problem. That has to > do with how we calculate the effective priority of the pi_task. And probably > the sorting order to if we agree we need to revert the above. If that is done > appropriately, I hope the current reweight approach could be used as-is. Hmm > but but as I write this I realize the compound weight will still be different. > Maybe we should inherit the weight rather than the prio, which IIUC should > already be the effective weight taking cgroup into account? > > Just to put it out on the clear, you don't think the issue is wrong, but just > that we need to look further for a proper fix, right? ie: it is a problem we > should fix, but we need to nail down more details IIUC.
Yes, I agree about your problem but your current proposal is not correct because there are more things to consider and fix
> > If that's the case it'd be good to know what else beside sorting order and > handling cgroup I need to take into account to make this more correct. > > There's the obvious SCHED_IDLE case of course that needs a policy promotion, > beside weight adjustment. > > > > > > > > > > > Here's my test app FWIW > > > > > > https://github.com/qais-yousef/pi_test > > > > > > When I run > > > > > > pi_test --lp-nice 0 --lp-nice 10 > > > > > > the lp thread runs at 0 still > > > > > > If I do > > > > > > pi_test --lp-nice 10 --lp-nice 5 > > > > > > low priority thread runs at 5 > > > > > > What combination are you worried about? I can give it a try. I use > > > sched-analyzer-pp [1] to see the division of runnable/running or you can > > > monitor them on top > > > > > > #!/bin/bash > > > set -eux > > > > > > sudo sched-analyzer & > > > > > > ./pi_test --lp-nice ${1:-10} --hp-nice ${2:-0} --affine-cpu ${3:-0} & > > > > > > sleep 10 > > > > > > pkill -SIGKILL pi_test > > > > > > sudo pkill -SIGINT sched-analyzer > > > > > > sched-analyzer-pp --sched-states pi_test sched-analyzer.perfetto-trace > > > > > > Picutres of output is attached for before and after > > > > > > pi_test --lp-nice 10 --hp-nice 0 > > > > > > [1] https://github.com/qais-yousef/sched-analyzer
| ![\](/images/icornerr.gif) |