On Wed, Aug 14, 2024 at 8:00 PM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote: > > 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@xxxxxxxxx> > >> Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx> > >> Reviewed-by: Subbaraya Sundeep <sbhatta@xxxxxxxxxxx> > >> Acked-by: Chuck Lever <chuck.lever@xxxxxxxxxx> > >> Acked-by: Sagi Grimberg <sagi@xxxxxxxxxxx> > >> --- > >> 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?" I didn't. I just don't see the point in pushing out the existing API to support that. In reality 2 and 3 are redundant. You probably only need 3. Like I mentioned earlier you can essentially just pass a page_frag via pointer to the function. With that you could also look at just returning a virtual address as well if you insist on having something that returns all of the above. No point in having 2 and 3 be seperate functions. I am going to nack this patch set if you insist on this pointless renaming. The fact is it is just adding noise that adds no value.
