| Message ID | 1432721046-4418-10-git-send-email-fziglio@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27 May 2015 at 20:04, Frediano Ziglio <fziglio@redhat.com> wrote: > qxl_bo structure has two reference counters, one in the GEM object and > another in the TTM object. The GEM object keep a counter to the TTM object > so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was > decremented. The qxl object is fully freed (both GEM and TTM part are cleaned) > when the TTM counter reach zero. > One issue was that surface idr structure has no owning on qxl_bo objects however > it contains a pointer to qxl_bo object. This caused some nasty race condition > for instance qxl_bo object was reaped even after counter was already zero. > This patch fix these races moving main counter (the one used by qxl_bo_(un)ref) > to GEM object which cleanup routine (qxl_gem_object_free) remove the idr pointer > (using qxl_surface_evict) when the counters are still valid. Uggh, but yes, not sure I like this fix for the problem, but if it works, Reviewed-by: Dave Airlie <airlied@redhat.com>
> On 27 May 2015 at 20:04, Frediano Ziglio <fziglio@redhat.com> wrote: > > qxl_bo structure has two reference counters, one in the GEM object and > > another in the TTM object. The GEM object keep a counter to the TTM object > > so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was > > decremented. The qxl object is fully freed (both GEM and TTM part are > > cleaned) > > when the TTM counter reach zero. > > One issue was that surface idr structure has no owning on qxl_bo objects > > however > > it contains a pointer to qxl_bo object. This caused some nasty race > > condition > > for instance qxl_bo object was reaped even after counter was already zero. > > This patch fix these races moving main counter (the one used by > > qxl_bo_(un)ref) > > to GEM object which cleanup routine (qxl_gem_object_free) remove the idr > > pointer > > (using qxl_surface_evict) when the counters are still valid. > > Uggh, but yes, not sure I like this fix for the problem, but if it works, > > Reviewed-by: Dave Airlie <airlied@redhat.com> > Well, the patch does not surely looks very clear but I can assure the problems it fixes are much less clear to understand. Having a pointer to a object the is going to be deleted whenever another thread decide to causes difficult races. I tried to avoid this kind of change and fix the races instead but was a nightmare. My first experimental patch added an additional counter on top of GEM and TTM one as the main counter but at the end was much more complicated and result was similar to move the main counter to GEM. Mainly external references (from userspace and kernel) are pointers to GEM. Pointers to TTM are from memory mapped files. Deleting the spice id after GEM object has no references assure the not owning reference from spice id still refer to a valid object. As user can't retrieve a pointer from a mapping (at most can remap it) so there are no risks counter to GEM is incremented again. Frediano
diff --git a/qxl/qxl_gem.c b/qxl/qxl_gem.c index b96f0c9..d9746e9 100644 --- a/qxl/qxl_gem.c +++ b/qxl/qxl_gem.c @@ -31,9 +31,15 @@ void qxl_gem_object_free(struct drm_gem_object *gobj) { struct qxl_bo *qobj = gem_to_qxl_bo(gobj); + struct qxl_device *qdev; + struct ttm_buffer_object *tbo; - if (qobj) - qxl_bo_unref(&qobj); + qdev = (struct qxl_device *)gobj->dev->dev_private; + + qxl_surface_evict(qdev, qobj, false); + + tbo = &qobj->tbo; + ttm_bo_unref(&tbo); } int qxl_gem_object_create(struct qxl_device *qdev, int size, diff --git a/qxl/qxl_object.c b/qxl/qxl_object.c index cdeaf08..6d6f33d 100644 --- a/qxl/qxl_object.c +++ b/qxl/qxl_object.c @@ -208,19 +208,16 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, void qxl_bo_unref(struct qxl_bo **bo) { - struct ttm_buffer_object *tbo; - if ((*bo) == NULL) return; - tbo = &((*bo)->tbo); - ttm_bo_unref(&tbo); - if (tbo == NULL) - *bo = NULL; + + drm_gem_object_unreference_unlocked(&(*bo)->gem_base); + *bo = NULL; } struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo) { - ttm_bo_reference(&bo->tbo); + drm_gem_object_reference(&bo->gem_base); return bo; }
qxl_bo structure has two reference counters, one in the GEM object and another in the TTM object. The GEM object keep a counter to the TTM object so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was decremented. The qxl object is fully freed (both GEM and TTM part are cleaned) when the TTM counter reach zero. One issue was that surface idr structure has no owning on qxl_bo objects however it contains a pointer to qxl_bo object. This caused some nasty race condition for instance qxl_bo object was reaped even after counter was already zero. This patch fix these races moving main counter (the one used by qxl_bo_(un)ref) to GEM object which cleanup routine (qxl_gem_object_free) remove the idr pointer (using qxl_surface_evict) when the counters are still valid. Signed-off-by: Frediano Ziglio <fziglio@redhat.com> --- qxl/qxl_gem.c | 10 ++++++++-- qxl/qxl_object.c | 11 ++++------- 2 files changed, 12 insertions(+), 9 deletions(-)