On Fri, Oct 18, 2024 at 5:25 PM Mina Almasry <almasrymina@xxxxxxxxxx> wrote: > > On Tue, Oct 15, 2024 at 3:44 PM Jason Gunthorpe <jgg@xxxxxxxx> wrote: > > > > On Tue, Oct 15, 2024 at 04:10:44AM +0300, Mina Almasry wrote: > > > On Tue, Oct 15, 2024 at 3:16 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote: > > > > > > > > On Tue, 15 Oct 2024 01:38:20 +0300 Mina Almasry wrote: > > > > > Thanks Jason. In that case I agree with Jakub we should take in his change here: > > > > > > > > > > https://lore.kernel.org/netdev/20241009170102.1980ed1d@xxxxxxxxxx/ > > > > > > > > > > With this change the driver would delegate dma_sync_for_device to the > > > > > page_pool, and the page_pool will skip it altogether for the dma-buf > > > > > memory provider. > > > > > > > > And we need a wrapper for a sync for CPU which will skip if the page > > > > comes from an unreadable pool? > > > > > > This is where it gets a bit tricky, no? > > > > > > Our production code does a dma_sync_for_cpu but no > > > dma_sync_for_device. That has been working reliably for us with GPU > > > > Those functions are all NOP on systems you are testing on. > > > > OK, thanks. This is what I wanted to confirm. If you already know this > here then there is no need to wait for me to confirm. > > > The question is what is correct to do on systems where it is not a > > NOP, and none of this is really right, as I explained.. > > > > > But if you or Jason think that enforcing the 'no dma_buf_sync_for_cpu' > > > now is critical, no problem. We can also provide this patch, and seek > > > to revert it or fix it up properly later in the event it turns out it > > > causes issues. > > > > What is important is you organize things going forward to be able to > > do this properly, which means the required sync type is dependent on > > the actual page being synced and you will eventually somehow learn > > which is required from the dmabuf. > > > > Most likely nobody will ever run this code on system where dma_sync is > > not a NOP, but we should still use the DMA API properly and things > > should make architectural sense. > > > > Makes sense. OK, we can do what Jakub suggested in the thread earlier. > I.e. likely some wrapper which skips the dma_sync_for_cpu if the > netmem is unreadable. > Thanks a lot for confirmation about it. I will pass the PP_FLAG_ALLOW_UNREADABLE_NETMEM flag regardless of enabling/disabling devmem TCP in a v4 patch. The page_pool core logic will handle flags properly. I think patches for changes of page_pool are worked on by Mina, so I will not include changes for page_pool in a v4 patch. If you think I missed something, please let me know :) Thanks a lot! Taehee Yoo
