linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: "Thomas Hellström (Intel)" <thomas_os@shipmail.org>
To: "Christian König" <christian.koenig@amd.com>,
	dri-devel@lists.freedesktop.org
Cc: David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Andrew Morton <akpm@linux-foundation.org>,
	Jason Gunthorpe <jgg@nvidia.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] mm,drm/ttm: Use VM_PFNMAP for TTM vmas
Date: Mon, 22 Mar 2021 09:13:13 +0100	[thread overview]
Message-ID: <5578ccaa-9751-3a01-1846-330c09bf9ce7@shipmail.org> (raw)
In-Reply-To: <d49fad17-f515-d4f2-cef2-4108c8375747@amd.com>

Hi!

On 3/22/21 8:47 AM, Christian König wrote:
> Am 21.03.21 um 19:45 schrieb Thomas Hellström (Intel):
>> To block fast gup we need to make sure TTM ptes are always special.
>> With MIXEDMAP we, on architectures that don't support pte_special,
>> insert normal ptes, but OTOH on those architectures, fast is not
>> supported.
>> At the same time, the function documentation to vm_normal_page() 
>> suggests
>> that ptes pointing to system memory pages of MIXEDMAP vmas are always
>> normal, but that doesn't seem consistent with what's implemented in
>> vmf_insert_mixed(). I'm thus not entirely sure this patch is actually
>> needed.
>>
>> But to make sure and to avoid also normal (non-fast) gup, make all
>> TTM vmas PFNMAP. With PFNMAP we can't allow COW mappings
>> anymore so make is_cow_mapping() available and use it to reject
>> COW mappigs at mmap time.
>
> I would separate the disallowing of COW mapping from the PFN change. 
> I'm pretty sure that COW mappings never worked on TTM BOs in the first 
> place.

COW doesn't work with PFNMAP together with non-linear maps, so as a 
consequence from moving from MIXEDMAP to PFNMAP we must disallow COW, so 
it seems logical to me to do it in one patch.

And working COW was one of the tests I used for huge PMDs/PUDs, so it 
has indeed been working, but I can't think of any relevant use-cases.

Did you, BTW, have a chance to test this with WC mappings?

Thanks,
/Thomas



>
> But either way this patch is Reviewed-by: Christian König 
> <christian.koenig@amd.com>.
>
> Thanks,
> Christian.
>
>>
>> There was previously a comment in the code that WC mappings together
>> with x86 PAT + PFNMAP was bad for performance. However from looking at
>> vmf_insert_mixed() it looks like in the current code PFNMAP and MIXEDMAP
>> are handled the same for architectures that support pte_special. This
>> means there should not be a performance difference anymore, but this
>> needs to be verified.
>>
>> Cc: Christian Koenig <christian.koenig@amd.com>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Jason Gunthorpe <jgg@nvidia.com>
>> Cc: linux-mm@kvack.org
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Thomas Hellström (Intel) <thomas_os@shipmail.org>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo_vm.c | 22 ++++++++--------------
>>   include/linux/mm.h              |  5 +++++
>>   mm/internal.h                   |  5 -----
>>   3 files changed, 13 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c 
>> b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>> index 1c34983480e5..708c6fb9be81 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>> @@ -372,12 +372,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct 
>> vm_fault *vmf,
>>            * at arbitrary times while the data is mmap'ed.
>>            * See vmf_insert_mixed_prot() for a discussion.
>>            */
>> -        if (vma->vm_flags & VM_MIXEDMAP)
>> -            ret = vmf_insert_mixed_prot(vma, address,
>> -                            __pfn_to_pfn_t(pfn, PFN_DEV),
>> -                            prot);
>> -        else
>> -            ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
>> +        ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
>>             /* Never error on prefaulted PTEs */
>>           if (unlikely((ret & VM_FAULT_ERROR))) {
>> @@ -555,18 +550,14 @@ static void ttm_bo_mmap_vma_setup(struct 
>> ttm_buffer_object *bo, struct vm_area_s
>>        * Note: We're transferring the bo reference to
>>        * vma->vm_private_data here.
>>        */
>> -
>>       vma->vm_private_data = bo;
>>         /*
>> -     * We'd like to use VM_PFNMAP on shared mappings, where
>> -     * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
>> -     * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
>> -     * bad for performance. Until that has been sorted out, use
>> -     * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
>> +     * PFNMAP forces us to block COW mappings in mmap(),
>> +     * and with MIXEDMAP we would incorrectly allow fast gup
>> +     * on TTM memory on architectures that don't have pte_special.
>>        */
>> -    vma->vm_flags |= VM_MIXEDMAP;
>> -    vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>> +    vma->vm_flags |= VM_PFNMAP | VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>>   }
>>     int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>> @@ -579,6 +570,9 @@ int ttm_bo_mmap(struct file *filp, struct 
>> vm_area_struct *vma,
>>       if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET_START))
>>           return -EINVAL;
>>   +    if (unlikely(is_cow_mapping(vma->vm_flags)))
>> +        return -EINVAL;
>> +
>>       bo = ttm_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma));
>>       if (unlikely(!bo))
>>           return -EINVAL;
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 77e64e3eac80..c6ebf7f9ddbb 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -686,6 +686,11 @@ static inline bool vma_is_accessible(struct 
>> vm_area_struct *vma)
>>       return vma->vm_flags & VM_ACCESS_FLAGS;
>>   }
>>   +static inline bool is_cow_mapping(vm_flags_t flags)
>> +{
>> +    return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
>> +}
>> +
>>   #ifdef CONFIG_SHMEM
>>   /*
>>    * The vma_is_shmem is not inline because it is used only by slow
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 9902648f2206..1432feec62df 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -296,11 +296,6 @@ static inline unsigned int buddy_order(struct 
>> page *page)
>>    */
>>   #define buddy_order_unsafe(page) READ_ONCE(page_private(page))
>>   -static inline bool is_cow_mapping(vm_flags_t flags)
>> -{
>> -    return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
>> -}
>> -
>>   /*
>>    * These three helpers classifies VMAs for virtual memory accounting.
>>    */


  reply	other threads:[~2021-03-22  8:13 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-21 18:45 [RFC PATCH 0/2] mm,drm/ttm: Always block GUP to TTM pages Thomas Hellström (Intel)
2021-03-21 18:45 ` [RFC PATCH 1/2] mm,drm/ttm: Block fast GUP to TTM huge pages Thomas Hellström (Intel)
2021-03-23 11:34   ` Daniel Vetter
2021-03-23 16:34     ` Thomas Hellström (Intel)
2021-03-23 16:37       ` Jason Gunthorpe
2021-03-23 16:59         ` Christoph Hellwig
2021-03-23 17:06         ` Thomas Hellström (Intel)
2021-03-24  9:56           ` Daniel Vetter
2021-03-24 12:24             ` Jason Gunthorpe
2021-03-24 12:35               ` Thomas Hellström (Intel)
2021-03-24 12:41                 ` Jason Gunthorpe
2021-03-24 13:35                   ` Thomas Hellström (Intel)
2021-03-24 13:48                     ` Jason Gunthorpe
2021-03-24 15:50                       ` Thomas Hellström (Intel)
2021-03-24 16:38                         ` Jason Gunthorpe
2021-03-24 18:31                           ` Christian König
2021-03-24 20:07                             ` Thomas Hellström (Intel)
2021-03-24 23:14                               ` Jason Gunthorpe
2021-03-25  7:48                                 ` Thomas Hellström (Intel)
2021-03-25  8:27                                   ` Christian König
2021-03-25  9:51                                     ` Thomas Hellström (Intel)
2021-03-25 11:30                                       ` Jason Gunthorpe
2021-03-25 11:53                                         ` Thomas Hellström (Intel)
2021-03-25 12:01                                           ` Jason Gunthorpe
2021-03-25 12:09                                             ` Christian König
2021-03-25 12:36                                               ` Thomas Hellström (Intel)
2021-03-25 13:02                                                 ` Christian König
2021-03-25 13:31                                                   ` Thomas Hellström (Intel)
2021-03-25 12:42                                               ` Jason Gunthorpe
2021-03-25 13:05                                                 ` Christian König
2021-03-25 13:17                                                   ` Jason Gunthorpe
2021-03-25 13:26                                                     ` Christian König
2021-03-25 13:33                                                       ` Jason Gunthorpe
2021-03-25 13:54                                                         ` Christian König
2021-03-25 13:56                                                           ` Jason Gunthorpe
2021-03-25  7:49                                 ` Christian König
2021-03-25  9:41                                   ` Daniel Vetter
2021-03-23 13:52   ` Jason Gunthorpe
2021-03-23 15:05     ` Thomas Hellström (Intel)
2021-03-23 19:52   ` Williams, Dan J
2021-03-23 20:42     ` Thomas Hellström (Intel)
2021-03-24  9:58       ` Daniel Vetter
2021-03-24 10:05         ` Thomas Hellström (Intel)
     [not found]           ` <75423f64-adef-a2c4-8e7d-2cb814127b18@intel.com>
2021-03-24 20:22             ` Thomas Hellström (Intel)
2021-03-24 20:25               ` Dave Hansen
2021-03-25 17:51                 ` Thomas Hellström (Intel)
2021-03-25 17:55                   ` Jason Gunthorpe
2021-03-25 18:13                     ` Thomas Hellström (Intel)
2021-03-25 18:24                       ` Jason Gunthorpe
2021-03-25 18:42                         ` Thomas Hellström (Intel)
2021-03-26  9:08                         ` Thomas Hellström (Intel)
2021-03-26 11:46                           ` Jason Gunthorpe
2021-03-26 12:33                             ` Thomas Hellström (Intel)
2021-03-21 18:45 ` [RFC PATCH 2/2] mm,drm/ttm: Use VM_PFNMAP for TTM vmas Thomas Hellström (Intel)
2021-03-22  7:47   ` Christian König
2021-03-22  8:13     ` Thomas Hellström (Intel) [this message]
2021-03-23 11:57       ` Christian König
2021-03-23 11:47   ` Daniel Vetter
2021-03-23 14:04     ` Jason Gunthorpe
2021-03-23 15:51       ` Thomas Hellström (Intel)
2021-03-23 14:00   ` Jason Gunthorpe
2021-03-23 15:46     ` Thomas Hellström (Intel)
2021-03-23 16:06       ` Jason Gunthorpe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5578ccaa-9751-3a01-1846-330c09bf9ce7@shipmail.org \
    --to=thomas_os@shipmail.org \
    --cc=airlied@linux.ie \
    --cc=akpm@linux-foundation.org \
    --cc=christian.koenig@amd.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jgg@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).