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 3C3D9C3DA61 for ; Mon, 29 Jul 2024 13:34:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AE1E76B0099; Mon, 29 Jul 2024 09:34:38 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A921E6B009C; Mon, 29 Jul 2024 09:34:38 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 931DA6B009D; Mon, 29 Jul 2024 09:34:38 -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 72A6B6B0099 for ; Mon, 29 Jul 2024 09:34:38 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 14BF4140334 for ; Mon, 29 Jul 2024 13:34:38 +0000 (UTC) X-FDA: 82392885036.06.1AB7285 Received: from mail-pf1-f173.google.com (mail-pf1-f173.google.com [209.85.210.173]) by imf24.hostedemail.com (Postfix) with ESMTP id 1D0D0180002 for ; Mon, 29 Jul 2024 13:34:35 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=vimeo.com header.s=google header.b="m/Bh00dW"; spf=pass (imf24.hostedemail.com: domain of davidf@vimeo.com designates 209.85.210.173 as permitted sender) smtp.mailfrom=davidf@vimeo.com; dmarc=pass (policy=reject) header.from=vimeo.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722260049; 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=tvWuoWcBz1GpvEecxVauV4qhtqCu4z7eC3CZ78S9mDw=; b=Ny8I0k/7lCKlRzZyGocTCwiTwTH4jJ7drL/A1xQzduRsn6NUiPblpxQKHs6eb/2IEpmYOA +qIsRONRyM/hUML6/Oxje3nuf7mYlFv/78537c2h2sl8fdISu66C2Wn3cjwpr4vwYnAlNs 8/vdJMfnxCcb1OPuMB+rN+F1TXNdO68= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=vimeo.com header.s=google header.b="m/Bh00dW"; spf=pass (imf24.hostedemail.com: domain of davidf@vimeo.com designates 209.85.210.173 as permitted sender) smtp.mailfrom=davidf@vimeo.com; dmarc=pass (policy=reject) header.from=vimeo.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722260049; a=rsa-sha256; cv=none; b=ObGyPpbwvvzsWpZXgbutSyWlPqD9nZE5+0W+psdwktAnbXqvxGFFpVpozQXtd4Tu46f0N8 Oqyc4986m6oHtSKEeP8QFJ2+A3FyNeJkiE5pj5PKoBmNWYeADnZfukwwlt+hwuroPX08gy o99LiphRz0f5+ahl3f5Yi+fw7Kg2iz4= Received: by mail-pf1-f173.google.com with SMTP id d2e1a72fcca58-70d2b921c48so2383667b3a.1 for ; Mon, 29 Jul 2024 06:34:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vimeo.com; s=google; t=1722260075; x=1722864875; 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=tvWuoWcBz1GpvEecxVauV4qhtqCu4z7eC3CZ78S9mDw=; b=m/Bh00dWIqZC7DY506zMsz31Y4NYGcCbBwyQ9V6vlQrXp39plDuJ2fuf6Y4cu/d35r UURa1V31PM2XSWTbN/CP5RKIGsWuJ1LaEkO4b0ZyktOuq0cw44bNYyYr1IuF972ZcyTG 61rxgxAhrsM4UrFMXjlQ1AJO7oEi4OmuCUMNY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722260075; x=1722864875; 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=tvWuoWcBz1GpvEecxVauV4qhtqCu4z7eC3CZ78S9mDw=; b=YjnnXnk7sJG2XJG0Sk0FqDmyTFFAATfuONYUjidu92XZisZdp0leIn/AoIcnQBgXKj gz5Hf38ztYvHMDLEx31b7R74T3Q6WO8Zo2MWA+NpkgEq56L1wUp/BGpONGSoY7f+BkSP XvcNYCTMCZ06LCQs5ChWlSY8llwz5m0H8pufOo6XUTlfEgbQlPhocCDTxwiFycVuZhHV 6RiVmVoo9A6iGpEf3dqPof5Q65ceazOMMHOZLrvBm8m7VX+VW8EHIk4xZjy/ZSjTEwj0 Ay92EYrlr5cxAPsi/BxXoqF1T0GmSZHnX82/dOxZ7lzFbxOchm88jCmXwZh+J0X0SoUA T5Ug== X-Forwarded-Encrypted: i=1; AJvYcCWD8J1LQvTPeipdg9Kkzlyr/SSrsYC9DdU5LFNdEG45VFo5I6HKh8HCJwrggRQVRENh4JGlTIF8k18fezTwQCbB6W8= X-Gm-Message-State: AOJu0YzPV6bl0B/QdxjBEWVcYidhbv7afjcRsUArq4ID6cmCzuy/13ow KwWE4AQ/SZcZEoQDnlYHyC5kieNFSLZf0H/G6ZdalfRka4QHnILr/cJQcpCp+UVtjE6Ggm3PCUL vviMBnCgGyo/W84d3ER/fC+qgmZ4tkNO5vIal+w== X-Google-Smtp-Source: AGHT+IHL1B4yv7dqzfE+at5TpW8qgzJgJa+v3PgxakxDfs21btY8jOQ0oemCyPqsr1+gTZTemCP0UAlXrEYN68qF1k4= X-Received: by 2002:a05:6a00:807:b0:70d:3587:c665 with SMTP id d2e1a72fcca58-70ece9fc3d9mr6189837b3a.2.1722260074510; Mon, 29 Jul 2024 06:34:34 -0700 (PDT) MIME-Version: 1.0 References: <20240724161942.3448841-1-davidf@vimeo.com> <20240724161942.3448841-2-davidf@vimeo.com> <5xlwzzz3gs4rk5df32kfh7fx5ftj3a4iwryqxdb4c3oniuehwk@d5kum5xr4uw6> In-Reply-To: <5xlwzzz3gs4rk5df32kfh7fx5ftj3a4iwryqxdb4c3oniuehwk@d5kum5xr4uw6> From: David Finkel Date: Mon, 29 Jul 2024 09:34:23 -0400 Message-ID: Subject: Re: [PATCH v5 1/2] mm, memcg: cg2 memory{.swap,}.peak write handlers To: =?UTF-8?Q?Michal_Koutn=C3=BD?= Cc: Muchun Song , Tejun Heo , Roman Gushchin , Andrew Morton , core-services@vimeo.com, Jonathan Corbet , Michal Hocko , Shakeel Butt , Shuah Khan , Johannes Weiner , Zefan Li , cgroups@vger.kernel.org, linux-doc@vger.kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org, Waiman Long Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: 1D0D0180002 X-Stat-Signature: 64du54rfqrgamssgzswfopz7btfaza33 X-HE-Tag: 1722260075-925306 X-HE-Meta: U2FsdGVkX19uoFEq/LrimoqmZJvzvMDztH846LXafXb9MQan11R45dj2X5Gim5g3Ryd62L9JgQT06yYI4oLoRaXC7N+HsIpi8sLOgu1MKreywlJTEgnrkRsat6t3KFkXWg+g879W0li7yW3E6v6w4+b/eGnSg3XZ0xsfajTponAHVnI+xj/N5wiytNMfMIK+lI5lO3iOvwzMPx5rJ+Fh8YpVwyIpom/RATV50AJh54zZvZTG+UjDV63KcWWkpRmyyCMHnS7LIDTrzgTKVl+88j7u9A07G5SVF+I/P5+thHup+Oge3TRP2DXfoWzYKuyLLeC9uk4pspqF+KTFoogUtpFcgp7iflKEaQWOW53fgzprSYKdVIKwuD5YqRiUAFXTAUM+LM+2hBqJpvZstNiyzvaNEs+PWCOHGq4T8Ct8SB2d65OQrZwawRfz3fOUsIinAjhceDSIor40KxB8wK/qByt8DzIOT/h1jufOzsNDZx06B3qpCAoPRWN2er2wJyL3OGgoWZvLtFd1sztjzNYOmZDbU6N0QoBaBfXR9+MPc4kJhGHZuVHqfpUYHU1to6sEdfGaNjhIdM2mQIvS/Oqlh+9KtSIXWtw/DFpLQhIHW6IsuTLT6u3NZ6taIEc8yJFdnkKOx1+LnMKXrUdD/OZQYJNw/G/NeGu1Sj8+Ay1SfxBvK6AJ2ryuaOi/9Y6tPQNKhc/oEfImx8/yEofjiq/b5tIc13EYu3LKQxCw7rnnCz4qV/sNRAu7nrVZeh7F00XiYGEsZgQoWRDQ4DSDT19cLMC+/jOtRuxqwlCpX9QsJa6/XIZ3d4fXPbilKh5ofG+mfkzwM5r81EgQbadWc41rkc+r/JHUCR7zcAl3jzn3LX/o105AGZwvDgiJvXSI4xijvSobjFEV7HWpN3PLjea5lIQdVGPLX8TA5uPgGCtxefNtvVNW5+ppIB2O9qn7SxOkhqdHuO2ujuLDHTRfWqX 7alx+yaK AUyPdBvyZJUh+Uwh8UE/jMw/81f7213v250ByiZuo2GRdM2MuUWVj5v2meTexkK9YTOe0h4nHbDWUicrQTvyWRIYXDvolcYjKPuR7ByH7BNE3v8hR1u5Jw7a/zBuMuiLDno0qP0qfr6gNem3fU0rNrJs3o5MfL65hp12McSgjLwi40Krn3PVc9zLp7Dq1bxiSAqlQ8yxt6zSGWZVQDr/Pfvr3Q02RykbO6cTenmrCnVbIP+idOPy9U2KCmg== 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: Hi Michal, On Fri, Jul 26, 2024 at 10:16=E2=80=AFAM Michal Koutn=C3=BD wrote: > > Hello David. > > On Wed, Jul 24, 2024 at 12:19:41PM GMT, David Finkel w= rote: > > Writing a specific string to the memory.peak and memory.swap.peak > > pseudo-files reset the high watermark to the current usage for > > subsequent reads through that same fd. > > This is elegant and nice work! (Caught my attention, so a few nits below.= ) Thanks! You can thank Johannes for the algorithm. > > > --- a/include/linux/cgroup-defs.h > > +++ b/include/linux/cgroup-defs.h > > @@ -775,6 +775,11 @@ struct cgroup_subsys { > > > > extern struct percpu_rw_semaphore cgroup_threadgroup_rwsem; > > > > +struct cgroup_of_peak { > > + long value; > > Wouldn't this better be unsigned like watermarks themselves? Hmm, interesting question. I originally set that to be signed to handle the special value of -1. However, that's kind of irrelevant if I'm casting it to an unsigned u64 in the only place that value's being handled. I've switched this over now. > > > + struct list_head list; > > +}; > > > > --- a/include/linux/page_counter.h > > +++ b/include/linux/page_counter.h > > @@ -26,6 +26,7 @@ struct page_counter { > > atomic_long_t children_low_usage; > > > > unsigned long watermark; > > + unsigned long local_watermark; > > At first, I struggled understading what the locality is (when the local > value is actually in of_peak), IIUC, it's more about temporal position. > > I'd suggest a comment (if not a name) like: > /* latest reset watermark */ > > + unsigned long local_watermark; Yeah, I had a comment before that was a bit inaccurate, and was advised to remove it instead of trying to fix it in a previous round. I've added one that says "Latest cg2 reset watermark". > > > > + > > + /* User wants global or local peak? */ > > + if (fd_peak =3D=3D -1UL) > > Here you use typed -1UL but not in other places. (Maybe define an > explicit macro value ((unsigned long)-1)?) Good idea! > > > +static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size= _t nbytes, > > + loff_t off, struct page_counter *pc, > > + struct list_head *watchers) > > +{ > ... > > + list_for_each_entry(peer_ctx, watchers, list) > > + if (usage > peer_ctx->value) > > + peer_ctx->value =3D usage; > > The READ_ONCE() in peak_show() suggests it could be WRITE_ONCE() here. Good point. I've sprinkled a few more READ_ONCE and WRITE_ONCE calls. > > > + > > + /* initial write, register watcher */ > > + if (ofp->value =3D=3D -1) > > + list_add(&ofp->list, watchers); > > + > > + ofp->value =3D usage; > > Move the registration before iteration and drop the extra assignment? My original reason is that I could avoid an extra list hop and conditional, but at this point I see two reasons to keep it separate: - We need to reset this value either way. If it's already been reset, it m= ay not get reset by the loop. - since these are now unsigned ints, -1 compares greater than everything, so it would need a special case (or an additional cast). (Assuming we're on a system that uses twos complement) - I think it's a bit clearer this way > > Thanks, > Michal Thanks for the review! -- David Finkel Senior Principal Software Engineer, Core Services