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, 24 Oct 2022 23:14:08 +0000
In-Reply-To: <20221024153022.1b81a182eac44797b40bfda1@linux-foundation.org>
Mime-Version: 1.0
References: <20221024052841.3291983-1-shakeelb@google.com> <20221024153022.1b81a182eac44797b40bfda1@linux-foundation.org>
Message-ID: <20221024231408.j3moql6q5qhct3jx@google.com>
Subject: Re: [PATCH] mm: convert mm's rss stats into percpu_counter
From:   Shakeel Butt <shakeelb@google.com>
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Mon, Oct 24, 2022 at 03:30:22PM -0700, Andrew Morton wrote:
> On Mon, 24 Oct 2022 05:28:41 +0000 Shakeel Butt <shakeelb@google.com> wrote:
> 
> > Currently mm_struct maintains rss_stats which are updated on page fault
> > and the unmapping codepaths. For page fault codepath the updates are
> > cached per thread with the batch of TASK_RSS_EVENTS_THRESH which is 64.
> > The reason for caching is performance for multithreaded applications
> > otherwise the rss_stats updates may become hotspot for such
> > applications.
> > 
> > However this optimization comes with the cost of error margin in the rss
> > stats. The rss_stats for applications with large number of threads can
> > be very skewed. At worst the error margin is (nr_threads * 64) and we
> > have a lot of applications with 100s of threads, so the error margin can
> > be very high. Internally we had to reduce TASK_RSS_EVENTS_THRESH to 32.
> > 
> > Recently we started seeing the unbounded errors for rss_stats for
> > specific applications which use TCP rx0cp. It seems like
> > vm_insert_pages() codepath does not sync rss_stats at all.
> > 
> > This patch converts the rss_stats into percpu_counter to convert the
> > error margin from (nr_threads * 64) to approximately (nr_cpus ^ 2).
> 
> Confused.  The max error should be O(nr_cpus)?
> 

So, percpu_counter code sets the percpu batch in the following way:

static int compute_batch_value(unsigned int cpu)
{
        int nr = num_online_cpus();

        percpu_counter_batch = max(32, nr*2);
        return 0;
}

This means each cpu can cache (nr_cpus*2) updates. Practically the
number of cpus do not change and are usually much less than the number
of threads of large applications, so error margin is lower.

> > However this conversion enable us to get the accurate stats for
> > situations where accuracy is more important than the cpu cost. Though
> > this patch does not make such tradeoffs.
> 
> Curiousity.  Can you expand on the final sentence here?
> 

Basically we can just use percpu_counter_add_local() for the updates and
percpu_counter_sum() (or percpu_counter_sync() + percpu_counter_read)
for the readers. At the moment the readers are either procfs interface,
oom_killer and memory reclaim which I think are not performance critical
and should be ok with slow read. However I think we can make that change
in a separate patch.

thanks,
Shakeel