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;
Date:   Mon, 24 Oct 2022 13:03:11 +0200
From:   Frederic Weisbecker <frederic@kernel.org>
To:     Aaron Tomlin <atomlin@redhat.com>
Cc:     mtosatti@redhat.com, cl@linux.com, tglx@linutronix.de,
        mingo@kernel.org, peterz@infradead.org, pauld@redhat.com,
        neelx@redhat.com, oleksandr@natalenko.name, atomlin@atomlin.com,
        akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH v8 3/5] mm/vmstat: Do not queue vmstat_update if tick is
 stopped
Message-ID: <20221024110311.GA1285913@lothringen>
References: <20220924152227.819815-1-atomlin@redhat.com>
 <20220924152227.819815-4-atomlin@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220924152227.819815-4-atomlin@redhat.com>
Precedence: bulk

On Sat, Sep 24, 2022 at 04:22:25PM +0100, Aaron Tomlin wrote:
> From: Marcelo Tosatti <mtosatti@redhat.com>
> 
> From the vmstat shepherd, for CPUs that have the tick stopped, do not
> queue local work to flush the per-CPU vmstats, since in that case the
> flush is performed on return to userspace or when entering idle. Also
> cancel any delayed work on the local CPU, when entering idle on nohz
> full CPUs. Per-CPU pages can be freed remotely from housekeeping CPUs.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> ---
>  mm/vmstat.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 472175642bd9..3b9a497965b4 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -29,6 +29,7 @@
>  #include <linux/page_ext.h>
>  #include <linux/page_owner.h>
>  #include <linux/migrate.h>
> +#include <linux/tick.h>
>  
>  #include "internal.h"
>  
> @@ -1990,19 +1991,23 @@ static void vmstat_update(struct work_struct *w)
>   */
>  void quiet_vmstat(void)
>  {
> +	struct delayed_work *dw;
> +
>  	if (system_state != SYSTEM_RUNNING)
>  		return;
>  
>  	if (!is_vmstat_dirty())
>  		return;
>  
> +	refresh_cpu_vm_stats(false);
> +
>  	/*
> -	 * Just refresh counters and do not care about the pending delayed
> -	 * vmstat_update. It doesn't fire that often to matter and canceling
> -	 * it would be too expensive from this path.
> -	 * vmstat_shepherd will take care about that for us.
> +	 * If the tick is stopped, cancel any delayed work to avoid
> +	 * interruptions to this CPU in the future.
>  	 */
> -	refresh_cpu_vm_stats(false);
> +	dw = &per_cpu(vmstat_work, smp_processor_id());
> +	if (delayed_work_pending(dw) && tick_nohz_tick_stopped())
> +		cancel_delayed_work(dw);
>  }
>  
>  /*
> @@ -2024,6 +2029,9 @@ static void vmstat_shepherd(struct work_struct *w)
>  	for_each_online_cpu(cpu) {
>  		struct delayed_work *dw = &per_cpu(vmstat_work, cpu);
>  
> +		if (tick_nohz_tick_stopped_cpu(cpu))
> +			continue;
> +
>  		if (!delayed_work_pending(dw) && per_cpu(vmstat_dirty, cpu))
>  			queue_delayed_work_on(cpu, mm_percpu_wq, dw, 0);

All these checks are racy though. You may well eventually:

1) Arm the timer after the CPU has entered in userspace
2) Not arm the timer when the CPU has entered the kernel

How about converting that to an IPI instead? This should be a good candidate
for the future IPI deferment.

Another possible way to go is this:

1) vmstat_shepherd completely ignores nohz_full CPUs
2) vmstat_work is only ever armed locally
3) A nohz_full CPU turning its local vmstat as dirty checks if vmstat_work is
   pending. If not, queue it, possibly through a self IPI (IRQ_WORK) to get
   away with current locking context.
3) Fold on idle if dirty
4) Fold on user enter and disarm vmstat_work if pending

Does that sound possible?

Thanks.


>  
> -- 
> 2.37.1
>