On 02.09.25 17:03, Kairui Song wrote:

On Tue, Sep 2, 2025 at 10:14 PM David Hildenbrand [off-list ref] wrote:

quoted

On 22.08.25 21:20, Kairui Song wrote:

quoted

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.

quoted

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.

... and not a lot of space saved :)

quoted

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.

Right, it's the "pro" version :)

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).

That's a perfectly fine length though :)

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_*`.

Yeah, I think that's just bad historical baggage we should clean up at 
some point.

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)

Looks great to me, but let's hear other opinions.

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

Yeah, there's also "swp_type" and "swp_offset_pfn". They really only 
extract basic properties of the entry, so they are a bit special.

I think we should call them "swap_entry_offset" "swap_entry_type" 
"swap_entry_pfn".

Now, that's not something I would expect in your series.

swp_cluster_offset()

That one could later become swap_entry_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 already happy once we name+document the new functions properly.

I could probably live with "swp_cluster_offset" for the time being :)

-- 
Cheers

David / dhildenb