Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2024/8/1 2:13, Alexander Duyck wrote:
> On Wed, Jul 31, 2024 at 5:50 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> Currently the page_frag API is returning 'virtual address'
>> or 'va' when allocing and expecting 'virtual address' or
>> 'va' as input when freeing.
>>
>> As we are about to support new use cases that the caller
>> need to deal with 'struct page' or need to deal with both
>> 'va' and 'struct page'. In order to differentiate the API
>> handling between 'va' and 'struct page', add '_va' suffix
>> to the corresponding API mirroring the page_pool_alloc_va()
>> API of the page_pool. So that callers expecting to deal with
>> va, page or both va and page may call page_frag_alloc_va*,
>> page_frag_alloc_pg*, or page_frag_alloc* API accordingly.
>>
>> CC: Alexander Duyck <alexander.duyck@gmail.com>
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>> Reviewed-by: Subbaraya Sundeep <sbhatta@marvell.com>
> 
> I am naking this patch. It is a pointless rename that is just going to
> obfuscate the git history for these callers.

I responded to your above similar comment in v2, and then responded more
detailedly in v11, both got not direct responding, it would be good to
have more concrete feedback here instead of abstract argument.

https://lore.kernel.org/all/74e7259a-c462-e3c1-73ac-8e3f49fb80b8@huawei.com/
https://lore.kernel.org/all/11187fe4-9419-4341-97b5-6dad7583b5b6@huawei.com/

> 
> As I believe I said before I would prefer to see this work more like
> the handling of __get_free_pages and __free_pages in terms of the use

I am not even sure why are you bringing up  __get_free_pages() and
__free_pages() here, as the declaration of them is something like below:

unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order);
void __free_pages(struct page *page, unsigned int order);

And I add another related one for completeness here:
extern void free_pages(unsigned long addr, unsigned int order);

I am failing to see there is any pattern or rule for the above API
naming. If there is some pattern for the above existing APIs, please
describe them in detail so that we have common understanding.

After the renaming, the declaration for both new and old APIs is
below, please be more specific about what exactly is the confusion
about them, what is the better naming for the below APIs in your
mind:
struct page *page_frag_alloc_pg(struct page_frag_cache *nc,
                               unsigned int *offset, unsigned int fragsz,
                               gfp_t gfp);
void *page_frag_alloc_va(struct page_frag_cache *nc,
                         unsigned int fragsz, gfp_t gfp_mask);
struct page *page_frag_alloc(struct page_frag_cache *nc,
                             unsigned int *offset,
                             unsigned int fragsz,
                             void **va, gfp_t gfp);

> of pages versus pointers and/or longs. Pushing this API aside because
> you want to reuse the name for something different isn't a valid
> reason to rename an existing API and will just lead to confusion.

Before this patchset, all the page_frag API renamed with a '_va' suffix
in this patch are dealing with virtual address, it would be better to be
more specific about what exactly is the confusion here by adding a explicit
'va' suffix for them in this patch?

I would argue that the renaming may avoid some confusion about whether
page_frag_alloc() returning a 'struct page' or returning a virtual address
instead of leading to confusion.

Anyway, naming is always hard, any better naming is welcome. But don't deny
any existing API renaming when we have not really started doing detailed
comparison between different API naming options yet.