[1/3] drm/i915/gvt: Use gtt_lock to protect gtt list

Submitted by Colin Xu on March 31, 2018, 12:56 a.m.

Details

Message ID 20180331005700.4341-2-colin.xu@intel.com
State New
Headers show
Series "Split gvt lock into per-logic locks" ( rev: 1 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Colin Xu March 31, 2018, 12:56 a.m.
From: Pei Zhang <pei.zhang@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   |       |         |    |      |      |        |     |      |
+------------+-------+---------+----+------+------+--------+-----+------+

Signed-off-by: Pei Zhang <pei.zhang@intel.com>
Signed-off-by: Colin Xu <colin.xu@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(-)

Patch hide | download patch | download mbox

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(&gtt->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(&gtt->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 */
 

Comments

On Sat, Mar 31, 2018 at 08:56:58AM +0800, Colin Xu wrote:
> From: Pei Zhang <pei.zhang@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!

> Signed-off-by: Pei Zhang <pei.zhang@intel.com>
> Signed-off-by: Colin Xu <colin.xu@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(&gtt->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(&gtt->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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
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@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@intel.com>
>> Signed-off-by: Colin Xu <colin.xu@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(&gtt->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(&gtt->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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev