Re: [PATCH net-next v3 2/6] net: skbuff: add napi_pp_get_page() and use it in tcp_recvmsg_dmabuf()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says that "no symmetric helper existed to
  acquire" a page_pool reference, "forcing callers like…

--

net: skbuff: add napi_pp_get_page() and use it in tcp_recvmsg_dmabuf()

This commit adds napi_pp_get_page(), which takes a page_pool pp_ref_count
reference on a netmem if it belongs to a page_pool, as the counterpart to
napi_pp_put_page(). It also converts tcp_recvmsg_dmabuf() to use it,
wrapped in WARN_ON_ONCE(), in place of the open-coded atomic_long_inc().

> While napi_pp_put_page() releases a page_pool pp_ref_count reference on
> a netmem, no symmetric helper existed to acquire one, forcing callers
> like tcp_recvmsg_dmabuf() to open-code atomic_long_inc() on
> niov->desc.pp_ref_count.

[Severity: Low]
Is this description accurate? A helper for this increment already exists
in include/net/page_pool/helpers.h:

static inline void page_pool_ref_netmem(netmem_ref netmem)
{
	atomic_long_inc(netmem_get_pp_ref_count_ref(netmem));
}

io_uring/zcrx.c already calls it in a similar zero-copy rx handoff:

	page_pool_ref_netmem(net_iov_to_netmem(niov));

Documentation/networking/netmem.rst also names page_pool_ref_netmem() as
the API for holding a page_pool reference. The new napi_pp_get_page()
calls it internally too.

Compared with page_pool_ref_netmem(), napi_pp_get_page() adds the
netmem_compound_head() lookup and a netmem_is_pp() check whose bool
result tells the caller whether the netmem is page_pool backed. Later
patches in the series seem to depend on that result, for example in
skb_netmem_ref(), which falls back to get_netmem() when it is false.

Could the commit message explain the page_pool membership check and
return value, rather than saying no helper existed? As written, it is
not clear why tcp_recvmsg_dmabuf() doesn't just call
page_pool_ref_netmem().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014173945.373295-1-almasrymina%40google.com