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 15E88C282D1 for ; Thu, 6 Mar 2025 21:47:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9563728000A; Thu, 6 Mar 2025 16:47:49 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 8DD86280009; Thu, 6 Mar 2025 16:47:49 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 70A2B28000A; Thu, 6 Mar 2025 16:47:49 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 50041280009 for ; Thu, 6 Mar 2025 16:47:49 -0500 (EST) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id C0ADD1C7FF0 for ; Thu, 6 Mar 2025 21:47:51 +0000 (UTC) X-FDA: 83192463942.22.D9A523B Received: from mail-pl1-f180.google.com (mail-pl1-f180.google.com [209.85.214.180]) by imf07.hostedemail.com (Postfix) with ESMTP id B49A940006 for ; Thu, 6 Mar 2025 21:47:49 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Ucl6nrmh; spf=pass (imf07.hostedemail.com: domain of inwardvessel@gmail.com designates 209.85.214.180 as permitted sender) smtp.mailfrom=inwardvessel@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741297669; 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=t9a0dAVL/AZWOUQR/lGFl+t00mrO/OYf4dYWFlvsLAw=; b=e7ixJS45VFHcZDGjYcqXVM9x/ifgPgAItgi/Z0y/IFtqih5Lgw1m/RoaXxoedSS2x1S8vs 4CIz+xnA1h9lk2nQ6Zije7YzWnaRIN51fKUYAqmvr62XYLWgS1xd3FrKp+O2Zc7xaxL80x ty7CCiicfNB/LIzy3iFj1os/fPXk4LM= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Ucl6nrmh; spf=pass (imf07.hostedemail.com: domain of inwardvessel@gmail.com designates 209.85.214.180 as permitted sender) smtp.mailfrom=inwardvessel@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741297669; a=rsa-sha256; cv=none; b=oNq2pyq2rbYyVFznUieq2bSjhglu5NWIMwOenWiwAr91FRPZIMW0b0zxwupCDO4o4vc6qq kNq4B7PxUb8mf112odxZNf9beExdtt+/tmw3H8DU9JYkmjRpAxGpRIkrEybho9UVk2iJik gYvebZVUllk8ugFC/Bd3bHg4U2akpvg= Received: by mail-pl1-f180.google.com with SMTP id d9443c01a7336-223f4c06e9fso22891925ad.1 for ; Thu, 06 Mar 2025 13:47:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741297668; x=1741902468; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=t9a0dAVL/AZWOUQR/lGFl+t00mrO/OYf4dYWFlvsLAw=; b=Ucl6nrmhh+83yj5mRKaJP6NRhr10m1nSC688lNbUFzZaZRyJmePM2D+8C0R+gceqTp Z4S9Tw0hgiBL0RPe1ZB0UcKwuBH50RGVAdauANNoz8eSzzt5m4hdfqgBhqZF3ae7JnSV 4kFt8bVsvjLGHDZxcpYeJPdS0RbrY7etQa5FLhjJ1vFD70aOkMcjfu8R8HRQvACCtvvv RGN0PwsqEHrBT9CP0UbaZTepkEV9QBDLG+bGoEQu2QSKfxWpBIt+UJUpXA0z1ocwvCLE 1Hyd4902aEZIHw9XFNk7xoigVY0BiZfX/5t/UCnxksf3V9QUjkwFw7Xckq1G9Pg34zTq GItw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741297668; x=1741902468; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=t9a0dAVL/AZWOUQR/lGFl+t00mrO/OYf4dYWFlvsLAw=; b=TKqHGGYYwlraL3lHi2V+wIm5LBJpRCA7cx8sIt1KQoaXe8Gg2WY0l8b2YoYPt6Rny2 cT9QOsHCYziT5MXJ0YNqAgNrmlZh1JwGV/QJoNeSZiIvc+izQO1nlGhmvFlRY8Gfpe6V 3lCDdP2oK+9kDE3yi5e3latQJUzsVsBLr+rnKXcSgazNJZ8ZEwPcmR1P+t8RvEvVkt5w f7nSNaLg0LfysH9EUGztzhpZjBnLxMJLTQbGSHWL0s6XwETEAD6s2x9At12ZUGKzJ+24 Hahf80/RhEKpzzFMJbddhb/GaGumqZqGejiWxkT4gYIi9pOq48EMOn94s6ZUPtRl0DXt 7hOA== X-Forwarded-Encrypted: i=1; AJvYcCVRbRQURzwo0S1Mt+EFd4WYFDuSt8cnrRTCE+7dRQxxaRckereOod51apC+PsT0/fM9w9fm24DPLg==@kvack.org X-Gm-Message-State: AOJu0YzeiWfvz2s5aepfUJ2u2o26wabR687RkgxrlRr6GgV3ud/VCLLY ENPQCrDkkTVTj7XqV7spdu36PWgKH8oe9hD0RgGo3DISyPj//ARO X-Gm-Gg: ASbGncsM3Kre5Yd/661karO5IeetTxpzBqKeN+T/fzFA8p4zjp7VasXeppszVqlDcTt frSpJJv0dA0zGchvQ4ZXczZ29cE2ZYhPfDx/3Rgo8nbHf4EWDdkx4M4z+gVeaMDLXlhM0ICMPWa p2pZ9pszcbTc/uSz9cjsN2nWNLjzJA95vKtNG8z0CSgJXi/iL8BbN1cTyJL/mPjrLbK0kx8bTjL +OYhGefUyqEbu77SyBwvihZaDAzsAwkdcV7bjdjjClrxPKS7J3nwzGEtmmZAXXKH9jF1DcB+ZAj DVomO0cUPd3a1J0nJ2tmMAfo3V1wkQpWdOJx2Qtza7kVkhVOU1jOlfWs/1GktxkCYMYO0l1Qb1L 3 X-Google-Smtp-Source: AGHT+IGVGsKex96vDISYzZdtD1CF7FPmnf5XXQSJH1a6blsHvrz5J51CDUj3DDOuLRqv9alKaayf2A== X-Received: by 2002:a17:903:11d0:b0:21f:53a5:19e0 with SMTP id d9443c01a7336-22409404e15mr79186615ad.12.1741297668292; Thu, 06 Mar 2025 13:47:48 -0800 (PST) Received: from ?IPV6:2a03:83e0:1151:15:d431:c86b:892f:8e30? ([2620:10d:c090:500::6:d8b]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-224109ddcaesm17349635ad.28.2025.03.06.13.47.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 06 Mar 2025 13:47:47 -0800 (PST) Message-ID: Date: Thu, 6 Mar 2025 13:47:46 -0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 3/4 v2] cgroup: separate rstat locks for subsystems To: Yosry Ahmed Cc: tj@kernel.org, shakeel.butt@linux.dev, mhocko@kernel.org, hannes@cmpxchg.org, akpm@linux-foundation.org, linux-mm@kvack.org, cgroups@vger.kernel.org, kernel-team@meta.com References: <20250227215543.49928-1-inwardvessel@gmail.com> <20250227215543.49928-4-inwardvessel@gmail.com> Content-Language: en-US From: JP Kobryn In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: B49A940006 X-Stat-Signature: 6jn79pzrp7zonge3we3ezrgcwcnh19pt X-HE-Tag: 1741297669-426105 X-HE-Meta: U2FsdGVkX19IGwpWSUr3DS5JiJ7rXI9rIx3hS4PrZV83rQa6/zmQgmpzNRMTEQQim4RWNgm6dBStO4UsEKqxV1jMcjlGO/yx2Ottvlq/XHTGRZfEE8zHEbrHCI0POSjYaOJ84ofSQtj4Lod0x8rId6EUiWTRMiVNdb65sEK4tT6uKE4OU6y3qlWGBuTupqtrhjwfllBy7BMd+uITSV42289STpMT0c+hu/Brr6tCGQEzGaxw8XCJVLzVoiai0ttaYWhHVxIC1poytbYNDTzGhZ1SLcXFql8Kfo0cGzQiSotuH7sLlv8SCm58Fxs0CJRZ5ks++J3O1T+cZ8sR2i6iDzq+gXeX7CkzJteqbYp68qqpSg+4Ct0WkyDXCPrkcHvWCd+rBK+HgB1jM1qrwDPx7xQcsTLBaAO7ObLgtt7zEZErvF1s2RLbVNAXt+JMiyLI8v/HveUNuJ+DZSZZk3Dl8lG4+2s/BF9/kUQLk18/rd+C7CTvMdFNYil0N0nm4UT8dcwtNyXWyn3bcZs6m7R7FinGjx7LS8m3c2jJTcvfaG7/rDue3npExZVT7r+3SDqcUwFakIiHiRvks8zZtaHkAR8RKAtio+L188sL2vOR9sxrvPzZuUtWhlvsrvU5cSg/rk+ItUl09IYfyKkBpkPBOTzGsYc0W+lcYGti4I0nmzDMe1QTUuMU2ZaPLAfSX0Dnbhc4izKFXwBxTNQDnVUsnltIGNvNuu9Iaqy4ifPTuL1BPncqtiUH2KWBgu379QFZIG/nLhv/qwVsvo9lUwldW86sRaQKo47C2rL66b90t0vDlbXjA1ELVvWYbyCepL3Kccb4Hly9Klysq8J2dDCjQV0PHj8PRuvukyc6kj7N8Qdui1i38QEFX//tNMuU+knQUR7lQw0VSZWaVjPqp7ZzDwxKAOI2q0N5kLJsbE6vkikGqvtDhGEtdNmrl1Lhvwz2Bc77cd4JBbqM8ny+QZl qjTsWSsI +Ub7bMwTKoVsAPIWWxqyhSV9rCkbtJ4r+heuqELsC2d4kxsw4LUK4yUc7+z4taZZ1vGJx9ckgBT9YFJS60v9l2Qy/ODTQUPlW6iNjJ/wqLuAjGWGiHV0waysiFKdfaJxgcsiFoBHQZ3JcJ0/Z1HTOY81M/BFCKc5p42akVwvLAuaoxGbZ6nGXV8NarVIb+4RR+KAwWrxLS6tsH6WthBTQuWUCrEOympQgJ4imw6JMsf8vYxuAdBK2th62Tcj74R1KNPRpmT1+Bsb7voGgpWMubfd3WjiN7eMNA2r5IDzvKKPsg1mg9MzWVIrjU3JVQ6bx9+wmQhKR09LZw34eHqIsB8pE3dQ9kAxBANdUoLh9zF8BPd3+PivgyaupBIqKNZ6dA7YIQSNHbNaPcqets1xdMJLXRq17lCLCABjiWS0p37DQpZpKA0xO9fWP4g== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, 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 2/28/25 11:20 AM, Yosry Ahmed wrote: > On Thu, Feb 27, 2025 at 01:55:42PM -0800, inwardvessel wrote: >> From: JP Kobryn >> >> Let the existing locks be dedicated to the base stats and rename them as >> such. Also add new rstat locks for each enabled subsystem. When handling >> cgroup subsystem states, distinguish between formal subsystems (memory, >> io, etc) and the base stats subsystem state (represented by >> cgroup::self) to decide on which locks to take. This change is made to >> prevent contention between subsystems when updating/flushing stats. >> >> Signed-off-by: JP Kobryn >> --- >> kernel/cgroup/rstat.c | 93 +++++++++++++++++++++++++++++++++---------- >> 1 file changed, 72 insertions(+), 21 deletions(-) >> >> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c >> index 88908ef9212d..b3eaefc1fd07 100644 >> --- a/kernel/cgroup/rstat.c >> +++ b/kernel/cgroup/rstat.c >> @@ -9,8 +9,12 @@ >> >> #include >> >> -static DEFINE_SPINLOCK(cgroup_rstat_lock); >> -static DEFINE_PER_CPU(raw_spinlock_t, cgroup_rstat_cpu_lock); >> +static DEFINE_SPINLOCK(cgroup_rstat_base_lock); >> +static DEFINE_PER_CPU(raw_spinlock_t, cgroup_rstat_base_cpu_lock); >> + >> +static spinlock_t cgroup_rstat_subsys_lock[CGROUP_SUBSYS_COUNT]; >> +static DEFINE_PER_CPU(raw_spinlock_t, >> + cgroup_rstat_subsys_cpu_lock[CGROUP_SUBSYS_COUNT]); >> >> static void cgroup_base_stat_flush(struct cgroup *cgrp, int cpu); >> >> @@ -20,8 +24,13 @@ static struct cgroup_rstat_cpu *cgroup_rstat_cpu( >> return per_cpu_ptr(css->rstat_cpu, cpu); >> } >> >> +static inline bool is_base_css(struct cgroup_subsys_state *css) >> +{ >> + return css->ss == NULL; >> +} >> + >> /* >> - * Helper functions for rstat per CPU lock (cgroup_rstat_cpu_lock). >> + * Helper functions for rstat per CPU locks. >> * >> * This makes it easier to diagnose locking issues and contention in >> * production environments. The parameter @fast_path determine the >> @@ -36,12 +45,12 @@ unsigned long _cgroup_rstat_cpu_lock(raw_spinlock_t *cpu_lock, int cpu, >> bool contended; >> >> /* >> - * The _irqsave() is needed because cgroup_rstat_lock is >> - * spinlock_t which is a sleeping lock on PREEMPT_RT. Acquiring >> - * this lock with the _irq() suffix only disables interrupts on >> - * a non-PREEMPT_RT kernel. The raw_spinlock_t below disables >> - * interrupts on both configurations. The _irqsave() ensures >> - * that interrupts are always disabled and later restored. >> + * The _irqsave() is needed because the locks used for flushing are >> + * spinlock_t which is a sleeping lock on PREEMPT_RT. Acquiring this lock >> + * with the _irq() suffix only disables interrupts on a non-PREEMPT_RT >> + * kernel. The raw_spinlock_t below disables interrupts on both >> + * configurations. The _irqsave() ensures that interrupts are always >> + * disabled and later restored. >> */ >> contended = !raw_spin_trylock_irqsave(cpu_lock, flags); >> if (contended) { >> @@ -87,7 +96,7 @@ __bpf_kfunc void cgroup_rstat_updated( >> struct cgroup_subsys_state *css, int cpu) >> { >> struct cgroup *cgrp = css->cgroup; >> - raw_spinlock_t *cpu_lock = per_cpu_ptr(&cgroup_rstat_cpu_lock, cpu); >> + raw_spinlock_t *cpu_lock; >> unsigned long flags; >> >> /* >> @@ -101,6 +110,12 @@ __bpf_kfunc void cgroup_rstat_updated( >> if (data_race(cgroup_rstat_cpu(css, cpu)->updated_next)) >> return; >> >> + if (is_base_css(css)) >> + cpu_lock = per_cpu_ptr(&cgroup_rstat_base_cpu_lock, cpu); >> + else >> + cpu_lock = per_cpu_ptr(cgroup_rstat_subsys_cpu_lock, cpu) + >> + css->ss->id; >> + > > Maybe wrap this in a macro or function since it's used more than once. > >> flags = _cgroup_rstat_cpu_lock(cpu_lock, cpu, cgrp, true); >> >> /* put @css and all ancestors on the corresponding updated lists */ >> @@ -208,11 +223,17 @@ static struct cgroup_subsys_state *cgroup_rstat_updated_list( >> struct cgroup_subsys_state *root, int cpu) >> { >> struct cgroup *cgrp = root->cgroup; >> - raw_spinlock_t *cpu_lock = per_cpu_ptr(&cgroup_rstat_cpu_lock, cpu); >> struct cgroup_rstat_cpu *rstatc = cgroup_rstat_cpu(root, cpu); >> struct cgroup_subsys_state *head = NULL, *parent, *child; >> + raw_spinlock_t *cpu_lock; >> unsigned long flags; >> >> + if (is_base_css(root)) >> + cpu_lock = per_cpu_ptr(&cgroup_rstat_base_cpu_lock, cpu); >> + else >> + cpu_lock = per_cpu_ptr(cgroup_rstat_subsys_cpu_lock, cpu) + >> + root->ss->id; >> + >> flags = _cgroup_rstat_cpu_lock(cpu_lock, cpu, cgrp, false); >> >> /* Return NULL if this subtree is not on-list */ >> @@ -315,7 +336,7 @@ static void cgroup_rstat_flush_locked(struct cgroup_subsys_state *css, >> struct cgroup *cgrp = css->cgroup; >> int cpu; >> >> - lockdep_assert_held(&cgroup_rstat_lock); >> + lockdep_assert_held(&lock); >> >> for_each_possible_cpu(cpu) { >> struct cgroup_subsys_state *pos; >> @@ -356,12 +377,18 @@ static void cgroup_rstat_flush_locked(struct cgroup_subsys_state *css, >> __bpf_kfunc void cgroup_rstat_flush(struct cgroup_subsys_state *css) >> { >> struct cgroup *cgrp = css->cgroup; >> + spinlock_t *lock; >> + >> + if (is_base_css(css)) >> + lock = &cgroup_rstat_base_lock; >> + else >> + lock = &cgroup_rstat_subsys_lock[css->ss->id]; > > Same here. Also, instead of passing locks around, can we just pass > the css to __cgroup_rstat_lock/unlock() and have them find the correct > lock and use it directly? I like this and will take that approach in v3. It will allow the lock selection code you previously commented on to exist in just one place. > >> >> might_sleep(); >> >> - __cgroup_rstat_lock(&cgroup_rstat_lock, cgrp, -1); >> - cgroup_rstat_flush_locked(css, &cgroup_rstat_lock); >> - __cgroup_rstat_unlock(&cgroup_rstat_lock, cgrp, -1); >> + __cgroup_rstat_lock(lock, cgrp, -1); >> + cgroup_rstat_flush_locked(css, lock); >> + __cgroup_rstat_unlock(lock, cgrp, -1); >> } >> >> /** >> @@ -376,10 +403,16 @@ __bpf_kfunc void cgroup_rstat_flush(struct cgroup_subsys_state *css) >> void cgroup_rstat_flush_hold(struct cgroup_subsys_state *css) >> { >> struct cgroup *cgrp = css->cgroup; >> + spinlock_t *lock; >> + >> + if (is_base_css(css)) >> + lock = &cgroup_rstat_base_lock; >> + else >> + lock = &cgroup_rstat_subsys_lock[css->ss->id]; >> >> might_sleep(); >> - __cgroup_rstat_lock(&cgroup_rstat_lock, cgrp, -1); >> - cgroup_rstat_flush_locked(css, &cgroup_rstat_lock); >> + __cgroup_rstat_lock(lock, cgrp, -1); >> + cgroup_rstat_flush_locked(css, lock); >> } >> >> /** >> @@ -389,7 +422,14 @@ void cgroup_rstat_flush_hold(struct cgroup_subsys_state *css) >> void cgroup_rstat_flush_release(struct cgroup_subsys_state *css) >> { >> struct cgroup *cgrp = css->cgroup; >> - __cgroup_rstat_unlock(&cgroup_rstat_lock, cgrp, -1); >> + spinlock_t *lock; >> + >> + if (is_base_css(css)) >> + lock = &cgroup_rstat_base_lock; >> + else >> + lock = &cgroup_rstat_subsys_lock[css->ss->id]; >> + >> + __cgroup_rstat_unlock(lock, cgrp, -1); >> } >> >> int cgroup_rstat_init(struct cgroup_subsys_state *css) >> @@ -435,10 +475,21 @@ void cgroup_rstat_exit(struct cgroup_subsys_state *css) >> >> void __init cgroup_rstat_boot(void) >> { >> - int cpu; >> + struct cgroup_subsys *ss; >> + int cpu, ssid; >> >> - for_each_possible_cpu(cpu) >> - raw_spin_lock_init(per_cpu_ptr(&cgroup_rstat_cpu_lock, cpu)); >> + for_each_subsys(ss, ssid) { >> + spin_lock_init(&cgroup_rstat_subsys_lock[ssid]); >> + } >> + >> + for_each_possible_cpu(cpu) { >> + raw_spin_lock_init(per_cpu_ptr(&cgroup_rstat_base_cpu_lock, cpu)); >> + >> + for_each_subsys(ss, ssid) { >> + raw_spin_lock_init( >> + per_cpu_ptr(cgroup_rstat_subsys_cpu_lock, cpu) + ssid); >> + } >> + } >> } >> >> /* >> -- >> 2.43.5 >> >>