Sanity check input and serialize vcpu data in sched_rt.c

Sanity check input and serialize vcpu data in sched_rt.c

[Meng Xu]

These two patches are to solve the issues found by Jan Beulich at http://lists.xen.org/archives/html/xen-devel/2014-09/msg03554.html.

The solution is summarized by Dario Faggioli at http://lists.xen.org/archives/html/xen-devel/2014-09/msg03603.html.

Here is the solution:
     - sanity checking input params in rt_dom_cntl()
     - serialize rt_dom_cntl() itself against the global lock
     - move the call to rt_update_deadline() from _alloc to _insert

[PATCH for Xen 4.5 v2 1/2] xen: sanity check input and serialization
[PATCH for Xen 4.5 v2 2/2] xen: serialize vcpu data in sched_rt.c

Meng

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

[PATCH for Xen 4.5 v2 1/2] xen: sanity check input and serialization in sched_rt.c

[Meng Xu]

Sanity check input params in rt_dom_cntl();
Serialize rt_dom_cntl() against the global lock.

Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
---
 xen/common/sched_rt.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 6c8faeb..b87c95b 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1042,9 +1042,11 @@ rt_dom_cntl(
     struct domain *d,
     struct xen_domctl_scheduler_op *op)
 {
+    struct rt_private *prv = rt_priv(ops);
     struct rt_dom * const sdom = rt_dom(d);
     struct rt_vcpu *svc;
     struct list_head *iter;
+    unsigned long flags;
     int rc = 0;
 
     switch ( op->cmd )
@@ -1055,12 +1057,19 @@ rt_dom_cntl(
         op->u.rtds.budget = svc->budget / MICROSECS(1);
         break;
     case XEN_DOMCTL_SCHEDOP_putinfo:
+        if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
+        {
+            rc = -EINVAL;
+            break;
+        }
+        spin_lock_irqsave(&prv->lock, flags);
         list_for_each( iter, &sdom->vcpu )
         {
             struct rt_vcpu * svc = list_entry(iter, struct rt_vcpu, sdom_elem);
             svc->period = MICROSECS(op->u.rtds.period); /* transfer to nanosec */
             svc->budget = MICROSECS(op->u.rtds.budget);
         }
+        spin_unlock_irqrestore(&prv->lock, flags);
         break;
     }
 
-- 
1.7.9.5

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

[Meng Xu]

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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 648 bytes --]

[I modified the Cc list a bit, by adding Konrad, as release manager, and
removing some toolstack people]

On Sat, 2014-10-25 at 10:16 -0400, 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>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH for Xen 4.5 v2 1/2] xen: sanity check input and serialization in sched_rt.c

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2421 bytes --]

[Ditto, about the Cc list]

On Sat, 2014-10-25 at 10:16 -0400, Meng Xu wrote:
> Sanity check input params in rt_dom_cntl();
> Serialize rt_dom_cntl() against the global lock.
> 
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Just a note. See below.

> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 6c8faeb..b87c95b 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -1042,9 +1042,11 @@ rt_dom_cntl(
>      struct domain *d,
>      struct xen_domctl_scheduler_op *op)
>  {
> +    struct rt_private *prv = rt_priv(ops);
>      struct rt_dom * const sdom = rt_dom(d);
>      struct rt_vcpu *svc;
>      struct list_head *iter;
> +    unsigned long flags;
>      int rc = 0;
>  
>      switch ( op->cmd )
> @@ -1055,12 +1057,19 @@ rt_dom_cntl(
>          op->u.rtds.budget = svc->budget / MICROSECS(1);
>          break;
>      case XEN_DOMCTL_SCHEDOP_putinfo:
> +        if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        spin_lock_irqsave(&prv->lock, flags);
>          list_for_each( iter, &sdom->vcpu )
>          {
>              struct rt_vcpu * svc = list_entry(iter, struct rt_vcpu, sdom_elem);
>              svc->period = MICROSECS(op->u.rtds.period); /* transfer to nanosec */
>              svc->budget = MICROSECS(op->u.rtds.budget);
>          }
> +        spin_unlock_irqrestore(&prv->lock, flags);
>
Thinking more about this, I actually prefer the approach where both the
get and set side are properly serialized. Races are very unlikely, and
probably not that harmful, but I don't see why leaving room for them,
and it looks more consistent to me that way.

I appreciate why you've done it this way in this patch, and I am fine
with it. As said, it's not a huge deal, and this code is experimental,
so we can always change it later.

I just wanted to let you know that, if you end up resending the patches
for some reasons, I'd personally go for the fully locked approach.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Sanity check input and serialize vcpu data in sched_rt.c

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1994 bytes --]

On Sat, 2014-10-25 at 10:16 -0400, Meng Xu wrote:
> These two patches are to solve the issues found by Jan Beulich at http://lists.xen.org/archives/html/xen-devel/2014-09/msg03554.html.
> 
> The solution is summarized by Dario Faggioli at http://lists.xen.org/archives/html/xen-devel/2014-09/msg03603.html.
> 
> Here is the solution:
>      - sanity checking input params in rt_dom_cntl()
>      - serialize rt_dom_cntl() itself against the global lock
>      - move the call to rt_update_deadline() from _alloc to _insert
> 
Ok, thanks Meng for the patches, and for this summary.

I've already reviewed the patches, and they look fine to me. If I can
add a few things about the submission:
 - threading is ok this time, good job with that :-D
 - cover letter subject summarizes properly the series content, but 
   should contain the [PATCH xxx xxx] prefix as regular patches, as if
   it were patch 0 of the series
 - since this is v2, it is really useful to include, in each patch, a
   quick summary of what changed wrt previous version. You usually do it
   in the changelog of each patch itself, after a "---" mark, below the
   Signed-off and similar tags

Finally, since we're in freeze, we should 'convince' Konrad that these
patches really need to be merged right now, instead of waiting for 4.6.

See Konrad's development update emails for more details. About that,
Konrad, my take is as follows:
 - this is a bugfix, so, always a good one to have :-)
 - this only touches the new scheduler's code, with basically zero
   chances of causing issues to others
 - the new scheduler is marked as experimental

So, yes, I think these patches should be considered for 4.5

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Sanity check input and serialize vcpu data in sched_rt.c

[Meng Xu]

2014-10-29 6:06 GMT-04:00 Dario Faggioli <dario.faggioli@citrix.com>:
> On Sat, 2014-10-25 at 10:16 -0400, Meng Xu wrote:
>> These two patches are to solve the issues found by Jan Beulich at http://lists.xen.org/archives/html/xen-devel/2014-09/msg03554.html.
>>
>> The solution is summarized by Dario Faggioli at http://lists.xen.org/archives/html/xen-devel/2014-09/msg03603.html.
>>
>> Here is the solution:
>>      - sanity checking input params in rt_dom_cntl()
>>      - serialize rt_dom_cntl() itself against the global lock
>>      - move the call to rt_update_deadline() from _alloc to _insert
>>
> Ok, thanks Meng for the patches, and for this summary.

Thank you very much for your comments!

>
> I've already reviewed the patches, and they look fine to me. If I can
> add a few things about the submission:
>  - threading is ok this time, good job with that :-D
>  - cover letter subject summarizes properly the series content, but
>    should contain the [PATCH xxx xxx] prefix as regular patches, as if
>    it were patch 0 of the series

I will do that next time. I used the git send-email --compose to
compose the cover letter. Next time I will add the prefix for the
cover letter. :-)

>  - since this is v2, it is really useful to include, in each patch, a
>    quick summary of what changed wrt previous version. You usually do it
>    in the changelog of each patch itself, after a "---" mark, below the
>    Signed-off and similar tags
>

Got it. The version 1 does not split the patch into two smaller
patches and is not properly threaded (I sent the patch as a reply to
your comment in the rtds scheduler patch.). So I labeled this one as
version 2 to distinguish with that.

> Finally, since we're in freeze, we should 'convince' Konrad that these
> patches really need to be merged right now, instead of waiting for 4.6.
>
> See Konrad's development update emails for more details. About that,
> Konrad, my take is as follows:
>  - this is a bugfix, so, always a good one to have :-)
>  - this only touches the new scheduler's code, with basically zero
>    chances of causing issues to others
>  - the new scheduler is marked as experimental

Yes! I totally agree. Thank you very much for your summary of the reasons. :-)

>
> So, yes, I think these patches should be considered for 4.5

Yes. :-)

Thank you very much!

Best,

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

Re: [PATCH for Xen 4.5 v2 1/2] xen: sanity check input and serialization in sched_rt.c

[Meng Xu]

2014-10-29 5:41 GMT-04:00 Dario Faggioli <dario.faggioli@citrix.com>:
> [Ditto, about the Cc list]
>
> On Sat, 2014-10-25 at 10:16 -0400, Meng Xu wrote:
>> Sanity check input params in rt_dom_cntl();
>> Serialize rt_dom_cntl() against the global lock.
>>
>> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
>>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
>
> Just a note. See below.
>
>> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
>> index 6c8faeb..b87c95b 100644
>> --- a/xen/common/sched_rt.c
>> +++ b/xen/common/sched_rt.c
>> @@ -1042,9 +1042,11 @@ rt_dom_cntl(
>>      struct domain *d,
>>      struct xen_domctl_scheduler_op *op)
>>  {
>> +    struct rt_private *prv = rt_priv(ops);
>>      struct rt_dom * const sdom = rt_dom(d);
>>      struct rt_vcpu *svc;
>>      struct list_head *iter;
>> +    unsigned long flags;
>>      int rc = 0;
>>
>>      switch ( op->cmd )
>> @@ -1055,12 +1057,19 @@ rt_dom_cntl(
>>          op->u.rtds.budget = svc->budget / MICROSECS(1);
>>          break;
>>      case XEN_DOMCTL_SCHEDOP_putinfo:
>> +        if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +        spin_lock_irqsave(&prv->lock, flags);
>>          list_for_each( iter, &sdom->vcpu )
>>          {
>>              struct rt_vcpu * svc = list_entry(iter, struct rt_vcpu, sdom_elem);
>>              svc->period = MICROSECS(op->u.rtds.period); /* transfer to nanosec */
>>              svc->budget = MICROSECS(op->u.rtds.budget);
>>          }
>> +        spin_unlock_irqrestore(&prv->lock, flags);
>>
> Thinking more about this, I actually prefer the approach where both the
> get and set side are properly serialized. Races are very unlikely, and
> probably not that harmful, but I don't see why leaving room for them,
> and it looks more consistent to me that way.
>
> I appreciate why you've done it this way in this patch, and I am fine
> with it. As said, it's not a huge deal, and this code is experimental,
> so we can always change it later.
>
> I just wanted to let you know that, if you end up resending the patches
> for some reasons, I'd personally go for the fully locked approach.
>

Sure! I can do that if no other objection to that approach of
serializing both the get and set method. :-)


Meng


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

Re: Sanity check input and serialize vcpu data in sched_rt.c

[Konrad Rzeszutek Wilk]

On Wed, Oct 29, 2014 at 10:23:46AM -0400, Meng Xu wrote:
> 2014-10-29 6:06 GMT-04:00 Dario Faggioli <dario.faggioli@citrix.com>:
> > On Sat, 2014-10-25 at 10:16 -0400, Meng Xu wrote:
> >> These two patches are to solve the issues found by Jan Beulich at http://lists.xen.org/archives/html/xen-devel/2014-09/msg03554.html.
> >>
> >> The solution is summarized by Dario Faggioli at http://lists.xen.org/archives/html/xen-devel/2014-09/msg03603.html.
> >>
> >> Here is the solution:
> >>      - sanity checking input params in rt_dom_cntl()
> >>      - serialize rt_dom_cntl() itself against the global lock
> >>      - move the call to rt_update_deadline() from _alloc to _insert
> >>
> > Ok, thanks Meng for the patches, and for this summary.
> 
> Thank you very much for your comments!
> 
> >
> > I've already reviewed the patches, and they look fine to me. If I can
> > add a few things about the submission:
> >  - threading is ok this time, good job with that :-D
> >  - cover letter subject summarizes properly the series content, but
> >    should contain the [PATCH xxx xxx] prefix as regular patches, as if
> >    it were patch 0 of the series
> 
> I will do that next time. I used the git send-email --compose to
> compose the cover letter. Next time I will add the prefix for the
> cover letter. :-)
> 
> >  - since this is v2, it is really useful to include, in each patch, a
> >    quick summary of what changed wrt previous version. You usually do it
> >    in the changelog of each patch itself, after a "---" mark, below the
> >    Signed-off and similar tags
> >
> 
> Got it. The version 1 does not split the patch into two smaller
> patches and is not properly threaded (I sent the patch as a reply to
> your comment in the rtds scheduler patch.). So I labeled this one as
> version 2 to distinguish with that.
> 
> > Finally, since we're in freeze, we should 'convince' Konrad that these
> > patches really need to be merged right now, instead of waiting for 4.6.
> >
> > See Konrad's development update emails for more details. About that,
> > Konrad, my take is as follows:
> >  - this is a bugfix, so, always a good one to have :-)
> >  - this only touches the new scheduler's code, with basically zero
> >    chances of causing issues to others
> >  - the new scheduler is marked as experimental
> 
> Yes! I totally agree. Thank you very much for your summary of the reasons. :-)

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> >
> > So, yes, I think these patches should be considered for 4.5
> 
> Yes. :-)
> 
> Thank you very much!
> 
> Best,
> 
> Meng
> -----------
> Meng Xu
> PhD Student in Computer and Information Science
> University of Pennsylvania
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[George Dunlap]

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

Re: [PATCH for Xen 4.5 v2 1/2] xen: sanity check input and serialization in sched_rt.c

[George Dunlap]

On 10/25/2014 03:16 PM, Meng Xu wrote:
> Sanity check input params in rt_dom_cntl();
> Serialize rt_dom_cntl() against the global lock.
>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>

Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

Thanks,
  -George

> ---
>   xen/common/sched_rt.c |    9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 6c8faeb..b87c95b 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -1042,9 +1042,11 @@ rt_dom_cntl(
>       struct domain *d,
>       struct xen_domctl_scheduler_op *op)
>   {
> +    struct rt_private *prv = rt_priv(ops);
>       struct rt_dom * const sdom = rt_dom(d);
>       struct rt_vcpu *svc;
>       struct list_head *iter;
> +    unsigned long flags;
>       int rc = 0;
>   
>       switch ( op->cmd )
> @@ -1055,12 +1057,19 @@ rt_dom_cntl(
>           op->u.rtds.budget = svc->budget / MICROSECS(1);
>           break;
>       case XEN_DOMCTL_SCHEDOP_putinfo:
> +        if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        spin_lock_irqsave(&prv->lock, flags);
>           list_for_each( iter, &sdom->vcpu )
>           {
>               struct rt_vcpu * svc = list_entry(iter, struct rt_vcpu, sdom_elem);
>               svc->period = MICROSECS(op->u.rtds.period); /* transfer to nanosec */
>               svc->budget = MICROSECS(op->u.rtds.budget);
>           }
> +        spin_unlock_irqrestore(&prv->lock, flags);
>           break;
>       }
>   

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

[Meng Xu]

[-- Attachment #1.1: Type: text/plain, Size: 3376 bytes --]

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@eu.citrix.com>:

> 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@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, 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

[-- Attachment #1.2: Type: text/html, Size: 6587 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 962 bytes --]

On Mon, 2014-11-10 at 10:29 -0500, Meng Xu wrote:
> I'm not sure if I should resend the patch just to change the commit
> log and add the reason of why doing this. 
> 
Yes, that is usually quite a good reason, as changelogs are really
important (for the reasons George pointed out already).

Sometimes, maintainers and committers do the edits themselves (after
asking or at least informing the mailing list), but this only happens
for minor and not substantial modifications.

In this case, the most correct thing is that you resend.


> George, if I should resend the patch set, please let me know.
> 
I'm not George but, yes, please, do resend. :-)

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[George Dunlap]

On 11/10/2014 03:36 PM, Dario Faggioli wrote:
> On Mon, 2014-11-10 at 10:29 -0500, Meng Xu wrote:
>> I'm not sure if I should resend the patch just to change the commit
>> log and add the reason of why doing this.
>>
> Yes, that is usually quite a good reason, as changelogs are really
> important (for the reasons George pointed out already).
>
> Sometimes, maintainers and committers do the edits themselves (after
> asking or at least informing the mailing list), but this only happens
> for minor and not substantial modifications.
>
> In this case, the most correct thing is that you resend.
>
>
>> George, if I should resend the patch set, please let me know.
>>
> I'm not George but, yes, please, do resend. :-)

What Dario said. :-)

  -George

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

[George Dunlap]

[-- Attachment #1.1: Type: text/plain, Size: 915 bytes --]

On 11/10/2014 03:29 PM, Meng Xu wrote:
> 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@eu.citrix.com
> <mailto: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;
>
>
> The runq queue lock is not grabbed when  ​rt_update_deadline is called
> in rt_alloc_vdata function, which may cause race condition.

Can you not grab the lock in rt_alloc_vdata?  Or do you think it's just
more efficient to do all the work you need all at once?

(I'm not suggesting a change to the code, I'm just trying to clarify the
motivation for moving the code rather than adding a lock.)

  -George


[-- Attachment #1.2: Type: text/html, Size: 2692 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Meng Xu]

[-- Attachment #1.1: Type: text/plain, Size: 1644 bytes --]

2014-11-10 10:40 GMT-05:00 George Dunlap <george.dunlap@eu.citrix.com>:

>  On 11/10/2014 03:29 PM, Meng Xu wrote:
>
>  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@eu.citrix.com>:
>
>> 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.
>
>
> Can you not grab the lock in rt_alloc_vdata?
>

​Yes. Because when we allocate a rt_vcpu, only one cpu will do that. In
addition, before the rt_vcpu is inserted into the runq, we won't have more
than one cpu operate on this rt_vcpu.​



> Or do you think it's just more efficient to do all the work you need all
> at once?
>

​I think ​the efficiency should be same. Actually, if we don't insert the
rt_vcpu into a runq, we don't need the vcpu's deadline information because
the rt_vcpu is not scheduled by the rtds scheduler. So I think updating the
deadline info when we insert the rt_vcpu into the runq should make more
sense.

Thanks,

Meng


> (I'm not suggesting a change to the code, I'm just trying to clarify the
> motivation for moving the code rather than adding a lock.)​
>



>
>
>  -George
>
>


-- 


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

[-- Attachment #1.2: Type: text/html, Size: 4462 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[George Dunlap]

[-- Attachment #1.1: Type: text/plain, Size: 1274 bytes --]

On 11/10/2014 04:04 PM, Meng Xu wrote:
>
>
> 2014-11-10 10:40 GMT-05:00 George Dunlap <george.dunlap@eu.citrix.com
> <mailto:george.dunlap@eu.citrix.com>>:
>
>     On 11/10/2014 03:29 PM, Meng Xu wrote:
>>     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@eu.citrix.com <mailto: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;
>>
>>
>>     The runq queue lock is not grabbed when  ​rt_update_deadline is
>>     called in rt_alloc_vdata function, which may cause race condition.
>
>     Can you not grab the lock in rt_alloc_vdata?
>
>
> ​Yes. Because when we allocate a rt_vcpu, only one cpu will do that.
> In addition, before the rt_vcpu is inserted into the runq, we won't
> have more than one cpu operate on this rt_vcpu.​

Right, well that's important information to help someone reading the
patch understand what's going on.

Thanks for being flexible. :-)

  -George


[-- Attachment #1.2: Type: text/html, Size: 4458 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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