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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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 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.

So, in summary, I'd be fine with adding a comment, and I'm ok
respinning for this. This patch is pretty independent from the rest of
the series anyway, so I can easily respin just this one.

Thoughts?

> Re Jan's comment -- you meant to say that spin_lock_irq() is wrong
> and
> spin_lock_irqsave() is unnecessary?  That's probably a bit more
> inference than we want changeset messages to have, ideally. :-)
> 
Yes, that's what I meant, as I replied myself to Jan.

> I wouldn't require a re-spin just for that, but if you are going to
> respin it, I would suggest just dropping the "and hence...".
> 
Ok, if I have to respin, I'll do this. If not, I'm fine with whoever
commits this has to drop that sentence.

Thanks and 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)



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