Messages in this thread Patch in this message | | | Subject | Re: [PATCH 01/10] mm/hmm: use reference counting for HMM struct | From | John Hubbard <> | Date | Wed, 20 Feb 2019 16:32:09 -0800 |
| |
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.
thanks, -- John Hubbard NVIDIA
| |