Re: [Qemu-devel] [PATCH] spapr_cpu_core: instantiate CPUs separately

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2893 bytes --]

On Mon, Nov 06, 2017 at 04:03:07PM +0100, Greg Kurz wrote:
> On Tue, 17 Oct 2017 17:16:09 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Oct 16, 2017 at 10:26:38AM +0200, Igor Mammedov wrote:
> > > On Sat, 14 Oct 2017 20:33:37 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:  
> > > > > The current code assumes that only the CPU core object holds a
> > > > > reference on each individual CPU object, and happily frees their
> > > > > allocated memory when the core is unrealized. This is dangerous
> > > > > as some other code can legitimely keep a pointer to a CPU if it
> > > > > calls object_ref(), but it would end up with a dangling pointer.
> > > > > 
> > > > > Let's allocate all CPUs with object_new() and let QOM frees them
> > > > > when their reference count reaches zero. This greatly simplify the
> > > > > code as we don't have to fiddle with the instance size anymore.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > > 
> > > > So, I'm pretty sure my first drafts of the core stuff did things this
> > > > waym and it got nacked, for QOM lifetime reasons that I never really
> > > > understood.  
> > > From what I remember, Andreas would like to see composite CPU object
> > > allocated in one go and then its children initialized with object_initialize()
> > > so that no more allocation were needed.  
> > 
> > Ah, ok.
> > 
> > > That potentially would benefit hotplug, since we could gracefully
> > > fail object creation early if there is not enough memory.  
> > 
> > Yeah, it sounds nice, but I don't see how we can do it.  In order to
> > do that the core object has to have enough space for all the threads,
> > which means we need both the size of each thread object and the number
> > of them.  The size we have (and will be easier to handle after Igor's
> > cleanups).  The number, we don't.
> > 
> > > But the way it's implemented currently doesn't really match that initial
> > > goal as array for threads is dynamically allocated later
> > > and then we need to dance around it with pointer arithmetic.
> > > 
> > > BTW: almost any allocation failure in qemu currently
> > > is fatal so whether we fail on array alloc or on individual
> > > object_new() won't make any difference.
> > > 
> > > I'd rather see this clean up merged as it simplifies code
> > > in these case.  
> > 
> > Ok, works for me.
> > 
> 
> Since we're in soft freeze already, I guess this won't go to 2.11. Maybe it's
> time to create ppc-for-2.12 and apply it there ?

Yeah, sounds like a plan.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]