linux-kernel - Re: [PATCH v2] sched/core: add forced idle accounting for cgroups

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 23 May 2022 14:19:36 -0700
From:   Josh Don <joshdon@...gle.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Tejun Heo <tj@...nel.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Valentin Schneider <vschneid@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Cruz Zhao <CruzZhao@...ux.alibaba.com>
Subject: Re: [PATCH v2] sched/core: add forced idle accounting for cgroups

On Sat, May 21, 2022 at 3:00 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Fri, May 20, 2022 at 04:51:38PM -0700, Josh Don wrote:
>
> > diff --git a/kernel/sched/core_sched.c b/kernel/sched/core_sched.c
> > index 38a2cec21014..ddef2b8ddf68 100644
> > --- a/kernel/sched/core_sched.c
> > +++ b/kernel/sched/core_sched.c
> > @@ -277,7 +277,11 @@ void __sched_core_account_forceidle(struct rq *rq)
> >               if (p == rq_i->idle)
> >                       continue;
> >
> > -             __schedstat_add(p->stats.core_forceidle_sum, delta);
> > +             /*
> > +              * Note: this will account forceidle to the current cpu, even
> > +              * if it comes from our SMT sibling.
> > +              */
> > +             account_forceidle_time(p, delta);
> >       }
>
> AFAICT this is the only caller of that function.
>
> >  }
> >
> > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> > index 78a233d43757..598d1026d629 100644
> > --- a/kernel/sched/cputime.c
> > +++ b/kernel/sched/cputime.c
> > @@ -226,6 +226,17 @@ void account_idle_time(u64 cputime)
> >               cpustat[CPUTIME_IDLE] += cputime;
> >  }
> >
> > +
> > +#ifdef CONFIG_SCHED_CORE
> > +/* Account for forceidle time due to core scheduling. */
> > +void account_forceidle_time(struct task_struct *p, u64 delta)
> > +{
> > +     schedstat_add(p->stats.core_forceidle_sum, delta);
>
> But then you loose the __, why?

Yea, this is probably better off as __ and a comment on the function
stating that it requires schedstat is enabled. My original thinking
was that it would be nice to avoid capturing this requirement here for
future potential use cases, especially given that the
schedstat_enabled() check is basically free anyway.

> > +
> > +     task_group_account_field(p, CPUTIME_FORCEIDLE, delta);
> > +}
> > +#endif