Message-ID: <11187fe4-9419-4341-97b5-6dad7583b5b6@huawei.com>
Date: Thu, 25 Jul 2024 20:21:42 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Alexander Duyck <alexander.duyck@...il.com>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Subbaraya Sundeep
	<sbhatta@...vell.com>, Jeroen de Borst <jeroendb@...gle.com>, Praveen
 Kaligineedi <pkaligineedi@...gle.com>, Shailend Chand <shailend@...gle.com>,
	Eric Dumazet <edumazet@...gle.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>, Sunil Goutham
	<sgoutham@...vell.com>, Geetha sowjanya <gakula@...vell.com>, hariprasad
	<hkelam@...vell.com>, Felix Fietkau <nbd@....name>, Sean Wang
	<sean.wang@...iatek.com>, Mark Lee <Mark-MC.Lee@...iatek.com>, Lorenzo
 Bianconi <lorenzo@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Keith
 Busch <kbusch@...nel.org>, Jens Axboe <axboe@...nel.dk>, Christoph Hellwig
	<hch@....de>, Sagi Grimberg <sagi@...mberg.me>, Chaitanya Kulkarni
	<kch@...dia.com>, "Michael S. Tsirkin" <mst@...hat.com>, Jason Wang
	<jasowang@...hat.com>, Eugenio Pérez <eperezma@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>, Alexei Starovoitov
	<ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Jesper Dangaard
 Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, Andrii
 Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, Eduard
 Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, Yonghong Song
	<yonghong.song@...ux.dev>, KP Singh <kpsingh@...nel.org>, Stanislav Fomichev
	<sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
	David Howells <dhowells@...hat.com>, Marc Dionne <marc.dionne@...istor.com>,
	Trond Myklebust <trondmy@...nel.org>, Anna Schumaker <anna@...nel.org>, Chuck
 Lever <chuck.lever@...cle.com>, Jeff Layton <jlayton@...nel.org>, Neil Brown
	<neilb@...e.de>, Olga Kornievskaia <kolga@...app.com>, Dai Ngo
	<Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
	<intel-wired-lan@...ts.osuosl.org>, <linux-arm-kernel@...ts.infradead.org>,
	<linux-mediatek@...ts.infradead.org>, <linux-nvme@...ts.infradead.org>,
	<kvm@...r.kernel.org>, <virtualization@...ts.linux.dev>,
	<linux-mm@...ck.org>, <bpf@...r.kernel.org>, <linux-afs@...ts.infradead.org>,
	<linux-nfs@...r.kernel.org>
Subject: Re: [RFC v11 04/14] mm: page_frag: add '_va' suffix to page_frag API

On 2024/7/22 4:41, Alexander Duyck wrote:
> On Fri, Jul 19, 2024 at 2:37 AM Yunsheng Lin <linyunsheng@...wei.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@...il.com>
>> Signed-off-by: Yunsheng Lin <linyunsheng@...wei.com>
>> Reviewed-by: Subbaraya Sundeep <sbhatta@...vell.com>
> 
> Rather than renaming the existing API I would rather see this follow
> the same approach as we use with the other memory subsystem functions.

I am not sure if I understand what 'the other memory subsystem functions'
is referring to, it would be better to be more specific about that.

For allocation side:
alloc_pages*()
extern unsigned long get_free_page*(gfp_t gfp_mask, unsigned int order);

For free side, it seems we have:
extern void __free_pages(struct page *page, unsigned int order);
extern void free_pages(unsigned long addr, unsigned int order);
static inline void put_page(struct page *page)

So there seems to be no clear pattern that the mm APIs with double
underscore is dealing with 'struct page' and the one without double
underscore is dealing with virtual address, at least not from the
allocation side.

> A specific example being that with free_page it is essentially passed
> a virtual address, while the double underscore version is passed a
> page. I would be more okay with us renaming the double underscore
> version of any functions we might have to address that rather than
> renaming all the functions with "va".

Before this patchset, page_frag has the below APIs as below:

void *__page_frag_alloc_align(struct page_frag_cache *nc, unsigned int fragsz,
			      gfp_t gfp_mask, unsigned int align_mask);

static inline void *page_frag_alloc_align(struct page_frag_cache *nc,
					  unsigned int fragsz, gfp_t gfp_mask,
					  unsigned int align)

extern void page_frag_free(void *addr);

It would be better to be more specific about what renaming does the above
APIs need in order to support the new usecases.

> 
> In general I would say this patch is adding no value as what it is

As above, it would be better to give a more specific suggestion to
back up the above somewhat abstract agrument, otherwise it is hard
to tell if there is better option here, and why it is better than
the one proposed in this patchset.

> doing is essentially pushing the primary users of this API to change
> to support use cases that won't impact most of them. It is just
> creating a ton of noise in terms of changes with no added value so we
> can reuse the function names.


After this patchset, we have the below page_frag APIs:

For allocation side, we have below APIs:
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,
                                 unsigned int align_mask);
struct page *page_frag_alloc*(struct page_frag_cache *nc,
                                     unsigned int *offset,
                                     unsigned int fragsz,
                                     void **va, gfp_t gfp);

For allocation side, we have below APIs:
void page_frag_free_va(void *addr);

The main rules for the about naming are:
1. The API with 'align' suffix ensure the offset or va is aligned
2. The API with double underscore has no checking for the algin parameter.
3. The API with 'va' suffix is dealing with virtual address.
4. The API with 'pg' suffix is dealing with 'struct page'.
5. The API without 'pg' and 'va' suffix is dealing with both 'struct page'
   and virtual address.

Yes, I suppose it is not perfect mainly because we reuse some existing mm
API for page_frag free API.

As mentioned before, I would be happy to change it if what you are proposing
is indeed the better option.

Powered by blists - more mailing lists