Re: [PATCH 2/2] drm/ttm: track kmap io_reserve(), only unreserve on unmap where needed

[Thomas Hellstrom <thomas@shipmail.org>]

[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]

Ben,

I had something like the attached in mind, although it might be more 
beneficial to do the actual refcounting in drivers that needs it. Atomic 
incs and decs are expensive, but I'm not sure how expensive relative to 
function pointer calls.

Patch is only compile-tested

/Thomas


On 11/04/2010 02:08 PM, Thomas Hellstrom wrote:
> On 11/04/2010 01:03 AM, Ben Skeggs wrote:
>> From: Ben Skeggs<bskeggs@redhat.com>
>>
>> If the driver kmaps an object userspace is expecting to be mapped, the
>> unmap would have called down into the drivers io_unreserve() function
>> and potentially unmapped the pages from its BARs (for example) and 
>> they'd
>> no longer be accessible for the userspace mapping.
>>
>> Signed-off-by: Ben Skeggs<bskeggs@redhat.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo_util.c |   14 ++++++++++----
>>   include/drm/ttm/ttm_bo_api.h      |    1 +
>>   2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
>> b/drivers/gpu/drm/ttm/ttm_bo_util.c
>> index ff358ad..e9dbe8b 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>> @@ -467,9 +467,12 @@ int ttm_bo_kmap(struct ttm_buffer_object *bo,
>>       if (num_pages>  1&&  !DRM_SUSER(DRM_CURPROC))
>>           return -EPERM;
>>   #endif
>> -    ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
>> -    if (ret)
>> -        return ret;
>> +    if (!bo->mem.bus.io_reserved) {
>> +        ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
>> +        if (ret)
>> +            return ret;
>> +        map->io_reserved = true;
>> +    }
>>       if (!bo->mem.bus.is_iomem) {
>>           return ttm_bo_kmap_ttm(bo, start_page, num_pages, map);
>>       } else {
>> @@ -487,7 +490,10 @@ void ttm_bo_kunmap(struct ttm_bo_kmap_obj *map)
>>       switch (map->bo_kmap_type) {
>>       case ttm_bo_map_iomap:
>>           iounmap(map->virtual);
>> -        ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
>> +        if (map->io_reserved) {
>> +            ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
>> +            map->io_reserved = false;
>> +        }
>>           break;
>>       case ttm_bo_map_vmap:
>>           vunmap(map->virtual);
>> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
>> index 5afa5b5..ce998ac 100644
>> --- a/include/drm/ttm/ttm_bo_api.h
>> +++ b/include/drm/ttm/ttm_bo_api.h
>> @@ -300,6 +300,7 @@ struct ttm_bo_kmap_obj {
>>           ttm_bo_map_premapped    = 4 | TTM_BO_MAP_IOMEM_MASK,
>>       } bo_kmap_type;
>>       struct ttm_buffer_object *bo;
>> +    bool io_reserved;
>>   };
>>
>>   /**
>>
>
> This doesn't solve the problem unfortunately. Consider the sequence
>
> kmap->io_mem_reserve
> fault()->
> kunmap->io_mem_free
> user_space_access()-> Invalid.
>
> I think this needs to be fixed by us maintaining an 
> mem:io_reserved_count, where all user-space triggered io_reserves 
> count as 1. A mem::user_space_io_reserved flag could be protected by 
> the bo::reserve lock, whereas a reserved_count can't, since strictly 
> you're allowed to kmap a bo without reserving it, but only if it's pinned
>
> /Thomas
> .
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-drm-ttm-Fix-up-io_mem_reserve-free.patch --]
[-- Type: text/x-patch; name="0001-drm-ttm-Fix-up-io_mem_reserve-free.patch", Size: 6918 bytes --]

>From 7fc680ed912c2f9f230d4d14eec47e34c83b3816 Mon Sep 17 00:00:00 2001
From: Thomas Hellstrom <thellstrom@vmware.com>
Date: Thu, 4 Nov 2010 19:27:51 +0100
Subject: [PATCH] drm/ttm: Fix up io_mem_reserve / free

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c      |   12 ++++++++----
 drivers/gpu/drm/ttm/ttm_bo_util.c |   34 ++++++++++++++++++++++++----------
 drivers/gpu/drm/ttm/ttm_bo_vm.c   |    4 ++--
 include/drm/ttm/ttm_bo_api.h      |    5 ++++-
 include/drm/ttm/ttm_bo_driver.h   |    8 ++++----
 mm/mmap.c                         |    2 +-
 6 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index ce46457..040d51c 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -442,6 +442,7 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
 		bo->ttm = NULL;
 	}
 
+	ttm_mem_io_free_vm(bo->bdev, &bo->mem);
 	ttm_bo_mem_put(bo, &bo->mem);
 
 	atomic_set(&bo->reserved, 0);
@@ -704,7 +705,8 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo, bool interruptible,
 
 	evict_mem = bo->mem;
 	evict_mem.mm_node = NULL;
-	evict_mem.bus.io_reserved = false;
+	evict_mem.bus.io_reserved_vm = false;
+	atomic_set(&evict_mem.bus.io_reserved_count, -1);
 
 	placement.fpfn = 0;
 	placement.lpfn = 0;
@@ -1041,7 +1043,8 @@ int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
 	mem.num_pages = bo->num_pages;
 	mem.size = mem.num_pages << PAGE_SHIFT;
 	mem.page_alignment = bo->mem.page_alignment;
-	mem.bus.io_reserved = false;
+	mem.bus.io_reserved_vm = false;
+	atomic_set(&mem.bus.io_reserved_count, -1);
 	/*
 	 * Determine where to move the buffer.
 	 */
@@ -1166,7 +1169,8 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
 	bo->mem.num_pages = bo->num_pages;
 	bo->mem.mm_node = NULL;
 	bo->mem.page_alignment = page_alignment;
-	bo->mem.bus.io_reserved = false;
+	bo->mem.bus.io_reserved_vm = false;
+	atomic_set(&bo->mem.bus.io_reserved_count, -1);
 	bo->buffer_start = buffer_start & PAGE_MASK;
 	bo->priv_flags = 0;
 	bo->mem.placement = (TTM_PL_FLAG_SYSTEM | TTM_PL_FLAG_CACHED);
@@ -1555,7 +1559,7 @@ void ttm_bo_unmap_virtual(struct ttm_buffer_object *bo)
 	if (!bdev->dev_mapping)
 		return;
 	unmap_mapping_range(bdev->dev_mapping, offset, holelen, 1);
-	ttm_mem_io_free(bdev, &bo->mem);
+	ttm_mem_io_free_vm(bdev, &bo->mem);
 }
 EXPORT_SYMBOL(ttm_bo_unmap_virtual);
 
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index ff358ad..ac2b2fa 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -75,26 +75,40 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
 }
 EXPORT_SYMBOL(ttm_bo_move_ttm);
 
-int ttm_mem_io_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
+static int ttm_mem_io_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
+{
+	if (bdev->driver->io_mem_reserve &&
+	    atomic_inc_and_test(&mem->bus.io_reserved_count))
+		return bdev->driver->io_mem_reserve(bdev, mem);
+	return 0;
+}
+
+static void ttm_mem_io_free(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
+{
+	if (bdev->driver->io_mem_reserve &&
+	    atomic_add_negative(-1, &mem->bus.io_reserved_count) &&
+	    bdev->driver->io_mem_free)
+		bdev->driver->io_mem_free(bdev, mem);
+}
+
+int ttm_mem_io_reserve_vm(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
 {
 	int ret;
 
-	if (!mem->bus.io_reserved) {
-		mem->bus.io_reserved = true;
-		ret = bdev->driver->io_mem_reserve(bdev, mem);
+	if (!mem->bus.io_reserved_vm) {
+		ret = ttm_mem_io_reserve(bdev, mem);
 		if (unlikely(ret != 0))
 			return ret;
+		mem->bus.io_reserved_vm = true;
 	}
 	return 0;
 }
 
-void ttm_mem_io_free(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
+void ttm_mem_io_free_vm(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
 {
-	if (bdev->driver->io_mem_reserve) {
-		if (mem->bus.io_reserved) {
-			mem->bus.io_reserved = false;
-			bdev->driver->io_mem_free(bdev, mem);
-		}
+	if (mem->bus.io_reserved_vm) {
+		mem->bus.io_reserved_vm = false;
+		ttm_mem_io_free(bdev, mem);
 	}
 }
 
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index fe6cb77..6383cc3 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -131,8 +131,8 @@ static int ttm_bo_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 		spin_unlock(&bo->lock);
 
 
-	ret = ttm_mem_io_reserve(bdev, &bo->mem);
-	if (ret) {
+	ret = ttm_mem_io_reserve_vm(bdev, &bo->mem);
+	if (unlikely(ret != 0)) {
 		retval = VM_FAULT_SIGBUS;
 		goto out_unlock;
 	}
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 5afa5b5..9b98b53 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -74,6 +74,8 @@ struct ttm_placement {
  * @is_iomem:		is this io memory ?
  * @size:		size in byte
  * @offset:		offset from the base address
+ * @io_reserved_vm:     The VM system has a refcount in @io_reserved_count
+ * @io_reserved_count:  Refcounting the numbers of callers to ttm_mem_io_reserve
  *
  * Structure indicating the bus placement of an object.
  */
@@ -83,7 +85,8 @@ struct ttm_bus_placement {
 	unsigned long	size;
 	unsigned long	offset;
 	bool		is_iomem;
-	bool		io_reserved;
+	bool		io_reserved_vm;
+	atomic_t        io_reserved_count;
 };
 
 
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 8e0c848..e42a8d7 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -773,10 +773,10 @@ extern int ttm_bo_pci_offset(struct ttm_bo_device *bdev,
 			     unsigned long *bus_offset,
 			     unsigned long *bus_size);
 
-extern int ttm_mem_io_reserve(struct ttm_bo_device *bdev,
-				struct ttm_mem_reg *mem);
-extern void ttm_mem_io_free(struct ttm_bo_device *bdev,
-				struct ttm_mem_reg *mem);
+extern int ttm_mem_io_reserve_vm(struct ttm_bo_device *bdev,
+				 struct ttm_mem_reg *mem);
+extern void ttm_mem_io_free_vm(struct ttm_bo_device *bdev,
+			       struct ttm_mem_reg *mem);
 
 extern void ttm_bo_global_release(struct drm_global_reference *ref);
 extern int ttm_bo_global_init(struct drm_global_reference *ref);
diff --git a/mm/mmap.c b/mm/mmap.c
index 00161a4..55935a9 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1872,7 +1872,7 @@ find_extend_vma(struct mm_struct * mm, unsigned long addr)
  *
  * Called with the mm semaphore held.
  */
-static void remove_vma_list(struct mm_struct *mm, struct vm_area_struct *vma)
+static void \±XAJ-SA34Y OAD ·QA<Z9<4weremove_vma_list(struct mm_struct *mm, struct vm_area_struct *vma)
 {
 	/* Update high watermark before we lower total_vm */
 	update_hiwater_vm(mm);
-- 
1.6.2.5


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel