Nouveau - Jan 2025 - [RFC 1/5] mm/hmm: HMM API to enable P2P DMA for device private pages

On Wed, Jan 29, 2025 at 02:38:58PM +0100, Simona Vetter wrote:
> > The pgmap->owner doesn't *have* to fixed, certainly during
early boot before
> > you hand out any page references it can be changed. I wouldn't be
> > surprised if this is useful to some requirements to build up the
> > private interconnect topology?
> 
> The trouble I'm seeing is device probe and the fundemantal issue that
you
> never know when you're done. And so if we entirely rely on
pgmap->owner to
> figure out the driver private interconnect topology, that's going to be
> messy. That's why I'm also leaning towards both comparing owners
and
> having an additional check whether the interconnect is actually there or
> not yet.
Hoenstely, I'd rather invest more effort into being able to update
owner for those special corner cases than to slow down the fast path
in hmm_range_fault..

The notion is that owner should represent a contiguous region of
connectivity. IMHO you can always create this, but I suppose there
could be corner cases where you need to split/merge owners.

But again, this isn't set in stone, if someone has a better way to
match the private interconnects without going to driver callbacks then
try that too.

I think driver callbacks inside hmm_range_fault should be the last resort..
> You can fake that by doing these checks after hmm_range_fault returned,
> and if you get a bunch of unsuitable pages, toss it back to
> hmm_range_fault asking for an unconditional migration to system memory for
> those. But that's kinda not great and I think goes at least against the
> spirit of how you want to handle pci p2p in step 2 below?
Right, hmm_range_fault should return pages that can be used and you
should not call it twice.

Jason

On Wed, 2025-01-29 at 09:47 -0400, Jason Gunthorpe
wrote:
> On Wed, Jan 29, 2025 at 02:38:58PM +0100, Simona Vetter wrote:
> 
> > > The pgmap->owner doesn't *have* to fixed, certainly during
early
> > > boot before
> > > you hand out any page references it can be changed. I
wouldn't be
> > > surprised if this is useful to some requirements to build up the
> > > private interconnect topology?
> > 
> > The trouble I'm seeing is device probe and the fundemantal issue
> > that you
> > never know when you're done. And so if we entirely rely on pgmap-
> > >owner to
> > figure out the driver private interconnect topology, that's going
> > to be
> > messy. That's why I'm also leaning towards both comparing
owners
> > and
> > having an additional check whether the interconnect is actually
> > there or
> > not yet.
> 
> Hoenstely, I'd rather invest more effort into being able to update
> owner for those special corner cases than to slow down the fast path
> in hmm_range_fault..
Just a comment on the performance concern here. This can be crafted in
a way that only if the driver provides a callback, there is a (small)
hit. If there is no callback at that point, we're looking at a
migration to ram. If there is a callback it's typically followed by an
address computation and page-table setup. Compared to those, the
callback performance impact is probably unmeasureable.

/Thomas

On Wed, Jan 29, 2025 at 09:47:57AM -0400, Jason Gunthorpe
wrote:
> On Wed, Jan 29, 2025 at 02:38:58PM +0100, Simona Vetter wrote:
> 
> > > The pgmap->owner doesn't *have* to fixed, certainly during
early boot before
> > > you hand out any page references it can be changed. I
wouldn't be
> > > surprised if this is useful to some requirements to build up the
> > > private interconnect topology?
> > 
> > The trouble I'm seeing is device probe and the fundemantal issue
that you
> > never know when you're done. And so if we entirely rely on
pgmap->owner to
> > figure out the driver private interconnect topology, that's going
to be
> > messy. That's why I'm also leaning towards both comparing
owners and
> > having an additional check whether the interconnect is actually there
or
> > not yet.
> 
> Hoenstely, I'd rather invest more effort into being able to update
> owner for those special corner cases than to slow down the fast path
> in hmm_range_fault..
I'm not sure how you want to make the owner mutable.

The only design that I think is solid is to evict all device private
memory, unregister the dev_pagemap and register a new one with the updated
owner. I think any other approach boils down to the same issue, except we
pretend it's easier and just ignore all the race conditions.

And I've looked at the lifetime fun of unregistering a dev_pagemap for
device hotunplug and pretty firmly concluded it's unfixable and that I
should run away to do something else :-P

An optional callback is a lot less scary to me here (or redoing
hmm_range_fault or whacking the migration helpers a few times) looks a lot
less scary than making pgmap->owner mutable in some fashion.

Cheers, Sima
> The notion is that owner should represent a contiguous region of
> connectivity. IMHO you can always create this, but I suppose there
> could be corner cases where you need to split/merge owners.
> 
> But again, this isn't set in stone, if someone has a better way to
> match the private interconnects without going to driver callbacks then
> try that too.
> 
> I think driver callbacks inside hmm_range_fault should be the last resort..
> 
> > You can fake that by doing these checks after hmm_range_fault
returned,
> > and if you get a bunch of unsuitable pages, toss it back to
> > hmm_range_fault asking for an unconditional migration to system memory
for
> > those. But that's kinda not great and I think goes at least
against the
> > spirit of how you want to handle pci p2p in step 2 below?
> 
> Right, hmm_range_fault should return pages that can be used and you
> should not call it twice.
> 
> Jason
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Thu, Jan 30, 2025 at 11:50:27AM +0100, Simona Vetter
wrote:
> On Wed, Jan 29, 2025 at 09:47:57AM -0400, Jason Gunthorpe wrote:
> > On Wed, Jan 29, 2025 at 02:38:58PM +0100, Simona Vetter wrote:
> > 
> > > > The pgmap->owner doesn't *have* to fixed, certainly
during early boot before
> > > > you hand out any page references it can be changed. I
wouldn't be
> > > > surprised if this is useful to some requirements to build up
the
> > > > private interconnect topology?
> > > 
> > > The trouble I'm seeing is device probe and the fundemantal
issue that you
> > > never know when you're done. And so if we entirely rely on
pgmap->owner to
> > > figure out the driver private interconnect topology, that's
going to be
> > > messy. That's why I'm also leaning towards both comparing
owners and
> > > having an additional check whether the interconnect is actually
there or
> > > not yet.
> > 
> > Hoenstely, I'd rather invest more effort into being able to update
> > owner for those special corner cases than to slow down the fast path
> > in hmm_range_fault..
> 
> I'm not sure how you want to make the owner mutable.
You'd probably have to use a system where you never free them until
all the page maps are destroyed.

You could also use an integer instead of a pointer to indicate the
cluster of interconnect, I think there are many options..
> And I've looked at the lifetime fun of unregistering a dev_pagemap for
> device hotunplug and pretty firmly concluded it's unfixable and that I
> should run away to do something else :-P
? It is supposed to work, it blocks until all the pages are freed, but
AFAIK ther is no fundamental life time issue. The driver is
responsible to free all its usage.
> An optional callback is a lot less scary to me here (or redoing
> hmm_range_fault or whacking the migration helpers a few times) looks a lot
> less scary than making pgmap->owner mutable in some fashion.
It extra for every single 4k page on every user :\

And what are you going to do better inside this callback?

Jason