| Message ID | 1406117103-10584-1-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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);
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(-)