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 3E380C3DA4A for ; Fri, 2 Aug 2024 16:11:34 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AB0786B008A; Fri, 2 Aug 2024 12:11:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A60316B0092; Fri, 2 Aug 2024 12:11:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 94EA36B0095; Fri, 2 Aug 2024 12:11:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 77EFB6B008A for ; Fri, 2 Aug 2024 12:11:33 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 26A5D141313 for ; Fri, 2 Aug 2024 16:11:33 +0000 (UTC) X-FDA: 82407795666.23.282A922 Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) by imf06.hostedemail.com (Postfix) with ESMTP id 45118180026 for ; Fri, 2 Aug 2024 16:11:31 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=Kq33NaQ4; spf=pass (imf06.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.46 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722615033; 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=aq+V7tIZ4SVhLEkffkvPe2h7pXCT1loXoSAK6ImEdLQ=; b=kXmstZgnh8AGmbq5QxTHA/bIlhjzV+oZ/qo/4Nqj9uHVJHuAt/VO01vX49s1PKaJKaGFmd EgylM/FiC7jsgRGvxaDSY/EmEyaYKlSNjGeOLlzVouJXFlpkaPfUwn8EkMXkdvjxKwgFec EjJLBeyhcmmF2amGLp26voQ5cvZ5ImM= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722615033; a=rsa-sha256; cv=none; b=oszEw6IWhpRmYEekHmGHY22JPgSztvt6cPQQRfj49nybJTK/H2I9YKv1A39q0ZMQl/XNPW L3LQVG2ysio5Fum8RHDTk1MwFDipC9wLdWPRs/dCtQE/UhWA82xcGmW7t7Fh244bxCvJc6 /DhoGObjgQWHnUtF6hSqXobJG6Vrj0E= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=Kq33NaQ4; spf=pass (imf06.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.46 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-ej1-f46.google.com with SMTP id a640c23a62f3a-a7aa4ca9d72so1030207566b.0 for ; Fri, 02 Aug 2024 09:11:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1722615090; x=1723219890; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=aq+V7tIZ4SVhLEkffkvPe2h7pXCT1loXoSAK6ImEdLQ=; b=Kq33NaQ4DI+8mZHklO4m8GZEwdghmmsXJXWprLR1lCof2Avp3+idqdWOKVQ5jmSc0/ LKZW7O3tIZQbuRQEq6/XBggQj1HzbfMq7h8nnq2iHS1Y1tknA8Z90snVAIv0Fuh5gh+S i26Q2AH06P3em/so5Yt8CSF0fCmgOqB4UkGjkBS9D9MG8C1QsLkoZlRrP5cOuwpc1AYH 7CMb50zBuniJ3iWkx/28RE/4vXHcEnaH3nBu4UTqYLOZJNcJtBk3qeURY4TqY+0tlf6W Wp5EavXAHtcR6cdgxeQ2A7TqbSOxz16Rb4/PtNdSu6Wv09nmY52OJqpx9fRgiLqO1n/7 EqOQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722615090; x=1723219890; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=aq+V7tIZ4SVhLEkffkvPe2h7pXCT1loXoSAK6ImEdLQ=; b=gcgOoKxp0UgQa45G5TzG5mzVFuRpW6LPqj6JGl50YfVcFtB/uWEKlJtO5HFbC/NKgC rDjVSktA6tTiYWPf50zUe5w3oBAQV8dq9eMJXUAPscUCvrj4pcQlx4L55wOHd/rVgQOY d66k8HYqbs6GeATbFXmfVtuZb+WwwDXadwGqOeK4/UI4MmPzPMF9BIK8Hi6aIuM5Cun9 9zETXwxQ9hh4xxjHEVYWibpXrD2ZpdQd3mcYYGs69gwu0WxOUVMfyJS5ZaEb6xtMusD0 i5KuSoGVvqzB2iq4WYoV67tJJXOBhLHtG1yQ//xizvpP7PYvu1obVqMwCbFNfWC4SYmf Ml4g== X-Forwarded-Encrypted: i=1; AJvYcCVjMw86T2u2Lgk9d7PnMNL2fR+WPmCD3xGWFYPSZQdksCSKdstK8/DCe1opNvQfweH8xdSABnE6z/S6AUv7+6OQvY0= X-Gm-Message-State: AOJu0YwKpx0EzlLkaBsiyMz4T8KikuaQPFXGqg8EV5sl9z956IVoUxdg QW+u1q+RTKFIU2hnQeuRB3QFr4JL8P3SuG7AtUNTN1qpXSM48dBOeSTM+X68vCwMoYOXXroQXYn G0ViqjVK9tpciW0QBHT1glcImplWEHqMznt2M X-Google-Smtp-Source: AGHT+IHn1JKs7M+6B7ipAt+vMTtY97ynjMO8RepS7WO1SGKTKodx/tcEcGxw5ix4N3R93xXO6kJsj0HWYTe6g4sOnn0= X-Received: by 2002:a17:907:1c19:b0:a7a:8876:4429 with SMTP id a640c23a62f3a-a7dc506f5aamr352379666b.45.1722615089045; Fri, 02 Aug 2024 09:11:29 -0700 (PDT) MIME-Version: 1.0 References: <172139415725.3084888.13770938453137383953.stgit@firesoul> In-Reply-To: From: Yosry Ahmed Date: Fri, 2 Aug 2024 09:10:51 -0700 Message-ID: Subject: Re: [PATCH V8 1/2] cgroup/rstat: Avoid flushing if there is an ongoing overlapping flush To: Jesper Dangaard Brouer Cc: tj@kernel.org, cgroups@vger.kernel.org, shakeel.butt@linux.dev, hannes@cmpxchg.org, lizefan.x@bytedance.com, longman@redhat.com, kernel-team@cloudflare.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 45118180026 X-Stat-Signature: ikx5e36r3cycwibyuoj6y6me4xnz91ts X-HE-Tag: 1722615091-222533 X-HE-Meta: U2FsdGVkX1/0YpjtPL+tzy+4a69evnbxqwq4sfTrbG4uH81BCALG+qe+86DTdiGAGheffTKjm0CI/TNMgs8PgWQPk7FSTEVFcYyotX2jgPJhsqgLtsKp3maOhmrK0VUPEqj8bKyGPpbxlPWG1a4Gd87uosfRSUjczvVSIj4clUxeOIL81ve7vs/TDe8FguHnHXGFw1sJoMpwhObqSphkx3AyCIJMH98KUGuX8dJ0fMEHLf3LlSFDaUvKBb43aduFUMgFcIsHwvlyv9jzJ12zJQlxm1MaaHRCrWolqMCp12DeTKZxYXUMpdqsHc7nksiXJlsMFWPLah+rAvrXYaflUmKWGrPrMATG82z41VSqWaFfGk/h6KiRBzyU1C05hhWvq4T5QO0WX32wzwtxsOuEqu6GAq5zdCN4U6j5yzl6SriSY36mLrJiZeFtHhTsCp1g/jzwP6HSKmtmZyKrp8yWC6fcUv6S4IRG1vieBkple8JdgGOjmFS/skCZ5nYbsX+DPRo17YOVD86Pgw0BtBhM1bhyEftbKBAZsoHyFAaCzfaSL/pReRmGCAaR7uvGUD3gT3rX4gsRyxyAvN3nUcWmfRHVJnPQ1js3oRFhuM8tceDE8vkWOAERJJVEhw83t3ATjUEDRvOSlYn4BcJtI13AjGEoxkcvfvRus2mE7+7SLGqgehObTqcwY/YYPxfwrv7bLiN2vL6N/aGAeqpR/0IfIVvrebSgvw4T8H1PH6riRo6PsMPggQoiodjjZ2lEyElwmIHB0BAO+ldbZ32+0ZVm3TwbKOhKm+xMPcRlV7Dv+d9ELnZ9y1Zm1sbvYa8sSNQWA7m47aHKHCWcJ1Io/5GEOvFRo+ZpOT9FKxjn/YmxRaMNEtFmCM3hpku8p7RQxRa5sPH+8qGLWRC5mz1lRPu2hVOnT8nV0uhluDMtv8jGadcvmMlIOEpATS1N2BkKwSm4w7YDrba+QuN/Z+K4DnY ZtDglXvL Qz6vs0uMG7+XzKTUz3JrL128RQA3Yeo6QsSQOkaU4YSQlxkIWCBUgIjB384FOStWsrXraZ0+dW1oO0fJ2KSUEn31Zr7sO+Mf8B6Ccz7tnr8STH1a0nnmhTwb4Zb34VHc3mKgszSgm8dYXbKui509me/4o7dDhX3+GBEVugqRo9mtuK2KNXNc885luHYix8lwXyH6YUIl+RWj/jPQhiney2/Vxgf0H5TvAwx7Ecu4K1oXWVHc= X-Bogosity: Ham, tests=bogofilter, spamicity=0.360183, 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 Fri, Aug 2, 2024 at 4:43=E2=80=AFAM Jesper Dangaard Brouer wrote: > > > > On 30/07/2024 20.54, Yosry Ahmed wrote: > > [..] > >> > >> Well... I'm still not convinced that it makes sense to have level >=3D= 2 > >> be the ongoing flusher. > >> > >> E.g. if a level 2 cgroup becomes ongoing flusher, and kswapd starts 12 > >> NUMA flushes at the same time, then the code will have these 12 kswapd > >> threads spin on the lock, until ongoing flusher finishes. That is like= ly > >> what happened above (for a level 1). These 12 spinning (root) flusher= s > >> will not recheck ongoing_flusher and will all flush the root > >> (unnecessarily 11 times). > > > > Hmm regardless of whether or not the level-2 cgroup becomes the > > ongoing flusher, the kswapd threads will all spin on the lock anyway > > since none of them can be the ongoing flusher until the level-2 cgroup > > finishes. Right? > > > > Is the scenario you have in mind that the level-2 cgroup starts > > flushing at the same time as kswapd, so there is a race on who gets to > > be the ongoing flusher? In this case as well, whoever gets the lock > > will be the ongoing flusher anyway. > > > > Not allowing whoever is holding the lock to be the ongoing flusher > > based on level is only useful when we can have multiple ongoing > > flushers (with lock yielding). Right? > > > > Perhaps I am missing something here. > > > >> > >> So, I don't think it is a good idea to have anything else that the roo= t > >> as the ongoing flusher. > >> > >> Can you explain/convince me why having sub-cgroups as ongoing flusher = is > >> an advantage? > > > > I just don't see the benefit of the special casing here as I mentioned > > above. If I missed something please let me know. > > > > I do think you missed something. Let me try to explain this in another > way. (I hope my frustrations doesn't shine through). > > The main purpose of the patch is/was to stop the thundering herd of > kswapd thread flushing (root-cgrp) at exactly the same time, leading to > lock contention. This happens all-the-time/constantly in production. > > The first versions (where ongoing was limited to root/level=3D0) solved > this 100%. The patches that generalized this to be all levels can > become ongoing flush, doesn't solve the problem any-longer! > > I hope it is clear what fails. E.g. When a level>0 becomes ongoing > flusher, and 12 kswapd simultaneously does a level=3D0/root-cgrp flush, > then we have 12 CPU cores spinning on the rstat lock. (These 12 kswapd > threads will all go-through completing the flush, as they do not > discover/recheck that ongoing flush was previously became their own level= ). I think we may be speaking past one another, let me try to clarify :) I agree with your assessment, all I am saying is that this restriction is only needed because of lock yielding, and can be removed after that IIUC. The problem when we allow non-root ongoing flushers now is that when the kswapd thread are woken up and the first one of them gets the lock and does the flush, it may be find that the ongoing_flusher is already set by another non-root flusher that yielded the lock. In this case, the following kswapd flushers will spin on the lock instead of waiting for the first kswapd to finish. If we remove lock yielding, then the above scenario cannot happen. When the lock/mutex is held by a flusher, it is guaranteed that ongoing_flusher is NULL and can be set by the flusher. In this case, we should allow any cgroup to be the ongoing_flusher because there can only be one anyway. Does this make sense to you? > > I think we need to go back to only having root-cgroup as ongoing flusher. > > --Jesper