Re: [RFC PATCH] mm/hmm, mm/migrate_device: Allow p2p access and p2p migration

[Thomas Hellström]

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?

> 
> > > 
> > > 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