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, 17 Oct 2022 13:04:47 -0300
From:   Marcelo Tosatti <mtosatti@redhat.com>
To:     Hillf Danton <hdanton@sina.com>
Cc:     Aaron Tomlin <atomlin@redhat.com>, frederic@kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v8 5/5] tick/sched: Ensure quiet_vmstat() is called when
 the idle tick was stopped too
Message-ID: <Y019H0l48s9bXfxc@fuller.cnet>
References: <20220924152227.819815-1-atomlin@redhat.com>
 <20220925010511.1482-1-hdanton@sina.com>
 <20221003124435.1769-1-hdanton@sina.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221003124435.1769-1-hdanton@sina.com>
Precedence: bulk

On Mon, Oct 03, 2022 at 08:44:35PM +0800, Hillf Danton wrote:
> On 26 Sep 2022 10:20:04 +0100  Aaron Tomlin <atomlin@redhat.com> wrote:
> > On Sun 2022-09-25 09:05 +0800, Hillf Danton wrote:
> > > On 24 Sep 2022 16:24:41 +0100 Aaron Tomlin <atomlin@redhat.com> wrote:
> > > > 
> > > > In the context of the idle task and an adaptive-tick mode/or a nohz_full
> > > > CPU, quiet_vmstat() can be called: before stopping the idle tick,
> > > > entering an idle state and on exit. In particular, for the latter case,
> > > > when the idle task is required to reschedule, the idle tick can remain
> > > > stopped and the timer expiration time endless i.e., KTIME_MAX. Now,
> > > > indeed before a nohz_full CPU enters an idle state, CPU-specific vmstat
> > > > counters should be processed to ensure the respective values have been
> > > > reset and folded into the zone specific 'vm_stat[]'. That being said, it
> > > > can only occur when: the idle tick was previously stopped, and
> > > > reprogramming of the timer is not required.
> > > > 
> > > > A customer provided some evidence which indicates that the idle tick was
> > > > stopped; albeit, CPU-specific vmstat counters still remained populated.
> > > > Thus one can only assume quiet_vmstat() was not invoked on return to the
> > > > idle loop.
> > > 
> > > Why did housekeeping CPUs fail to do their works, with this assumption
> > > put aside?
> > 
> > Hi Hillf,
> > 
> > I'm not sure I understand your question.
> > 
> > In this context, when tick processing is stopped, delayed work is not going
> > to be handled until the CPU exits idle.
> 
> Given work canceled because per-CPU pages can be freed remotely from
> housekeeping CPUs (see patch 3/5), what is added here is not needed.
> 
> IOW which one is incorrect?
> 
> BTW given delayed work is not going to be handled until the CPU exits idle,

Hi Hilf,

The comment on the codebase now is:

void quiet_vmstat(void)
{
        if (system_state != SYSTEM_RUNNING)
                return;

        if (!delayed_work_pending(this_cpu_ptr(&vmstat_work)))
                return;

        if (!need_update(smp_processor_id()))
                return;
        
        /*
         * 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.
         */
        refresh_cpu_vm_stats(false);
}

However this is incorrect. The pending delayed work is only cancelled
when executed and not requeued from:

static void vmstat_update(struct work_struct *w)
{
        if (refresh_cpu_vm_stats(true)) {
                /*
                 * Counters were updated so we expect more updates
                 * to occur in the future. Keep on running the
                 * update worker thread.
                 */
                queue_delayed_work_on(smp_processor_id(), mm_percpu_wq,
                                this_cpu_ptr(&vmstat_work),
                                round_jiffies_relative(sysctl_stat_interval));
        }
}

Since this patchset changes the synchronization to happen at return to
userspace or entering idle, we do want to cancel that work (which, after
synchronization, is not necessary).

> canceling work is noop in 3/5, despite what the vmstat shepherd does depends
> not on tick.

Canceling work is a not a noop in 3/5: If the work is not cancelled (if 3/5 
is dropped), there will be a pending work to be executed, from the kworker thread 
on an isolated CPU. Which is undesired for a fully isolated CPU, with no
interruptions.