linux-kernel - Re: [PATCH 04/24] mm/swap: avoid setting page lock bit and doing extra unlock check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF8kJuOzXAy8Er_knD0hi60Tb+XDMrijg-W83KD=Zdjqbu1Dmw@mail.gmail.com>
Date:   Mon, 20 Nov 2023 09:44:46 -0800
From:   Chris Li <chrisl@...nel.org>
To:     Kairui Song <ryncsn@...il.com>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        "Huang, Ying" <ying.huang@...el.com>,
        David Hildenbrand <david@...hat.com>,
        Hugh Dickins <hughd@...gle.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Matthew Wilcox <willy@...radead.org>,
        Michal Hocko <mhocko@...e.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/24] mm/swap: avoid setting page lock bit and doing
 extra unlock check

On Mon, Nov 20, 2023 at 3:15 AM Kairui Song <ryncsn@...il.com> wrote:
> > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > index ac4fa404eaa7..45dd8b7c195d 100644
> > > --- a/mm/swap_state.c
> > > +++ b/mm/swap_state.c
> > > @@ -458,6 +458,8 @@ struct page *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> >
> > You move the mem_cgroup_swapin_charge_folio() inside the for loop:
> >
> >
> >         for (;;) {
> >                 int err;
> >                 /*
> >                  * First check the swap cache.  Since this is normally
> >                  * called after swap_cache_get_folio() failed, re-calling
> >                  * that would confuse statistics.
> >                  */
> >                 folio = filemap_get_folio(swap_address_space(entry),
> >                                                 swp_offset(entry));
> >
> >
> > >                                                 mpol, ilx, numa_node_id());
> > >                 if (!folio)
> > >                          goto fail_put_swap;
> > > +               if (mem_cgroup_swapin_charge_folio(folio, NULL, gfp_mask, entry))
> > > +                       goto fail_put_folio;
> >
> > Wouldn't it cause repeat charging of the folio when it is racing
> > against others in the for loop?
>
> The race loser will call folio_put and discharge it?

There are two different charges. Memcg charging and memcg swapin charging.
The folio_put will do the memcg discharge, the corresponding memcg
charge is in follio allocation.
Memcg swapin charge does things differently, it needs to modify the
swap relately accounting.
The memcg uncharge is not a pair for memcg swapin charge.

> > >                 /*
> > >                  * Swap entry may have been freed since our caller observed it.
> > > @@ -483,13 +485,9 @@ struct page *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> > >         /*
> > >          * The swap entry is ours to swap in. Prepare the new page.
> > >          */
> > > -
> > >         __folio_set_locked(folio);
> > >         __folio_set_swapbacked(folio);
> > >
> > > -       if (mem_cgroup_swapin_charge_folio(folio, NULL, gfp_mask, entry))
> > > -               goto fail_unlock;
> > > -
> >
> > The original code makes the charge outside of the for loop. Only the
> > winner can charge once.
>
> Right, this patch may make the charge/dis-charge path more complex for
> race swapin, I'll re-check this part.

It is more than just complex, it seems to change the behavior of this code.

Chris