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 18:05 +0100, George Dunlap wrote:
> On 04/05/16 16:58, Dario Faggioli wrote:
> > On Wed, 2016-05-04 at 16:11 +0100, George Dunlap wrote:
> > 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".
> 
No, you were clear. I wasn't, sorry. :-)

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

instead of another ASSERT, I would prefer to add a comment, like we are
doing in other similar places already:

static void
csched2_deinit_pdata(...)
{
    ...
    /* No need to save IRQs here, they're already disabled */
    spin_lock(&rqd->lock);
   ...
}

And I'm now adding this:

After all, I'm fine with an ASSERT() too, but then I think we should
add one to the same effect to csched_switch_sched() too.

> > 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. :-)
> 
Great, thanks. :-)

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