Prev: [PATCH] FRV: ARCH_KMALLOC_MINALIGN was already defined
Next: [PATCHv2 0/7] Several improvements to USB gadgets
From: Doug Doan on 2 Jun 2010 19:40
On 06/01/2010 11:16 PM, Andrew Morton wrote:
> On Thu, 27 May 2010 13:43:00 -0700 Doug Doan<dougd(a)cray.com> wrote:
>
>>
>> When a copy-on-write occurs, we take one of two paths in handle_mm_fault:
>> through handle_pte_fault for normal pages, or through hugetlb_fault for huge pages.
>>
>> In the normal page case, we eventually get to do_wp_page and call mmu notifiers
>> via ptep_clear_flush_notify. There is no callout to the mmmu notifiers in the
>> huge page case. This patch fixes that.
>>
>> Signed-off-by: Doug Doan<dougd(a)cray.com>
>> ---
>>
>> [patch text/plain (802B)]
>> --- mm/hugetlb.c.orig 2010-05-27 13:07:58.569546314 -0700
>> +++ mm/hugetlb.c 2010-05-26 14:41:06.449296524 -0700
>
> (In patch -p1 form, please. So a/mm/hugetlb.c)
>
>> @@ -2345,11 +2345,17 @@ retry_avoidcopy:
>> ptep = huge_pte_offset(mm, address& huge_page_mask(h));
>> if (likely(pte_same(huge_ptep_get(ptep), pte))) {
>> /* Break COW */
>> + mmu_notifier_invalidate_range_start(mm,
>> + address& huge_page_mask(h),
>> + (address& huge_page_mask(h)) + huge_page_size(h));
>> huge_ptep_clear_flush(vma, address, ptep);
>> set_huge_pte_at(mm, address, ptep,
>> make_huge_pte(vma, new_page, 1));
>> /* Make the old page be freed below */
>> new_page = old_page;
>> + mmu_notifier_invalidate_range_end(mm,
>> + address& huge_page_mask(h),
>> + (address& huge_page_mask(h)) + huge_page_size(h));
>> }
>> page_cache_release(new_page);
>> page_cache_release(old_page);
>
> This causes mmu_notifier_invalidate_range_start() to be called under
> page_table_lock. The immediately preceding code seems to take some
> care to avoid doing that. I took a quick look at other callsites and
> cannot immediately see other cases where
> mmu_notifier_invalidate_range_start/end() are called under that lock.
>
> This may not introduce bugs with current notifier implementations (I
> didn't check), but it does lessen flexibility?

In the normal page case, handle_pte_fault calls do_wp_page inside a spinlock on
ptl = pte_lockptr(mm, pmd), which uses mm->page_table_lock if USE_SPLIT_PTLOCKS
is not defined.

I don't understand what you mean by lessen flexibilty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Doug Doan on 3 Jun 2010 13:40
On 06/02/2010 04:33 PM, Andrew Morton wrote:
> On Wed, 2 Jun 2010 16:13:42 -0700
> Doug Doan<dougd(a)cray.com> wrote:
>
>> On 06/01/2010 11:16 PM, Andrew Morton wrote:
>>> On Thu, 27 May 2010 13:43:00 -0700 Doug Doan<dougd(a)cray.com> wrote:
>>>
>>>>
>>>> When a copy-on-write occurs, we take one of two paths in handle_mm_fault:
>>>> through handle_pte_fault for normal pages, or through hugetlb_fault for huge pages.
>>>>
>>>> In the normal page case, we eventually get to do_wp_page and call mmu notifiers
>>>> via ptep_clear_flush_notify. There is no callout to the mmmu notifiers in the
>>>> huge page case. This patch fixes that.
>>>>
>>>> Signed-off-by: Doug Doan<dougd(a)cray.com>
>>>> ---
>>>>
>>>> [patch text/plain (802B)]
>>>> --- mm/hugetlb.c.orig 2010-05-27 13:07:58.569546314 -0700
>>>> +++ mm/hugetlb.c 2010-05-26 14:41:06.449296524 -0700
>>>
>>> (In patch -p1 form, please. So a/mm/hugetlb.c)
>>>
>>>> @@ -2345,11 +2345,17 @@ retry_avoidcopy:
>>>> ptep = huge_pte_offset(mm, address& huge_page_mask(h));
>>>> if (likely(pte_same(huge_ptep_get(ptep), pte))) {
>>>> /* Break COW */
>>>> + mmu_notifier_invalidate_range_start(mm,
>>>> + address& huge_page_mask(h),
>>>> + (address& huge_page_mask(h)) + huge_page_size(h));
>>>> huge_ptep_clear_flush(vma, address, ptep);
>>>> set_huge_pte_at(mm, address, ptep,
>>>> make_huge_pte(vma, new_page, 1));
>>>> /* Make the old page be freed below */
>>>> new_page = old_page;
>>>> + mmu_notifier_invalidate_range_end(mm,
>>>> + address& huge_page_mask(h),
>>>> + (address& huge_page_mask(h)) + huge_page_size(h));
>>>> }
>>>> page_cache_release(new_page);
>>>> page_cache_release(old_page);
>>>
>>> This causes mmu_notifier_invalidate_range_start() to be called under
>>> page_table_lock. The immediately preceding code seems to take some
>>> care to avoid doing that. I took a quick look at other callsites and
>>> cannot immediately see other cases where
>>> mmu_notifier_invalidate_range_start/end() are called under that lock.
>>>
>>> This may not introduce bugs with current notifier implementations (I
>>> didn't check), but it does lessen flexibility?
>>
>> In the normal page case, handle_pte_fault calls do_wp_page inside a spinlock on
>> ptl = pte_lockptr(mm, pmd), which uses mm->page_table_lock if USE_SPLIT_PTLOCKS
>> is not defined.
>>
>> I don't understand what you mean by lessen flexibilty.
>
> Well, specifically it means that
> mmu_notifier_invalidate_range_start/end() implemetnations can no longer
> take page_table_lock or any lock which nests outside page_table_lock.
> That lessens flexibility.
>
> As the other mmu_notifier_invalidate_range_start/end() callsite in this
> function carefully nested those calls outside page_table_lock, perhaps
> that was thought to be a significant thing.

Here's my rationale: for the normal page case, the invalidation call is done
inside a page_table_lock, so the same should also be done in the huge page case.
Does it really make sense to call invalidation on one hugepage and have another
call invalidate the same hugepage while the first call is still not finished?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 3 Jun 2010 14:20
On Thu, 3 Jun 2010 10:36:00 -0700
Doug Doan <dougd(a)cray.com> wrote:

> > Well, specifically it means that
> > mmu_notifier_invalidate_range_start/end() implemetnations can no longer
> > take page_table_lock or any lock which nests outside page_table_lock.
> > That lessens flexibility.
> >
> > As the other mmu_notifier_invalidate_range_start/end() callsite in this
> > function carefully nested those calls outside page_table_lock, perhaps
> > that was thought to be a significant thing.
>
> Here's my rationale: for the normal page case, the invalidation call is done
> inside a page_table_lock,

It is? Where does that happen?

> so the same should also be done in the huge page case.
> Does it really make sense to call invalidation on one hugepage and have another
> call invalidate the same hugepage while the first call is still not finished?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Doug Doan on 3 Jun 2010 14:40
On 06/03/2010 11:11 AM, Andrew Morton wrote:
> On Thu, 3 Jun 2010 10:36:00 -0700
> Doug Doan<dougd(a)cray.com> wrote:
>
>>> Well, specifically it means that
>>> mmu_notifier_invalidate_range_start/end() implemetnations can no longer
>>> take page_table_lock or any lock which nests outside page_table_lock.
>>> That lessens flexibility.
>>>
>>> As the other mmu_notifier_invalidate_range_start/end() callsite in this
>>> function carefully nested those calls outside page_table_lock, perhaps
>>> that was thought to be a significant thing.
>>
>> Here's my rationale: for the normal page case, the invalidation call is done
>> inside a page_table_lock,
>
> It is? Where does that happen?

handle_pte_fault() acquires the lock before calling do_wp_page():

ptl = pte_lockptr(mm, pmd);
spin_lock(ptl);
if (unlikely(!pte_same(*pte, entry)))
goto unlock;
if (flags & FAULT_FLAG_WRITE) {
if (!pte_write(entry))
return do_wp_page(mm, vma, address,
pte, pmd, ptl, entry);
entry = pte_mkdirty(entry);
}

do_wp_page() calls set_pte_at_notify(), which either calls
mmu_notifier_change_pte() or mmu_notifier_invalidate_page().

>
>> so the same should also be done in the huge page case.
>> Does it really make sense to call invalidation on one hugepage and have another
>> call invalidate the same hugepage while the first call is still not finished?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  | 
Pages: 1 2
Prev: [PATCH] FRV: ARCH_KMALLOC_MINALIGN was already defined
Next: [PATCHv2 0/7] Several improvements to USB gadgets