From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-187.mta1.migadu.com (out-187.mta1.migadu.com [95.215.58.187]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 61AC03E8C64 for ; Wed, 5 Aug 2026 08:08:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.187 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785917330; cv=none; b=A9PA2QcCHovt12rnBIs+zrTXb61OhTIG3/3xhiszOCYeqHDtWu3aNmvvQbeC3rD3wTta3frHPXedWGBgfBSndSyaGIZrbKhGGucf7DmHdJ/iVNuv1ElW63OSiLm88TyOwK4GFrQN3krqurZNYrCOp8LflLn03Lh2D2g9nAsQQ6c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785917330; c=relaxed/simple; bh=qUqdIpxFayzdafXDV7bjN+mZzb5PCQfolFdzg7ehgdw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=nwTod9/scLLwGoTqu8BqvyjPUwPC60sSXV2atzyBZ8JYFlm4DxNRpN51uELbq6ig3IkhCWZcHqWIUo3MTfd9ww+GpK0bA26llAND8VRmkaLU/Nw7F9wwzSl5P/j1Fd8nGepTNfaiw5S58l2k5cO3mXSxBTXZSxVyPlmYMQCQ7lY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=HjR4yllr; arc=none smtp.client-ip=95.215.58.187 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="HjR4yllr" Date: Wed, 5 Aug 2026 16:08:31 +0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1785917319; h=from:from: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; bh=VNTYfYPWVn5gO8iCYhFkdTqm48nYcVoAs5BXPHIVpNE=; b=HjR4yllrAeLtrzLaCEHuwj8pyey5QsbSq+OeatUB6oWNw/Ta0F9V9B4BkD0nkHHaiiaSIC cpkSwFbFWYq0N/KhocsSTBH0VDXwa2NF2HZs7/fiTpAst61oLPfhZ6aClnrGJRXPVEAT4q DyC/toxXkKZgBtoofiiZnIGB0WoTzbo= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Baoquan He To: Nhat Pham , chrisl@kernel.org Cc: linux-mm@kvack.org, akpm@linux-foundation.org, kasong@tencent.com, baohua@kernel.org, youngjun.park@lge.com, hannes@cmpxchg.org, yosry@kernel.org, david@kernel.org, shikemeng@huaweicloud.com, chengming.zhou@linux.dev, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 03/11] mm, swap: add xswap cluster grow via VM_SPARSE vmalloc Message-ID: References: <20260727135029.1059441-1-baoquan.he@linux.dev> <20260727140503.1060918-1-baoquan.he@linux.dev> <20260727140503.1060918-2-baoquan.he@linux.dev> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Migadu-Flow: FLOW_OUT On 07/29/26 at 11:12pm, Baoquan He wrote: > On 07/27/26 at 08:05am, Nhat Pham wrote: > > On Mon, Jul 27, 2026 at 7:05 AM Baoquan He wrote: ...snip... > > > +#ifdef CONFIG_XSWAP > > > +static int xswap_map_clusters(struct swap_info_struct *si, > > > + unsigned long start_idx, unsigned long nr) > > > +{ > > > + unsigned long start_addr = (unsigned long)si->cluster_info + > > > + (size_t)start_idx * sizeof(struct swap_cluster_info); > > > + unsigned long end_addr = start_addr + (size_t)nr * sizeof(struct swap_cluster_info); > > > + /* > > > + * vm_area_map_pages() requires that start and end be page-aligned. > > > + * If start_addr falls within a page that was already mapped by a > > > + * previous batch (grow path), round it up to skip the already-mapped > > > + * partial page. Always round end_addr up so the vmap page table walk > > > + * terminates correctly (the walk loop exits when addr == end, and addr > > > + * advances by PAGE_SIZE each iteration). > > > + */ > > > + unsigned long vm_start = PAGE_ALIGN(start_addr); > > > + unsigned long vm_end = PAGE_ALIGN(end_addr); > > > + unsigned long npages; > > > + struct page **pages; > > > + unsigned long i; > > > + > > > + if (vm_start >= vm_end) { > > > + /* All requested clusters fall within already-mapped pages. */ > > > + for (i = start_idx; i < start_idx + nr; i++) > > > + spin_lock_init(&si->cluster_info[i].lock); > > > + WRITE_ONCE(si->nr_clusters_mapped, start_idx + nr); > > > + return 0; > > > + } > > > + > > > + npages = (vm_end - vm_start) >> PAGE_SHIFT; > > > + > > > + pages = kmalloc_array(npages, sizeof(*pages), GFP_KERNEL); > > > + if (!pages) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < npages; i++) { > > > + /* > > > + * __GFP_ZERO is critical: cluster_info structs contain pointer > > > + * fields (extend_table, zero_bitmap, memcg_table, table) that > > > + * must start as NULL. Without zeroing, stale data from a > > > + * previous user of the page would look like valid pointers. > > > + */ > > > + pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO); > > > + if (!pages[i]) > > > + goto fail; > > > + } > > > + > > > + if (vm_area_map_pages(si->cluster_vm, vm_start, vm_end, pages)) { > > > + i = npages; /* free all pages on failure */ > > > + goto fail; > > > > I was evaluating whether your vmalloc array can be extended to support > > kernel-driven dynamic growth at least (either as a slot-in replacement > > for xarray, or as a follow-up optimization if it's too complicated), > > and I stumble this mapping action. > > > > Seems like it does not take any GFP flag argument, and under the hood > > it calls GFP_KERNEL. Would this be safe in the swap allocation path > > (where you slotted it in in patch 4)? Kairui used a more precise set > > of flags in this path, for e.g for swap table allocation: > > > > ret = swap_cluster_alloc_table(ci, __GFP_HIGH | __GFP_NOMEMALLOC | > > GFP_KERNEL); > > > > > > I'm guessing this has to do with the fact that we often entered > > swapping out paths with PF_MEMALLOC... Is there any risk of deadlock > > etc.? Hi Chris, I gave the kmem_cache approach a try, but ended up keeping VM_SPARSE. The kmem_cache approach has two structural problems: 1. With individually kmem_cache_alloc()'d clusters, ci - si->cluster_info no longer works — cluster_info is a pointer array, not a contiguous array. Each struct swap_cluster_info would need a new field to store its own index. VM_SPARSE preserves the contiguous array layout, so ci - si->cluster_info continues to work for O(1) index lookup everywhere. 2. The struct swap_cluster_info ** pointer array itself costs extra memory: 8 bytes per cluster. That's 8 KB for a 1 GB swap device, and 8 MB for a 1 TB device — paid upfront at swapon, regardless of how many clusters are actually used. VM_SPARSE needs no such indirection array; the cluster_info is addressed directly through the vmalloc area. By comparison, the introduced change in struct swap_cluster_info isn't that great. How the GFP concern is addressed -------------------------------- Hi Nhat, You were right about the three layers of hardcoded GFP_KERNEL in v1. In the current code (the version I'll post as RFC v2), each is fixed: 1. alloc_page() and kmalloc_array() now use: __GFP_HIGH | __GFP_NOMEMALLOC | GFP_KERNEL This matches the pattern Kairui established in swap_cluster_alloc_table(). __GFP_NOMEMALLOC prevents the pfmemalloc reserve bypass during reclaim. 2. The internal page-table allocations inside vmap_pages_range() use GFP_PGTABLE_KERNEL and are not directly controllable from the caller. To prevent these from recursing into swap reclaim, the entire allocation block is wrapped with: noreclaim_flags = memalloc_noreclaim_save(); ... alloc_page / kmalloc_array / vm_area_map_pages ... memalloc_noreclaim_restore(noreclaim_flags); memalloc_noreclaim_save() ensures __GFP_FS and __GFP_IO are cleared for all allocations within the scope, so the page-table allocation cannot recurse into filesystem or swap reclaim. 3. The residual risk is that the page-table allocations still lack __GFP_NOMEMALLOC, meaning they could theoretically dip into emergency reserves under PF_MEMALLOC. However, each grow operation maps at most one PTE page (XSWAP_GROW_CLUSTERS clusters per page), so the total order-0 allocation is tiny — typically a single page-table page. This seems acceptable compared to the complexity of plumbing a GFP parameter through the entire vmap_pages_range() call chain. A variant vm_area_map_pages_gfp that passes the caller's GFP context down to the page-table allocations. That cleanly solves the problem for all VM_SPARSE users, not just xswap. I'm happy to work on that if people think it's the right direction, but I'd prefer to keep it as a separate improvement rather than blocking this series on it. The shrink path also avoids this problem entirely: it runs in a workqueue context where PF_MEMALLOC is never set, so GFP_KERNEL is correct there. Thanks Baoquan > > Thanks for catching this. You're right — there are actually three > layers of hardcoded GFP_KERNEL in the VM_SPARSE path: > > 1. alloc_page(GFP_KERNEL | __GFP_ZERO) in xswap_map_clusters() for the > backing pages > 2. vmap_pages_range() hardcodes GFP_KERNEL (passed only to KMSAN, but > still semantically wrong) > 3. The page-table allocations inside vmap_pages_range_noflush_walk() > (pte_alloc_kernel_track etc.) use GFP_PGTABLE_KERNEL, which is > GFP_KERNEL | __GFP_ZERO — also lacking __GFP_NOMEMALLOC, and > completely invisible to the caller > > When PF_MEMALLOC is set (as it is during reclaim), __gfp_pfmemalloc_flags() > returns ALLOC_NO_WATERMARKS for any allocation without __GFP_NOMEMALLOC, > meaning those allocations can consume all emergency reserves — including > the reserves that the reclaim machinery itself needs to make forward > progress. That is a real deadlock risk. > > One way is to fix them one by one. Another way is to drop the VM_SPARSE > approach entirely and implemented Chris's suggestion instead: cluster_info > is now a struct swap_cluster_info ** pointer array, with each cluster > individually kmem_cache_zalloc()'d. The grow path (xswap_grow_clusters) > now uses: > > kmem_cache_zalloc(swap_cluster_cachep, > __GFP_HIGH | __GFP_NOMEMALLOC | GFP_KERNEL) > matching the pattern Kairui established in swap_cluster_alloc_table(). > All allocations are fully GFP-controllable — no hidden vmap page-table > allocations, no hardcoded GFP_PGTABLE_KERNEL. > > With kmem_cache, shrink is just kmem_cache_free() — each cluster is > independently allocated and freed. Seems the logic is simpler. If no > objection, I will give it a shot soon. > > > Thanks > Baoquan