Messages in this thread | | | Subject | Re: [PATCH 01/10] mm/hmm: use reference counting for HMM struct | From | John Hubbard <> | Date | Wed, 20 Feb 2019 16:42:45 -0800 |
| |
On 2/20/19 4:37 PM, Jerome Glisse wrote: > On Wed, Feb 20, 2019 at 04:32:09PM -0800, John Hubbard wrote: >> On 2/20/19 4:15 PM, Jerome Glisse wrote: >>> On Wed, Feb 20, 2019 at 04:06:50PM -0800, John Hubbard wrote: >>>> On 2/20/19 3:59 PM, Jerome Glisse wrote: >>>>> On Wed, Feb 20, 2019 at 03:47:50PM -0800, John Hubbard wrote: >>>>>> On 1/29/19 8:54 AM, jglisse@redhat.com wrote: >>>>>>> From: Jérôme Glisse <jglisse@redhat.com> >>>>>>> >>>>>>> Every time i read the code to check that the HMM structure does not >>>>>>> vanish before it should thanks to the many lock protecting its removal >>>>>>> i get a headache. Switch to reference counting instead it is much >>>>>>> easier to follow and harder to break. This also remove some code that >>>>>>> is no longer needed with refcounting. >>>>>> >>>>>> Hi Jerome, >>>>>> >>>>>> That is an excellent idea. Some review comments below: >>>>>> >>>>>> [snip] >>>>>> >>>>>>> static int hmm_invalidate_range_start(struct mmu_notifier *mn, >>>>>>> const struct mmu_notifier_range *range) >>>>>>> { >>>>>>> struct hmm_update update; >>>>>>> - struct hmm *hmm = range->mm->hmm; >>>>>>> + struct hmm *hmm = hmm_get(range->mm); >>>>>>> + int ret; >>>>>>> VM_BUG_ON(!hmm); >>>>>>> + /* Check if hmm_mm_destroy() was call. */ >>>>>>> + if (hmm->mm == NULL) >>>>>>> + return 0; >>>>>> >>>>>> Let's delete that NULL check. It can't provide true protection. If there >>>>>> is a way for that to race, we need to take another look at refcounting. >>>>> >>>>> I will do a patch to delete the NULL check so that it is easier for >>>>> Andrew. No need to respin. >>>> >>>> (Did you miss my request to make hmm_get/hmm_put symmetric, though?) >>> >>> Went over my mail i do not see anything about symmetric, what do you >>> mean ? >>> >>> Cheers, >>> Jérôme >> >> I meant the comment that I accidentally deleted, before sending the email! >> doh. Sorry about that. :) Here is the recreated comment: >> >> diff --git a/mm/hmm.c b/mm/hmm.c >> index a04e4b810610..b9f384ea15e9 100644 >> >> --- a/mm/hmm.c >> >> +++ b/mm/hmm.c >> >> @@ -50,6 +50,7 @@ >> >> static const struct mmu_notifier_ops hmm_mmu_notifier_ops; >> >> */ >> struct hmm { >> struct mm_struct *mm; >> + struct kref kref; >> spinlock_t lock; >> struct list_head ranges; >> struct list_head mirrors; >> >> @@ -57,6 +58,16 @@ >> >> struct hmm { >> >> struct rw_semaphore mirrors_sem; >> }; >> >> +static inline struct hmm *hmm_get(struct mm_struct *mm) >> +{ >> + struct hmm *hmm = READ_ONCE(mm->hmm); >> + >> + if (hmm && kref_get_unless_zero(&hmm->kref)) >> + return hmm; >> + >> + return NULL; >> +} >> + >> >> So for this, hmm_get() really ought to be symmetric with >> hmm_put(), by taking a struct hmm*. And the null check is >> not helping here, so let's just go with this smaller version: >> >> static inline struct hmm *hmm_get(struct hmm *hmm) >> { >> if (kref_get_unless_zero(&hmm->kref)) >> return hmm; >> >> return NULL; >> } >> >> ...and change the few callers accordingly. >> > > What about renaning hmm_get() to mm_get_hmm() instead ? >
For a get/put pair of functions, it would be ideal to pass the same argument type to each. It looks like we are passing around hmm*, and hmm retains a reference count on hmm->mm, so I think you have a choice of using either mm* or hmm* as the argument. I'm not sure that one is better than the other here, as the lifetimes appear to be linked pretty tightly.
Whichever one is used, I think it would be best to use it in both the _get() and _put() calls.
thanks, -- John Hubbard NVIDIA
| |