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

[David Hildenbrand <david@redhat.com>]

On 02.09.25 17:03, Kairui Song wrote:
> 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.

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

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