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

[Mina Almasry]

On Thu, Oct 15, 2026 at 1:37 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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().
>
>

Commit message is accurate. There is no helper that mirror
napi_pp_put_page(), only a hepler that unconditionally increaments
pp-ref and barfs if you give it a non-pp-page.

I think we're meant to ignore low (and sometimes medium) comments,
unless they're obviously correct?

-- 
Thanks,
Mina