On Thu, Jan 4, 2024 at 12:48 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote: > > On 2024/1/4 2:38, Mina Almasry wrote: > > On Wed, Jan 3, 2024 at 1:47 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote: > >> > >> On 2024/1/3 0:14, Mina Almasry wrote: > >>> > >>> 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. > To be honest I definitely read that we have agreement that we should have a unified driver facing struct from the responses in this thread like this one: https://lore.kernel.org/netdev/20231215190126.1040fa12@xxxxxxxxxx/ But I'll let folks correct me if I'm wrong. > > 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. > Correct, the drivers should never cast directly, but helpers like skb_frag_page() must check that the netmem is a page before doing a cast. > > 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. > With static branch the checks should be complete no-ops unless the user's set up enabled devmem. > 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@xxxxxxxxxxxxxx/ > > The above test case does not even seems to be testing a code path calling > skb_frag_page() as my understanding. > > > -- Thanks, Mina
