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

[Meng Xu <xumengpanda@xxxxxxxxx>]

I'm not sure if I should resend the patch just to change the commit log and add the reason of why doing this.Â

I want to first add the reason. If I should resend the patch set, please let me know.Â

2014-11-10 7:53 GMT-05:00 George Dunlap <george.dunlap@xxxxxxxxxxxxx>:

On 10/25/2014 03:16 PM, Meng Xu wrote:

Move call to rt_update_deadline from _alloc to _insert;

The runq queue lock is not grabbed when Âârt_update_deadline is called inÂrt_alloc_vdata function, which may cause race condition.

Â

Add lock in rt_vcpu_remove().

rt_vcpu_remove

â should grab the runq lock before remove the vcpu from runq; otherwise, race condition may happen.

â

Â

Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>

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, if I should resend the patch set, please let me know.

Thanks,

Mengâ

Â

Â-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);

--

-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

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