On Fri, Aug 16, 2024 at 4:55 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote: > > On 2024/8/15 23:00, Alexander Duyck wrote: > > 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 > > If the caller just expect [page, offset], do you expect the caller also > type 3 API, which return both [va] and [page, offset]? > > I am not sure if I understand why you think 2 and 3 are redundant here? > If you think 2 and 3 are redundant here, aren't 1 and 3 also redundant > as the similar agrument? The big difference is the need to return page and offset. Basically to support returning page and offset you need to pass at least one value as a pointer so you can store the return there. The reason why 3 is just a redundant form of 2 is that you will normally just be converting from a va to a page and offset so the va should already be easily accessible. > > 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. > > Let's be more specific about what are your suggestion here: which way > is the prefer way to return the virtual address. It seems there are two > options: > > 1. Return the virtual address by function returning as below: > void *page_frag_alloc_bio(struct page_frag_cache *nc, struct bio_vec *bio); > > 2. Return the virtual address by double pointer as below: > int page_frag_alloc_bio(struct page_frag_cache *nc, struct bio_vec *bio, > void **va); I was thinking more of option 1. Basically this is a superset of page_frag_alloc_va that is also returning the page and offset via a page frag. However instead of bio_vec I would be good with "struct page_frag *" being the value passed to the function to play the role of container. Basically the big difference between 1 and 2/3 if I am not mistaken is the fact that for 1 you pass the size, whereas with 2/3 you are peeling off the page frag from the larger page frag cache after the fact via a commit type action.
