Re: [RFC PATCH v3 2/4] mm: distinguish large folio swap allocation failures
Barry Song <[email protected]>
| Newsgroups | org.kernel.vger.cgroups,org.kernel.vger.linux-kernel,org.kvack.linux-mm |
|---|---|
| Message-ID | <CAGsJ_4xv2AGrLY2PBA77fZiqctPEt_ERcUXUvFOdOG72LD_usQ@mail.gmail.com> |
On Fri, Jul 17, 2026 at 8:25 PM Xueyuan Chen <[email protected]> wrote: > > folio_alloc_swap() reports most allocation failures with a generic > negative error code. Reclaim cannot tell whether splitting a large folio > could make progress or whether there is no backing space at all. > > Track the global free swap count around the allocation attempt and let the > memcg swap charge path cap it by the remaining hierarchical swap margin. > Return -E2BIG for large folios when a smaller allocation might still fit, > -ENOSPC when no swap space is available, and -ENOMEM when the failure is > not helped by splitting. > > This only refines folio_alloc_swap() return codes. The reclaim caller is > updated separately. > > Signed-off-by: Xueyuan Chen <[email protected]> > --- > include/linux/swap.h | 10 ++++++---- > mm/memcontrol.c | 10 +++++++++- > mm/swapfile.c | 21 +++++++++++++++------ > 3 files changed, 30 insertions(+), 11 deletions(-) > > diff --git a/include/linux/swap.h b/include/linux/swap.h > index 7a09df6977a5..0695ac56457f 100644 > --- a/include/linux/swap.h > +++ b/include/linux/swap.h > @@ -571,13 +571,14 @@ static inline void folio_throttle_swaprate(struct folio *folio, gfp_t gfp) > #endif > > #if defined(CONFIG_MEMCG) && defined(CONFIG_SWAP) > -int __mem_cgroup_try_charge_swap(struct folio *folio, swp_entry_t entry); > +int __mem_cgroup_try_charge_swap(struct folio *folio, swp_entry_t entry, > + long *nr_swap_pages); > static inline int mem_cgroup_try_charge_swap(struct folio *folio, > - swp_entry_t entry) > + swp_entry_t entry, long *nr_swap_pages) > { > if (mem_cgroup_disabled()) > return 0; > - return __mem_cgroup_try_charge_swap(folio, entry); > + return __mem_cgroup_try_charge_swap(folio, entry, nr_swap_pages); > } > > extern void __mem_cgroup_uncharge_swap(swp_entry_t entry, unsigned int nr_pages); > @@ -592,7 +593,8 @@ extern long mem_cgroup_get_nr_swap_pages(struct mem_cgroup *memcg); > extern bool mem_cgroup_swap_full(struct folio *folio); > #else > static inline int mem_cgroup_try_charge_swap(struct folio *folio, > - swp_entry_t entry) > + swp_entry_t entry, > + long *nr_swap_pages) > { > return 0; > } > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 1e10f493d2a9..7c9e7072fb39 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5472,12 +5472,14 @@ int __init mem_cgroup_init(void) > * __mem_cgroup_try_charge_swap - try charging swap space for a folio > * @folio: folio being added to swap > * @entry: swap entry to charge > + * @nr_swap_pages: optional swap availability to cap by memcg margin > * > * Try to charge @folio's memcg for the swap space at @entry. > * > * Returns 0 on success, -ENOMEM on failure. > */ > -int __mem_cgroup_try_charge_swap(struct folio *folio, swp_entry_t entry) > +int __mem_cgroup_try_charge_swap(struct folio *folio, swp_entry_t entry, > + long *nr_swap_pages) > { > unsigned int nr_pages = folio_nr_pages(folio); > struct page_counter *counter; > @@ -5495,6 +5497,9 @@ int __mem_cgroup_try_charge_swap(struct folio *folio, swp_entry_t entry) > rcu_read_lock(); > memcg = obj_cgroup_memcg(objcg); > if (!entry.val) { > + if (nr_swap_pages && !mem_cgroup_is_root(memcg)) > + *nr_swap_pages = min(*nr_swap_pages, > + page_counter_margin(&memcg->swap)); This looks a bit odd. Could we decouple *nr_swap_pages from margin and have the function always return margin instead? I don't think this function needs to care about the initial value returned by get_nr_swap_pages(). While folio_alloc_swap() may depend on it, they're different software layers, and we shouldn't couple them together. > memcg_memory_event(memcg, MEMCG_SWAP_FAIL); > rcu_read_unlock(); > return 0; > @@ -5509,6 +5514,9 @@ int __mem_cgroup_try_charge_swap(struct folio *folio, swp_entry_t entry) > memcg_memory_event(memcg, MEMCG_SWAP_MAX); > memcg_memory_event(memcg, MEMCG_SWAP_FAIL); > mem_cgroup_private_id_put(memcg, nr_pages); > + if (nr_swap_pages) > + *nr_swap_pages = min(*nr_swap_pages, > + page_counter_margin(counter)); > return -ENOMEM; > } > mod_memcg_state(memcg, MEMCG_SWAP, nr_pages); > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 9174f1eeffb0..53a921ca099a 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -1690,12 +1690,14 @@ static int swap_dup_entries_cluster(struct swap_info_struct *si, > * swap cache. > * > * Context: Caller needs to hold the folio lock. > - * Return: Whether the folio was added to the swap cache. > + * Return: 0 on success, -E2BIG if splitting the folio might allow swapout, > + * or another negative error code if splitting would not help. > */ > int folio_alloc_swap(struct folio *folio) > { > unsigned int order = folio_order(folio); > unsigned int size = 1 << order; > + long nr_swap_pages; > > VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio); > VM_BUG_ON_FOLIO(!folio_test_uptodate(folio), folio); > @@ -1706,7 +1708,7 @@ int folio_alloc_swap(struct folio *folio) > * the caller should split the folio and try again. > */ > if (!IS_ENABLED(CONFIG_THP_SWAP)) > - return -EAGAIN; > + return -E2BIG; We could also have the case where we're running out of swap space and it's not worth splitting, even when THP_SWAP is disabled. > > /* > * Allocation size should never exceed cluster size > @@ -1714,10 +1716,12 @@ int folio_alloc_swap(struct folio *folio) > */ > if (size > SWAPFILE_CLUSTER) { > VM_WARN_ON_ONCE(1); > - return -EINVAL; > + return -E2BIG; Similar to the above. > } > } > > + nr_swap_pages = get_nr_swap_pages(); > + > again: > local_lock(&percpu_swap_cluster.lock); > if (!swap_alloc_fast(folio)) > @@ -1730,11 +1734,16 @@ int folio_alloc_swap(struct folio *folio) > } > > /* Need to call this even if allocation failed, for MEMCG_SWAP_FAIL. */ > - if (unlikely(mem_cgroup_try_charge_swap(folio, folio->swap))) > + if (unlikely(mem_cgroup_try_charge_swap(folio, folio->swap, > + &nr_swap_pages))) { As explained above, we shouldn't couple two different software layers. don't use nr_swap_pages() as the initial value of memcg charge. Best Regards Barry