LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jason Gunthorpe <jgg@ziepe.ca>
To: rcampbell@nvidia.com
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	John Hubbard <jhubbard@nvidia.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Balbir Singh <bsingharora@gmail.com>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Matthew Wilcox <willy@infradead.org>,
	Souptick Joarder <jrdr.linux@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 3/5] mm/hmm: Use mm_get_hmm() in hmm_range_register()
Date: Thu, 23 May 2019 09:51:08 -0300	[thread overview]
Message-ID: <20190523125108.GA14013@ziepe.ca> (raw)
In-Reply-To: <20190506232942.12623-4-rcampbell@nvidia.com>

On Mon, May 06, 2019 at 04:29:40PM -0700, rcampbell@nvidia.com wrote:
> From: Ralph Campbell <rcampbell@nvidia.com>
> 
> In hmm_range_register(), the call to hmm_get_or_create() implies that
> hmm_range_register() could be called before hmm_mirror_register() when
> in fact, that would violate the HMM API.
> 
> Use mm_get_hmm() instead of hmm_get_or_create() to get the HMM structure.
> 
> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Balbir Singh <bsingharora@gmail.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
>  mm/hmm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/hmm.c b/mm/hmm.c
> index f6c4c8633db9..2aa75dbed04a 100644
> +++ b/mm/hmm.c
> @@ -936,7 +936,7 @@ int hmm_range_register(struct hmm_range *range,
>  	range->start = start;
>  	range->end = end;
>  
> -	range->hmm = hmm_get_or_create(mm);
> +	range->hmm = mm_get_hmm(mm);
>  	if (!range->hmm)
>  		return -EFAULT;

I looked for documentation saying that hmm_range_register should only
be done inside a hmm_mirror_register and didn't see it. Did I miss it?
Can you add a comment? 

It is really good to fix this because it means we can rely on mmap sem
to manage mm->hmm!

If this is true then I also think we should change the signature of
the function to make this dependency relationship clear, and remove
some possible confusing edge cases.

What do you think about something like this? (unfinished)

commit 29098bd59cf481ad1915db40aefc8435dabb8b28
Author: Jason Gunthorpe <jgg@mellanox.com>
Date:   Thu May 23 09:41:19 2019 -0300

    mm/hmm: Use hmm_mirror not mm as an argument for hmm_register_range
    
    Ralf observes that hmm_register_range() can only be called by a driver
    while a mirror is registered. Make this clear in the API by passing
    in the mirror structure as a parameter.
    
    This also simplifies understanding the lifetime model for struct hmm,
    as the hmm pointer must be valid as part of a registered mirror
    so all we need in hmm_register_range() is a simple kref_get.
    
    Suggested-by: Ralph Campbell <rcampbell@nvidia.com>
    Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 8b91c90d3b88cb..87d29e085a69f7 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -503,7 +503,7 @@ static inline bool hmm_mirror_mm_is_alive(struct hmm_mirror *mirror)
  * Please see Documentation/vm/hmm.rst for how to use the range API.
  */
 int hmm_range_register(struct hmm_range *range,
-		       struct mm_struct *mm,
+		       struct hmm_mirror *mirror,
 		       unsigned long start,
 		       unsigned long end,
 		       unsigned page_shift);
@@ -539,7 +539,8 @@ static inline bool hmm_vma_range_done(struct hmm_range *range)
 }
 
 /* This is a temporary helper to avoid merge conflict between trees. */
-static inline int hmm_vma_fault(struct hmm_range *range, bool block)
+static inline int hmm_vma_fault(struct hmm_mirror *mirror,
+				struct hmm_range *range, bool block)
 {
 	long ret;
 
@@ -552,7 +553,7 @@ static inline int hmm_vma_fault(struct hmm_range *range, bool block)
 	range->default_flags = 0;
 	range->pfn_flags_mask = -1UL;
 
-	ret = hmm_range_register(range, range->vma->vm_mm,
+	ret = hmm_range_register(range, mirror,
 				 range->start, range->end,
 				 PAGE_SHIFT);
 	if (ret)
diff --git a/mm/hmm.c b/mm/hmm.c
index 824e7e160d8167..fa1b04fcfc2549 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -927,7 +927,7 @@ static void hmm_pfns_clear(struct hmm_range *range,
  * Track updates to the CPU page table see include/linux/hmm.h
  */
 int hmm_range_register(struct hmm_range *range,
-		       struct mm_struct *mm,
+		       struct hmm_mirror *mirror,
 		       unsigned long start,
 		       unsigned long end,
 		       unsigned page_shift)
@@ -935,7 +935,6 @@ int hmm_range_register(struct hmm_range *range,
 	unsigned long mask = ((1UL << page_shift) - 1UL);
 
 	range->valid = false;
-	range->hmm = NULL;
 
 	if ((start & mask) || (end & mask))
 		return -EINVAL;
@@ -946,15 +945,12 @@ int hmm_range_register(struct hmm_range *range,
 	range->start = start;
 	range->end = end;
 
-	range->hmm = hmm_get_or_create(mm);
-	if (!range->hmm)
-		return -EFAULT;
-
 	/* Check if hmm_mm_destroy() was call. */
-	if (range->hmm->mm == NULL || range->hmm->dead) {
-		hmm_put(range->hmm);
+	if (mirror->hmm->mm == NULL || mirror->hmm->dead)
 		return -EFAULT;
-	}
+
+	range->hmm = mirror->hmm;
+	kref_get(&range->hmm->kref);
 
 	/* Initialize range to track CPU page table update */
 	mutex_lock(&range->hmm->lock);


  reply	other threads:[~2019-05-23 12:51 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-06 23:29 [PATCH 0/5] mm/hmm: HMM documentation updates and code fixes rcampbell
2019-05-06 23:29 ` [PATCH 1/5] mm/hmm: Update HMM documentation rcampbell
2019-06-06 14:02   ` Jason Gunthorpe
2019-06-06 18:50     ` Ralph Campbell
2019-06-06 19:32       ` Jason Gunthorpe
2019-05-06 23:29 ` [PATCH 2/5] mm/hmm: Clean up some coding style and comments rcampbell
2019-06-06 14:16   ` Jason Gunthorpe
2019-06-06 14:27     ` Jerome Glisse
2019-06-06 15:41       ` Jason Gunthorpe
2019-06-06 15:52         ` Jerome Glisse
2019-06-06 16:55           ` Joe Perches
2019-06-06 18:54           ` Jason Gunthorpe
2019-06-06 15:57   ` Jason Gunthorpe
2019-06-07  0:44     ` Ralph Campbell
2019-05-06 23:29 ` [PATCH 3/5] mm/hmm: Use mm_get_hmm() in hmm_range_register() rcampbell
2019-05-23 12:51   ` Jason Gunthorpe [this message]
2019-05-23 18:19     ` Ralph Campbell
2019-05-23 18:46     ` John Hubbard
2019-05-06 23:29 ` [PATCH 4/5] mm/hmm: hmm_vma_fault() doesn't always call hmm_range_unregister() rcampbell
2019-05-07 13:15   ` Souptick Joarder
2019-05-07 18:12     ` Ralph Campbell
2019-05-09  4:42       ` Souptick Joarder
2019-05-12 15:07       ` Jerome Glisse
2019-05-12 15:28         ` Jerome Glisse
2019-05-13 17:23         ` Ralph Campbell
2019-06-06 14:50   ` Jason Gunthorpe
2019-06-06 19:44     ` Ralph Campbell
2019-06-06 19:54       ` Jason Gunthorpe
2019-06-06 21:08         ` Ralph Campbell
2019-05-12 15:08 ` [PATCH 0/5] mm/hmm: HMM documentation updates and code fixes Jerome Glisse
2019-05-13 17:26   ` Ralph Campbell
2019-05-13 18:10     ` Jerome Glisse
2019-06-06 14:53 ` Jason Gunthorpe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190523125108.GA14013@ziepe.ca \
    --to=jgg@ziepe.ca \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@arndb.de \
    --cc=bsingharora@gmail.com \
    --cc=dan.carpenter@oracle.com \
    --cc=dan.j.williams@intel.com \
    --cc=ira.weiny@intel.com \
    --cc=jhubbard@nvidia.com \
    --cc=jrdr.linux@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=rcampbell@nvidia.com \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH 3/5] mm/hmm: Use mm_get_hmm() in hmm_range_register()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).