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 B61A3C282C1 for ; Fri, 28 Feb 2025 16:07:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 46A40280001; Fri, 28 Feb 2025 11:07:57 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4199F6B0088; Fri, 28 Feb 2025 11:07:57 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2E113280001; Fri, 28 Feb 2025 11:07:57 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 10BB96B0085 for ; Fri, 28 Feb 2025 11:07:57 -0500 (EST) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id B0BABB33CE for ; Fri, 28 Feb 2025 16:07:56 +0000 (UTC) X-FDA: 83169834552.30.5C1B43F Received: from mail-pl1-f172.google.com (mail-pl1-f172.google.com [209.85.214.172]) by imf15.hostedemail.com (Postfix) with ESMTP id A0572A0007 for ; Fri, 28 Feb 2025 16:07:54 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=VCmIWZqn; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of inwardvessel@gmail.com designates 209.85.214.172 as permitted sender) smtp.mailfrom=inwardvessel@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740758874; a=rsa-sha256; cv=none; b=jhPvcA5rQB75Y0qzjUmT1emZfRuk/9OAm0rON/f4lY0u1zxTA5wK3HCnMgB0h07+M+VGuT ZNh6ePty11nEe2B9Bn9f5O+J+fZE0V6fiYZRnTvDepbU8Dxhda5ttBduPXSR87CT3Wbh98 tgaqk5juMluBUgjzx1vfqFgurzYYhb4= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=VCmIWZqn; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of inwardvessel@gmail.com designates 209.85.214.172 as permitted sender) smtp.mailfrom=inwardvessel@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740758874; 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=RbNq4X7wcuSfUBCqJJ3Aotkg4KH2vHbVeL4DNaPJDPE=; b=K+J5qZkqiGvSnmgtALvooLQhV/MZNxHYwKVOBsQdMFoelEa5dXp9l4nwGNU6n+d+pBFm/8 gByTgJtqraYZuM4NEgFZv6ApUe3Ml4sr8ydoogkSsjL3sdulzOh4DbeunxsfRRZvWsRywq nK7xqpqIksz0X08LDHyrvNSZT398cd0= Received: by mail-pl1-f172.google.com with SMTP id d9443c01a7336-2234e5347e2so49348205ad.1 for ; Fri, 28 Feb 2025 08:07:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740758873; x=1741363673; 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=RbNq4X7wcuSfUBCqJJ3Aotkg4KH2vHbVeL4DNaPJDPE=; b=VCmIWZqnWfW6VsJdl92vRlWNXNhW+nKniLepzx68V4+C9D3Es3cmccqo9fQalU3Ny8 Y4ekLCYjNs1Sur9lFJIvelaCQSUHBVRnvPTDAwMzOWapkNdRE/YZnjdI+NZwRBzxAndw wb6Ru2VdCK/lO/2Gqh+/iLS6JuUUgD1rUBJeWZO9C/iRSrsc8atanmhowUTThwzNJLfA /GL9Cx0xHZCoQQU14jnjtj+ssO8RVu3CEqmPN/29DUseuC+7Y/GnqMp/drK4TUexGWE8 RVlDAWHp0ClLwXi/9DKDj1CitXsFFpUrx0dVDSzqxH7knY7Q/mGYV2DK/3E6mj7zwtoz 1USg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740758873; x=1741363673; 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=RbNq4X7wcuSfUBCqJJ3Aotkg4KH2vHbVeL4DNaPJDPE=; b=KUFSrZfniZ3ofNU7+PrXsgb7eRUs878xvupfaG5ffJk5kl4spX53BOqM0EsVQmTCbp yB8OcJJyAF+X8x1x9MMxFDcu7Ao7AR8lQ/aAsID5Uc8+4vdISWa882Gg2I0uzZtCzhIK BzV2qY5Pq0srSx7jkKXjJ1DcTiGX7FhtIEHshRl2VSOdpqe8jrFvYHMm0vEqkrkc0sS5 u1XINm5AefYF/d778NWOx82ZWKDU264VNlyDnxhgmlBA+6EUPLkFk50MxMI+wPta6pTx 74Fb6jrACoxQWTZWDb6xKf5oOSqu8sNAVnODy4mDbl4npLtI6Ny/kFwrclziZ1xNYdF/ Z8LA== X-Forwarded-Encrypted: i=1; AJvYcCUONYjVAMuI15JopzYtrhB7rcz1pr7JtuiAzv4VoUSb0+BBi1Si86ZW5a9l+pjwv6qVDWQnF+9UjQ==@kvack.org X-Gm-Message-State: AOJu0YwiRzDVu1y029o97hT9YqXniXpZh26jzlf1m/FQA+P+jiRVtX1M MzDyB5PENiKO1oq/ijOsDPzfO+PVhXRMidXE4TL+zaCbtTkSai/W+H+8uw== X-Gm-Gg: ASbGncu4nXJ2KbSkfa8FKaBmBLZKWA7/mb/nVPByWGeBwOQuHfxBf26OV9nAV6k7bbe 98xs6h9yk2x2rB6AoARg3k6u1x4vsEB7l6WLZcPZ9c8slxBvrJvI9CfADNBz2cILjb2CimxbXdQ KNTSpCcM/n6JCEr2AaKl5cYtmURutNUmWVlg1tO27QiL9ZmJmO+MNfpwbtRCXWHzZofovk/Jptz y4W2JQE+GlHXRnZTHSJeOqTky00gv1DyD20vfw2o8RTTTKKuJs0NqvTiXVH4/rTzfjSk5mhKa4b nTvRwbaLz56xRmVzxPN2uXDi2HGXQtaoIMKQZskRLj6w9/17+il6uRInoUa8/JB6j5lvB/zI X-Google-Smtp-Source: AGHT+IEc/LpjyJNyc94QD2v1vBhS/25v1h4rkPWRkrce2r8rUx8g1O7eBeS9N2xW3uDvn0nqBf1Epw== X-Received: by 2002:a05:6a21:1796:b0:1ee:89ba:d9e6 with SMTP id adf61e73a8af0-1f2f4c94844mr7680017637.7.1740758873270; Fri, 28 Feb 2025 08:07:53 -0800 (PST) Received: from [192.168.2.117] (c-67-188-127-15.hsd1.ca.comcast.net. [67.188.127.15]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-7349fe48873sm3906691b3a.49.2025.02.28.08.07.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 28 Feb 2025 08:07:52 -0800 (PST) Message-ID: <066a7505-a4fe-4b76-b7a6-ae97f786dac7@gmail.com> Date: Fri, 28 Feb 2025 08:07:50 -0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 3/4 v2] cgroup: separate rstat locks for subsystems To: Shakeel Butt Cc: tj@kernel.org, yosryahmed@google.com, 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-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: A0572A0007 X-Stat-Signature: zmidwiathc7qajchws5ru8ofkciqqhgt X-Rspam-User: X-HE-Tag: 1740758874-384224 X-HE-Meta: U2FsdGVkX184fhuZTQvkryuw0eetKpmJHvULGdX8JmT7T6/AQCe7P3btnSK0PCfUia0fF/azhwY+ThBDLp8S5nLV596SF3FbHZuAfR4Lyyf9HpQsEm5njmbY2YDFceqUOfhHSjgI1NHX2HHDSNO+/JtWXQaEBtknZNiLalHddBxbPllxbAfRZXd23QUvK1yc5DrFV0Lg6W8dw/gtPikp2bSCt963Vb9viluGgSxxJZa7t5SC9masgxr3IfyWIaOgMJLjdp1tlefGl79NF2vDJYGeziJjE9EKi3C293oazc9dlbgKkq/ED0C0m8dL3YkMc+JtKczytsKF8UnyuGumQFv/HgTxhZ/0Ms8ZXGwkQG4Ks2ACGqOkV12fGfqV9eBIvmqyHospLIgShQTLr3Ud8xhaxfh0MJJPhHumoRPZDwIqAVqPLPPZBvyF6dGtbZtDloIBACCc5RIx1TL5LMQZyDoFR0tFAAOcszXzMay5HKsmg5dsIQzeZlQpl5rhStoE6XzbYTPoszEm+yNT5LBg+Z8SeKtdLzubLTnx1bBvqWMrRgnbdOM0QDoOlER7ezuF8sneHkRQFgU6pKSOFMBZBstUYu0ePkTm5ObIp0U9Jpg1qg/9jGX8ffAVTsxu0XfLFD6doAGtAl8OfNTsSZpqVAiRTXh9DpM1pxotl3GMlzo+H79OOeXyzNTLsiVpsfzvJ1HhEOV8Im5laOWy+4yrNAXRqESoUJSFU/BppzeDRRYDiPbPrrhvpe5EnOaqKFyozZwgmccpOoJQfRpkYu9k+k6pmSqIZ7ametyg7zlabQd5glzzaivpW+Qa7fJT0J0OpqVsll+06IbEgOoTL9y141xKwRqG1hPixF9s+eurEhyawUpPw9IYvm/8VaMay2I5JM+wTRKlqKUEjijwox2kl199YxWM9gmvQQ6SHF2OzMufLU24d1D0vWxSbLKZrkfgUnDQtALr149O11cM5YZ VMeIRJz2 cc/H0ZMMj9LJizF5lcLJkP1Ur1IUHeoyCug5+pbEUbnLYqNK6FSRt1kXiKP1n+xY/mV1Np7UffRTnFAgUDAgNI3KJ5WfJRjHAoR1b9VgVfWWCt8gsjSsU3DnTV0VTI51FFfVt+gZkeEK8LM2JfIRQNFF5FBH4eFlsKqxA+Hc+77/aZjm8AcckXZXgYPeaWUvokhfrbEOjjtRZO5tnCOokFzotf1RobMb5645AUfJkJSbeF43roXQHQfmA/6X7YQZuuHtOco+40Ibf+ZzymewlnVno/Tg5UQ6RmKgPXXcrjtO69q0FRngdaO6HmpTrem2GpXmS2hlkl3zkh0wGEcp3Lq7piwT/dbrbBVGhI/PRnfdpcfCqfLMwgy9WHaAHtWKv/U2REviCbZ2LRQ1+YSeZilVvc2UHbpZ1hJ8cy/Wp8F2clhM= 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/27/25 2:52 PM, Shakeel Butt 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 > > Couple of nits below otherwise: > > Reviewed-by: Shakeel Butt > >> --- >> 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]); >> > > The name of these locks are too long and you had to go multi-line. > Please just reduce the size of the names. These are local to this file, > so maybe you can drop cgroup_rstat_ from them or keep the rstat. Agreed. Will do in next rev. > >> 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; > > Use the array index here like cgroup_rstat_subsys_cpu_lock[css->ss->id]. Good call. I see the arithmetic may be more appropriate in cases where the per-cpu array/buffer is dynamic. I'll use the index notation in v3. > >> + >> 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; > > Same here. > >> + >> 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]; >> >> 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); > > Same here. > >> + } >> + } >> } >> >> /* >> -- >> 2.43.5 >>