Re: [PATCH v2 3/3] mm/vmstat: do not refresh stats for nohz_full CPUs

[Marcelo Tosatti]

On Mon, Jun 05, 2023 at 03:14:25PM -0300, Marcelo Tosatti wrote:
> On Mon, Jun 05, 2023 at 06:10:57PM +0200, Michal Hocko wrote:
> > On Mon 05-06-23 12:43:24, Marcelo Tosatti wrote:
> > > On Mon, Jun 05, 2023 at 09:59:57AM +0200, Michal Hocko wrote:
> > > > On Fri 02-06-23 15:58:00, Marcelo Tosatti wrote:
> > > > > The interruption caused by queueing work on nohz_full CPUs 
> > > > > is undesirable for certain aplications.
> > > > 
> > > > This is not a proper changelog. I am not going to write a changelog for
> > > > you this time. Please explain why this is really needed and why this
> > > > approach is desired. 
> > > > E.g. why don't you prevent userspace from
> > > > refreshing stats if interference is not desirable.
> > > 
> > > Michal,
> > > 
> > > Can you please check if the following looks better, as
> > > a changelog? thanks
> > > 
> > > ---
> > > 
> > > schedule_work_on API uses the workqueue mechanism to
> > > queue a work item on a queue. A kernel thread, which
> > > runs on the target CPU, executes those work items.
> > > 
> > > Therefore, when using the schedule_work_on API,
> > > it is necessary for the kworker kernel thread to
> > > be scheduled in, for the work function to be executed.
> > > 
> > > Time sensitive applications such as SoftPLCs
> > > (https://tum-esi.github.io/publications-list/PDF/2022-ETFA-How_Real_Time_Are_Virtual_PLCs.pdf),
> > > have their response times affected by such interruptions.
> > > 
> > > The /proc/sys/vm/stat_refresh file was originally introduced by
> > > 
> > > commit 52b6f46bc163eef17ecba4cd552beeafe2b24453
> > > Author: Hugh Dickins <hughd@xxxxxxxxxx>
> > > Date:   Thu May 19 17:12:50 2016 -0700
> > > 
> > >     mm: /proc/sys/vm/stat_refresh to force vmstat update
> > > 
> > >     Provide /proc/sys/vm/stat_refresh to force an immediate update of
> > >     per-cpu into global vmstats: useful to avoid a sleep(2) or whatever
> > >     before checking counts when testing.  Originally added to work around a
> > >     bug which left counts stranded indefinitely on a cpu going idle (an
> > >     inaccuracy magnified when small below-batch numbers represent "huge"
> > >     amounts of memory), but I believe that bug is now fixed: nonetheless,
> > >     this is still a useful knob.
> > 
> > No need to quote the full changelog.

I think its useful to put things in perspective.

> > > Other than the potential interruption to a time sensitive application,
> > > if using SCHED_FIFO or SCHED_RR priority on the isolated CPU, then
> > > system hangs can occur:
> > > 
> > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=978688
> > 
> > Confused... This report says that accessing the file (i.e. to force the
> > refresh) can get stalled because high priority tasks will not allow
> > kworkers to run. No?
> 
> Yes.
> 
> > There is simply no way around that unless those kworkers inherit the
> > priority.
> 
> stalld is an attempt to workaround the situation by allowing the 
> lower priority processes to execute for a small amount of time
> (for example 20us every 1s). https://github.com/bristot/stalld:
> 
> "The stalld program (which stands for 'stall daemon') is a mechanism to
> prevent the starvation of operating system threads in a Linux system.
> The premise is to start up on a housekeeping cpu (one that is not used
> for real-application purposes) and to periodically monitor the state of
> each thread in the system, looking for a thread that has been on a run
> queue (i.e. ready to run) for a specifed length of time without being
> run. This condition is usually hit when the thread is on the same cpu
> as a high-priority cpu-intensive task and therefore is being given no
> opportunity to run.
> 
> When a thread is judged to be starving, stalld changes that thread to
> use the SCHED_DEADLINE policy and gives the thread a small slice of time
> for that cpu (specified on the command line). The thread then runs and
> when that timeslice is used, the thread is then returned to its original
> scheduling policy and stalld then continues to monitor thread states."
> 
> Unfortunately, if you allow that, then the latency sensitive
> application might be interrupted for longer than acceptable
> (which is the case for a certain class of applications, for example
> SoftPLC inside a VM).
> 
> > It certainly is unfortunate that the call is not killable
> > but being stuck behind real time busy looping processes is nothing
> > really uncommong. One has to be really careful when using real time
> > priorities.
> 
> Yes.
> 
> > > To avoid the problems above, do not schedule the work to synchronize
> > > per-CPU mm counters on isolated CPUs. Given the possibility for
> > > breaking existing userspace applications, avoid changing
> > > behaviour of access to /proc/sys/vm/stat_refresh, such as
> > > returning errors to userspace.
> > 
> > You are changing the behavior. The preexisting behavior was to flush
> > everything. This is clearly changing that.
> 
> I meant that this patch does not cause read/write to the procfs file 
> to return errors.
> 
> I believe returning errors has a higher potential for regressions
> than not flushing per-CPU VM counters of isolated CPUs (which are
> bounded).
> 
> > > ---
> > > 
> > > > Also would it make some sense to reduce flushing to cpumask 
> > > > of the calling process? (certainly a daring thought but have
> > > > you even considered it?)
> > > 
> > > Fail to see the point here ?
> > 
> > I mean that, if you already want to change the semantic of the call then
> > it would likely be safer to change it in a more robust way and only
> > flush pcp vmstat caches that are in the process effective cpu mask. 
> 
> That would change behaviour for systems without isolated CPUs.
> 
> > This
> > way one can control which pcp caches to flush (e.g. those that are not
> > on isolated CPUs or contrary those that are isolated but you can afford 
> > to flush at the specific moment). See?
> 
> Yes, but not sure what to think of this idea. 
> 
> > Now I am not saying this is the right way to go because there is still a
> > slim chance this will break userspace expectations. Therefore I have
> > asked why you simply do not stop any random application accessing
> > stat_refresh in the first place.
> 
> I think this is what should be done, but not on the current patchset.
> 
> https://lkml.iu.edu/hypermail/linux/kernel/2209.1/01263.html
> 
> Regarding housekeeping flags, it is usually the case that initialization might
> require code execution on interference blocked CPUs (for example MTRR
> initialization, resctrlfs initialization, MSR writes, ...). Therefore
> tagging the CPUs after system initialization is necessary, which
> is not possible with current housekeeping flags infrastructure.
> 
> >  These highly specialized setups with
> > isolated resources shouldn't run arbitrary crap, should that?
> 
> Problem is that its hard to control what people run on a system.
> 
> > What if I just start allocating memory and get the system close to OOM. 
> 
> Sure, or "poweroff".
> 
> > I am
> > pretty sure a small latency induced by the vmstat refreshes is the least
> > problem you will have.
> 
> If OOM codepath sends no IPI or queues work on isolated CPUs, then OOM
> should be fine.
> 
> > So please step back and try to think whether this is actually fixing
> > anything real before trying to change a user visible interface.
> 
> It is fixing either a latency violation or a hang on a system where some user or
> piece of software happens to run "sysctl -a" (or read vmstat_refresh).
> 
> If one is using CPU isolation, the latency violation has higher 
> priority than vmstat_refresh returning proper counters.

OK, so this patch is not going to include the per-CPU vmstat counters
(up to the threshold) in the synchronization step of reading/writing to the
vmstat_refresh file.

This is a tradeoff: one prefers not to have accurate counters 
(for a procfs file whose value is going to be interpreted, and which 
accurate value might or might not be important) than to interrupt
an isolated CPU.