[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b4zbdp4wq35nvq34njzdscqj72nk6e5vmy63n4csakcxnonpc5@kqvtacyg6n4i>
Date: Mon, 19 Jun 2023 14:53:13 +0200
From: Michal Koutný <mkoutny@...e.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Christian Brauner <brauner@...nel.org>,
Suren Baghdasaryan <surenb@...gle.com>,
"Liam R . Howlett" <Liam.Howlett@...cle.com>,
Mike Christie <michael.christie@...cle.com>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Peter Zijlstra <peterz@...radead.org>,
Nicholas Piggin <npiggin@...il.com>,
Andrei Vagin <avagin@...il.com>,
Shakeel Butt <shakeelb@...gle.com>,
Adam Majer <amajer@...e.com>, Jan Kara <jack@...e.cz>,
Michal Hocko <mhocko@...nel.org>
Subject: Re: [PATCH] mm: Sync percpu mm RSS counters before querying
On Fri, Jun 16, 2023 at 12:31:44PM -0700, Andrew Morton <akpm@...ux-foundation.org> wrote:
> On Fri, 16 Jun 2023 20:07:18 +0200 Michal Koutný <mkoutny@...e.com> wrote:
>
> > An issue was observed with stats collected in struct rusage on ppc64le
> > with 64kB pages. The percpu counters use batching with
> > percpu_counter_batch = max(32, nr*2) # in PAGE_SIZE
> > i.e. with larger pages but similar RSS consumption (bytes), there'll be
> > less flushes and error more noticeable.
>
> A fully detailed description of the issue would be helpful. Obviously
> "inaccuracy", but how bad?
Any reader of get_mm_counter() could see the inaccuracy given by the
formula.
In this particular case it is detected by a testsuite of time(1) utility
that feeds from rusage:
> FAIL: tests/time-max-rss
> ========================
>
> time(1) failed to detect 5MB allcoation.
> mem-baseline(kb): 0
> mem-5MB(kb): 4096
> delta(kb): 4096
> FAIL tests/time-max-rss.sh (exit status: 1)
(i.e. 1MB missing)
> Far too large to be inlined! For six callsites it adds 1kb of text.
Ah, thanks, I can change that.
> Why even modify the counter? Can't <whatever this issue is> be
> addressed by using percpu_counter_sum() in an appropriate place?
I considered modifying get_mm_counter(), however, I decided not to put
the per-cpu summing there as it'd incur the impact to many more places
than sync_mm_rss().
> For unknown reasons percpu_counter_set() uses for_each_possible_cpu().
> Probably just a mistake - percpu_counters are hotplug-aware and
> for_each_online_cpu should suffice.
Yeah, that could be cleaned up in another patch (cf mask in
__percpu_counter_sum).
> I'm really not liking percpu_counter_set(). It's only safe in
> situations where the caller knows that no other CPU can be modifying
> the counter. I wonder if all the callers know that.
I admit I only considered the do_exit() path (and even that isn't
granted in a multithreaded process) -- so I don't like
percpu_counter_set() in this current form neither.
I will need to review effects of parallel updates more.
Thanks,
Michal
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists