Re: [PATCH] drm: Eliminate dev->vmalist

Hi

On Fri, Aug 16, 2013 at 1:24 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

The primary purpose of this list is for pretty printing the maps through debugfs/vma - per process information can be found in /proc/*/map et al. However, the list manipulation eats processor cycles resulting in a near indefinite (I got bored) stall for a leaky i-g-t/gem_concurrent_blit.

There is one true user of the vmalist though. drm_vm_shm_close() uses the list to count the number of drm_local_map references. For this, we switch to using a counter inside drm_local_map.

This patch kills drm_vm_open_locked and drm_vm_close_locked, and redirects the one user outside of the core (exoynos) to use the now semantically equivalent drm_gem_vm_open.

I don't like the drm_vma_open/close() function names. We currently use the "drm_vma_*" prefix in drm_vma_manager.c. On the other hand, this file exclusively uses "drm_vma_node_*" and "drm_vma_offset_*" prefixes, so maybe we're fine here. Don't know..

I tried removing drm_vma_open/close entirely, but it seems not all drivers set vma->vma_file->private_data to the drm_device, hmm. So the patch looks fine: Reviewed-by: David Herrmann dh.herrmann@gmail.com

Regards David

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Dave Airlie airlied@redhat.com Cc: Rob Clark robdclark@gmail.com Cc: Inki Dae inki.dae@samsung.com

---

drivers/gpu/drm/drm_bufs.c | 1 + drivers/gpu/drm/drm_drv.c | 8 ----- drivers/gpu/drm/drm_gem.c | 16 +++------ drivers/gpu/drm/drm_info.c | 38 --------------------- drivers/gpu/drm/drm_stub.c | 1 - drivers/gpu/drm/drm_vm.c | 58 +++++++-------------------------- drivers/gpu/drm/exynos/exynos_drm_gem.c | 9 +---- include/drm/drmP.h | 7 ++-- 8 files changed, 20 insertions(+), 118 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index bef4abf..b0aa2eb 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -152,6 +152,7 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset, map->size = size; map->flags = flags; map->type = type;

•   map->count = 0;
  
    /* Only allow shared memory to be removable since we only keep enough
     * book keeping information about shared memory to allow for removal
  

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index dddd799..7ec9959 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -182,8 +182,6 @@ static const struct drm_ioctl_desc drm_ioctls[] = { */ int drm_lastclose(struct drm_device * dev) {

•   struct drm_vma_entry *vma, *vma_temp;
  

•   DRM_DEBUG("\n");
  
    if (dev->driver->lastclose)
  

@@ -203,12 +201,6 @@ int drm_lastclose(struct drm_device * dev) dev->sg = NULL; }

•   /* Clear vma list (only built for debugging) */
  

•   list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) {
  

•           list_del(&vma->head);
  

•           kfree(vma);
  

•   }
  

•   if (drm_core_check_feature(dev, DRIVER_HAVE_DMA) &&
        !drm_core_check_feature(dev, DRIVER_MODESET))
            drm_dma_takedown(dev);
  

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 9ab038c..84a5834 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -573,22 +573,16 @@ void drm_gem_vm_open(struct vm_area_struct *vma) struct drm_gem_object *obj = vma->vm_private_data;

    drm_gem_object_reference(obj);

•   mutex_lock(&obj->dev->struct_mutex);
  

•   drm_vm_open_locked(obj->dev, vma);
  

•   mutex_unlock(&obj->dev->struct_mutex);
  

•   drm_vma_open(obj->dev, vma);
  

} EXPORT_SYMBOL(drm_gem_vm_open);

void drm_gem_vm_close(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data;

•   struct drm_device *dev = obj->dev;
  
  

•   mutex_lock(&dev->struct_mutex);
  

•   drm_vm_close_locked(obj->dev, vma);
  

•   drm_gem_object_unreference(obj);
  

•   mutex_unlock(&dev->struct_mutex);
  

•   drm_vma_close(obj->dev, vma);
  

•   drm_gem_object_unreference_unlocked(obj);
  

} EXPORT_SYMBOL(drm_gem_vm_close);

@@ -639,9 +633,7 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, * (which should happen whether the vma was created by this call, or * by a vm_open due to mremap or partial unmap or whatever). */

•   drm_gem_object_reference(obj);
  

•   drm_vm_open_locked(dev, vma);
  

•   drm_gem_vm_open(vma);
    return 0;
  

} EXPORT_SYMBOL(drm_gem_mmap_obj); diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index d4b20ce..6ea817c 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -228,49 +228,11 @@ int drm_vma_info(struct seq_file *m, void *data) { struct drm_info_node *node = (struct drm_info_node *) m->private; struct drm_device *dev = node->minor->dev;

•   struct drm_vma_entry *pt;
  

•   struct vm_area_struct *vma;
  

-#if defined(__i386__)

•   unsigned int pgprot;
  

-#endif

•   mutex_lock(&dev->struct_mutex);
    seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n",
               atomic_read(&dev->vma_count),
               high_memory, (void *)(unsigned long)virt_to_phys(high_memory));
  
  

•   list_for_each_entry(pt, &dev->vmalist, head) {
  

•           vma = pt->vma;
  

•           if (!vma)
  

•                   continue;
  

•           seq_printf(m,
  

•                      "\n%5d 0x%pK-0x%pK %c%c%c%c%c%c 0x%08lx000",
  

•                      pt->pid,
  

•                      (void *)vma->vm_start, (void *)vma->vm_end,
  

•                      vma->vm_flags & VM_READ ? 'r' : '-',
  

•                      vma->vm_flags & VM_WRITE ? 'w' : '-',
  

•                      vma->vm_flags & VM_EXEC ? 'x' : '-',
  

•                      vma->vm_flags & VM_MAYSHARE ? 's' : 'p',
  

•                      vma->vm_flags & VM_LOCKED ? 'l' : '-',
  

•                      vma->vm_flags & VM_IO ? 'i' : '-',
  

•                      vma->vm_pgoff);
  

-#if defined(__i386__)

•           pgprot = pgprot_val(vma->vm_page_prot);
  

•           seq_printf(m, " %c%c%c%c%c%c%c%c%c",
  

•                      pgprot & _PAGE_PRESENT ? 'p' : '-',
  

•                      pgprot & _PAGE_RW ? 'w' : 'r',
  

•                      pgprot & _PAGE_USER ? 'u' : 's',
  

•                      pgprot & _PAGE_PWT ? 't' : 'b',
  

•                      pgprot & _PAGE_PCD ? 'u' : 'c',
  

•                      pgprot & _PAGE_ACCESSED ? 'a' : '-',
  

•                      pgprot & _PAGE_DIRTY ? 'd' : '-',
  

•                      pgprot & _PAGE_PSE ? 'm' : 'k',
  

•                      pgprot & _PAGE_GLOBAL ? 'g' : 'l');
  

-#endif

•           seq_printf(m, "\n");
  

•   }
  

•   mutex_unlock(&dev->struct_mutex);
    return 0;
  

}

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index d663f7d..0a1db60 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -258,7 +258,6 @@ int drm_fill_in_dev(struct drm_device *dev,

    INIT_LIST_HEAD(&dev->filelist);
    INIT_LIST_HEAD(&dev->ctxlist);

•   INIT_LIST_HEAD(&dev->vmalist);
    INIT_LIST_HEAD(&dev->maplist);
    INIT_LIST_HEAD(&dev->vblank_event_list);
  
  

diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index feb2003..bc2b26f 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -213,7 +213,6 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) { struct drm_file *priv = vma->vm_file->private_data; struct drm_device *dev = priv->minor->dev;

•   struct drm_vma_entry *pt, *temp;
    struct drm_local_map *map;
    struct drm_map_list *r_list;
    int found_maps = 0;
  

@@ -225,17 +224,8 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) map = vma->vm_private_data;

    mutex_lock(&dev->struct_mutex);

•   list_for_each_entry_safe(pt, temp, &dev->vmalist, head) {
  

•           if (pt->vma->vm_private_data == map)
  

•                   found_maps++;
  

•           if (pt->vma == vma) {
  

•                   list_del(&pt->head);
  

•                   kfree(pt);
  

•           }
  

•   }
  

•   /* We were the only map that was found */
  

•   if (found_maps == 1 && map->flags & _DRM_REMOVABLE) {
  

•   if (--map->count == 0) {
            /* Check to see if we are in the maplist, if we are not, then
             * we delete this mappings information.
             */
  

@@ -402,68 +392,41 @@ static const struct vm_operations_struct drm_vm_sg_ops = {

• Create a new drm_vma_entry structure as the \p vma private data entry and
• add it to drm_device::vmalist.

*/ -void drm_vm_open_locked(struct drm_device *dev,

•           struct vm_area_struct *vma)
  

+void drm_vma_open(struct drm_device *dev,

•             struct vm_area_struct *vma)
  

{

•   struct drm_vma_entry *vma_entry;
  

•   DRM_DEBUG("0x%08lx,0x%08lx\n",
              vma->vm_start, vma->vm_end - vma->vm_start);
    atomic_inc(&dev->vma_count);
  

•   vma_entry = kmalloc(sizeof(*vma_entry), GFP_KERNEL);
  

•   if (vma_entry) {
  

•           vma_entry->vma = vma;
  

•           vma_entry->pid = current->pid;
  

•           list_add(&vma_entry->head, &dev->vmalist);
  

•   }
  

} -EXPORT_SYMBOL_GPL(drm_vm_open_locked);

static void drm_vm_open(struct vm_area_struct *vma) { struct drm_file *priv = vma->vm_file->private_data; struct drm_device *dev = priv->minor->dev;

•   mutex_lock(&dev->struct_mutex);
  

•   drm_vm_open_locked(dev, vma);
  

•   mutex_unlock(&dev->struct_mutex);
  

•   drm_vma_open(dev, vma);
  

}

-void drm_vm_close_locked(struct drm_device *dev,

•           struct vm_area_struct *vma)
  

+void drm_vma_close(struct drm_device *dev,

•              struct vm_area_struct *vma)
  

{

•   struct drm_vma_entry *pt, *temp;
  

•   DRM_DEBUG("0x%08lx,0x%08lx\n",
              vma->vm_start, vma->vm_end - vma->vm_start);
    atomic_dec(&dev->vma_count);
  

•   list_for_each_entry_safe(pt, temp, &dev->vmalist, head) {
  

•           if (pt->vma == vma) {
  

•                   list_del(&pt->head);
  

•                   kfree(pt);
  

•                   break;
  

•           }
  

•   }
  

}

/**

• \c close method for all virtual memory types.
• \param vma virtual memory area.

• • Search the \p vma private data entry in drm_device::vmalist, unlink it, and
• • free it.
  */

static void drm_vm_close(struct vm_area_struct *vma) { struct drm_file *priv = vma->vm_file->private_data; struct drm_device *dev = priv->minor->dev;

•   mutex_lock(&dev->struct_mutex);
  

•   drm_vm_close_locked(dev, vma);
  

•   mutex_unlock(&dev->struct_mutex);
  

•   drm_vma_close(dev, vma);
  

}

/** @@ -513,7 +476,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma)

    vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;

•   drm_vm_open_locked(dev, vma);
  

•   drm_vma_open(dev, vma);
    return 0;
  

}

@@ -543,7 +506,7 @@ int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) { struct drm_file *priv = filp->private_data; struct drm_device *dev = priv->minor->dev;

•   struct drm_local_map *map = NULL;
  

•   struct drm_local_map *map;
    resource_size_t offset = 0;
    struct drm_hash_item *hash;
  
  

@@ -648,8 +611,9 @@ int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) return -EINVAL; /* This should never happen. */ } vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;

•   map->count++;
  
  

•   drm_vm_open_locked(dev, vma);
  

•   drm_vma_open(dev, vma);
    return 0;
  

}

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index b904633..04cb3ea 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -401,14 +401,7 @@ static int exynos_drm_gem_mmap_buffer(struct file *filp, return ret; }

•   /*
  

•    * take a reference to this mapping of the object. And this reference
  

•    * is unreferenced by the corresponding vm_close call.
  

•    */
  

•   drm_gem_object_reference(obj);
  

•   drm_vm_open_locked(drm_dev, vma);
  

•   drm_gem_vm_open(vma);
    return 0;
  

}

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3ecdde6..e4b8658 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -573,6 +573,7 @@ struct drm_local_map { void *handle; /**< User-space: "Handle" to pass to mmap() */ /**< Kernel-space: kernel-virtual address */ int mtrr; /**< MTRR slot used */

•   int count;
  

};

typedef struct drm_local_map drm_local_map_t; @@ -1116,8 +1117,6 @@ struct drm_device {

    struct idr ctx_idr;

•   struct list_head vmalist;       /**< List of vmas (for debugging) */
  

•   /*@} */
  
    /** \name DMA support */
  

@@ -1274,8 +1273,8 @@ extern int drm_release(struct inode *inode, struct file *filp); /* Mapping support (drm_vm.h) */ extern int drm_mmap(struct file *filp, struct vm_area_struct *vma); extern int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma); -extern void drm_vm_open_locked(struct drm_device *dev, struct vm_area_struct *vma); -extern void drm_vm_close_locked(struct drm_device *dev, struct vm_area_struct *vma); +extern void drm_vma_open(struct drm_device *dev, struct vm_area_struct *vma); +extern void drm_vma_close(struct drm_device *dev, struct vm_area_struct *vma); extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);

                            /* Memory management support (drm_memory.h) */

-- 1.8.4.rc2

---

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