From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36889) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuatR-0005pP-QO for qemu-devel@nongnu.org; Tue, 19 May 2015 02:20:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YuatM-0002Oe-OM for qemu-devel@nongnu.org; Tue, 19 May 2015 02:20:45 -0400 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:46719) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuatM-0002N4-47 for qemu-devel@nongnu.org; Tue, 19 May 2015 02:20:40 -0400 Received: from /spool/local by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 19 May 2015 16:20:35 +1000 Received: from d23relay06.au.ibm.com (d23relay06.au.ibm.com [9.185.63.219]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 250323578048 for ; Tue, 19 May 2015 16:20:32 +1000 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay06.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t4J6KM8g31064240 for ; Tue, 19 May 2015 16:20:31 +1000 Received: from d23av03.au.ibm.com (localhost [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t4J6JwpY018840 for ; Tue, 19 May 2015 16:19:58 +1000 Date: Tue, 19 May 2015 11:49:36 +0530 From: Bharata B Rao Message-ID: <20150519061936.GB31025@in.ibm.com> References: <1431408986-18323-1-git-send-email-bharata@linux.vnet.ibm.com> <1431408986-18323-3-git-send-email-bharata@linux.vnet.ibm.com> <20150518183654.GD17796@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150518183654.GD17796@thinpad.lan.raisama.net> Subject: Re: [Qemu-devel] [PATCH v2 2/3] cpus: Convert cpu_index into a bitmap Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: zhugh.fnst@cn.fujitsu.com, agraf@suse.de, qemu-devel@nongnu.org, imammedo@redhat.com, afaerber@suse.de, david@gibson.dropbear.id.au On Mon, May 18, 2015 at 03:36:54PM -0300, Eduardo Habkost wrote: > On Tue, May 12, 2015 at 11:06:25AM +0530, Bharata B Rao wrote: > > Currently CPUState.cpu_index is monotonically increasing and a newly > > created CPU always gets the next higher index. The next available > > index is calculated by counting the existing number of CPUs. This is > > fine as long as we only add CPUs, but there are architectures which > > are starting to support CPU removal too. For an architecture like PowerPC > > which derives its CPU identifier (device tree ID) from cpu_index, the > > existing logic of generating cpu_index values causes problems. > > > > With the currently proposed method of handling vCPU removal by parking > > the vCPU fd in QEMU > > (Ref: http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02604.html), > > generating cpu_index this way will not work for PowerPC. > > > > This patch changes the way cpu_index is handed out by maintaining > > a bit map of the CPUs that tracks both addition and removal of CPUs. > > > > The CPU bitmap allocation logic is part of cpu_exec_init() which is > > called by instance_init routines of various CPU targets. Newly added > > cpu_exec_exit() API handles the deallocation part and this routine is > > called from generic CPU::instance_finalize(). > > > > Note: This new CPU enumeration is for !CONFIG_USER_ONLY only. > > CONFIG_USER_ONLY continues to have the old enumeration logic. > > > > Signed-off-by: Bharata B Rao > > Looks good to me, minor comments below: > > > --- > > exec.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++----- > > include/qom/cpu.h | 1 + > > qom/cpu.c | 7 +++++++ > > 3 files changed, 58 insertions(+), 5 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index 5cf821e..c8c4e53 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -518,21 +518,66 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as) > > } > > #endif > > > > +#ifndef CONFIG_USER_ONLY > > +static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS); > > + > > +static int cpu_get_free_index(Error **errp) > > +{ > > + int cpu = find_first_zero_bit(cpu_index_map, max_cpus); > > + > > + if (cpu >= max_cpus) { > > + error_setg(errp, "Trying to use more CPUs than allowed max of %d\n", > > + max_cpus); > > + return -1; > > + } else { > > + bitmap_set(cpu_index_map, cpu, 1); > > + return cpu; > > + } > > I prefer the following style, to avoid indenting the non-error path: > > if (error_condition) { > error_setg(...); > return; > } > > bitmap_set(...) > return cpu; > > (Which is exactly the style you used in cpu_exec_exit() below, by the > way). Sure, I can change this in next post. > > > +} > > + > > +void cpu_exec_exit(CPUState *cpu) > > +{ > > + if (cpu->cpu_index == -1) { > > + /* cpu_index was never allocated by this @cpu or was already freed. */ > > + return; > > + } > > + > > + bitmap_clear(cpu_index_map, cpu->cpu_index, 1); > > + cpu->cpu_index = -1; > > +} > > +#else > > + > > +static int cpu_get_free_index(Error **errp) > > +{ > > + CPUState *some_cpu; > > + int cpu_index = 0; > > + > > + CPU_FOREACH(some_cpu) { > > + cpu_index++; > > + } > > + return cpu_index; > > +} > > + > > +void cpu_exec_exit(CPUState *cpu) > > +{ > > +} > > +#endif > > + > > void cpu_exec_init(CPUArchState *env, Error **errp) > > { > > CPUState *cpu = ENV_GET_CPU(env); > > CPUClass *cc = CPU_GET_CLASS(cpu); > > - CPUState *some_cpu; > > int cpu_index; > > + Error *local_err = NULL; > > > > #if defined(CONFIG_USER_ONLY) > > cpu_list_lock(); > > #endif > > - cpu_index = 0; > > - CPU_FOREACH(some_cpu) { > > - cpu_index++; > > + cpu_index = cpu->cpu_index = cpu_get_free_index(&local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > } > > - cpu->cpu_index = cpu_index; > > cpu->numa_node = 0; > > QTAILQ_INIT(&cpu->breakpoints); > > QTAILQ_INIT(&cpu->watchpoints); > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > index 39f0f19..7db310e 100644 > > --- a/include/qom/cpu.h > > +++ b/include/qom/cpu.h > > @@ -672,6 +672,7 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask); > > > > void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...) > > GCC_FMT_ATTR(2, 3); > > +void cpu_exec_exit(CPUState *cpu); > > Why don't we keep both cpu_exec_init() and cpu_exec_exit() in the same > header file? Currently cpu_exec_init() is in exec-all.h. 1. If I put cpu_exec_exit() also there, qom/cpu.c doesn't like it since many definitions (like ram_addr_t etc) aren't known in qom/cpu.c. 2. I can't move cpu_exec_init() declaration to qom/cpu.h since it results in the use of poisoned definition of CPUArchState from qom/cpu.c Hence I separated them into different hearder files. Regards, Bharata.