| Message ID | 20190606184438.31646-4-jgg@ziepe.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Various revisions from a locking/code review | expand |
On 6/6/19 11:44 AM, Jason Gunthorpe wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > So long a a struct hmm pointer exists, so should the struct mm it is > linked too. Hold the mmgrab() as soon as a hmm is created, and mmdrop() it > once the hmm refcount goes to zero. > > Since mmdrop() (ie a 0 kref on struct mm) is now impossible with a !NULL > mm->hmm delete the hmm_hmm_destroy(). > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > Reviewed-by: Jérôme Glisse <jglisse@redhat.com> > --- > v2: > - Fix error unwind paths in hmm_get_or_create (Jerome/Jason) > --- > include/linux/hmm.h | 3 --- > kernel/fork.c | 1 - > mm/hmm.c | 22 ++++------------------ > 3 files changed, 4 insertions(+), 22 deletions(-) > > diff --git a/include/linux/hmm.h b/include/linux/hmm.h > index 2d519797cb134a..4ee3acabe5ed22 100644 > --- a/include/linux/hmm.h > +++ b/include/linux/hmm.h > @@ -586,14 +586,11 @@ static inline int hmm_vma_fault(struct hmm_mirror *mirror, > } > > /* Below are for HMM internal use only! Not to be used by device driver! */ > -void hmm_mm_destroy(struct mm_struct *mm); > - > static inline void hmm_mm_init(struct mm_struct *mm) > { > mm->hmm = NULL; > } > #else /* IS_ENABLED(CONFIG_HMM_MIRROR) */ > -static inline void hmm_mm_destroy(struct mm_struct *mm) {} > static inline void hmm_mm_init(struct mm_struct *mm) {} > #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */ > > diff --git a/kernel/fork.c b/kernel/fork.c > index b2b87d450b80b5..588c768ae72451 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm) > WARN_ON_ONCE(mm == current->active_mm); > mm_free_pgd(mm); > destroy_context(mm); > - hmm_mm_destroy(mm); This is particularly welcome, not to have an "HMM is special" case in such a core part of process/mm code. > mmu_notifier_mm_destroy(mm); > check_mm(mm); > put_user_ns(mm->user_ns); > diff --git a/mm/hmm.c b/mm/hmm.c > index 8796447299023c..cc7c26fda3300e 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -29,6 +29,7 @@ > #include <linux/swapops.h> > #include <linux/hugetlb.h> > #include <linux/memremap.h> > +#include <linux/sched/mm.h> > #include <linux/jump_label.h> > #include <linux/dma-mapping.h> > #include <linux/mmu_notifier.h> > @@ -82,6 +83,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm) > hmm->notifiers = 0; > hmm->dead = false; > hmm->mm = mm; > + mmgrab(hmm->mm); > > spin_lock(&mm->page_table_lock); > if (!mm->hmm) > @@ -109,6 +111,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm) > mm->hmm = NULL; > spin_unlock(&mm->page_table_lock); > error: > + mmdrop(hmm->mm); > kfree(hmm); > return NULL; > } > @@ -130,6 +133,7 @@ static void hmm_free(struct kref *kref) > mm->hmm = NULL; > spin_unlock(&mm->page_table_lock); > > + mmdrop(hmm->mm); > mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu); > } > > @@ -138,24 +142,6 @@ static inline void hmm_put(struct hmm *hmm) > kref_put(&hmm->kref, hmm_free); > } > > -void hmm_mm_destroy(struct mm_struct *mm) > -{ > - struct hmm *hmm; > - > - spin_lock(&mm->page_table_lock); > - hmm = mm_get_hmm(mm); > - mm->hmm = NULL; > - if (hmm) { > - hmm->mm = NULL; > - hmm->dead = true; > - spin_unlock(&mm->page_table_lock); > - hmm_put(hmm); > - return; > - } > - > - spin_unlock(&mm->page_table_lock); > -} > - > static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm) > { > struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier); > Failed to find any problems with this. :) Reviewed-by: John Hubbard <jhubbard@nvidia.com> thanks,
On Thu, Jun 06, 2019 at 07:44:58PM -0700, John Hubbard wrote: > On 6/6/19 11:44 AM, Jason Gunthorpe wrote: > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > So long a a struct hmm pointer exists, so should the struct mm it is > > linked too. Hold the mmgrab() as soon as a hmm is created, and mmdrop() it > > once the hmm refcount goes to zero. > > > > Since mmdrop() (ie a 0 kref on struct mm) is now impossible with a !NULL > > mm->hmm delete the hmm_hmm_destroy(). > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > Reviewed-by: Jérôme Glisse <jglisse@redhat.com> > > v2: > > - Fix error unwind paths in hmm_get_or_create (Jerome/Jason) > > include/linux/hmm.h | 3 --- > > kernel/fork.c | 1 - > > mm/hmm.c | 22 ++++------------------ > > 3 files changed, 4 insertions(+), 22 deletions(-) > > > > diff --git a/include/linux/hmm.h b/include/linux/hmm.h > > index 2d519797cb134a..4ee3acabe5ed22 100644 > > +++ b/include/linux/hmm.h > > @@ -586,14 +586,11 @@ static inline int hmm_vma_fault(struct hmm_mirror *mirror, > > } > > > > /* Below are for HMM internal use only! Not to be used by device driver! */ > > -void hmm_mm_destroy(struct mm_struct *mm); > > - > > static inline void hmm_mm_init(struct mm_struct *mm) > > { > > mm->hmm = NULL; > > } > > #else /* IS_ENABLED(CONFIG_HMM_MIRROR) */ > > -static inline void hmm_mm_destroy(struct mm_struct *mm) {} > > static inline void hmm_mm_init(struct mm_struct *mm) {} > > #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */ > > > > diff --git a/kernel/fork.c b/kernel/fork.c > > index b2b87d450b80b5..588c768ae72451 100644 > > +++ b/kernel/fork.c > > @@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm) > > WARN_ON_ONCE(mm == current->active_mm); > > mm_free_pgd(mm); > > destroy_context(mm); > > - hmm_mm_destroy(mm); > > > This is particularly welcome, not to have an "HMM is special" case > in such a core part of process/mm code. I would very much like to propose something like 'per-net' for struct mm, as rdma also need to add some data to each mm to make it's use of mmu notifiers work (for basically this same reason as HMM) Thanks, Jason
On 6/6/19 11:44 AM, Jason Gunthorpe wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > So long a a struct hmm pointer exists, so should the struct mm it is s/a a/as a/ > linked too. Hold the mmgrab() as soon as a hmm is created, and mmdrop() it > once the hmm refcount goes to zero. > > Since mmdrop() (ie a 0 kref on struct mm) is now impossible with a !NULL > mm->hmm delete the hmm_hmm_destroy(). > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > Reviewed-by: Jérôme Glisse <jglisse@redhat.com> Reviewed-by: Ralph Campbell <rcampbell@nvidia.com> > --- > v2: > - Fix error unwind paths in hmm_get_or_create (Jerome/Jason) > --- > include/linux/hmm.h | 3 --- > kernel/fork.c | 1 - > mm/hmm.c | 22 ++++------------------ > 3 files changed, 4 insertions(+), 22 deletions(-) > > diff --git a/include/linux/hmm.h b/include/linux/hmm.h > index 2d519797cb134a..4ee3acabe5ed22 100644 > --- a/include/linux/hmm.h > +++ b/include/linux/hmm.h > @@ -586,14 +586,11 @@ static inline int hmm_vma_fault(struct hmm_mirror *mirror, > } > > /* Below are for HMM internal use only! Not to be used by device driver! */ > -void hmm_mm_destroy(struct mm_struct *mm); > - > static inline void hmm_mm_init(struct mm_struct *mm) > { > mm->hmm = NULL; > } > #else /* IS_ENABLED(CONFIG_HMM_MIRROR) */ > -static inline void hmm_mm_destroy(struct mm_struct *mm) {} > static inline void hmm_mm_init(struct mm_struct *mm) {} > #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */ > > diff --git a/kernel/fork.c b/kernel/fork.c > index b2b87d450b80b5..588c768ae72451 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm) > WARN_ON_ONCE(mm == current->active_mm); > mm_free_pgd(mm); > destroy_context(mm); > - hmm_mm_destroy(mm); > mmu_notifier_mm_destroy(mm); > check_mm(mm); > put_user_ns(mm->user_ns); > diff --git a/mm/hmm.c b/mm/hmm.c > index 8796447299023c..cc7c26fda3300e 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -29,6 +29,7 @@ > #include <linux/swapops.h> > #include <linux/hugetlb.h> > #include <linux/memremap.h> > +#include <linux/sched/mm.h> > #include <linux/jump_label.h> > #include <linux/dma-mapping.h> > #include <linux/mmu_notifier.h> > @@ -82,6 +83,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm) > hmm->notifiers = 0; > hmm->dead = false; > hmm->mm = mm; > + mmgrab(hmm->mm); > > spin_lock(&mm->page_table_lock); > if (!mm->hmm) > @@ -109,6 +111,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm) > mm->hmm = NULL; > spin_unlock(&mm->page_table_lock); > error: > + mmdrop(hmm->mm); > kfree(hmm); > return NULL; > } > @@ -130,6 +133,7 @@ static void hmm_free(struct kref *kref) > mm->hmm = NULL; > spin_unlock(&mm->page_table_lock); > > + mmdrop(hmm->mm); > mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu); > } > > @@ -138,24 +142,6 @@ static inline void hmm_put(struct hmm *hmm) > kref_put(&hmm->kref, hmm_free); > } > > -void hmm_mm_destroy(struct mm_struct *mm) > -{ > - struct hmm *hmm; > - > - spin_lock(&mm->page_table_lock); > - hmm = mm_get_hmm(mm); > - mm->hmm = NULL; > - if (hmm) { > - hmm->mm = NULL; > - hmm->dead = true; > - spin_unlock(&mm->page_table_lock); > - hmm_put(hmm); > - return; > - } > - > - spin_unlock(&mm->page_table_lock); > -} > - > static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm) > { > struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier); >
On Fri, Jun 07, 2019 at 11:41:20AM -0700, Ralph Campbell wrote: > > On 6/6/19 11:44 AM, Jason Gunthorpe wrote: > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > So long a a struct hmm pointer exists, so should the struct mm it is > > s/a a/as a/ Got it, thanks Jason
On Thu, Jun 06, 2019 at 03:44:30PM -0300, Jason Gunthorpe wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > So long a a struct hmm pointer exists, so should the struct mm it is > linked too. Hold the mmgrab() as soon as a hmm is created, and mmdrop() it > once the hmm refcount goes to zero. > > Since mmdrop() (ie a 0 kref on struct mm) is now impossible with a !NULL > mm->hmm delete the hmm_hmm_destroy(). > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > Reviewed-by: Jérôme Glisse <jglisse@redhat.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > v2: > - Fix error unwind paths in hmm_get_or_create (Jerome/Jason) > --- > include/linux/hmm.h | 3 --- > kernel/fork.c | 1 - > mm/hmm.c | 22 ++++------------------ > 3 files changed, 4 insertions(+), 22 deletions(-) > > diff --git a/include/linux/hmm.h b/include/linux/hmm.h > index 2d519797cb134a..4ee3acabe5ed22 100644 > --- a/include/linux/hmm.h > +++ b/include/linux/hmm.h > @@ -586,14 +586,11 @@ static inline int hmm_vma_fault(struct hmm_mirror *mirror, > } > > /* Below are for HMM internal use only! Not to be used by device driver! */ > -void hmm_mm_destroy(struct mm_struct *mm); > - > static inline void hmm_mm_init(struct mm_struct *mm) > { > mm->hmm = NULL; > } > #else /* IS_ENABLED(CONFIG_HMM_MIRROR) */ > -static inline void hmm_mm_destroy(struct mm_struct *mm) {} > static inline void hmm_mm_init(struct mm_struct *mm) {} > #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */ > > diff --git a/kernel/fork.c b/kernel/fork.c > index b2b87d450b80b5..588c768ae72451 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm) > WARN_ON_ONCE(mm == current->active_mm); > mm_free_pgd(mm); > destroy_context(mm); > - hmm_mm_destroy(mm); > mmu_notifier_mm_destroy(mm); > check_mm(mm); > put_user_ns(mm->user_ns); > diff --git a/mm/hmm.c b/mm/hmm.c > index 8796447299023c..cc7c26fda3300e 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -29,6 +29,7 @@ > #include <linux/swapops.h> > #include <linux/hugetlb.h> > #include <linux/memremap.h> > +#include <linux/sched/mm.h> > #include <linux/jump_label.h> > #include <linux/dma-mapping.h> > #include <linux/mmu_notifier.h> > @@ -82,6 +83,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm) > hmm->notifiers = 0; > hmm->dead = false; > hmm->mm = mm; > + mmgrab(hmm->mm); > > spin_lock(&mm->page_table_lock); > if (!mm->hmm) > @@ -109,6 +111,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm) > mm->hmm = NULL; > spin_unlock(&mm->page_table_lock); > error: > + mmdrop(hmm->mm); > kfree(hmm); > return NULL; > } > @@ -130,6 +133,7 @@ static void hmm_free(struct kref *kref) > mm->hmm = NULL; > spin_unlock(&mm->page_table_lock); > > + mmdrop(hmm->mm); > mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu); > } > > @@ -138,24 +142,6 @@ static inline void hmm_put(struct hmm *hmm) > kref_put(&hmm->kref, hmm_free); > } > > -void hmm_mm_destroy(struct mm_struct *mm) > -{ > - struct hmm *hmm; > - > - spin_lock(&mm->page_table_lock); > - hmm = mm_get_hmm(mm); > - mm->hmm = NULL; > - if (hmm) { > - hmm->mm = NULL; > - hmm->dead = true; > - spin_unlock(&mm->page_table_lock); > - hmm_put(hmm); > - return; > - } > - > - spin_unlock(&mm->page_table_lock); > -} > - > static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm) > { > struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier); > -- > 2.21.0 >
diff --git a/include/linux/hmm.h b/include/linux/hmm.h index 2d519797cb134a..4ee3acabe5ed22 100644 --- a/include/linux/hmm.h +++ b/include/linux/hmm.h @@ -586,14 +586,11 @@ static inline int hmm_vma_fault(struct hmm_mirror *mirror, } /* Below are for HMM internal use only! Not to be used by device driver! */ -void hmm_mm_destroy(struct mm_struct *mm); - static inline void hmm_mm_init(struct mm_struct *mm) { mm->hmm = NULL; } #else /* IS_ENABLED(CONFIG_HMM_MIRROR) */ -static inline void hmm_mm_destroy(struct mm_struct *mm) {} static inline void hmm_mm_init(struct mm_struct *mm) {} #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */ diff --git a/kernel/fork.c b/kernel/fork.c index b2b87d450b80b5..588c768ae72451 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm) WARN_ON_ONCE(mm == current->active_mm); mm_free_pgd(mm); destroy_context(mm); - hmm_mm_destroy(mm); mmu_notifier_mm_destroy(mm); check_mm(mm); put_user_ns(mm->user_ns); diff --git a/mm/hmm.c b/mm/hmm.c index 8796447299023c..cc7c26fda3300e 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -29,6 +29,7 @@ #include <linux/swapops.h> #include <linux/hugetlb.h> #include <linux/memremap.h> +#include <linux/sched/mm.h> #include <linux/jump_label.h> #include <linux/dma-mapping.h> #include <linux/mmu_notifier.h> @@ -82,6 +83,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm) hmm->notifiers = 0; hmm->dead = false; hmm->mm = mm; + mmgrab(hmm->mm); spin_lock(&mm->page_table_lock); if (!mm->hmm) @@ -109,6 +111,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm) mm->hmm = NULL; spin_unlock(&mm->page_table_lock); error: + mmdrop(hmm->mm); kfree(hmm); return NULL; } @@ -130,6 +133,7 @@ static void hmm_free(struct kref *kref) mm->hmm = NULL; spin_unlock(&mm->page_table_lock); + mmdrop(hmm->mm); mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu); } @@ -138,24 +142,6 @@ static inline void hmm_put(struct hmm *hmm) kref_put(&hmm->kref, hmm_free); } -void hmm_mm_destroy(struct mm_struct *mm) -{ - struct hmm *hmm; - - spin_lock(&mm->page_table_lock); - hmm = mm_get_hmm(mm); - mm->hmm = NULL; - if (hmm) { - hmm->mm = NULL; - hmm->dead = true; - spin_unlock(&mm->page_table_lock); - hmm_put(hmm); - return; - } - - spin_unlock(&mm->page_table_lock); -} - static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm) { struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);