Re: [RFC PATCH] mm, compaction: kcompactd work shouldn't count towards memory PSI

[Doug Anderson]

Hi,

On Tue, Apr 18, 2023 at 12:53 PM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> On Tue, Apr 18, 2023 at 09:58:54AM -0700, Douglas Anderson wrote:
> > When the main kcompactd thread is doing compaction then it's always
> > proactive compaction. This is a little confusing because kcompactd has
> > two phases and one of them is called the "proactive" phase.
> > Specifically:
> > * Phase 1 (the "non-proactive" phase): we've been told by someone else
> >   that it would be a good idea to try to compact memory.
> > * Phase 2 (the "proactive" phase): we analyze memory fragmentation
> >   ourselves and compact if it looks fragmented.
> >
> > From the context of kcompactd, the above naming makes sense. However,
> > from the context of the kernel as a whole both phases are "proactive"
> > because in both cases we're trying compact memory ahead of time and
> > we're not actually blocking (stalling) any task who is trying to use
> > memory.
> >
> > Specifically, if any task is actually blocked needing memory to be
> > compacted then it will be in direct reclaim. That won't block waiting
> > on kcompactd task but instead call try_to_compact_pages() directly.
> > The caller of that direct compaction, __alloc_pages_direct_compact(),
> > already marks itself as counting towards PSI.
> >
> > Sanity checking by looking at this from another perspective, we can
> > look at all the people who explicitly ask kcompactd to do a reclaim by
> > calling wakeup_kcompactd(). That leads us to 3 places in vmscan.c.
> > Those are all requests from kswapd, which is also a "proactive"
> > mechanism in the kernel (tasks aren't blocked waiting for it).
>
> There is a reason behind annotating kswapd/kcompactd like this, it's
> in the longish comment in psi.c:
>
>  * The time in which a task can execute on a CPU is our baseline for
>  * productivity. Pressure expresses the amount of time in which this
>  * potential cannot be realized due to resource contention.
>  *
>  * This concept of productivity has two components: the workload and
>  * the CPU. To measure the impact of pressure on both, we define two
>  * contention states for a resource: SOME and FULL.
>  *
>  * In the SOME state of a given resource, one or more tasks are
>  * delayed on that resource. This affects the workload's ability to
>  * perform work, but the CPU may still be executing other tasks.
>  *
>  * In the FULL state of a given resource, all non-idle tasks are
>  * delayed on that resource such that nobody is advancing and the CPU
>  * goes idle. This leaves both workload and CPU unproductive.
>  *
>  *      SOME = nr_delayed_tasks != 0
>  *      FULL = nr_delayed_tasks != 0 && nr_productive_tasks == 0
>  *
>  * What it means for a task to be productive is defined differently
>  * for each resource. For IO, productive means a running task. For
>  * memory, productive means a running task that isn't a reclaimer. For
>  * CPU, productive means an oncpu task.

Ah, thanks for the pointer!


> So when you have a CPU that's running reclaim/compaction work, that
> CPU isn't available to execute the workload.
>
> Say you only have one CPU shared between an allocating thread and
> kswapd. Even if the allocating thread never has to do reclaim on its
> own, if it has to wait for the CPU behind kswapd 50% of the time, that
> workload is positively under memory pressure.

I guess I'm so much in the mindset of having 2-8 CPUs that I didn't
think as much about the single CPU case. What you say makes a lot of
sense for the single CPU case or for very parallel workloads that take
up all available CPUs, but what about when you've got extra CPUs
sitting there idling? In that case we're really not taking any CPU
cycles away from someone by having one of those CPUs doing compaction
in the background.

I'm sure this has been discussed before somewhere, but I'd also wonder
if there's ever a reason why we should prioritize kswapd/kcompactd
over user programs. AKA: why don't kswapd/kcompactd run with a big
"sched_nice" value? If kswapd/kcompactd were low enough priority then
even in the single CPU case (or the multiple CPU case with a very
parallel workload) kswapd/kcompactd would never be taking away
significant time from a real program.


> I don't think the distinction between proactive and reactive is all
> that meaningful. It's generally assumed that all the work done by
> these background threads is work that later doesn't have to be done by
> an allocating thread. It might matter from a latency perspective, but
> otherwise the work is fungible as it relates to memory pressure.

If all compaction should count towards PSI then it feels like we have
a different bug. The proactive_compact_node() function should also be
marked as counting towards PSI, shouldn't it?

-Doug