[Intel-gfx] [PATCH] drm/i915: Shrink the inactive bo-cache under memory pressure

[Shaohua Li]

On Fri, Jun 19, 2009 at 07:14:55AM +0800, Chris Wilson wrote:
> Register a shrinker that evicts inactive objects when under memory
> pressure.  In order to discard unused pages directly we introduce a
> madvise style ioctl for userspace to mark unneeded objects. This ioctl
> also provides a facility by which userspace can query whether a buffer was
> discarded and so may then take remedial action. (This can be, for instance,
> used to implement APPLE_purgeable_object. However, in the first instance
> it is used by libdrm to clear discarded buffers from its own bo-cache.)
Thanks for merging the patches.
 
> index 8104eca..9df4c48 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -141,6 +141,18 @@ drm_gem_object_alloc(struct drm_device *dev, size_t size)
>                 kfree(obj);
>                 return NULL;
>         }
> +       /* Basically we want to disable the OOM killer and handle ENOMEM
> +        * ourselves by sacrificing pages from cached buffers.
> +        * XXX shmem_file_[gs]et_gfp_mask()
> +        */
> +       mapping_set_gfp_mask(obj->filp->f_path.dentry->d_inode->i_mapping,
> +                            GFP_HIGHUSER |
> +                            __GFP_COLD |
> +                            __GFP_FS |
> +                            __GFP_RECLAIMABLE |
> +                            __GFP_NORETRY |
> +                            __GFP_NOWARN |
> +                            __GFP_NOMEMALLOC);
I have concern about the logic to handle the out of memory by i915_gem.
In below code, the patch will try to free bo if page allocation fail. But this
seems not reliable. If memory is tight, even i915_gem frees some memory, other
applications will use it soon. It's not guaranteed gem can get back the freed
memory.
Besides, if memory is tight, page reclaim will do its job and already hook gem
shrinking into page reclaim, why bother to add another mechanism to address the
issue by gem? It just makes things complex.

> @@ -1404,15 +1424,23 @@ i915_gem_object_put_pages(struct drm_gem_object *obj)
>         if (obj_priv->tiling_mode != I915_TILING_NONE)
>                 i915_gem_object_save_bit_17_swizzle(obj);
> 
> -       for (i = 0; i < page_count; i++)
> -               if (obj_priv->pages[i] != NULL) {
> -                       if (obj_priv->dirty)
> -                               set_page_dirty(obj_priv->pages[i]);
> -                       mark_page_accessed(obj_priv->pages[i]);
> -                       page_cache_release(obj_priv->pages[i]);
> -               }
> +       if (obj_priv->madv == I915_MADV_DONTNEED)
> +           obj_priv->dirty = 0;
> +
> +       for (i = 0; i < page_count; i++) {
> +               if (obj_priv->pages[i] == NULL)
> +                       break;
> +
> +               if (obj_priv->dirty)
> +                       set_page_dirty(obj_priv->pages[i]);
> +
> +               if (obj_priv->madv == I915_MADV_WILLNEED)
> +                   mark_page_accessed(obj_priv->pages[i]);
> +       }
>         obj_priv->dirty = 0;
> 
> +       release_pages(obj_priv->pages, i, 1);
It appears release_pages() isn't exported to module. And it will try to delete
page from zone lru list, so I thought this isn't a good API here.

> +static int
> +i915_gem_shrink(int nr_to_scan, gfp_t gfp_mask)
> +{
> +       drm_i915_private_t *dev_priv, *next_dev;
> +       struct drm_i915_gem_object *obj_priv, *next_obj;
> +       int cnt = 0;
> +       int would_deadlock = 1;
> +
> +
i915_gem_object_unbind() might sleep, so we need a check here:
if (!(gfp_mask &__GFP_WAIT))
	return 0;
> +       /* XXX fairness over multiple devices? */
> +
> +       /* first scan for clean buffers */
> +       list_for_each_entry_safe(dev_priv, next_dev,
> +                                &shrink_list, mm.shrink_list) {
> +               struct drm_device *dev = dev_priv->dev;
> +
> +               if (mutex_trylock(&dev->struct_mutex)) {
> +                       spin_unlock(&shrink_list_lock);
> +
> +                       list_for_each_entry_safe(obj_priv, next_obj,
> +                                                &dev_priv->mm.inactive_list,
> +                                                list) {
> +                               struct drm_gem_object *obj = obj_priv->obj;
> +
> +                               if (obj_priv->madv == I915_MADV_DONTNEED) {
> +                                       i915_gem_object_unbind(obj);
> +                                       if (gfp_mask & __GFP_FS)
> +                                               i915_gem_object_truncate (obj);
why had __GFP_FS check here?

> +       if (nr_to_scan > 0) {
> +               int have_waited = 0;
> +
> +               /* third pass, wait for request to retire */
> +               list_for_each_entry_safe(dev_priv, next_dev,
> +                                        &shrink_list, mm.shrink_list) {
> +                       struct drm_device *dev = dev_priv->dev;
> +
> +                       if (mutex_trylock(&dev->struct_mutex)) {
> +                               spin_unlock(&shrink_list_lock);
> +
> +                               if (!list_empty(&dev_priv->mm.request_list)) {
> +                                       struct drm_i915_gem_request *request;
> +                                       int ret;
> +
> +                                       request = list_first_entry(&dev_priv->mm.request_list,
> +                                                                  struct drm_i915_gem_request,
> +                                                                  list);
> +                                       ret = i915_wait_request(dev,
> +                                                               request->seqno);
> +                                       have_waited += ret == 0;
> +                               }
> +
> +                               spin_lock(&shrink_list_lock);
> +                               mutex_unlock(&dev->struct_mutex);
> +                       }
> +               }
this is a little complex. Page reclaim will do sleep if it can't reclaim memory
and try again. So doing wait here seems unnecessary.

Thanks,
Shaohua