Re: [Qemu-devel] [v2 3/3] hmp: fix MemdevList memory leak
On Tue, 2014-08-12 at 12:00 +1000, Peter Crosthwaite wrote:
> On Mon, Aug 4, 2014 at 2:21 PM, Chen Fan wrote:
> > the memdev_list in hmp_info_memdev() is never freed.
> > so we use existent method qapi_free_MemdevList() to free it.
> > and also we can use qapi_free_MemdevList() to replace list loops
> > to clean up the memdev list in error path.
> >
> > Signed-off-by: Chen Fan
>
> I'm hazy on the subtleties of these QAPI stuffs. Round one review I
> was thinking pure C but your approach looks better than my suggestion.
>
> Reviewed-by: Peter Crosthwaite
Thanks,
Chen
>
> > ---
> > hmp.c | 2 ++
> > numa.c | 9 ++---
> > 2 files changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index ba40c75..40a90da 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1715,4 +1715,6 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
> > }
> >
> > monitor_printf(mon, "\n");
> > +
> > +qapi_free_MemdevList(memdev_list);
> > }
> > diff --git a/numa.c b/numa.c
> > index a2b4bca..b3e140e 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -379,7 +379,7 @@ error:
> > MemdevList *qmp_query_memdev(Error **errp)
> > {
> > Object *obj;
> > -MemdevList *list = NULL, *m;
> > +MemdevList *list = NULL;
> >
> > obj = object_resolve_path("/objects", NULL);
> > if (obj == NULL) {
> > @@ -393,11 +393,6 @@ MemdevList *qmp_query_memdev(Error **errp)
> > return list;
> >
> > error:
> > -while (list) {
> > -m = list;
> > -list = list->next;
> > -g_free(m->value);
> > -g_free(m);
> > -}
> > +qapi_free_MemdevList(list);
> > return NULL;
> > }
> > --
> > 1.9.3
> >
> >
Re: [Qemu-devel] [RESEND v2 0/3] Fix some memory leaks about query memdev
ping... This patches have been reviewed-by, only need someone ack it. Thanks, Chen On Mon, 2014-08-18 at 14:46 +0800, Chen Fan wrote: > when using valgrind to test the command "query memdev", I had > found some memory leaks. the test result: > > ==13802== 4 bytes in 1 blocks are definitely lost in loss record 125 of 8,508 > ==13802==at 0x4A08934: malloc (vg_replace_malloc.c:291) > ==13802==by 0x4A08AA8: realloc (vg_replace_malloc.c:687) > ==13802==by 0x64C5736: g_realloc (in /usr/lib64/libglib-2.0.so.0.3400.2) > ==13802==by 0x64DE226: ??? (in /usr/lib64/libglib-2.0.so.0.3400.2) > ==13802==by 0x64DE279: g_string_sized_new (in > /usr/lib64/libglib-2.0.so.0.3400.2) > ==13802==by 0x47CFBB: string_output_visitor_new > (string-output-visitor.c:341) > ==13802==by 0x3F456F: object_property_get_uint16List (object.c:970) > ==13802==by 0x1E8764: query_memdev (numa.c:361) > ==13802==by 0x3F3248: object_child_foreach (object.c:686) > ==13802==by 0x1E9141: qmp_query_memdev (numa.c:389) > ==13802==by 0x2D79A0: qmp_marshal_input_query_memdev (qmp-marshal.c:5057) > ==13802==by 0x1DD7D7: handle_qmp_command (monitor.c:5038) > > ==15046== 48 (16 direct, 32 indirect) bytes in 1 blocks are definitely lost > in loss record 4,722 of 8,549 > ==15046==at 0x4A08934: malloc (vg_replace_malloc.c:291) > ==15046==by 0x64C541D: ??? (in /usr/lib64/libglib-2.0.so.0.3400.2) > ==15046==by 0x64C56E6: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.3400.2) > ==15046==by 0x1E868C: query_memdev (numa.c:325) > ==15046==by 0x3F3258: object_child_foreach (object.c:686) > ==15046==by 0x1E9141: qmp_query_memdev (numa.c:389) > ==15046==by 0x2DDFF3: hmp_info_memdev (hmp.c:1687) > ==15046==by 0x1E4B08: handle_user_command (monitor.c:4119) > ==15046==by 0x1E4E7A: monitor_command_cb (monitor.c:5156) > ==15046==by 0x496056: readline_handle_byte (readline.c:391) > ==15046==by 0x1E4BCE: monitor_read (monitor.c:5139) > ==15046==by 0x2BCDEF: fd_chr_read (qemu-char.c:213) > > > Chen Fan (3): > query-memdev: fix potential memory leaks > qom/object.c: fix string_output_get_string() memory leak > hmp: fix MemdevList memory leak > > hmp.c| 8 ++-- > numa.c | 15 +++ > qom/object.c | 12 ++-- > 3 files changed, 23 insertions(+), 12 deletions(-) >
Re: [Qemu-devel] [RESEND v2 0/3] Fix some memory leaks about query memdev
ping ? On Mon, 2014-08-18 at 14:46 +0800, Chen Fan wrote: > when using valgrind to test the command "query memdev", I had > found some memory leaks. the test result: > > ==13802== 4 bytes in 1 blocks are definitely lost in loss record 125 of 8,508 > ==13802==at 0x4A08934: malloc (vg_replace_malloc.c:291) > ==13802==by 0x4A08AA8: realloc (vg_replace_malloc.c:687) > ==13802==by 0x64C5736: g_realloc (in /usr/lib64/libglib-2.0.so.0.3400.2) > ==13802==by 0x64DE226: ??? (in /usr/lib64/libglib-2.0.so.0.3400.2) > ==13802==by 0x64DE279: g_string_sized_new (in > /usr/lib64/libglib-2.0.so.0.3400.2) > ==13802==by 0x47CFBB: string_output_visitor_new > (string-output-visitor.c:341) > ==13802==by 0x3F456F: object_property_get_uint16List (object.c:970) > ==13802==by 0x1E8764: query_memdev (numa.c:361) > ==13802==by 0x3F3248: object_child_foreach (object.c:686) > ==13802==by 0x1E9141: qmp_query_memdev (numa.c:389) > ==13802==by 0x2D79A0: qmp_marshal_input_query_memdev (qmp-marshal.c:5057) > ==13802==by 0x1DD7D7: handle_qmp_command (monitor.c:5038) > > ==15046== 48 (16 direct, 32 indirect) bytes in 1 blocks are definitely lost > in loss record 4,722 of 8,549 > ==15046==at 0x4A08934: malloc (vg_replace_malloc.c:291) > ==15046==by 0x64C541D: ??? (in /usr/lib64/libglib-2.0.so.0.3400.2) > ==15046==by 0x64C56E6: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.3400.2) > ==15046==by 0x1E868C: query_memdev (numa.c:325) > ==15046==by 0x3F3258: object_child_foreach (object.c:686) > ==15046==by 0x1E9141: qmp_query_memdev (numa.c:389) > ==15046==by 0x2DDFF3: hmp_info_memdev (hmp.c:1687) > ==15046==by 0x1E4B08: handle_user_command (monitor.c:4119) > ==15046==by 0x1E4E7A: monitor_command_cb (monitor.c:5156) > ==15046==by 0x496056: readline_handle_byte (readline.c:391) > ==15046==by 0x1E4BCE: monitor_read (monitor.c:5139) > ==15046==by 0x2BCDEF: fd_chr_read (qemu-char.c:213) > > > Chen Fan (3): > query-memdev: fix potential memory leaks > qom/object.c: fix string_output_get_string() memory leak > hmp: fix MemdevList memory leak > > hmp.c| 8 ++-- > numa.c | 15 +++ > qom/object.c | 12 ++-- > 3 files changed, 23 insertions(+), 12 deletions(-) >
Re: [Qemu-devel] [RFC 0/3] cpu: add device_add foo-x86_64-cpu support
Hi, I think if we want to use 'device/device_add' to implement CPU, we must do some check before qemu_init_vcpu(). how can we to do that? Thanks, Chen On Tue, 2014-05-13 at 18:08 +0800, Chen Fan wrote: > this patches tried to make cpu hotplug with device_add, > and made -device foo-x86_64-cpu available，also we can > set apic-id property with command line, if without setting > apic-id property, we added first unoccupied apic id as the > default new apic id. and hotplug cpu with device_add, we > must make check of APIC ID after cpu object initialization > that was different from 'cpu_add' command which check 'ids' > at the beginning. > > Chen Fan (3): > using CPUMASK bitmaps to calculate cpu index > cpu: introduce CpuTopoInfo structure for argument simplification > cpu: add device_add foo-x86_64-cpu support > > exec.c | 9 +++-- > include/qom/cpu.h | 11 ++ > include/sysemu/sysemu.h | 7 > qdev-monitor.c | 11 ++ > target-i386/cpu.c | 91 > - > target-i386/topology.h | 51 ++- > 6 files changed, 151 insertions(+), 29 deletions(-) >
Re: [Qemu-devel] [Qemu-trivial] [RESEND v2 0/3] Fix some memory leaks about query memdev
On Fri, 2014-08-29 at 20:29 +0400, Michael Tokarev wrote: > 18.08.2014 10:46, Chen Fan wrote: > > when using valgrind to test the command "query memdev", I had > > found some memory leaks. the test result: > ... > > Applied all 3 to -trivial. Usually these are not really trivial. > Please note that your 2nd patch - "qom/object.c: fix > string_output_get_string() > memory leak" - also touches hmp.c (which has a maintainer), but you > didn't mention this in patch description. I've added it. Thanks, Chen > > Thanks, > > /mjt
Re: [Qemu-devel] [PATCH] gtk.c: Fix memory leak in gd_set_keycode_type()
ping...
On Tue, 2014-09-02 at 14:33 +0800, Chen Fan wrote:
> this memory leak is introduced by the original
> commit 3158a3482b0093e41f2b2596fba50774ea31ae08
>
> valgrind out showing:
> ==14553== 21,459 (72 direct, 21,387 indirect) bytes in 1 blocks are
> definitely
> lost in loss record 8,055 of 8,082
> ==14553==at 0x4A06BC3: calloc (vg_replace_malloc.c:618)
> ==14553==by 0x80DBFBC: XkbGetKeyboardByName (in
> /usr/lib64/libX11.so.6.3.0)
> ==14553==by 0x40C704: gtk_display_init (gtk.c:1798)
> ==14553==by 0x1AEDC1: main (vl.c:4480)
>
> Signed-off-by: Chen Fan
> ---
> ui/gtk.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 2345d7e..cdd2567 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1810,6 +1810,13 @@ static void gd_set_keycode_type(GtkDisplayState *s)
> fprintf(stderr, "unknown keycodes `%s', please report to "
> "qemu-devel@nongnu.org\n", keycodes);
> }
> +
> +if (desc) {
> +XkbFreeKeyboard(desc, XkbGBN_AllComponentsMask, True);
> +}
> +if (keycodes) {
> +XFree(keycodes);
> +}
> }
> #endif
> }
Re: [Qemu-devel] [PATCH] po: Add Chinese translation
On Thu, 2014-07-31 at 10:44 +0800, Fam Zheng wrote: > Signed-off-by: Fam Zheng > --- > po/zh_CN.po | 86 > + > 1 file changed, 86 insertions(+) > create mode 100644 po/zh_CN.po > > diff --git a/po/zh_CN.po b/po/zh_CN.po > new file mode 100644 > index 000..5724ef9 > --- /dev/null > +++ b/po/zh_CN.po > @@ -0,0 +1,86 @@ > +# Chinese translation for QEMU. > +# This file is put in the public domain. > +# > +# Fam Zheng , 2014 > +msgid "" > +msgstr "" > +"Project-Id-Version: QEMU 2.2\n" > +"Report-Msgid-Bugs-To: qemu-devel@nongnu.org\n" > +"POT-Creation-Date: 2014-07-31 10:03+0800\n" > +"PO-Revision-Date: 2014-07-31 10:00+0800\n" > +"Last-Translator: Fam Zheng \n" > +"Language-Team: Chinese \n" > +"Language: zh\n" > +"MIME-Version: 1.0\n" > +"Content-Type: text/plain; charset=UTF-8\n" > +"Content-Transfer-Encoding: 8bit\n" > +"Plural-Forms: nplurals=2; plural=n != 1;\n" > +"X-Generator: Lokalize 1.4\n" > + > +#: ui/gtk.c:321 > +msgid " - Press Ctrl+Alt+G to release grab" > +msgstr "- 按下 Ctrl+Alt+G 取消捕获" > + > +#: ui/gtk.c:325 > +msgid " [Paused]" > +msgstr " [已暂停]" > + > +#: ui/gtk.c:1601 > +msgid "_Pause" > +msgstr "暂停(_P)" > + > +#: ui/gtk.c:1607 > +msgid "_Reset" > +msgstr "重置(_R)" > + > +#: ui/gtk.c:1610 > +msgid "Power _Down" > +msgstr "关闭电源(_D)" > + > +#: ui/gtk.c:1616 > +msgid "_Quit" > +msgstr "退出(_Q)" > + > +#: ui/gtk.c:1692 > +msgid "_Fullscreen" > +msgstr "全屏(_F)" > + > +#: ui/gtk.c:1702 > +msgid "Zoom _In" > +msgstr "放大(_I)" > + > +#: ui/gtk.c:1709 > +msgid "Zoom _Out" > +msgstr "缩小(_O)" > + > +#: ui/gtk.c:1716 > +msgid "Best _Fit" > +msgstr "最合适大小(_F)" > + > +#: ui/gtk.c:1723 > +msgid "Zoom To _Fit" > +msgstr "缩放以适应大小(_F)" > + > +#: ui/gtk.c:1729 > +msgid "Grab On _Hover" > +msgstr "鼠标经过时捕获(_H)" > + > +#: ui/gtk.c:1732 > +msgid "_Grab Input" > +msgstr "捕获输入(_G)" > + > +#: ui/gtk.c:1761 > +msgid "Show _Tabs" > +msgstr "显示标签页(_T)" > + > +#: ui/gtk.c:1764 > +msgid "Detach Tab" > +msgstr "分离标签页" > + > +#: ui/gtk.c:1778 > +msgid "_Machine" > +msgstr "虚拟机(_M)" > + > +#: ui/gtk.c:1783 > +msgid "_View" > +msgstr "试图(_V)" ^^ Hi, I think this should be "视图". Thanks, Chen
Re: [Qemu-devel] [PATCH 2/3] qom/object.c: fix string_output_get_string() memory leak
On Fri, 2014-08-01 at 23:26 +1000, Peter Crosthwaite wrote:
> On Fri, Aug 1, 2014 at 8:22 PM, Chen Fan wrote:
> > string_output_get_string() always return the data the sov->string
>
> "returns the data sov->string points to and never frees it"
>
> Although "sov" is a little out of context however of this change, and
> you need to go diving into qapi code to get context on what you mean
> by that. Perhaps just say it uses g_string_free which requires callers
> to free the returned data?
oh, I will change the commit to better describe that.
>
> > point. and never free.
> >
> > Signed-off-by: Chen Fan
> > ---
> > hmp.c| 6 --
> > qom/object.c | 11 ---
> > 2 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index 4d1838e..2414cc7 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1687,6 +1687,7 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
> > MemdevList *memdev_list = qmp_query_memdev(&err);
> > MemdevList *m = memdev_list;
> > StringOutputVisitor *ov;
> > +char *str;
> > int i = 0;
> >
> >
> > @@ -1704,10 +1705,11 @@ void hmp_info_memdev(Monitor *mon, const QDict
> > *qdict)
> > m->value->prealloc ? "true" : "false");
> > monitor_printf(mon, " policy: %s\n",
> > HostMemPolicy_lookup[m->value->policy]);
> > -monitor_printf(mon, " host nodes: %s\n",
> > - string_output_get_string(ov));
> > +str = string_output_get_string(ov);
> > +monitor_printf(mon, " host nodes: %s\n", str);
> >
> > string_output_visitor_cleanup(ov);
> > +g_free(str);
> > m = m->next;
> > i++;
> > }
> > diff --git a/qom/object.c b/qom/object.c
> > index 0e8267b..7ae4f33 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -948,16 +948,18 @@ int object_property_get_enum(Object *obj, const char
> > *name,
> > {
> > StringOutputVisitor *sov;
> > StringInputVisitor *siv;
> > +char *str;
> > int ret;
> >
> > sov = string_output_visitor_new(false);
> > object_property_get(obj, string_output_get_visitor(sov), name, errp);
> > -siv = string_input_visitor_new(string_output_get_string(sov));
> > +str = string_output_get_string(sov);
> > +siv = string_input_visitor_new(str);
>
> Free here (ASAP) instead of below.
>
> > string_output_visitor_cleanup(sov);
> > visit_type_enum(string_input_get_visitor(siv),
> > &ret, strings, NULL, name, errp);
> > string_input_visitor_cleanup(siv);
> > -
>
> Don't delete existing blank lines that separate sections like this.
>
> > +g_free(str);
> > return ret;
> > }
> >
> > @@ -966,15 +968,18 @@ void object_property_get_uint16List(Object *obj,
> > const char *name,
> > {
> > StringOutputVisitor *ov;
> > StringInputVisitor *iv;
> > +char *str;
> >
> > ov = string_output_visitor_new(false);
> > object_property_get(obj, string_output_get_visitor(ov),
> > name, errp);
> > -iv = string_input_visitor_new(string_output_get_string(ov));
> > +str = string_output_get_string(ov);
> > +iv = string_input_visitor_new(str);
>
> Ditto on the ASAP free
I will fix them soon.
Thanks,
Chen
>
> Regards,
> Peter
>
> > visit_type_uint16List(string_input_get_visitor(iv),
> >list, NULL, errp);
> > string_output_visitor_cleanup(ov);
> > string_input_visitor_cleanup(iv);
> > +g_free(str);
> > }
> >
> > void object_property_parse(Object *obj, const char *string,
> > --
> > 1.9.3
> >
> >
Re: [Qemu-devel] [PATCH 3/3] hmp: fix MemdevList memory leak
On Fri, 2014-08-01 at 23:38 +1000, Peter Crosthwaite wrote:
> On Fri, Aug 1, 2014 at 8:22 PM, Chen Fan wrote:
> > Signed-off-by: Chen Fan
> > ---
> > hmp.c | 9 ++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index 2414cc7..0b1c830 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1685,13 +1685,14 @@ void hmp_info_memdev(Monitor *mon, const QDict
> > *qdict)
> > {
> > Error *err = NULL;
> > MemdevList *memdev_list = qmp_query_memdev(&err);
> > -MemdevList *m = memdev_list;
> > +MemdevList *m;
> > StringOutputVisitor *ov;
> > char *str;
> > int i = 0;
> >
> >
> > -while (m) {
> > +while (memdev_list) {
> > +m = memdev_list;
> > ov = string_output_visitor_new(false);
> > visit_type_uint16List(string_output_get_visitor(ov),
> >&m->value->host_nodes, NULL, NULL);
> > @@ -1710,7 +1711,9 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
> >
> > string_output_visitor_cleanup(ov);
> > g_free(str);
> > -m = m->next;
> > +memdev_list = memdev_list->next;
> > +g_free(m->value);
> > +g_free(m);
>
> This doesnt feel quite right. You are piecewise freeing an entire data
> structure as allocated by a single function (qmp_query_memdev). I
> think you should create a function that cleans up MemdevList (just
> loops and frees) so that any and all callers of qmp_query_memdev can
> cleanup in one single action.
>
> Note that qmp_query_memdev already has the code you need in it's error path:
>
> while (list) {
> m = list;
> list = list->next;
> g_free(m->value);
> g_free(m);
> }
>
> So you can split this out into it's own fn and call it both here and
> in that error path.
You're right. I will add a common fn to free them.
Thanks,
Chen
>
> Regards,
> Peter
>
> > i++;
> > }
> >
> > --
> > 1.9.3
> >
> >
Re: [Qemu-devel] [PATCH 2/3] qom/object.c: fix string_output_get_string() memory leak
On Fri, 2014-08-01 at 23:26 +1000, Peter Crosthwaite wrote:
> On Fri, Aug 1, 2014 at 8:22 PM, Chen Fan wrote:
> > string_output_get_string() always return the data the sov->string
>
> "returns the data sov->string points to and never frees it"
>
> Although "sov" is a little out of context however of this change, and
> you need to go diving into qapi code to get context on what you mean
> by that. Perhaps just say it uses g_string_free which requires callers
> to free the returned data?
>
> > point. and never free.
> >
> > Signed-off-by: Chen Fan
> > ---
> > hmp.c| 6 --
> > qom/object.c | 11 ---
> > 2 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index 4d1838e..2414cc7 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1687,6 +1687,7 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
> > MemdevList *memdev_list = qmp_query_memdev(&err);
> > MemdevList *m = memdev_list;
> > StringOutputVisitor *ov;
> > +char *str;
> > int i = 0;
> >
> >
> > @@ -1704,10 +1705,11 @@ void hmp_info_memdev(Monitor *mon, const QDict
> > *qdict)
> > m->value->prealloc ? "true" : "false");
> > monitor_printf(mon, " policy: %s\n",
> > HostMemPolicy_lookup[m->value->policy]);
> > -monitor_printf(mon, " host nodes: %s\n",
> > - string_output_get_string(ov));
> > +str = string_output_get_string(ov);
> > +monitor_printf(mon, " host nodes: %s\n", str);
> >
> > string_output_visitor_cleanup(ov);
> > +g_free(str);
> > m = m->next;
> > i++;
> > }
> > diff --git a/qom/object.c b/qom/object.c
> > index 0e8267b..7ae4f33 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -948,16 +948,18 @@ int object_property_get_enum(Object *obj, const char
> > *name,
> > {
> > StringOutputVisitor *sov;
> > StringInputVisitor *siv;
> > +char *str;
> > int ret;
> >
> > sov = string_output_visitor_new(false);
> > object_property_get(obj, string_output_get_visitor(sov), name, errp);
> > -siv = string_input_visitor_new(string_output_get_string(sov));
> > +str = string_output_get_string(sov);
> > +siv = string_input_visitor_new(str);
>
> Free here (ASAP) instead of below.
here, I think free here is incorrect, the below visit_type_enum() will
call visit_type_str which strdups the 'str', we should not free it
before that, so let it there.
Thanks,
Chen
>
> > string_output_visitor_cleanup(sov);
> > visit_type_enum(string_input_get_visitor(siv),
> > &ret, strings, NULL, name, errp);
> > string_input_visitor_cleanup(siv);
> > -
>
> Don't delete existing blank lines that separate sections like this.
>
> > +g_free(str);
> > return ret;
> > }
> >
> > @@ -966,15 +968,18 @@ void object_property_get_uint16List(Object *obj,
> > const char *name,
> > {
> > StringOutputVisitor *ov;
> > StringInputVisitor *iv;
> > +char *str;
> >
> > ov = string_output_visitor_new(false);
> > object_property_get(obj, string_output_get_visitor(ov),
> > name, errp);
> > -iv = string_input_visitor_new(string_output_get_string(ov));
> > +str = string_output_get_string(ov);
> > +iv = string_input_visitor_new(str);
>
> Ditto on the ASAP free
>
> Regards,
> Peter
>
> > visit_type_uint16List(string_input_get_visitor(iv),
> >list, NULL, errp);
> > string_output_visitor_cleanup(ov);
> > string_input_visitor_cleanup(iv);
> > +g_free(str);
> > }
> >
> > void object_property_parse(Object *obj, const char *string,
> > --
> > 1.9.3
> >
> >
Re: [Qemu-devel] [PATCH v2 0/3] prebuild cpu QOM tree /machine/node/socket/core ->link-cpu
Ping... On Thu, 2014-03-20 at 14:33 +0800, Chen Fan wrote: > at present, after hotplug a discontinuous cpu id on source, then done > migration, > on target, it will fail to add the unoccupied cpu id which was skipped at > source, > this cause is on target Qemu prebuild CPU with continuous cpu_index. so after > migration, the cpu infrastructure bewteen source and target are different. > > I suppose we could use apic_id as instance_id which was used at registering > vmstate > when create cpu. on target, we prebuild the specified cpu topology using > comand line: > -device /machine/node[]/socket[]/core[]/cpu[], then migration, we could > keep the same > cpu infrastructure on both side. > > v1-v2: squash patch 2/4 and 3/4 together. > > RFC: > V4: rename CpuTopoInfo to X86CPUTopoInfo. and move cpu_exsit() to > pc_new_cpu(). > > V3: get rid of thread object and tie link to directly. and > prebuild full > core[] and thread[] as init socket[] according to smp_cores and smp_threads. > > TODO: > 1. add cpu "path" property which used for specifying the QOM path. > 2. add -device cpu-foo.path supported. > 3. then we could introduce hot-remove cpu probably. > > I don't know wether this way is right or not. pls tell me. :) > > Chen Fan (3): > cpu: introduce CpuTopoInfo structure for argument simplification > i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu() > i386: introduce cpu QOM hierarchy tree > > hw/i386/pc.c | 25 +++--- > target-i386/Makefile.objs | 2 +- > target-i386/cpu-topology.c | 199 > + > target-i386/cpu-topology.h | 71 > target-i386/cpu.c | 55 + > target-i386/cpu.h | 8 ++ > target-i386/topology.h | 53 +++- > tests/test-x86-cpuid.c | 165 + > 8 files changed, 494 insertions(+), 84 deletions(-) > create mode 100644 target-i386/cpu-topology.c > create mode 100644 target-i386/cpu-topology.h >
Re: [Qemu-devel] [RFC 1/3] using CPUMASK bitmaps to calculate cpu index
On Thu, 2014-05-22 at 15:26 +0200, Igor Mammedov wrote:
> On Tue, 13 May 2014 18:08:47 +0800
> Chen Fan wrote:
>
> > instead of seeking the number of CPUs, using CPUMASK bitmaps to
> > calculate the cpu index, also would be a gread benefit to remove
> > cpu index.
> How would it help to remove cpu_index?
I think the cpu_index is just a CPU counter，if we remove one sparse CPU
by setting corresponding bit to zero. then adding a new one we could
easily get this vacancy bit by find_first_zero_bit() rather than get a
duplicate index by calculating the number of CPU.
> What if there is only one CPU at start nad there is a need to add
> CPU with cpu_index==10?
>
> Bitmap hardly changes here anything, it's just another way of
> doing:
>cpu_index = 0;
>CPU_FOREACH(some_cpu) {
>cpu_index++;
>}
>
>
> What would help however is replacing cpu_index at read points with
> CPUClass->get_arch_id()
>
> Then targets that need sparse index could override default
> get_arch_id() to suite their needs and use their own properties
> to set arch_id value.
>
> Caution: this change would cause migration breakage for target-i386,
> so x86_cpu_get_arch_id() should take care about it when used with old
> machine types.
yes, but for migration, I think we should need to override the cpu index
in function vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu)
with an unique instance_id while hot-add/hot-remove CPU arbitrarily.
maybe this unique instance_id can be 'apic-id'.
Thanks,
Chen
>
> >
> > Signed-off-by: Chen Fan
> > ---
> > exec.c | 9 -
> > include/qom/cpu.h | 9 +
> > include/sysemu/sysemu.h | 7 ---
> > 3 files changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index cf12049..2948841 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -473,16 +473,15 @@ void cpu_exec_init(CPUArchState *env)
> > {
> > CPUState *cpu = ENV_GET_CPU(env);
> > CPUClass *cc = CPU_GET_CLASS(cpu);
> > -CPUState *some_cpu;
> > int cpu_index;
> >
> > #if defined(CONFIG_USER_ONLY)
> > cpu_list_lock();
> > #endif
> > -cpu_index = 0;
> > -CPU_FOREACH(some_cpu) {
> > -cpu_index++;
> > -}
> > +cpu_index = find_first_zero_bit(cc->cpu_present_mask,
> > +MAX_CPUMASK_BITS);
> > +set_bit(cpu_index, cc->cpu_present_mask);
> > +
> > cpu->cpu_index = cpu_index;
> > cpu->numa_node = 0;
> > QTAILQ_INIT(&cpu->breakpoints);
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index df977c8..b8f46b1 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -70,6 +70,13 @@ typedef void (*CPUUnassignedAccess)(CPUState *cpu,
> > hwaddr addr,
> >
> > struct TranslationBlock;
> >
> > +/* The following shall be true for all CPUs:
> > + * cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS
> > + *
> > + * Note that cpu->get_arch_id() may be larger than MAX_CPUMASK_BITS.
> > + */
> > +#define MAX_CPUMASK_BITS 255
> > +
> > /**
> > * CPUClass:
> > * @class_by_name: Callback to map -cpu command line model name to an
> > @@ -142,6 +149,8 @@ typedef struct CPUClass {
> > const struct VMStateDescription *vmsd;
> > int gdb_num_core_regs;
> > const char *gdb_core_xml_file;
> > +
> > +DECLARE_BITMAP(cpu_present_mask, MAX_CPUMASK_BITS);
> > } CPUClass;
> >
> > #ifdef HOST_WORDS_BIGENDIAN
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index ba5c7f8..04edb8b 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -134,13 +134,6 @@ extern QEMUClockType rtc_clock;
> >
> > #define MAX_NODES 64
> >
> > -/* The following shall be true for all CPUs:
> > - * cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS
> > - *
> > - * Note that cpu->get_arch_id() may be larger than MAX_CPUMASK_BITS.
> > - */
> > -#define MAX_CPUMASK_BITS 255
> > -
> > extern int nb_numa_nodes;
> > extern uint64_t node_mem[MAX_NODES];
> > extern unsigned long *node_cpumask[MAX_NODES];
>
