xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Dario Faggioli <dario.faggioli@citrix.com>
To: George Dunlap <george.dunlap@citrix.com>, xen-devel@lists.xenproject.org
Cc: George Dunlap <george.dunlap@eu.citrix.com>,
	Tianyang Chen <tiche@seas.upenn.edu>,
	Josh Whitehead <josh.whitehead@dornerworks.com>,
	Meng Xu <mengxu@cis.upenn.edu>,
	Robert VanVossen <robert.vanvossen@dornerworks.com>
Subject: Re: [PATCH 09/16] xen: sched: close potential races when switching scheduler to CPUs
Date: Wed, 6 Apr 2016 18:21:10 +0200	[thread overview]
Message-ID: <1459959670.3166.100.camel@citrix.com> (raw)
In-Reply-To: <1459877839.3166.84.camel@citrix.com>


[-- Attachment #1.1: Type: text/plain, Size: 3636 bytes --]

On Tue, 2016-04-05 at 19:37 +0200, Dario Faggioli wrote:
> On Thu, 2016-03-24 at 12:14 +0000, George Dunlap wrote:
> > On 18/03/16 19:05, Dario Faggioli wrote:
> > So I think there should be no problem with:
> > 1. Grabbing the pcpu schedule lock in schedule_cpu_switch()
> > 2. Grabbing prv->lock in csched2_switch_sched()
> > 3. Setting the per_cpu schedule lock as the very last thing in
> > csched2_switch_sched()
> > 4. Releasing the (old) pcpu schedule lock in schedule_cpu_switch().
> > 
> > What do you think?
> > 
> I think it should work. We'll be doing the scheduler lock
> manipulation
> protected by the old and wrong (as it's the one of another scheduler)
> per-cpu/runq lock, and the correct global private lock. It would look
> like the ordering between the two locks is the wrong one, in Credit2,
> but it's not because of the fact that the per-runq lock is the other
> scheduler's one.
> 
> Tricky, but everything is in here! :-/
> 
I've done it as you suggest above.

The new .switch_sched hook is still there, and still does look the
same. But I do indeed like the final look of the code better, and it
appears to be working ok.

Have a look. ;-)

> > As an aside -- it seems to me that as soon as we change the
> > scheduler
> > lock, there's a risk that something else may come along and try to
> > grab
> > it / access the data.  Does that mean we really ought to use memory
> > barriers to make sure that the lock is written only after all
> > changes
> > to
> > the scheduler data have been appropriately made?
> > 
> Yes, if it were only for this code, I think you're right, barriers
> would be necessary. I once again think this is actually safe, because
> it's serialized elsewhere, but thinking more about it, I can well add
> both barriers (and a comment).
> 
And I've added smp_mb()-s too.

> > > This also means that, in Credit2 and RTDS, we can get rid
> > > of the code that was doing the scheduler lock remapping
> > > in csched2_free_pdata() and rt_free_pdata(), and of their
> > > triggering ASSERT-s.
> > Right -- so to put it a different way, *all* schedulers must now
> > set
> > the
> > locking scheme they wish to use, even if they want to use the
> > default
> > per-cpu locks.  
> > 
> Exactly.
> 
> > 
> > I think that means we have to do that for arinc653 too,
> > right?
> > 
> Mmm... right, I'll have a look at that.
> 
And, finally, I did have a look at this too, and I actually don't think
ARINC needs any of this.

In fact, ARINC brings the idea of "doing its own locking" much further
than the other schedulers we have. They have their lock and they use it
in such a way that they don't even care to what
{v,p}cpu_schedule_lock() and friends points to.

As an example, check a653sched_do_schedule(). It's called from
schedule(), right after taking the runqueue lock,
with pcpu_schedule_lock_irq(), and yet it does this:

 spin_lock_irqsave(&sched_priv->lock, flags);

So, I actually better _not_ add anything to this series that re-maps
sd->schedule_lock to point to their sched_priv->lock, or we'd deadlock!

I'm not sure the design behind all this is the best possible one, but
that's a different issue, to be dealt with with another series in
another moment. :-)

In any case, I've added Robert and Josh.

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

  reply	other threads:[~2016-04-06 16:21 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-18 19:03 [PATCH 00/16] Fixes and improvement (including hard affinity!) for Credit2 Dario Faggioli
2016-03-18 19:04 ` [PATCH 01/16] xen: sched: fix locking when allocating an RTDS pCPU Dario Faggioli
2016-03-19  2:22   ` Meng Xu
2016-03-23 15:37   ` George Dunlap
2016-03-18 19:04 ` [PATCH 02/16] xen: sched: add .init_pdata hook to the scheduler interface Dario Faggioli
2016-03-22  8:08   ` Juergen Gross
2016-03-23 17:32   ` George Dunlap
2016-03-18 19:04 ` [PATCH 03/16] xen: sched: make implementing .alloc_pdata optional Dario Faggioli
2016-03-19  2:23   ` Meng Xu
2016-03-21 14:22   ` Jan Beulich
2016-03-23 17:36     ` George Dunlap
2016-03-24  9:43       ` Jan Beulich
2016-03-24 13:14         ` Dario Faggioli
2016-03-21 14:48   ` Juergen Gross
2016-03-21 15:07     ` Jan Beulich
2016-04-01 17:01       ` Dario Faggioli
2016-04-04  4:21         ` Juergen Gross
2016-04-04  6:13         ` Jan Beulich
2016-04-05 16:01           ` Dario Faggioli
2016-03-23 17:38   ` George Dunlap
2016-03-18 19:04 ` [PATCH 04/16] xen: sched: implement .init_pdata in all schedulers Dario Faggioli
2016-03-19  2:24   ` Meng Xu
2016-03-22  8:03   ` Juergen Gross
2016-03-23 17:46     ` George Dunlap
2016-03-18 19:04 ` [PATCH 05/16] xen: sched: move pCPU initialization in an helper Dario Faggioli
2016-03-23 17:51   ` George Dunlap
2016-03-23 18:09     ` George Dunlap
2016-03-24 13:21     ` Dario Faggioli
2016-03-18 19:04 ` [PATCH 06/16] xen: sched: prepare a .switch_sched hook for Credit1 Dario Faggioli
2016-03-18 19:04 ` [PATCH 07/16] xen: sched: prepare a .switch_sched hook for Credit2 Dario Faggioli
2016-03-18 19:04 ` [PATCH 08/16] " Dario Faggioli
2016-03-19  2:24   ` Meng Xu
2016-03-21 14:25   ` Jan Beulich
2016-03-18 19:05 ` [PATCH 09/16] xen: sched: close potential races when switching scheduler to CPUs Dario Faggioli
2016-03-19  2:25   ` Meng Xu
2016-03-23 19:05   ` George Dunlap
2016-04-05 16:26     ` Dario Faggioli
2016-04-06 15:51       ` Dario Faggioli
2016-03-24 12:14   ` George Dunlap
2016-04-05 17:37     ` Dario Faggioli
2016-04-06 16:21       ` Dario Faggioli [this message]
2016-03-18 19:05 ` [PATCH 10/16] xen: sched: improve credit2 bootparams' scope, placement and signedness Dario Faggioli
2016-03-21 14:51   ` Juergen Gross
2016-03-24 12:20   ` George Dunlap
2016-03-18 19:05 ` [PATCH 11/16] xen: sched: on Credit2, don't reprogram the timer if idle Dario Faggioli
2016-03-24 15:03   ` George Dunlap
2016-03-18 19:05 ` [PATCH 12/16] xen: sched: fix per-socket runqueue creation in credit2 Dario Faggioli
2016-03-24 12:24   ` George Dunlap
2016-03-18 19:05 ` [PATCH 13/16] xen: sched: allow for choosing credit2 runqueues configuration at boot Dario Faggioli
2016-03-22  7:46   ` Juergen Gross
2016-03-24 12:36   ` George Dunlap
2016-03-18 19:05 ` [PATCH 14/16] xen: sched: per-core runqueues as default in credit2 Dario Faggioli
2016-03-24 12:37   ` George Dunlap
2016-03-18 19:06 ` [PATCH 15/16] xen: sched: scratch space for cpumasks on Credit2 Dario Faggioli
2016-03-18 19:27   ` Andrew Cooper
2016-03-24 12:44     ` George Dunlap
2016-03-24 12:56       ` Andrew Cooper
2016-03-24 13:10       ` Dario Faggioli
2016-03-18 19:06 ` [PATCH 16/16] xen: sched: implement vcpu hard affinity in Credit2 Dario Faggioli
2016-03-24 15:42   ` George Dunlap
2016-04-05 16:50     ` Dario Faggioli

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1459959670.3166.100.camel@citrix.com \
    --to=dario.faggioli@citrix.com \
    --cc=george.dunlap@citrix.com \
    --cc=george.dunlap@eu.citrix.com \
    --cc=josh.whitehead@dornerworks.com \
    --cc=mengxu@cis.upenn.edu \
    --cc=robert.vanvossen@dornerworks.com \
    --cc=tiche@seas.upenn.edu \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH 09/16] xen: sched: close potential races when switching scheduler to CPUs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox