On Wed, Oct 9, 2024 at 4:50 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote: > > On Fri, 4 Oct 2024 19:34:45 +0900 Taehee Yoo wrote: > > > Our intention with the whole netmem design is that drivers should > > > never have to call netmem_to_page(). I.e. the driver should use netmem > > > unaware of whether it's page or non-page underneath, to minimize > > > complexity driver needs to handle. > > > > > > This netmem_to_page() call can be removed by using > > > skb_frag_fill_netmem_desc() instead of the page variant. But, more > > > improtantly, why did the code change here? The code before calls > > > skb_frag_fill_page_desc, but the new code sometimes will > > > skb_frag_fill_netmem_desc() and sometimes will skb_add_rx_frag_netmem. > > > I'm not sure why that logic changed. > > > > The reason why skb_add_rx_frag_netmem() is used here is to set > > skb->unreadable flag. the skb_frag_fill_netmem_desc() doesn't set > > skb->unreadable because it doesn't handle skb, it only handles frag. > > As far as I know, skb->unreadable should be set to true for devmem > > TCP, am I misunderstood? > > I tested that don't using skb_add_rx_frag_netmem() here, and it > > immediately fails. > > Yes, but netmem_ref can be either a net_iov or a normal page, > and skb_add_rx_frag_netmem() and similar helpers should automatically > set skb->unreadable or not. > > IOW you should be able to always use netmem-aware APIs, no? I'm not sure the update skb->unreadable flag is possible because frag API like skb_add_rx_frag_netmem(), receives only frag, not skb. How about an additional API to update skb->unreadable flag? skb_update_unreadable() or skb_update_netmem()? > > > > This is not the intended use of PP_FLAG_ALLOW_UNREADABLE_NETMEM. > > > > > > The driver should set PP_FLAG_ALLOW_UNREADABLE_NETMEM when it's able > > > to handle unreadable netmem, it should not worry about whether > > > rxq->mp_params.mp_priv is set or not. > > > > > > You should set PP_FLAG_ALLOW_UNREADABLE_NETMEM when HDS is enabled. > > > Let core figure out if mp_params.mp_priv is enabled. All the driver > > > needs to report is whether it's configured to be able to handle > > > unreadable netmem (which practically means HDS is enabled). > > > > The reason why the branch exists here is the PP_FLAG_ALLOW_UNREADABLE_NETMEM > > flag can't be used with PP_FLAG_DMA_SYNC_DEV. > > Hm. Isn't the existing check the wrong way around? Is the driver > supposed to sync the buffers for device before passing them down? I haven't thought the failure of PP_FLAG_DMA_SYNC_DEV for dmabuf may be wrong. I think device memory TCP is not related to this flag. So device memory TCP core API should not return failure when PP_FLAG_DMA_SYNC_DEV flag is set. How about removing this condition check code in device memory TCP core?
