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 D1CA3C27C75 for ; Thu, 13 Jun 2024 16:39:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5C3FD6B00C8; Thu, 13 Jun 2024 12:39:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5731B6B00C9; Thu, 13 Jun 2024 12:39:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3ECE76B00CA; Thu, 13 Jun 2024 12:39:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 1BE3C6B00C8 for ; Thu, 13 Jun 2024 12:39:02 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id B17F412020C for ; Thu, 13 Jun 2024 16:39:01 +0000 (UTC) X-FDA: 82226424882.15.2513550 Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by imf28.hostedemail.com (Postfix) with ESMTP id E37F2C0005 for ; Thu, 13 Jun 2024 16:38:59 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=2I61zajI; spf=pass (imf28.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.44 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1718296738; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=f0+hUYfcg/6wCMFIjwJOq9go/0av2eejFWyVyZxl9Js=; b=4jkfj2IUcu6nShc13h8ELoIP/QNXBwiEcGzynzqho4Fkcxvk5ePZVeQYC4In/YbV9GP5+C 3TAkk6nhDLO6+NBVFJkTKR6e3iOjXBczBFaEGo7aRMPvW4iNIWFC4oyPHnb/JoE3KCrjUB XiN5ctxDADS7lfrSbGTNZ9vM2S0e8Nw= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=2I61zajI; spf=pass (imf28.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.44 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718296738; a=rsa-sha256; cv=none; b=3SaG8lLNSTIxlQl+qso2p4O9lbMtUlvngBAa2JX5H9AUxE2XiYur2dLIwKLd30pnFt6B6E U59xuLJ8+UVlJSqd79zgD2dBSeNDvvuqbEDpVM3rQQDELiAxGOlnTHLiaqVzmnvnVQwh+r P9v878RpULQOe0S2nqIk8DNQfKDe3KA= Received: by mail-ej1-f44.google.com with SMTP id a640c23a62f3a-a63359aaaa6so178830666b.2 for ; Thu, 13 Jun 2024 09:38:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1718296738; x=1718901538; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=f0+hUYfcg/6wCMFIjwJOq9go/0av2eejFWyVyZxl9Js=; b=2I61zajImu7Jt9JL9vsOik+/6/mAhMpVLgloHLR4kokN1loeMolv3UlghEQWNTgVAQ Z4kyVK/mkXj6NH65JPe2NAmyEbUjsrkga/y31vhUV36gsrvrDYgx3xbWoEYEpIs/L8z5 OukA6fyUSECi6OewBmLizpeXAF9TmMZCs67icmKtNY6meeoNzUrvGsZsDVPo/LamtA4p e1yu/khFAATt+BL6mbTQOb13VXQCvC6UGyhMesHDZy7zMdNP2ACqNPGGgFtxamPKkwZ/ sXevCYrHTyW2yLE9nBblWBREa/4xewG8pDA4/r7pHtxRDRgK3S5yodoVMFxPkbOQ40zB nflw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718296738; x=1718901538; h=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=f0+hUYfcg/6wCMFIjwJOq9go/0av2eejFWyVyZxl9Js=; b=e03MZOZImzkIEPM4J0FG01+yoaks12PkDO55T8XYTWu+zaIsiOL0VtrZiZQC6OWS2f ycUqbe2H9n8btXAlE3z/XpjRoVTYXBVnNBP0AAwpzSTXeRLLSwJvlO2gJ6b4YQhnJ3cB bmZjcnNwa3RwxbLVLRjCflyXW4/uCK+tAgOM8HB4091FhrewGdP5xE31QRrAd27OR1qU bR2qBOU347ZdClpLPy2FO8MlYoCVEGfeuoNk8N/pqCUBha6VU6d/1885gJWrc/n8QRi1 nACEUtOBsyT6zRQoFU0DF9ht2eHLXS2oEwTpzcxOp03PKW4uTTnqfaVPcqnD0A1zWQ1t sK8g== X-Forwarded-Encrypted: i=1; AJvYcCWFvU65s3REy+9i7290JTLLCdTAhxb5msKcSsnk8evyPodwp9Nxr2uQUAxsB2spocyAsq3Dv9E0HkxNFoJfLMOM4fc= X-Gm-Message-State: AOJu0Yw3qrVGm+Ka825HO8qq7tbAu6M135GZNFRdfEJJDLEdk5VIAHbQ wCAnbd1TMEfsbLnEwYhOmUOdfMTGpa2P01GPPMuazKsLboZUVIMHbZBpYoHCoDzajs7x3MZqopl sI1fYdAw6H7C1FSPDcVyScEOs4+rXTxfT8k8Qk3OJjExKVQ4XfxLV3K0osQ== X-Google-Smtp-Source: AGHT+IEvGD4fSm3ydygWLoTfbaCnK79SffQTu/wBgZUu6TR1D/t/B12H5lg4vCBg6YG63Pn4824qOynOJfGllokJ1aE= X-Received: by 2002:a17:906:3ec4:b0:a6f:48b2:aac5 with SMTP id a640c23a62f3a-a6f60cf44efmr19942566b.15.1718296737931; Thu, 13 Jun 2024 09:38:57 -0700 (PDT) MIME-Version: 1.0 References: <20240612124750.2220726-1-usamaarif642@gmail.com> <20240612124750.2220726-2-usamaarif642@gmail.com> <85804484-9973-41a1-a05d-000833285f39@gmail.com> In-Reply-To: <85804484-9973-41a1-a05d-000833285f39@gmail.com> From: Yosry Ahmed Date: Thu, 13 Jun 2024 09:38:19 -0700 Message-ID: Subject: Re: [PATCH v4 1/2] mm: store zero pages to be swapped out in a bitmap To: Usama Arif 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 Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: E37F2C0005 X-Stat-Signature: uyjadsw4r6e1pqgw5gbc3igqumsudepp X-Rspam-User: X-HE-Tag: 1718296739-254500 X-HE-Meta: U2FsdGVkX19S7a8NMJ5Wyo/s93wlAlTJLEjhIo20prF5uX8LHTCVdOwqYZ941BBAZnoxP88TaKNHRy9Frlp194dQTQNjwXlmccFMrZ1HoIKgp5dLk3lcxHpdaLeeAD0sO/GpYdNelj0xzsT755Rwd4C6Juan9kiRVEqfAaqGhcNneTjAo1Xe1XJxT0MxGP2gc4hLyP40Xx1nkI2mpVI8AaM5R56X2eAjKBAyRr4c4W5/+qiYtfW7aYyJGGqeS36De0859f6DhOKkCws/rRa56bLirdMLZdpb2LRfy92fLcUIcnPsMKqDlAnpeGC1S8G5QHOPVmd9o0oLcMUziI2lJZRrSa3nttZ8qHLM28YntZIOsm7bSXioOkkH8MqlcJqfcs9vQ/bLH+8mICRThc99f8xKOwcRWtyhjDrR7jyQ0tFpt98BkhtGmXr0P0dWfLvtFDiqjTDcZS9arQSwqcM+0qwgIErdG8RaU88xoe2lWoHV8d+K5XpJDlj4ZvOZeAyEGi4HuTXQRMdBgbS4k292S9coe5FtxurfOWqp1JHash6F5dp6xirnSL8ndjvPq3my6eKIt0TVYDwo3SomSxENK6SJSyGfz9fvSTVO3NxVw7Tep3NqQrZDP0bb64TJBwz2RjaZNV+/DNdGqow2VjGy4dtlC8lL0F02mAx0CByEweDBdwCw7VIEbyzQ17fQxwVVB/Sua8jE71okBzIpvDG52KVz0vXogBGk8JIE8AhhbrkH8aNvgmf4my6VC37IwoQv5zatUR+kVuaIN51rbKVd/zYEJCxpuEIkdM4ICI8YXiD7vxpD6zYsgzd55HxgmCVTnTqpkljhX3kY+hOUrM1wCvXGfLNjPeywW0h0QzbP4Eobigf0OjONzvQpzzVklQlZ21gtsbxxI7D9G1IAK/9D/GturgsnchtmtZAxAsu+ACp3xQME6BiiYp9CHT9xJ27/Fizw2mUxN2IsrXRXvFG dYs4//6E YxXag27hBKXOVsBNipYn9RLBmOA4HRown63gQHQiwg3iTqlHlr+5llweZPNl6akTdnV83C3wNd885lC8choJllq9ZTXxuWKnnIsHwpQTCqO5n+Wars7eTBuorKZIP8K2NUCxM+chI8rMC+lum/dGIW6/Y/7A4UISh8lCpWzCa9BhV7bc= 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: [..] > > > > >> + 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? > > 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. 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.