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@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.
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
