From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 93BD5C4345F for ; Thu, 18 Apr 2024 11:00:43 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 27D336B0092; Thu, 18 Apr 2024 07:00:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 22D216B009C; Thu, 18 Apr 2024 07:00:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0CE536B009D; Thu, 18 Apr 2024 07:00:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id E454F6B0092 for ; Thu, 18 Apr 2024 07:00:42 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 910B61A11EE for ; Thu, 18 Apr 2024 11:00:42 +0000 (UTC) X-FDA: 82022359524.18.B96A299 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf26.hostedemail.com (Postfix) with ESMTP id 2D58C140039 for ; Thu, 18 Apr 2024 11:00:39 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Z6ip7dtK; spf=pass (imf26.hostedemail.com: domain of hawk@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=hawk@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713438040; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=e7yDPFBEt+JDNTl3sUwsJeWrLelrFcb5naBx1E3V/n8=; b=Yfa6jeG8UCyc/MYX26s7Y9QjW9iXQhRG9NyE/dyo6DGicyjatKfB/qIpCn68eHZbPxqLrb oCyuIJcR8uCnfR3xMrzEGtogq9wehCwAFL50gpXlL9xZZnUjAe8VLMfkVJsWJxJ6Ws8gsH aJ8UUasiwwaUqYBJDo6Nnn2X+gG/+z8= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713438040; a=rsa-sha256; cv=none; b=UxO5/S7ETepOkXXdR0ezd45Rotc7yNGHcOM181N/6MMm/h0rqPOwX51stb4qhtGXS8GSk0 rsiDrxjc17S2re+z9/1Kv9k01OxxCCuZF0qYoFnMjW6JWEW7JXUgYw+EdZkJQ9PH/aYkgW 2o8U+Ab+LVxkQhEPKsxVTc4qkFT9VTI= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Z6ip7dtK; spf=pass (imf26.hostedemail.com: domain of hawk@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=hawk@kernel.org; dmarc=pass (policy=none) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id A4651CE180E; Thu, 18 Apr 2024 11:00:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 45BAAC32781; Thu, 18 Apr 2024 11:00:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1713438034; bh=4h5v9OO81NjzQxlsyJ2cFIRM5mqx39CLg3+LcKvwFH8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Z6ip7dtK3MN/InqrV8vpt64q9inPO3FDln1IFe6XpUyoi0Uq9ItKx/iDVjvrEDfd6 LBrwNEoqeeWPKyA4NxkspkErOnWCdxcUuGYH+/J/MJijm1z7/bvqnNbPfQUa9koyRT H/fVvLAgcxQUr6pzTUF3augzLjqBBuPPLWRK+9Q657atEjkKrqrUPnfbxpSxKG0r4M r/iNJbiR28UnytvFu1xbZsvKjgIMSpCyPKh6P3SKjZBTHtbmEvDBbWVjpsis9PYrNO jOGOuhRpMHJoQuzGPcaINNFD6+jNlv6hyPHQRzlJYsq3MZVzqYo1kdQ+s1BgFqvVlw IudtuDtr/YBOg== Message-ID: <72e4a55e-a246-4e28-9d2e-d4f1ef5637c2@kernel.org> Date: Thu, 18 Apr 2024 13:00:30 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 3/3] cgroup/rstat: introduce ratelimited rstat flushing To: Yosry Ahmed Cc: tj@kernel.org, hannes@cmpxchg.org, lizefan.x@bytedance.com, cgroups@vger.kernel.org, longman@redhat.com, netdev@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, shakeel.butt@linux.dev, kernel-team@cloudflare.com, Arnaldo Carvalho de Melo , Sebastian Andrzej Siewior , mhocko@kernel.org References: <171328983017.3930751.9484082608778623495.stgit@firesoul> <171328990014.3930751.10674097155895405137.stgit@firesoul> Content-Language: en-US From: Jesper Dangaard Brouer In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Stat-Signature: iidk6hakbu5foqfzhd4gqckc4ppzwxm8 X-Rspamd-Queue-Id: 2D58C140039 X-Rspamd-Server: rspam06 X-Rspam-User: X-HE-Tag: 1713438039-104669 X-HE-Meta: U2FsdGVkX1+mMeHx6PcA7M6iDv7LWzyz0Iu2PnizILkoPiAOff5C1DtqrWhMDaLO7LDDQ/BfrgaevNN0/IC1d/z0nIsnhDCtcvdC45I4dYZFKAj7x0ZHFhZF8PzhOE7GkVqgZbmJmnDgNT9SVdpejTNxqTmE/WLoD57q8jjyAPHEbgnHGXA3x9j8XgaQW6OWx0/ZDSrBtjBZp+FsmAgPBrM+GIZsicTwbvwMlGc/vB1MNXlmvD0iYGKmOi1SwMuWPO8md7RUrtRz72TwjBra84R/Z5hrvYVdEY6DSg+1mfloQ+QD4q7SUSZXoHUvxBZie4S3OMkn6WMkFrAfWmUvjODnMbbiAGXU+MesH7oiUxuRl7whCzWM3Mq7WUAmtCVuLQDGTB1XJ5SUk/QYfK2dQ4Hg+tp496DmVgZYWM7t09UrXo7SVX6Z9uJoV9CDWsfYcaRnOrr5RrdbZ59lR67GjQUfk248Wffh9IEfPa278y/v8gpx/JfCGgsZcIHa9O/LLfkslrkalSkHrkA0CNysFSGn4PkE7Hsmt5ugABUZv0WEIu9wYo80FPxuP8hEn1S3bLagEj9ruyHdXSwb5KtqzKwgwoiD9ZO2XRwpLVAAz0YB5HPYczMBqR0h9tOA9iMHJd7Bc0DYlfwLQH79LlwcJCj9SyxdYQblnUjN5dYKZbClp8Q3cEj+HMqTcdGd5+NOf9wMTgvAV/30Per1n/JldRD8qnM8V8SO5N6IRFchO14yhYAYwKHrnlCyR40ITquSHUjBMbOlhiiOUa6d2IM5CfLz+Id3PNXlfjkZ780AFDZdK2oCjJHG1yAJ/EOOcMkOUwvZfp7h7hrZ2DyeeN/XqKAVtrPjPmqRK4LI8GKNJf5yL7z+ZK0Q638Cc8c4johsm2AP/inRbj0+88XElrk+zGGVqGs80dEbi7CQdk2neIrNVIjYSclHVQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000033, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 18/04/2024 04.21, Yosry Ahmed wrote: > On Tue, Apr 16, 2024 at 10:51 AM Jesper Dangaard Brouer wrote: >> >> This patch aims to reduce userspace-triggered pressure on the global >> cgroup_rstat_lock by introducing a mechanism to limit how often reading >> stat files causes cgroup rstat flushing. >> >> In the memory cgroup subsystem, memcg_vmstats_needs_flush() combined with >> mem_cgroup_flush_stats_ratelimited() already limits pressure on the >> global lock (cgroup_rstat_lock). As a result, reading memory-related stat >> files (such as memory.stat, memory.numa_stat, zswap.current) is already >> a less userspace-triggerable issue. >> >> However, other userspace users of cgroup_rstat_flush(), such as when >> reading io.stat (blk-cgroup.c) and cpu.stat, lack a similar system to >> limit pressure on the global lock. Furthermore, userspace can easily >> trigger this issue by reading those stat files. >> >> Typically, normal userspace stats tools (e.g., cadvisor, nomad, systemd) >> spawn threads that read io.stat, cpu.stat, and memory.stat (even from the >> same cgroup) without realizing that on the kernel side, they share the >> same global lock. This limitation also helps prevent malicious userspace >> applications from harming the kernel by reading these stat files in a >> tight loop. >> >> To address this, the patch introduces cgroup_rstat_flush_ratelimited(), >> similar to memcg's mem_cgroup_flush_stats_ratelimited(). >> >> Flushing occurs per cgroup (even though the lock remains global) a >> variable named rstat_flush_last_time is introduced to track when a given >> cgroup was last flushed. This variable, which contains the jiffies of the >> flush, shares properties and a cache line with rstat_flush_next and is >> updated simultaneously. >> >> For cpu.stat, we need to acquire the lock (via cgroup_rstat_flush_hold) >> because other data is read under the lock, but we skip the expensive >> flushing if it occurred recently. >> >> Regarding io.stat, there is an opportunity outside the lock to skip the >> flush, but inside the lock, we must recheck to handle races. >> >> Signed-off-by: Jesper Dangaard Brouer > > As I mentioned in another thread, I really don't like time-based > rate-limiting [1]. Would it be possible to generalize the > magnitude-based rate-limiting instead? Have something like > memcg_vmstats_needs_flush() in the core rstat code? > I've taken a closer look at memcg_vmstats_needs_flush(). And I'm concerned about overhead maintaining the stats (that is used as a filter). static bool memcg_vmstats_needs_flush(struct memcg_vmstats *vmstats) { return atomic64_read(&vmstats->stats_updates) > MEMCG_CHARGE_BATCH * num_online_cpus(); } I looked at `vmstats->stats_updates` to see how often this is getting updated. It is updated in memcg_rstat_updated(), but it gets inlined into a number function (__mod_memcg_state, __mod_memcg_lruvec_state, __count_memcg_events), plus it calls cgroup_rstat_updated(). Counting invocations per sec (via funccount): 10:28:09 FUNC COUNT __mod_memcg_state 377553 __count_memcg_events 393078 __mod_memcg_lruvec_state 1229673 cgroup_rstat_updated 2632389 I'm surprised to see how many time per sec this is getting invoked. Originating from memcg_rstat_updated() = 2,000,304 times per sec. (On a 128 CPU core machine with 39% idle CPU-load.) Maintaining these stats seems excessive... Then how often does the filter lower pressure on lock: MEMCG_CHARGE_BATCH(64) * 128 CPU = 8192 2000304/(64*128) = 244 time per sec (every ~4ms) (assuming memcg_rstat_updated val=1) > Also, why do we keep the memcg time rate-limiting with this patch? Is > it because we use a much larger window there (2s)? Having two layers > of time-based rate-limiting is not ideal imo. > I'm also not-a-fan of having two layer of time-based rate-limiting, but they do operate a different time scales *and* are not active at the same time with current patch, if you noticed the details, then I excluded memcg from using this as I commented "memcg have own ratelimit layer" (in do_flush_stats). I would prefer removing the memcg time rate-limiting and use this more granular 50ms (20 timer/sec) for memcg also. And I was planning to do that in a followup patchset. The 50ms (20 timer/sec) limit will be per cgroup in the system, which then "scales"/increase with the number of cgroups, but better than unbounded read/access locks per sec. --Jesper > [1]https://lore.kernel.org/lkml/CAJD7tkYnSRwJTpXxSnGgo-i3-OdD7cdT-e3_S_yf7dSknPoRKw@mail.gmail.com/ sudo ./bcc/tools/funccount -Ti 1 -d 10 '__mod_memcg_state|__mod_memcg_lruvec_state|__count_memcg_events|cgroup_rstat_updated'