Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <20231119194740.94101-1-ryncsn@gmail.com> <20231119194740.94101-5-ryncsn@gmail.com>
 <CAF8kJuNbcvwmojMB2Vp2LjTBxZtKFO_Zng4TODqADxmNPn2kCg@mail.gmail.com> <CAMgjq7D_OJiaNPWquFqxk5DKZXqoUMrD7Y_CRM7D0RnRnHmW7Q@mail.gmail.com>
In-Reply-To: <CAMgjq7D_OJiaNPWquFqxk5DKZXqoUMrD7Y_CRM7D0RnRnHmW7Q@mail.gmail.com>
From:   Chris Li <chrisl@kernel.org>
Date:   Mon, 20 Nov 2023 09:44:46 -0800
Message-ID: <CAF8kJuOzXAy8Er_knD0hi60Tb+XDMrijg-W83KD=Zdjqbu1Dmw@mail.gmail.com>
Subject: Re: [PATCH 04/24] mm/swap: avoid setting page lock bit and doing
 extra unlock check
To:     Kairui Song <ryncsn@gmail.com>
Cc:     linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
        "Huang, Ying" <ying.huang@intel.com>,
        David Hildenbrand <david@redhat.com>,
        Hugh Dickins <hughd@google.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Matthew Wilcox <willy@infradead.org>,
        Michal Hocko <mhocko@suse.com>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Nov 20, 2023 at 3:15=E2=80=AFAM Kairui Song <ryncsn@gmail.com> wrot=
e:
> > > 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-calli=
ng
> >                  * that would confuse statistics.
> >                  */
> >                 folio =3D 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_m=
ask, 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 ob=
served 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, ent=
ry))
> > > -               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