Re: [PATCH v7 1/2] xen: export do_block, add a parameter to enable local events delivery

[Keir Fraser <keir.xen@gmail.com>]

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.