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 E3ACAC27C4F for ; Thu, 13 Jun 2024 19:21:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 48CA06B00C4; Thu, 13 Jun 2024 15:21:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 43B3C6B00CC; Thu, 13 Jun 2024 15:21:08 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2B3EB6B00CD; Thu, 13 Jun 2024 15:21:08 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 0B3276B00C4 for ; Thu, 13 Jun 2024 15:21:08 -0400 (EDT) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 8527C140309 for ; Thu, 13 Jun 2024 19:21:07 +0000 (UTC) X-FDA: 82226833374.22.14C1206 Received: from mail-wm1-f53.google.com (mail-wm1-f53.google.com [209.85.128.53]) by imf24.hostedemail.com (Postfix) with ESMTP id 79763180017 for ; Thu, 13 Jun 2024 19:21:05 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Dshxio8i; spf=pass (imf24.hostedemail.com: domain of usamaarif642@gmail.com designates 209.85.128.53 as permitted sender) smtp.mailfrom=usamaarif642@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=1718306464; 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=1UH4DZYDKD0QmUQIdEQDsEgA4q9hMP7Ue0M6+5VHF/o=; b=QSzevrwzOQa24+2x0ynEE1mdyDT2mE4dydLbNQA6tl2AQh54v/QHRu7js9SWMCqISCDVB4 IlGP4Y3z2A549R5XVpF47FoSA6dQULPjLV5vevqx92koOtsKCiOL7d4yft9gA/lx5LKBRg DpFGf8Ucqmh0rEXIs9I1CAOO0AHYVdw= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Dshxio8i; spf=pass (imf24.hostedemail.com: domain of usamaarif642@gmail.com designates 209.85.128.53 as permitted sender) smtp.mailfrom=usamaarif642@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718306464; a=rsa-sha256; cv=none; b=pT8kn3zZC9lWj6z1AgGnXKNMMt7lMeqgSQA3fuHxGQ+Fx4/eiFAfJbsLnh364DjHa6nG43 a+TDtzGes6U7hU0S3D1ukTr+/6+BiyGXx8GT9v9qXGtGszYyZlNcTdbHZeE6exGRew0VXk 8UeS6YajUvbtFcHfeZoKnaR2h5GAd08= Received: by mail-wm1-f53.google.com with SMTP id 5b1f17b1804b1-4230366ad7bso8054285e9.1 for ; Thu, 13 Jun 2024 12:21:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1718306464; x=1718911264; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=1UH4DZYDKD0QmUQIdEQDsEgA4q9hMP7Ue0M6+5VHF/o=; b=Dshxio8iMoxQnhnU04JgcLtolNhjXLMBcXO4A1JBYP2l+L95LItlTh4I7OnWTlfc38 UiSPfaQpjPTUflYpW52/6jerUDzUHCFVHhpVz3JrPkeUuUQGtij3o8uR3jwYlWxV+29h EzwYpsF/LdEMrzEpTg9cYHTny9SD9HILq6J+fAxQwgyFux3dH7PJQSLxk3MWIlma7g/J ZLuCosB4YQ/EhkoC1bzKohT/G6wWoJwVx3mvJPCSN+rckCxpDBZPtMALHm1TAYUpj2fQ hUTQoZQEK8sPSyxDnWvcyepIyKO4aaXl5nboO82PGZBmxArad1t852JpgjoweheBkl49 NU4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718306464; x=1718911264; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=1UH4DZYDKD0QmUQIdEQDsEgA4q9hMP7Ue0M6+5VHF/o=; b=ooqKqWdBw3/m+RgKSOMNdGn4idJ2ibPvOY4ge5budBpureHyt/MOn1iX3fOhPDafNL 5zDQECHQHHZLD4ZzApk2IqTLEwYPm8UyYVVvOu5mu/K3gbkVLfSVS0cJ6NlLKWWJ1eW3 erKZ/nEQVJnh6kga78euwXwyR8fZjGS/YifnB4rBfszGJvoTQkapYBVhqwmsaMJIshjv tNzqTcmCM+0lpbal4qg8YSujt1Y+WkEKlUVjw6ruJ1aV/H6xdb+ywwcXzgv8MImG6WAw TKzPaG3SKaesos18R/y11DoYu4kUzveZ8X8pCpA9XKeSvqzSe0xn2QLvqGpnRWXRpMzy RJsQ== X-Forwarded-Encrypted: i=1; AJvYcCWShjMAIQnkZSpO+iLOiIvnzyPRXlyEhKqsTlagMF81CQ4DbSqIhjeYwmVauAjPlhChVpJzQpyBHibtCVUPwUl4pYo= X-Gm-Message-State: AOJu0Yxuxvy6olgzRMXApfRPfhenTGw3bAW0Z9sxGO9B6ZvYHhKgnUD+ 4/VXwsC8G8k6HmQ+kiynXv5JXKfWxXNLUKnbAIpMKWbAIBDQaTzc X-Google-Smtp-Source: AGHT+IFGB2fcqqSxYH0BeMxq92MYvGFm82w9yP2bjMzUaNJBhxgTTXjvdvNThNo2EJQP/zZyxIuvUg== X-Received: by 2002:a05:600c:4448:b0:421:81b7:785e with SMTP id 5b1f17b1804b1-4230481a0a5mr7936065e9.2.1718306463635; Thu, 13 Jun 2024 12:21:03 -0700 (PDT) Received: from ?IPV6:2a02:6b6a:b75d:0:64:3301:4710:ec21? ([2a02:6b6a:b75d:0:64:3301:4710:ec21]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-422870e95eesm72049015e9.25.2024.06.13.12.21.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 13 Jun 2024 12:21:03 -0700 (PDT) Message-ID: <0572d8b1-3b17-45a8-bf75-f66e19216d38@gmail.com> Date: Thu, 13 Jun 2024 20:21:02 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 1/2] mm: store zero pages to be swapped out in a bitmap To: Yosry Ahmed Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, shakeel.butt@linux.dev, david@redhat.com, ying.huang@intel.com, hughd@google.com, willy@infradead.org, nphamcs@gmail.com, chengming.zhou@linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@meta.com References: <20240612124750.2220726-1-usamaarif642@gmail.com> <20240612124750.2220726-2-usamaarif642@gmail.com> <85804484-9973-41a1-a05d-000833285f39@gmail.com> Content-Language: en-US From: Usama Arif In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Stat-Signature: wr3oq7oc4hce4eof85rrngxg94bx7nr6 X-Rspam-User: X-Rspamd-Queue-Id: 79763180017 X-Rspamd-Server: rspam02 X-HE-Tag: 1718306465-43568 X-HE-Meta: U2FsdGVkX19s+niwFYv9x/BZH+iETfOxCe3Bh/kQa368ORhwSgk98fQslZi1XtvvFFnPmosCq/kbXXsNft+cHDjUS5shG8XMcf0YaCD32vV22xf/1PR8bg1fgGD053npgZ4Cc5mbwGPCwhtDt8k5DwTHpkOwejE9V/5XIRzziDGBjkLj2wxw1aQjzBXS2c5Bl/Y0AdBMXbGeqUvjWar3kPqiS++B9tMa8PlzNi8+FZW5rC6bZEOeUukWM4LQ8IYoLxDw2Tcv5d0RwuyuqbJpPUEJPTjaklipGdEMv97tirVP2UD5OsW8EXxiWHm/cy3jO5WXAviBJFYjuAfDHpB0iGEmf5rxwBmydh1DOkpGIj2WrsHKhYFC+8QZw7Jthgl2rtDT7NI/UmFJgdnnajMfngvo02sKrVgcUFrQfkgcQh+SatfC0Ly8h7D4bXKzH051pXK74ovc1AYoIaSaec6DJKYRLGk2/OCbYBvm5N0MYyXrbvXyy1gR9Jp/eaURaRfqxZhAJ6m2++jhkUbsUuGShGPKUcyQ5ztZPUMgUo07jnhlVnH4RA8yb3uwjhQ+n+fDUnFlr+zH+IM3GtxYLgACToB1N/tboWw+Vav+2qTbJfakvRd73prBGQdgg/bSVABjlYrxhTJdowda95fcpAf67+Y1nWkDFONSsU8/wngsKGumN6J1UvoW5C4uLSacGbZ17lxzE6nj9o/QcFLdKbkkmR/AyK7DpM7O0M6gUagMH5hvzUyOKizqpu7Nz+keYsRayKG8LugRhP/pDm2DUWjHHOG+9KBVkhzu/mSHwUUwR58AVcYfS3jazs2wAO6OFUSehMupStRSsNNIWIJzeA+oiEu7ZEZVAFgh50zMv8x9M9yocQtTTMwA2d4H7it487N9n/XKQ7L9FRjZqQ/7xSMs4ogCDO2m+B9ZZiq4L+pASzOKA7f8NcILnNL1fCsUwNidxUMqnN+TgZ9kwmKMtF5 C4G05/1M vQ33zqGQgtmfJFDaNZFYuXc1aVEMwNu2nTGw+vjnqmEE7brEtvSh0oT2Q/2adjTYBa1H4cYExSlMPrJ6vMqX/OzyVm0F2TmHlyZKM/JFFkuX3TqGhUgC8SnHz333Bq0X89vtluCYKqIY6Ou9zs+ssL7j2vfAtLfXWIe8AS8ukoDRtlfV8q/VG7O1oS8wrEGD0XD4adnyDNJX9okJaTO64+Qzj3r3Ck1gtUgij0fX0iCeDam0boICwmyNoZK8PmXFpcdKnenwcRirIhENgCX3rnvJXYLq26FpLHynCDfHXG7/saE4VSJ1ilvw1VskEgpDGvtjq+8TZ+zkU6HMA5ty/7P6GeTMaRXOdslvGEyRtqN3WMG5V/DBVtBNxKhjrQB2wMSe3 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 13/06/2024 17:38, Yosry Ahmed wrote: > [..] >>>> + for (i = 0; i < SWAPFILE_CLUSTER; i++) >>>> + clear_bit(idx * SWAPFILE_CLUSTER + i, si->zeromap); >>> Could you explain why we need to clear the zeromap here? >>> >>> swap_cluster_schedule_discard() is called from: >>> - swap_free_cluster() -> free_cluster() >>> >>> This is already covered below. >>> >>> - swap_entry_free() -> dec_cluster_info_page() -> free_cluster() >>> >>> Each entry in the cluster should have its zeromap bit cleared in >>> swap_entry_free() before the entire cluster is free and we call >>> free_cluster(). >>> >>> Am I missing something? >> Yes, it looks like this one is not needed as swap_entry_free and >> swap_free_cluster would already have cleared the bit. Will remove it. >> >> I had initially started checking what codepaths zeromap would need to be >> cleared. But then thought I could do it wherever si->swap_map is cleared >> or set to SWAP_MAP_BAD, which is why I added it here. >> >>>> cluster_list_add_tail(&si->discard_clusters, si->cluster_info, idx); >>>> >>>> @@ -482,7 +491,7 @@ static void __free_cluster(struct swap_info_struct *si, unsigned long idx) >>>> static void swap_do_scheduled_discard(struct swap_info_struct *si) >>>> { >>>> struct swap_cluster_info *info, *ci; >>>> - unsigned int idx; >>>> + unsigned int idx, i; >>>> >>>> info = si->cluster_info; >>>> >>>> @@ -498,6 +507,8 @@ static void swap_do_scheduled_discard(struct swap_info_struct *si) >>>> __free_cluster(si, idx); >>>> memset(si->swap_map + idx * SWAPFILE_CLUSTER, >>>> 0, SWAPFILE_CLUSTER); >>>> + for (i = 0; i < SWAPFILE_CLUSTER; i++) >>>> + clear_bit(idx * SWAPFILE_CLUSTER + i, si->zeromap); >>> Same here. I didn't look into the specific code paths, but shouldn't the >>> cluster be unused (and hence its zeromap bits already cleared?). >>> >> I think this one is needed (or atleast very good to have). There are 2 >> paths: >> >> 1) swap_cluster_schedule_discard (clears zeromap) -> swap_discard_work >> -> swap_do_scheduled_discard (clears zeromap) >> >> Path 1 doesnt need it as swap_cluster_schedule_discard already clears it. >> >> 2) scan_swap_map_slots -> scan_swap_map_try_ssd_cluster -> >> swap_do_scheduled_discard (clears zeromap) >> >> Path 2 might need it as zeromap isnt cleared earlier I believe >> (eventhough I think it might already be 0). > Aren't the clusters in the discard list free by definition? It seems > like we add a cluster there from swap_cluster_schedule_discard(), > which we establish above that it gets called on a free cluster, right? You mean for path 2? Its not from swap_cluster_schedule_discard. The whole call path is get_swap_pages -> scan_swap_map_slots -> scan_swap_map_try_ssd_cluster -> swap_do_scheduled_discard. Nowhere up until swap_do_scheduled_discard was the zeromap cleared, which is why I think we should add it here. >> Even if its cleared in path 2, I think its good to keep this one, as the >> function is swap_do_scheduled_discard, i.e. incase it gets directly >> called or si->discard_work gets scheduled anywhere else in the future, >> it should do as the function name suggests, i.e. swap discard(clear >> zeromap). > I think we just set the swap map to SWAP_MAP_BAD in > swap_cluster_schedule_discard() and then clear it in > swap_do_scheduled_discard(), and the clusters are already freed at > that point. Ying could set me straight if I am wrong here. I think you might be mixing up path 1 and path 2 above? swap_cluster_schedule_discard is not called in Path 2 where swap_do_scheduled_discard ends up being called, which is why I think we would need to clear the zeromap here. > It is confusing to me to keep an unnecessary call tbh, it makes sense > to clear zeromap bits once, when the swap entry/cluster is not being > used anymore and before it's reallocated. > >>>> unlock_cluster(ci); >>>> } >>>> } >>>> @@ -1059,9 +1070,12 @@ static void swap_free_cluster(struct swap_info_struct *si, unsigned long idx) >>>> { >>>> unsigned long offset = idx * SWAPFILE_CLUSTER; >>>> struct swap_cluster_info *ci; >>>> + unsigned int i; >>>> >>>> ci = lock_cluster(si, offset); >>>> memset(si->swap_map + offset, 0, SWAPFILE_CLUSTER); >>>> + for (i = 0; i < SWAPFILE_CLUSTER; i++) >>>> + clear_bit(offset + i, si->zeromap); >>>> cluster_set_count_flag(ci, 0, 0); >>>> free_cluster(si, idx); >>>> unlock_cluster(ci); >>>> @@ -1336,6 +1350,7 @@ static void swap_entry_free(struct swap_info_struct *p, swp_entry_t entry) >>>> count = p->swap_map[offset]; >>>> VM_BUG_ON(count != SWAP_HAS_CACHE); >>>> p->swap_map[offset] = 0; >>>> + clear_bit(offset, p->zeromap); >>> I think instead of clearing the zeromap in swap_free_cluster() and here >>> separately, we can just do it in swap_range_free(). I suspect this may >>> be the only place we really need to clear the zero in the swapfile code. >> Sure, we could move it to swap_range_free, but then also move the >> clearing of swap_map. >> >> When it comes to clearing zeromap, I think its just generally a good >> idea to clear it wherever swap_map is cleared. > I am not convinced about this argument. The swap_map is used for > multiple reasons beyond just keeping track of whether a swap entry is > in-use. The zeromap on the other hand is simpler and just needs to be > cleared once when an entry is being freed. > > Unless others disagree, I prefer to only clear the zeromap once in > swap_range_free() and keep the swap_map code as-is for now. If we > think there is value in moving clearing the swap_map to > swap_range_free(), it should at least be in a separate patch to be > evaluated separately. > > Just my 2c. Sure, I am indifferent to this. I dont think it makes a difference if the zeromap is cleared in swap_free_cluster + swap_entry_free or later on in a common swap_range_free function, so will just move it in the next revision. Wont move swap_map clearing code.