[PATCH 1/3] drm/i915/gvt: Use gtt_lock to protect gtt list
Colin Xu
Colin.Xu at intel.com
Thu Apr 5 01:10:00 UTC 2018
On 04/02/2018 04:58 PM, Du, Changbin wrote:
> On Sat, Mar 31, 2018 at 08:56:58AM +0800, Colin Xu wrote:
>> From: Pei Zhang <pei.zhang at intel.com>
>>
>> The patch set splits out 3 small locks from the original big gvt lock:
>> - gtt_lock is used to protect the gvt global gtt oos lists.
>> - vgpu_lock protects per-vGPU data and logic, especially the vGPU
>> trap emulation path.
>> - sched_lock protects gvt scheudler structure, context schedule logic
>> and vGPU's schedule data.
>>
>> GVT gtt contains 2 oos list, and one global mm list. Use a special
>> gvt->gtt_lock to protect those 3 lists.
>>
>> v4:
>> - Rebase to latest gvt-staging.
>> - Add missing protection when access list in clean_spt_oos().
>> - Fix locked recursive call of detach_oos_page when cleanup. (zhenyu)
>> v3: Update to latest code base.
>> v2: use gtt_lock to cover correct scope in ppgtt_allocate_oos_page().
>>
>> Performance comparison on Kabylake platform.
>> - Configuration:
>> Host: Ubuntu 16.04.
>> Guest 1 & 2: Ubuntu 16.04.
>>
>> glmark2 score comparison:
>> - Configuration:
>> Host: glxgears.
>> Guests: glmark2.
>> +--------------------------------+-----------------+
>> | Setup | glmark2 score |
>> +--------------------------------+-----------------+
>> | unified lock, iommu=on | 58~62 (avg. 60) |
>> +--------------------------------+-----------------+
>> | unified lock, iommu=igfx_off | 57~61 (avg. 59) |
>> +--------------------------------+-----------------+
>> | per-logic lock, iommu=on | 60~68 (avg. 64) |
>> +--------------------------------+-----------------+
>> | per-logic lock, iommu=igfx_off | 61~67 (avg. 64) |
>> +--------------------------------+-----------------+
>>
>> lock_stat comparison:
>> - Configuration:
>> Stop lock stat immediately after boot up.
>> Boot 2 VM Guests.
>> Run glmark2 in guests.
>> Start perf lock_stat for 20 seconds and stop again.
>> - Legend: c - contentions; w - waittime-avg
>> +------------+-----------------+-----------+---------------+------------+
>> | | gvt_lock |sched_lock | vgpu_lock | gtt_lock |
>> + lock type; +-----------------+-----------+---------------+------------+
>> | iommu set | c | w | c | w | c | w | c | w |
>> +------------+-------+---------+----+------+------+--------+-----+------+
>> | unified; | 20697 | 839 |N/A | N/A | N/A | N/A | N/A | N/A |
>> | on | | | | | | | | |
>> +------------+-------+---------+----+------+------+--------+-----+------+
>> | unified; | 21838 | 658.15 |N/A | N/A | N/A | N/A | N/A | N/A |
>> | igfx_off | | | | | | | | |
>> +------------+-------+---------+----+------+------+--------+-----+------+
>> | per-logic; | 1553 | 1599.96 |9458|429.97| 5846 | 274.33 | 0 | 0.00 |
>> | on | | | | | | | | |
>> +------------+-------+---------+----+------+------+--------+-----+------+
>> | per-logic; | 1911 | 1678.32 |8335|445.16| 5451 | 244.80 | 0 | 0.00 |
>> | igfx_off | | | | | | | | |
>> +------------+-------+---------+----+------+------+--------+-----+------+
>>
> Colin,
> Seeing above data, seems this new gtt_lock is not necessoray. The scope of
> gtt_lock has already covered by gvt_lock. Because
> intel_vgpu_page_track_handler() has acquired the gvt lock. Besides,
> intel_vgpu_page_track_handler() should acquire vpu_lock instead of gvt_lock.
> Thanks!
Hi Changbin,
Yes as implied by perf data and current threading model, accessing gtt is already
under gvt->lock protection so may not necessary from implementation aspect.
However from modularized aspect by splitting one single lock into per-logic locks,
gtt_lock could still be useful in future, in parallel with vgpu_lock and sched_lock.
So I'll suggest keep gtt_lock in initialize and cleanup functions, remove reduntant
gtt_lock in update functions since it's already locked.
And Yes, intel_vgpu_page_track_handler() should acquire vgpu_lock, thanks for
point out.
>> Signed-off-by: Pei Zhang <pei.zhang at intel.com>
>> Signed-off-by: Colin Xu <colin.xu at intel.com>
>> ---
>> drivers/gpu/drm/i915/gvt/gtt.c | 38 +++++++++++++++++++++++++++-----------
>> drivers/gpu/drm/i915/gvt/gvt.c | 1 +
>> drivers/gpu/drm/i915/gvt/gvt.h | 2 ++
>> 3 files changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>> index 78e55aafc8bc..f78b4b9c10a8 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>> @@ -639,7 +639,7 @@ static void free_spt(struct intel_vgpu_ppgtt_spt *spt)
>> kfree(spt);
>> }
>>
>> -static int detach_oos_page(struct intel_vgpu *vgpu,
>> +static int detach_oos_page_locked(struct intel_vgpu *vgpu,
>> struct intel_vgpu_oos_page *oos_page);
>>
>> static void ppgtt_free_spt(struct intel_vgpu_ppgtt_spt *spt)
>> @@ -653,8 +653,11 @@ static void ppgtt_free_spt(struct intel_vgpu_ppgtt_spt *spt)
>>
>> radix_tree_delete(&spt->vgpu->gtt.spt_tree, spt->shadow_page.mfn);
>>
>> - if (spt->guest_page.oos_page)
>> - detach_oos_page(spt->vgpu, spt->guest_page.oos_page);
>> + if (spt->guest_page.oos_page) {
>> + mutex_lock(&spt->vgpu->gvt->gtt_lock);
>> + detach_oos_page_locked(spt->vgpu, spt->guest_page.oos_page);
>> + mutex_unlock(&spt->vgpu->gvt->gtt_lock);
>> + }
>>
>> intel_vgpu_unregister_page_track(spt->vgpu, spt->guest_page.gfn);
>>
>> @@ -1150,7 +1153,7 @@ static int sync_oos_page(struct intel_vgpu *vgpu,
>> return 0;
>> }
>>
>> -static int detach_oos_page(struct intel_vgpu *vgpu,
>> +static int detach_oos_page_locked(struct intel_vgpu *vgpu,
>> struct intel_vgpu_oos_page *oos_page)
>> {
>> struct intel_gvt *gvt = vgpu->gvt;
>> @@ -1169,7 +1172,7 @@ static int detach_oos_page(struct intel_vgpu *vgpu,
>> return 0;
>> }
>>
>> -static int attach_oos_page(struct intel_vgpu_oos_page *oos_page,
>> +static int attach_oos_page_locked(struct intel_vgpu_oos_page *oos_page,
>> struct intel_vgpu_ppgtt_spt *spt)
>> {
>> struct intel_gvt *gvt = spt->vgpu->gvt;
>> @@ -1216,19 +1219,23 @@ static int ppgtt_allocate_oos_page(struct intel_vgpu_ppgtt_spt *spt)
>>
>> WARN(oos_page, "shadow PPGTT page has already has a oos page\n");
>>
>> + mutex_lock(&gvt->gtt_lock);
>> if (list_empty(>t->oos_page_free_list_head)) {
>> oos_page = container_of(gtt->oos_page_use_list_head.next,
>> struct intel_vgpu_oos_page, list);
>> ret = ppgtt_set_guest_page_sync(oos_page->spt);
>> if (ret)
>> - return ret;
>> - ret = detach_oos_page(spt->vgpu, oos_page);
>> + goto out;
>> + ret = detach_oos_page_locked(spt->vgpu, oos_page);
>> if (ret)
>> - return ret;
>> + goto out;
>> } else
>> oos_page = container_of(gtt->oos_page_free_list_head.next,
>> struct intel_vgpu_oos_page, list);
>> - return attach_oos_page(oos_page, spt);
>> + ret = attach_oos_page_locked(oos_page, spt);
>> +out:
>> + mutex_unlock(&gvt->gtt_lock);
>> + return ret;
>> }
>>
>> static int ppgtt_set_guest_page_oos(struct intel_vgpu_ppgtt_spt *spt)
>> @@ -1653,8 +1660,10 @@ int intel_vgpu_pin_mm(struct intel_vgpu_mm *mm)
>> if (ret)
>> return ret;
>>
>> + mutex_lock(&mm->vgpu->gvt->gtt_lock);
>> list_move_tail(&mm->ppgtt_mm.lru_list,
>> &mm->vgpu->gvt->gtt.ppgtt_mm_lru_list_head);
>> + mutex_unlock(&mm->vgpu->gvt->gtt_lock);
>>
>> }
>>
>> @@ -1663,9 +1672,11 @@ int intel_vgpu_pin_mm(struct intel_vgpu_mm *mm)
>>
>> static int reclaim_one_ppgtt_mm(struct intel_gvt *gvt)
>> {
>> + int ret = 0;
>> struct intel_vgpu_mm *mm;
>> struct list_head *pos, *n;
>>
>> + mutex_lock(&gvt->gtt_lock);
>> list_for_each_safe(pos, n, &gvt->gtt.ppgtt_mm_lru_list_head) {
>> mm = container_of(pos, struct intel_vgpu_mm, ppgtt_mm.lru_list);
>>
>> @@ -1674,9 +1685,11 @@ static int reclaim_one_ppgtt_mm(struct intel_gvt *gvt)
>>
>> list_del_init(&mm->ppgtt_mm.lru_list);
>> invalidate_ppgtt_mm(mm);
>> - return 1;
>> + ret = 1;
>> }
>> - return 0;
>> + mutex_unlock(&gvt->gtt_lock);
>> +
>> + return ret;
>> }
>>
>> /*
>> @@ -2109,6 +2122,8 @@ static void clean_spt_oos(struct intel_gvt *gvt)
>> struct list_head *pos, *n;
>> struct intel_vgpu_oos_page *oos_page;
>>
>> + mutex_lock(&gvt->gtt_lock);
>> +
>> WARN(!list_empty(>t->oos_page_use_list_head),
>> "someone is still using oos page\n");
>>
>> @@ -2117,6 +2132,7 @@ static void clean_spt_oos(struct intel_gvt *gvt)
>> list_del(&oos_page->list);
>> kfree(oos_page);
>> }
>> + mutex_unlock(&gvt->gtt_lock);
>> }
>>
>> static int setup_spt_oos(struct intel_gvt *gvt)
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
>> index 61bd14fcb649..91c37f115780 100644
>> --- a/drivers/gpu/drm/i915/gvt/gvt.c
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
>> @@ -379,6 +379,7 @@ int intel_gvt_init_device(struct drm_i915_private *dev_priv)
>> idr_init(&gvt->vgpu_idr);
>> spin_lock_init(&gvt->scheduler.mmio_context_lock);
>> mutex_init(&gvt->lock);
>> + mutex_init(&gvt->gtt_lock);
>> gvt->dev_priv = dev_priv;
>>
>> init_device_info(gvt);
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>> index efacd8abbedc..01fe294eab74 100644
>> --- a/drivers/gpu/drm/i915/gvt/gvt.h
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>> @@ -295,6 +295,8 @@ struct intel_vgpu_type {
>>
>> struct intel_gvt {
>> struct mutex lock;
>> + struct mutex gtt_lock;
>> +
>> struct drm_i915_private *dev_priv;
>> struct idr vgpu_idr; /* vGPU IDR pool */
>>
>> --
>> 2.16.3
>>
>> _______________________________________________
>> intel-gvt-dev mailing list
>> intel-gvt-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
--
Best Regards,
Colin Xu
More information about the intel-gvt-dev
mailing list