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: <20220225012819.1807147-1-surenb@google.com> <20220302002150.2113-1-hdanton@sina.com>
 <CAJuCfpG3TRgs8fA5kpkhrFRcDF=C9VyqLTCz42sxOyKZ0pRaNQ@mail.gmail.com> <YiY0Zf9XH/m/IqcR@alley>
In-Reply-To: <YiY0Zf9XH/m/IqcR@alley>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Mon, 7 Mar 2022 08:48:53 -0800
Message-ID: <CAJuCfpGU3uimENmBP=J8OrfxCrFQtOsPMqU_JD5DyW46aW1M2g@mail.gmail.com>
Subject: Re: [RFC 1/1] mm: page_alloc: replace mm_percpu_wq with kthreads in drain_all_pages
To:     Petr Mladek <pmladek@suse.com>
Cc:     Hillf Danton <hdanton@sina.com>, linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Mel Gorman <mgorman@suse.de>, Michal Hocko <mhocko@suse.com>,
        Tim Murray <timmurray@google.com>,
        Minchan Kim <minchan@google.com>,
        Johannes Weiner <hannes@cmpxchg.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Mar 7, 2022 at 8:35 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2022-03-02 15:06:24, Suren Baghdasaryan wrote:
> > On Tue, Mar 1, 2022 at 4:22 PM Hillf Danton <hdanton@sina.com> wrote:
> > >
> > > On Thu, 24 Feb 2022 17:28:19 -0800 Suren Baghdasaryan wrote:
> > > > Sending as an RFC to confirm if this is the right direction and to
> > > > clarify if other tasks currently executed on mm_percpu_wq should be
> > > > also moved to kthreads. The patch seems stable in testing but I want
> > > > to collect more performance data before submitting a non-RFC version.
> > > >
> > > >
> > > > Currently drain_all_pages uses mm_percpu_wq to drain pages from pcp
> > > > list during direct reclaim. The tasks on a workqueue can be delayed
> > > > by other tasks in the workqueues using the same per-cpu worker pool.
> > >
> > > The pending works may be freeing a couple of slabs/pages each. Who knows?
> >
> > If we are talking about work specifically scheduled on mm_percpu_wq
> > then apart from drain_all_pages, mm_percpu_wq is used to execute
> > vmstat_update and lru_add_drain_cpu for drainig pagevecs. If OTOH what
> > you mean is that the work might be blocked by say kswapd, which is
> > freeing memory, then sure, who knows...
>
> Note that the same worker pool is used by many workqueues. And
> work items in per-cpu workqueues are serialized on a single worker.
> Another worker is used only when a work goes into a sleeping wait.
>
> I want to say that "drain_all_pages" are not blocked only by other
> works using "mm_percpu_wq" but also by works from many other
> workqueues, including "system_wq".
>
> These works might do anything, including memory allocation, freeing.

Ah, I didn't know this (I think you mentioned it in one of your
previous replies but I missed it). Thank you for clarifying!

>
> > >
> > > > This results in sizable delays in drain_all_pages when cpus are highly
> > > > contended.
> > > > Memory management operations designed to relieve memory pressure should
> > > > not be allowed to block by other tasks, especially if the task in direct
> > > > reclaim has higher priority than the blocking tasks.
> > >
> > > Wonder why priority is the right cure to tight memory - otherwise it was
> > > not a problem given a direct reclaimer of higher priority.
> > >
> > > Off topic question - why is it making sense from begining for a task of
> > > lower priority to peel pages off from another of higher priority if
> > > priority is considered in direct reclaim?
> >
> > The way I understood your question is that you are asking why we have
> > to use workqueues of potentially lower priority to drain pages for a
> > potentially higher priority process in direct reclaim (which is
> > blocked waiting for workqueues to complete draining)?
> > If so, IIUC this mechanism was introduced in
> > https://lore.kernel.org/all/20170117092954.15413-4-mgorman@techsingularity.net
> > to avoid draining from IPI context (CC'ing Mel Gorman to correct me if
> > I'm wrong).
> > I think the issue here is that in the process we are losing
> > information about the priority of the process in direct reclaim, which
> > might lead to priority inversion.
>
> Note that priority of workqueue workers is static. It is defined
> by the workqueue parameters.
>
> kthread_worker API allows to create custom kthreads. The user could
> modify the priority as needed. It allows to prevent priority
> inversion. It can hardly be achieved by workques where the workers
> are heavily shared by unrelated tasks.

Yes but I suspect we would not want to dynamically change the priority
of the kthreads performing drain_all_pages? I guess we could adjust it
based on the highest priority among the tasks waiting for
drain_all_pages and that would eliminate priority inversion. However
I'm not sure about the possible overhead associated with such dynamic
priority adjustments.
My RFC sets up the kthreads to be low priority FIFO threads. It's a
simplification and I'm not sure that is the right approach here...

>
> Best Regards,
> Petr