Xen devel - Apr 2013 - [PATCH v7 1/2] xen: export do_block, add a parameter to enable local events delivery

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

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index c1cd3d0..1f41619 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -677,11 +677,12 @@ 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)
+int do_block(int event_delivery_enable)
 {
     struct vcpu *v = current;
 
-    local_event_delivery_enable();
+    if ( event_delivery_enable )
+        local_event_delivery_enable();
     set_bit(_VPF_blocked, &v->pause_flags);
 
     /* Check for events /after/ blocking: avoids wakeup waiting race. */
@@ -870,7 +871,7 @@ long do_sched_op_compat(int cmd, unsigned long arg)
 
     case SCHEDOP_block:
     {
-        ret = do_block();
+        ret = do_block(1);
         break;
     }
 
@@ -907,7 +908,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case SCHEDOP_block:
     {
-        ret = do_block();
+        ret = do_block(1);
         break;
     }
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index ad971d2..23d5efd 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -690,6 +690,7 @@ static inline int vcpu_runnable(struct vcpu *v)
              atomic_read(&v->domain->pause_count));
 }
 
+int do_block(int event_delivery_enable);
 void vcpu_unblock(struct vcpu *v);
 void vcpu_pause(struct vcpu *v);
 void vcpu_pause_nosync(struct vcpu *v);
-- 
1.7.2.5

>>> On 19.04.13 at 16:06, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> -static long do_block(void)
> +int do_block(int event_delivery_enable)
In the previous version you didn''t need this new parameter, so
what changed since then?

Also, please use bool_t for boolean parameters.

Jan

On Fri, 19 Apr 2013, Jan Beulich wrote:
> >>> On 19.04.13 at 16:06, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> > -static long do_block(void)
> > +int do_block(int event_delivery_enable)
> 
> In the previous version you didn''t need this new parameter, so
> what changed since then?
I changed the ARM implementation of local_event_delivery_enable: now it
clears PSR_IRQ_MASK (the interrupt flag). That is the correct behavior
for the implementation of the SCHEDOP_block hypercall but it is not the
right behavior for the WFI trap handler. In order to reuse this code, I
had to add the event_delivery_enable parameter.
> Also, please use bool_t for boolean parameters.
OK

On 19/04/2013 17:23, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
wrote:
> On Fri, 19 Apr 2013, Jan Beulich wrote:
>>>>> On 19.04.13 at 16:06, Stefano Stabellini
>>>>> <stefano.stabellini@eu.citrix.com> wrote:
>>> -static long do_block(void)
>>> +int do_block(int event_delivery_enable)
>> 
>> In the previous version you didn''t need this new parameter, so
>> what changed since then?
> 
> I changed the ARM implementation of local_event_delivery_enable: now it
> clears PSR_IRQ_MASK (the interrupt flag). That is the correct behavior
> for the implementation of the SCHEDOP_block hypercall but it is not the
> right behavior for the WFI trap handler. In order to reuse this code, I
> had to add the event_delivery_enable parameter.
Given that basically you need special versions of both local_events_*()
calls in do_block() (in one case you nop it out, and in the other it is a
special case which ignores the irq mask), I think perhaps you are better
with your own version of do_block() rather than code sharing. It''s only
a
small function and it skanks up every caller in common code, and you''re
practically abusing do_block() by trying to play nicely with it.

I would also suggest that _local_events_need_delivery() takes no parameter,
always ignores the mask, and instead push the mask check into
local_events_need_delivery(). Perhaps rename the former to
local_events_need_delivery_ignore_mask() or something.

 -- Keir
>> Also, please use bool_t for boolean parameters.
> 
> OK
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On 19/04/2013 18:12, "Keir Fraser" <keir.xen@gmail.com> wrote:
> On 19/04/2013 17:23, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
> wrote:
> 
>> On Fri, 19 Apr 2013, Jan Beulich wrote:
>>>>>> On 19.04.13 at 16:06, Stefano Stabellini
>>>>>> <stefano.stabellini@eu.citrix.com> wrote:
>>>> -static long do_block(void)
>>>> +int do_block(int event_delivery_enable)
>>> 
>>> In the previous version you didn''t need this new
parameter, so
>>> what changed since then?
>> 
>> I changed the ARM implementation of local_event_delivery_enable: now it
>> clears PSR_IRQ_MASK (the interrupt flag). That is the correct behavior
>> for the implementation of the SCHEDOP_block hypercall but it is not the
>> right behavior for the WFI trap handler. In order to reuse this code, I
>> had to add the event_delivery_enable parameter.
> 
> Given that basically you need special versions of both local_events_*()
> calls in do_block() (in one case you nop it out, and in the other it is a
> special case which ignores the irq mask), I think perhaps you are better
> with your own version of do_block() rather than code sharing. It''s
only a
> small function and it skanks up every caller in common code, and
you''re
> practically abusing do_block() by trying to play nicely with it.
> 
> I would also suggest that _local_events_need_delivery() takes no parameter,
> always ignores the mask, and instead push the mask check into
> local_events_need_delivery(). Perhaps rename the former to
> local_events_need_delivery_ignore_mask() or something.
Both these suggestions by the way are partly driven by my fairly strong
hatred of boolean parameters to functions. I hate those 1/0 true/false
arguments, almost always have to go look up what they mean because
they''re
often the sign of some gross hack or special case ''mode
change'' for a
function. This is arguable of course, my hatred is probably irrational to
some extent, but I my bad-code senses tingle when I see such parameters! :)

 -- Keir
>  -- Keir
> 
>>> Also, please use bool_t for boolean parameters.
>> 
>> OK
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
>

On Fri, 19 Apr 2013, Keir Fraser wrote:
> On 19/04/2013 17:23, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
> wrote:
> 
> > On Fri, 19 Apr 2013, Jan Beulich wrote:
> >>>>> On 19.04.13 at 16:06, Stefano Stabellini
> >>>>> <stefano.stabellini@eu.citrix.com> wrote:
> >>> -static long do_block(void)
> >>> +int do_block(int event_delivery_enable)
> >> 
> >> In the previous version you didn''t need this new
parameter, so
> >> what changed since then?
> > 
> > I changed the ARM implementation of local_event_delivery_enable: now
it
> > clears PSR_IRQ_MASK (the interrupt flag). That is the correct behavior
> > for the implementation of the SCHEDOP_block hypercall but it is not
the
> > right behavior for the WFI trap handler. In order to reuse this code,
I
> > had to add the event_delivery_enable parameter.
> 
> Given that basically you need special versions of both local_events_*()
> calls in do_block() (in one case you nop it out, and in the other it is a
> special case which ignores the irq mask), I think perhaps you are better
> with your own version of do_block() rather than code sharing. It''s
only a
> small function and it skanks up every caller in common code, and
you''re
> practically abusing do_block() by trying to play nicely with it.
It is a small function, but the implementation is not trivial.
What if I do something like:


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


> I would also suggest that _local_events_need_delivery() takes no parameter,
> always ignores the mask, and instead push the mask check into
> local_events_need_delivery(). Perhaps rename the former to
> local_events_need_delivery_ignore_mask() or something.
Yeah, this is a good idea.

On 22/04/2013 11:20, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
wrote:
> On Fri, 19 Apr 2013, Keir Fraser wrote:
>> On 19/04/2013 17:23, "Stefano Stabellini"
<stefano.stabellini@eu.citrix.com>
>> wrote:
>> 
>>> On Fri, 19 Apr 2013, Jan Beulich wrote:
>>>>>>> On 19.04.13 at 16:06, Stefano Stabellini
>>>>>>> <stefano.stabellini@eu.citrix.com> wrote:
>>>>> -static long do_block(void)
>>>>> +int do_block(int event_delivery_enable)
>>>> 
>>>> In the previous version you didn''t need this new
parameter, so
>>>> what changed since then?
>>> 
>>> I changed the ARM implementation of local_event_delivery_enable:
now it
>>> clears PSR_IRQ_MASK (the interrupt flag). That is the correct
behavior
>>> for the implementation of the SCHEDOP_block hypercall but it is not
the
>>> right behavior for the WFI trap handler. In order to reuse this
code, I
>>> had to add the event_delivery_enable parameter.
>> 
>> Given that basically you need special versions of both local_events_*()
>> calls in do_block() (in one case you nop it out, and in the other it is
a
>> special case which ignores the irq mask), I think perhaps you are
better
>> with your own version of do_block() rather than code sharing.
It''s only a
>> small function and it skanks up every caller in common code, and
you''re
>> practically abusing do_block() by trying to play nicely with it.
> 
> It is a small function, but the implementation is not trivial.
> What if I do something like:
Yes, I like this well enough.

 -- Keir
> 
> 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;
>      }
>  
> 
> 
> 
>> I would also suggest that _local_events_need_delivery() takes no
parameter,
>> always ignores the mask, and instead push the mask check into
>> local_events_need_delivery(). Perhaps rename the former to
>> local_events_need_delivery_ignore_mask() or something.
> 
> Yeah, this is a good idea.