Re: [Resend PATCH] psi : calc cfs task memstall time more precisely

[Zhaoyang Huang]

On Thu, Nov 4, 2021 at 4:58 PM Dietmar Eggemann
<dietmar.eggemann@xxxxxxx> wrote:
>
> On 03/11/2021 08:08, Zhaoyang Huang wrote:
> > +Vincent Guittot
> >
> > On Wed, Nov 3, 2021 at 3:07 PM Zhaoyang Huang <huangzhaoyang@xxxxxxxxx> wrote:
> >>
> >> On Wed, Nov 3, 2021 at 3:47 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> >>>
> >>> CC peterz as well for rt and timekeeping magic
> >>>
> >>> On Fri, Oct 15, 2021 at 02:16:52PM +0800, Huangzhaoyang wrote:
> >>>> From: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
> >>>>
> >>>> In an EAS enabled system, there are two scenarios discordant to current design,
>
> I don't understand the EAS (probably asymmetric CPU capacity is meant
> here) angle of the story. Pressure on CPU capacity which is usable for
> CFS happens on SMP as well?
 Mentioning EAS here mainly about RT tasks preempting small CFS tasks
(big CFS tasks could be scheduled to big core), which would introduce
more proportion of preempted time within PSI_MEM_STALL than SMP does.
>
> >>>>
> >>>> 1. workload used to be heavy uneven among cores for sake of scheduler policy.
> >>>> RT task usually preempts CFS task in little core.
> >>>> 2. CFS task's memstall time is counted as simple as exit - entry so far, which
> >>>> ignore the preempted time by RT, DL and Irqs.
> >>>>
> >>>> With these two constraints, the percpu nonidle time would be mainly consumed by
> >>>> none CFS tasks and couldn't be averaged. Eliminating them by calc the time growth
> >>>> via the proportion of cfs_rq's utilization on the whole rq.
> >>>>
> >>>> eg.
> >>>> Here is the scenario which this commit want to fix, that is the rt and irq consume
> >>>> some utilization of the whole rq. This scenario could be typical in a core
> >>>> which is assigned to deal with all irqs. Furthermore, the rt task used to run on
> >>>> little core under EAS.
> >>>>
> >>>> Binder:305_3-314    [002] d..1   257.880195: psi_memtime_fixup: original:30616,adjusted:25951,se:89,cfs:353,rt:139,dl:0,irq:18
> >>>> droid.phone-1525    [001] d..1   265.145492: psi_memtime_fixup: original:61616,adjusted:53492,se:55,cfs:225,rt:121,dl:0,irq:15
> >>>>
> >>>> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
> >>>> ---
> >>>>  kernel/sched/psi.c | 20 +++++++++++++++++++-
> >>>>  1 file changed, 19 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> >>>> index cc25a3c..754a836 100644
> >>>> --- a/kernel/sched/psi.c
> >>>> +++ b/kernel/sched/psi.c
> >>>> @@ -182,6 +182,8 @@ struct psi_group psi_system = {
> >>>>
> >>>>  static void psi_avgs_work(struct work_struct *work);
> >>>>
> >>>> +static unsigned long psi_memtime_fixup(u32 growth);
> >>>> +
> >>>>  static void group_init(struct psi_group *group)
> >>>>  {
> >>>>       int cpu;
> >>>> @@ -492,6 +494,21 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
> >>>>       return growth;
> >>>>  }
> >>>>
> >>>> +static unsigned long psi_memtime_fixup(u32 growth)
> >>>> +{
> >>>> +     struct rq *rq = task_rq(current);
> >>>> +     unsigned long growth_fixed = (unsigned long)growth;
> >>>> +
> >>>> +     if (!(current->policy == SCHED_NORMAL || current->policy == SCHED_BATCH))
> >>>> +             return growth_fixed;
>
> This will let the idle task (swapper) pass. Is this indented? Or do you
> want to only let CFS tasks (including SCHED_IDLE) pass?
idle tasks will NOT call psi_memstall_xxx. We just want CFS tasks to
scale the STALL time.
>
> if (current->sched_class != &fair_sched_class)
>     return growth_fixed;
>
> >>>> +
> >>>> +     if (current->in_memstall)
> >>>> +             growth_fixed = div64_ul((1024 - rq->avg_rt.util_avg - rq->avg_dl.util_avg
> >>>> +                                     - rq->avg_irq.util_avg + 1) * growth, 1024);
> >>>> +
>
> We do this slightly different in scale_rt_capacity() [fair.c]:
>
> max = arch_scale_cpu_capacity(cpu_of(rq) /* instead of 1024 to support
>                                             asymmetric CPU capacity */
Is it possible that the SUM of rqs' util_avg large than
arch_scale_cpu_capacity because of task migration things?
>
> used = cpu_util_rt(rq);
> used += cpu_util_dl(rq);
> used += thermal_load_avg(rq);
>
> free = max - used
> irq = cpu_util_irq(rq)
>
> used = scale_irq_capacity(free, irq, max);
>
> scaling then with with: max - used / max
ok. so introduce thermal util.
>
> >>>> +     return growth_fixed;
> >>>> +}
> >>>> +
> >>>>  static void init_triggers(struct psi_group *group, u64 now)
> >>>>  {
> >>>>       struct psi_trigger *t;
> >>>> @@ -658,6 +675,7 @@ static void record_times(struct psi_group_cpu *groupc, u64 now)
> >>>>       }
> >>>>
> >>>>       if (groupc->state_mask & (1 << PSI_MEM_SOME)) {
> >>>> +             delta = psi_memtime_fixup(delta);
> >>>
> >> add vincent for advise on cpu load mechanism
> >>
> >>> Ok, so we want to deduct IRQ and RT preemption time from the memstall
> >>> period of an active reclaimer, since it's technically not stalled on
> >>> memory during this time but on CPU.
> >>>
> >>> However, we do NOT want to deduct IRQ and RT time from memstalls that
> >>> are sleeping on refaults swapins, since they are not affected by what
> >>> is going on on the CPU.
> >>>
> >>> Does util_avg capture that difference? I'm not confident it does - but
> >>> correct me if I'm wrong. We need length of time during which and IRQ
> >>> or an RT task preempted the old rq->curr, not absolute irq/rt length.
> >> As far as my understanding, core's capacity = IRQ + DEADLINE + RT +
> >> CFS. For a certain time period, all cfs tasks preempt each other
> >> inside CFS's utilization. So the sleeping on refaults is counted in.
> >>>
> >>> (Btw, such preemption periods, in addition to being deducted from
> >>> memory stalls, should probably also be added to CPU contention stalls,
> >>> to make CPU pressure reporting more accurate as well.)
>