linux-kernel - Re: [PATCH v7 00/13] fold per-CPU vmstats remotely

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZESFas7tX9W/yfha@tpad>
Date:   Sat, 22 Apr 2023 22:10:02 -0300
From:   Marcelo Tosatti <mtosatti@...hat.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     Frederic Weisbecker <frederic@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Christoph Lameter <cl@...ux.com>,
        Aaron Tomlin <atomlin@...mlin.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Russell King <linux@...linux.org.uk>,
        Huacai Chen <chenhuacai@...nel.org>,
        Heiko Carstens <hca@...ux.ibm.com>, x86@...nel.org,
        Vlastimil Babka <vbabka@...e.cz>
Subject: Re: [PATCH v7 00/13] fold per-CPU vmstats remotely

On Thu, Apr 20, 2023 at 10:40:25AM +0200, Michal Hocko wrote:
> On Wed 19-04-23 13:35:12, Marcelo Tosatti wrote:
> [...]
> > This is a burden for application writers and for system configuration.
> 
> Yes. And I find it reasonable to expect that burden put there as there
> are non-trivial requirements for those workloads anyway. It is not
> out-of-the-box thing, right?

See below.

> > Or it could be done automatically (from outside of the application).
> > Which is what is described and implemented here:
> > 
> > https://lore.kernel.org/lkml/20220204173537.429902988@fedora.localdomain/
> > 
> > "Task isolation is divided in two main steps: configuration and
> > activation.
> > 
> > Each step can be performed by an external tool or the latency
> > sensitive application itself. util-linux contains the "chisol" tool
> > for this purpose."
> 
> I cannot say I would be a fan of prctl interfaces in general but I do
> agree with the overal idea to forcing a quiescent state on a set of
> CPUs.

This has been avoided with success so far.

> > But not only that, the second thing is:
> > 
> > "> Another important point is this: if an application dirties
> > > its own per-CPU vmstat cache, while performing a system call,
> > 
> > Or while handling a VM-exit from a vCPU.
> 
> Do you have any specific examples on this?

Interrupt handling freeing a page. 

handle_access_fault             (ARM64) ->
handle_changed_spte_acc_track   (x86)   -> kvm_set_pfn_accessed -> kvm_set_page_accessed -> mark_page_accessed ->
                                           folio_mark_accessed -> folio_activate -> folio_activate_fn ->
                                           lruvec_add_folio -> update_lru_size -> __update_lru_size ->
                                            __mod_zone_page_state(&pgdat->node_zones[zid],
                                                                  NR_ZONE_LRU_BASE + lru, nr_pages);


The other option would be to _FORBID_ use of  __mod_zone_page_state in certain code sections.

> > This are, in my mind, sufficient reasons to discard the "flush per-cpu
> > caches" idea. This is also why i chose to abandon the prctrl interface
> > patchset.
> > 
> > > and a vmstat sync event is triggered on a different CPU, you'd have to:                                                             
> > >                                                                                                                                     
> > > 1) Wait for that CPU to return to userspace and sync its stats
> > > (unfeasible).
> > >
> > > 2) Queue work to execute on that CPU (undesirable, as that causes
> > > an interruption).
> > >
> > > 3) Remotely sync the vmstat for that CPU."
> > 
> > So the only option is to remotely sync vmstat for the CPU
> > (unless you have a better suggestion).
> 
> `echo 1 > /proc/sys/vm/stat_refresh' achieves essentially the same
> without any kernel changes.

It is unsuitable. You'd have to guarantee that, by the time you return
from the write() system call to that file, there has been no other 
mod_zone_page_state call. For example, no interrupt 
or exception that frees or allocates a page through rmqueue 
(NR_FREE_PAGES counter), or that bounce_end_io cannot be called 
(since it calls dec_zone_page_state).

It has been used internally as a workaround, but it is not reliable.

> But let me repeat, this is not just about vmstats. Just have a look at
> other queue_work_on users. You do not want to handy pick each and every
> one and do so in the future as well.

The ones that are problematic are being fixed for sometime now. For example:

commit 2de79ee27fdb52626ac4ac48ec6d8d52ba6f9047
Author: Paolo Abeni <pabeni@...hat.com>
Date:   Thu Sep 10 23:33:18 2020 +0200

    net: try to avoid unneeded backlog flush

    flush_all_backlogs() may cause deadlock on systems
    running processes with FIFO scheduling policy.

    The above is critical in -RT scenarios, where user-space
    specifically ensure no network activity is scheduled on
    the CPU running the mentioned FIFO process, but still get
    stuck.

    This commit tries to address the problem checking the
    backlog status on the remote CPUs before scheduling the
    flush operation. If the backlog is empty, we can skip it.

    v1 -> v2:
     - explicitly clear flushed cpu mask - Eric

    Signed-off-by: Paolo Abeni <pabeni@...hat.com>
    Signed-off-by: David S. Miller <davem@...emloft.net>


And it has been a normal process so far.

I think what needs to be done is to avoid new queue_work_on() 
users from being introduced in the tree (the number of
existing ones is finite and can therefore be fixed).