linux-kernel - Re: [PATCH v2 01/11] mm/vmstat: remove remote node draining

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: <ZADbC9RnmVtpC6kE@x1n>
Date:   Thu, 2 Mar 2023 12:21:15 -0500
From:   Peter Xu <peterx@...hat.com>
To:     Marcelo Tosatti <mtosatti@...hat.com>
Cc:     Christoph Lameter <cl@...ux.com>,
        Aaron Tomlin <atomlin@...mlin.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v2 01/11] mm/vmstat: remove remote node draining

On Thu, Feb 09, 2023 at 12:01:51PM -0300, Marcelo Tosatti wrote:
> Draining of pages from the local pcp for a remote zone was necessary
> since:
> 
> "Note that remote node draining is a somewhat esoteric feature that is
> required on large NUMA systems because otherwise significant portions
> of system memory can become trapped in pcp queues. The number of pcp is
> determined by the number of processors and nodes in a system. A system
> with 4 processors and 2 nodes has 8 pcps which is okay. But a system
> with 1024 processors and 512 nodes has 512k pcps with a high potential
> for large amount of memory being caught in them."

How about mentioning more details on where does this come from?

afaict it's from commit 4037d45 since 2007.

So I digged that out mostly because I want to know why we did flush pcp at
all during vmstat update.  It already sounds weird to me but I could have
been missing important details.

The rational I had here is refresh_cpu_vm_stats(true) is mostly being
called by the shepherd afaict, while:

  (1) The frequency of that interval is defined as sysctl_stat_interval,
      which has nothing yet to do with pcp pages but only stat at least in
      the name of it, and,

  (2) vmstat_work is only queued if need_update() here:

	for_each_online_cpu(cpu) {
		struct delayed_work *dw = &per_cpu(vmstat_work, cpu);

		if (!delayed_work_pending(dw) && need_update(cpu))
			queue_delayed_work_on(cpu, mm_percpu_wq, dw, 0);

		cond_resched();
	}

      need_update() tells us "we should flush vmstats", nothing it tells
      about "we should flush pcp list"..

I looked into the 2007 commit, besides what Marcelo quoted, I do see
there's a major benefit of reusing cache lines, quotting from the commit:

        Move the node draining so that is is done when the vm statistics
        are updated.  At that point we are already touching all the
        cachelines with the pagesets of a processor.

However I didn't see why it's rational to flush pcp list when vmstat needs
flushing either.  I also don't know whether that "cacheline locality" hold
true or not, because I saw that the pcp page list is split from vmstats
since 2021:

    commit 28f836b6777b6f42dce068a40d83a891deaaca37
    Author: Mel Gorman <mgorman@...hsingularity.net>
    Date:   Mon Jun 28 19:41:38 2021 -0700

    mm/page_alloc: split per cpu page lists and zone stats

I'm not even sure my A-b or R-b worth anything at all here, just offer
something I got from git archaeology so maybe helpful to readers and
reasoning to this patch.  The correctness of archaeology needs help from
others (Christoph and Gel?)..  I would just say if there's anything useful
or correct may worth collect some into the commit log.

So from what I can tell this patch makes sense.

-- 
Peter Xu