diff mbox

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

Message ID 1462540900.3355.43.camel@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dario Faggioli May 6, 2016, 1:21 p.m. UTC 
On Wed, 2016-05-04 at 18:34 +0100, George Dunlap wrote:
> On 04/05/16 18:21, Dario Faggioli wrote:
> > 
> > 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.
> 
Ok, here you go (inline and attached) the patch with ASSERT()-s both in
Credit2 and Credit1 (despite the freeze, I think it's the best thing to
do, see the changelog).

Thanks and Regards,
Dario
--
commit cbabd44e171d0bd2169f1c7100e69a9e48289980
Author: Dario Faggioli <dario.faggioli@citrix.com>
Date:   Tue Apr 26 18:56:56 2016 +0200

    xen: sched: avoid spuriously re-enabling IRQs in csched2_switch_sched()
    
    interrupts are already disabled when calling the hook
    (from schedule_cpu_switch()), so we must use spin_lock()
    and spin_unlock().
    
    Add an ASSERT(), so we will notice if this code and its
    caller get out of sync with respect to disabling interrupts
    (and add one at the same exact occurrence of this pattern
    in Credit1 too)
    
    Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
    ---
    Cc: George Dunlap <george.dunlap@citrix.com>
    Cc: Wei Liu <wei.liu2@citrix.com>
    --
    Changes from v1:
     * add the ASSERT(), as requested by George
     * add the ASSERT in Credit1 too
    --
    For Wei:
     - the Credit2 spin_lock_irq()-->spin_lock() change needs
       to go in, as it fixes a bug;
     - adding the ASSERT was requested during review;
     - adding the ASSERT in Credit1 is not strictly necessary,
       but imptoves code quality and consistency at zero cost
       and risk, so I think we should just go for it now, instead
       of waitign for 4.8 (it's basically like I'm adding a
       comment!).

Comments

Wei Liu May 6, 2016, 1:48 p.m. UTC | #1 
On Fri, May 06, 2016 at 03:21:40PM +0200, Dario Faggioli wrote:
> On Wed, 2016-05-04 at 18:34 +0100, George Dunlap wrote:
> > On 04/05/16 18:21, Dario Faggioli wrote:
> > > 
> > > 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.
> > 
> Ok, here you go (inline and attached) the patch with ASSERT()-s both in
> Credit2 and Credit1 (despite the freeze, I think it's the best thing to
> do, see the changelog).
> 
> Thanks and Regards,
> Dario
> --
> commit cbabd44e171d0bd2169f1c7100e69a9e48289980
> Author: Dario Faggioli <dario.faggioli@citrix.com>
> Date:   Tue Apr 26 18:56:56 2016 +0200
> 
>     xen: sched: avoid spuriously re-enabling IRQs in csched2_switch_sched()
>     
>     interrupts are already disabled when calling the hook
>     (from schedule_cpu_switch()), so we must use spin_lock()
>     and spin_unlock().
>     
>     Add an ASSERT(), so we will notice if this code and its
>     caller get out of sync with respect to disabling interrupts
>     (and add one at the same exact occurrence of this pattern
>     in Credit1 too)
>     
>     Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
>     ---
>     Cc: George Dunlap <george.dunlap@citrix.com>
>     Cc: Wei Liu <wei.liu2@citrix.com>
>     --
>     Changes from v1:
>      * add the ASSERT(), as requested by George
>      * add the ASSERT in Credit1 too
>     --
>     For Wei:
>      - the Credit2 spin_lock_irq()-->spin_lock() change needs
>        to go in, as it fixes a bug;
>      - adding the ASSERT was requested during review;
>      - adding the ASSERT in Credit1 is not strictly necessary,
>        but imptoves code quality and consistency at zero cost
>        and risk, so I think we should just go for it now, instead
>        of waitign for 4.8 (it's basically like I'm adding a
>        comment!).
> 

Release-acked-by: Wei Liu <wei.liu2@citrix.com>
George Dunlap May 9, 2016, 2:42 p.m. UTC | #2 
On Fri, May 6, 2016 at 2:21 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Wed, 2016-05-04 at 18:34 +0100, George Dunlap wrote:
>> On 04/05/16 18:21, Dario Faggioli wrote:
>> >
>> > 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.
>>
> Ok, here you go (inline and attached) the patch with ASSERT()-s both in
> Credit2 and Credit1 (despite the freeze, I think it's the best thing to
> do, see the changelog).
>
> Thanks and Regards,
> Dario
> --
> commit cbabd44e171d0bd2169f1c7100e69a9e48289980
> Author: Dario Faggioli <dario.faggioli@citrix.com>
> Date:   Tue Apr 26 18:56:56 2016 +0200
>
>     xen: sched: avoid spuriously re-enabling IRQs in csched2_switch_sched()
>
>     interrupts are already disabled when calling the hook
>     (from schedule_cpu_switch()), so we must use spin_lock()
>     and spin_unlock().
>
>     Add an ASSERT(), so we will notice if this code and its
>     caller get out of sync with respect to disabling interrupts
>     (and add one at the same exact occurrence of this pattern
>     in Credit1 too)
>
>     Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

And queued, thanks.

 -George

>     ---
>     Cc: George Dunlap <george.dunlap@citrix.com>
>     Cc: Wei Liu <wei.liu2@citrix.com>
>     --
>     Changes from v1:
>      * add the ASSERT(), as requested by George
>      * add the ASSERT in Credit1 too
>     --
>     For Wei:
>      - the Credit2 spin_lock_irq()-->spin_lock() change needs
>        to go in, as it fixes a bug;
>      - adding the ASSERT was requested during review;
>      - adding the ASSERT in Credit1 is not strictly necessary,
>        but imptoves code quality and consistency at zero cost
>        and risk, so I think we should just go for it now, instead
>        of waitign for 4.8 (it's basically like I'm adding a
>        comment!).
>
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index db4d42a..a38a63d 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -615,6 +615,7 @@ csched_switch_sched(struct scheduler *new_ops, unsigned int cpu,
>       * schedule_cpu_switch()). It actually may or may not be the 'right'
>       * one for this cpu, but that is ok for preventing races.
>       */
> +    ASSERT(!local_irq_is_enabled());
>      spin_lock(&prv->lock);
>      init_pdata(prv, pdata, cpu);
>      spin_unlock(&prv->lock);
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index f3b62ac..f95e509 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -2238,7 +2238,8 @@ csched2_switch_sched(struct scheduler *new_ops, unsigned int cpu,
>       * And owning exactly that one (the lock of the old scheduler of this
>       * cpu) is what is necessary to prevent races.
>       */
> -    spin_lock_irq(&prv->lock);
> +    ASSERT(!local_irq_is_enabled());
> +    spin_lock(&prv->lock);
>
>      idle_vcpu[cpu]->sched_priv = vdata;
>
> @@ -2263,7 +2264,7 @@ csched2_switch_sched(struct scheduler *new_ops, unsigned int cpu,
>      smp_mb();
>      per_cpu(schedule_data, cpu).schedule_lock = &prv->rqd[rqi].lock;
>
> -    spin_unlock_irq(&prv->lock);
> +    spin_unlock(&prv->lock);
>  }
>
>  static void
> --
> <<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@lists.xen.org
> http://lists.xen.org/xen-devel
>
diff mbox

Patch

commit cbabd44e171d0bd2169f1c7100e69a9e48289980
Author: Dario Faggioli <dario.faggioli@citrix.com>
Date:   Tue Apr 26 18:56:56 2016 +0200

    xen: sched: avoid spuriously re-enabling IRQs in csched2_switch_sched()
    
    interrupts are already disabled when calling the hook
    (from schedule_cpu_switch()), so we must use spin_lock()
    and spin_unlock().
    
    Add an ASSERT(), so we will notice if this code and its
    caller get out of sync with respect to disabling interrupts
    (and add one at the same exact occurrence of this pattern
    in Credit1 too)
    
    Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
    ---
    Cc: George Dunlap <george.dunlap@citrix.com>
    Cc: Wei Liu <wei.liu2@citrix.com>
    --
    Changes from v1:
     * add the ASSERT(), as requested by George
     * add the ASSERT in Credit1 too
    --
    For Wei:
     - the Credit2 spin_lock_irq()-->spin_lock() change needs
       to go in, as it fixes a bug;
     - adding the ASSERT was requested during review;
     - adding the ASSERT in Credit1 is not strictly necessary,
       but imptoves code quality and consistency at zero cost
       and risk, so I think we should just go for it now, instead
       of waitign for 4.8 (it's basically like I'm adding a
       comment!).

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index db4d42a..a38a63d 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -615,6 +615,7 @@  csched_switch_sched(struct scheduler *new_ops, unsigned int cpu,
      * schedule_cpu_switch()). It actually may or may not be the 'right'
      * one for this cpu, but that is ok for preventing races.
      */
+    ASSERT(!local_irq_is_enabled());
     spin_lock(&prv->lock);
     init_pdata(prv, pdata, cpu);
     spin_unlock(&prv->lock);
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index f3b62ac..f95e509 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -2238,7 +2238,8 @@  csched2_switch_sched(struct scheduler *new_ops, unsigned int cpu,
      * And owning exactly that one (the lock of the old scheduler of this
      * cpu) is what is necessary to prevent races.
      */
-    spin_lock_irq(&prv->lock);
+    ASSERT(!local_irq_is_enabled());
+    spin_lock(&prv->lock);
 
     idle_vcpu[cpu]->sched_priv = vdata;
 
@@ -2263,7 +2264,7 @@  csched2_switch_sched(struct scheduler *new_ops, unsigned int cpu,
     smp_mb();
     per_cpu(schedule_data, cpu).schedule_lock = &prv->rqd[rqi].lock;
 
-    spin_unlock_irq(&prv->lock);
+    spin_unlock(&prv->lock);
 }
 
 static void