Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1631635551-8583-1-git-send-email-wang.yong12@zte.com.cn> <YUHqG0P6Ahs8FvN+@dhcp22.suse.cz>
In-Reply-To: <YUHqG0P6Ahs8FvN+@dhcp22.suse.cz>
From:   yong w <yongw.pur@gmail.com>
Date:   Fri, 17 Sep 2021 23:38:31 +0800
Message-ID: <CAOH5QeB==iASFSeHWZXT0ESBCN5W6mJ8cugJih1uKpqJZbtgqQ@mail.gmail.com>
Subject: Re: [PATCH v2] vmpressure: wake up work only when there is
 registration event
To:     Michal Hocko <mhocko@suse.com>
Cc:     Tejun Heo <tj@kernel.org>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        wang.yong12@zte.com.cn, Cgroups <cgroups@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux MM <linux-mm@kvack.org>, yang.yang29@zte.com.cn
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Michal Hocko <mhocko@suse.com> =E4=BA=8E2021=E5=B9=B49=E6=9C=8815=E6=97=A5=
=E5=91=A8=E4=B8=89 =E4=B8=8B=E5=8D=888:42=E5=86=99=E9=81=93=EF=BC=9A
>
> On Tue 14-09-21 09:05:51, yongw.pur@gmail.com wrote:
> > From: wangyong <wang.yong12@zte.com.cn>
> >
> > Use the global variable num_events to record the number of vmpressure
> > events registered by the system, and wake up work only when there is
> > registration event.
> > Usually, the vmpressure event is not registered in the system, this pat=
ch
> > can avoid waking up work and doing nothing.
>
> I have asked in the previous version and this changelog doesn't that
> explain again. Why don't you simply bail out early in vmpressure()
> entry?
Thanks for your reply.
Because the else branch will modify the socket_pressure, and will not wake =
up
the work. It is necessary to judge the tree parameters at the same
time, like this
if (tree && !static_branch_unlikely(&num_events))
    return;
It's not good to judge the tree twice parameters in the function.
>
> > Test with 5.14.0-rc5-next-20210813 on x86_64 4G ram.
> > Consume cgroup memory until it is about to be reclaimed, then execute
> > "perf stat -I 2000 malloc.out" command to trigger memory reclamation
> > and get performance results.
> > The context-switches is reduced by about 20 times.
>
> Is this test somewhere available so that it can be reproduced by
> others. Also while the number of context switches can be an interesting
> it is not really clear from this evaluation whether that actually
> matters or not. E.g. what does an increase of task-clock and twice as
> many instructions recorded tell us?
>
The test program is a simple malloc  process, which allocate memory
and fill some data.
I think it may be that more instructions can be executed per unit time.
> > unpatched:
> > Average of 10 test results
> > 582.4674048   task-clock(msec)
> > 19910.8               context-switches
> > 0             cpu-migrations
> > 1292.9                page-faults
> > 414784733.1   cycles
>
> > <not supported>       stalled-cycles-frontend
> > <not supported>       stalled-cycles-backend
>
> Why is this a part of the data?
>
> > 580070698.4   instructions
> > 125572244.7   branches
> > 2073541.2     branch-misses
> >
> > patched
> > Average of 10 test results
> > 973.6174796   task-clock(msec)
> > 988.6         context-switches
> > 0             cpu-migrations
> > 1785.2                page-faults
> > 772883602.4   cycles
> > <not supported>       stalled-cycles-frontend
> > <not supported>       stalled-cycles-backend
> > 1360280911    instructions
> > 290519434.9   branches
> > 3378378.2     branch-misses
> >
> > Tested-by: Zeal Robot <zealci@zte.com.cn>
> > Signed-off-by: wangyong <wang.yong12@zte.com.cn>
> > ---
> >
> [...]
> > @@ -272,6 +277,9 @@ void vmpressure(gfp_t gfp, struct mem_cgroup *memcg=
, bool tree,
> >               return;
> >
> >       if (tree) {
> > +             if (!static_branch_unlikely(&num_events))
> > +                     return;
>
> We usually hide the change behind a static inline helper (e.g.
> vmpressure_disabled()). I would also put it to the beginning of
> vmpressure or put an explanation why it makes sense only in this branch.
> --
Because only this branch needs to wake up work.
Yes, static inline helper is more easier to read and understand.