[PATCH 0/2] xen/arm: vcpu_yield on WFE

[PATCH 0/2] xen/arm: vcpu_yield on WFE

[Stefano Stabellini]

Hi all,
the patches should be self-explanatory.


Stefano Stabellini (2):
      xen: export do_yield as vcpu_yield
      xen/arm: call vcpu_yield on WFE trap

 xen/arch/arm/traps.c    |    2 +-
 xen/common/schedule.c   |   10 +++++-----
 xen/include/xen/sched.h |    1 +
 3 files changed, 7 insertions(+), 6 deletions(-)

[PATCH 1/2] xen: export do_yield as vcpu_yield

[Stefano Stabellini]

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

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index e9eb0bc..6631dc8 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -795,9 +795,8 @@ static long do_poll(struct sched_poll *sched_poll)
 }
 
 /* Voluntarily yield the processor for this allocation. */
-static long do_yield(void)
+void vcpu_yield(struct vcpu *v)
 {
-    struct vcpu * v=current;
     spinlock_t *lock = vcpu_schedule_lock_irq(v);
 
     SCHED_OP(VCPU2OP(v), yield, v);
@@ -805,7 +804,6 @@ static long do_yield(void)
 
     TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
     raise_softirq(SCHEDULE_SOFTIRQ);
-    return 0;
 }
 
 static void domain_watchdog_timeout(void *data)
@@ -888,7 +886,8 @@ long do_sched_op_compat(int cmd, unsigned long arg)
     {
     case SCHEDOP_yield:
     {
-        ret = do_yield();
+        vcpu_yield(current);
+        ret = 0;
         break;
     }
 
@@ -925,7 +924,8 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     {
     case SCHEDOP_yield:
     {
-        ret = do_yield();
+        vcpu_yield(current);
+        ret = 0;
         break;
     }
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 2f876f5..b99a8d3 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -634,6 +634,7 @@ int  sched_id(void);
 void sched_tick_suspend(void);
 void sched_tick_resume(void);
 void vcpu_wake(struct vcpu *v);
+void vcpu_yield(struct vcpu *v);
 void vcpu_sleep_nosync(struct vcpu *v);
 void vcpu_sleep_sync(struct vcpu *v);
 
-- 
1.7.10.4

Re: [PATCH 1/2] xen: export do_yield as vcpu_yield

[Ian Campbell]

On Wed, 2014-07-23 at 13:05 +0100, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: george.dunlap@eu.citrix.com
> CC: JBeulich@suse.com
> ---
>  xen/common/schedule.c   |   10 +++++-----
>  xen/include/xen/sched.h |    1 +
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index e9eb0bc..6631dc8 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -795,9 +795,8 @@ static long do_poll(struct sched_poll *sched_poll)
>  }
>  
>  /* Voluntarily yield the processor for this allocation. */
> -static long do_yield(void)
> +void vcpu_yield(struct vcpu *v)
>  {
> -    struct vcpu * v=current;

Is it safe to yield another vcpu?

Ian.

Re: [PATCH 1/2] xen: export do_yield as vcpu_yield

[George Dunlap]

On 07/23/2014 01:05 PM, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: george.dunlap@eu.citrix.com
> CC: JBeulich@suse.com
> ---
>   xen/common/schedule.c   |   10 +++++-----
>   xen/include/xen/sched.h |    1 +
>   2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index e9eb0bc..6631dc8 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -795,9 +795,8 @@ static long do_poll(struct sched_poll *sched_poll)
>   }
>   
>   /* Voluntarily yield the processor for this allocation. */
> -static long do_yield(void)
> +void vcpu_yield(struct vcpu *v)

What are you actually trying to do here?  Why do you add a vcpu struct, 
when all the callers (including the one you add in 2/2) just pass current?

>   {
> -    struct vcpu * v=current;
>       spinlock_t *lock = vcpu_schedule_lock_irq(v);
>   
>       SCHED_OP(VCPU2OP(v), yield, v);
> @@ -805,7 +804,6 @@ static long do_yield(void)
>   
>       TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
>       raise_softirq(SCHEDULE_SOFTIRQ);

This is broken: it's still tracing current instead of v, and you're 
raising the schedule softirq on the current cpu rather than on the cpu 
on which v is running.  If v!=current, you have to go through force 
reschedule for safety.

Just rename the function and make it non-static, leaving the arguments 
and return value alone.

  -George

Re: [PATCH 1/2] xen: export do_yield as vcpu_yield

[Stefano Stabellini]

On Wed, 23 Jul 2014, George Dunlap wrote:
> On 07/23/2014 01:05 PM, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > CC: george.dunlap@eu.citrix.com
> > CC: JBeulich@suse.com
> > ---
> >   xen/common/schedule.c   |   10 +++++-----
> >   xen/include/xen/sched.h |    1 +
> >   2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> > index e9eb0bc..6631dc8 100644
> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> > @@ -795,9 +795,8 @@ static long do_poll(struct sched_poll *sched_poll)
> >   }
> >     /* Voluntarily yield the processor for this allocation. */
> > -static long do_yield(void)
> > +void vcpu_yield(struct vcpu *v)
> 
> What are you actually trying to do here?  Why do you add a vcpu struct, when
> all the callers (including the one you add in 2/2) just pass current?

I was just trying to be coherent with the other vcpu_* functions in
sched.h.


> >   {
> > -    struct vcpu * v=current;
> >       spinlock_t *lock = vcpu_schedule_lock_irq(v);
> >         SCHED_OP(VCPU2OP(v), yield, v);
> > @@ -805,7 +804,6 @@ static long do_yield(void)
> >         TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id,
> > current->vcpu_id);
> >       raise_softirq(SCHEDULE_SOFTIRQ);
> 
> This is broken: it's still tracing current instead of v, and you're raising
> the schedule softirq on the current cpu rather than on the cpu on which v is
> running.  If v!=current, you have to go through force reschedule for safety.
> 
> Just rename the function and make it non-static, leaving the arguments and
> return value alone.

OK

Re: [PATCH 1/2] xen: export do_yield as vcpu_yield

[George Dunlap]

On 07/23/2014 02:04 PM, Stefano Stabellini wrote:
> On Wed, 23 Jul 2014, George Dunlap wrote:
>> On 07/23/2014 01:05 PM, Stefano Stabellini wrote:
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> CC: george.dunlap@eu.citrix.com
>>> CC: JBeulich@suse.com
>>> ---
>>>    xen/common/schedule.c   |   10 +++++-----
>>>    xen/include/xen/sched.h |    1 +
>>>    2 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>>> index e9eb0bc..6631dc8 100644
>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -795,9 +795,8 @@ static long do_poll(struct sched_poll *sched_poll)
>>>    }
>>>      /* Voluntarily yield the processor for this allocation. */
>>> -static long do_yield(void)
>>> +void vcpu_yield(struct vcpu *v)
>> What are you actually trying to do here?  Why do you add a vcpu struct, when
>> all the callers (including the one you add in 2/2) just pass current?
> I was just trying to be coherent with the other vcpu_* functions in
> sched.h.

I see.  Yeah, I think even apart from the rest of it, "yield" implies 
"myself"; you can't really yield someone else. :-)

  -George

[PATCH 2/2] xen/arm: call vcpu_yield on WFE trap

[Stefano Stabellini]

No need to call vcpu_force_reschedule, is too expensive.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/traps.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 3dfabd0..8cd06cc 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1805,7 +1805,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         }
         if ( hsr.wfi_wfe.ti ) {
             /* Yield the VCPU for WFE */
-            vcpu_force_reschedule(current);
+            vcpu_yield(current);
         } else {
             /* Block the VCPU for WFI */
             vcpu_block_unless_event_pending(current);
-- 
1.7.10.4

Re: [PATCH 2/2] xen/arm: call vcpu_yield on WFE trap

[George Dunlap]

On Wed, Jul 23, 2014 at 1:05 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> No need to call vcpu_force_reschedule, is too expensive.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/traps.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 3dfabd0..8cd06cc 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1805,7 +1805,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>          }
>          if ( hsr.wfi_wfe.ti ) {
>              /* Yield the VCPU for WFE */
> -            vcpu_force_reschedule(current);
> +            vcpu_yield(current);

Actually, sorry -- why are you wanting to yield here?

At the moment "yield" is only initiated by the guest itself, and the
individual schedulers are notified that the guest has called "yield".
If what you want to do is yield for some other reason, it might be
better to have a slightly separate path for that.

 -George

Re: [PATCH 2/2] xen/arm: call vcpu_yield on WFE trap

[Stefano Stabellini]

On Wed, 23 Jul 2014, George Dunlap wrote:
> On Wed, Jul 23, 2014 at 1:05 PM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > No need to call vcpu_force_reschedule, is too expensive.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  xen/arch/arm/traps.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 3dfabd0..8cd06cc 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1805,7 +1805,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> >          }
> >          if ( hsr.wfi_wfe.ti ) {
> >              /* Yield the VCPU for WFE */
> > -            vcpu_force_reschedule(current);
> > +            vcpu_yield(current);
> 
> Actually, sorry -- why are you wanting to yield here?
> 
> At the moment "yield" is only initiated by the guest itself, and the
> individual schedulers are notified that the guest has called "yield".
> If what you want to do is yield for some other reason, it might be
> better to have a slightly separate path for that.
 
It is a very similar situation: WFE means "wait for event" and it is
used in the implementation of spin_locks, for example.
I think that trapping the instruction and yielding is what we want.

Re: [PATCH 2/2] xen/arm: call vcpu_yield on WFE trap

[George Dunlap]

On 07/23/2014 02:13 PM, Stefano Stabellini wrote:
> On Wed, 23 Jul 2014, George Dunlap wrote:
>> On Wed, Jul 23, 2014 at 1:05 PM, Stefano Stabellini
>> <stefano.stabellini@eu.citrix.com> wrote:
>>> No need to call vcpu_force_reschedule, is too expensive.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> ---
>>>   xen/arch/arm/traps.c |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index 3dfabd0..8cd06cc 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -1805,7 +1805,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>>>           }
>>>           if ( hsr.wfi_wfe.ti ) {
>>>               /* Yield the VCPU for WFE */
>>> -            vcpu_force_reschedule(current);
>>> +            vcpu_yield(current);
>> Actually, sorry -- why are you wanting to yield here?
>>
>> At the moment "yield" is only initiated by the guest itself, and the
>> individual schedulers are notified that the guest has called "yield".
>> If what you want to do is yield for some other reason, it might be
>> better to have a slightly separate path for that.
>   
> It is a very similar situation: WFE means "wait for event" and it is
> used in the implementation of spin_locks, for example.
> I think that trapping the instruction and yielding is what we want.

Right -- sounds good then.

  -George