Xen devel - Apr 2013 - [PATCH v6 1/2] xen: introduce vcpu_block, use it instead of do_block

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/common/schedule.c   |   14 ++++++--------
 xen/include/xen/sched.h |    1 +
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index c1cd3d0..89e4b7f 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -676,11 +676,9 @@ int vcpu_set_affinity(struct vcpu *v, const cpumask_t
*affinity)
     return 0;
 }
 
-/* Block the currently-executing domain until a pertinent event occurs. */
-static long do_block(void)
+/* Block a vcpu until a pertinent event occurs. */
+void vcpu_block(struct vcpu *v)
 {
-    struct vcpu *v = current;
-
     local_event_delivery_enable();
     set_bit(_VPF_blocked, &v->pause_flags);
 
@@ -694,8 +692,6 @@ static long do_block(void)
         TRACE_2D(TRC_SCHED_BLOCK, v->domain->domain_id, v->vcpu_id);
         raise_softirq(SCHEDULE_SOFTIRQ);
     }
-
-    return 0;
 }
 
 static long do_poll(struct sched_poll *sched_poll)
@@ -870,7 +866,8 @@ long do_sched_op_compat(int cmd, unsigned long arg)
 
     case SCHEDOP_block:
     {
-        ret = do_block();
+        vcpu_block(current);
+        ret = 0;
         break;
     }
 
@@ -907,7 +904,8 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case SCHEDOP_block:
     {
-        ret = do_block();
+        vcpu_block(current);
+        ret = 0;
         break;
     }
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index ad971d2..e74f0d7 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -691,6 +691,7 @@ static inline int vcpu_runnable(struct vcpu *v)
 }
 
 void vcpu_unblock(struct vcpu *v);
+void vcpu_block(struct vcpu *v);
 void vcpu_pause(struct vcpu *v);
 void vcpu_pause_nosync(struct vcpu *v);
 void domain_pause(struct domain *d);
-- 
1.7.2.5

On 18/04/2013 19:49, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Why? Patch 2/2 only passes current, and the changes here still leave
vcpu_block() implicitly assuming v==current (through use of local_event_*
calls). You may as well leave this as do_block(void).

 -- Keir
> ---
>  xen/common/schedule.c   |   14 ++++++--------
>  xen/include/xen/sched.h |    1 +
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index c1cd3d0..89e4b7f 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -676,11 +676,9 @@ int vcpu_set_affinity(struct vcpu *v, const cpumask_t
> *affinity)
>      return 0;
>  }
>  
> -/* Block the currently-executing domain until a pertinent event occurs. */
> -static long do_block(void)
> +/* Block a vcpu until a pertinent event occurs. */
> +void vcpu_block(struct vcpu *v)
>  {
> -    struct vcpu *v = current;
> -
>      local_event_delivery_enable();
>      set_bit(_VPF_blocked, &v->pause_flags);
>  
> @@ -694,8 +692,6 @@ static long do_block(void)
>          TRACE_2D(TRC_SCHED_BLOCK, v->domain->domain_id,
v->vcpu_id);
>          raise_softirq(SCHEDULE_SOFTIRQ);
>      }
> -
> -    return 0;
>  }
>  
>  static long do_poll(struct sched_poll *sched_poll)
> @@ -870,7 +866,8 @@ long do_sched_op_compat(int cmd, unsigned long arg)
>  
>      case SCHEDOP_block:
>      {
> -        ret = do_block();
> +        vcpu_block(current);
> +        ret = 0;
>          break;
>      }
>  
> @@ -907,7 +904,8 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void)
> arg)
>  
>      case SCHEDOP_block:
>      {
> -        ret = do_block();
> +        vcpu_block(current);
> +        ret = 0;
>          break;
>      }
>  
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ad971d2..e74f0d7 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -691,6 +691,7 @@ static inline int vcpu_runnable(struct vcpu *v)
>  }
>  
>  void vcpu_unblock(struct vcpu *v);
> +void vcpu_block(struct vcpu *v);
>  void vcpu_pause(struct vcpu *v);
>  void vcpu_pause_nosync(struct vcpu *v);
>  void domain_pause(struct domain *d);

On Thu, 18 Apr 2013, Keir Fraser wrote:
> On 18/04/2013 19:49, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
> wrote:
> 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> 
> Why? Patch 2/2 only passes current, and the changes here still leave
> vcpu_block() implicitly assuming v==current (through use of local_event_*
> calls). You may as well leave this as do_block(void).
> 
Just cosmetics: I was trying to make it look like the corresponding
function of vcpu_unblock.


> > ---
> >  xen/common/schedule.c   |   14 ++++++--------
> >  xen/include/xen/sched.h |    1 +
> >  2 files changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> > index c1cd3d0..89e4b7f 100644
> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> > @@ -676,11 +676,9 @@ int vcpu_set_affinity(struct vcpu *v, const
cpumask_t
> > *affinity)
> >      return 0;
> >  }
> >  
> > -/* Block the currently-executing domain until a pertinent event
occurs. */
> > -static long do_block(void)
> > +/* Block a vcpu until a pertinent event occurs. */
> > +void vcpu_block(struct vcpu *v)
> >  {
> > -    struct vcpu *v = current;
> > -
> >      local_event_delivery_enable();
> >      set_bit(_VPF_blocked, &v->pause_flags);
> >  
> > @@ -694,8 +692,6 @@ static long do_block(void)
> >          TRACE_2D(TRC_SCHED_BLOCK, v->domain->domain_id,
v->vcpu_id);
> >          raise_softirq(SCHEDULE_SOFTIRQ);
> >      }
> > -
> > -    return 0;
> >  }
> >  
> >  static long do_poll(struct sched_poll *sched_poll)
> > @@ -870,7 +866,8 @@ long do_sched_op_compat(int cmd, unsigned long
arg)
> >  
> >      case SCHEDOP_block:
> >      {
> > -        ret = do_block();
> > +        vcpu_block(current);
> > +        ret = 0;
> >          break;
> >      }
> >  
> > @@ -907,7 +904,8 @@ ret_t do_sched_op(int cmd,
XEN_GUEST_HANDLE_PARAM(void)
> > arg)
> >  
> >      case SCHEDOP_block:
> >      {
> > -        ret = do_block();
> > +        vcpu_block(current);
> > +        ret = 0;
> >          break;
> >      }
> >  
> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > index ad971d2..e74f0d7 100644
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -691,6 +691,7 @@ static inline int vcpu_runnable(struct vcpu *v)
> >  }
> >  
> >  void vcpu_unblock(struct vcpu *v);
> > +void vcpu_block(struct vcpu *v);
> >  void vcpu_pause(struct vcpu *v);
> >  void vcpu_pause_nosync(struct vcpu *v);
> >  void domain_pause(struct domain *d);
> 
>

On 19/04/2013 10:34, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
wrote:
> On Thu, 18 Apr 2013, Keir Fraser wrote:
>> On 18/04/2013 19:49, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
>> wrote:
>> 
>>> Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
>> 
>> Why? Patch 2/2 only passes current, and the changes here still leave
>> vcpu_block() implicitly assuming v==current (through use of
local_event_*
>> calls). You may as well leave this as do_block(void).
>> 
> 
> Just cosmetics: I was trying to make it look like the corresponding
> function of vcpu_unblock.
Three acceptable options:
 - Leave it as it is
 - Rename it to vcpu_block_current(void)
 - Rename to vcpu_block(vcpu *v) but ASSERT(v == current) as a pre-condition

I like options 1 & 2 best. I''m okay with option 3.

 -- Keir
> 
>>> ---
>>>  xen/common/schedule.c   |   14 ++++++--------
>>>  xen/include/xen/sched.h |    1 +
>>>  2 files changed, 7 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>>> index c1cd3d0..89e4b7f 100644
>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -676,11 +676,9 @@ int vcpu_set_affinity(struct vcpu *v, const
cpumask_t
>>> *affinity)
>>>      return 0;
>>>  }
>>>  
>>> -/* Block the currently-executing domain until a pertinent event
occurs. */
>>> -static long do_block(void)
>>> +/* Block a vcpu until a pertinent event occurs. */
>>> +void vcpu_block(struct vcpu *v)
>>>  {
>>> -    struct vcpu *v = current;
>>> -
>>>      local_event_delivery_enable();
>>>      set_bit(_VPF_blocked, &v->pause_flags);
>>>  
>>> @@ -694,8 +692,6 @@ static long do_block(void)
>>>          TRACE_2D(TRC_SCHED_BLOCK, v->domain->domain_id,
v->vcpu_id);
>>>          raise_softirq(SCHEDULE_SOFTIRQ);
>>>      }
>>> -
>>> -    return 0;
>>>  }
>>>  
>>>  static long do_poll(struct sched_poll *sched_poll)
>>> @@ -870,7 +866,8 @@ long do_sched_op_compat(int cmd, unsigned long
arg)
>>>  
>>>      case SCHEDOP_block:
>>>      {
>>> -        ret = do_block();
>>> +        vcpu_block(current);
>>> +        ret = 0;
>>>          break;
>>>      }
>>>  
>>> @@ -907,7 +904,8 @@ ret_t do_sched_op(int cmd,
XEN_GUEST_HANDLE_PARAM(void)
>>> arg)
>>>  
>>>      case SCHEDOP_block:
>>>      {
>>> -        ret = do_block();
>>> +        vcpu_block(current);
>>> +        ret = 0;
>>>          break;
>>>      }
>>>  
>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>> index ad971d2..e74f0d7 100644
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -691,6 +691,7 @@ static inline int vcpu_runnable(struct vcpu *v)
>>>  }
>>>  
>>>  void vcpu_unblock(struct vcpu *v);
>>> +void vcpu_block(struct vcpu *v);
>>>  void vcpu_pause(struct vcpu *v);
>>>  void vcpu_pause_nosync(struct vcpu *v);
>>>  void domain_pause(struct domain *d);
>> 
>>

>>> On 19.04.13 at 18:33, Keir Fraser <keir.xen@gmail.com> wrote:
> Three acceptable options:
>  - Leave it as it is
>  - Rename it to vcpu_block_current(void)
>  - Rename to vcpu_block(vcpu *v) but ASSERT(v == current) as a
pre-condition
> 
> I like options 1 & 2 best. I''m okay with option 3.
Yet if you go with 3, I''d prefer the parameter to be named
"curr"
(some time ago we started to slowly transition the tree towards
that naming in places where the specified vCPU has to be current,
and is being passed as argument to functions just because in
general "current" has slightly higher overhead than argument
passing).

Jan