On Sun, 2017-04-16 at 10:47 -0600, Logan Gunthorpe wrote: I'm still not 100% why do you need a "p2mem device" mind you ... Cheers, Ben.

On Sun, 2017-04-16 at 23:13 -0600, Logan Gunthorpe wrote: But is it ? For example take a GPU, does it, in your scheme, need an additional "p2pmem" child ? Why can't the GPU driver just use some helper to instantiate the necessary struct pages ? What does having an actual "struct device" child buys you ? Yup, but you already have one in the actual pci_dev ... What is the benefit of adding a second one ? Which isn't terribly useful in itself right ? What you care about is the "enclosing" pci_dev no ? Or am I missing something ? What is "some p2pmem" ? Again what is "some p2pmem" ? That a device might have some memory-like buffer space is all well and good but does it need to be specifically distinguished at the device level ? It could be inherent to what the device is... for example again take the GPU example, why would you call the FB memory "p2pmem" ? Again I'm not sure why it needs to "instanciate a p2pmem" device. Maybe it's the term "p2pmem" that offputs me. If p2pmem allowed to have a standard way to lookup the various offsets etc... I mentioned earlier, then yes, it would make sense to have it as a staging point. As-is, I don't know. Yup, I'll have another look a bit more in depth. Cheers, Ben.

On Mon, Apr 17, 2017 at 9:52 AM, Logan Gunthorpe <logang(a)deltatee.com&gt; wrote: I think you want to go the other way in the hierarchy and find a shared *parent* to land the p2pmem capability. Because that same agent is going to be responsible handling address translation for the peers. Peer-dma is always going to be a property of the bus and not the end devices. Requiring each bus implementation to explicitly enable peer-to-peer support is a feature not a bug. We shouldn't design for some future possible use case. Solve it for pci and when / if another bus comes along then look at a more generic abstraction.

On Mon, Apr 17, 2017 at 10:52:29AM -0600, Logan Gunthorpe wrote: I disagree here. I would rather see Peer-to-Peer mapping as a form of helper so that device driver can opt-in for multiple mecanisms concurrently. Like HMM and p2p. Also it seems you are having a static vision for p2p. For GPU and network the use case is you move some buffer into the device memory and then you create mapping for some network adapter while the buffer is in device memory. But this is only temporary and buffer might move to different device memory. So usecase is highly dynamic (well mapping lifetime is still probably few second/minutes). I see no reason for exposing sysfs tree to userspace for all this. This isn't too dynamic, either 2 devices can access each others memory, either they can't. This can be hidden through the device kernel API. Again for GPU the idea is that it is always do-able in the sense that when it is not you fallback to using system memory. Yes this could conflict and that's why i would rather see this as a set of helper like HMM is doing. So device driver can opt-in HMM and p2pmem at the same time. This seems to duplicate things that already exist in each individual driver. If a device has memory than device driver already have some form of memory management and most likely expose some API to userspace to allow program to use that memory. Peer to peer DMA mapping is orthogonal to memory management, it is an optimization ie you have some buffer allocated through some device driver specific IOCTL and now you want some other device to directly access it. Having to first do another allocation in a different device driver for that to happen seems overkill. What you really want from device driver point of view is an helper to first tell you if 2 device can access each other and second an helper that allow the second device to import the other device memory to allow direct access. Discovering possible peer is a onetime only thing and designing around that is wrong in my view. There is already existing hierarchy in kernel for that in the form of the bus hierarchy (i am thinking pci bus here). So there is already existing way to discover this and you are just duplicating informations here. Cheers, Jérôme

On Mon, 2017-04-17 at 10:52 -0600, Logan Gunthorpe wrote: Is it ? Again, you create a "concept" the user may have no idea about, "p2pmem memory". So now any kind of memory buffer on a device can could be use for p2p but also potentially a bunch of other things becomes special and called "p2pmem" ... But what do you have in p2pmem that somebody benefits from. Again I don't understand what that "p2pmem" device buys you in term of functionality vs. having the device just instanciate the pages. Now having some kind of way to override the dma_ops, yes I do get that, and it could be that this "p2pmem" is typically the way to do it, but at the moment you don't even have that. So I'm a bit at a loss here. But it doesn't *have* to be. Again, take my GPU example. The fact that a NIC might be able to DMA into it doesn't make it specifically "p2p memory". Essentially you are saying that any device that happens to have a piece of mappable "memory" (or something that behaves like it) and can be DMA'ed into should now have that "p2pmem" thing attached to it. Now take an example where that becomes really awkward (it's also a real example of something people want to do). I have a NIC and a GPU, the NIC DMA's data to/from the GPU, but they also want to poke at each other doorbell, the GPU to kick the NIC into action when data is ready to send, the NIC to poke the GPU when data has been received. Those doorbells are MMIO registers. So now your "p2pmem" device needs to also be laid out on top of those MMIO registers ? It's becoming weird. See, basically, doing peer 2 peer between devices has 3 main challenges today: The DMA API needing struct pages, the MMIO translation issues and the IOMMU translation issues. You seem to create that added device as some kind of "owner" for the struct pages, solving #1, but leave #2 and #3 alone. Now, as I said, it could very well be that having the devmap pointer point to some specific device-type with a well known structure to provide solutions for #2 and #3 such as dma_ops overrides, is indeed the right way to solve these problems. If we go down that path, though, rather than calling it p2pmem I would call it something like dma_target which I find much clearer especially since it doesn't have to be just memory. For the sole case of creating struct page's however, I fail to see the point. If we are going to create some sort of struct dma_target, HMM could potentially just look for the parent if it needs the PCI device. But why not directly use that other bus' device in that case ? So back to my base objections: - There is no reason why this has to just be memory. There are good reasons to want to do peer DMA to MMIO registers (see above) - There is no reason why that memory on a device is specifically dedicated to "peer to peer" and thus calling it "p2pmem" is something I find actually confusing. Im not only using it for that :) Except it doesn't handle any of the dma_map stuff today as far as I can see. The memory allocation should be a completely orthogonal and separate thing yes. You are conflating two completely different things now into a single concept. No, the allocation should not even have links to the DMA peering mechanism. This is completely orthogonal. I feel more and more like your entire infrastructure is designed for a special use case and conflates several problems of that specific use case into one single "solution" rather than separating the various problems and solving them independently.

On Mon, 2017-04-17 at 23:43 -0600, Logan Gunthorpe wrote: Not necessarily. There are many cases where the "end user" won't have any idea. In any case, I think we bring the story down to those two points of conflating the allocator with the peer to peer DMA, and the lack of generality in the approach to solve the peer to peer DMA problem. Now now now .... calm down. We are being civil here. I'm not shitting on anything, I'm asking what seems to be a reasonable question in term of benefits of the approach you have chosen. Using that sort of language will not get you anywhere. But there is no such thing as a "p2pmem" device.. that's what I'm trying to tell you... As both Jerome and I tried to explain, there are many reason why one may want to do peer DMA into some device memory, that doesn't make that memory some kind of "p2pmem". It's trying to stick a generic label onto something that isn't. That's why I'm suggesting we disconnect the two aspects. On one hand the problem of handling p2p DMA, whether the target is some memory, some MMIO registers, etc... On the other hand, some generic "utility" that can optionally be used by drivers to manage a pool of DMA memory in the device, essentially a simple allocator. The two things are completely orthogonal. Possibly. In any case, I think it should be separate from the allocation. But it tries to conflate the allocation, which is basically the fact that this is some kind of "memory pool" with the problem of doing peer DMA. I'm advocating for separating the concepts. It's not bike shedding. It's about taking out the allocator part and making it clear that this isn't something to lay out on top of a pre- decided chunk of "memory". Again, you are bringing everything down to your special case of "p2p memory". That's where you lose me. This looks like a special case to me and you are making the centre point of your design. What we need is: - On one hand a way to expose device space (whether it's MMIO registers, memory, something else ...) to the DMA ops so another device can do standard dma_map_* to/from it. (Not dma_alloc_* those shouldn't relate to p2p at all, they are intended for a driver own allocation for the device it manages). This includes the creation of struct pages and the mechanism to override/adjust the dma_ops etc.... along with all the PCI specific gunk to figure out if we are on the same bus or not etc. - Some kind of generic utility you can use to manage a pool of "memory" which seems to be what your special devices use or wantf or use by peer DMA. We are s seen as a benefit to us. I'm not disagreeing. I'm saying that it is completely orthogonal to the solving the the DMA peer issue. I'm simply objecting to conflating the two. Cheers, Ben.

On Mon, Apr 17, 2017 at 08:23:16AM +1000, Benjamin Herrenschmidt wrote: A lot of people feel this way in the RDMA community too. We have had vendors shipping out of tree code to enable P2P for RDMA with GPU years and years now. :( Attempts to get things in mainline have always run into the same sort of road blocks you've identified in this thread.. FWIW, I read this discussion and it sounds closer to an agreement than I've ever seen in the past. is needed here is simply a function to return the 'struct device' backing a CPU address range. This is the minimal required information for the arch or IOMMU code under the dma ops to figure out the fabric source/dest, compute the traffic path, determine if P2P is even possible, what translation hardware is crossed, and what DMA address should be used. If there is going to be more core support for this stuff I think it will be under the topic of more robustly describing the fabric to the core and core helpers to extract data from the description: eg compute the path, check if the path crosses translation, etc But that isn't really related to P2P, and is probably better left to the arch authors to figure out where they need to enhance the existing topology data.. I think the key agreement to get out of Logan's series is that P2P DMA means: - The BAR will be backed by struct pages - Passing the CPU __iomem address of the BAR to the DMA API is valid and, long term, dma ops providers are expected to fail or return the right DMA address - Mapping BAR memory into userspace and back to the kernel via get_user_pages works transparently, and with the DMA API above - The dma ops provider must be able to tell if source memory is bar mapped and recover the pci device backing the mapping. At least this is what we'd like in RDMA :) FWIW, RDMA probably wouldn't want to use a p2mem device either, we already have APIs that map BAR memory to user space, and would like to keep using them. A 'enable P2P for bar' helper function sounds better to me. Jason

On Tue, Apr 18, 2017 at 10:27:47AM -0700, Dan Williams wrote: What I mean I could write in a RDMA driver: /* Allow the memory in BAR 1 to be the target of P2P transactions */ pci_enable_p2p_bar(dev, 1); And not require anything else.. Not sure, until we see what arches actually need to do here it is hard to design common helpers. Here are a few obvious things that arches will need to implement to support this broadly: - Virtualization might need to do a hypervisor call to get the right translation, or consult some hypervisor specific description table. - Anything using IOMMUs for virtualization will need to setup IOMMU permissions to allow the P2P flow, this might require translation to an address cookie. - Fail if the PCI devices are in different domains, or setup hardware to do completion bus/device/function translation. - All platforms can succeed if the PCI devices are under the same 'segment', but where segments begin is somewhat platform specific knowledge. (this is 'same switch' idea Logan has talked about) So, we can eventually design helpers for various common scenarios, but until we see what arch code actually needs to do it seems premature. Much of this seems to involve interaction with some kind of hardware, or consulation of some kind of currently platform specific data, so I'm not sure what a software-iommu would be doing?? The main thing to agree on is that this code belongs under dma ops and that arches have to support struct page mapped BAR addresses in their dma ops inputs. Is that resonable? Jason

On Tue, 2017-04-18 at 12:00 -0600, Jason Gunthorpe wrote: We also need to be careful whether P2P is enabled in the switch or not. Cheers, Ben.

On Tue, Apr 18, 2017 at 3:46 PM, Benjamin Herrenschmidt <benh(a)kernel.crashing.org&gt; wrote: Yes.

On Tue, Apr 18, 2017 at 12:30:59PM -0600, Logan Gunthorpe wrote: I think that is how it will end up working out if this is the path.. Ultimately every dma_ops will need special code to support P2P with the special hardware that ops is controlling, so it makes some sense to start by pushing the check down there in the first place. This advice is partially motivated by how dma_map_sg is just a small wrapper around the function pointer call... Something like: foo_dma_map_sg(...) { for (every page in sg) if (page is p2p) dma_addr[I] = p2p_same_segment_map_page(...); } Where p2p_same_segment_map_page checks if the two devices are on the 'same switch' and if so returns the address translated to match the bus address programmed into the BAR or fails. We knows this case is required to work by the PCI spec, so it makes sense to use it as the first canned helper. This also proves out the basic idea that the dma ops can recover the pci device and perform an inspection of the traversed fabric path. support a wider range of things. Eg x86 with no iommu and no offset could allow every address to be used based on a host bridge white list. Jason

On Tue, Apr 18, 2017 at 12:35 PM, Logan Gunthorpe <logang(a)deltatee.com&gt; wrote: I don't think you need to go touch all dma_ops, I think you can just arrange for devices that are going to do dma to get redirected to a p2p aware provider of operations that overrides the system default dma_ops. I.e. just touch get_dma_ops().

On Tue, Apr 18, 2017 at 1:29 PM, Jerome Glisse <jglisse(a)redhat.com&gt; wrote: ...and that dma_map goes through get_dma_ops(), so I don't see the conflict?

On Tue, 2017-04-18 at 14:48 -0600, Logan Gunthorpe wrote: We can handle that in our "overriden" dma ops. It's a bit tricky but it *could* break it down into segments and forward portions back to the original dma ops. Ben.

On Tue, Apr 18, 2017 at 2:03 PM, Jason Gunthorpe <jgunthorpe(a)obsidianresearch.com&gt; wrote: No, I don't think it does. You'd only shim when the target page is backed by a device, not host memory, and you can figure this out by a is_zone_device_page()-style lookup.

On Tue, Apr 18, 2017 at 2:22 PM, Jason Gunthorpe <jgunthorpe(a)obsidianresearch.com&gt; wrote: This goes back to my original comment to make this capability a function of the pci bridge itself. The kernel has an implementation of a dynamically created bridge device that injects its own dma_ops for the devices behind the bridge. See vmd_setup_dma_ops() in drivers/pci/host/vmd.c. Check to see if the arch requires this offset translation that Ben brought up and if not provide the physical address as the patches are doing now.

On Tue, Apr 18, 2017 at 3:15 PM, Logan Gunthorpe <logang(a)deltatee.com&gt; wrote: Unlike the pci bus address offset case which I think is fundamental to support since shipping archs do this today, I think it is ok to say p2p is restricted to a single sgl that gets to talk to host memory or a single device. That said, what's wrong with a p2p aware map_sg implementation calling up to the host memory map_sg implementation on a per sgl basis? This goes back to limiting p2p to a single pci host bridge. If the p2p capability is coordinated with the bridge rather than between the individual devices then we have a central point to catch this case. ...of course this is all hand wavy until someone writes the code and proves otherwise. It's at least a proof point for injecting special behavior for devices behind a (virtual) pci bridge without needing to go touch a bunch of drivers.

On Tue, Apr 18, 2017 at 3:42 PM, Jason Gunthorpe <jgunthorpe(a)obsidianresearch.com&gt; wrote: I don't think the root bus iommu drivers have any business knowing or caring about dma happening between devices lower in the hierarchy. When we're crossing device and host memory boundaries how much batching is possible? As far as I can see you'll always be splitting the sgl on these dma mapping boundaries.

On Tue, 2017-04-18 at 17:21 -0600, Jason Gunthorpe wrote: I think it's easier to get us started to just use a helper and stick it in the existing sglist processing loop of the architecture. As we noticed, stacking dma_ops is actually non-trivial and opens quite the can of worms. As Jerome mentioned, you can end up with IOs ops containing an sglist that is a collection of memory and GPU pages for example. Cheers, Ben.

On Tue, Apr 18, 2017 at 3:48 PM, Logan Gunthorpe <logang(a)deltatee.com&gt; wrote: No, we can't do that at get_user_pages() time, it will always need to be up to the device driver to fail dma that it can't perform.

On Tue, Apr 18, 2017 at 3:56 PM, Logan Gunthorpe <logang(a)deltatee.com&gt; wrote: Ah, I see what you're saying now. Yes, we do need something that guarantees any dma mapping implementation that gets a struct page that it does now know how to translate properly fails the request.

On 18/04/17 04:24 PM, Jason Gunthorpe wrote: Yes, unfortunately, I have to agree with this statement completely. Yeah, I'm also personally thinking your proposal is the way to go as well. Dan's injected ops suggestion is interesting but I can't see how it solves the issue completely. Your proposal is the only one that seems to be complete to me. It just has a few minor pain points which I've already described but are likely manageable and less than the pain stacked dma_ops creates. Logan

On Tue, 2017-04-18 at 15:03 -0600, Jason Gunthorpe wrote: Good point. We only know on a per-page basis ... ugh. So we really need to change the arch main dma_ops. I'm not opposed to that. What we then need to do is have that main arch dma_map_sg, when it encounters a "device" page, call into a helper attached to the devmap to handle *that page*, providing sufficient context. That helper wouldn't perform the actual iommu mapping. It would simply return something along the lines of: - "use that alternate bus address and don't map in the iommu" - "use that alternate bus address and do map in the iommu" - "proceed as normal" - "fail" What do you think ? Cheers, Ben.

On 19/04/17 09:55 AM, Jason Gunthorpe wrote: Pivoting slightly: I was looking at how HMM uses ZONE_DEVICE. They add a type flag to the dev_pagemap structure which would be very useful to us. We could add another MEMORY_DEVICE_P2P type to distinguish p2p pages. Then, potentially, we could add a dma_map callback to the structure (possibly unioned with an hmm field). The dev_ops providers would then just need to do something like this (enclosed in a helper): if (is_zone_device_page(page)) { pgmap = get_dev_pagemap(page_to_pfn(page)); if (!pgmap || pgmap->type != MEMORY_DEVICE_P2P || !pgmap->dma_map) return 0; dma_addr = pgmap->dma_map(dev, pgmap->dev, page); put_dev_pagemap(pgmap); if (!dma_addr) return 0; ... } The pci_enable_p2p_bar function would then just need to call devm_memremap_pages with the dma_map callback set to a function that does the segment check and the offset calculation. Thoughts? @Jerome: my feedback to you would be that your patch assumes all users of devm_memremap_pages are MEMORY_DEVICE_PERSISTENT. It would be more useful if it was generic. My suggestion would be to have the caller allocate the dev_pagemap structure, populate it and pass it into devm_memremap_pages. Given that pretty much everything in that structure are already arguments to that function, I feel like this makes sense. This should also help to unify hmm_devmem_pages_create and devm_memremap_pages which look very similar to each other. Logan

On Wed, Apr 19, 2017 at 10:01:23AM -0700, Dan Williams wrote: Dan you asked me to not use devm_memremap_pages() because you didn't want to have HMM memory in the pgmap_radix, did you change opinion on that ? :) Note i won't make any change now on that front but if it make sense i am happy to do it as separate patchset on top of HMM. Also i don't want p2pmem to be an exclusive or with HMM, we will want GPU to do peer to peer DMA and thus HMM ZONE_DEVICE pages to support this too. I do believe it is easier to special case in ZONE_DEVICE into existing dma_ops for each architecture. For x86 i think there is only 3 different set of dma_ops to modify. For other arch i guess there is even less. But in all the case i think p2pmem should stay out of memory management business. If some set of device do not have memory management it is better to propose helper to do that as part of the subsystem to which those devices belong. Just wanted to reiterate that point. Cheers, Jérôme

On 19/04/17 11:41 AM, Dan Williams wrote: Would you necessarily even need a create step? I was thinking more along the lines that struct dev_pagemap _could_ just be a member in another structure. The caller would set the attributes they needed and pass it to devm_memremap. (Similar to how we commonly do things with struct device, et al). Potentially, that could also get rid of the need for the *data pointer HMM is using to get back the struct hmm_devmem seeing container_of could be used instead. That depends though. If unaddressable memory needs different steps to get to the DMA address (ie if it has to setup a gpu window) then we may still need a way to distinguish the two types of non-host memory. Logan

On Wed, Apr 19, 2017 at 11:19 AM, Logan Gunthorpe <logang(a)deltatee.com&gt; wrote: Yes, that's already how we handle struct page_map, it's an internal implementation detail of devm_memremap_pages(). Letting others users do the container_of() arrangement means that struct page_map needs to become public and move into struct dev_pagemap directly. ...I think that encapsulation loss is worth it for the gain of clearly separating the HMM-case from the base case.

On Wed, Apr 19, 2017 at 11:41 AM, Logan Gunthorpe <logang(a)deltatee.com&gt; wrote: Yeah, I forgot that dev_pagemap grew those fields, so we don't have any real encapsulation today. So this would just be a pure cleanup to kill struct page_map.

On Thu, Apr 20, 2017 at 1:43 PM, Stephen Bates <sbates(a)raithlin.com&gt; wrote: Why persistence? It has zero meaning to the mm.

On Thu, Apr 20, 2017 at 4:07 PM, Stephen Bates <sbates(a)raithlin.com&gt; wrote: We do have memory type data in the global iomem_resource tree, see IORES_DESC_PERSISTENT_MEMORY. A persistence flag does not convey endurance or latency information. Not for the mm, data at rest security might be a property of the device, but that's not the mm's concern. Maybe, but to me "Useful for the future" == "don't add it to the kernel until that future arrives". Do the IORES_DESC flags give you what you need?

On 19/04/17 11:14 AM, Jason Gunthorpe wrote: Yes, it is kind of like designing for the future. I just find it a little odd calling the pci functions in the iommu. I'm just spit balling here but if HMM wanted to use unaddressable memory as a DMA target, it could set that function to create a window ine gpu memory, then call the pci_p2p_same_segment and return the result as the dma address. I'm not sure I like the name pci_p2p_same_segment. It reads as though it's only checking if the devices are not the same segment. It also may be that, in the future, it supports devices on different segments. I'd call it more like pci_p2p_dma_map. Thanks, this code fleshes things out nicely Yes, this is true. Yup. Logan

On 19/04/17 12:32 PM, Jason Gunthorpe wrote: Hmm, yes. Though it's still not clear what, if anything, arch_p2p_cross_segment would be doing. In my experience, if you are going between host bridges, the CPU address (or PCI address -- I'm not sure which seeing they are the same on my system) would still work fine -- it just _may_ be a bad idea because of performance. Similarly if you are crossing via a QPI bus or similar, I'd expect the CPU address to work fine. But here the performance is even less likely to be any good. So, in this case, what device does the completer point to? The PCI device or a more specific GPU device? If it's the former, who's responsible for setting the new dma_ops? Typically the dma_ops are arch specific but now you'd be adding ones that are tied to hmm or the gpu. It appears to me like it's calculating the DMA address, and the check is just a side requirement. It reads as though it's only doing the check. Logan

On 19/04/17 01:31 PM, Jason Gunthorpe wrote: Ah, ok. Seems a bit awkward to me that in order for the intended use case, you have to proxy the dma_ops. I'd probably still suggest throwing a couple ops for things like this in the dev_pagemap. Ok, I think that's a bit clearer. Logan

On 19/04/17 02:48 PM, Jason Gunthorpe wrote: I feel like expanding dev_pagemap has a much lower impact than expanding struct device... dev_pagemap is only one instance per zone device region so expanding it shouldn't be a huge issue. Expanding struct device means every device struct in the system gets bigger. Logan

On Tue, Apr 18, 2017 at 01:35:32PM -0600, Logan Gunthorpe wrote: I think this is why progress on this keeps getting stuck - every solution is a lot of work. Since the mechanics of the check is essentially unique to every dma-ops I would not hoist it out of the map function without a really good reason. You could try to do a dummy mapping / create a MR early on to detect this. FWIW, I wonder if from a RDMA perspective we have another problem.. Should we allow P2P memory to be used with the local DMA lkey? There are potential designs around virtualization that would not allow that. Should we mandate that P2P memory be in its own MR? Jason