Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220520235138.3140590-1-joshdon@google.com> <20220521100037.GA2578@worktop.programming.kicks-ass.net>
In-Reply-To: <20220521100037.GA2578@worktop.programming.kicks-ass.net>
From:   Josh Don <joshdon@google.com>
Date:   Mon, 23 May 2022 14:19:36 -0700
Message-ID: <CABk29NuaNSCs-sXsDjkEbnFChRsqUYPWG9VrwS=O88Q1aMzuNA@mail.gmail.com>
Subject: Re: [PATCH v2] sched/core: add forced idle accounting for cgroups
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Ingo Molnar <mingo@redhat.com>, Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Tejun Heo <tj@kernel.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Valentin Schneider <vschneid@redhat.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Cruz Zhao <CruzZhao@linux.alibaba.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, May 21, 2022 at 3:00 AM Peter Zijlstra <peterz@infradead.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