Re: [Xen-devel] [PATCH for 4.7 1/4] xen: sched: avoid spuriously re-enabling IRQs in csched2_switch_sched()

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 04/05/16 16:58, Dario Faggioli wrote:
> On Wed, 2016-05-04 at 16:11 +0100, George Dunlap wrote:
>> On 03/05/16 22:46, Dario Faggioli wrote:
>>>
>>> In fact, interrupts are already disabled when calling
>>> the hook from schedule_cpu_switch(), and hence using
>>> anything different than just spin_lock() is wrong (and
>>> ASSERT()-s in debug builds) or unnecessary.
>>>
>>> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
>> Good catch.  
>>
> Well, much rather my bad introducing this! :-/
> 
>> But would it be better to either 1) add an assert that irqs
>> are disabled, or 2) use spin_lock_irqsave(), just to make sure the
>> two
>> bits of code won't silently go out of sync?
>>
> There's an ASSERT() already, in spin_lock_irq() (asking for IRQs to be
> enabled), which is in fact the one that triggers.
> 
> I introduced the bug because of an oversight when applying the last
> review comments to a previous patch series, and did not spot it because
> --and this is the true mistake, IMO-- I tested the final result only
> with debug=n. This is why I think an ASSERT() is after all not that
> useful here... In fact, if this were tested with debug=y, what's
> already in place would have been enough.
> 
> There are quite a few other similar cases all around scheduling code.
> Some of them have comments, none has ASSERT()-s, and I think that is
> fine.

I'm a bit confused by these few paragraphs, and it makes me wonder if
maybe I wasn't very clear.  By #1, I meant, "Do exactly what is done in
this patch (replace spin_lock_irq() with spin_lock()), but also add to
it an assert to make sure that if irqs ever *stop* being disabled when
calling this, we'll find out".

Is that what you understood me to mean, or did you think I meant,
"Instead of changing to spin_lock(), [do something else]"?

> I also don't like using spin_lock_irqsave() when it is not necessary,
> even if this is not an hot path. In fact, apart from being slower, I
> find it more confusing than helpful, especially when looking at the
> code and trying to reason about whether we can be called with IRQ
> enabled or not.

I agree with this; I mainly included the suggestion as a potential
alternative to making the code robust.

I've checked in patches 2 and 3, btw, so no need to re-send them. :-)

 -George


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