Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <20231201073240.T9bFNCkU@linutronix.de> <20231201080522.GA31309@didi-ThinkCentre-M930t-N000>
 <20231201161640.Z0cJLUi3@linutronix.de>
In-Reply-To: <20231201161640.Z0cJLUi3@linutronix.de>
From:   Yuanhan Zhang <zyhtheonly@gmail.com>
Date:   Sat, 2 Dec 2023 05:28:15 -0500
Message-ID: <CAEQmJ=jbB5ag5HRMjZqjych_wj_v6wTMLQxJbnJsgwNdn37kbw@mail.gmail.com>
Subject: Re: [PATCH v3] sched/cputime: let ktimers align with ksoftirqd in
 accounting CPUTIME_SOFTIRQ
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc:     zyhtheonly@yeah.net, tglx@linutronix.de, rostedt@goodmis.org,
        mingo@redhat.com, Venkatesh Pallipadi <venki@google.com>,
        peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, linux-kernel@vger.kernel.org,
        dietmar.eggemann@arm.com, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com, vschneid@redhat.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,
Thanks for your kindly interpretation.

Sebastian Andrzej Siewior <bigeasy@linutronix.de> =E4=BA=8E2023=E5=B9=B412=
=E6=9C=881=E6=97=A5=E5=91=A8=E4=BA=94 11:16=E5=86=99=E9=81=93=EF=BC=9A
>
> On 2023-12-01 16:05:41 [+0800], tiozhang wrote:
> > In CONFIG_PREEMPT_RT kernel, ktimers also calls __do_softirq,
> > so when accounting CPUTIME_SOFTIRQ, ktimers need to be accounted the sa=
me
> > as ksoftirqd.
>
> I still don't understand why this is a good thing and why want to align
> it with ksoftirqd and what breaks if we don't.

My motivation of doing this is to keep CPUTIME_SOFTIRQ in /proc/stat
remaining more accurate in PREEPT_RT kernel.

If we dont align, ktimers' cpu time is added to CPUTIME_SYSTEM when
CONFIG_IRQ_TIME_ACCOUNTING is enabled, make our stats less accurate..

>
> This "skip ksoftirqd for accounting" has been added in commit
>         b52bfee445d31 ("sched: Add IRQ_TIME_ACCOUNTING, finer accounting =
of irq time")
>
> At this point (v2.6.37) it had no accounting of time spent in ksoftirqd a=
s
> SOFTIRQ time. This was then fixed/ added by commit
>         414bee9ba613a ("softirqs: Account ksoftirqd time as cpustat softi=
rq")
>
> which went in v2.6.39. It started accounting it when it was noticed by
> the tick. So it is less accurate. The "benefit" seems to be that this
> accounting pops up in /proc/stat. As per-CPU or overall.
>
> I *guess* this was to align the softirqs which occur at the end of an
> interrupt with those which were outsourced to ksoftirqd because they
> took too long. This would patch the wording
>         =E2=80=A6 wanted to see more complete solution in not accounting =
irq
>         processing time to tasks at all.
>         https://lore.kernel.org/all/1284688596-6731-1-git-send-email-venk=
i@google.com/
>
> Or it tried to preserve the current status.
>
> A different account occurs for SOFTIRQs if they occur as port of
> hardirq and are maybe deferred to ksoftirqd vs a task raising softirqs
> on their own like packet over loopback.
> Don't see the benefit that but this is my interpretation based on what
> it does.
>
> This was v2.6.39. Since then we got threaded interrupts (also v2.6.39)
> or threaded-NAPI which utilise mostly the same mechanism as ksoftirqd
> but are treated differently. I don't see why ktimers should align with
> ksoftirqd and honestly and I don't understand why ksoftirqd had to be
> excluded to in the first place.

The main diff between ksoftirqd and force-threaded interrupt is that ksofti=
rq
is in SOFTIRQ_OFFSET (serving softirqs) while  force-threaded is in
SOFTIRQ_DISABLE_OFFSET (by using local_disable_bh).

CPUTIME_SOFTIRQ serves for time in SOFTIRQ_OFFSET (processing softirqs).
See
https://lore.kernel.org/all/1285619753-10892-1-git-send-email-venki@google.=
com/

So this leads to ksoftirqd is counted into CPUTIME_SOFTIRQ but irq-threads
into CPUTIME_SYSTEM.

Since ktimers is also in SOFTIRQ_OFFSET, align it with ksoftirq will
put it into CPUTIME_SOFTIRQ, making /proc/stat more accurate.

>
> Sorry, but I would need to go on than this.

Thanks for your time.

>
> > Signed-off-by: tiozhang <tiozhang@didiglobal.com>
>
> Sebastian