linux-kernel - Re: [PATCH 03/24] mm/swap: move no readahead swapin code to a stand alone helper

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]

Date:   Mon, 20 Nov 2023 21:34:55 -0800
From:   Chris Li <chrisl@...nel.org>
To:     Kairui Song <kasong@...cent.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 03/24] mm/swap: move no readahead swapin code to a stand
 alone helper

On Sun, Nov 19, 2023 at 11:48 AM Kairui Song <ryncsn@...il.com> wrote:
>
> From: Kairui Song <kasong@...cent.com>
>
> No feature change, simply move the routine to a standalone function to
> be used later. The error path handling is copied from the "out_page"
> label, to make the code change minimized for easier reviewing.
>
> Signed-off-by: Kairui Song <kasong@...cent.com>
> ---
>  mm/memory.c     | 33 +++++----------------------------
>  mm/swap.h       |  2 ++
>  mm/swap_state.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+), 28 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 70ffa867b1be..fba4a5229163 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3794,7 +3794,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>         swp_entry_t entry;
>         pte_t pte;
>         vm_fault_t ret = 0;
> -       void *shadow = NULL;
>
>         if (!pte_unmap_same(vmf))
>                 goto out;
> @@ -3858,33 +3857,11 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>         if (!folio) {
>                 if (data_race(si->flags & SWP_SYNCHRONOUS_IO) &&
>                     __swap_count(entry) == 1) {
> -                       /* skip swapcache */
> -                       folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0,
> -                                               vma, vmf->address, false);
> -                       if (folio) {
> -                               __folio_set_locked(folio);
> -                               __folio_set_swapbacked(folio);
> -
> -                               if (mem_cgroup_swapin_charge_folio(folio,
> -                                                       vma->vm_mm, GFP_KERNEL,
> -                                                       entry)) {
> -                                       ret = VM_FAULT_OOM;
> -                                       goto out_page;
> -                               }
> -                               mem_cgroup_swapin_uncharge_swap(entry);
> -
> -                               shadow = get_shadow_from_swap_cache(entry);
> -                               if (shadow)
> -                                       workingset_refault(folio, shadow);
> -
> -                               folio_add_lru(folio);
> -                               page = &folio->page;
> -
> -                               /* To provide entry to swap_readpage() */
> -                               folio->swap = entry;
> -                               swap_readpage(page, true, NULL);
> -                               folio->private = NULL;
> -                       }
> +                       /* skip swapcache and readahead */
> +                       page = swapin_no_readahead(entry, GFP_HIGHUSER_MOVABLE,
> +                                               vmf);

A minor point,  swapin_no_readahead() is expressed in negative words.

Better use positive words to express what the function does rather
than what the function does not do.
I am terrible at naming functions myself. I can think of something
along the lines of:
swapin_direct (no cache).
swapin_minimal?
swapin_entry_only?

Please suggest better names for basic, bare minimum.

Chris