[v2,1/2] drm/i915/gvt: Use real time to do timer check

Submitted by Zhipeng Gong on April 3, 2018, 1:24 a.m.

Details

Message ID 1522718665-4136-2-git-send-email-zhipeng.gong@intel.com
State New
Headers show
Series "drm/i915/gvt: fix some scheduler issues" ( rev: 2 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Zhipeng Gong April 3, 2018, 1:24 a.m.
intel_gvt_schedule check timer through a counter and is supposed
to wake up to increase the counter every ms.
In a system with heavy workload, gvt_service_thread can not get
a chance to run right after wake up and will be delayed several
milliseconds. As a result, one hundred counter interval means
several hundred milliseconds in real time.

This patch use real time instead of counter to do timer check.

v2: remove static variable. (Zhenyu)

Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Min He <min.he@intel.com>
---
 drivers/gpu/drm/i915/gvt/sched_policy.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gvt/sched_policy.c b/drivers/gpu/drm/i915/gvt/sched_policy.c
index 75b7bc7..a8b544a 100644
--- a/drivers/gpu/drm/i915/gvt/sched_policy.c
+++ b/drivers/gpu/drm/i915/gvt/sched_policy.c
@@ -66,6 +66,7 @@  struct gvt_sched_data {
 	struct hrtimer timer;
 	unsigned long period;
 	struct list_head lru_runq_head;
+	ktime_t expire_time;
 };
 
 static void vgpu_update_timeslice(struct intel_vgpu *pre_vgpu)
@@ -226,14 +227,17 @@  static void tbs_sched_func(struct gvt_sched_data *sched_data)
 void intel_gvt_schedule(struct intel_gvt *gvt)
 {
 	struct gvt_sched_data *sched_data = gvt->scheduler.sched_data;
-	static uint64_t timer_check;
 
 	mutex_lock(&gvt->lock);
 
 	if (test_and_clear_bit(INTEL_GVT_REQUEST_SCHED,
 				(void *)&gvt->service_request)) {
-		if (!(timer_check++ % GVT_TS_BALANCE_PERIOD_MS))
+		if (ktime_get() >= sched_data->expire_time) {
 			gvt_balance_timeslice(sched_data);
+			sched_data->expire_time = ktime_add_ms(
+				sched_data->expire_time,
+				GVT_TS_BALANCE_PERIOD_MS);
+		}
 	}
 	clear_bit(INTEL_GVT_REQUEST_EVENT_SCHED, (void *)&gvt->service_request);
 

Comments

On 2018.04.03 09:24:24 +0800, Zhipeng Gong wrote:
> intel_gvt_schedule check timer through a counter and is supposed
> to wake up to increase the counter every ms.
> In a system with heavy workload, gvt_service_thread can not get
> a chance to run right after wake up and will be delayed several
> milliseconds. As a result, one hundred counter interval means
> several hundred milliseconds in real time.
> 
> This patch use real time instead of counter to do timer check.
> 
> v2: remove static variable. (Zhenyu)
> 
> Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Min He <min.he@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/sched_policy.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/sched_policy.c b/drivers/gpu/drm/i915/gvt/sched_policy.c
> index 75b7bc7..a8b544a 100644
> --- a/drivers/gpu/drm/i915/gvt/sched_policy.c
> +++ b/drivers/gpu/drm/i915/gvt/sched_policy.c
> @@ -66,6 +66,7 @@ struct gvt_sched_data {
>  	struct hrtimer timer;
>  	unsigned long period;
>  	struct list_head lru_runq_head;
> +	ktime_t expire_time;
>  };
>  
>  static void vgpu_update_timeslice(struct intel_vgpu *pre_vgpu)
> @@ -226,14 +227,17 @@ static void tbs_sched_func(struct gvt_sched_data *sched_data)
>  void intel_gvt_schedule(struct intel_gvt *gvt)
>  {
>  	struct gvt_sched_data *sched_data = gvt->scheduler.sched_data;
> -	static uint64_t timer_check;
>  
>  	mutex_lock(&gvt->lock);
>  
>  	if (test_and_clear_bit(INTEL_GVT_REQUEST_SCHED,
>  				(void *)&gvt->service_request)) {
> -		if (!(timer_check++ % GVT_TS_BALANCE_PERIOD_MS))
> +		if (ktime_get() >= sched_data->expire_time) {
>  			gvt_balance_timeslice(sched_data);
> +			sched_data->expire_time = ktime_add_ms(
> +				sched_data->expire_time,
> +				GVT_TS_BALANCE_PERIOD_MS);
> +		}
>  	}

Use if ktime_after(ktime_get(), expire_time) instead and should initialize expire_time
or we currently just kick in first schedule?

>  	clear_bit(INTEL_GVT_REQUEST_EVENT_SCHED, (void *)&gvt->service_request);
>  
> -- 
> 2.7.4
>
> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Tuesday, April 3, 2018 10:36 AM
> To: Gong, Zhipeng <zhipeng.gong@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org; He, Min <min.he@intel.com>
> Subject: Re: [PATCH v2 1/2] drm/i915/gvt: Use real time to do timer check
> 
> On 2018.04.03 09:24:24 +0800, Zhipeng Gong wrote:
> > intel_gvt_schedule check timer through a counter and is supposed
> > to wake up to increase the counter every ms.
> > In a system with heavy workload, gvt_service_thread can not get
> > a chance to run right after wake up and will be delayed several
> > milliseconds. As a result, one hundred counter interval means
> > several hundred milliseconds in real time.
> >
> > This patch use real time instead of counter to do timer check.
> >
> > v2: remove static variable. (Zhenyu)
> >
> > Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
> > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > Cc: Min He <min.he@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/sched_policy.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/sched_policy.c
> b/drivers/gpu/drm/i915/gvt/sched_policy.c
> > index 75b7bc7..a8b544a 100644
> > --- a/drivers/gpu/drm/i915/gvt/sched_policy.c
> > +++ b/drivers/gpu/drm/i915/gvt/sched_policy.c
> > @@ -66,6 +66,7 @@ struct gvt_sched_data {
> >  	struct hrtimer timer;
> >  	unsigned long period;
> >  	struct list_head lru_runq_head;
> > +	ktime_t expire_time;
> >  };
> >
> >  static void vgpu_update_timeslice(struct intel_vgpu *pre_vgpu)
> > @@ -226,14 +227,17 @@ static void tbs_sched_func(struct gvt_sched_data
> *sched_data)
> >  void intel_gvt_schedule(struct intel_gvt *gvt)
> >  {
> >  	struct gvt_sched_data *sched_data = gvt->scheduler.sched_data;
> > -	static uint64_t timer_check;
> >
> >  	mutex_lock(&gvt->lock);
> >
> >  	if (test_and_clear_bit(INTEL_GVT_REQUEST_SCHED,
> >  				(void *)&gvt->service_request)) {
> > -		if (!(timer_check++ % GVT_TS_BALANCE_PERIOD_MS))
> > +		if (ktime_get() >= sched_data->expire_time) {
> >  			gvt_balance_timeslice(sched_data);
> > +			sched_data->expire_time = ktime_add_ms(
> > +				sched_data->expire_time,
> > +				GVT_TS_BALANCE_PERIOD_MS);
> > +		}
> >  	}
> 
> Use if ktime_after(ktime_get(), expire_time) instead and should initialize
> expire_time
> or we currently just kick in first schedule?
>

ktime_after will introduce 1ms gap if intel_gvt_schedule is called right at 100ms.
I will use !ktime_before instead.
Currently expire_time is inited to 0 by kzalloc in tbs_sched_init, and gvt_balance_timeslice 
is called in first schedule. Is it ok to keep this behavior?

> >  	clear_bit(INTEL_GVT_REQUEST_EVENT_SCHED, (void *)&gvt-
> >service_request);
> >
> > --
> > 2.7.4
> >
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
On 2018.04.03 04:58:10 +0000, Gong, Zhipeng wrote:
> > On 2018.04.03 09:24:24 +0800, Zhipeng Gong wrote:
> > > intel_gvt_schedule check timer through a counter and is supposed
> > > to wake up to increase the counter every ms.
> > > In a system with heavy workload, gvt_service_thread can not get
> > > a chance to run right after wake up and will be delayed several
> > > milliseconds. As a result, one hundred counter interval means
> > > several hundred milliseconds in real time.
> > >
> > > This patch use real time instead of counter to do timer check.
> > >
> > > v2: remove static variable. (Zhenyu)
> > >
> > > Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
> > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > Cc: Min He <min.he@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gvt/sched_policy.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/sched_policy.c
> > b/drivers/gpu/drm/i915/gvt/sched_policy.c
> > > index 75b7bc7..a8b544a 100644
> > > --- a/drivers/gpu/drm/i915/gvt/sched_policy.c
> > > +++ b/drivers/gpu/drm/i915/gvt/sched_policy.c
> > > @@ -66,6 +66,7 @@ struct gvt_sched_data {
> > >  	struct hrtimer timer;
> > >  	unsigned long period;
> > >  	struct list_head lru_runq_head;
> > > +	ktime_t expire_time;
> > >  };
> > >
> > >  static void vgpu_update_timeslice(struct intel_vgpu *pre_vgpu)
> > > @@ -226,14 +227,17 @@ static void tbs_sched_func(struct gvt_sched_data
> > *sched_data)
> > >  void intel_gvt_schedule(struct intel_gvt *gvt)
> > >  {
> > >  	struct gvt_sched_data *sched_data = gvt->scheduler.sched_data;
> > > -	static uint64_t timer_check;
> > >
> > >  	mutex_lock(&gvt->lock);
> > >
> > >  	if (test_and_clear_bit(INTEL_GVT_REQUEST_SCHED,
> > >  				(void *)&gvt->service_request)) {
> > > -		if (!(timer_check++ % GVT_TS_BALANCE_PERIOD_MS))
> > > +		if (ktime_get() >= sched_data->expire_time) {
> > >  			gvt_balance_timeslice(sched_data);
> > > +			sched_data->expire_time = ktime_add_ms(
> > > +				sched_data->expire_time,
> > > +				GVT_TS_BALANCE_PERIOD_MS);
> > > +		}
> > >  	}
> > 
> > Use if ktime_after(ktime_get(), expire_time) instead and should initialize
> > expire_time
> > or we currently just kick in first schedule?
> >
> 
> ktime_after will introduce 1ms gap if intel_gvt_schedule is called right at 100ms.
> I will use !ktime_before instead.

Then that might simply use your current form, and to update expire_time shouldn't use
ktime_add_ms(ktime_get(), expire)?

> Currently expire_time is inited to 0 by kzalloc in tbs_sched_init, and gvt_balance_timeslice 
> is called in first schedule. Is it ok to keep this behavior?
>

ok.
> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Tuesday, April 3, 2018 2:11 PM
> To: Gong, Zhipeng <zhipeng.gong@intel.com>
> Cc: He, Min <min.he@intel.com>; intel-gvt-dev@lists.freedesktop.org
> Subject: Re: [PATCH v2 1/2] drm/i915/gvt: Use real time to do timer check
> 
> On 2018.04.03 04:58:10 +0000, Gong, Zhipeng wrote:
> > > On 2018.04.03 09:24:24 +0800, Zhipeng Gong wrote:
> > > > intel_gvt_schedule check timer through a counter and is supposed
> > > > to wake up to increase the counter every ms.
> > > > In a system with heavy workload, gvt_service_thread can not get
> > > > a chance to run right after wake up and will be delayed several
> > > > milliseconds. As a result, one hundred counter interval means
> > > > several hundred milliseconds in real time.
> > > >
> > > > This patch use real time instead of counter to do timer check.
> > > >
> > > > v2: remove static variable. (Zhenyu)
> > > >
> > > > Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
> > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > > Cc: Min He <min.he@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/gvt/sched_policy.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/sched_policy.c
> > > b/drivers/gpu/drm/i915/gvt/sched_policy.c
> > > > index 75b7bc7..a8b544a 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/sched_policy.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/sched_policy.c
> > > > @@ -66,6 +66,7 @@ struct gvt_sched_data {
> > > >  	struct hrtimer timer;
> > > >  	unsigned long period;
> > > >  	struct list_head lru_runq_head;
> > > > +	ktime_t expire_time;
> > > >  };
> > > >
> > > >  static void vgpu_update_timeslice(struct intel_vgpu *pre_vgpu)
> > > > @@ -226,14 +227,17 @@ static void tbs_sched_func(struct
> gvt_sched_data
> > > *sched_data)
> > > >  void intel_gvt_schedule(struct intel_gvt *gvt)
> > > >  {
> > > >  	struct gvt_sched_data *sched_data = gvt->scheduler.sched_data;
> > > > -	static uint64_t timer_check;
> > > >
> > > >  	mutex_lock(&gvt->lock);
> > > >
> > > >  	if (test_and_clear_bit(INTEL_GVT_REQUEST_SCHED,
> > > >  				(void *)&gvt->service_request)) {
> > > > -		if (!(timer_check++ % GVT_TS_BALANCE_PERIOD_MS))
> > > > +		if (ktime_get() >= sched_data->expire_time) {
> > > >  			gvt_balance_timeslice(sched_data);
> > > > +			sched_data->expire_time = ktime_add_ms(
> > > > +				sched_data->expire_time,
> > > > +				GVT_TS_BALANCE_PERIOD_MS);
> > > > +		}
> > > >  	}
> > >
> > > Use if ktime_after(ktime_get(), expire_time) instead and should initialize
> > > expire_time
> > > or we currently just kick in first schedule?
> > >
> >
> > ktime_after will introduce 1ms gap if intel_gvt_schedule is called right at
> 100ms.
> > I will use !ktime_before instead.
> 
> Then that might simply use your current form, and to update expire_time
> shouldn't use
> ktime_add_ms(ktime_get(), expire)?
> 

You are right, I will correct it. 

> > Currently expire_time is inited to 0 by kzalloc in tbs_sched_init, and
> gvt_balance_timeslice
> > is called in first schedule. Is it ok to keep this behavior?
> >
> 
> ok.
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827