On 2024/8/19 23:54, Alexander Duyck wrote: ... >>>> >>>> "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. I am assuming that by 'easily accessible', you meant the 'va' can be calculated as below, right? va = encoded_page_address(encoded_va) + (page_frag_cache_page_size(encoded_va) - remaining); I guess it is easily accessible, but it is not without some overhead to calculate the 'va' here. > >>> 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 Let's be clear here: The callers just expecting [page, offset] also need to call type 3 API, which return both [va] and [page, offset]? and it is ok to ignore the overhead of calculating the 'va' for those kinds of callers just because we don't want to do the renaming for a existing API and can't come up with good naming for that? > after the fact via a commit type action. Just be clear here, there is no commit type action for some subtype of type 2/3 API. For example, for type 2 API in this patchset, it has below subtypes: subtype 1: it does not need a commit type action, it just return [page, offset] instead of page_frag_alloc_va() returning [va], and it does not return the allocated fragsz back to the caller as page_frag_alloc_va() does not too: struct page *page_frag_alloc_pg(struct page_frag_cache *nc, unsigned int *offset, unsigned int fragsz, gfp_t gfp) subtype 2: it does need a commit type action, and @fragsz is returned to the caller and caller used that to commit how much fragsz to commit. struct page *page_frag_alloc_pg_prepare(struct page_frag_cache *nc, unsigned int *offset, unsigned int *fragsz, gfp_t gfp) Do you see subtype 1 as valid API? If no, why? If yes, do you also expect the caller to use "struct page_frag *" as the container? If yes, what is the caller expected to do with the size field in "struct page_frag *" from API perspective? Just ignore it?