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 7C7D6E7717F for ; Tue, 10 Dec 2024 08:15:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6C2056B0132; Tue, 10 Dec 2024 03:15:58 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 670E46B0134; Tue, 10 Dec 2024 03:15:58 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 511916B0135; Tue, 10 Dec 2024 03:15:58 -0500 (EST) 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 2E9646B0132 for ; Tue, 10 Dec 2024 03:15:58 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 85D771205F6 for ; Tue, 10 Dec 2024 08:15:57 +0000 (UTC) X-FDA: 82878340314.25.9B87B75 Received: from mail-lj1-f181.google.com (mail-lj1-f181.google.com [209.85.208.181]) by imf14.hostedemail.com (Postfix) with ESMTP id C3E4E10000E for ; Tue, 10 Dec 2024 08:15:31 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Liz7z2kT; spf=pass (imf14.hostedemail.com: domain of ryncsn@gmail.com designates 209.85.208.181 as permitted sender) smtp.mailfrom=ryncsn@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=1733818541; 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=yDNr0SQs7JARf1tXnEFLADDYHcO1kynEBs5jHdseHtQ=; b=bkyl+KsRl1HVqAvV6O04ksPzJlc5Cx0NKe3NhSFlXSbAVOOIjjv0ud/k1cK2spaOcPnYm/ Tr8sK4HQTBd8wGMHSd8Bye59jsFv45MacWto+sp4MjCgqzTDA+Vrm8RNnVkkPIAClf2j5d ywrC+0oa08/MXqR/IuDa8nwynTIEvPg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1733818541; a=rsa-sha256; cv=none; b=oD8wTL/pNTE3cBbRpSUcNYKiUxkmFKSDsg53PN7QLPbjqymN0Ov2NpTpQv5n+xpxmu0Gsb yGOIcgt/w4TZ/rIaamebGyWx6D5HTS0/ri6rXmuqhA2mY0OaKhi9abZq6+27dJ6bc3GOD1 /VJ78loOV0b/ozQFjDnT0jN4lIkSuGE= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Liz7z2kT; spf=pass (imf14.hostedemail.com: domain of ryncsn@gmail.com designates 209.85.208.181 as permitted sender) smtp.mailfrom=ryncsn@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-lj1-f181.google.com with SMTP id 38308e7fff4ca-30225b2586cso15959851fa.0 for ; Tue, 10 Dec 2024 00:15:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1733818554; x=1734423354; 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=yDNr0SQs7JARf1tXnEFLADDYHcO1kynEBs5jHdseHtQ=; b=Liz7z2kTM5rVjyCRX0wehcobTIKndBABjsr97jZfnJ4uy8lal0eMEmqu69CjRYVSuY UZcM9Y7FgpkU/WZj6IyEAWDNFG+HAvimxrFjp1Fb1G6sbZbYZZ1VdVo0Dhl1jFYyzPUJ Ga11eamBQoFCY5AJNT7XlqKmidKNz+8qzBcEMX0+JOp3PyjsqV4G1t/mc965ZrjQl6wk H0S6IZG1rbO5XF158BiVtwU//sgd/9eIwYOAy32nC25bBFNPw1scFOj1JPGCfbAT/ZXE WZOmLykcRFlMnH/P/SVW2wSo8v94tt55AwN90xGIV41wuw/aewWY4Zt9/ppQEEz4k5EX jy3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733818554; x=1734423354; 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=yDNr0SQs7JARf1tXnEFLADDYHcO1kynEBs5jHdseHtQ=; b=sgssdmEO7Nq1nagqtpTQ/vbJL8pHIvS851qV7/SgyGj6xYZzXmEMXHzfQJqcl++d4y IufG9WDOBOAAd44CnK5c62lFYBFDMkDsRzKZEoJ+e2JLkedt5iEZvPHST4nFNQofjsIq wVOMnEqsD4XjDpgz4qSz4oEhxLs6/icbmlZy1/i44jg0Ov4bS80tun3n39Otj+NLAA9F PD7J4X6yRQFS6qRY8pxSn6C8h8GGyyQo6nVqr9iigkDouy59u+n8zQxcUesogNN/10n/ Lsl7sg6rybIKEsB0F2QQnXBbHP4SNkY+cmvkR98sYahHj/q5UxMcuJAnjPp0np0UtdCS abiA== X-Gm-Message-State: AOJu0YwJhEluKHUJFmQW1Ibiwafwlj8diMzcpMbYoE/aEp7rAENalceW OvAxkPWrFFihlfVL+QZFRbb0kDPbDpk1mJcqPhiT9/dd6OajSkaFWDT89ISb7kxrYxHTiS1/K/G 0U3e3JgI3UDMSKqTQWU0ERGWk6tM= X-Gm-Gg: ASbGncvdvknwfneum7lyUlbupfuGrTCwFWo6zBrIgqBsVYGyUTQjC/0awXZUv3nHBmn m9iUP+twqOxwIAEGoN8XvLzRguqG1ksjDeG4= X-Google-Smtp-Source: AGHT+IEzneTwBmpUpltRe848A10lZV7XqHl+ipUeEsWgFCV/zsCiLt5v8a7s7FlvB0Q2Guoi16YZq8WwKm9pGrze2W4= X-Received: by 2002:a05:651c:1986:b0:302:2097:392f with SMTP id 38308e7fff4ca-302328381d2mr8186461fa.7.1733818553302; Tue, 10 Dec 2024 00:15:53 -0800 (PST) MIME-Version: 1.0 References: <20241202184154.19321-1-ryncsn@gmail.com> <20241202184154.19321-4-ryncsn@gmail.com> In-Reply-To: From: Kairui Song Date: Tue, 10 Dec 2024 16:15:36 +0800 Message-ID: Subject: Re: [PATCH 3/4] mm/swap_cgroup: simplify swap cgroup definitions To: Yosry Ahmed Cc: linux-mm@kvack.org, Andrew Morton , Chris Li , Hugh Dickins , "Huang, Ying" , Roman Gushchin , Shakeel Butt , Johannes Weiner , Barry Song , Michal Hocko , linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: C3E4E10000E X-Stat-Signature: nfi6zydumoc4t9r7ih1q8mp5so1y8sb8 X-Rspam-User: X-HE-Tag: 1733818531-699983 X-HE-Meta: U2FsdGVkX1/VNa7j9zqAK+MsFYRiK7YEVsIYC6ke6T0AfSCK1eDRPHo+REt0/P3KrijBX5XI1UlSvM7e4Pv3HRSZ85oGwjGobICy37h80RcjJfMBu0SAnkUb9M69R2+AtepC09BnFwiJRN7lqx1RWjzKCTsYQXk9QvhD8r9Q9qsVrXqaL77Tni8rzXPhm3Kf2fnqtXrmUPOd+FY14Qeg/iLGMFBNwuHVaNgwT2qxYksMJN1FJMD8gbUEyHLRXFSS2UqDEJx4zeUi1CjEcfYr6PFcYzNv1qHvTTz+zWai8rKNPt1hSBNa32dnBqDlrf5HIKWUFb960BMDRnFylHWwwUvBcaqzT7wS1WAn2DY5qzeeTFOCO12xKEmxPf6RBHT7f2C18GpVdZeTHP2bLtCmUBokoKQkpA5szuPAZZVa032HDIdGBSbOxgbSnQOWT9SUZRiu7umKya2fAkiDggUeAcTRocxNG+mcUNvzfZbILiyBFQTH5zuXDM6lh+e5NjxGemg19Se+64WQx1uRsmA2YeasJfbjPSOf5IpO7MDi+UbsuEGTV6tQug8SwlQD6nbvSo0SG7AVTr1Luj4jvEixIUalXlJZDSgb+eJEXmT3WQ/62n1pV9tKbMw8bHg+BGIoH2ELloeuO+F8Nb0K2R6gjoPEx77MVk+ShyMjWjRSv51x9OtsbJTkenY9kYPnIzAoykpJXpr31w5RwR0qtGK7mQLiaMJGgx7DCxe4hdqsPPw8bXiBo+7JlhalY4T59TZ9v+01aeKw71QpQ5oJOXl0XtL79WPo2OMWAzeUfIGPaVKRvQQ1qPmeeI5Ze8qBIYUG2v0esMU5IH5LF8I8q7JbqGt9zWC4ppr4B8JaXcBcX8/xk3l+nfO7s/lWskeHLsjbnT+tnmIrOFkIOuHgZkYQ8BlQN0fNhv765+XmjE6uYD5rTrArqcmm13CP6zGJ3C8l+u6y+gF6GT4e+8dznyr oGvzkwKB M6w4euonuEsaEFt3wD+muX9s2xVFbtoPASQ2RjqYDAJaGT5n7MXfmqQVnh1KzZzN5skzMrRqmGjhVVKYvUsVKLI7AG8Qox1lsLyNukuaBxA/wG88LcuWOoaGgk4G2hDasWx3s9QEZDyVyh6P/VYSgCJkLtzCcsvjgB9i6qYa2kCpJg3RedmbFAdWU8NsU79IGr50NPQkC6xINzl6ffPDnDELGqqrzwM04IOkRtrB7i3+nNku49H7je+YYmc/rKvS5Sa3y7DWrvqnKueZyZgMP5hvKeNivBf5316gSGN6KRDzsnYGBS3G+r+EZZdNRryKbXW/Df5J/skF3cVzo3nRKd9QPWHRmZZF38xDOkveWFN8FQ1oTL+oMSsikyRqYQGA1V2mamI1Tw7CWHt9VAxKkddDsa4JvOeK0MfLIdsUMWnfZxSz+QzxUkv49aA== 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 Tue, Dec 3, 2024 at 3:26=E2=80=AFAM Yosry Ahmed = wrote: > > On Mon, Dec 2, 2024 at 10:42=E2=80=AFAM Kairui Song wr= ote: > > > > From: Kairui Song > > > > Remove the intermediate struct swap_cgroup, it just a unsigned short > > wrapper, simplify the code. > > Did you actually remove the struct? It doesn't seem like it. Oops, I forgot to drop these lines of code indeed, I'll send V2 merging this into patch 4/4 so this commit can be ignored for now. > > > > > Also zero the map on initialization to prevent unexpected behaviour as > > swap cgroup helpers are suppose to return 0 on error. > > All the callers lookup the id of an already swapped out page, so it > should never be uninitialized. Maybe we should WARN if the result of > the lookup is 0 in this case? Yes, just a fallback for robustness. Lookup returning 0 is expected in some cases, eg. Cgroup V1 will call mem_cgroup_swapin_uncharge_swap early when the folio is added to swap cache, and erase the record. Then the mem_cgroup_uncharge_swap in swapfile.c (called when the swap cache is being dropped and entry is freed) won't double uncharge it. So we can't WARN on that. > > > > > Signed-off-by: Kairui Song > > --- > > mm/swap_cgroup.c | 45 +++++++++++++++++++-------------------------- > > 1 file changed, 19 insertions(+), 26 deletions(-) > > > > diff --git a/mm/swap_cgroup.c b/mm/swap_cgroup.c > > index 1770b076f6b7..a76afdc3666a 100644 > > --- a/mm/swap_cgroup.c > > +++ b/mm/swap_cgroup.c > > @@ -12,14 +12,12 @@ struct swap_cgroup { > > }; > > > > struct swap_cgroup_ctrl { > > - struct swap_cgroup *map; > > + unsigned short *map; > > spinlock_t lock; > > }; > > > > static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES]; > > > > -#define SC_PER_PAGE (PAGE_SIZE/sizeof(struct swap_cgroup)) > > - > > /* > > * SwapCgroup implements "lookup" and "exchange" operations. > > * In typical usage, this swap_cgroup is accessed via memcg's charge/u= ncharge > > @@ -33,18 +31,6 @@ static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_= SWAPFILES]; > > * > > * TODO: we can push these buffers out to HIGHMEM. > > */ > > -static struct swap_cgroup *lookup_swap_cgroup(swp_entry_t ent, > > - struct swap_cgroup_ctrl **ctrlp= ) > > -{ > > - pgoff_t offset =3D swp_offset(ent); > > - struct swap_cgroup_ctrl *ctrl; > > - > > - ctrl =3D &swap_cgroup_ctrl[swp_type(ent)]; > > - if (ctrlp) > > - *ctrlp =3D ctrl; > > - return &ctrl->map[offset]; > > -} > > - > > /** > > * swap_cgroup_record - record mem_cgroup for a set of swap entries > > * @ent: the first swap entry to be recorded into > > @@ -58,20 +44,21 @@ unsigned short swap_cgroup_record(swp_entry_t ent, = unsigned short id, > > unsigned int nr_ents) > > { > > struct swap_cgroup_ctrl *ctrl; > > - struct swap_cgroup *sc; > > + unsigned short *map; > > unsigned short old; > > unsigned long flags; > > pgoff_t offset =3D swp_offset(ent); > > pgoff_t end =3D offset + nr_ents; > > > > - sc =3D lookup_swap_cgroup(ent, &ctrl); > > + ctrl =3D &swap_cgroup_ctrl[swp_type(ent)]; > > + map =3D ctrl->map; > > > > spin_lock_irqsave(&ctrl->lock, flags); > > - old =3D sc->id; > > - for (; offset < end; offset++, sc++) { > > - VM_BUG_ON(sc->id !=3D old); > > - sc->id =3D id; > > - } > > + old =3D map[offset]; > > + do { > > + VM_BUG_ON(map[offset] !=3D old); > > + map[offset] =3D id; > > + } while (++offset !=3D end); > > Why did you change the for loop here? > > > spin_unlock_irqrestore(&ctrl->lock, flags); > > > > return old; > > @@ -85,20 +72,26 @@ unsigned short swap_cgroup_record(swp_entry_t ent, = unsigned short id, > > */ > > unsigned short lookup_swap_cgroup_id(swp_entry_t ent) > > { > > + struct swap_cgroup_ctrl *ctrl; > > + > > if (mem_cgroup_disabled()) > > return 0; > > - return lookup_swap_cgroup(ent, NULL)->id; > > + > > + ctrl =3D &swap_cgroup_ctrl[swp_type(ent)]; > > + pgoff_t offset =3D swp_offset(ent); > > + > > + return READ_ONCE(ctrl->map[offset]); > > The READ_ONCE() does not exist today in lookup_swap_cgroup(). Why is it n= eeded? > > > } > > > > int swap_cgroup_swapon(int type, unsigned long max_pages) > > { > > - struct swap_cgroup *map; > > + void *map; > > struct swap_cgroup_ctrl *ctrl; > > > > if (mem_cgroup_disabled()) > > return 0; > > > > - map =3D vcalloc(max_pages, sizeof(struct swap_cgroup)); > > + map =3D vzalloc(max_pages * sizeof(unsigned short)); > > if (!map) > > goto nomem; > > > > @@ -117,7 +110,7 @@ int swap_cgroup_swapon(int type, unsigned long max_= pages) > > > > void swap_cgroup_swapoff(int type) > > { > > - struct swap_cgroup *map; > > + void *map; > > Why void? > > > struct swap_cgroup_ctrl *ctrl; > > > > if (mem_cgroup_disabled()) > > -- > > 2.47.0 > > >