Re: [PATCH RFC V1] mm: convert rcu_read_lock() to srcu_read_lock(), thus allowing to sleep in callbacks

[Konstantin Khlebnikov <khlebnikov@openvz.org>]

sagig wrote:
> On 2/5/2012 10:27 PM, Konstantin Khlebnikov wrote:
>> sagig@mellanox.com wrote:
>>> Now that anon_vma lock and i_mmap_mutex are both sleepable mutex, it
>>> is possible to schedule inside invalidation callbacks
>>> (such as invalidate_page, invalidate_range_start/end and change_pte) .
>>> This is essential for a scheduling HW sync in RDMA drivers which
>>> apply on demand paging methods.
>>>
>>> Signed-off-by: sagi grimberg<sagig@mellanox.co.il>
>>
>> Ok, this is better, but it still does not work =)
>> Nobody synchronize with this srcu. There at least two candidates:
>> mmu_notifier_release() and mmu_notifier_unregister().
>> They call synchronize_rcu(), you must replace it with synchronize_srcu().
>>
>
> Yes, I understand - will fix.
>
>>> ---
>>>    changes from V0:
>>>    1. srcu_struct should be shared and not allocated in each callback
>>> - removed from callbacks
>>>    2. added srcu_struct under mmu_notifier_mm
>>>    3. init_srcu_struct when creating mmu_notifier_mm
>>>    4. srcu_cleanup when destroying mmu_notifier_mm
>>>
>>
>>> @@ -204,6 +208,8 @@ static int do_mmu_notifier_register(struct
>>> mmu_notifier *mn,
>>>
>>>        if (!mm_has_notifiers(mm)) {
>>>            INIT_HLIST_HEAD(&mmu_notifier_mm->list);
>>> +        if (init_srcu_struct(&mmu_notifier_mm->srcu))
>>> +            goto out_cleanup;
>>
>> move it upper, out of mm->mmap_sem lock. and fix error path.
>>
>
> Yes, I see that init_srcu_struct is using GFP_KERNEL allocations.
> But what if do_mmu_notifier_register was called from
> __mmu_notifier_register (where mmap_sem is held)? won't I end up with
> the same violation?

In this case, it is not strictly necessary, but allocation outside of locks
is usually better than under lock.

>
> Another question,
> Just to understand - I should move only the init_srcu_struct() call out
> of mmap_sem (will require checking !mm_has_notifiers(mm) twice)? or the
> entire mmu_notifier_mm initialization?

this code should do this steps:
* allocate new struct mmu_notifiler_mm with all sub-structures, like srcu.
* take locks
* try to install new mmu-notifier
* install our notifier into mmu-notifier
* release locks
* free new mmu-notifier and all sub-structures if it unused

This is very commonly used pattern, sometimes it has fast-paths, sometimes not.
Looks like in this case, there are usually only one notifier per-mm,
so newly allocated mmu-notifier unlikely to be released.

>
>>
>>>            spin_lock_init(&mmu_notifier_mm->lock);
>>>            mm->mmu_notifier_mm = mmu_notifier_mm;
>>>            mmu_notifier_mm = NULL;
>>> @@ -266,6 +272,7 @@ EXPORT_SYMBOL_GPL(__mmu_notifier_register);
>>>    void __mmu_notifier_mm_destroy(struct mm_struct *mm)
>>>    {
>>>        BUG_ON(!hlist_empty(&mm->mmu_notifier_mm->list));
>>> +    cleanup_srcu_struct(&mm->mmu_notifier_mm->srcu);
>>>        kfree(mm->mmu_notifier_mm);
>>>        mm->mmu_notifier_mm = LIST_POISON1; /* debug */
>>>    }
>>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>