Re: [RFC PATCH net-next v1 4/4] net: page_pool: use netmem_t instead of struct page in API

On 2024/1/4 2:38, Mina Almasry wrote:

On Wed, Jan 3, 2024 at 1:47 AM Yunsheng Lin [off-list ref] wrote:

quoted

On 2024/1/3 0:14, Mina Almasry wrote:

quoted

The idea being that skb_frag_page() can return NULL if the frag is not
paged, and the relevant callers are modified to handle that.

There are many existing drivers which are not expecting NULL returning for
skb_frag_page() as those drivers are not supporting devmem, adding additionl
checking overhead in skb_frag_page() for those drivers does not make much
sense, IMHO, it may make more sense to introduce a new helper for the driver
supporting devmem or networking core that needing dealing with both normal
page and devmem.

And we are also able to keep the old non-NULL returning semantic for
skb_frag_page().

I think I'm seeing agreement that the direction we're heading into
here is that most net stack & drivers should use the abstract netmem

As far as I see, at least for the drivers, I don't think we have a clear
agreement if we should have a unified driver facing struct or API for both
normal page and devmem yet.

type, and only specific code that needs a page or devmem (like
tcp_receive_zerocopy or tcp_recvmsg_dmabuf) will be the ones that
unpack the netmem and get the underlying page or devmem, using
skb_frag_page() or something like skb_frag_dmabuf(), etc.

As Jason says repeatedly, I'm not allowed to blindly cast a netmem to
a page and assume netmem==page. Netmem can only be cast to a page
after checking the low bits and verifying the netmem is actually a

I thought it would be best to avoid casting a netmem or devmem to a
page in the driver, I think the main argument is that it is hard
to audit very single driver doing a checking before doing the casting
in the future? and we can do better auditting if the casting is limited
to a few core functions in the networking core.

page. I think any suggestions that blindly cast a netmem to page
without the checks will get nacked by Jason & Christian, so the
checking in the specific cases where the code needs to know the
underlying memory type seems necessary.

IMO I'm not sure the checking is expensive. With likely/unlikely &
static branches the checks should be very minimal or a straight no-op.
For example in RFC v2 where we were doing a lot of checks for devmem
(we don't do that anymore for RFCv5), I had run the page_pool perf
tests and proved there is little to no perf regression:

For MAX_SKB_FRAGS being 17, it means we may have 17 additional checking
overhead for the drivers not supporting devmem, not to mention we may
have bigger value for MAX_SKB_FRAGS if BIG TCP is enable.

Even there is no notiable performance degradation for a specific case,
we should avoid the overhead as much as possible for the existing use
case when supporting a new use case.

https://lore.kernel.org/netdev/CAHS8izM4w2UETAwfnV7w+ZzTMxLkz+FKO+xTgRdtYKzV8RzqXw@mail.gmail.com/ (local)

The above test case does not even seems to be testing a code path calling
skb_frag_page() as my understanding.