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 50A29C47DD9 for ; Fri, 22 Mar 2024 17:12:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CCE666B009A; Fri, 22 Mar 2024 13:12:04 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C7E416B009B; Fri, 22 Mar 2024 13:12:04 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B205B6B009C; Fri, 22 Mar 2024 13:12:04 -0400 (EDT) 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 A16506B009A for ; Fri, 22 Mar 2024 13:12:04 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 81F9E1216C2 for ; Fri, 22 Mar 2024 17:12:04 +0000 (UTC) X-FDA: 81925317768.13.82CA18E Received: from mail-qv1-f47.google.com (mail-qv1-f47.google.com [209.85.219.47]) by imf05.hostedemail.com (Postfix) with ESMTP id 73CAF10001E for ; Fri, 22 Mar 2024 17:12:02 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b="Tmy/v7yU"; dmarc=pass (policy=none) header.from=cmpxchg.org; spf=pass (imf05.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.219.47 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1711127522; 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=AhgKXUbVNsRIrFdcvhPW/fXvU30n5049WUO63MYWMHk=; b=OZfEcudkv4oFbEe3qgRa81EpORL6Bxhm6gStFlWhRBG8JC0IRM5ToZkR/9zyWUraTWC4PE LrhiNLLhR9gFyRf72+in4HpZkP/Zw36CxM+ELH7vGYTqkoSxizY6fDeRDPXOQPMRMeXB0a cwktULHsWuk9dQKmwvQ6yYOXgzj5XGU= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b="Tmy/v7yU"; dmarc=pass (policy=none) header.from=cmpxchg.org; spf=pass (imf05.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.219.47 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711127522; a=rsa-sha256; cv=none; b=lcjLzAeMU4VTE9ws45h2jDoAzLWWGSN8tCygKpUoivpYAdUEElprNUDqHmKFsp1CitK2Dx J9v/+p12VNmxUgbF+AXxiABuTYmgCo8ID0i3n0QYPe57/yXsvwfAGlk/sXUj7cNspNVjiy 3673pYiEClQ4W7mtxDSVkNbKzf+IO3o= Received: by mail-qv1-f47.google.com with SMTP id 6a1803df08f44-690de619293so15368106d6.0 for ; Fri, 22 Mar 2024 10:12:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20230601.gappssmtp.com; s=20230601; t=1711127521; x=1711732321; darn=kvack.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=AhgKXUbVNsRIrFdcvhPW/fXvU30n5049WUO63MYWMHk=; b=Tmy/v7yUGA5YkoCJTwWLp/KmSlyEKbwNTcuCCSUCJTknhJwOYfcnOyYDKOulJg7i3+ PhxSLD+YnQeNa/tU96upsH3FaX/Uf1cCsVoJz1hiRcNxSAXtJrc9ZtFpfOwoRZKOQ8RM jqTm9ly9uxmxvt8JHnjbw3wpoh8a8531uN13sLPxd+ivA4Aw2fYd/fJ5noPnvfIL+x+u x8GH1aDs8Dldo6rj+XTD87XuFqx0O/YgyLtPdnjaxhbqP7K7b4IWixnGrMYBouBsz6ut xB1F9KkCbDhKLjkeqkfnrWhdRQdD9dtThc0g5yxPBV5jxFHMtYCSaRFBL0auk0VTyYvs QVlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711127521; x=1711732321; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=AhgKXUbVNsRIrFdcvhPW/fXvU30n5049WUO63MYWMHk=; b=TLypqykk7tQgWXx5VEFIfSo3ycj8RcP1h/U2/C798wKchAL9wXZT7hP9/FGQJs7naI +oyewuSm0/e6btxDXHY0ES6cXCu+up2b+SlReIzOiRrkoVyH+JDE6Eqx+AwZPoHyITPZ IAx3TFfZNhourwkw996G93kaLVr95rEDE+VNpiFwiGg/xFzjIoKL4Op/U/29PoCIVZ7G cI7ECS0PmQvr33Dtvm+tdGa1SZp3/Murpu2czp5pe98GaaHBGBCLRpbmDzn4OCIoG+15 pgksUuknDtEmPdLDeiwWpJ4w2fqhmVKI3Hj0uKkGVUqUORRe65tQfEjPIZqpIOeKq2Jg RwUw== X-Forwarded-Encrypted: i=1; AJvYcCUO/eWSLMGEe1BBfsuQHpOvqPnNLcxrCCQMG3JDfaAmf0B71ddXkILRCFlpKSAJl9F2/YYz8Zxd1/Og9QmSbZq6Bxs= X-Gm-Message-State: AOJu0YxRLU/uVxTAeqRSycHr8I3ZS0jaYNlrRyD6qYO+vXr7G7PuADkP ZZmrNf63FjgGrbdPjT1TopzEJvz0RfcRHCpyKL3/YQrNfQyNr4FVyEtDgGAS4HM= X-Google-Smtp-Source: AGHT+IGagVLORI8RsabPqhy5Z9tIk+Bm9PW30xqfpOL0RjEQw3cGOQSh2YuEBEt6E2gvn2JnlObyng== X-Received: by 2002:a05:6214:2525:b0:68f:e779:716c with SMTP id gg5-20020a056214252500b0068fe779716cmr3486339qvb.63.1711127521539; Fri, 22 Mar 2024 10:12:01 -0700 (PDT) Received: from localhost ([2620:10d:c091:400::5:16be]) by smtp.gmail.com with ESMTPSA id g15-20020a0562140acf00b0068fdb03a3a3sm1250906qvi.95.2024.03.22.10.12.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 Mar 2024 10:12:01 -0700 (PDT) Date: Fri, 22 Mar 2024 13:11:56 -0400 From: Johannes Weiner To: Chris Li Cc: Yosry Ahmed , Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Nhat Pham , Chengming Zhou Subject: Re: [PATCH] zswap: initialize entry->pool on same filled entry Message-ID: <20240322171156.GC237176@cmpxchg.org> References: <20240321-zswap-fill-v1-1-b6180dbf7c27@kernel.org> <20240322031907.GA237176@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspam-User: X-Stat-Signature: itjtyk9k39y4pt17uiqmhi6z3u19y1hw X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 73CAF10001E X-HE-Tag: 1711127522-698091 X-HE-Meta: U2FsdGVkX195Uss7tCCOAGyD72n2XZw4Hmbfe+PWBELhAKAh/E6qnlaUGGG8S5WihUzw+jMNoI3NIGAvO2pnN94S0PvHgyGidCn1x0K9EGlyzyfSzczFOx1d85uwmFMZbPs6Ghc8VcsUXX3JX/V4mfHOG0n+gkgx+68T8jXbgfRy8k7Kugt+lkYXil8ldnSkqp2UZsc8HZgHI/p2wc3tFo1Zq6DpG4AP5lnQwAtDjTeEr9pngws1i/aIoCNJo1MzQxt9iLWb72hZGobxRDYbK5rhIgblzWomidBL5AFUEytnScy2WGQVdNYBb+AFIKFsiHdh7SdqWUciwkklOwu9FFAZnT28CuTmTOPoQlnjJFE+F/mv1J3JMtovvpabo76eHwkG4jQFcgC16alMCI9Q9+9doCuKWv7ohxCMVe46tE59Tj17nvzctVfOSH2sVqa9bZYirlsG6Ekiv9ei53tLA9HZb4fbWNar+oeuXvf/44vpwUPA5wFt4j9WPc6X6VJDCaCkKzeGEzINk+9CifsaMAbieNnTHcNtNIbF/cKm1LpfWy/EHrrDEqPxndfFZEy7JPzJNL17wdgRz5ogadm8d9Yy8twt2sAsWvvnrbdyHdQSNyZ8fBYhdPYy9xCuMiCOVnS10wdx5Z4cNmO2jIIjgsZ5kUJY94Ymi4Tbt3HJ/jbR805hazPg6ZP6GMNKjZjbVbSg499lu96BqDHnqCgeGOXrmhZvHHaRuF+hPWwdDUGZmiC0o+8VZ+8eRF3GMyIEzu0Fjm4qPZMgh4ZWtRd5LZ5oMX3NJz96yu50RB5BQ02y9cZZDCk7oLmLVwo3UG/IDdeFp8tXaD8IS6wsa2E73veNd7uvcmZ/YsfeGjHVOQmu/vNa0Phb72BcM5wD6A0ZkG/5I1aC6Ap01OWcwYRyNyjYDIyJOodQQaP/n6KdfYq5jC5wRmQ7jdj/7qjFt4IBSYsTqpqfZg1h0keJks0 IWGMSwpg v944Dhh1w3QlNlruzF1r5iUWXgyoZMke+N51KPFaxQ1NneIXGZLYF4BV8QswMVpi1FkfQBEJYIH0AU4150PmXEZEGzZkdhDXwztipUgtR96ZtcN/puipWKUTyCFKrCL47WTIbdFBaOML8hVf1clP5KGOb5YapO+r2UR/ZKgj6txqPVEtlRtGkLHI+G6YmGJB1dQiAhAN+bRy3NYvC4tQuiAVh3wNv7krV6rFTDrkGv28tReNPTqO29Pk4aZNQpZ4cCugibRhgETbEuE8kiJbtZBo4WzAoi6PYP/5/Y0x2AiP2Dw3zkxZU+RxnxdwBaSA5OSXP6ra4M7PPXD68Bibr1V1E2kF6ElhKwv55OeF43Mki1OP5paUhxPEm6E9fCnr0gXvByQ0+5omuTW5+sDwE7KUhK8GxYCpTlvy6Hs1sYWk6CZDv9ICyOThj7Jvdul8YVFPTvDG3NpOh8HEWCrnEgAkDrQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000001, 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 06:35:43AM -0700, Chris Li wrote: > On Thu, Mar 21, 2024 at 8:19 PM Johannes Weiner wrote: > > > > On Thu, Mar 21, 2024 at 04:56:05PM -0700, Yosry Ahmed wrote: > > > On Thu, Mar 21, 2024 at 4:53 PM 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 = 0; > > > > entry->value = value; > > > > + entry->pool = 0; > > > > > > This should be NULL. > > > > > > That being said, I am working on a series that should make non-filled > > > 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 valid > > 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, and > > 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). 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. > 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.