HMM_MIRROR has less than useful help text

HMM_MIRROR has less than useful help text

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

Hi!

Commit c0b124054f9e42eb6da545a10fe9122a7d7c3f72 has very nice commit
message, explaining what HMM_MIRROR is and when it is
needed. Unfortunately, it did not make it into Kconfig help:

CONFIG_HMM_MIRROR:

Select HMM_MIRROR if you want to mirror range of the CPU page table of
a
process into a device page table. Here, mirror means "keep
synchronized".
Prerequisites: the device must provide the ability to write-protect
its
page tables (at PAGE_SIZE granularity), and must be able to recover
from
the resulting potential page faults.

Could that be fixed?

This is key information for me:

# This is a heterogeneous memory management (HMM) process address space
# mirroring.
# This is useful for NVidia GPU >= Pascal, Mellanox IB >= mlx5 and more
# hardware in the future.

Thanks,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: HMM_MIRROR has less than useful help text

[Balbir Singh]

On Wed, Jul 17, 2019 at 5:41 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> Commit c0b124054f9e42eb6da545a10fe9122a7d7c3f72 has very nice commit
> message, explaining what HMM_MIRROR is and when it is
> needed. Unfortunately, it did not make it into Kconfig help:
>
> CONFIG_HMM_MIRROR:
>
> Select HMM_MIRROR if you want to mirror range of the CPU page table of
> a
> process into a device page table. Here, mirror means "keep
> synchronized".
> Prerequisites: the device must provide the ability to write-protect
> its
> page tables (at PAGE_SIZE granularity), and must be able to recover
> from
> the resulting potential page faults.
>
> Could that be fixed?
>
> This is key information for me:
>
> # This is a heterogeneous memory management (HMM) process address space
> # mirroring.
> # This is useful for NVidia GPU >= Pascal, Mellanox IB >= mlx5 and more
> # hardware in the future.
>

That seems like a reasonable request

Balbir

> Thanks,
>                                                                 Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: HMM_MIRROR has less than useful help text

[Dan Williams]

On Thu, Jul 18, 2019 at 4:04 AM Balbir Singh <bsingharora@gmail.com> wrote:
>
> On Wed, Jul 17, 2019 at 5:41 PM Pavel Machek <pavel@ucw.cz> wrote:
> >
> > Hi!
> >
> > Commit c0b124054f9e42eb6da545a10fe9122a7d7c3f72 has very nice commit
> > message, explaining what HMM_MIRROR is and when it is
> > needed. Unfortunately, it did not make it into Kconfig help:
> >
> > CONFIG_HMM_MIRROR:
> >
> > Select HMM_MIRROR if you want to mirror range of the CPU page table of
> > a
> > process into a device page table. Here, mirror means "keep
> > synchronized".
> > Prerequisites: the device must provide the ability to write-protect
> > its
> > page tables (at PAGE_SIZE granularity), and must be able to recover
> > from
> > the resulting potential page faults.
> >
> > Could that be fixed?
> >
> > This is key information for me:
> >
> > # This is a heterogeneous memory management (HMM) process address space
> > # mirroring.
> > # This is useful for NVidia GPU >= Pascal, Mellanox IB >= mlx5 and more
> > # hardware in the future.
> >
>
> That seems like a reasonable request

Hi Pavel, care to send a patch?

Re: HMM_MIRROR has less than useful help text

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

On Thu 2019-07-18 07:25:42, Dan Williams wrote:
> On Thu, Jul 18, 2019 at 4:04 AM Balbir Singh <bsingharora@gmail.com> wrote:
> >
> > On Wed, Jul 17, 2019 at 5:41 PM Pavel Machek <pavel@ucw.cz> wrote:
> > >
> > > Hi!
> > >
> > > Commit c0b124054f9e42eb6da545a10fe9122a7d7c3f72 has very nice commit
> > > message, explaining what HMM_MIRROR is and when it is
> > > needed. Unfortunately, it did not make it into Kconfig help:
> > >
> > > CONFIG_HMM_MIRROR:
> > >
> > > Select HMM_MIRROR if you want to mirror range of the CPU page table of
> > > a
> > > process into a device page table. Here, mirror means "keep
> > > synchronized".
> > > Prerequisites: the device must provide the ability to write-protect
> > > its
> > > page tables (at PAGE_SIZE granularity), and must be able to recover
> > > from
> > > the resulting potential page faults.
> > >
> > > Could that be fixed?
> > >
> > > This is key information for me:
> > >
> > > # This is a heterogeneous memory management (HMM) process address space
> > > # mirroring.
> > > # This is useful for NVidia GPU >= Pascal, Mellanox IB >= mlx5 and more
> > > # hardware in the future.
> > >
> >
> > That seems like a reasonable request
> 
> Hi Pavel, care to send a patch?

I hoped patch author would fix up their code. I'm not HMM expert, he
should be...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[john.hubbard]

From: John Hubbard <jhubbard@nvidia.com>

The HMM_MIRROR option in Kconfig is a little underdocumented and
mysterious, and leaves people wondering whether to enable it.

Add text explaining just a little bit more about HMM, and also
mention which hardware would benefit from having HMM_MIRROR
enabled.

Suggested-by: Pavel Machek <pavel@ucw.cz>
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Jerome Glisse <jglisse@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---

Hi Pavel and all, does this help? I've tried to capture the key missing bits
of documentation, but still keep it small, for Kconfig.

thanks,
John Hubbard
NVIDIA

 mm/Kconfig | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/mm/Kconfig b/mm/Kconfig
index 56cec636a1fc..2fcb92e7f696 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -681,11 +681,18 @@ config HMM_MIRROR
 	depends on MMU && 64BIT
 	select MMU_NOTIFIER
 	help
-	  Select HMM_MIRROR if you want to mirror range of the CPU page table of a
-	  process into a device page table. Here, mirror means "keep synchronized".
-	  Prerequisites: the device must provide the ability to write-protect its
-	  page tables (at PAGE_SIZE granularity), and must be able to recover from
-	  the resulting potential page faults.
+	  This is Heterogeneous Memory Management (HMM) process address space
+	  mirroring.
+
+	  HMM_MIRROR provides a way to mirror ranges of the CPU page tables
+	  of a process into a device page table. Here, mirror means "keep
+	  synchronized". Prerequisites: the device must provide the ability
+	  to write-protect its page tables (at PAGE_SIZE granularity), and
+	  must be able to recover from the resulting potential page faults.
+
+	  Select HMM_MIRROR if you have hardware that meets the above
+	  description. An early, partial list of such hardware is:
+	  an NVIDIA GPU >= Pascal, Mellanox IB >= mlx5, or an AMD GPU.
 
 config DEVICE_PRIVATE
 	bool "Unaddressable device memory (GPU memory, ...)"
-- 
2.22.0

Re: [PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[Ira Weiny]

On Thu, Jul 18, 2019 at 06:32:53PM -0700, john.hubbard@gmail.com wrote:
> From: John Hubbard <jhubbard@nvidia.com>
> 
> The HMM_MIRROR option in Kconfig is a little underdocumented and
> mysterious, and leaves people wondering whether to enable it.
> 
> Add text explaining just a little bit more about HMM, and also
> mention which hardware would benefit from having HMM_MIRROR
> enabled.
> 
> Suggested-by: Pavel Machek <pavel@ucw.cz>
> Cc: Balbir Singh <bsingharora@gmail.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Jerome Glisse <jglisse@redhat.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
> 
> Hi Pavel and all, does this help? I've tried to capture the key missing bits
> of documentation, but still keep it small, for Kconfig.
> 
> thanks,
> John Hubbard
> NVIDIA
> 
>  mm/Kconfig | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 56cec636a1fc..2fcb92e7f696 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -681,11 +681,18 @@ config HMM_MIRROR
>  	depends on MMU && 64BIT
>  	select MMU_NOTIFIER
>  	help
> -	  Select HMM_MIRROR if you want to mirror range of the CPU page table of a
> -	  process into a device page table. Here, mirror means "keep synchronized".
> -	  Prerequisites: the device must provide the ability to write-protect its
> -	  page tables (at PAGE_SIZE granularity), and must be able to recover from
> -	  the resulting potential page faults.
> +	  This is Heterogeneous Memory Management (HMM) process address space
> +	  mirroring.
> +
> +	  HMM_MIRROR provides a way to mirror ranges of the CPU page tables
> +	  of a process into a device page table. Here, mirror means "keep
> +	  synchronized". Prerequisites: the device must provide the ability
> +	  to write-protect its page tables (at PAGE_SIZE granularity), and
> +	  must be able to recover from the resulting potential page faults.
> +
> +	  Select HMM_MIRROR if you have hardware that meets the above
> +	  description. An early, partial list of such hardware is:
> +	  an NVIDIA GPU >= Pascal, Mellanox IB >= mlx5, or an AMD GPU.

I don't think we want to put device information here.  If we want that
information in Kconfig best to put it in the devices themselves.  Otherwise
this list will get stale.

Other than that, looks good.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Ira

>  
>  config DEVICE_PRIVATE
>  	bool "Unaddressable device memory (GPU memory, ...)"
> -- 
> 2.22.0
> 

Re: [PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[John Hubbard]

On 7/18/19 9:34 PM, Ira Weiny wrote:
> On Thu, Jul 18, 2019 at 06:32:53PM -0700, john.hubbard@gmail.com wrote:
>> From: John Hubbard <jhubbard@nvidia.com>
...
>> +	  Select HMM_MIRROR if you have hardware that meets the above
>> +	  description. An early, partial list of such hardware is:
>> +	  an NVIDIA GPU >= Pascal, Mellanox IB >= mlx5, or an AMD GPU.
> 
> I don't think we want to put device information here.  If we want that
> information in Kconfig best to put it in the devices themselves.  Otherwise
> this list will get stale.
> 
> Other than that, looks good.
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> 

Hi Ira, thanks for the review, I'll remove that last sentence. I'll post a v2
with your reviewed by tag, in a new email thread. But first I'll wait to see 
if there are other replies.

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[Christoph Hellwig]

On Thu, Jul 18, 2019 at 06:32:53PM -0700, john.hubbard@gmail.com wrote:
> +	  HMM_MIRROR provides a way to mirror ranges of the CPU page tables
> +	  of a process into a device page table. Here, mirror means "keep
> +	  synchronized". Prerequisites: the device must provide the ability
> +	  to write-protect its page tables (at PAGE_SIZE granularity), and
> +	  must be able to recover from the resulting potential page faults.
> +
> +	  Select HMM_MIRROR if you have hardware that meets the above
> +	  description. An early, partial list of such hardware is:
> +	  an NVIDIA GPU >= Pascal, Mellanox IB >= mlx5, or an AMD GPU.

Nevermind that the Nvidia support is stagaging and looks rather broken,
there is no Mellanox user of this either at this point.

But either way this has no business in a common kconfig help.  Just
drop the fine grained details and leave it to the overview.

Re: [PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

On Thu 2019-07-18 22:57:48, Christoph Hellwig wrote:
> On Thu, Jul 18, 2019 at 06:32:53PM -0700, john.hubbard@gmail.com wrote:
> > +	  HMM_MIRROR provides a way to mirror ranges of the CPU page tables
> > +	  of a process into a device page table. Here, mirror means "keep
> > +	  synchronized". Prerequisites: the device must provide the ability
> > +	  to write-protect its page tables (at PAGE_SIZE granularity), and
> > +	  must be able to recover from the resulting potential page faults.
> > +
> > +	  Select HMM_MIRROR if you have hardware that meets the above
> > +	  description. An early, partial list of such hardware is:
> > +	  an NVIDIA GPU >= Pascal, Mellanox IB >= mlx5, or an AMD GPU.
> 
> Nevermind that the Nvidia support is stagaging and looks rather broken,
> there is no Mellanox user of this either at this point.
> 
> But either way this has no business in a common kconfig help.  Just
> drop the fine grained details and leave it to the overview.

I disagree here. This explains what kind of hardware this is for (very
new). Partial list does not hurt, and I know that I probably don't
need to enable this.

How else am I supposed to know if my computer needs page tables
synchronized?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[Jason Gunthorpe]

On Fri, Jul 19, 2019 at 12:52:39PM +0200, Pavel Machek wrote:
> On Thu 2019-07-18 22:57:48, Christoph Hellwig wrote:
> > On Thu, Jul 18, 2019 at 06:32:53PM -0700, john.hubbard@gmail.com wrote:
> > > +	  HMM_MIRROR provides a way to mirror ranges of the CPU page tables
> > > +	  of a process into a device page table. Here, mirror means "keep
> > > +	  synchronized". Prerequisites: the device must provide the ability
> > > +	  to write-protect its page tables (at PAGE_SIZE granularity), and
> > > +	  must be able to recover from the resulting potential page faults.
> > > +
> > > +	  Select HMM_MIRROR if you have hardware that meets the above
> > > +	  description. An early, partial list of such hardware is:
> > > +	  an NVIDIA GPU >= Pascal, Mellanox IB >= mlx5, or an AMD GPU.
> > 
> > Nevermind that the Nvidia support is stagaging and looks rather broken,
> > there is no Mellanox user of this either at this point.
> > 
> > But either way this has no business in a common kconfig help.  Just
> > drop the fine grained details and leave it to the overview.
> 
> I disagree here. This explains what kind of hardware this is for (very
> new). Partial list does not hurt, and I know that I probably don't
> need to enable this.
> 
> How else am I supposed to know if my computer needs page tables
> synchronized?

It is like MMU_NOTIFIERS, if something needs it, then it will select
it.

Maybe it should just be a hidden kconfig anyhow as there is no reason
to turn it on without also turning on a using driver.

Jason

Re: [PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[Christoph Hellwig]

On Fri, Jul 19, 2019 at 08:48:53AM -0300, Jason Gunthorpe wrote:
> It is like MMU_NOTIFIERS, if something needs it, then it will select
> it.
> 
> Maybe it should just be a hidden kconfig anyhow as there is no reason
> to turn it on without also turning on a using driver.

We can't just select it due to the odd X86_64 || PPC64 dependency.

Which also answers Pavels question:  you never really need it, as we
can only use it for optional functionality due to that.

Re: [PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

On Fri 2019-07-19 05:00:43, Christoph Hellwig wrote:
> On Fri, Jul 19, 2019 at 08:48:53AM -0300, Jason Gunthorpe wrote:
> > It is like MMU_NOTIFIERS, if something needs it, then it will select
> > it.
> > 
> > Maybe it should just be a hidden kconfig anyhow as there is no reason
> > to turn it on without also turning on a using driver.
> 
> We can't just select it due to the odd X86_64 || PPC64 dependency.
> 
> Which also answers Pavels question:  you never really need it, as we
> can only use it for optional functionality due to that.

Okay, just explain it in the help text :-)..

Alternatively... you can have WANT_HMM_MIRROR option drivers select,
and option HMM_MIRROR which is yes if WANT_HMM_MIRROR && (X86_64 ||
PPC64), no?

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[John Hubbard]

On 7/19/19 5:04 AM, Pavel Machek wrote:
> On Fri 2019-07-19 05:00:43, Christoph Hellwig wrote:
>> On Fri, Jul 19, 2019 at 08:48:53AM -0300, Jason Gunthorpe wrote:
>>> It is like MMU_NOTIFIERS, if something needs it, then it will select
>>> it.
>>>
>>> Maybe it should just be a hidden kconfig anyhow as there is no reason
>>> to turn it on without also turning on a using driver.
>>
>> We can't just select it due to the odd X86_64 || PPC64 dependency.
>>
>> Which also answers Pavels question:  you never really need it, as we
>> can only use it for optional functionality due to that.
> 
> Okay, just explain it in the help text :-)..
> 
> Alternatively... you can have WANT_HMM_MIRROR option drivers select,
> and option HMM_MIRROR which is yes if WANT_HMM_MIRROR && (X86_64 ||
> PPC64), no?
> 

Yes. This really should be a hidden option that just auto-enables. It's
not ideal to require people to both *find* HMM_MIRROR, *and* figure out 
that they need it. (I think it's just this way due to the history of how
HMM got merged--it started off as a kind of experimental sandbox, so
it had it's own config options, to avoid bothering anything else.)

I'll send out a new patch to just auto-select. The WANT_HMM_MIRROR
approach seems accurate, given the (X86_64 || PPC64) complication, probably
after -rc1 is ready (I don't see the ODP code using HMM yet, so that
must not have been merged yet.)

Longer term, I vaguely recall that there is no strong reason preventing 
HMM from being made to work on other arches, and am hoping that it was
just done this way to save development time. I don't want to leave it 
this way unless there's a good reason to.

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[Jason Gunthorpe]

On Fri, Jul 19, 2019 at 01:38:28PM -0700, John Hubbard wrote:
> On 7/19/19 5:04 AM, Pavel Machek wrote:
> > On Fri 2019-07-19 05:00:43, Christoph Hellwig wrote:
> >> On Fri, Jul 19, 2019 at 08:48:53AM -0300, Jason Gunthorpe wrote:
> >>> It is like MMU_NOTIFIERS, if something needs it, then it will select
> >>> it.
> >>>
> >>> Maybe it should just be a hidden kconfig anyhow as there is no reason
> >>> to turn it on without also turning on a using driver.
> >>
> >> We can't just select it due to the odd X86_64 || PPC64 dependency.
> >>
> >> Which also answers Pavels question:  you never really need it, as we
> >> can only use it for optional functionality due to that.
> > 
> > Okay, just explain it in the help text :-)..
> > 
> > Alternatively... you can have WANT_HMM_MIRROR option drivers select,
> > and option HMM_MIRROR which is yes if WANT_HMM_MIRROR && (X86_64 ||
> > PPC64), no?
> > 
> 
> Yes. This really should be a hidden option that just auto-enables. It's
> not ideal to require people to both *find* HMM_MIRROR, *and* figure out 
> that they need it. (I think it's just this way due to the history of how
> HMM got merged--it started off as a kind of experimental sandbox, so
> it had it's own config options, to avoid bothering anything else.)
> 
> I'll send out a new patch to just auto-select. The WANT_HMM_MIRROR
> approach seems accurate, given the (X86_64 || PPC64) complication, probably
> after -rc1 is ready (I don't see the ODP code using HMM yet, so that
> must not have been merged yet.)
> 
> Longer term, I vaguely recall that there is no strong reason preventing 
> HMM from being made to work on other arches, and am hoping that it was
> just done this way to save development time. I don't want to leave it 
> this way unless there's a good reason to.

No one has given me a satisfactory answer about the restriction
either.

The only thing this kconfig controls that could possibly be arch
specific is the page walking code in hmm_range_snapshot and
related. 

Maybe there is/was some arch entanglement there?

Jason

Re: [PATCH] mm/Kconfig: additional help text for HMM_MIRROR option

[Christoph Hellwig]

On Mon, Jul 22, 2019 at 08:58:04AM -0300, Jason Gunthorpe wrote:
> No one has given me a satisfactory answer about the restriction
> either.
> 
> The only thing this kconfig controls that could possibly be arch
> specific is the page walking code in hmm_range_snapshot and
> related. 
> 
> Maybe there is/was some arch entanglement there?

The page walking code is supposed to be platform independent.
I did push a branch to the buildbot a few days ago to catch issues,
and the only one found so far is an abuse of pte_index() that can be
trivially fixed.  The other thing I noticed is that the use of some
of the p??_none/protnone/present checks seems inconsistent, but I did
not have time to audit that yet.