[email protected] wrote:
> From: Paolo 'Blaisorblade' Giarrusso <[email protected]>
>
> pte_present(pte) implies that pte_pfn(pte) is valid. Normally even with a
> _PAGE_PROTNONE pte this holds, but not when such a PTE is installed by
> the new install_file_pte; previously it didn't store protections, only file
> offsets, with the patches it also stores protections, and can set
> _PAGE_PROTNONE|_PAGE_FILE.
Why is this combination useful? Can't you just drop the _PAGE_FILE from
_PAGE_PROTNONE ptes?
>
> zap_pte_range, when acting on such a pte, calls vm_normal_page and gets
> &mem_map[0], does page_remove_rmap, and we're easily in trouble, because it
> happens to find a page with mapcount == 0. And it BUGs on this!
>
> I've seen this trigger easily and repeatably on UML on 2.6.16-rc3. This was
> likely avoided in the past by the PageReserved test - page 0 *had* to be
> reserved on i386 (dunno on UML).
>
> Implementation follows for UML and i386.
>
> To avoid additional overhead, I also considered adding likely() for
> _PAGE_PRESENT and unlikely() for the rest, but I'm uncertain about validity of
> possible [un]likely(pte_present()) occurrences.
Not present pages are likely to be pretty common when unmapping.
I don't like this patch much.
--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com
On Tuesday 02 May 2006 05:53, Nick Piggin wrote:
> [email protected] wrote:
> > From: Paolo 'Blaisorblade' Giarrusso <[email protected]>
> >
> > pte_present(pte) implies that pte_pfn(pte) is valid. Normally even with a
> > _PAGE_PROTNONE pte this holds, but not when such a PTE is installed by
> > the new install_file_pte; previously it didn't store protections, only
> > file offsets, with the patches it also stores protections, and can set
> > _PAGE_PROTNONE|_PAGE_FILE.
What could be done is to set a PTE with "no protection", use another bit
rather than _PAGE_PROTNONE. This wastes one more bit but doable.
> Why is this combination useful? Can't you just drop the _PAGE_FILE from
> _PAGE_PROTNONE ptes?
I must think on this, but the semantics are not entirely the same between the
two cases. You have no page attached when _PAGE_FILE is there, but a page is
attached to the PTE with only _PAGE_PROTNONE. Testing that via VM_MANYPROTS
is just as slow as-is (can be changed with code duplication for the linear
and non-linear cases).
The application semantics can also be different when you remap as read/write
that page - the app could have stored an offset there (this is less definite
since you can't remap & keep the offset currently).
Also, this wouldn't solve the problem, it would make the solution harder: how
do I know that there's no page to call page_remove_rmap() on, without
_PAGE_FILE?
I thought to change _PAGE_PROTNONE: it is used to hold a page present and
referenced but unaccessible. It seems it could be released when
_PAGE_PROTNONE is set, but for anonymous memory it's impossible. When I've
asked Hugh about this, he imagined the case when an application faults in a
page in a VMA then mprotects(PROT_NONE) it; the PTE is set as PROT_NONE. We
can avoid that in the VM_MAYSHARE case (VM_SHARED or PROT_SHARED was set but
the file is readonly), but not when anonymous memory is present - the
application could want it back.
> > To avoid additional overhead, I also considered adding likely() for
> > _PAGE_PRESENT and unlikely() for the rest, but I'm uncertain about
> > validity of possible [un]likely(pte_present()) occurrences.
>
> Not present pages are likely to be pretty common when unmapping.
Ok, only unlikely for test on _PAGE_PROTNONE and ! _PAGE_FILE.
--
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade
Chiacchiera con i tuoi amici in tempo reale!
http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com
Blaisorblade wrote:
> On Tuesday 02 May 2006 05:53, Nick Piggin wrote:
>
>>[email protected] wrote:
>>
>>>From: Paolo 'Blaisorblade' Giarrusso <[email protected]>
>>>
>>>pte_present(pte) implies that pte_pfn(pte) is valid. Normally even with a
>>>_PAGE_PROTNONE pte this holds, but not when such a PTE is installed by
>>>the new install_file_pte; previously it didn't store protections, only
>>>file offsets, with the patches it also stores protections, and can set
>>>_PAGE_PROTNONE|_PAGE_FILE.
>
>
> What could be done is to set a PTE with "no protection", use another bit
> rather than _PAGE_PROTNONE. This wastes one more bit but doable.
I see.
>
>
>>Why is this combination useful? Can't you just drop the _PAGE_FILE from
>>_PAGE_PROTNONE ptes?
>
>
> I must think on this, but the semantics are not entirely the same between the
> two cases.
And yes, this won't work. I was misunderstanding what was happening.
I guess your problem is that you're overloading the pte protection bits
for present ptes as protection bits for not present (file) ptes. I'd rather
you just used a different encoding for file pte protections then.
"Wasting" a bit seems much more preferable for this very uncommon case (for
most people) rather than bloating pte_present check, which is called in
practically every performance critical inner loop).
That said, if the patch is i386/uml specific then I don't have much say in
it. If Ingo/Linus and Jeff/Yourself, respectively, accept the patch, then
fine.
But I think you should drop the comment from the core code. It seems wrong.
--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com
On Saturday 06 May 2006 12:03, Nick Piggin wrote:
> Blaisorblade wrote:
> > On Tuesday 02 May 2006 05:53, Nick Piggin wrote:
> >>[email protected] wrote:
> >>>From: Paolo 'Blaisorblade' Giarrusso <[email protected]>
> >>>
> >>>pte_present(pte) implies that pte_pfn(pte) is valid. Normally even with
> >>> a _PAGE_PROTNONE pte this holds, but not when such a PTE is installed
> >>> by the new install_file_pte; previously it didn't store protections,
> >>> only file offsets, with the patches it also stores protections, and can
> >>> set _PAGE_PROTNONE|_PAGE_FILE.
> >
> > What could be done is to set a PTE with "no protection", use another bit
> > rather than _PAGE_PROTNONE. This wastes one more bit but doable.
> I see.
> I guess your problem is that you're overloading the pte protection bits
> for present ptes as protection bits for not present (file) ptes. I'd rather
> you just used a different encoding for file pte protections then.
Yes, this is what I said above, so we agree; and indeed this overloading was
decided when the present problem didn't trigger, so it can now change. As
detailed in the patch description, the previous PageReserved handling
prevented freeing page 0 and hided this.
> "Wasting" a bit seems much more preferable for this very uncommon case (for
> most people) rather than bloating pte_present check, which is called in
> practically every performance critical inner loop).
Yes, I thought about this problem, I wasn't sure how hard it was.
> That said, if the patch is i386/uml specific then I don't have much say in
> it.
It's presently specific, but will probably extend. Implementations for some
other archs were already sent and I've collected them (will send
afterwards,I've avoided excess bloat).
> If Ingo/Linus and Jeff/Yourself, respectively, accept the patch, then
> fine.
> But I think you should drop the comment from the core code. It seems wrong.
Yep, forgot there, thanks for reminding, I've now removed it.
--
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade
Chiacchiera con i tuoi amici in tempo reale!
http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com