* [PATCH] drm/i915: Only free the oldest stale object before a fresh allocation
@ 2017-07-05 17:21 Chris Wilson
2017-07-05 17:39 ` ✓ Fi.CI.BAT: success for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Chris Wilson @ 2017-07-05 17:21 UTC (permalink / raw)
To: intel-gfx
Inspired by Tvrtko's critique of the reaping of the stale contexts
before allocating a new one, also limit the freed object reaping to the
oldest stale object before allocating a fresh object. Unlike contexts,
objects may have radically different sizes of backing storage, but
similar to contexts, whilst we want to prevent starvation due to
excessive freed lists, we also want do not want to delay fresh
allocations for too long. Only freeing the oldest on the freed object
list before each allocation is a reasonable compromise.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 91145c39cd43..2048532c9cae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4473,9 +4473,13 @@ static void i915_gem_flush_free_objects(struct drm_i915_private *i915)
{
struct llist_node *freed;
- freed = llist_del_all(&i915->mm.free_list);
- if (unlikely(freed))
+ freed = NULL;
+ if (!llist_empty(&i915->mm.free_list))
+ freed = llist_del_first(&i915->mm.free_list);
+ if (unlikely(freed)) {
+ freed->next = NULL;
__i915_gem_free_objects(i915, freed);
+ }
}
static void __i915_gem_free_work(struct work_struct *work)
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Only free the oldest stale object before a fresh allocation
2017-07-05 17:21 [PATCH] drm/i915: Only free the oldest stale object before a fresh allocation Chris Wilson
@ 2017-07-05 17:39 ` Patchwork
2017-07-06 11:40 ` [PATCH] " Tvrtko Ursulin
2017-07-18 17:32 ` [PATCH v2] " Chris Wilson
2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2017-07-05 17:39 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Only free the oldest stale object before a fresh allocation
URL : https://patchwork.freedesktop.org/series/26864/
State : success
== Summary ==
Series 26864v1 drm/i915: Only free the oldest stale object before a fresh allocation
https://patchwork.freedesktop.org/api/1.0/series/26864/revisions/1/mbox/
Test kms_busy:
Subgroup basic-flip-default-b:
dmesg-warn -> PASS (fi-skl-6700hq) fdo#101144
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:437s
fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:358s
fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:538s
fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:508s
fi-byt-j1900 total:279 pass:254 dwarn:1 dfail:0 fail:0 skip:24 time:485s
fi-byt-n2820 total:279 pass:250 dwarn:1 dfail:0 fail:0 skip:28 time:480s
fi-glk-2a total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:593s
fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:435s
fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:415s
fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:427s
fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:496s
fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:474s
fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:464s
fi-kbl-7560u total:279 pass:268 dwarn:1 dfail:0 fail:0 skip:10 time:569s
fi-kbl-r total:279 pass:260 dwarn:1 dfail:0 fail:0 skip:18 time:584s
fi-pnv-d510 total:279 pass:221 dwarn:3 dfail:0 fail:0 skip:55 time:556s
fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:460s
fi-skl-6700hq total:279 pass:262 dwarn:0 dfail:0 fail:0 skip:17 time:586s
fi-skl-6700k total:279 pass:257 dwarn:4 dfail:0 fail:0 skip:18 time:463s
fi-skl-6770hq total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:480s
fi-skl-gvtdvm total:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:437s
fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:544s
fi-snb-2600 total:279 pass:250 dwarn:0 dfail:0 fail:0 skip:29 time:406s
24346e831017070c18f3c33b74a7b098682e20f7 drm-tip: 2017y-07m-04d-15h-39m-34s UTC integration manifest
51e95b2 drm/i915: Only free the oldest stale object before a fresh allocation
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5117/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Only free the oldest stale object before a fresh allocation
2017-07-05 17:21 [PATCH] drm/i915: Only free the oldest stale object before a fresh allocation Chris Wilson
2017-07-05 17:39 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-07-06 11:40 ` Tvrtko Ursulin
2017-07-06 11:44 ` Chris Wilson
2017-07-18 17:32 ` [PATCH v2] " Chris Wilson
2 siblings, 1 reply; 6+ messages in thread
From: Tvrtko Ursulin @ 2017-07-06 11:40 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 05/07/2017 18:21, Chris Wilson wrote:
> Inspired by Tvrtko's critique of the reaping of the stale contexts
> before allocating a new one, also limit the freed object reaping to the
> oldest stale object before allocating a fresh object. Unlike contexts,
> objects may have radically different sizes of backing storage, but
> similar to contexts, whilst we want to prevent starvation due to
> excessive freed lists, we also want do not want to delay fresh
> allocations for too long. Only freeing the oldest on the freed object
> list before each allocation is a reasonable compromise.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 91145c39cd43..2048532c9cae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4473,9 +4473,13 @@ static void i915_gem_flush_free_objects(struct drm_i915_private *i915)
> {
> struct llist_node *freed;
>
> - freed = llist_del_all(&i915->mm.free_list);
> - if (unlikely(freed))
> + freed = NULL;
> + if (!llist_empty(&i915->mm.free_list))
> + freed = llist_del_first(&i915->mm.free_list);
Looks like llist_del_first already handles the empty list case by
returning NULL so you could simplify this.
> + if (unlikely(freed)) { > + freed->next = NULL;
> __i915_gem_free_objects(i915, freed);
> + }
> }
>
> static void __i915_gem_free_work(struct work_struct *work)
>
With the simplification:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Only free the oldest stale object before a fresh allocation
2017-07-06 11:40 ` [PATCH] " Tvrtko Ursulin
@ 2017-07-06 11:44 ` Chris Wilson
2017-07-06 11:51 ` Tvrtko Ursulin
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2017-07-06 11:44 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2017-07-06 12:40:56)
>
> On 05/07/2017 18:21, Chris Wilson wrote:
> > Inspired by Tvrtko's critique of the reaping of the stale contexts
> > before allocating a new one, also limit the freed object reaping to the
> > oldest stale object before allocating a fresh object. Unlike contexts,
> > objects may have radically different sizes of backing storage, but
> > similar to contexts, whilst we want to prevent starvation due to
> > excessive freed lists, we also want do not want to delay fresh
> > allocations for too long. Only freeing the oldest on the freed object
> > list before each allocation is a reasonable compromise.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 91145c39cd43..2048532c9cae 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4473,9 +4473,13 @@ static void i915_gem_flush_free_objects(struct drm_i915_private *i915)
> > {
> > struct llist_node *freed;
> >
> > - freed = llist_del_all(&i915->mm.free_list);
> > - if (unlikely(freed))
> > + freed = NULL;
> > + if (!llist_empty(&i915->mm.free_list))
> > + freed = llist_del_first(&i915->mm.free_list);
>
> Looks like llist_del_first already handles the empty list case by
> returning NULL so you could simplify this.
I didn't want the extra call here (since object allocation is far more
frequent than context allocation), but that's just me.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Only free the oldest stale object before a fresh allocation
2017-07-06 11:44 ` Chris Wilson
@ 2017-07-06 11:51 ` Tvrtko Ursulin
0 siblings, 0 replies; 6+ messages in thread
From: Tvrtko Ursulin @ 2017-07-06 11:51 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 06/07/2017 12:44, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-07-06 12:40:56)
>>
>> On 05/07/2017 18:21, Chris Wilson wrote:
>>> Inspired by Tvrtko's critique of the reaping of the stale contexts
>>> before allocating a new one, also limit the freed object reaping to the
>>> oldest stale object before allocating a fresh object. Unlike contexts,
>>> objects may have radically different sizes of backing storage, but
>>> similar to contexts, whilst we want to prevent starvation due to
>>> excessive freed lists, we also want do not want to delay fresh
>>> allocations for too long. Only freeing the oldest on the freed object
>>> list before each allocation is a reasonable compromise.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_gem.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index 91145c39cd43..2048532c9cae 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -4473,9 +4473,13 @@ static void i915_gem_flush_free_objects(struct drm_i915_private *i915)
>>> {
>>> struct llist_node *freed;
>>>
>>> - freed = llist_del_all(&i915->mm.free_list);
>>> - if (unlikely(freed))
>>> + freed = NULL;
>>> + if (!llist_empty(&i915->mm.free_list))
>>> + freed = llist_del_first(&i915->mm.free_list);
>>
>> Looks like llist_del_first already handles the empty list case by
>> returning NULL so you could simplify this.
>
> I didn't want the extra call here (since object allocation is far more
> frequent than context allocation), but that's just me.
Ah OK then, I didn't spot that angle. R-b applies to v1 then. Could be
useful to stick a comment in there so someone does not clean it up in
the future. :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] drm/i915: Only free the oldest stale object before a fresh allocation
2017-07-05 17:21 [PATCH] drm/i915: Only free the oldest stale object before a fresh allocation Chris Wilson
2017-07-05 17:39 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-07-06 11:40 ` [PATCH] " Tvrtko Ursulin
@ 2017-07-18 17:32 ` Chris Wilson
2 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2017-07-18 17:32 UTC (permalink / raw)
To: intel-gfx
Inspired by Tvrtko's critique of the reaping of the stale contexts
before allocating a new one, also limit the freed object reaping to the
oldest stale object before allocating a fresh object. Unlike contexts,
objects may have radically different sizes of backing storage, but
similar to contexts, whilst we want to prevent starvation due to
excessive freed lists, we also want do not want to delay fresh
allocations for too long. Only freeing the oldest on the freed object
list before each allocation is a reasonable compromise.
v2: Only a single consumer of llist_del_first() is allowed (although
multiple llist_add are still allowed in parallel). Unlike
i915_gem_context, i915_gem_flush_free_objects() is itself not serialized
and so we need to add our own spinlock. Otherwise KASAN eventually spots
a use-after-free for the race on *first->next.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v1
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++++++--
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 667fb5c44483..e1c2bc66e0fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1447,6 +1447,7 @@ struct i915_gem_mm {
*/
struct llist_head free_list;
struct work_struct free_work;
+ spinlock_t free_lock;
/** Usable portion of the GTT for GEM */
dma_addr_t stolen_base; /* limited to low memory (32-bit) */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2c2907fef612..d57603f64e96 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4542,9 +4542,18 @@ static void i915_gem_flush_free_objects(struct drm_i915_private *i915)
{
struct llist_node *freed;
- freed = llist_del_all(&i915->mm.free_list);
- if (unlikely(freed))
+ /* Free the oldest, most stale object to keep the free_list short */
+ freed = NULL;
+ if (!llist_empty(&i915->mm.free_list)) { /* quick test for hotpath */
+ /* Only one consumer of llist_del_first() allowed */
+ spin_lock(&i915->mm.free_lock);
+ freed = llist_del_first(&i915->mm.free_list);
+ spin_unlock(&i915->mm.free_lock);
+ }
+ if (unlikely(freed)) {
+ freed->next = NULL;
__i915_gem_free_objects(i915, freed);
+ }
}
static void __i915_gem_free_work(struct work_struct *work)
@@ -5026,6 +5035,7 @@ i915_gem_load_init(struct drm_i915_private *dev_priv)
INIT_WORK(&dev_priv->mm.free_work, __i915_gem_free_work);
spin_lock_init(&dev_priv->mm.obj_lock);
+ spin_lock_init(&dev_priv->mm.free_lock);
init_llist_head(&dev_priv->mm.free_list);
INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
INIT_LIST_HEAD(&dev_priv->mm.bound_list);
--
2.13.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-18 17:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-05 17:21 [PATCH] drm/i915: Only free the oldest stale object before a fresh allocation Chris Wilson
2017-07-05 17:39 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-07-06 11:40 ` [PATCH] " Tvrtko Ursulin
2017-07-06 11:44 ` Chris Wilson
2017-07-06 11:51 ` Tvrtko Ursulin
2017-07-18 17:32 ` [PATCH v2] " Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.