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 660C2C47DD9 for ; Fri, 22 Mar 2024 17:57:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id ECB146B0087; Fri, 22 Mar 2024 13:57:19 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E7B526B0089; Fri, 22 Mar 2024 13:57:19 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D42F96B008A; Fri, 22 Mar 2024 13:57:19 -0400 (EDT) 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 C6A7E6B0087 for ; Fri, 22 Mar 2024 13:57:19 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 65993816E2 for ; Fri, 22 Mar 2024 17:57:19 +0000 (UTC) X-FDA: 81925431798.10.4E5804E Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf04.hostedemail.com (Postfix) with ESMTP id 6D7DA40009 for ; Fri, 22 Mar 2024 17:57:17 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=WKLQsoOf; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf04.hostedemail.com: domain of chrisl@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=chrisl@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711130237; a=rsa-sha256; cv=none; b=CNVLKy6wqNlcDaShFRsEtt+NygzbtybHYxDbNUeo+bHDidSpSlCiahxQCQ9xuaevYa/VXf W2KZUM3izvCvjtRui212O9cxHHjQRDeF3uOf6pEuVbOU3CwOdXb6VwCR4cEGZiVQmUCXIt kgX3EMFm24GAVl8/h9hwsdUjs0AWuM0= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=WKLQsoOf; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf04.hostedemail.com: domain of chrisl@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=chrisl@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1711130237; 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=7DPlfq7REdv+F5b0f5epp7KdbTEHC2MQyjs/CFlY9Ps=; b=MQBrdQUbtBRGVevDzfDLrmLMwJuUFm2/auSfln4PFEh+bFFmadID3Qp7o/0rn8WE3mljCN wU9MOh6O5gbtz+47C9ST4km1NbgZFLZkI2QpgvAMPzkbVUsJdM1X7s7mlcRuUu+7NFbeBt +y16oMZsw3SIR9Xcit4yrV282yLo3Js= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 60E196147C for ; Fri, 22 Mar 2024 17:57:16 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0D12DC433A6 for ; Fri, 22 Mar 2024 17:57:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1711130236; bh=mM7NWhSY6z5TddPTSw8WVC1hSX82LYfXMQln00iystk=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=WKLQsoOfLM84nHZsCNIBwYx5ELFGnsaCFTZySFMjn7ya3HaXdINPhqWzPiPlhL1uu cN7q28v7TRcPG3NUPGNK5W//Nnf85jIKTExiNeRvN4QZIZkmHUVh/ZSUVw1Fujv4Ca hpCELeIybKK6TWXcpNjV3/vY8+VGzURkBXqbcgvuFZTN/xY8Nb8KNsMjIiCib3KCJi ES+f/kcGGcFLly0khLRW/U5PaKq5E8PtJ9aFclh/HlKT/1HR8cSyOylp8aDOFz3uJl ZaDkZllTlNzOHokcZ1sBsJW6OC07QfkyRAc9iiMdE+GnkORGdFIRwBgq2tpsQ0HMfP HCsuQBPUnHzag== Received: by mail-lj1-f177.google.com with SMTP id 38308e7fff4ca-2d41d1bedc9so44985161fa.3 for ; Fri, 22 Mar 2024 10:57:15 -0700 (PDT) X-Forwarded-Encrypted: i=1; AJvYcCWUEtEpoSEbzpa3g78qlfhtiUH2zmGhwuNTy/dgOaQRw24v233YP2lJK1ULnybTXDdy2li38AHID9+ErXdUGNnTUFY= X-Gm-Message-State: AOJu0YwUKQtRHgaTwATEZUo8sU45Tup1rIZ7zlHvUETLdOCIcCdfmE3M w6uekW9jGFr0M+pChdsjS+yKC11PQvVmq3x/itvGh9IOvvqDNYGl7M7X3A06dSq54i1O0slZeuZ +lubjWTgTJoKl1BEIpOyatYI0wQ== X-Google-Smtp-Source: AGHT+IGg00Z3xXqEO5amjFulED6haeKRnEP8QENf7DK6uJX4i0kQc6vKh+x3JT0k3RCDKeGrNMBsNZv8I+/E2If8mFw= X-Received: by 2002:a2e:80d9:0:b0:2d4:8bfe:69b7 with SMTP id r25-20020a2e80d9000000b002d48bfe69b7mr209193ljg.46.1711130234650; Fri, 22 Mar 2024 10:57:14 -0700 (PDT) MIME-Version: 1.0 References: <20240321-zswap-fill-v1-1-b6180dbf7c27@kernel.org> <20240322031907.GA237176@cmpxchg.org> <20240322171156.GC237176@cmpxchg.org> In-Reply-To: <20240322171156.GC237176@cmpxchg.org> From: Chris Li Date: Fri, 22 Mar 2024 10:57:01 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] zswap: initialize entry->pool on same filled entry To: Johannes Weiner Cc: Yosry Ahmed , Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Nhat Pham , Chengming Zhou Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 6D7DA40009 X-Stat-Signature: f39ngdfe9qtqo6dhdxcqh19ucgndagni X-HE-Tag: 1711130237-600647 X-HE-Meta: U2FsdGVkX1+XbMD1aiN8QdyBCdPtXIu91bfV1sTMlbuhcQNGo0T7ylz1zRwZCOTtZ8F9Ovkam79vtv2CzZyRfkp2pnuehCmX1wB7RVePawRewgnxMADeNT05MJf779BbrwNfObam+Etg1QVYEiQB2cSMr+FRl1hrETcQYC5eNlJVn6bU1Zwm43LlbPd9/Ws+/cifNChBqxY8RBdnIm/2oyANd0kI3CaksX/yt8AXVc6rSXc7j/Jw4HkMlExGaFQQRtMMpMG9OEKEGo4/nmMMm9oYNmXK5v2XYoX0Vjc/EA+L2tMB1dPI33AxxO3V1HR4uLVKOA4YaM7Kg24qBwtpGOIbNWLCcU0GGr8Vlyq7v80gXHwOcJTrNk4KsQ00vztfJqO0CxVQPIPwQh7i9dsQ6/84yzlNHseXS/9HGOrn2i3LIc5TfO0K6BnZq1RMUGQ8eWq9EE3KLhMYsqJ3SFb2euvHC2887AEV86MJ+Vdv+HhFHgOSiNLD3CfGCpX9nqkV2D+9mzRXS8+fq0DKJssCQpqB+LdafRFvcvXKALz6sKI5DMcsJ8PQrKe3ZuAOHd1NAtwBS9OnYKfKOstA+QXQ+jj2BvVY/cRF+CG5dbyE5SrkVQD2K2MrMyZZ5ljI0Dl54UjlLY1Ntl0thIkYhnRyJqh46kzXr3mhHUxm3vXHQ/KpZuc9Dx6nOgR+htRrC1yaf4Jh91yypptTar/JspYTVN0pnTYGtGsiE5MTV4Pirvz0U1a1MpcVcGj9GUCe4fpRwxbkl9QY2iOBmSRueXtXNDUEasDLHuRllUMuBJXdzj+Nw1siEvyuRY5gvAru8pEFUM7hNqRUvwgt5CKyZlV79aia/fhYrUJKSOMoIoHLZbNVH+OpiUmxTJd7YyQev1U9NVU19rPK8GYGNBgyTzZweBylVUxux8W2k62IVpAyY1G42D93ld1yFZZesZyakt0ydpsV0OLe8u3dsDVIfBt Y87Imv7R aDDw+TeQgheqygZ9EywaIe1u9jRbHPZSmUltI5KYEslRZVkV6DXtwKXjeePWqCDsBM+ab+wFyEEEU7JzleanFq1I2YXQ73/LcZNs8Ocse3SYP4A5qAaeoGYM94vlctTuS+i6Qo9KnfPkG8hQVuIgU52iIGB4T/CZvVasjc4W8VPi6wraX/9//Rqo0mDB70WjrsnIOF9HanB9IoAVavkI1CHS1ggZdII1Q5j37kc7ZSgHCWUnok5IAhVvuwJaFAmlzUONB/6m7TftOzNhFVg/Zg19KL/FhywUC8bIRPeCO/mmtpY0frTjQIqYJ3GX/en311FDVsg9hrwWJrZ/D4mtm16sNNnE7Dn+sqKyP1SWe6OW6bmU= 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 Fri, Mar 22, 2024 at 10:12=E2=80=AFAM Johannes Weiner wrote: > > On Fri, Mar 22, 2024 at 06:35:43AM -0700, Chris Li wrote: > > On Thu, Mar 21, 2024 at 8:19=E2=80=AFPM Johannes Weiner wrote: > > > > > > On Thu, Mar 21, 2024 at 04:56:05PM -0700, Yosry Ahmed wrote: > > > > On Thu, Mar 21, 2024 at 4:53=E2=80=AFPM Chris Li wrote: > > > > > > > > > > Current zswap will leave the entry->pool uninitialized if > > > > > the page is same filled. The entry->pool pointer can > > > > > contain data written by previous usage. > > > > > > > > > > Initialize entry->pool to zero for the same filled zswap entry. > > > > > > > > > > Signed-off-by: Chris Li > > > > > --- > > > > > Per Yosry's suggestion to split out this clean up > > > > > from the zxwap rb tree to xarray patch. > > > > > > > > > > https://lore.kernel.org/all/ZemDuW25YxjqAjm-@google.com/ > > > > > --- > > > > > mm/zswap.c | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > > > > index b31c977f53e9..f04a75a36236 100644 > > > > > --- a/mm/zswap.c > > > > > +++ b/mm/zswap.c > > > > > @@ -1527,6 +1527,7 @@ bool zswap_store(struct folio *folio) > > > > > kunmap_local(src); > > > > > entry->length =3D 0; > > > > > entry->value =3D value; > > > > > + entry->pool =3D 0; > > > > > > > > This should be NULL. > > > > > > > > That being said, I am working on a series that should make non-fill= ed > > > > entries not use a zswap_entry at all. So I think this cleanup is > > > > unnecessary, especially that it is documented in the definition of > > > > struct zswap_entry that entry->pool is invalid for same-filled > > > > entries. > > > > > > Yeah I don't think it's necessary to initialize. The field isn't vali= d > > > when it's a same-filled entry, just like `handle` would contain > > > nonsense as it's unionized with value. > > > > > > What would actually be safer is to make the two subtypes explicit, an= d > > > not have unused/ambiguous/overloaded members at all: > > > > > > struct zswap_entry { > > > unsigned int length; > > > struct obj_cgroup *objcg; > > > }; > > > > > > struct zswap_compressed_entry { > > > struct zswap_entry entry; > > > struct zswap_pool *pool; > > > unsigned long handle; > > > struct list_head lru; > > > swp_entry_t swpentry; > > > }; > > > > > > struct zswap_samefilled_entry { > > > struct zswap_entry entry; > > > unsigned long value; > > > }; > > > > I think the 3 struct with embedded and container of is a bit complex, > > because the state breaks into different struct members > > That's kind of the point. They're different types that have their own > rules and code paths. The code as it is right now makes it seem like > they're almost the same. From the above you can see that they have > actually almost nothing in common (the bits in struct zswap_entry). Not sure about how you envision the different code paths look like. > This would force the code to show the difference as well. > > Depending on how Yosry's patches work out, this may or may not be > worth doing. It's just an idea that could help make it easier. Agree, would need to see the actual code to reason about the minor differen= ce. > > > How about: > > > > struct zswap_entry { > > unsigned int length; > > struct obj_cgroup *objcg; > > union { > > struct /* compressed */ { > > struct zswap_pool *pool; > > unsigned long handle; > > swp_entry_t swpentry; > > struct list_head lru; > > }; > > struct /* same filled */ { > > unsigned long value; > > }; > > }; > > }; > > > > That should have the same effect of the above three structures. Easier > > to visualize the containing structure. > > I suppose it makes the struct a bit clearer when you directly look at > it, but I don't see how it would help with code clarity. Just curious, would changing the anonymous struct to the named struct helps to address code clarity you have in mind? It would go through entry->compressed.pool for example. Chris