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 571D0C6FD1D for ; Thu, 30 Mar 2023 04:27:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 776B26B0072; Thu, 30 Mar 2023 00:27:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6FFCB6B0074; Thu, 30 Mar 2023 00:27:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 551EC6B0075; Thu, 30 Mar 2023 00:27:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 3EB916B0072 for ; Thu, 30 Mar 2023 00:27:05 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 0B1891C645A for ; Thu, 30 Mar 2023 04:27:05 +0000 (UTC) X-FDA: 80624279610.21.10F2D12 Received: from mail-ed1-f47.google.com (mail-ed1-f47.google.com [209.85.208.47]) by imf17.hostedemail.com (Postfix) with ESMTP id 345EA40003 for ; Thu, 30 Mar 2023 04:27:02 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=UB9ilX9x; spf=pass (imf17.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.47 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=1680150423; 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=mPxXZwrGY9EQBVrz+rM6XeJQecrNiNwMGKoahuxks9c=; b=75nacWx/R6uKUqf96FvVkZj+S7/wUSsO8l36YFBJUUYLgi0YXEh4JMG97inH/n9rHScq5y IHHGh++5Mlq3WKe0/4Eyse8DDDyO7hawGZKtwxkkRzhDfXBUF41IZ05bdshbBWrU5bghbj xJnefT/on5dNiNqBFKgzFzbLT4b8+UY= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=UB9ilX9x; spf=pass (imf17.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.47 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680150423; a=rsa-sha256; cv=none; b=6CtAPCPdOUJ2kvG24bYgPDPwXT+WtiQSYCriMCibdA9QIx7gQZwToYhoXohSTbFJm8bElT Oqsm7AfG9tV1ezowbrkCvjC7vhXZth219/eVfbBVWfceXmYILdyFwEPUWzlBx3p/x8e+eV L5L3KKDIne656Gn9GwakbT6axB6/nbY= Received: by mail-ed1-f47.google.com with SMTP id h8so71562661ede.8 for ; Wed, 29 Mar 2023 21:27:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680150421; 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=mPxXZwrGY9EQBVrz+rM6XeJQecrNiNwMGKoahuxks9c=; b=UB9ilX9x0DFClvH2MBsafXYctyfQiMtqneVbDgIn8dlkb7SeJEzllIUHniTmROr9zA QNEcN1WFlEJGxgMboHiWeAft9d3KIZB6LTea9dtS1hOwT9F0hILhYXmS0E6kIWINYTZy vBEL04m8KhM3aacYuUlP7egfig+eopFO62We1ZE3+r978w7tMG7VVUF5nAvktNbztfWy DPIUhgo3yBtBMUXACd+J8qLZYHzhxPnNgyVg366YOY7pdoxSIli5kTx+TEp3cf9hhDzv kmAe2yK9UIVG5k/8muUa7zOO6grvclhgaCpWWvyR2kWnYuNTZ48TJfXwnL+iNuR2zeoS O3lA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680150421; 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=mPxXZwrGY9EQBVrz+rM6XeJQecrNiNwMGKoahuxks9c=; b=ryNI62zsXII4wtgkyq3avEuGz0h6dz7YGJ+WzT4CRTtcgmXbhoMtn2HJFtUHq8evWz FgtKe6W3Uv4p+gqBGRjixigPavHpauI9AfprWzYDEB4La3I33cbWLBf9KwMTCbfePuu/ YY40LO1pbYd0l3KV1PxG2Vb/faBdjn4O+jTWAm5NiXu8Z0I95HWOUn6tZ+bWQKciLSUa QyNWbyiBPa8IhNIW4ayZKdSgf0iMlNLiFtt8c5fNMFSqbyMMMAiqY76H9qeSh15/87AQ zMt1eHNXnvZa62pm589a2lNs8BltTO7bLod1hmcNb3ryi0ZZx/gcJAbxE5D3q17x5Tdi 54FA== X-Gm-Message-State: AAQBX9dM8GvtU94oNN48m1ojZY8q+rsKWetNDKcJ7AIifN8EWkLf3Nll UUD212w2UEnVjHw0tpckZDBlYUFILUujN7Rhig821w== X-Google-Smtp-Source: AKy350Y3jW8mXAik9Znw5lXXT0a3LkoBTafwWbTIHw4pWyMN7nAmXuXsz/YasuYog2yKCeL23uzXcExl0Qv0CZmiwXM= X-Received: by 2002:a50:d756:0:b0:4fc:e5c:902 with SMTP id i22-20020a50d756000000b004fc0e5c0902mr10775755edj.8.1680150421386; Wed, 29 Mar 2023 21:27:01 -0700 (PDT) MIME-Version: 1.0 References: <98cb3ce-7ed9-3d17-9015-ef7193d6627@google.com> In-Reply-To: <98cb3ce-7ed9-3d17-9015-ef7193d6627@google.com> From: Yosry Ahmed Date: Wed, 29 Mar 2023 21:26:24 -0700 Message-ID: Subject: Re: [RFC PATCH 1/7] cgroup: rstat: only disable interrupts for the percpu lock To: Hugh Dickins Cc: Tejun Heo , Shakeel Butt , Josef Bacik , Jens Axboe , Zefan Li , Johannes Weiner , Michal Hocko , Roman Gushchin , Muchun Song , Andrew Morton , Vasily Averin , cgroups@vger.kernel.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, bpf@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: soxjw7z8fpjds1u1acm6yftjxt4wckyn X-Rspam-User: X-Rspamd-Queue-Id: 345EA40003 X-Rspamd-Server: rspam06 X-HE-Tag: 1680150422-501718 X-HE-Meta: U2FsdGVkX1+eEl+hK41qvVuFfXvL6tXvPPImDezjfrW9ESl/a6bBYRZWr6lArv/YTGTCPeA8I29lGwfwWIPWUYkHKFv55DnAg7pbWdwo4xmf3ugTJyhF+N7117N4TJdkKRtX9MJK9X8VA9xhyp4F7o5+pQf5x2Mk+5Z6wRi0SIyYjLzh+d5Bjti+5st+uMWB7Pns0ZzIo321sfEAfBLttX/rwUiPffCu8DMaSXikbl3UUmVzqReCgvUMZdMghhBlsqawVUjiS71teXKcBXRJREmFtzofUo79Lvo3S7r3Y12J/3GYojAqSIT1kDO83SqplqxFtwK8iWoz3qP3JXRbgmYZwymebKTkJ4dzMZMtnBw1vHvupiePfg4h6yY+AC2d5hPdLyxfIRFTucV1Ogxkqi98qglgFSVy39zGPI3bVOBaBCb3jIbGgkfWfai+/uxrEulbBCavQlkoIWy4XiyQg6HCRiDnV3dyOY+IA7RhtZM1ylOS/Im84LYxmQXlMRxmzyAA1u20GeyvNDT38K1Fbu2itaahsgvidbC+F260dPFrFKCvj7ys1iKZD2ah6XYL4jDJPEtzQbhod3r18pNAOH4gl78gX/CSfJYpobv02UEtR5DIv34ZohKEUgXAmUs6XIB7ig293FUtIsAyS0wnctFAOH4xupgG1AddsH85gS8huPMgTkh6fmaQreCVh4y/yJarz8Cz/CRJ8NQVMyNd/K13Z3AxcEJkrrJ7CFqxcH93o0wBcnfWHT9s2xfgA3f9Ic6a3gJ2FlLTCeMHcorXrxzWyy/Q6qdORNsnxzCc1cbnxzUNezvv5+s8svsNcRzdOb8Tda2T147NPp+4wLSkGFp/WxD5D9eJ8arXYnceHhcap8l6QFk9t5Ax0NZ4SHTmUnTzf/N6FNrcgI/QTN2Z4Mpz/IiHPAtODqNwM7MlU9/9CglZntRzOnAi8YMHa/PFDPyr+AAqzHQFzNVIbLO ZZjoE02P chPOJIuafUbaK/HxO2BOVnrIrbEKdFZSNAhaSiAarRKzsQRtny7GhfijoBcZYwYM48cACCq1ep0cLu7+bWjsoARe5m/ks/pJHaVuVCFA+Kifi5uCL/7rsHqRARTPjNznr7Q9ZSuvY6Lvqjo8QILqnqmc6MOwaXqnBQmt/7oLjSJmEvaxnsIxXFYymN6k3YHnKwrjAeIPu1vL9ylSDFdGENUPAxbpyIfIklC1dfDhVI7qHSYupkO314pILiXWkH93ALlTL2JNtmbUHpgf9Ux41iz+ENDIRwMVMvHI02ElER8oJbdSypG2j7doualSLUJj7+O3d1adi1bZHDKc= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000013, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Thanks for a great discussion, Tejun and Hugh. On Wed, Mar 29, 2023 at 1:38=E2=80=AFPM Hugh Dickins wro= te: > > On Wed, 29 Mar 2023, Tejun Heo wrote: > > > Hello, Hugh. How have you been? > > > > On Wed, Mar 29, 2023 at 12:22:24PM -0700, Hugh Dickins wrote: > > > Hi Tejun, > > > Butting in here, I'm fascinated. This is certainly not my area, I kn= ow > > > nothing about rstat, but this is the first time I ever heard someone > > > arguing for more disabling of interrupts rather than less. > > > > > > An interrupt coming in while holding a contended resource can certain= ly > > > add to latencies, that I accept of course. But until now, I thought = it > > > was agreed best practice to disable irqs only regretfully, when stric= tly > > > necessary. > > > > > > If that has changed, I for one want to know about it. How should we > > > now judge which spinlocks should disable interrupts and which should = not? > > > Page table locks are currently my main interest - should those be cha= nged? > > > > For rstat, it's a simple case because the global lock here wraps around > > per-cpu locks which have to be irq-safe, so the only difference we get > > between making the global irq-unsafe and keeping it so but releasing > > inbetween is: > > > > Global lock held: G > > IRQ disabled: I > > Percpu lock held: P > > > > 1. IRQ unsafe > > > > GGGGGGGGGGGGGGG~~GGGGG > > IIII IIII IIII ~~ IIII > > PPPP PPPP PPPP ~~ PPPP > > > > 2. IRQ safe released inbetween cpus > > > > GGGG GGGG GGGG ~~ GGGG > > IIII IIII IIII ~~ IIII > > PPPP PPPP PPPP ~~ PPPP > > > > #2 seems like the obvious thing to do here given how the lock is used a= nd > > each P section may take a bit of time. > > Many thanks for the detailed response. I'll leave it to the rstat folks, > to agree or disagree with your analysis there. Thanks for the analysis, Tejun, it does indeed make sense. I perf'd releasing and reacquiring the lock at each CPU boundary and the overhead seems to be minimal. It would be higher with contention, but all memcg flushers should be held back by the memcg code, and flushers outside memcg are not frequent (reading blkcg and cpu base stats from user space, and when a cgroup is being removed). I realized that after v2 of this patch series [1], we would only end up with two atomic flushing contexts, mem_cgroup_wb_stats() and mem_cgroup_usage(). The latter is already disabling irqs for other reasons, so anything we do within the rstat core code doesn't really help, it needs to be addressed separately. So only the call site in mem_cgroup_wb_stats() would benefit from not having irqs disabled throughout the flush. I will hold off on sending a patch until I observe that this call site is causing us pain and/or other atomic call sites emerge (or we have to revert one of the ones we made non-atomic), so that we don't hurt other flushers unnecessarily. Does this make sense to you? [1] https://lore.kernel.org/linux-mm/20230328221644.803272-1-yosryahmed@goo= gle.com/ > > > > > So, in the rstat case, the choice is, at least to me, obvious, but even= for > > more generic cases where the bulk of actual work isn't done w/ irq disa= bled, > > I don't think the picture is as simple as "use the least protected vari= ant > > possible" anymore because the underlying hardware changed. > > > > For an SMP kernel running on an UP system, "the least protected variant= " is > > the obvious choice to make because you don't lose anything by holding a > > spinlock longer than necessary. However, as you increase the number of = CPUs, > > there rises a tradeoff between local irq servicing latency and global l= ock > > contention. > > > > Imagine a, say, 128 cpu system with a few cores servicing relatively hi= gh > > frequency interrupts. Let's say there's a mildly hot lock. Usually, it = shows > > up in the system profile but only just. Let's say something happens and= the > > irq rate on those cores went up for some reason to the point where it > > becomes a rather common occurrence when the lock is held on one of thos= e > > cpus, irqs are likely to intervene lengthening how long the lock is hel= d, > > sometimes, signficantly. Now because the lock is on average held for mu= ch > > longer, it become a lot hotter as more CPUs would stall on it and depen= ding > > on luck or lack thereof these stalls can span many CPUs on the system f= or > > quite a while. This is actually something we saw in production. > > > > So, in general, there's a trade off between local irq service latency a= nd > > inducing global lock contention when using unprotected locks. With more= and > > more CPUs, the balance keeps shifting. The balance still very much depe= nds > > on the specifics of a given lock but yeah I think it's something we nee= d to > > be a lot more careful about now. > > And this looks a very plausible argument to me: I'll let it sink in. > > But I hadn't heard that the RT folks were clamouring for more irq disabli= ng: > perhaps they partition their machines with more care, and are not devotee= s > of high CPU counts. > > What I hope is that others will chime in one way or the other - > it does sound as if a reappraisal of the balances is overdue. > > Thanks, > Hugh (disabling interrupts for as long as he can)