| Message ID | 20190523153436.19102-10-jgg@ziepe.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hmm: Various revisions from a locking/code review | expand |
On Sat, Jun 08, 2019 at 01:08:37AM +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> > > > > No other register/unregister kernel API attempts to provide this kind of > > protection as it is inherently racy, so just drop it. > > > > Callers should provide their own protection, it appears nouveau already > > does, but just in case drop a debugging POISON. > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > mm/hmm.c | 9 ++------- > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/mm/hmm.c b/mm/hmm.c > > index 46872306f922bb..6c3b7398672c29 100644 > > +++ b/mm/hmm.c > > @@ -286,18 +286,13 @@ EXPORT_SYMBOL(hmm_mirror_register); > > */ > > void hmm_mirror_unregister(struct hmm_mirror *mirror) > > { > > - struct hmm *hmm = READ_ONCE(mirror->hmm); > > - > > - if (hmm == NULL) > > - return; > > + struct hmm *hmm = mirror->hmm; > > How about remove struct hmm *hmm and replace the code like below - > > down_write(&mirror->hmm->mirrors_sem); > list_del_init(&mirror->list); > up_write(&mirror->hmm->mirrors_sem); > hmm_put(hmm); > memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm)); > > Similar to hmm_mirror_register(). I think we get there in patch 10, right? When the series is all done the function looks like this: void hmm_mirror_unregister(struct hmm_mirror *mirror) { struct hmm *hmm = mirror->hmm; down_write(&hmm->mirrors_sem); list_del(&mirror->list); up_write(&hmm->mirrors_sem); hmm_put(hmm); memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm)); } I think this mostly matches what you wrote above, or do you think we should s/hmm/mirror->hmm/ anyhow? I think Ralph just added that :) Regards, Jason
On Thu, May 23, 2019 at 9:05 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > From: Jason Gunthorpe <jgg@mellanox.com> > > No other register/unregister kernel API attempts to provide this kind of > protection as it is inherently racy, so just drop it. > > Callers should provide their own protection, it appears nouveau already > does, but just in case drop a debugging POISON. > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > --- > mm/hmm.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/mm/hmm.c b/mm/hmm.c > index 46872306f922bb..6c3b7398672c29 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -286,18 +286,13 @@ EXPORT_SYMBOL(hmm_mirror_register); > */ > void hmm_mirror_unregister(struct hmm_mirror *mirror) > { > - struct hmm *hmm = READ_ONCE(mirror->hmm); > - > - if (hmm == NULL) > - return; > + struct hmm *hmm = mirror->hmm; How about remove struct hmm *hmm and replace the code like below - down_write(&mirror->hmm->mirrors_sem); list_del_init(&mirror->list); up_write(&mirror->hmm->mirrors_sem); hmm_put(hmm); memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm)); Similar to hmm_mirror_register(). > down_write(&hmm->mirrors_sem); > list_del_init(&mirror->list); > - /* To protect us against double unregister ... */ > - mirror->hmm = NULL; > up_write(&hmm->mirrors_sem); > - > hmm_put(hmm); > + memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm)); > } > EXPORT_SYMBOL(hmm_mirror_unregister); > > -- > 2.21.0 >
On Sat, Jun 8, 2019 at 1:07 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Sat, Jun 08, 2019 at 01:08:37AM +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> > > > > > > No other register/unregister kernel API attempts to provide this kind of > > > protection as it is inherently racy, so just drop it. > > > > > > Callers should provide their own protection, it appears nouveau already > > > does, but just in case drop a debugging POISON. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > > mm/hmm.c | 9 ++------- > > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > > > diff --git a/mm/hmm.c b/mm/hmm.c > > > index 46872306f922bb..6c3b7398672c29 100644 > > > +++ b/mm/hmm.c > > > @@ -286,18 +286,13 @@ EXPORT_SYMBOL(hmm_mirror_register); > > > */ > > > void hmm_mirror_unregister(struct hmm_mirror *mirror) > > > { > > > - struct hmm *hmm = READ_ONCE(mirror->hmm); > > > - > > > - if (hmm == NULL) > > > - return; > > > + struct hmm *hmm = mirror->hmm; > > > > How about remove struct hmm *hmm and replace the code like below - > > > > down_write(&mirror->hmm->mirrors_sem); > > list_del_init(&mirror->list); > > up_write(&mirror->hmm->mirrors_sem); > > hmm_put(hmm); > > memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm)); > > > > Similar to hmm_mirror_register(). > > I think we get there in patch 10, right? No, Patch 10 of this series has modified hmm_range_unregister(). > > When the series is all done the function looks like this: > > void hmm_mirror_unregister(struct hmm_mirror *mirror) > { > struct hmm *hmm = mirror->hmm; > > down_write(&hmm->mirrors_sem); > list_del(&mirror->list); > up_write(&hmm->mirrors_sem); > hmm_put(hmm); > memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm)); > } > > I think this mostly matches what you wrote above, or do you think we > should s/hmm/mirror->hmm/ anyhow? I think Ralph just added that :) I prefer, s/hmm/mirror->hmm and remove struct hmm *hmm :)
diff --git a/mm/hmm.c b/mm/hmm.c index 46872306f922bb..6c3b7398672c29 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -286,18 +286,13 @@ EXPORT_SYMBOL(hmm_mirror_register); */ void hmm_mirror_unregister(struct hmm_mirror *mirror) { - struct hmm *hmm = READ_ONCE(mirror->hmm); - - if (hmm == NULL) - return; + struct hmm *hmm = mirror->hmm; down_write(&hmm->mirrors_sem); list_del_init(&mirror->list); - /* To protect us against double unregister ... */ - mirror->hmm = NULL; up_write(&hmm->mirrors_sem); - hmm_put(hmm); + memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm)); } EXPORT_SYMBOL(hmm_mirror_unregister);