From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50275) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e492U-00070T-Lm for qemu-devel@nongnu.org; Mon, 16 Oct 2017 13:18:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1e492Q-0005qt-Lt for qemu-devel@nongnu.org; Mon, 16 Oct 2017 13:18:54 -0400 Received: from 15.mo4.mail-out.ovh.net ([91.121.62.11]:52991) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1e492Q-0005pz-Fi for qemu-devel@nongnu.org; Mon, 16 Oct 2017 13:18:50 -0400 Received: from player159.ha.ovh.net (b9.ovh.net [213.186.33.59]) by mo4.mail-out.ovh.net (Postfix) with ESMTP id F1A97E85D4 for ; Mon, 16 Oct 2017 19:18:48 +0200 (CEST) Date: Mon, 16 Oct 2017 19:16:33 +0200 From: Greg Kurz Message-ID: <20171016191633.4f5082f8@bahia.lan> In-Reply-To: <20171016100718.1ec98df6@nial.brq.redhat.com> References: <150788370618.25736.8030708425923435364.stgit@bahia> <150788373123.25736.7359515819699182906.stgit@bahia> <20171013112459.0082e823@nial.brq.redhat.com> <20171013120546.7a89eeaf@bahia.lan> <20171016100718.1ec98df6@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] monitor: add proper reference counting of the current CPU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, Cornelia Huck , "Dr. David Alan Gilbert" , Markus Armbruster , qemu-ppc@nongnu.org, David Gibson On Mon, 16 Oct 2017 10:07:18 +0200 Igor Mammedov wrote: > On Fri, 13 Oct 2017 12:05:46 +0200 > Greg Kurz wrote: > > > On Fri, 13 Oct 2017 11:24:59 +0200 > > Igor Mammedov wrote: > > > > > On Fri, 13 Oct 2017 10:35:31 +0200 > > > Greg Kurz wrote: > > > > > > > If a CPU selected with the "cpu" command is hot-unplugged then "info cpus" > > > > causes QEMU to exit: > > > > > > > > (qemu) device_del cpu1 > > > > (qemu) info cpus > > > > qemu:qemu_cpu_kick_thread: No such process > > > > > > > > This happens because "cpu" stores the pointer to the selected CPU into > > > > the monitor structure. When the CPU is hot-unplugged, we end up with a > > > > dangling pointer. The "info cpus" command then does: > > > > > > > > hmp_info_cpus() > > > > monitor_get_cpu_index() > > > > mon_get_cpu() > > > > cpu_synchronize_state() <--- called with dangling pointer > > > > > > > > This could cause a QEMU crash as well. > > > > > > > > This patch switches the monitor to use object_ref() to ensure the > > > > CPU object doesn't vanish unexpectedly. The reference is dropped > > > > either when "cpu" is used to switch to another CPU, or when the > > > > selected CPU is unrealized and cpu_list_remove() sets its cpu_index > > > > back to UNASSIGNED_CPU_INDEX. > > > I don't really like an idea of leaving dangling cpu around. > > > Is it possible to store QOM path on set_cpu in monitor and > > > resolving it each to instance each time it's needed? > > > > > > > It sounds workable. Also it would allow the fix to not depend on patch 1 > > for ppc. > yep, > also if you'd touch these cpu_index based monitor commands, > it might make sense to convert them to use qmo path directly > instead if cpu_index (with command completion it would be > easy to use) > Ok, I'll look into that but I'm not sure I can work something out before soft freeze (2017-10-31 AFAIK). > > > > > Thanks for the suggestion! > > > > > > > > > Signed-off-by: Greg Kurz > > > > --- > > > > monitor.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/monitor.c b/monitor.c > > > > index fe0d1bdbb461..1c0b9a2c3ad3 100644 > > > > --- a/monitor.c > > > > +++ b/monitor.c > > > > @@ -579,6 +579,9 @@ static void monitor_data_init(Monitor *mon) > > > > > > > > static void monitor_data_destroy(Monitor *mon) > > > > { > > > > + if (mon->mon_cpu) { > > > > + object_unref((Object *) mon->mon_cpu); > > > > + } > > > > qemu_chr_fe_deinit(&mon->chr, false); > > > > if (monitor_is_qmp(mon)) { > > > > json_message_parser_destroy(&mon->qmp.parser); > > > > @@ -1047,12 +1050,21 @@ int monitor_set_cpu(int cpu_index) > > > > if (cpu == NULL) { > > > > return -1; > > > > } > > > > + if (cur_mon->mon_cpu) { > > > > + object_unref((Object *) cur_mon->mon_cpu); > > > > + } > > > > cur_mon->mon_cpu = cpu; > > > > + object_ref((Object *) cpu); > > > > return 0; > > > > } > > > > > > > > CPUState *mon_get_cpu(void) > > > > { > > > > + if (cur_mon->mon_cpu && > > > > + cur_mon->mon_cpu->cpu_index == UNASSIGNED_CPU_INDEX) { > > > > + object_unref((Object *) cur_mon->mon_cpu); > > > > + cur_mon->mon_cpu = NULL; > > > > + } > > > > if (!cur_mon->mon_cpu) { > > > > if (!first_cpu) { > > > > return NULL; > > > > > > > > > > > > > >