Re: [RFC 1/1] mm: page_alloc: replace mm_percpu_wq with kthreads in drain_all_pages

[Suren Baghdasaryan]

On Tue, Mar 1, 2022 at 4:25 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Thu 2022-02-24 17:28:19, 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.
> > 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.
> > Replace the usage of mm_percpu_wq with per-cpu low priority FIFO
> > kthreads to execute draining tasks.
> >
> > Suggested-by: Petr Mladek <pmladek@xxxxxxxx>
> > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
>
> The patch looks good to me. See few comments below about things
> where I was in doubts. But I do not see any real problem with
> this approach.

Thanks for the review, Petr. One question inline.

Other than that I would like to check if:
1. Using low priority FIFO for these kthreads is warranted. From
https://lore.kernel.org/all/CAEe=Sxmow-jx60cDjFMY7qi7+KVc+BT++BTdwC5+G9E=1soMmQ@xxxxxxxxxxxxxx/#t
my understanding was that we want this work to be done by RT
kthread_worker but maybe that's not appropriate here?
2. Do we want to move any other work done on mm_percpu_wq
(vmstat_work, lru_add_drain_all) to these kthreads?
If what I have currently is ok, I'll post the first version.
Thanks,
Suren.



>
> > ---
> >  mm/page_alloc.c | 84 ++++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 70 insertions(+), 14 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 3589febc6d31..c9ab2cf4b05b 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2209,6 +2210,58 @@ _deferred_grow_zone(struct zone *zone, unsigned int order)
> >
> >  #endif /* CONFIG_DEFERRED_STRUCT_PAGE_INIT */
> >
> > +static void drain_local_pages_func(struct kthread_work *work);
> > +
> > +static int alloc_drain_worker(unsigned int cpu)
> > +{
> > +     struct pcpu_drain *drain;
> > +
> > +     mutex_lock(&pcpu_drain_mutex);
> > +     drain = per_cpu_ptr(&pcpu_drain, cpu);
> > +     drain->worker = kthread_create_worker_on_cpu(cpu, 0, "pg_drain/%u", cpu);
> > +     if (IS_ERR(drain->worker)) {
> > +             drain->worker = NULL;
> > +             pr_err("Failed to create pg_drain/%u\n", cpu);
> > +             goto out;
> > +     }
> > +     /* Ensure the thread is not blocked by normal priority tasks */
> > +     sched_set_fifo_low(drain->worker->task);
> > +     kthread_init_work(&drain->work, drain_local_pages_func);
> > +out:
> > +     mutex_unlock(&pcpu_drain_mutex);
> > +
> > +     return 0;
> > +}
> > +
> > +static int free_drain_worker(unsigned int cpu)
> > +{
> > +     struct pcpu_drain *drain;
> > +
> > +     mutex_lock(&pcpu_drain_mutex);
> > +     drain = per_cpu_ptr(&pcpu_drain, cpu);
> > +     kthread_cancel_work_sync(&drain->work);
>
> I do see not how CPU down was handled in the original code.
>
> Note that workqueues call unbind_workers() when a CPU
> is going down. The pending work items might be proceed
> on another CPU. From this POV, the new code looks more
> safe.
>
> > +     kthread_destroy_worker(drain->worker);
> > +     drain->worker = NULL;
> > +     mutex_unlock(&pcpu_drain_mutex);
> > +
> > +     return 0;
> > +}
> > +
> > +static void __init init_drain_workers(void)
> > +{
> > +     unsigned int cpu;
> > +
> > +     for_each_online_cpu(cpu)
> > +             alloc_drain_worker(cpu);
>
> I though whether this need to be called under cpus_read_lock();
> And I think that the code should be safe as it is. There
> is this call chain:
>
>   + kernel_init_freeable()
>     + page_alloc_init_late()
>       + init_drain_workers()
>
> It is called after smp_init() but before the init process
> is executed. I guess that nobody could trigger CPU hotplug
> at this state. So there there is no need to synchronize
> against it.

Should I add a comment here to describe why we don't need
cpus_read_lock here (due to init process not being active at this
time)?

>
> > +
> > +     if (cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> > +                                     "page_alloc/drain:online",
> > +                                     alloc_drain_worker,
> > +                                     free_drain_worker)) {
> > +             pr_err("page_alloc_drain: Failed to allocate a hotplug state\n");
>
> I am not sure if there are any special requirements about the
> ordering vs. other CPU hotplug operations.
>
> Just note that the per-CPU workqueues are started/stopped
> via CPUHP_AP_WORKQUEUE_ONLINE. They are available slightly
> earlier before CPUHP_AP_ONLINE_DYN when the CPU is being
> enabled.
>
> > +     }
> > +}
> > +
> >  void __init page_alloc_init_late(void)
> >  {
> >       struct zone *zone;
>
> Best Regards,
> Petr