linux-kernel - Re: [PATCH v2 4/4] sched: Fix cgroup irq accounting for CONFIG_IRQ_TIME

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CALOAHbCg08ARz4-8=4+28gb+DnHxQFdxiJTLSoD6xAoea5E_FQ@mail.gmail.com>
Date: Wed, 9 Oct 2024 19:40:13 +0800
From: Yafang Shao <laoar.shao@...il.com>
To: Johannes Weiner <hannes@...xchg.org>
Cc: mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com, 
	vincent.guittot@...aro.org, dietmar.eggemann@....com, rostedt@...dmis.org, 
	bsegall@...gle.com, mgorman@...e.de, vschneid@...hat.com, surenb@...gle.com, 
	cgroups@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] sched: Fix cgroup irq accounting for CONFIG_IRQ_TIME_ACCOUNTING

On Tue, Oct 8, 2024 at 10:23 PM Johannes Weiner <hannes@...xchg.org> wrote:
>
> On Tue, Oct 08, 2024 at 02:19:51PM +0800, Yafang Shao wrote:
> > @@ -5587,7 +5587,24 @@ void sched_tick(void)
> >       rq_lock(rq, &rf);
> >
> >       curr = rq->curr;
> > -     psi_account_irqtime(rq, curr, NULL);
> > +
> > +#ifdef CONFIG_IRQ_TIME_ACCOUNTING
> > +     if (static_branch_likely(&sched_clock_irqtime)) {
> > +             u64 now, irq;
> > +             s64 delta;
> > +
> > +             now = cpu_clock(cpu);
> > +             irq = irq_time_read(cpu);
> > +             delta = (s64)(irq - rq->psi_irq_time);
> > +             if (delta > 0) {
> > +                     rq->psi_irq_time = irq;
> > +                     psi_account_irqtime(rq, curr, NULL, now, delta);
> > +                     cgroup_account_cputime(curr, delta);
> > +                     /* We account both softirq and irq into softirq */
> > +                     cgroup_account_cputime_field(curr, CPUTIME_SOFTIRQ, delta);
> > +             }
> > +     }
> > +#endif
> >
> >       update_rq_clock(rq);
> >       hw_pressure = arch_scale_hw_pressure(cpu_of(rq));
> > @@ -6667,7 +6684,25 @@ static void __sched notrace __schedule(int sched_mode)
> >               ++*switch_count;
> >
> >               migrate_disable_switch(rq, prev);
> > -             psi_account_irqtime(rq, prev, next);
> > +
> > +#ifdef CONFIG_IRQ_TIME_ACCOUNTING
> > +             if (static_branch_likely(&sched_clock_irqtime)) {
> > +                     u64 now, irq;
> > +                     s64 delta;
> > +
> > +                     now = cpu_clock(cpu);
> > +                     irq = irq_time_read(cpu);
> > +                     delta = (s64)(irq - rq->psi_irq_time);
> > +                     if (delta > 0) {
> > +                             rq->psi_irq_time = irq;
> > +                             psi_account_irqtime(rq, prev, next, now, delta);
> > +                             cgroup_account_cputime(prev, delta);
> > +                             /* We account both softirq and irq into softirq */
> > +                             cgroup_account_cputime_field(prev, CPUTIME_SOFTIRQ, delta);
> > +                     }
> > +             }
> > +#endif
>
> This should be inside its own function - to avoid duplication of
> course, but also the ifdefs and overly detailed accounting code in the
> middle of core scheduling logic.
>
> #ifdef CONFIG_IRQ_TIME_ACCOUNTING
> static void account_irqtime(struct rq *rq, ...)
> {
>         ...
> }
> #else
> static inline void account_irqtime(...) {}
> #endif

Good suggestion. Will do it in the next version.

-- 
Regards
Yafang