| Message ID | 1354912628-7776-3-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 07 Dec 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: > As the shrinker may be invoked for the allocation, and it may reap > neighbouring objects in the offset range mm, we need to be careful in > the order in which we allocate the node, search for free space and then > insert the node into the mmap offset range manager. > > Fixes i-g-t/gem_tiled_swapping > > Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem.c | 59 ++++++++++++++++++++++++++++++++------- > 1 file changed, 49 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index d17f52d..3ab97c6 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1512,14 +1512,29 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev, > static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) > { > struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > + struct drm_gem_mm *mm = obj->base.dev->mm_private; > + struct drm_map_list *list; > int ret; > > - if (obj->base.map_list.map) > + list = &obj->base.map_list; > + if (list->map) > return 0; > > - ret = drm_gem_create_mmap_offset(&obj->base); > - if (ret != -ENOSPC) > - return ret; > + /* Set the object up for mmap'ing */ > + list->map = kzalloc(sizeof(struct drm_map_list), GFP_KERNEL); sizeof(struct drm_local_map) or sizeof(*list->map) instead? Hmm, it's like this in drm_gem_create_mmap_offset too, either it's a bug or I'm being clueless. Other than that, with the same disclaimer as on the previous patch, Reviewed-by: Jani Nikula <jani.nikula@intel.com> > + if (!list->map) > + return -ENOMEM; > + > + list->map->type = _DRM_GEM; > + list->map->size = obj->base.size; > + list->map->handle = &obj->base; > + > + /* Get a DRM GEM mmap offset allocated... */ > + list->file_offset_node = kzalloc(sizeof(struct drm_mm_node), GFP_KERNEL); > + if (list->file_offset_node == NULL) { > + ret = -ENOMEM; > + goto out_free_list; > + } > > /* Badly fragmented mmap space? The only way we can recover > * space is by destroying unwanted objects. We can't randomly release > @@ -1528,13 +1543,37 @@ static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) > * offsets on purgeable objects by truncating it and marking it purged, > * which prevents userspace from ever using that object again. > */ > - i915_gem_purge(dev_priv, obj->base.size >> PAGE_SHIFT); > - ret = drm_gem_create_mmap_offset(&obj->base); > - if (ret != -ENOSPC) > - return ret; > + ret = drm_mm_insert_node(&mm->offset_manager, list->file_offset_node, > + obj->base.size / PAGE_SIZE, 0); > + if (ret) { > + i915_gem_purge(dev_priv, obj->base.size >> PAGE_SHIFT); > + ret = drm_mm_insert_node(&mm->offset_manager, list->file_offset_node, > + obj->base.size / PAGE_SIZE, 0); > + } > + if (ret) { > + i915_gem_shrink_all(dev_priv); > + ret = drm_mm_insert_node(&mm->offset_manager, list->file_offset_node, > + obj->base.size / PAGE_SIZE, 0); > + } > + if (ret) { > + kfree(list->file_offset_node); > + goto out_free_list; > + } > + > + list->hash.key = list->file_offset_node->start; > + ret = drm_ht_insert_item(&mm->offset_hash, &list->hash); > + if (ret) > + goto out_free_mm; > > - i915_gem_shrink_all(dev_priv); > - return drm_gem_create_mmap_offset(&obj->base); > + return 0; > + > +out_free_mm: > + drm_mm_put_block(list->file_offset_node); > +out_free_list: > + kfree(list->map); > + list->map = NULL; > + > + return ret; > } > > static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj) > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 12 Dec 2012 12:48:43 +0200, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Fri, 07 Dec 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > As the shrinker may be invoked for the allocation, and it may reap > > neighbouring objects in the offset range mm, we need to be careful in > > the order in which we allocate the node, search for free space and then > > insert the node into the mmap offset range manager. > > > > Fixes i-g-t/gem_tiled_swapping > > > > Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 59 ++++++++++++++++++++++++++++++++------- > > 1 file changed, 49 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index d17f52d..3ab97c6 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -1512,14 +1512,29 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev, > > static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) > > { > > struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > > + struct drm_gem_mm *mm = obj->base.dev->mm_private; > > + struct drm_map_list *list; > > int ret; > > > > - if (obj->base.map_list.map) > > + list = &obj->base.map_list; > > + if (list->map) > > return 0; > > > > - ret = drm_gem_create_mmap_offset(&obj->base); > > - if (ret != -ENOSPC) > > - return ret; > > + /* Set the object up for mmap'ing */ > > + list->map = kzalloc(sizeof(struct drm_map_list), GFP_KERNEL); > > sizeof(struct drm_local_map) or sizeof(*list->map) instead? > > Hmm, it's like this in drm_gem_create_mmap_offset too, either it's a bug > or I'm being clueless. You're right it's a a bug but fortunately in the safe direction, I'd vote for sizeof(*list->map) as unequivocal. -Chris
On Wed, Dec 12, 2012 at 10:55:17AM +0000, Chris Wilson wrote: > On Wed, 12 Dec 2012 12:48:43 +0200, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Fri, 07 Dec 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > As the shrinker may be invoked for the allocation, and it may reap > > > neighbouring objects in the offset range mm, we need to be careful in > > > the order in which we allocate the node, search for free space and then > > > insert the node into the mmap offset range manager. > > > > > > Fixes i-g-t/gem_tiled_swapping > > > > > > Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > drivers/gpu/drm/i915/i915_gem.c | 59 ++++++++++++++++++++++++++++++++------- > > > 1 file changed, 49 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > > index d17f52d..3ab97c6 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > @@ -1512,14 +1512,29 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev, > > > static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) > > > { > > > struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > > > + struct drm_gem_mm *mm = obj->base.dev->mm_private; > > > + struct drm_map_list *list; > > > int ret; > > > > > > - if (obj->base.map_list.map) > > > + list = &obj->base.map_list; > > > + if (list->map) > > > return 0; > > > > > > - ret = drm_gem_create_mmap_offset(&obj->base); > > > - if (ret != -ENOSPC) > > > - return ret; > > > + /* Set the object up for mmap'ing */ > > > + list->map = kzalloc(sizeof(struct drm_map_list), GFP_KERNEL); > > > > sizeof(struct drm_local_map) or sizeof(*list->map) instead? > > > > Hmm, it's like this in drm_gem_create_mmap_offset too, either it's a bug > > or I'm being clueless. > > You're right it's a a bug but fortunately in the safe direction, I'd > vote for sizeof(*list->map) as unequivocal. All applied with bikeshed and pushed to -fixes, thanks. -Daniel
> As the shrinker may be invoked for the allocation, and it may reap > neighbouring objects in the offset range mm, we need to be careful in > the order in which we allocate the node, search for free space and then > insert the node into the mmap offset range manager. Maybe I'm being a bit stupid here, but this seems like a pointless micro optimisation thrown in with a deinlining. Surely just calling drm_gem_create_mmap_offset a few times would work fine. Like does it really fail that often, and is the extra overhead when it does going to matter? Dave.
On Tue, Dec 18, 2012 at 10:11:14PM +0100, Daniel Vetter wrote: > On Wed, Dec 12, 2012 at 10:55:17AM +0000, Chris Wilson wrote: > > On Wed, 12 Dec 2012 12:48:43 +0200, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > On Fri, 07 Dec 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > As the shrinker may be invoked for the allocation, and it may reap > > > > neighbouring objects in the offset range mm, we need to be careful in > > > > the order in which we allocate the node, search for free space and then > > > > insert the node into the mmap offset range manager. > > > > > > > > Fixes i-g-t/gem_tiled_swapping > > > > > > > > Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > --- > > > > drivers/gpu/drm/i915/i915_gem.c | 59 ++++++++++++++++++++++++++++++++------- > > > > 1 file changed, 49 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > > > index d17f52d..3ab97c6 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > > @@ -1512,14 +1512,29 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev, > > > > static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) > > > > { > > > > struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > > > > + struct drm_gem_mm *mm = obj->base.dev->mm_private; > > > > + struct drm_map_list *list; > > > > int ret; > > > > > > > > - if (obj->base.map_list.map) > > > > + list = &obj->base.map_list; > > > > + if (list->map) > > > > return 0; > > > > > > > > - ret = drm_gem_create_mmap_offset(&obj->base); > > > > - if (ret != -ENOSPC) > > > > - return ret; > > > > + /* Set the object up for mmap'ing */ > > > > + list->map = kzalloc(sizeof(struct drm_map_list), GFP_KERNEL); > > > > > > sizeof(struct drm_local_map) or sizeof(*list->map) instead? > > > > > > Hmm, it's like this in drm_gem_create_mmap_offset too, either it's a bug > > > or I'm being clueless. > > > > You're right it's a a bug but fortunately in the safe direction, I'd > > vote for sizeof(*list->map) as unequivocal. > > All applied with bikeshed and pushed to -fixes, thanks. Reconsidering just this patch, this smells too much like we leak our own shrinker internals still. Can't we just set a dev_priv flag which tells the shrinker not to attempt lock-stealing while we call down into the drm mmap functions? The issue is that people are talking about unifying the mmap lookup stuff a bit since it's superflously duplicated between ttm and drm. So us rolling our own special-case is a bit awkward. I've dropped the patch for now. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index d17f52d..3ab97c6 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1512,14 +1512,29 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev, static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) { struct drm_i915_private *dev_priv = obj->base.dev->dev_private; + struct drm_gem_mm *mm = obj->base.dev->mm_private; + struct drm_map_list *list; int ret; - if (obj->base.map_list.map) + list = &obj->base.map_list; + if (list->map) return 0; - ret = drm_gem_create_mmap_offset(&obj->base); - if (ret != -ENOSPC) - return ret; + /* Set the object up for mmap'ing */ + list->map = kzalloc(sizeof(struct drm_map_list), GFP_KERNEL); + if (!list->map) + return -ENOMEM; + + list->map->type = _DRM_GEM; + list->map->size = obj->base.size; + list->map->handle = &obj->base; + + /* Get a DRM GEM mmap offset allocated... */ + list->file_offset_node = kzalloc(sizeof(struct drm_mm_node), GFP_KERNEL); + if (list->file_offset_node == NULL) { + ret = -ENOMEM; + goto out_free_list; + } /* Badly fragmented mmap space? The only way we can recover * space is by destroying unwanted objects. We can't randomly release @@ -1528,13 +1543,37 @@ static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) * offsets on purgeable objects by truncating it and marking it purged, * which prevents userspace from ever using that object again. */ - i915_gem_purge(dev_priv, obj->base.size >> PAGE_SHIFT); - ret = drm_gem_create_mmap_offset(&obj->base); - if (ret != -ENOSPC) - return ret; + ret = drm_mm_insert_node(&mm->offset_manager, list->file_offset_node, + obj->base.size / PAGE_SIZE, 0); + if (ret) { + i915_gem_purge(dev_priv, obj->base.size >> PAGE_SHIFT); + ret = drm_mm_insert_node(&mm->offset_manager, list->file_offset_node, + obj->base.size / PAGE_SIZE, 0); + } + if (ret) { + i915_gem_shrink_all(dev_priv); + ret = drm_mm_insert_node(&mm->offset_manager, list->file_offset_node, + obj->base.size / PAGE_SIZE, 0); + } + if (ret) { + kfree(list->file_offset_node); + goto out_free_list; + } + + list->hash.key = list->file_offset_node->start; + ret = drm_ht_insert_item(&mm->offset_hash, &list->hash); + if (ret) + goto out_free_mm; - i915_gem_shrink_all(dev_priv); - return drm_gem_create_mmap_offset(&obj->base); + return 0; + +out_free_mm: + drm_mm_put_block(list->file_offset_node); +out_free_list: + kfree(list->map); + list->map = NULL; + + return ret; } static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
As the shrinker may be invoked for the allocation, and it may reap neighbouring objects in the offset range mm, we need to be careful in the order in which we allocate the node, search for free space and then insert the node into the mmap offset range manager. Fixes i-g-t/gem_tiled_swapping Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 59 ++++++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 10 deletions(-)