lkml.org 
[lkml]   [2018]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 03/14] mm/hmm: HMM should have a callback before MM is destroyed v2
From
Date
On 03/16/2018 07:36 PM, John Hubbard wrote:
> On 03/16/2018 12:14 PM, jglisse@redhat.com wrote:
>> From: Ralph Campbell <rcampbell@nvidia.com>
>>
>
> <snip>
>
>> +static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
>> +{
>> + struct hmm *hmm = mm->hmm;
>> + struct hmm_mirror *mirror;
>> + struct hmm_mirror *mirror_next;
>> +
>> + down_write(&hmm->mirrors_sem);
>> + list_for_each_entry_safe(mirror, mirror_next, &hmm->mirrors, list) {
>> + list_del_init(&mirror->list);
>> + if (mirror->ops->release)
>> + mirror->ops->release(mirror);
>> + }
>> + up_write(&hmm->mirrors_sem);
>> +}
>> +
>
> OK, as for actual code review:
>
> This part of the locking looks good. However, I think it can race against
> hmm_mirror_register(), because hmm_mirror_register() will just add a new
> mirror regardless.
>
> So:
>
> thread 1 thread 2
> -------------- -----------------
> hmm_release hmm_mirror_register
> down_write(&hmm->mirrors_sem); <blocked: waiting for sem>
> // deletes all list items
> up_write
> unblocked: adds new mirror
>
>
> ...so I think we need a way to back out of any pending hmm_mirror_register()
> calls, as part of the .release steps, right? It seems hard for the device driver,
> which could be inside of hmm_mirror_register(), to handle that. Especially considering
> that right now, hmm_mirror_register() will return success in this case--so
> there is no indication that anything is wrong.
>
> Maybe hmm_mirror_register() could return an error (and not add to the mirror list),
> in such a situation, how's that sound?
>

In other words, I think this would help (not tested yet beyond a quick compile,
but it's pretty simple):

diff --git a/mm/hmm.c b/mm/hmm.c
index 7ccca5478ea1..da39f8522dca 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -66,6 +66,7 @@ struct hmm {
struct list_head mirrors;
struct mmu_notifier mmu_notifier;
struct rw_semaphore mirrors_sem;
+ bool shutting_down;
};

/*
@@ -99,6 +100,7 @@ static struct hmm *hmm_register(struct mm_struct *mm)
INIT_LIST_HEAD(&hmm->ranges);
spin_lock_init(&hmm->lock);
hmm->mm = mm;
+ hmm->shutting_down = false;

/*
* We should only get here if hold the mmap_sem in write mode ie on
@@ -167,6 +169,7 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
struct hmm_mirror *mirror_next;

down_write(&hmm->mirrors_sem);
+ hmm->shutting_down = true;
list_for_each_entry_safe(mirror, mirror_next, &hmm->mirrors, list) {
list_del_init(&mirror->list);
if (mirror->ops->release)
@@ -227,6 +230,10 @@ int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm)
return -ENOMEM;

down_write(&mirror->hmm->mirrors_sem);
+ if (mirror->hmm->shutting_down) {
+ up_write(&mirror->hmm->mirrors_sem);
+ return -ESRCH;
+ }
list_add(&mirror->list, &mirror->hmm->mirrors);
up_write(&mirror->hmm->mirrors_sem);

thanks,
--
John Hubbard
NVIDIA

\
 
 \ /
  Last update: 2018-03-17 04:48    [W:0.092 / U:0.384 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site