On 2021-03-12 8:51 a.m., Robin Murphy wrote: > On 2021-03-11 23:31, Logan Gunthorpe wrote: >> Hi, >> >> This is a rework of the first half of my RFC for doing P2PDMA in >> userspace >> with O_DIRECT[1]. >> >> The largest issue with that series was the gross way of flagging P2PDMA >> SGL segments. This RFC proposes a different approach, (suggested by >> Dan Williams[2]) which uses the third bit in the page_link field of the >> SGL. >> >> This approach is a lot less hacky but comes at the cost of adding a >> CONFIG_64BIT dependency to CONFIG_PCI_P2PDMA and using up the last >> scarce bit in the page_link. For our purposes, a 64BIT restriction is >> acceptable but it's not clear if this is ok for all usecases hoping >> to make use of P2PDMA. >> >> Matthew Wilcox has already suggested (off-list) that this is the wrong >> approach, preferring a new dma mapping operation and an SGL >> replacement. I >> don't disagree that something along those lines would be a better long >> term solution, but it involves overcoming a lot of challenges to get >> there. Creating a new mapping operation still means adding support to >> more >> than 25 dma_map_ops implementations (many of which are on obscure >> architectures) or creating a redundant path to fallback with dma_map_sg() >> for every driver that uses the new operation. This RFC is an approach >> that doesn't require overcoming these blocks. > > I don't really follow that argument - you're only adding support to two > implementations with the awkward flag, so why would using a dedicated > operation instead be any different? Whatever callers need to do if > dma_pci_p2pdma_supported() says no, they could equally do if > dma_map_p2p_sg() (or whatever) returns -ENXIO, no? The thing is if the dma_map_sg doesn't support P2PDMA then P2PDMA transactions cannot be done, but regular transactions can still go through as they always did. But replacing dma_map_sg() with dma_map_new() affects all operations, P2PDMA or otherwise. If dma_map_new() isn't supported it can't simply not support P2PDMA; it has to maintain a fallback path to dma_map_sg(). Given that the inputs and outputs for dma_map_new() will be completely different data structures this will be quite a lot of similar paths required in the driver. (ie mapping a bvec to the input struct and the output struct to hardware requirements) If a bug crops up in the old dma_map_sg(), developers might not notice it for some time seeing it won't be used on the most popular architectures. Logan
