Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> writes: > On Tue, 2024-10-15 at 10:02 -0300, Jason Gunthorpe wrote: >> On Tue, Oct 15, 2024 at 02:41:24PM +0200, Thomas Hellström wrote: >> > > It has nothing to do with kernel P2P, you are just allowing more >> > > selective filtering of dev_private_owner. You should focus on >> > > that in >> > > the naming, not p2p. ie allow_dev_private() >> > > >> > > P2P is stuff that is dealing with MEMORY_DEVICE_PCI_P2PDMA. >> > >> > Yes, although the intention was to incorporate also other fast >> > interconnects in "P2P", not just "PCIe P2P", but I'll definitely >> > take a >> > look at the naming. >> >> It has nothing to do with that, you are just filtering the device >> private pages differently than default. >> >> Your end use might be P2P, but at this API level it certainly is not. > > Sure. Will find something more suitable. > >> >> > > This is just allowing more instances of the same driver to co- >> > > ordinate >> > > their device private memory handle, for whatever purpose. >> > >> > Exactly, or theoretically even cross-driver. >> >> I don't want to see things like drivers changing their pgmap handles >> privately somehow. If we are going to make it cross driver then it >> needs to be generalized alot more. > > Cross-driver is initially not a thing, so let's worry about that later. > My impression though is that this is the only change required for > hmm_range_fault() and that infrastructure for opt-in and dma-mapping > would need to be provided elsewhere? Cross-driver is tricky because the device-private pages have no meaning outside of the driver which owns/allocates them. One option is to have a callback which returns P2PDMA pages which can then be dma-mapped. See https://lore.kernel.org/linux-mm/20241015152348.3055360-1-ymaman@xxxxxxxxxx/ for an example of that. >> >> > > >> > > Otherwise I don't see a particular problem, though we have talked >> > > about widening the matching for device_private more broadly using >> > > some >> > > kind of grouping tag or something like that instead of a >> > > callback. >> > > You >> > > may consider that as an alternative >> > >> > Yes. Looked at that, but (if I understand you correctly) that would >> > be >> > the case mentioned in the commit message where the group would be >> > set >> > up statically at dev_pagemap creation time? >> >> Not necessarily statically, but the membership would be stored in the >> pagemap and by updated during hotplug/etc >> >> If this is for P2P then the dynamic behavior is pretty limited, some >> kind of NxN bitmap. >> >> > > hmm_range struct inside a caller private data struct and use that >> > > instead if inventing a whole new struct and pointer. >> > >> > Our first attempt was based on that but then that wouldn't be >> > reusable >> > in the migrate_device.c code. Hence the extra indirection. >> >> It is performance path, you should prefer duplication rather than >> slowing it down.. > > OK. Will look at duplicating. > > Thanks, > Thomas > > >> >> Jason
