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

[George Dunlap <george.dunlap@citrix.com>]

On 04/05/16 18:21, Dario Faggioli wrote:
> 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.

Well an ASSERT() is sort of like a comment, in that if you see
ASSERT(irqs_disabled()), you know there's no need to save irqs because
they should already disabled.  But it has the advantage that osstest
will be able to "read" it once we get some proper cpupool tests for
osstest. :-)

If we weren't in the feature freeze, I'd definitely favor adding an
ASSERT to credit1.  As it is, I think either way (adding now or waiting
until the 4.8 development window) should be fine.

 -George

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