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

[Chris Wilson]

On Fri, 2009-06-19 at 10:06 +0800, Shaohua Li wrote:
> 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.

The issue here is that we cannot shrink the inactive list during a
failed allocation within i915 (as we already hold the dev->struct_mutex
and the shrinker also requires that lock). The shmfs pages are all
allocated at once during i915_gem_object_get_pages() which is quite
often the first victim during memory pressure. So in order to do
emergency shrinkage to map the current working set into the apperture,
we have to manually purge the inactive lists.

If we fail to obtain memory for ourselves, we report the error back to
userspace. But at that point the system is extremely short on memory and
the OOM killer will no doubt select a victim, which tends to be an
unpleasant experience.

> > @@ -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.

No problem, from my reading of release_pages() I thought it was just a
more efficient, batched put_page().

> > +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;

We could add the check, but the shrinker will never be called if
(gfp_mask & __GFP_WAIT) and I did not see any instances of the guard in
other shrinkers.

> > +       /* 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?

After reading fs/inode.c, I was paranoid about calling
truncate_inode_pages() from a GFP_NOFS context.

> > +       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.

I was getting carried away... :-)

Thanks for reviewing the patch.
-ickle