Message-ID: <CAA7rmPG7ktzQ1_Xn3hUiC8kfoMM5R=3JqaFsjjSfTH+H2u12Nw@mail.gmail.com>
Date:   Fri, 17 Dec 2021 22:43:33 +0100
From:   Daniel Vacek <neelx.g@...il.com>
To:     unlisted-recipients:; (no To-header on input)
Cc:     Song Liu <song@...nel.org>, stockhausen@...logia.de,
        open list <linux-kernel@...r.kernel.org>,
        linux-rt-users <linux-rt-users@...r.kernel.org>,
        linux-raid <linux-raid@...r.kernel.org>, masahiroy@...nel.org,
        williams@...hat.com
Subject: Fwd: [PATCH] lib/raid6: fix abnormally high latency

Hello,

This is perfectly correct and expected result. Further explanation
follows down below.

On Wed, Dec 15, 2021 at 8:41 PM <yajun.deng@...ux.dev> wrote:
>
> December 15, 2021 1:27 AM, "Song Liu" <song@...nel.org> wrote:
>
> > On Mon, Dec 13, 2021 at 7:17 PM Yajun Deng <yajun.deng@...ux.dev> wrote:
> >
> >> We found an abnormally high latency when executing modprobe raid6_pq, the
> >> latency is greater than 1.2s when CONFIG_PREEMPT_VOLUNTARY=y, greater than
> >> 67ms when CONFIG_PREEMPT=y, and greater than 16ms when CONFIG_PREEMPT_RT=y.
> >> This is caused by disable the preemption, this time is too long and
> >> unreasonable. We just need to disable migration. so used migrate_disable()/
> >> migrate_enable() instead of preempt_disable()/preempt_enable(). This is
> >> beneficial for CONFIG_PREEMPT=y or CONFIG_PREEMPT_RT=y, but no effect for
> >> CONFIG_PREEMPT_VOLUNTARY=y.
> >>
> >> Fixes: fe5cbc6e06c7 ("md/raid6 algorithms: delta syndrome functions")
> >> Signed-off-by: Yajun Deng <yajun.deng@...ux.dev>
> >
> > We measure the speed of different RAID algorithms.If we don't disable
> > preempt, the result may be inaccurate, right? IIUC, we only disable preempt
> > for 16 jiffies. Why do we see 1.2 second delay?
>
> Here are the command of my test：
> Execute "sudo cyclictest -S -p 95 -d 0 -i 1000 -D 24h -m" in one terminal and "sudo modprobe raid6_pq" in the other terminal.
>
> Here are the results of my test：
> CONFIG_PREEMPT_VOLUNTARY=y，CONFIG_HZ_250=y
> T:17 ( 3109) P:95 I:1000 C:   7255 Min:      1 Act:    2 Avg:  170 Max: 1220832

No preemption is expected with this config so the CPU is busy and
occupied for full duration of all the benchmarks.

> CONFIG_PREEMPT=y,CONFIG_HZ_250=y
> T: 2 ( 2462) P:95 I:1000 C:  49024 Min:      1 Act:    2 Avg:    4 Max:   67888

16 jiffies with 250 Hz equals to 64 ms. So again, this result is
perfectly expected. The benchmarks can be preempted in between the
individual measurements with preemptible kernel configuration.

> CONFIG_PREEMPT_RT=y,CONFIG_HZ_1000=y
> T: 6 ( 2561) P:95 I:1000 C:  25284 Min:      3 Act:    3 Avg:    4 Max:   16593

16 jiffies by 1000 Hz is 16 ms. Ditto. So far so good.

The thing is the preemption disabled region is introduced here
precisely to guard the performance measurements and it cannot be
removed. The purpose is to ensure the measurements are protected from
external noise and hence the results are reliable and the best
performing implementation can be selected.
Changing it to migrate disable is incorrect. You can as well just
remove it completely as with preemption enabled (migrate disable does
not disable preemption) the benchmarks can be flawed with external
noise and the results are not reliable anymore.
In other words, the latency is desired here by design. Removing the
preemption disabled region is a BUG.

But really the main thing here is your test scenario is plain wrong.
It's a common misunderstanding that you should never see high latency
spikes. Sometimes you really should. There is an extended set of
operations which should be strictly avoided (with regards to latency
applications). Loading kernel modules is one example of them. It is
not considered a latency safe operation and hence it is not allowed in
production environment!

The application (or operator/the system configuration) should ensure
all the necessary modules are preloaded in setup phase before the
application workload is executed. Ie. before you start the cyclictest.

For example here, all the raid related modules *should* be loaded on
system boot. So that the initialization does not interfere with the
running production application.

Makes sense?

--nX

> >
> > Thanks,
> > Song
> >
> >> ---
> >> lib/raid6/algos.c | 8 ++++----
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/lib/raid6/algos.c b/lib/raid6/algos.c
> >> index 6d5e5000fdd7..21611d05c34c 100644
> >> --- a/lib/raid6/algos.c
> >> +++ b/lib/raid6/algos.c
> >> @@ -162,7 +162,7 @@ static inline const struct raid6_calls *raid6_choose_gen(
> >>
> >> perf = 0;
> >>
> >> - preempt_disable();
> >> + migrate_disable();
> >> j0 = jiffies;
> >> while ((j1 = jiffies) == j0)
> >> cpu_relax();
> >> @@ -171,7 +171,7 @@ static inline const struct raid6_calls *raid6_choose_gen(
> >> (*algo)->gen_syndrome(disks, PAGE_SIZE, *dptrs);
> >> perf++;
> >> }
> >> - preempt_enable();
> >> + migrate_enable();
> >>
> >> if (perf > bestgenperf) {
> >> bestgenperf = perf;
> >> @@ -186,7 +186,7 @@ static inline const struct raid6_calls *raid6_choose_gen(
> >>
> >> perf = 0;
> >>
> >> - preempt_disable();
> >> + migrate_disable();
> >> j0 = jiffies;
> >> while ((j1 = jiffies) == j0)
> >> cpu_relax();
> >> @@ -196,7 +196,7 @@ static inline const struct raid6_calls *raid6_choose_gen(
> >> PAGE_SIZE, *dptrs);
> >> perf++;
> >> }
> >> - preempt_enable();
> >> + migrate_enable();
> >>
> >> if (best == *algo)
> >> bestxorperf = perf;
> >> --
> >> 2.32.0

Powered by blists - more mailing lists