Re: [Xen-devel] [PATCH v8]xen: sched: convert RTDS from time to event driven model

[Meng Xu <xumengpanda@xxxxxxxxx>]

On Mon, Mar 14, 2016 at 11:38 AM, Meng Xu <mengxu@xxxxxxxxxxxxx> wrote:
> Hi Dario,
>
> On Mon, Mar 14, 2016 at 7:58 AM, Dario Faggioli
> <dario.faggioli@xxxxxxxxxx> wrote:
>> On Fri, 2016-03-11 at 23:54 -0500, Meng Xu wrote:
>>>
>>> > @@ -1150,6 +1300,101 @@ rt_dom_cntl(
>>> >      return rc;
>>> >  }
>>> >
>>> > +/*
>>> > + * The replenishment timer handler picks vcpus
>>> > + * from the replq and does the actual replenishment.
>>> > + */
>>> > +static void repl_handler(void *data){
>>> > +    unsigned long flags;
>>> > +    s_time_t now = NOW();
>>> > +    struct scheduler *ops = data;
>>> > +    struct rt_private *prv = rt_priv(ops);
>>> > +    struct list_head *replq = rt_replq(ops);
>>> > +    struct list_head *runq = rt_runq(ops);
>>> > +    struct timer *repl_timer = prv->repl_timer;
>>> > +    struct list_head *iter, *tmp;
>>> > +    struct list_head tmp_replq;
>>> > +    struct rt_vcpu *svc = NULL;
>>> > +
>>> > +    spin_lock_irqsave(&prv->lock, flags);
>>> Hmm, I haven't thought hard about the choice between
>>> spin_lock_irqsave() and spin_lock_irq(), before.
>>>
>>> Hi Dario,
>>> Is it better to use spin_lock_irqsave() or spin_lock_irq() at this
>>> place?
>>>
>> Just very quickly about this (I'll comment about the rest of the patch
>> later).
>>
>>> I'm not quite sure if the handler can be called in an interrupt
>>> disabled context? Can it?
>>>
>> I recommend looking at what happens inside init_timer(), i.e., where a
>> pointer to this function is stashed. Then, still in xen/common/timer.c,
>> check where this (and, in general, a timer handling function provided
>> to timer_init()) is actually invoked.
>>
>> When you'll find that spot, the answer to whether spin_lock_irq() is
>> safe or not in here, will appear quite evident. :-)
>
> Thanks for the pointer! However, it just makes me think that
> spin_lock_irq() can be used.
>
> timer_softirq_action() will call the execute_timer(), which will call
> the handler function.
>
> static void execute_timer(struct timers *ts, struct timer *t)
>
> {
>
>     void (*fn)(void *) = t->function;
>
>     void *data = t->data;
>
>
>     t->status = TIMER_STATUS_inactive;
>
>     list_add(&t->inactive, &ts->inactive);
>
>
>     ts->running = t;
>
>     spin_unlock_irq(&ts->lock);
>
>     (*fn)(data);
>
>     spin_lock_irq(&ts->lock);
>
>     ts->running = NULL;
>
> }
>
> I loooked into the spin_unlock_irq()
>
> void _spin_unlock_irq(spinlock_t *lock)
>
> {
>
>     _spin_unlock(lock);
>
>     local_irq_enable();
>
> }
>
> in which, lock_irq_enable() will set the interrupt flag:
> #define local_irq_enable()      asm volatile ( "sti" : : : "memory" )
>
> So IMHO, the replenishment handler will be called in interrupt handler
> *and* with interrupt enabled.
> The only difference between lock_irq() and lock_irqsave() is that
> lock_irq() can only be called with interrupt enabled. (lock_irq will
> check if the interrupt is enabled before it disable the interrupt.)
> So I think it is safe to use lock_irq() inside replenishment handler,
> unless there is somewhere call this handler *with interrupt disabled*.
>
> OK. I changed the spin_lock_irqsave to spin_lock_irq based on this
> patch. The system works well: system can boot up and will not crash
> after I create a VM
> So I think spin_lock_irq should work...
> Of course, spin_lock_irqsave() is a more safe choice, with several
> extra instruction overhead.
> And in order to be sure _irq works, we need further tests for sure.
>
> I actually checked somewhere else in schedule.c and couldn't find a
> place that the priv->lock is used in an irq context with interrupt
> disabled.
> So I guess maybe we overuse the spin_lock_irqsave() in the scheduler?
>
> What do you think?
>
> I'm ok that we keep using spin_lock_irqsave() for now. But maybe
> later, it will be a better idea to explore if spin_lock_irq() can
> replace all spin_lock_irqsave() in the RTDS scheduler?
>
> BTW, I'm unsure about the impact of such change on ARM right now.
>

I rethink about the choice of replacing spin_lock_irqsave with spin_lock_irq().
If in the future ,we will introduce new locks and there may exit the
situaiton when we want to lock two locks in the same function. In that
case, we won't use spin_lock_irq() but have to use
spin_lock_irqsave(). If we can mix up spin_lock_irq() with
spin_lock_irqsave() in different fucntiosn for the same lock, which I
think we can (right?), we should be fine. Otherwise, we will have to
keep using spin_lock_irqsave().

Thanks,

Meng

-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel