On Fri, Aug 16, 2024 at 4:55 AM Yunsheng Lin linyunsheng@huawei.com wrote:
On 2024/8/15 23:00, Alexander Duyck wrote:
On Wed, Aug 14, 2024 at 8:00 PM Yunsheng Lin linyunsheng@huawei.com 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@gmail.com Signed-off-by: Yunsheng Lin linyunsheng@huawei.com Reviewed-by: Subbaraya Sundeep sbhatta@marvell.com Acked-by: Chuck Lever chuck.lever@oracle.com Acked-by: Sagi Grimberg sagi@grimberg.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:
- page_frag_alloc_va() returns [va].
- page_frag_alloc_pg() returns [page, offset].
- 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:
- Return the virtual address by function returning as below:
void *page_frag_alloc_bio(struct page_frag_cache *nc, struct bio_vec *bio);
- 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.