On Mon, Dec 11, 2023 at 12:37 PM Pavel Begunkov asml.silence@gmail.com wrote: ...
If you remove the branch, let it fall into ->release and rely on refcounting there, then the callback could also fix up release_cnt or ask pp to do it, like in the patch I linked above
Sadly I don't think this is possible due to the reasons I mention in the commit message of that patch. Prematurely releasing ppiov and not having them be candidates for recycling shows me a 4-5x degradation in performance.
I don't think I follow. The concept is to only recycle a buffer (i.e. make it available for allocation) when its refs drop to zero, which is IMHO the only way it can work, and IIUC what this patchset is doing.
That's also I suggest to do, but through a slightly different path. Let's say at some moment there are 2 refs (e.g. 1 for an skb and 1 for userspace/xarray).
Say it first puts the skb:
napi_pp_put_page() -> page_pool_return_page() -> mp_ops->release_page() -> need_to_free = put_buf() // not last ref, need_to_free==false, // don't recycle, don't increase release_cnt
Then you put the last ref:
page_pool_iov_put_many() -> page_pool_return_page() -> mp_ops->release_page() -> need_to_free = put_buf() // last ref, need_to_free==true, // recycle and release_cnt++
And that last put can even be recycled right into the pp / ptr_ring, in which case it doesn't need to touch release_cnt. Does it make sense? I don't see where 4-5x degradation would come from
Sorry for the late reply, I have been working on this locally.
What you're saying makes sense, and I'm no longer sure why I was seeing a perf degradation without '[net-next v1 10/16] page_pool: don't release iov on elevanted refcount'. However, even though what you're saying is technically correct, AFAIU it's actually semantically wrong. When a page is released by the page_pool, we should call page_pool_clear_pp_info() and completely disconnect the page from the pool. If we call release_page() on a page and then the page pool sees it again in page_pool_return_page(), I think that is considered a bug. In fact I think what you're proposing is as a result of a bug because we don't call a page_pool_clear_pp_info() equivalent on releasing ppiov.
However, I'm reasonably confident I figured out the right thing to do here. The page_pool uses page->pp_frag_count for its refcounting. pp_frag_count is a misnomer, it's being renamed to pp_ref_count in Liang's series[1]). In this series I used a get_page/put_page equivalent for refcounting. Once I transitioned to using pp_[frag|ref]_count for refcounting inside the page_pool, the issue went away, and I no longer need the patch 'page_pool: don't release iov on elevanted refcount'.
There is an additional upside, since pages and ppiovs are both being refcounted using pp_[frag|ref]_count, we get some unified handling for ppiov and we reduce the checks around ppiov. This should be fixed properly in the next series.
I still need to do some work (~1 week) before I upload the next version as there is a new requirement from MM that we transition to a new type and not re-use page*, but I uploaded my changes github with the refcounting issues resolved in case they're useful to you. Sorry for the churn:
https://github.com/mina/linux/commits/tcpdevmem-v1.5/
[1] https://patchwork.kernel.org/project/netdevbpf/list/?series=809049&state...