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

[Meng Xu <mengxu@xxxxxxxxxxxxx>]

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.

>
>> When I used the spin_lock_irq(save), I just refered to what credit2
>> scheduler does, but didn't think hard enough about which one has
>> better performance.
>>
> I'm not sure what you mean when you talk about Credit2, as there are no
> timers in there. In any case, it is indeed the case that, if just
> _irq() is safe, we should use it, as it's cheaper.

I mean when I first wrote the RTDS scheduler, I just use
spin_lock_irqsave() instead of spin_lock_irq() without considering the
overhead. At that time, I just refer to credit2 and see how it uses
locks. Since it uses spin_lock_irqsave() in other functions, say
_dom_cntl(), I just use the same function and it worked. ;-) (Well, I
should have thought more about the better choice as what I'm doing
now. :-))

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