|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6][RFC]xen: sched: convert RTDS from time to event driven model
On Fri, 2016-02-26 at 00:15 -0500, Tianyang Chen wrote:
> On 2/25/2016 6:31 PM, Dario Faggioli wrote:
> > As far as my opinion goes, this patch is not ready to
> > go in
> > right now (as I said, I've got questions and comments), but its
> > status
> > is way past RFC.
> >
> Oh OK, I had the impression that RFC means request for comments and
> it
> should always be used because indeed, I'm asking for comments.
>
Exactly. Everyone is asking for comments when sending out a patch
series, and that's why it's not necessary to state it with the tag...
it's always the case, so no need to tell it all the times!
And, for the same reason, this means that, when the tag is there,
you're not only asking for comments and/or, if everything is ok,
inclusion, but you're asking for "just some feedback on a draft", and
as I said, we're beyond that phase. :-)
> > Wait, maybe you misunderstood and/or I did not make myself clear
> > enough
> > (in which case, sorry). I never meant to say "always stop the
> > timer".
> > Atually, in one of my last email I said the opposite, and I think
> > that
> > would be the best thing to do.
> >
> > Do you think there is any specific reason why we need to always
> > stop
> > and restart it? If not, I think we can:
> > - have deadline_queue_remove() also return whether the element
> > removed was the first one (i.e., the one the timer was
> > programmed
> > after);
> > - if it was, re-program the timer after the new front of the
> > queue;
> > - if it wasn't, nothing to do.
> >
> > Thoughts?
> >
> It was my thought originally that the timer needs to be re-
> programmed
> only when the top vcpu is taken off. So did you mean when I
> manipulated
> repl_timer->expires manually, the timer should be stopped using
> proper
> timer API? The manipulation is gone now.
>
I know... This is mostly due to my fault commenting on this in two
different emails.
So, basically, yes, I meant that, if you want to fiddle with the timer
--like you where doing with those 'repl_timer->expires = 0'-- you need
to do it properly, with the proper API, locking, etc.
Then, in a subsequent email, I said that you just only need to do that
in a subset of the cases when this function is called.
Of course, the desired result is the combination of the two above
considerations, i.e.:
- only stop/restart the timer when necessary,
- if necessary, do it properly.
> Also, set_timer internally
> disables the timer so I assume it's safe just to call set_timer()
> directly, right?
>
Yes, it is.
> > > @@ -405,22 +500,38 @@ __runq_insert(const struct scheduler *ops,
> > > struct rt_vcpu *svc)
> > >
> > > /* add svc to runq if svc still has budget */
> > > if ( svc->cur_budget > 0 )
> > > - {
> > > - list_for_each(iter, runq)
> > > - {
> > > - struct rt_vcpu * iter_svc = __q_elem(iter);
> > > - if ( svc->cur_deadline <= iter_svc->cur_deadline )
> > > - break;
> > > - }
> > > - list_add_tail(&svc->q_elem, iter);
> > > - }
> > > + deadline_queue_insert(&__q_elem, svc, &svc->q_elem,
> > > runq);
> > > else
> > > {
> > > list_add(&svc->q_elem, &prv->depletedq);
> > > + ASSERT( __vcpu_on_replq(svc) );
> > >
> > So, by doing this, you're telling me that, if the vcpu is added to
> > the
> > depleted queue, there must be a replenishment planned for it (or
> > the
> > ASSERT() would fail).
> >
> > But if we are adding it to the runq, there may or may not be a
> > replenishment planned for it.
> >
> > Is this what we want? Why there must not be a replenishment planned
> > already for a vcpu going into the runq (to happen at its next
> > deadline)?
> >
> It looks like the current code doesn't add a vcpu to the
> replenishment
> queue when vcpu_insert() is called. When the scheduler is
> initialized,
> all the vcpus are added to the replenishment queue after waking up
> from
> sleep. This needs to be changed (add vcpu to replq in vcpu_insert())
> to
> make it consistent in a sense that when rt_vcpu_insert() is called,
> it
> needs to have a corresponding replenishment event queued.
>
> This way the ASSERT() is for both cases in __runq_insert() to
> enforce
> the fact that "when a vcpu is inserted to runq/depletedq, a
> replenishment event is waiting for it".
>
I am ok with this (calling replq_insert() in rt_vcpu_insert()). Not
just doing that unconditionally though, as a vcpu can, e.g., be paused
when the insert_vcpu hook is called, and in that case, I don't think we
want to enqueue the replenishment event, do we?
> > static void
> > rt_context_saved(const struct scheduler *ops, struct vcpu *vc)
> > {
> > struct rt_vcpu *svc = rt_vcpu(vc);
> > spinlock_t *lock = vcpu_schedule_lock_irq(vc);
> >
> > clear_bit(__RTDS_scheduled, &svc->flags);
> > /* not insert idle vcpu to runq */
> > if ( is_idle_vcpu(vc) )
> > goto out;
> >
> > if ( test_and_clear_bit(__RTDS_delayed_runq_add, &svc->flags)
> > &&
> > likely(vcpu_runnable(vc)) )
> > {
> > __runq_insert(ops, svc);
> > runq_tickle(ops, snext);
> > }
> > else
> > __replq_remove(ops, svc);
> > out:
> > vcpu_schedule_unlock_irq(lock, vc);
> > }
> >
> > And, as said above, if you do this, try also removing the
> > __replq_remove() call from rt_vcpu_sleep(), this one should cover
> > for
> > that (and, actually, more than just that!).
> >
> > After all this, check whether you still have the assert in
> > __replq_insert() triggering and let me know
> So after moving the __replq_remove() to rt_context_saved(), the
> assert
> in __replq_insert() still fails when dom0 boots up.
>
Well, maybe removing __replq_remove() from rt_vcpu_sleep() is not
entirely ok, as if we do that we fail to deal with the case of when
(still in rt_vcpu_sleep()), __vcpu_on_q(svc) is true.
So, I'd say, do as I said above wrt rt_context_saved(). For
rt_vcpu_sleep(), you can try something like this:
static void
rt_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
{
struct rt_vcpu * const svc = rt_vcpu(vc);
BUG_ON( is_idle_vcpu(vc) );
SCHED_STAT_CRANK(vcpu_sleep);
if ( curr_on_cpu(vc->processor) == vc )
cpu_raise_softirq(vc->processor, SCHEDULE_SOFTIRQ);
else if ( __vcpu_on_q(svc) )
{
__q_remove(svc);
__replq_remove(svc); <=== *** LOOK HERE ***
}
else if ( svc->flags & RTDS_delayed_runq_add )
clear_bit(__RTDS_delayed_runq_add, &svc->flags);
}
In fact, in both the first and the third case, we go will at some point
pass through rt_context_switch(), and hit the __replq_remove() that I
made you put there.
In the case in the middle, as the vcpu was just queued, and for making
it go to sleep, it is enough to remove it from the runq (or depletedq,
in the case of this scheduler), we won't go through
rt_schedule()-->rt_context_saved(), and hence the __replq_remove()
won't be called.
Sorry for the overlook, can you try this.
That being said...
> (XEN) Assertion '!vcpu_on_replq(svc)' failed at sched_rt.c:524
> (XEN) ----[ Xen-4.7-unstable x86_64 debug=y Tainted: C ]----
>
> (XEN) Xen call trace:
> (XEN) [<ffff82d080128c07>] sched_rt.c#__replq_insert+0x2b/0x64
> (XEN) [<ffff82d08012a072>] sched_rt.c#rt_vcpu_wake+0xf2/0x12c
> (XEN) [<ffff82d08012be2c>] vcpu_wake+0x213/0x3d4
> (XEN) [<ffff82d08012c347>] vcpu_unblock+0x4b/0x4d
> (XEN) [<ffff82d080169cea>] vcpu_kick+0x20/0x6f
> (XEN) [<ffff82d080169d65>] vcpu_mark_events_pending+0x2c/0x2f
> (XEN) [<ffff82d08010762a>]
> event_2l.c#evtchn_2l_set_pending+0xa9/0xb9
> (XEN) [<ffff82d08010822a>] evtchn_send+0x158/0x183
> (XEN) [<ffff82d0801096fc>] do_event_channel_op+0xe21/0x147d
> (XEN) [<ffff82d0802439e2>] lstar_enter+0xe2/0x13c
> (XEN)
>
... This says that the we call __replq_insert() from rt_vcpu_wake() and
find out in there that vcpu_on_replq() is true.
However, in v6 code for rt_vcpu_wake(), I can see this:
+ if( !__vcpu_on_replq(svc) )
+ {
+ __replq_insert(ops, svc);
+ ASSERT( vcpu_runnable(vc) );
+ }
which would make me think that, if vcpu_on_replq() is true, we
shouldn't be calling __replq_insert() in the first place.
So, have you made other changes wrt v6 when trying this?
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
Attachment:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |