Re: [Xen-devel] [PATCH] xen: sched: avoid races on time values read from NOW()

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 19/05/16 09:11, Dario Faggioli wrote:
> or (even in cases where there is no race, e.g., outside
> of Credit2) avoid using a time sample which may be rather
> old, and hence stale.
> 
> In fact, we should only sample NOW() from _inside_
> the critical region within which the value we read is
> used. If we don't, in case we have to spin for a while
> before entering the region, when actually using it:
> 
>  1) we will use something that, at the veryy least, is
>     not really "now", because of the spinning,
> 
>  2) if someone else sampled NOW() during a critical
>     region protected by the lock we are spinning on,
>     and if we compare the two samples when we get
>     inside our region, our one will be 'earlier',
>     even if we actually arrived later, which is a
>     race.
> 
> In Credit2, we see an instance of 2), in runq_tickle(),
> when it is called by csched2_context_saved() as it samples
> NOW() before acquiring the runq lock. This makes things
> look like the time went backwards, and it confuses the
> algorithm (there's even a d2printk() about it, which would
> trigger all the time, if enabled).
> 
> In RTDS, something similar happens in repl_timer_handler(),
> and there's another instance in schedule() (in generic code),
> so fix these cases too.
> 
> While there, improve csched2_vcpu_wake() and and rt_vcpu_wake()
> a little as well (removing a pointless initialization, and
> moving the sampling a bit closer to its use). These two hunks
> entail no further functional changes.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>

I agree this is a fairly low-risk bugfix that should be considered for 4.7.

 -George

> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
> Cc: Meng Xu <mengxu@xxxxxxxxxxxxx>
> Cc: Wei Liu <wei.liu@xxxxxxxxxx>
> ---
>  xen/common/sched_credit2.c |    4 ++--
>  xen/common/sched_rt.c      |    7 +++++--
>  xen/common/schedule.c      |    4 +++-
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index f95e509..1933ff1 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -1028,7 +1028,7 @@ static void
>  csched2_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
>  {
>      struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
> -    s_time_t now = 0;
> +    s_time_t now;
>  
>      /* Schedule lock should be held at this point. */
>  
> @@ -1085,8 +1085,8 @@ static void
>  csched2_context_saved(const struct scheduler *ops, struct vcpu *vc)
>  {
>      struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
> -    s_time_t now = NOW();
>      spinlock_t *lock = vcpu_schedule_lock_irq(vc);
> +    s_time_t now = NOW();
>  
>      BUG_ON( !is_idle_vcpu(vc) && svc->rqd != RQD(ops, vc->processor));
>  
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index aa3ffd2..0946101 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -1198,7 +1198,7 @@ static void
>  rt_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
>  {
>      struct rt_vcpu * const svc = rt_vcpu(vc);
> -    s_time_t now = NOW();
> +    s_time_t now;
>      bool_t missed;
>  
>      BUG_ON( is_idle_vcpu(vc) );
> @@ -1225,6 +1225,7 @@ rt_vcpu_wake(const struct scheduler *ops, struct vcpu 
> *vc)
>       * If a deadline passed while svc was asleep/blocked, we need new
>       * scheduling parameters (a new deadline and full budget).
>       */
> +    now = NOW();
>  
>      missed = ( now >= svc->cur_deadline );
>      if ( missed )
> @@ -1394,7 +1395,7 @@ rt_dom_cntl(
>   * from the replq and does the actual replenishment.
>   */
>  static void repl_timer_handler(void *data){
> -    s_time_t now = NOW();
> +    s_time_t now;
>      struct scheduler *ops = data;
>      struct rt_private *prv = rt_priv(ops);
>      struct list_head *replq = rt_replq(ops);
> @@ -1406,6 +1407,8 @@ static void repl_timer_handler(void *data){
>  
>      spin_lock_irq(&prv->lock);
>  
> +    now = NOW();
> +
>      /*
>       * Do the replenishment and move replenished vcpus
>       * to the temporary list to tickle.
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 80fea39..5e35310 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1320,7 +1320,7 @@ static void vcpu_periodic_timer_work(struct vcpu *v)
>  static void schedule(void)
>  {
>      struct vcpu          *prev = current, *next = NULL;
> -    s_time_t              now = NOW();
> +    s_time_t              now;
>      struct scheduler     *sched;
>      unsigned long        *tasklet_work = &this_cpu(tasklet_work_to_do);
>      bool_t                tasklet_work_scheduled = 0;
> @@ -1355,6 +1355,8 @@ static void schedule(void)
>  
>      lock = pcpu_schedule_lock_irq(cpu);
>  
> +    now = NOW();
> +
>      stop_timer(&sd->s_timer);
>      
>      /* get policy-specific decision on scheduling... */
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel