Re: [PATCH 01/10] mm/hmm: use reference counting for HMM struct

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx wrote:
From: Jérôme Glisse <jglisse@xxxxxxxxxx>

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





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux