| Message ID | 20190523153436.19102-9-jgg@ziepe.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hmm: Various revisions from a locking/code review | expand |
On Thu, May 23, 2019 at 9:05 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > From: Jason Gunthorpe <jgg@mellanox.com> > > So we can check locking at runtime. > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > --- > mm/hmm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/hmm.c b/mm/hmm.c > index 2695925c0c5927..46872306f922bb 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -256,11 +256,11 @@ static const struct mmu_notifier_ops hmm_mmu_notifier_ops = { > * > * To start mirroring a process address space, the device driver must register > * an HMM mirror struct. > - * > - * THE mm->mmap_sem MUST BE HELD IN WRITE MODE ! > */ > int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm) > { > + lockdep_assert_held_exclusive(mm->mmap_sem); > + Gentle query, does the same required in hmm_mirror_unregister() ? > /* Sanity check */ > if (!mm || !mirror || !mirror->ops) > return -EINVAL; > -- > 2.21.0 >
On Sat, Jun 08, 2019 at 01:03:48AM +0530, Souptick Joarder wrote: > On Thu, May 23, 2019 at 9:05 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > So we can check locking at runtime. > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > mm/hmm.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/mm/hmm.c b/mm/hmm.c > > index 2695925c0c5927..46872306f922bb 100644 > > +++ b/mm/hmm.c > > @@ -256,11 +256,11 @@ static const struct mmu_notifier_ops hmm_mmu_notifier_ops = { > > * > > * To start mirroring a process address space, the device driver must register > > * an HMM mirror struct. > > - * > > - * THE mm->mmap_sem MUST BE HELD IN WRITE MODE ! > > */ > > int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm) > > { > > + lockdep_assert_held_exclusive(mm->mmap_sem); > > + > > Gentle query, does the same required in hmm_mirror_unregister() ? No.. The unregistration path does its actual work in the srcu callback, which is in a different context than this function. So any locking held by the caller of unregister will not apply. The hmm_range_free SRCU callback obtains the write side of mmap_sem to protect the same data that the write side above in register is touching, mostly &mm->hmm. Jason
On Sat, Jun 8, 2019 at 1:09 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Sat, Jun 08, 2019 at 01:03:48AM +0530, Souptick Joarder wrote: > > On Thu, May 23, 2019 at 9:05 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > > > So we can check locking at runtime. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > > mm/hmm.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/mm/hmm.c b/mm/hmm.c > > > index 2695925c0c5927..46872306f922bb 100644 > > > +++ b/mm/hmm.c > > > @@ -256,11 +256,11 @@ static const struct mmu_notifier_ops hmm_mmu_notifier_ops = { > > > * > > > * To start mirroring a process address space, the device driver must register > > > * an HMM mirror struct. > > > - * > > > - * THE mm->mmap_sem MUST BE HELD IN WRITE MODE ! > > > */ > > > int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm) > > > { > > > + lockdep_assert_held_exclusive(mm->mmap_sem); > > > + > > > > Gentle query, does the same required in hmm_mirror_unregister() ? > > No.. The unregistration path does its actual work in the srcu > callback, which is in a different context than this function. So any > locking held by the caller of unregister will not apply. > > The hmm_range_free SRCU callback obtains the write side of mmap_sem to > protect the same data that the write side above in register is > touching, mostly &mm->hmm. Looking into https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/tree/?h=hmm, unable trace hmm_range_free(). Am I looking into correct tree ?
On Sat, Jun 08, 2019 at 02:32:23AM +0530, Souptick Joarder wrote: > On Sat, Jun 8, 2019 at 1:09 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Sat, Jun 08, 2019 at 01:03:48AM +0530, Souptick Joarder wrote: > > > On Thu, May 23, 2019 at 9:05 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > > > > > So we can check locking at runtime. > > > > > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > > > mm/hmm.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/mm/hmm.c b/mm/hmm.c > > > > index 2695925c0c5927..46872306f922bb 100644 > > > > +++ b/mm/hmm.c > > > > @@ -256,11 +256,11 @@ static const struct mmu_notifier_ops hmm_mmu_notifier_ops = { > > > > * > > > > * To start mirroring a process address space, the device driver must register > > > > * an HMM mirror struct. > > > > - * > > > > - * THE mm->mmap_sem MUST BE HELD IN WRITE MODE ! > > > > */ > > > > int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm) > > > > { > > > > + lockdep_assert_held_exclusive(mm->mmap_sem); > > > > + > > > > > > Gentle query, does the same required in hmm_mirror_unregister() ? > > > > No.. The unregistration path does its actual work in the srcu > > callback, which is in a different context than this function. So any > > locking held by the caller of unregister will not apply. > > > > The hmm_range_free SRCU callback obtains the write side of mmap_sem to > > protect the same data that the write side above in register is > > touching, mostly &mm->hmm. > > Looking into https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/tree/?h=hmm, > unable trace hmm_range_free(). Am I looking into correct tree ? The cover letter for the v2 posting has a note about the git tree for this series: https://github.com/jgunthorpe/linux/tree/hmm The above rdma.git is only for already applied patches on their way to Linus. This series is still in review. Thanks, Jason
diff --git a/mm/hmm.c b/mm/hmm.c index 2695925c0c5927..46872306f922bb 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -256,11 +256,11 @@ static const struct mmu_notifier_ops hmm_mmu_notifier_ops = { * * To start mirroring a process address space, the device driver must register * an HMM mirror struct. - * - * THE mm->mmap_sem MUST BE HELD IN WRITE MODE ! */ int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm) { + lockdep_assert_held_exclusive(mm->mmap_sem); + /* Sanity check */ if (!mm || !mirror || !mirror->ops) return -EINVAL;