Re: Happy New Year PARISC

[John David Anglin <dave.anglin@bell.net>]

On 1/3/2012 11:42 AM, James Bottomley wrote:
> On Tue, 2012-01-03 at 11:26 -0500, John David Anglin wrote:
>> On 1/3/2012 10:32 AM, James Bottomley wrote:
>>> On Tue, 2012-01-03 at 10:13 -0500, John David Anglin wrote:
>>>> On 1/3/2012 6:50 AM, Carlos O'Donell wrote:
>>>>> On Mon, Jan 2, 2012 at 6:12 PM, John David Anglin<dave.anglin@bell.net>    wrote:
>>>>>> None of this worked.  Attached patch as it stands.  Comments and testing
>>>>>> appreciated.
>>>>> Could you clarify what you mean by "none of this worked?"
>>>>>
>>>> I tried eliminating the flushes that occur in kunmap_atomic on PA8800
>>>> and PA8900
>>>> after the calls to clear_user_page and copy_user_page by defining
>>>> clear_user_highpage
>>>> and copy_user_highpage.  I had thought the flushes weren't necessary.
>>>> There's something
>>>> about this that I don't understand.  Why do we need to flush
>>>> non-equivalent page mappings
>>>> that aren't used?
>>> But they are used:  Your work makes sure that all user space mappings
>>> are equivalent.  However, because of the way Linux sets up kernel
>>> mappings (from the pfn array and offsets) the user virtual address and
>>> kernel virtual address almost never are.  kmap is exclusively used so
>>> the kernel can access a user page, and at that point, we need to flush
>>> because we've set up an inequivalent alias (even if it's only done for
>>> read)
>>>
>>> kmap/kmap_atomic is used in more than just copy/flush ... or did you
>>> mean that you removed the kmap calls in copy/flush and the whole thing
>>> doesn't work (rather than as you imply you removed the flush in kunmap?)
>>>
>> I didn't modify kmap/kunmap_atomic.  I wrote versions of
>> clear_user_highpage and
>> copy_user_highpage to replace the default versions in linux/highmem.h.
>> I replaced
>> the kunmap_atomic calls with pagefault_enable to avoid the flush in the
>> returns from
>> clear/copy_user_page (actually, I only used one call to
>> pagefault_enable, so maybe
>> that was the issue).  As far as I could tell, clear/copy_user_page are
>> only called via
>> clear/copy_user_highpage.  The behavior of kmap/kunmap_atomic in other
>> situations
>> shouldn't have changed.
>>
>> Chapter F makes it clear that *all* inequivalent aliases to a page have
>> to be removed
>> when a write capable translation is enabled (no flush needed).  When a
>> write-capable
>> translation needs to be read through an inequivalent alias, the page is
>> supposed to
>> be flushed, the write-capable translation is supposed to be removed from
>> the page
>> directory and then purged.
>>
>> That's why I added the purge_tlb_entries calls to set_pte_at and
>> ptep_set_wrprotect.
>> We avoid the flush by doing the `from' read through an equivalent
>> mapping.  However,
>> the inequivalent mapping is still there.  It seems to be necessary to
>> purge the TLB
>> entries prior to clearing/copying.  However, from what I read in Chapter
>> F, the purge
>> is probably insufficient to speculative prevent move in.  If I recall
>> correctly, the
>> kunmap_atomic also generates another TLB purge as well as a flush.
>>
>> There is a special access type (7) that can be used to prevent read and
>> write move in.
> Actually, now I recall why copy_user_highpage never got implemented
> through the tmpalias space:  it does cache hot copies (so we effectively
> copy straight from the cache of the source address into the cache of the
> destination).  This is all fine and dandy and very fast until we have to
> copy executable pages: in this case, we set up an I/D cache
> inconsistency in userspace (which userspace apparently doesn't expect).
> It can be resolved by flushing the userspace cache, so the page becomes
> up to date an I movein sees the correct data, which is probably what the
> flush you still need is doing.
>
Note that I added flush code to "copy_user_page_asm" to handle this.  I 
guess this
flush could be avoided if we knew the page wasn't executable, but it is 
not that easy
to figure out if a page is executable without the vma or mm.  I also 
added 64-bit
support and renamed the old version to copy_page_asm so both versions 
would be
available for use/testing.

I see that copy_user_highpage does have the vma, so maybe there is some hope
of further optimizing copy_user_page_asm.

I came to the conclusion that it was better to do a flush when copying 
through the
tmpalias space than try to flush the `from' page in ptep_set_wrprotect 
as proposed
by Niibe a couple of years ago.  First, a COW page may never be written 
to, so
why do  a unnecessary flush (my assumption based on the documentation is 
that
clear/copy_user_page are primarily for COW support)?

At the time the minifail bug was being discussed, I hadn't realized that 
the TLB
needed purging in set_pte_at and ptep_set_wrprotect.  This became clear when
I looked at arm.  Without the TLB purge, the flush proposed by Niibe was 
still
racy.

Dave

-- 
John David Anglin    dave.anglin@bell.net