Message-ID: <676a2a15-d390-48a7-a8d7-6e491c89e200@huawei.com>
Date: Thu, 15 Aug 2024 10:59:57 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Alexander H Duyck <alexander.duyck@...il.com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Subbaraya
 Sundeep <sbhatta@...vell.com>, Chuck Lever <chuck.lever@...cle.com>, Sagi
 Grimberg <sagi@...mberg.me>, 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>, 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>,
	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>, Trond Myklebust <trondmy@...nel.org>, Anna Schumaker
	<anna@...nel.org>, Shuah Khan <shuah@...nel.org>,
	<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>, <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH net-next v13 04/14] mm: page_frag: add '_va' suffix to
 page_frag API

On 2024/8/14 23:49, Alexander H Duyck wrote:
> On Thu, 2024-08-08 at 20:37 +0800, Yunsheng Lin 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>
>> Acked-by: Chuck Lever <chuck.lever@...cle.com>
>> Acked-by: Sagi Grimberg <sagi@...mberg.me>
>> ---
>>  drivers/net/ethernet/google/gve/gve_rx.c      |  4 ++--
>>  drivers/net/ethernet/intel/ice/ice_txrx.c     |  2 +-
>>  drivers/net/ethernet/intel/ice/ice_txrx.h     |  2 +-
>>  drivers/net/ethernet/intel/ice/ice_txrx_lib.c |  2 +-
>>  .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |  4 ++--
>>  .../marvell/octeontx2/nic/otx2_common.c       |  2 +-
>>  drivers/net/ethernet/mediatek/mtk_wed_wo.c    |  4 ++--
>>  drivers/nvme/host/tcp.c                       |  8 +++----
>>  drivers/nvme/target/tcp.c                     | 22 +++++++++----------
>>  drivers/vhost/net.c                           |  6 ++---
>>  include/linux/page_frag_cache.h               | 21 +++++++++---------
>>  include/linux/skbuff.h                        |  2 +-
>>  kernel/bpf/cpumap.c                           |  2 +-
>>  mm/page_frag_cache.c                          | 12 +++++-----
>>  net/core/skbuff.c                             | 16 +++++++-------
>>  net/core/xdp.c                                |  2 +-
>>  net/rxrpc/txbuf.c                             | 15 +++++++------
>>  net/sunrpc/svcsock.c                          |  6 ++---
>>  .../selftests/mm/page_frag/page_frag_test.c   | 13 ++++++-----
>>  19 files changed, 75 insertions(+), 70 deletions(-)
>>
> 
> I still say no to this patch. It is an unnecessary name change and adds
> no value. If you insist on this patch I will reject the set every time.
> 
> The fact is it is polluting the git history and just makes things
> harder to maintain without adding any value as you aren't changing what
> the function does and there is no need for this. In addition it just

I guess I have to disagree with the above 'no need for this' part for
now, as mentioned in [1]:

"There are three types of API as proposed in this patchset instead of
two types of API:
1. page_frag_alloc_va() returns [va].
2. page_frag_alloc_pg() returns [page, offset].
3. page_frag_alloc() returns [va] & [page, offset].

You seemed to miss that we need a third naming for the type 3 API.
Do you see type 3 API as a valid API? if yes, what naming are you
suggesting for it? if no, why it is not a valid API?"


1. https://lore.kernel.org/all/ca6be29e-ab53-4673-9624-90d41616a154@huawei.com/

> makes it that much harder to backport fixes in the future as people
> will have to work around the rename.
> 

Powered by blists - more mailing lists