Re: [Xen-devel] [Help] Trigger Watchdog when adding an IPI in vcpu_wake

[Wei Yang <richard.weiyang@xxxxxxxxxx>]

On Mon, Sep 26, 2016 at 12:18:06PM +0200, Dario Faggioli wrote:
>On Sat, 2016-09-24 at 11:39 +0800, Wei Yang wrote:
>> On Thu, Sep 22, 2016 at 11:12:15AM +0200, Dario Faggioli wrote:
>> > _Almost_ correct. However, the problem is more that vcpu_wake() can
>> > happen in response to an IRQ, and when you grab a spinlock in IRQ
>> > context, you need to disable IRQs.
>> > 
>> 
>> Ok, now I have a question to this sentence.
>> 
>> I have checked some posts which says in the interrupt handling, irq
>> would be
>> disabled. Well, this really depends on the implementation.
>> 
>> This means in Xen, irq is still enabled during interrupt handling?
>> Because of
>> this, we have to disable IRQ when we need to grab the spin lock?
>> 
>So, I don't think I'm getting all you're saying and asking.
>
>The situation is like this: currently, vcpu_wake() can be called both
>from inside or outside IRQ context, and both with IRQs enabled or
>disabled.
>
>Given this situation, to be safe, we need to disable IRQs when taking
>whatever spinlock vcpu_wake() (and the functions it calls, of course)
>calls. Since it takes the scheduler's runq lock, this means we need to
>take the lock with IRQ disabled.
>
>And because of that, _everyone_ that takes the schedulr's lock must do
>that with IRQs disabled. If we manage to *not* take the scheduler's
>lock with IRQ disabled in vcpu_wake(), we then can do the same
>everywhere else. But we can't do that with the current structure of the
>code, and that's why we're thinking to defer the actual wakeup --i.e.,
>the actual call to vcpu_wake()-- to a moment when:
> - IRQs are enabled,
> - we don't need to disable them.
>

Dario,

I appreciate your patience and almost get the background and your purpose.

First, let me give my conclusion. By taking the schedule lock with IRQ
enabled, it will improve the parallelism to some extend, while it would be
hard to say the net effect. Need more data from benchmark to verify.

Then, let me describe what I understand. (Maybe not correct :-))

This is all about the SMP, spinlock and IRQ/IRQ context.

Two basic assumptions:
a. all interrupt are the same priority, interrupt could not be preempted by 
others. 
b. in the whole interrupt handler, IRQ is disabled on local processor

Let's assume there is only one processor first,
1. If the spinlock just used in interrupt handler, looks it is save, no race
   condition.
2. While if the spinlock maybe used outside interrupt handler, ok, we need to
   at least disable IRQ when grab the lock outside interrupt handler.
   Otherwise there is deadlock.

Then if there are several CPUs in the system.
1. If the spinlock just used in interrupt handler, would this be still save to
   not disable the IRQ? It looks ok to me.
2. While if the spinlock maybe used outside interrupt handler, ok, we need to
   at least disable IRQ when grab the lock outside interrupt handler.

This leads to the spinlock usage convention: if a spinlock would be grabbed in
interrupt handler(IRQ context), we need to grab it with IRQ disabled _always_.

Then, to achieve your purpose -- grab the spinlock with IRQ enabled, you need
to move all the lock operation outside of the interrupt handler. What you are
doing in the code is to deffer the job to softirq when it is in_irq.

By now, I can understand your first part of your check. If vcpu_wake() is
not running in_irq() we can do the job directly now. But not the second part,
why we need to check the local irq is also enabled? In my mind, we need to
check it is not in_irq() is enough.

Thanks a lot for your time :-)

>This is it. About the following...
>
>> Looks I almost get every thing. Let me do a summary to see whether I
>> have got
>> your words.
>> 
>>    (spinlock usage convention in Xen)  &  (vcpu_wake() may happen in
>> an IRQ)
>> 
>This is ok.
>
>> =>
>> 
>>    (irq all disabled || irq all enabled) & (disable irq on grabbing
>> spinlock)
>> 
>This, I don't get it.
>
>> =>
>> 
>>    should grab schedule lock with IRQ disabled
>> 
>Yes, sounds right.
>
>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)


-- 
Richard Yang\nHelp you, Help me

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