On Wed, Feb 21, 2018 at 6:06 PM, Minchan Kim [off-list ref] wrote:

On Wed, Feb 21, 2018 at 04:23:43PM -0800, Daniel Colascione wrote:

quoted

Thanks for taking a look.

On Wed, Feb 21, 2018 at 4:16 PM, Minchan Kim [off-list ref] wrote:

quoted

Hi Daniel,

On Wed, Feb 21, 2018 at 11:05:04AM -0800, Daniel Colascione wrote:

quoted

On Mon, Feb 5, 2018 at 2:03 PM, Daniel Colascione <dancol@google.com

quoted

wrote:

quoted

quoted

When SPLIT_RSS_COUNTING is in use (which it is on SMP systems,
generally speaking), we buffer certain changes to mm-wide counters
through counters local to the current struct task, flushing them to
the mm after seeing 64 page faults, as well as on task exit and
exec. This scheme can leave a large amount of memory

unaccounted-for

quoted

quoted

quoted

quoted

in process memory counters, especially for processes with many

threads

quoted

quoted

quoted

quoted

(each of which gets 64 "free" faults), and it produces an
inconsistency with the same memory counters scanned VMA-by-VMA

using

quoted

quoted

quoted

quoted

smaps. This inconsistency can persist for an arbitrarily long time,
since there is no way to force a task to flush its counters to its

mm.

quoted

quoted

Nice catch. Incosistency is bad but we usually have done it for
performance.
So, FWIW, it would be much better to describe what you are suffering

from

quoted

quoted

for matainter to take it.

The problem is that the per-process counters in /proc/pid/status lag

behind

quoted

the actual memory allocations, leading to an inaccurate view of overall
memory consumed by each process.

Yub, true. The key of question was why you need a such accurate count.

For more context: on Android, we've historically scanned each processes's
address space using /proc/pid/smaps (and /proc/pid/smaps_rollup more
recently) to extract memory management statistics. We're looking at
replacing this mechanism with the new /proc/pid/status per-memory-type
(e.g., anonymous, file-backed) counters so that we can be even more
efficient, but we'd like the counts we collect to be accurate.

Don't get me wrong. I'm not saying we don't need it.
I was just curious why it becomes important now because we have been with
such inaccurate count for a decade.

quoted

quoted

quoted

quoted

This patch flushes counters on context switch. This way, we bound

the

quoted

quoted

quoted

quoted

amount of unaccounted memory without forcing tasks to flush to the
mm-wide counters on each minor page fault. The flush operation

should

quoted

quoted

quoted

quoted

be cheap: we only have a few counters, adjacent in struct task,

and we

quoted

quoted

quoted

quoted

don't atomically write to the mm counters unless we've changed
something since the last flush.

Signed-off-by: Daniel Colascione <redacted>
---
 kernel/sched/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index a7bf32aabfda..7f197a7698ee 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c

@@ -3429,6 +3429,9 @@ asmlinkage __visible void __sched

schedule(void)

quoted

quoted

quoted

quoted

        struct task_struct *tsk = current;

        sched_submit_work(tsk);
+       if (tsk->mm)
+               sync_mm_rss(tsk->mm);
+
        do {
                preempt_disable();
                __schedule(false);

Ping? Is this approach just a bad idea? We could instead just

manually

quoted

quoted

sync

quoted

all mm-attached tasks at counter-retrieval time.

IMHO, yes, it should be done when user want to see which would be

really

quoted

quoted

cold path while this shecule function is hot.

The problem with doing it that way is that we need to look at each task
attached to a particular mm. AFAIK (and please tell me if I'm wrong), the
only way to do that is to iterate over all processes, and for each

process

quoted

attached to the mm we want, iterate over all its tasks (since each one

has

quoted

to have the same mm, I think). Does that sound right?

Hmm, it seems you're right. I spent some time to think over but cannot
reach
a better idea. One of option was to change RSS_EVENT_THRESH to per-mm and
control it dynamically with the count of mm_users when forking time.
However, it makes the process with many thread harmful without reason.

So, I support your idea at this moment. But let's hear other's opinions.

FWIW, I just sent a patch that does the same thing a different way. It has
the virtue of not increasing context-switch path length, but it adds a
spinlock (almost never contended) around the per-task mm counter struct.
I'd be happy with either this version or my previous version.