Re: [PATCH 1/5] mm/hmm: Update HMM documentation
From: Ralph Campbell
Date: Thu Jun 06 2019 - 14:54:27 EST
On 6/6/19 7:02 AM, Jason Gunthorpe wrote:
On Mon, May 06, 2019 at 04:29:38PM -0700, rcampbell@xxxxxxxxxx wrote:
From: Ralph Campbell <rcampbell@xxxxxxxxxx>
Update the HMM documentation to reflect the latest API and make a few minor
wording changes.
Signed-off-by: Ralph Campbell <rcampbell@xxxxxxxxxx>
Reviewed-by: JÃrÃme Glisse <jglisse@xxxxxxxxxx>
Cc: John Hubbard <jhubbard@xxxxxxxxxx>
Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
Cc: Arnd Bergmann <arnd@xxxxxxxx>
Cc: Balbir Singh <bsingharora@xxxxxxxxx>
Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
Cc: Souptick Joarder <jrdr.linux@xxxxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Documentation/vm/hmm.rst | 139 ++++++++++++++++++++-------------------
1 file changed, 73 insertions(+), 66 deletions(-)
Okay, lets start picking up hmm patches in to the new shared hmm.git,
as promised I will take responsibility to send these to Linus. The
tree is here:
https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=hmm
This looks fine to me with one minor comment:
diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst
index ec1efa32af3c..7c1e929931a0 100644
+++ b/Documentation/vm/hmm.rst
@@ -151,21 +151,27 @@ registration of an hmm_mirror struct::
int hmm_mirror_register(struct hmm_mirror *mirror,
struct mm_struct *mm);
- int hmm_mirror_register_locked(struct hmm_mirror *mirror,
- struct mm_struct *mm);
-
-The locked variant is to be used when the driver is already holding mmap_sem
-of the mm in write mode. The mirror struct has a set of callbacks that are used
+The mirror struct has a set of callbacks that are used
to propagate CPU page tables::
struct hmm_mirror_ops {
+ /* release() - release hmm_mirror
+ *
+ * @mirror: pointer to struct hmm_mirror
+ *
+ * This is called when the mm_struct is being released.
+ * The callback should make sure no references to the mirror occur
+ * after the callback returns.
+ */
This is not quite accurate (at least, as the other series I sent
intends), the struct hmm_mirror is valid up until
hmm_mirror_unregister() is called - specifically it remains valid
after the release() callback.
I will revise it (and the hmm.h comment it came from) to read the
below. Please let me know if you'd like something else:
/* release() - release hmm_mirror
*
* @mirror: pointer to struct hmm_mirror
*
* This is called when the mm_struct is being released. The callback
* must ensure that all access to any pages obtained from this mirror
* is halted before the callback returns. All future access should
* fault.
*/
The key task for release is to fence off all device access to any
related pages as the mm is about to recycle them and the device must
not cause a use-after-free.
I applied it to hmm.git
Thanks,
Jason
Yes, I agree this is better.
Also, I noticed the sample code for hmm_range_register() is wrong.
If you could merge this minor change into this patch, that
would be appreciated.
diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst
index dc8fe4241a18..b5fb9bc02aa2 100644
--- a/Documentation/vm/hmm.rst
+++ b/Documentation/vm/hmm.rst
@@ -245,7 +245,7 @@ The usage pattern is::
hmm_range_wait_until_valid(&range, TIMEOUT_IN_MSEC);
goto again;
}
- hmm_mirror_unregister(&range);
+ hmm_range_unregister(&range);
return ret;
}
take_lock(driver->update);
@@ -257,7 +257,7 @@ The usage pattern is::
// Use pfns array content to update device page table
- hmm_mirror_unregister(&range);
+ hmm_range_unregister(&range);
release_lock(driver->update);
up_read(&mm->mmap_sem);
return 0;