linux-kernel - Re: [PATCH 4/9] mm, swap: tidy up swap device and cluster info helpers

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: <CAMgjq7C+mChdnbcrYEkKyuuRN9-THXwBdFeCVwvW_m-_CWCzvw@mail.gmail.com>
Date: Tue, 2 Sep 2025 23:03:02 +0800
From: Kairui Song <ryncsn@...il.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>, 
	Matthew Wilcox <willy@...radead.org>, Hugh Dickins <hughd@...gle.com>, Chris Li <chrisl@...nel.org>, 
	Barry Song <baohua@...nel.org>, Baoquan He <bhe@...hat.com>, Nhat Pham <nphamcs@...il.com>, 
	Kemeng Shi <shikemeng@...weicloud.com>, Baolin Wang <baolin.wang@...ux.alibaba.com>, 
	Ying Huang <ying.huang@...ux.alibaba.com>, Johannes Weiner <hannes@...xchg.org>, 
	Yosry Ahmed <yosryahmed@...gle.com>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, 
	Zi Yan <ziy@...dia.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/9] mm, swap: tidy up swap device and cluster info helpers

On Tue, Sep 2, 2025 at 10:14 PM David Hildenbrand <david@...hat.com> wrote:
>
> On 22.08.25 21:20, Kairui Song wrote:
> > From: Kairui Song <kasong@...cent.com>
> >
> > swp_swap_info is the most commonly used helper for retrieving swap info.
> > It has an internal check that may lead to a NULL return value, but
> > almost none of its caller checks the return value, making the internal
> > check pointless. In fact, most of these callers already ensured the
> > entry is valid and never expect a NULL value.
> >
> > Tidy this up and shorten the name.
>
> Shorter != better. But yes, "swp_swap" was a mess.
>
> > If the caller can make sure the
> > swap entry/type is valid and the device is pinned, use the new introduced
> > swp_info/swp_type_info instead. They have more debug sanity checks and
> > lower overhead as they are inlined.
> >
> > Callers that may expect a NULL value should use
> > swp_get_info/swp_type_get_info instead.
>
> High-level comments:
>
> 1) I hate the "swp" vs. "swap". Is that a valuable distinction or could
> we just convert it to "swap" as we touch it?

Totally agree. I was just blindly following the old style. It's kind
of confusing indeed.

>
> You're converting swap_type_to_swap_info() to swp_type_to_swap_info(),
> and I am not sure if that is the right direction :)
>
>
> 2) Can we just call it "swap_entry" when we work on a swap entry and
> "swap_type" when we work on a swap type in the function name?
>
> swp_info() is a rather bad function name.
>
>
> 3) I am not sure about "to" -> "get". "to" is much more readable in that
> context and consistent.
>
>
> 4) swp_info[] vs. swap_info() gah.
>
>
> I would just have done:
>
> swap_type_to_info(int type)
> __swap_type_to_info(int type)
> swap_entry_to_info(swp_entry_t entry)
> __swap_entry_to_info(swp_entry_t entry)
>
> __ are the expert functions where we don't expect NULL.
>

Thanks a lot for the suggestions! I also like the idea of using "__"
to seperate the non-NULL version a lot and implis the caller have to
careful.

My concern was that names will be getting very long in later commits
following this convention. Which is also the reason I want to shorten
them here.

A lot of SWAP relate operations will be cluster based, so it will be
very common to get offset or the swap cluster from a swap entry.
We will end up having a really long name like
__swap_entry_to_cluster_offset (convert swap entry to offset inside a
cluster).

Since we already have the swap entry type called `swp_entry_t` and
helprs like `swp_offset` and 'swp_swap_info' that convert an entry to
other swap things, so I thought that anything converts swap entry /
offset to others are named `swp_*`.

Maybe a bad practise here, we can fix it while at it, or at least no
longer introduce more confusing names.

I can follow this suggested style, will it be a good idea if we have
following set of helpers?

For swap cluster and swap device (swap_info_struct):
swap_type_to_info(int)
__swap_type_to_info(int)
swap_entry_to_info(swp_entry_t)
__swap_entry_to_info(swp_entry_t)
__swap_offset_to_cluster(struct swap_info_struct *, pgoff_t)
__swap_entry_to_cluster(swp_entry_t)

And for offsets, we still use:
swp_offset() (Existing helper)
swp_cluster_offset()

Now all swp_* helpers are pure arithmetic operations (we just renamed
swp_swap_info which seems the only exception). Is this better?
I'm open to suggestions as I'm really bad at naming things :)