Re: [Intel-gfx] [PATCH 1/3] drm/i915/gem: Defer obj->base.resv fini until RCU callback
Quoting Mika Kuoppala (2019-07-04 15:18:40)
> Chris Wilson writes:
>
> > Quoting Mika Kuoppala (2019-07-04 14:53:09)
> >> Chris Wilson writes:
> >> > +static void shmem_release(struct drm_i915_gem_object *obj)
> >> > +{
> >> > + fput(obj->base.filp);
> >>
> >> We lose the check for filp existence. But as internal
> >> ops have their own mechanics, we should always have the filp.
> >
> > Exactly. drm_gem_object should not have filp anymore.
>
> ..for internal objects.
I mean the struct drm_gem_object should not include a struct file *filp
anymore as not all subclasses are shmem based. And where people want to
use it, it raises more questions than answers.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915/gem: Defer obj->base.resv fini until RCU callback
Chris Wilson writes:
> Quoting Mika Kuoppala (2019-07-04 14:53:09)
>> Chris Wilson writes:
>>
>> > Since reservation_object_fini() does an immediate free, rather than
>> > kfree_rcu as normal, we have to delay the release until after the RCU
>> > grace period has elapsed (i.e. from the rcu cleanup callback) so that we
>> > can rely on the RCU protected access to the fences while the object is a
>> > zombie.
>> >
>> > i915_gem_busy_ioctl relies on having an RCU barrier to protect the
>> > reservation in order to avoid having to take a reference and strong
>> > memory barriers.
>>
>> Ok so for gem busy to be able to operate on a 'to be freed' object
>> we need to keep the reservation object alive?
>
> Yup. It could equally be kept alive if resv_obj_fini used kfree_rcu()
> instead, but we already need an RCU barrier for our object lookup so
> might as well use one stone for both birds.
>
>> > Fixes: c03467ba40f7 ("drm/i915/gem: Free pages before rcu-freeing the
>> > object")
>> > Testcase: igt/gem_busy/close-race
>> > Signed-off-by: Chris Wilson
>> > Cc: Matthew Auld
>> > ---
>> > drivers/gpu/drm/i915/gem/i915_gem_object.c | 3 ++-
>> > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 7 +++
>> > 2 files changed, 9 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> > b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> > index d3e96f09c6b7..0dced4a20e20 100644
>> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> > @@ -152,6 +152,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head
>> > *head)
>> > container_of(head, typeof(*obj), rcu);
>> > struct drm_i915_private *i915 = to_i915(obj->base.dev);
>> >
>> > + reservation_object_fini(&obj->base._resv);
>> > i915_gem_object_free(obj);
>> >
>> > GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
>> > @@ -198,7 +199,7 @@ static void __i915_gem_free_objects(struct
>> > drm_i915_private *i915,
>> > if (obj->base.import_attach)
>> > drm_prime_gem_destroy(&obj->base, NULL);
>> >
>> > - drm_gem_object_release(&obj->base);
>> > + drm_gem_free_mmap_offset(&obj->base);
>> >
>> > /* But keep the pointer alive for RCU-protected lookups */
>> > call_rcu(&obj->rcu, __i915_gem_free_object_rcu);
>> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> > index 19d9ecdb2894..d2a1158868e7 100644
>> > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> > @@ -414,6 +414,11 @@ shmem_pwrite(struct drm_i915_gem_object *obj,
>> > return 0;
>> > }
>> >
>> > +static void shmem_release(struct drm_i915_gem_object *obj)
>> > +{
>> > + fput(obj->base.filp);
>>
>> We lose the check for filp existence. But as internal
>> ops have their own mechanics, we should always have the filp.
>
> Exactly. drm_gem_object should not have filp anymore.
..for internal objects.
>
>> We lose a warn for dma_buf existence tho.
>
> Good. Let me hand you a tiny violin ;)
Let's see how it plays out.
Reviewed-by: Mika Kuoppala
> -Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915/gem: Defer obj->base.resv fini until RCU callback
Quoting Mika Kuoppala (2019-07-04 14:53:09)
> Chris Wilson writes:
>
> > Since reservation_object_fini() does an immediate free, rather than
> > kfree_rcu as normal, we have to delay the release until after the RCU
> > grace period has elapsed (i.e. from the rcu cleanup callback) so that we
> > can rely on the RCU protected access to the fences while the object is a
> > zombie.
> >
> > i915_gem_busy_ioctl relies on having an RCU barrier to protect the
> > reservation in order to avoid having to take a reference and strong
> > memory barriers.
>
> Ok so for gem busy to be able to operate on a 'to be freed' object
> we need to keep the reservation object alive?
Yup. It could equally be kept alive if resv_obj_fini used kfree_rcu()
instead, but we already need an RCU barrier for our object lookup so
might as well use one stone for both birds.
> > Fixes: c03467ba40f7 ("drm/i915/gem: Free pages before rcu-freeing the
> > object")
> > Testcase: igt/gem_busy/close-race
> > Signed-off-by: Chris Wilson
> > Cc: Matthew Auld
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_object.c | 3 ++-
> > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 7 +++
> > 2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > index d3e96f09c6b7..0dced4a20e20 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > @@ -152,6 +152,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head
> > *head)
> > container_of(head, typeof(*obj), rcu);
> > struct drm_i915_private *i915 = to_i915(obj->base.dev);
> >
> > + reservation_object_fini(&obj->base._resv);
> > i915_gem_object_free(obj);
> >
> > GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
> > @@ -198,7 +199,7 @@ static void __i915_gem_free_objects(struct
> > drm_i915_private *i915,
> > if (obj->base.import_attach)
> > drm_prime_gem_destroy(&obj->base, NULL);
> >
> > - drm_gem_object_release(&obj->base);
> > + drm_gem_free_mmap_offset(&obj->base);
> >
> > /* But keep the pointer alive for RCU-protected lookups */
> > call_rcu(&obj->rcu, __i915_gem_free_object_rcu);
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > index 19d9ecdb2894..d2a1158868e7 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > @@ -414,6 +414,11 @@ shmem_pwrite(struct drm_i915_gem_object *obj,
> > return 0;
> > }
> >
> > +static void shmem_release(struct drm_i915_gem_object *obj)
> > +{
> > + fput(obj->base.filp);
>
> We lose the check for filp existence. But as internal
> ops have their own mechanics, we should always have the filp.
Exactly. drm_gem_object should not have filp anymore.
> We lose a warn for dma_buf existence tho.
Good. Let me hand you a tiny violin ;)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915/gem: Defer obj->base.resv fini until RCU callback
Chris Wilson writes:
> Since reservation_object_fini() does an immediate free, rather than
> kfree_rcu as normal, we have to delay the release until after the RCU
> grace period has elapsed (i.e. from the rcu cleanup callback) so that we
> can rely on the RCU protected access to the fences while the object is a
> zombie.
>
> i915_gem_busy_ioctl relies on having an RCU barrier to protect the
> reservation in order to avoid having to take a reference and strong
> memory barriers.
Ok so for gem busy to be able to operate on a 'to be freed' object
we need to keep the reservation object alive?
>
> Fixes: c03467ba40f7 ("drm/i915/gem: Free pages before rcu-freeing the object")
> Testcase: igt/gem_busy/close-race
> Signed-off-by: Chris Wilson
> Cc: Matthew Auld
> ---
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 3 ++-
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 7 +++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index d3e96f09c6b7..0dced4a20e20 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -152,6 +152,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head
> *head)
> container_of(head, typeof(*obj), rcu);
> struct drm_i915_private *i915 = to_i915(obj->base.dev);
>
> + reservation_object_fini(&obj->base._resv);
> i915_gem_object_free(obj);
>
> GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
> @@ -198,7 +199,7 @@ static void __i915_gem_free_objects(struct
> drm_i915_private *i915,
> if (obj->base.import_attach)
> drm_prime_gem_destroy(&obj->base, NULL);
>
> - drm_gem_object_release(&obj->base);
> + drm_gem_free_mmap_offset(&obj->base);
>
> /* But keep the pointer alive for RCU-protected lookups */
> call_rcu(&obj->rcu, __i915_gem_free_object_rcu);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 19d9ecdb2894..d2a1158868e7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -414,6 +414,11 @@ shmem_pwrite(struct drm_i915_gem_object *obj,
> return 0;
> }
>
> +static void shmem_release(struct drm_i915_gem_object *obj)
> +{
> + fput(obj->base.filp);
We lose the check for filp existence. But as internal
ops have their own mechanics, we should always have the filp.
We lose a warn for dma_buf existence tho.
-Mika
> +}
> +
> const struct drm_i915_gem_object_ops i915_gem_shmem_ops = {
> .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE |
>I915_GEM_OBJECT_IS_SHRINKABLE,
> @@ -424,6 +429,8 @@ const struct drm_i915_gem_object_ops i915_gem_shmem_ops =
> {
> .writeback = shmem_writeback,
>
> .pwrite = shmem_pwrite,
> +
> + .release = shmem_release,
> };
>
> static int create_shmem(struct drm_i915_private *i915,
> --
> 2.20.1
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915/gem: Defer obj->base.resv fini until RCU callback
Since reservation_object_fini() does an immediate free, rather than
kfree_rcu as normal, we have to delay the release until after the RCU
grace period has elapsed (i.e. from the rcu cleanup callback) so that we
can rely on the RCU protected access to the fences while the object is a
zombie.
i915_gem_busy_ioctl relies on having an RCU barrier to protect the
reservation in order to avoid having to take a reference and strong
memory barriers.
Fixes: c03467ba40f7 ("drm/i915/gem: Free pages before rcu-freeing the object")
Testcase: igt/gem_busy/close-race
Signed-off-by: Chris Wilson
Cc: Matthew Auld
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 3 ++-
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 7 +++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index d3e96f09c6b7..0dced4a20e20 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -152,6 +152,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head
*head)
container_of(head, typeof(*obj), rcu);
struct drm_i915_private *i915 = to_i915(obj->base.dev);
+ reservation_object_fini(&obj->base._resv);
i915_gem_object_free(obj);
GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
@@ -198,7 +199,7 @@ static void __i915_gem_free_objects(struct drm_i915_private
*i915,
if (obj->base.import_attach)
drm_prime_gem_destroy(&obj->base, NULL);
- drm_gem_object_release(&obj->base);
+ drm_gem_free_mmap_offset(&obj->base);
/* But keep the pointer alive for RCU-protected lookups */
call_rcu(&obj->rcu, __i915_gem_free_object_rcu);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 19d9ecdb2894..d2a1158868e7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -414,6 +414,11 @@ shmem_pwrite(struct drm_i915_gem_object *obj,
return 0;
}
+static void shmem_release(struct drm_i915_gem_object *obj)
+{
+ fput(obj->base.filp);
+}
+
const struct drm_i915_gem_object_ops i915_gem_shmem_ops = {
.flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE |
I915_GEM_OBJECT_IS_SHRINKABLE,
@@ -424,6 +429,8 @@ const struct drm_i915_gem_object_ops i915_gem_shmem_ops = {
.writeback = shmem_writeback,
.pwrite = shmem_pwrite,
+
+ .release = shmem_release,
};
static int create_shmem(struct drm_i915_private *i915,
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
