Re: [PATCH] drm/vma-manager: Don't unmap COW'd pages when zapping bo ptes

[Thomas Hellstrom <thellstrom@vmware.com>]

On 11/21/2013 09:18 AM, Daniel Vetter wrote:
> On Wed, Nov 20, 2013 at 11:35:31PM +0100, Thomas Hellstrom wrote:
>> On 11/20/2013 03:24 PM, Daniel Vetter wrote:
>>> On Wed, Nov 20, 2013 at 01:55:49AM -0800, Thomas Hellstrom wrote:
>>>> Not sure if there are any user-space users of private bo mappings, but
>>>> if there are, or will be, zapping the COW'd pages when, for example,
>>>> moving a bo would confuse the user immensely since the net effect for the
>>>> user would be that pages written to would lose their contents.
>>>>
>>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>>> Presuming I'm not horribly confused about that all the vm slang in the
>>> kerneldoc means this changes is
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> Now I still hold that userspace creating anynomous bo mappings is rather
>>> crazy, but meh ;-) I guess the real question is whether we have anyone
>>> relying on this out there (or planing to), in which case we either need to
>>> funnel this through stable kernels or whack a drm feature flag onto
>>> drivers/kernels with this fixed. But I seriously hope the answer is no.
>>>
>>> Cheers, Daniel
>>>
>> Thanks for reviewing. I don't think there's a need to take this
>> through stable, since I don't know of anyone using private VMAs,
>>
>> Actually I'll need to hold off on this for a while since on some
>> archs this may cause
>> ptes of shared pages to not be zapped. If the arch doesn't have
>> PTE_SPECIAL, shared pages on MIXEDMAP vmas will come through as
>> vm_normal_page, and since page->mapping is usually (un)set to NULL
>> by our drivers, this will result in false positives for COW'ed
>> pages.
>>
>> So that test is buggy, or us not setting page->mapping to the
>> mapping we use is buggy. It's too late in the day to decide which.
> On second thought we also use this helper when zapping a bo when
> deleting it or purging it's backing storage. And there I'd say it would be
> consistent with normal semantics to also zap cow pages.

Agreed. That's consistent with truncation. Perhaps this was what David 
meant.
Out of interest, why do you delete a bo or purge backing store before 
all vmas are gone?

>   So I'm no longer
> sure this is the right approach in general. Maybe we should instead wire
> up the switch, but not sure whether that's indeed worth the effort.

I'll leave it as is. If I happen to sort out the non-cow zapping, I'll 
post a new patch.

/Thomas



> -Daniel