| Message ID | 20190606184438.31646-10-jgg@ziepe.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various revisions from a locking/code review | expand |
On Thu, Jun 06, 2019 at 08:37:42PM -0700, John Hubbard wrote: > On 6/6/19 11:44 AM, Jason Gunthorpe wrote: > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON > > and poison bytes to detect this condition. > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > Reviewed-by: Jérôme Glisse <jglisse@redhat.com> > > v2 > > - Keep range start/end valid after unregistration (Jerome) > > mm/hmm.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/mm/hmm.c b/mm/hmm.c > > index 6802de7080d172..c2fecb3ecb11e1 100644 > > +++ b/mm/hmm.c > > @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range) > > struct hmm *hmm = range->hmm; > > > > /* Sanity check this really should not happen. */ > > That comment can also be deleted, as it has the same meaning as > the WARN_ON() that you just added. > > > - if (hmm == NULL || range->end <= range->start) > > + if (WARN_ON(range->end <= range->start)) > > return; > > > > mutex_lock(&hmm->lock); > > @@ -948,7 +948,10 @@ void hmm_range_unregister(struct hmm_range *range) > > range->valid = false; > > mmput(hmm->mm); > > hmm_put(hmm); > > - range->hmm = NULL; > > + > > + /* The range is now invalid, leave it poisoned. */ > > To be precise, we are poisoning the range's back pointer to it's > owning hmm instance. Maybe this is clearer: > > /* > * The range is now invalid, so poison it's hmm pointer. > * Leave other range-> fields in place, for the caller's use. > */ > > ...or something like that? > > > + range->valid = false; > > + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm)); > > } > > EXPORT_SYMBOL(hmm_range_unregister); > > > > > > The above are very minor documentation points, so: > > Reviewed-by: John Hubbard <jhubbard@nvidia.com> done thanks Jason
On 6/6/19 11:44 AM, Jason Gunthorpe wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON > and poison bytes to detect this condition. > > 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 > - Keep range start/end valid after unregistration (Jerome) > --- > mm/hmm.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/mm/hmm.c b/mm/hmm.c > index 6802de7080d172..c2fecb3ecb11e1 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range) > struct hmm *hmm = range->hmm; > > /* Sanity check this really should not happen. */ > - if (hmm == NULL || range->end <= range->start) > + if (WARN_ON(range->end <= range->start)) > return; WARN_ON() is definitely better than silent return but I wonder how useful it is since the caller shouldn't be modifying the hmm_range once it is registered. Other fields could be changed too... > mutex_lock(&hmm->lock); > @@ -948,7 +948,10 @@ void hmm_range_unregister(struct hmm_range *range) > range->valid = false; > mmput(hmm->mm); > hmm_put(hmm); > - range->hmm = NULL; > + > + /* The range is now invalid, leave it poisoned. */ > + range->valid = false; > + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm)); > } > EXPORT_SYMBOL(hmm_range_unregister); > >
On Fri, Jun 07, 2019 at 01:46:30PM -0700, Ralph Campbell wrote: > > On 6/6/19 11:44 AM, Jason Gunthorpe wrote: > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON > > and poison bytes to detect this condition. > > > > 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 > > - Keep range start/end valid after unregistration (Jerome) > > mm/hmm.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/mm/hmm.c b/mm/hmm.c > > index 6802de7080d172..c2fecb3ecb11e1 100644 > > +++ b/mm/hmm.c > > @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range) > > struct hmm *hmm = range->hmm; > > /* Sanity check this really should not happen. */ > > - if (hmm == NULL || range->end <= range->start) > > + if (WARN_ON(range->end <= range->start)) > > return; > > WARN_ON() is definitely better than silent return but I wonder how > useful it is since the caller shouldn't be modifying the hmm_range > once it is registered. Other fields could be changed too... I deleted the warn on (see the other thread), but I'm confused by your "shouldn't be modified" statement. The only thing that needs to be set and remain unchanged for register is the virtual start/end address. Everything else should be done once it is clear to proceed based on the collision-retry locking scheme this uses. Basically the range register only setups a 'detector' for colliding invalidations. The other stuff in the struct is just random temporary storage for the API. AFAICS at least.. Jason
On Thu, Jun 06, 2019 at 03:44:36PM -0300, Jason Gunthorpe wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON > and poison bytes to detect this condition. > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > Reviewed-by: Jérôme Glisse <jglisse@redhat.com> > --- > v2 > - Keep range start/end valid after unregistration (Jerome) > --- > mm/hmm.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/mm/hmm.c b/mm/hmm.c > index 6802de7080d172..c2fecb3ecb11e1 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range) > struct hmm *hmm = range->hmm; > > /* Sanity check this really should not happen. */ > - if (hmm == NULL || range->end <= range->start) > + if (WARN_ON(range->end <= range->start)) > return; > > mutex_lock(&hmm->lock); > @@ -948,7 +948,10 @@ void hmm_range_unregister(struct hmm_range *range) > range->valid = false; > mmput(hmm->mm); > hmm_put(hmm); > - range->hmm = NULL; > + > + /* The range is now invalid, leave it poisoned. */ > + range->valid = false; No need to set valid false again as you just did this 5 lines above. Reviewed-by: Ira Weiny <ira.weiny@intel.com> > + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm)); > } > EXPORT_SYMBOL(hmm_range_unregister); > > -- > 2.21.0 >
diff --git a/mm/hmm.c b/mm/hmm.c index 6802de7080d172..c2fecb3ecb11e1 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range) struct hmm *hmm = range->hmm; /* Sanity check this really should not happen. */ - if (hmm == NULL || range->end <= range->start) + if (WARN_ON(range->end <= range->start)) return; mutex_lock(&hmm->lock); @@ -948,7 +948,10 @@ void hmm_range_unregister(struct hmm_range *range) range->valid = false; mmput(hmm->mm); hmm_put(hmm); - range->hmm = NULL; + + /* The range is now invalid, leave it poisoned. */ + range->valid = false; + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm)); } EXPORT_SYMBOL(hmm_range_unregister);