Re: [RFC PATCH v5 2/4] mm: distinguish large folio swap allocation failures

[Xueyuan Chen]

Kairui Song <ryncsn@xxxxxxxxx> 于2026年8月7日周五 16:29写道：
>
[...]
>
> Hello Xueyuan,
>
> Thanks for the patch!
>
> > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > index 0544b2ec4c56..7d12058174ae 100644
> > --- a/include/linux/swap.h
> > +++ b/include/linux/swap.h
> > @@ -509,12 +509,13 @@ 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);
> > -static inline int mem_cgroup_try_charge_swap(struct folio *folio)
> > +int __mem_cgroup_try_charge_swap(struct folio *folio, long *swap_margin);
> > +static inline int mem_cgroup_try_charge_swap(struct folio *folio,
> > +                                          long *swap_margin)
>
> Am I the only one that feel this returning argument is a bit ugly? See below..

Hi Kairui,

Yes, it does look a bit odd :P

>
> > +/**
> > + * mem_cgroup_get_folio_swap_margin - get a folio's memcg swap margin
> > + * @folio: folio whose memcg margin is queried
> > + *
> > + * Return: Remaining chargeable pages in the folio's memcg hierarchy.
> > + */
> > +long mem_cgroup_get_folio_swap_margin(struct folio *folio)
> > +{
> > +     long swap_margin = PAGE_COUNTER_MAX;
> > +     struct mem_cgroup *memcg;
> > +     struct obj_cgroup *objcg;
> > +
> > +     if (mem_cgroup_disabled() || do_memsw_account())
> > +             return swap_margin;
> > +
> > +     objcg = folio_objcg(folio);
> > +     if (!objcg)
> > +             return swap_margin;
> > +
> > +     rcu_read_lock();
> > +     memcg = obj_cgroup_memcg(objcg);
> > +     swap_margin = page_counter_margin(&memcg->swap);
> > +     rcu_read_unlock();
> > +
> > +     return swap_margin;
> > +}
> > +
>
> Will is be good if we just always check the margin use this helper
> on alloc failure? Alloc failure should be a rather cold path I think?
>
> >  bool mem_cgroup_swap_full(struct folio *folio)
> >  {
> >       struct mem_cgroup *memcg;
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 70b90fa9c2a0..ae62c9f9c0f2 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -1735,23 +1735,28 @@ 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,
> > + * %-ENOSPC if no global swap space is available, or %-ENOMEM if splitting
> > + * would not help.
> >   */
> >  int folio_alloc_swap(struct folio *folio)
> >  {
> >       unsigned int order = folio_order(folio);
> >       unsigned int size = 1 << order;
> > +     long swap_margin = PAGE_COUNTER_MAX;
> >
> >       VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> >       VM_BUG_ON_FOLIO(!folio_test_uptodate(folio), folio);
> >
> >       if (order) {
> >               /*
> > -              * Reject large allocation when THP_SWAP is disabled,
> > -              * the caller should split the folio and try again.
> > +              * Reject large allocation when THP_SWAP is disabled. Check below
> > +              * whether splitting and retrying can make progress.
> >                */
> > -             if (!IS_ENABLED(CONFIG_THP_SWAP))
> > -                     return -EAGAIN;
> > +             if (!IS_ENABLED(CONFIG_THP_SWAP)) {
> > +                     swap_margin = mem_cgroup_get_folio_swap_margin(folio);
> > +                     goto failed;
> > +             }
> >
> >               /*
> >                * Allocation size should never exceed cluster size
> > @@ -1759,7 +1764,8 @@ int folio_alloc_swap(struct folio *folio)
> >                */
> >               if (size > SWAPFILE_CLUSTER) {
> >                       VM_WARN_ON_ONCE(1);
> > -                     return -EINVAL;
> > +                     swap_margin = mem_cgroup_get_folio_swap_margin(folio);
> > +                     goto failed;
> >               }
> >       }
> >
> > @@ -1775,13 +1781,23 @@ 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)))
> > +     if (unlikely(mem_cgroup_try_charge_swap(folio, &swap_margin))) {
> >               swap_cache_del_folio(folio);
> > +             return order && swap_margin > 0 ? -E2BIG : -ENOMEM;
> > +     }
> >
> >       if (unlikely(!folio_test_swapcache(folio)))
> > -             return -ENOMEM;
> > +             goto failed;
> >
> >       return 0;
> > +
> > +failed:
> > +     if (get_nr_swap_pages() <= 0)
> > +             return -ENOSPC;
> > +     if (swap_margin <= 0)
> > +             return -ENOMEM;
> > +
> > +     return order ? -E2BIG : -ENOMEM;
> >  }
>
> How do you think if we apply this on top of this? (Not tested)
> Should be no behavior change but outside the existing races, the margin
> read moves from charge time to failure classification time, a small
> TOCTOU, which the original also has but in a different way.

Thanks a lot for the review and suggestion. I'll simplify the code along
the lines of your diff in the next version. :)

>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 7d6216c8b830..dcf01d4c5e1b 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -490,13 +490,12 @@ 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, long *swap_margin);
> -static inline int mem_cgroup_try_charge_swap(struct folio *folio,
> -                                            long *swap_margin)
> +int __mem_cgroup_try_charge_swap(struct folio *folio);
> +static inline int mem_cgroup_try_charge_swap(struct folio *folio)
>  {
>         if (mem_cgroup_disabled())
>                 return 0;
> -       return __mem_cgroup_try_charge_swap(folio, swap_margin);
> +       return __mem_cgroup_try_charge_swap(folio);
>  }
>
>  extern void __mem_cgroup_uncharge_swap(unsigned short id, unsigned int nr_pages);
> @@ -511,8 +510,7 @@ long mem_cgroup_get_folio_swap_margin(struct folio *folio);
>  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,
> -                                            long *swap_margin)
> +static inline int mem_cgroup_try_charge_swap(struct folio *folio)
>  {
>         return 0;
>  }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b4c65ccf3538..d89054dd96a8 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5650,13 +5650,12 @@ int __init mem_cgroup_init(void)
>  /**
>   * __mem_cgroup_try_charge_swap - try charging swap space for a folio
>   * @folio: folio being added to swap
> - * @swap_margin: remaining memcg swap margin if allocation or charge fails
>   *
>   * Try to charge @folio's memcg for the swap space at folio->swap.
>   *
>   * Returns 0 on success, -ENOMEM on failure.
>   */
> -int __mem_cgroup_try_charge_swap(struct folio *folio, long *swap_margin)
> +int __mem_cgroup_try_charge_swap(struct folio *folio)
>  {
>         unsigned int nr_pages = folio_nr_pages(folio);
>         struct swap_cluster_info *ci;
> @@ -5675,7 +5674,6 @@ int __mem_cgroup_try_charge_swap(struct folio *folio, long *swap_margin)
>         rcu_read_lock();
>         memcg = obj_cgroup_memcg(objcg);
>         if (!folio_test_swapcache(folio)) {
> -               *swap_margin = page_counter_margin(&memcg->swap);
>                 memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
>                 rcu_read_unlock();
>                 return 0;
> @@ -5689,7 +5687,6 @@ int __mem_cgroup_try_charge_swap(struct folio *folio, long *swap_margin)
>             !page_counter_try_charge(&memcg->swap, nr_pages, &counter)) {
>                 memcg_memory_event(memcg, MEMCG_SWAP_MAX);
>                 memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
> -               *swap_margin = page_counter_margin(counter);
>                 mem_cgroup_private_id_put(memcg, nr_pages);
>                 return -ENOMEM;
>         }
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 5d8d04576c13..09760985b911 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1756,7 +1756,6 @@ int folio_alloc_swap(struct folio *folio)
>  {
>         unsigned int order = folio_order(folio);
>         unsigned int size = 1 << order;
> -       long swap_margin = PAGE_COUNTER_MAX;
>
>         VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
>         VM_BUG_ON_FOLIO(!folio_test_uptodate(folio), folio);
> @@ -1766,10 +1765,8 @@ int folio_alloc_swap(struct folio *folio)
>                  * Reject large allocation when THP_SWAP is disabled. Check below
>                  * whether splitting and retrying can make progress.
>                  */
> -               if (!IS_ENABLED(CONFIG_THP_SWAP)) {
> -                       swap_margin = mem_cgroup_get_folio_swap_margin(folio);
> +               if (!IS_ENABLED(CONFIG_THP_SWAP))
>                         goto failed;
> -               }
>
>                 /*
>                  * Allocation size should never exceed cluster size
> @@ -1777,7 +1774,6 @@ int folio_alloc_swap(struct folio *folio)
>                  */
>                 if (size > SWAPFILE_CLUSTER) {
>                         VM_WARN_ON_ONCE(1);
> -                       swap_margin = mem_cgroup_get_folio_swap_margin(folio);
>                         goto failed;
>                 }
>         }
> @@ -1794,10 +1790,8 @@ 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, &swap_margin))) {
> +       if (unlikely(mem_cgroup_try_charge_swap(folio)))
>                 swap_cache_del_folio(folio);
> -               return order && swap_margin > 0 ? -E2BIG : -ENOMEM;
> -       }
>
>         if (unlikely(!folio_test_swapcache(folio)))
>                 goto failed;
> @@ -1807,7 +1801,7 @@ int folio_alloc_swap(struct folio *folio)
>  failed:
>         if (get_nr_swap_pages() <= 0)
>                 return -ENOSPC;
> -       if (swap_margin <= 0)
> +       if (mem_cgroup_get_folio_swap_margin(folio) <= 0)
>                 return -ENOMEM;
>
>         return order ? -E2BIG : -ENOMEM;
>