Re: [PATCH for Xen 4.5 v2 2/2] xen: serialize vcpu data in sched_rt.c

[George Dunlap <george.dunlap@eu.citrix.com>]

On 10/25/2014 03:16 PM, Meng Xu wrote:
> Move call to rt_update_deadline from _alloc to _insert;
> Add lock in rt_vcpu_remove().
>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>

This describes what the patch does, but not why.

While it's nice to  have a summary of the technical changes the patch  
makes, that's information I can get from the patch itself. What's 
critical is an explanation of *why* you are moving rt_update_deadline 
from _alloc to _insert.

Having the information in the cover letter isn't enough, because that 
probably won't be available to someone going through the git history and 
trying to figure out why you made this change.

  -George

> ---
>   xen/common/sched_rt.c |   12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index b87c95b..f136724 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -508,7 +508,6 @@ static void *
>   rt_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
>   {
>       struct rt_vcpu *svc;
> -    s_time_t now = NOW();
>   
>       /* Allocate per-VCPU info */
>       svc = xzalloc(struct rt_vcpu);
> @@ -526,10 +525,6 @@ rt_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
>       if ( !is_idle_vcpu(vc) )
>           svc->budget = RTDS_DEFAULT_BUDGET;
>   
> -    ASSERT( now >= svc->cur_deadline );
> -
> -    rt_update_deadline(now, svc);
> -
>       return svc;
>   }
>   
> @@ -552,11 +547,15 @@ static void
>   rt_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
>   {
>       struct rt_vcpu *svc = rt_vcpu(vc);
> +    s_time_t now = NOW();
>   
>       /* not addlocate idle vcpu to dom vcpu list */
>       if ( is_idle_vcpu(vc) )
>           return;
>   
> +    if ( now >= svc->cur_deadline )
> +        rt_update_deadline(now, svc);
> +
>       if ( !__vcpu_on_q(svc) && vcpu_runnable(vc) && !vc->is_running )
>           __runq_insert(ops, svc);
>   
> @@ -573,11 +572,14 @@ rt_vcpu_remove(const struct scheduler *ops, struct vcpu *vc)
>   {
>       struct rt_vcpu * const svc = rt_vcpu(vc);
>       struct rt_dom * const sdom = svc->sdom;
> +    spinlock_t *lock;
>   
>       BUG_ON( sdom == NULL );
>   
> +    lock = vcpu_schedule_lock_irq(vc);
>       if ( __vcpu_on_q(svc) )
>           __q_remove(svc);
> +    vcpu_schedule_unlock_irq(lock, vc);
>   
>       if ( !is_idle_vcpu(vc) )
>           list_del_init(&svc->sdom_elem);