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

[Kairui Song <ryncsn@gmail.com>]

On Tue, Sep 2, 2025 at 10:14 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 22.08.25 21:20, Kairui Song wrote:
> > From: Kairui Song <kasong@tencent.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 :)