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 840CDC6FD18 for ; Tue, 28 Mar 2023 18:53:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 01DEC6B0071; Tue, 28 Mar 2023 14:53:30 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F10A56B0072; Tue, 28 Mar 2023 14:53:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DB2A46B0074; Tue, 28 Mar 2023 14:53:29 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id CAF386B0071 for ; Tue, 28 Mar 2023 14:53:29 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id A5C071A03E9 for ; Tue, 28 Mar 2023 18:53:29 +0000 (UTC) X-FDA: 80619205338.19.84CB8B3 Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com [209.85.208.41]) by imf07.hostedemail.com (Postfix) with ESMTP id C0F034000A for ; Tue, 28 Mar 2023 18:53:27 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=eBFPS5up; spf=pass (imf07.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.41 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=1680029607; 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=Knz0ICKyVQWqTN98QRlsl94SOUBbAgJSfpknOWcWIz8=; b=qtnDOoBAFNLFVgL+gRUX8mBX/xyMmiUA1NA8qGtK17hFb/tKqREjv8Rcmd9yDLCXKIDrBv 9n1pYiIb9NpBpJ+Z53rfpTjr1yPSn0dB5KL/LPO5baRy9igPwHULDpnGgq8l8eihsvZ8Z7 47zuCzdn1N4axd64aBZOksrMcu+Tgg8= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=eBFPS5up; spf=pass (imf07.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.41 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=1680029607; a=rsa-sha256; cv=none; b=JhQIQ34VGEH9fJ6AngvDbV2bktB2qMrakXS73JRgQCiICwyN08/apiN89LBjabUaA6fhhU MYD21biWAYXh5nllI3nT/3KHVjRTan8JgKHCgireqxxkXpBlj3mFLXgzr6IdbKCl8ARQ8S Hr7CZjn789lszjaRh9h48/sggEytYKQ= Received: by mail-ed1-f41.google.com with SMTP id w9so53782902edc.3 for ; Tue, 28 Mar 2023 11:53:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680029606; 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=Knz0ICKyVQWqTN98QRlsl94SOUBbAgJSfpknOWcWIz8=; b=eBFPS5upevzYowGb39P08hDPA5ByzP8CKwYQMFgOKj+6IYq2Vkfhm5oL5cVrbZhHzf eQHkB8C313lW8nsV9BpZNUZGGFqiswJwY5FMsPU6z/cd/O9XwAIDMFqeGPfx35miDyrF +bGkozLip971EeAk9h6UC0HOXtdfFwRiZJkEUoXuLJ3fBqgLl1/lbS6hfhsmfL9XRhH+ KR9XbqwM57zERf5JS531f6qDu8XI49Nmmt2jHNhErVh4imKzomY5+lfULAhHKaJb3jM2 Rye/OtfKqLqEEIMO08YuFZgxXxl9NZRbCLkFj6dKWKMGFVVM0dFa3ui8jA4/djrvfILk dA1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680029606; 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=Knz0ICKyVQWqTN98QRlsl94SOUBbAgJSfpknOWcWIz8=; b=PKQ6Fi9MluOG8wxQk5caGeGcyUnRi0+DqHeW0QqUueKxaH3LLgDRd49CWZUXrxB63A az1Iu2JJo813j2+OXBssP1U81cIyX4UGAKDSLDtylOkESNH7TUeYC4BAVGjL1GXCJsW7 Y6zTksbzqI+itoMZ+m9JDcqLoIzO0InPftc3uFokiTH0V4OI/vgrHNcDzAxjPZHa7eA1 YHpMRi/Axd6lLt2jBctma/1PCqz7aIMjMVPnXPvk/ngVxQHYyc3UU8DOlzAfy5LRdlEC x4CzrOl9MAhRGcL3CBdAW+iRvFgNOzNIVr3Yho6iUPsu5+LNoSsHXSgnnKGcGPPaAb/J 6xdw== X-Gm-Message-State: AAQBX9e+MlA8hSbygNDWD2nP2vCrocrOtzQerU4/gXA1KZ4sjKF5FRRQ F1xwhVuFc1uh2rpE6NTp/6Xg5VYU4LlI9s6A5KWuNw== X-Google-Smtp-Source: AKy350Z3M7QR34ahCbawZrKpU+fclV+7eeIoXTqQ34bVv12otunyd4ooJvbGExuWfU2TzFjs2F14JyzAfPoOyuqb8SA= X-Received: by 2002:a17:907:9870:b0:8b1:28f6:8ab3 with SMTP id ko16-20020a170907987000b008b128f68ab3mr8771567ejc.15.1680029606204; Tue, 28 Mar 2023 11:53:26 -0700 (PDT) MIME-Version: 1.0 References: <20230328061638.203420-1-yosryahmed@google.com> <20230328061638.203420-6-yosryahmed@google.com> <20230328141523.txyhl7wt7wtvssea@google.com> In-Reply-To: <20230328141523.txyhl7wt7wtvssea@google.com> From: Yosry Ahmed Date: Tue, 28 Mar 2023 11:52:50 -0700 Message-ID: Subject: Re: [PATCH v1 5/9] memcg: replace stats_flush_lock with an atomic To: Shakeel Butt Cc: Tejun Heo , Josef Bacik , Jens Axboe , Zefan Li , Johannes Weiner , Michal Hocko , Roman Gushchin , Muchun Song , Andrew Morton , =?UTF-8?Q?Michal_Koutn=C3=BD?= , 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-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: uckwpkiwgx47t64f1fjcwdjk9pfcj6t6 X-Rspamd-Queue-Id: C0F034000A X-HE-Tag: 1680029607-467919 X-HE-Meta: U2FsdGVkX1/ruhCGO/tG+S5gBANKocA7kpFI7IZiowgbnWcwb85IplaVPJ+slMLf6rhTeGr9IPGMFSrlJwrRGzO2eqNXfCzMWzJRn/Nmag9TnblmBDjv4qDSr5ur50J8BTEbk1UhNgFKVhVkjIVPVPLKg6ga82XumylBt3724kWwPJ2LDBDk64adas3Hegt6UEnCdKRae2Rkg1iwJDaZxSfIpFHoSk8WnmNU+9fdkFONsaroltiNKPDKx7I7jWKcbvyXhX+WnKQM+8MnwriDeCmIvKX886bl2tP1GwhecHmkX9nIMjekZ28AlgtWjCzWrRrlnjU9CaXJFW9fSzXNxvuHZjFDpSRfrbkZ8stYyYSPxk/+yRd9cQcE50Zzjf2pYFFKKbvNCU7BjcuUn1ISVSvHsgKi2aYkR6rPhkBpArLt3AIhjHoVAFWNqruRD1xiJdRZmWZc3yMKC2UCvnq37BYNbbre8kDhnCcxlqm1I+0QMjbRek350j2X8OQV+uDFTtHN8WYRXQ2OPbVEb3ZuCAY5qnrcQLGdthaODP+0G0g6uYmyqy7RTmKMXfIWjuwk2VkpJ6YUh6Z8KgySYUu82QArB7lvPnB9ZVIY4L5u7UNhvwua/Q6nrWxsjlyGndXfjpsBsRELHuNVrM/Ms1lqzgSpLC7jjNhpIG2VNeH3ouQ6o4tEPaDvCkFAlxX8Z0bXJ5SQjAaurHuGp1oxMO/LEeNkIrBxx9w12vsXP3cFPPcWStV6PwK47GPQCYt5kLzhht1fWg84EfQ3oX2xZCPkMzS2+x04ri9ETYIOA/SMCxWt7xSTAVLDBAtnCoB40hvstIGKqCQA4/HufzDEv6KpX3dWXfjhd4+b2kCOqLmdUsm4ev5bfL1OgqmNsSrnZzfZ1TorSAPuINRfG+yp9KHkq/uNkSj3NDdhmOy5YeyXTgFrmQJfGR7GSui5B2FKOXOgK3XVAflune60R8wbboq 1jGkkl8z o2e4vy23RWZGq+lofYic9GzXKDDwMLcUp/vdUXmJrMwo5kOdOA+TinQAKz/iiW2J3MQEk4TxNEtodOnBHBllp8/KKbDO/tDkI/ZegDjpfkJ3PEzg+UIBwj13gYqDcQTM0G97Ddtj/zFekkw4xUzZX6Ca0AXQ8kr28IPdJ/kmRdNJfoz/XDr6UNi7BYnuIoDkmuHowR7gq7U872Sqhs2h9xq/KzevkFfuuKzO2wnaZQ2kkyREmAtFzjACtq4eDk5yxuqTnq4sG1/b1FbsYNrWwMUm45mYmjq/XYGbugpoOAT0peWVsXhXfLszCIA== 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: On Tue, Mar 28, 2023 at 7:15=E2=80=AFAM Shakeel Butt = wrote: > > On Tue, Mar 28, 2023 at 06:16:34AM +0000, Yosry Ahmed wrote: > [...] > > @@ -585,8 +585,8 @@ mem_cgroup_largest_soft_limit_node(struct mem_cgrou= p_tree_per_node *mctz) > > */ > > static void flush_memcg_stats_dwork(struct work_struct *w); > > static DECLARE_DEFERRABLE_WORK(stats_flush_dwork, flush_memcg_stats_dw= ork); > > -static DEFINE_SPINLOCK(stats_flush_lock); > > static DEFINE_PER_CPU(unsigned int, stats_updates); > > +static atomic_t stats_flush_ongoing =3D ATOMIC_INIT(0); > > static atomic_t stats_flush_threshold =3D ATOMIC_INIT(0); > > static u64 flush_next_time; > > > > @@ -636,15 +636,18 @@ static inline void memcg_rstat_updated(struct mem= _cgroup *memcg, int val) > > > > static void __mem_cgroup_flush_stats(void) > > { > > - unsigned long flag; > > - > > - if (!spin_trylock_irqsave(&stats_flush_lock, flag)) > > + /* > > + * We always flush the entire tree, so concurrent flushers can ju= st > > + * skip. This avoids a thundering herd problem on the rstat globa= l lock > > + * from memcg flushers (e.g. reclaim, refault, etc). > > + */ > > + if (atomic_xchg(&stats_flush_ongoing, 1)) > > Have you profiled this? I wonder if we should replace the above with > > if (atomic_read(&stats_flush_ongoing) || atomic_xchg(&stats_flush= _ongoing, 1)) I profiled the entire series with perf and I haven't noticed a notable difference between before and after the patch series -- but maybe some specific access patterns cause a regression, not sure. Does an atomic_cmpxchg() satisfy the same purpose? it's easier to read / more concise I guess. Something like if (atomic_cmpxchg(&stats_flush_ongoing, 0, 1)) WDYT? > > to not always dirty the cacheline. This would not be an issue if there > is no cacheline sharing but I suspect percpu stats_updates is sharing > the cacheline with it and may cause false sharing with the parallel stat > updaters (updaters only need to read the base percpu pointer). > > Other than that the patch looks good.