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, 5 Jun 2023 22:22:13 +0200
From:   Michal Hocko <mhocko@suse.com>
To:     Marcelo Tosatti <mtosatti@redhat.com>
Cc:     Christoph Lameter <cl@linux.com>,
        Aaron Tomlin <atomlin@atomlin.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Vlastimil Babka <vbabka@suse.cz>
Subject: Re: [PATCH v3 3/3] mm/vmstat: do not refresh stats for isolated CPUs
Message-ID: <ZH5D9cqJnPJ27OY8@dhcp22.suse.cz>
References: <20230605185627.923698377@redhat.com>
 <20230605190132.087124739@redhat.com>
 <ZH41bFWrc0LWae/U@dhcp22.suse.cz>
 <ZH49Tx4kbOG1zESS@tpad>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZH49Tx4kbOG1zESS@tpad>
Precedence: bulk

On Mon 05-06-23 16:53:51, Marcelo Tosatti wrote:
> On Mon, Jun 05, 2023 at 09:20:12PM +0200, Michal Hocko wrote:
> > On Mon 05-06-23 15:56:30, Marcelo Tosatti wrote:
> > > 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
> > > with the goal to:
> > > 
> > > "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."
> > > 
> > > 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:
> > 
> > The same thing can happen without isolated CPUs and this patch doesn't
> > help at all.
> > 
> > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=978688
> > 
> > And this is an example of that...
> > 
> > > 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 returning
> > > errors from access to /proc/sys/vm/stat_refresh.
> > > 
> > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > It would be really helpful to not post new versions while discussion of
> > the previous one is still not done.
> > 
> > Anyway
> > Nacked-by: Michal Hocko <mhocko@suse.com>
> > 
> > This is silently changing semantic and I do not think you have actually
> > shown this is a real life problem. 
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1921601

Yet anothe new reference. Is the story complete now?

> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=978688

Again, nothing isolcpu related. Simple RR/realtime cpu hoggers that this
patch has not way to deal with. Why do you keep referencing it?

> > To me it sounds like a theoretical
> > issue 
> 
> Its not (see data above).

No it't not. Either your sosreport need precise counters and
stat_refresh is exactly what it should be doing or it can live with
imprecision and then it should avoid it. Full stop. There is no reason
to tweak the existing semantic to half bake isolcpu case while the
fundamental problem is still there.
-- 
Michal Hocko
SUSE Labs