Xen devel - Apr 2013 - [PATCH v8 1/2] xen: introduce vcpu_block and vcpu_block_enable_events

Rename do_block to vcpu_block.

Move the call to local_event_delivery_enable out of vcpu_block, to a new
function called vcpu_block_enable_events.

Use vcpu_block_enable_events instead of do_block throughout in
schedule.c


Changes in v8:
- rename do_block to vcpu_block;
- move local_event_delivery_enable out of vcpu_block to
vcpu_block_enable_events.

Changes in v7:
- introduce a event_delivery_enable parameter to make the call to
local_event_delivery_enable conditional;
- export do_block as is.

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

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index c1cd3d0..489c23d 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -677,11 +677,10 @@ int vcpu_set_affinity(struct vcpu *v, const cpumask_t
*affinity)
 }
 
 /* Block the currently-executing domain until a pertinent event occurs. */
-static long do_block(void)
+long vcpu_block(void)
 {
     struct vcpu *v = current;
 
-    local_event_delivery_enable();
     set_bit(_VPF_blocked, &v->pause_flags);
 
     /* Check for events /after/ blocking: avoids wakeup waiting race. */
@@ -698,6 +697,12 @@ static long do_block(void)
     return 0;
 }
 
+long vcpu_block_enable_events(void)
+{
+    local_event_delivery_enable();
+    return vcpu_block();
+}
+
 static long do_poll(struct sched_poll *sched_poll)
 {
     struct vcpu   *v = current;
@@ -870,7 +875,7 @@ long do_sched_op_compat(int cmd, unsigned long arg)
 
     case SCHEDOP_block:
     {
-        ret = do_block();
+        ret = vcpu_block_enable_events();
         break;
     }
 
@@ -907,7 +912,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case SCHEDOP_block:
     {
-        ret = do_block();
+        ret = vcpu_block_enable_events();
         break;
     }
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index ad971d2..1347138 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -690,6 +690,8 @@ static inline int vcpu_runnable(struct vcpu *v)
              atomic_read(&v->domain->pause_count));
 }
 
+long vcpu_block(void);
+long vcpu_block_enable_events(void);
 void vcpu_unblock(struct vcpu *v);
 void vcpu_pause(struct vcpu *v);
 void vcpu_pause_nosync(struct vcpu *v);
-- 
1.7.2.5

On 22/04/2013 18:42, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
wrote:
> Rename do_block to vcpu_block.
> 
> Move the call to local_event_delivery_enable out of vcpu_block, to a new
> function called vcpu_block_enable_events.
> 
> Use vcpu_block_enable_events instead of do_block throughout in
> schedule.c
While you''re there, could you make both vcpu_block variants return
void?

 -- Keir
> 
> Changes in v8:
> - rename do_block to vcpu_block;
> - move local_event_delivery_enable out of vcpu_block to
> vcpu_block_enable_events.
> 
> Changes in v7:
> - introduce a event_delivery_enable parameter to make the call to
> local_event_delivery_enable conditional;
> - export do_block as is.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/common/schedule.c   |   13 +++++++++----
>  xen/include/xen/sched.h |    2 ++
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index c1cd3d0..489c23d 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -677,11 +677,10 @@ int vcpu_set_affinity(struct vcpu *v, const cpumask_t
> *affinity)
>  }
>  
>  /* Block the currently-executing domain until a pertinent event occurs. */
> -static long do_block(void)
> +long vcpu_block(void)
>  {
>      struct vcpu *v = current;
>  
> -    local_event_delivery_enable();
>      set_bit(_VPF_blocked, &v->pause_flags);
>  
>      /* Check for events /after/ blocking: avoids wakeup waiting race. */
> @@ -698,6 +697,12 @@ static long do_block(void)
>      return 0;
>  }
>  
> +long vcpu_block_enable_events(void)
> +{
> +    local_event_delivery_enable();
> +    return vcpu_block();
> +}
> +
>  static long do_poll(struct sched_poll *sched_poll)
>  {
>      struct vcpu   *v = current;
> @@ -870,7 +875,7 @@ long do_sched_op_compat(int cmd, unsigned long arg)
>  
>      case SCHEDOP_block:
>      {
> -        ret = do_block();
> +        ret = vcpu_block_enable_events();
>          break;
>      }
>  
> @@ -907,7 +912,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void)
> arg)
>  
>      case SCHEDOP_block:
>      {
> -        ret = do_block();
> +        ret = vcpu_block_enable_events();
>          break;
>      }
>  
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ad971d2..1347138 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -690,6 +690,8 @@ static inline int vcpu_runnable(struct vcpu *v)
>               atomic_read(&v->domain->pause_count));
>  }
>  
> +long vcpu_block(void);
> +long vcpu_block_enable_events(void);
>  void vcpu_unblock(struct vcpu *v);
>  void vcpu_pause(struct vcpu *v);
>  void vcpu_pause_nosync(struct vcpu *v);

>>> On 22.04.13 at 22:00, Keir Fraser <keir.xen@gmail.com> wrote:
> On 22/04/2013 18:42, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com> wrote:
> 
>> Rename do_block to vcpu_block.
>> 
>> Move the call to local_event_delivery_enable out of vcpu_block, to a
new
>> function called vcpu_block_enable_events.
>> 
>> Use vcpu_block_enable_events instead of do_block throughout in
>> schedule.c
> 
> While you''re there, could you make both vcpu_block variants return
void?
And I don''t see why vcpu_block_enable_events() needs to
become non-static...

Jan

On 23/04/2013 08:23, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 22.04.13 at 22:00, Keir Fraser <keir.xen@gmail.com>
wrote:
>> On 22/04/2013 18:42, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
>> wrote:
>> 
>>> Rename do_block to vcpu_block.
>>> 
>>> Move the call to local_event_delivery_enable out of vcpu_block, to
a new
>>> function called vcpu_block_enable_events.
>>> 
>>> Use vcpu_block_enable_events instead of do_block throughout in
>>> schedule.c
>> 
>> While you''re there, could you make both vcpu_block variants
return void?
> 
> And I don''t see why vcpu_block_enable_events() needs to
> become non-static...
I must admit that I don''t mind, no reason not to make it part of the
API of
the scheduler, even if noone else uses it yet. OTOH I don''t have a very
strong opinion either way.

 -- Keir
> Jan
>

On Mon, 2013-04-22 at 18:42 +0100, Stefano Stabellini
wrote:
> Rename do_block to vcpu_block.
Please can you tweak whatever .gitconfig variable needs tweaking to
thread the various related patches together, it makes life unnecessarily
difficult for reviewers and committers because the patches and their
subthreads don''t stay together. I think you want either
sendemail.thread
= true or formatpatch.thread = true. My config appears to be
sendemail.thread=false (in my .gitconfig) & formatpatch.thread=true (the
default)

Also it is customary to include a 0/N mail to serve as a root for the
thread in threaded mailers, which also keeps them all together. This
would be useful for this reason even if you don''t have much to say and
its only content is "This is v$N of this series".

Ian.
> 
> Move the call to local_event_delivery_enable out of vcpu_block, to a new
> function called vcpu_block_enable_events.
> 
> Use vcpu_block_enable_events instead of do_block throughout in
> schedule.c
> 
> 
> Changes in v8:
> - rename do_block to vcpu_block;
> - move local_event_delivery_enable out of vcpu_block to
> vcpu_block_enable_events.
> 
> Changes in v7:
> - introduce a event_delivery_enable parameter to make the call to
> local_event_delivery_enable conditional;
> - export do_block as is.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/common/schedule.c   |   13 +++++++++----
>  xen/include/xen/sched.h |    2 ++
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index c1cd3d0..489c23d 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -677,11 +677,10 @@ int vcpu_set_affinity(struct vcpu *v, const cpumask_t
*affinity)
>  }
>  
>  /* Block the currently-executing domain until a pertinent event occurs. */
> -static long do_block(void)
> +long vcpu_block(void)
>  {
>      struct vcpu *v = current;
>  
> -    local_event_delivery_enable();
>      set_bit(_VPF_blocked, &v->pause_flags);
>  
>      /* Check for events /after/ blocking: avoids wakeup waiting race. */
> @@ -698,6 +697,12 @@ static long do_block(void)
>      return 0;
>  }
>  
> +long vcpu_block_enable_events(void)
> +{
> +    local_event_delivery_enable();
> +    return vcpu_block();
> +}
> +
>  static long do_poll(struct sched_poll *sched_poll)
>  {
>      struct vcpu   *v = current;
> @@ -870,7 +875,7 @@ long do_sched_op_compat(int cmd, unsigned long arg)
>  
>      case SCHEDOP_block:
>      {
> -        ret = do_block();
> +        ret = vcpu_block_enable_events();
>          break;
>      }
>  
> @@ -907,7 +912,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void)
arg)
>  
>      case SCHEDOP_block:
>      {
> -        ret = do_block();
> +        ret = vcpu_block_enable_events();
>          break;
>      }
>  
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ad971d2..1347138 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -690,6 +690,8 @@ static inline int vcpu_runnable(struct vcpu *v)
>               atomic_read(&v->domain->pause_count));
>  }
>  
> +long vcpu_block(void);
> +long vcpu_block_enable_events(void);
>  void vcpu_unblock(struct vcpu *v);
>  void vcpu_pause(struct vcpu *v);
>  void vcpu_pause_nosync(struct vcpu *v);