| Message ID | 1430146411-34632-5-git-send-email-mimu@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 27, 2015 at 04:53:18PM +0200, Michael Mueller wrote: > The HMP command info cpus now displays the CPU model name and the > backing accelerator if part of the CPUState. > > (qemu) info cpus > * CPU #0: (halted) model=2827-ga2 accel=kvm thread_id=1679 > > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> Do we really need this? I mean: I expect the amount of CPU data we provide to QMP clients to grow a lot in the near future, but that doesn't mean HMP users need all that data to be printed by "info cpus". > --- > hmp.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hmp.c b/hmp.c > index f142d36..676d821 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -290,6 +290,13 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict) > monitor_printf(mon, " (halted)"); > } > > + if (cpu->value->has_model) { > + monitor_printf(mon, " model=%s", cpu->value->model); > + } > + if (cpu->value->has_accel) { > + monitor_printf(mon, " accel=%s", AccelId_lookup[cpu->value->accel]); > + } > + > monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id); > } > > -- > 1.8.3.1 >
On Tue, 5 May 2015 10:14:32 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Apr 27, 2015 at 04:53:18PM +0200, Michael Mueller wrote: > > The HMP command info cpus now displays the CPU model name and the > > backing accelerator if part of the CPUState. > > > > (qemu) info cpus > > * CPU #0: (halted) model=2827-ga2 accel=kvm thread_id=1679 > > > > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > > Do we really need this? I mean: I expect the amount of CPU data we > provide to QMP clients to grow a lot in the near future, but that > doesn't mean HMP users need all that data to be printed by "info cpus". Where do you see the limit of what is worth to be shown an what not. I personally use "info cpus" less then sporadic but already got a comment internally on that information being worthwhile to be shown. > > > > --- > > hmp.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/hmp.c b/hmp.c > > index f142d36..676d821 100644 > > --- a/hmp.c > > +++ b/hmp.c > > @@ -290,6 +290,13 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict) > > monitor_printf(mon, " (halted)"); > > } > > > > + if (cpu->value->has_model) { > > + monitor_printf(mon, " model=%s", cpu->value->model); > > + } > > + if (cpu->value->has_accel) { > > + monitor_printf(mon, " accel=%s", AccelId_lookup[cpu->value->accel]); > > + } > > + > > monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id); > > } > > > > -- > > 1.8.3.1 > > >
On Wed, May 06, 2015 at 09:32:58AM +0200, Michael Mueller wrote: > On Tue, 5 May 2015 10:14:32 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Apr 27, 2015 at 04:53:18PM +0200, Michael Mueller wrote: > > > The HMP command info cpus now displays the CPU model name and the > > > backing accelerator if part of the CPUState. > > > > > > (qemu) info cpus > > > * CPU #0: (halted) model=2827-ga2 accel=kvm thread_id=1679 > > > > > > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> > > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > > > > Do we really need this? I mean: I expect the amount of CPU data we > > provide to QMP clients to grow a lot in the near future, but that > > doesn't mean HMP users need all that data to be printed by "info cpus". > > Where do you see the limit of what is worth to be shown an what not. > I personally use "info cpus" less then sporadic but already got a comment > internally on that information being worthwhile to be shown. I really don't know, but I think we shouldn't add stuff to HMP unless we have a good reason. For each new piece of data in HMP I would like to at least see the description of a real use case that justifies adding it to HMP and not just implementing a simple script on top of QMP. For accel info we already have "info kvm" that is not ideal but is enough for current use cases, isn't it? CPU model name information seems to be more useful, but if it is just for debugging, people can just run QMP query-cpus command. Luiz, what do you think? > > > > > > > > --- > > > hmp.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/hmp.c b/hmp.c > > > index f142d36..676d821 100644 > > > --- a/hmp.c > > > +++ b/hmp.c > > > @@ -290,6 +290,13 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict) > > > monitor_printf(mon, " (halted)"); > > > } > > > > > > + if (cpu->value->has_model) { > > > + monitor_printf(mon, " model=%s", cpu->value->model); > > > + } > > > + if (cpu->value->has_accel) { > > > + monitor_printf(mon, " accel=%s", AccelId_lookup[cpu->value->accel]); > > > + } > > > + > > > monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id); > > > } > > > > > > -- > > > 1.8.3.1 > > > > > >
On Wed, 6 May 2015 07:38:53 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, May 06, 2015 at 09:32:58AM +0200, Michael Mueller wrote: > > On Tue, 5 May 2015 10:14:32 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, Apr 27, 2015 at 04:53:18PM +0200, Michael Mueller wrote: > > > > The HMP command info cpus now displays the CPU model name and the > > > > backing accelerator if part of the CPUState. > > > > > > > > (qemu) info cpus > > > > * CPU #0: (halted) model=2827-ga2 accel=kvm thread_id=1679 > > > > > > > > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> > > > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > > > > > > Do we really need this? I mean: I expect the amount of CPU data we > > > provide to QMP clients to grow a lot in the near future, but that > > > doesn't mean HMP users need all that data to be printed by "info cpus". > > > > Where do you see the limit of what is worth to be shown an what not. > > I personally use "info cpus" less then sporadic but already got a comment > > internally on that information being worthwhile to be shown. > > I really don't know, but I think we shouldn't add stuff to HMP unless we > have a good reason. For each new piece of data in HMP I would like to at > least see the description of a real use case that justifies adding it to > HMP and not just implementing a simple script on top of QMP. > > For accel info we already have "info kvm" that is not ideal but is > enough for current use cases, isn't it? CPU model name information seems > to be more useful, but if it is just for debugging, people can just run > QMP query-cpus command. > > Luiz, what do you think? I don't see a problem with that. HMP is a debugging interface anyways. Actually, I think it's a good test-case for QMP having a high-level in-tree client (vs. qmp-shell, which is too low-level). If the problem is that a command is dumping too much information to the point of hurting usability, we can split the command or add a '-a' option or something like that. > > > > > > > > > > > > > --- > > > > hmp.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/hmp.c b/hmp.c > > > > index f142d36..676d821 100644 > > > > --- a/hmp.c > > > > +++ b/hmp.c > > > > @@ -290,6 +290,13 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict) > > > > monitor_printf(mon, " (halted)"); > > > > } > > > > > > > > + if (cpu->value->has_model) { > > > > + monitor_printf(mon, " model=%s", cpu->value->model); > > > > + } > > > > + if (cpu->value->has_accel) { > > > > + monitor_printf(mon, " accel=%s", AccelId_lookup[cpu->value->accel]); > > > > + } > > > > + > > > > monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id); > > > > } > > > > > > > > -- > > > > 1.8.3.1 > > > > > > > > > >
On Wed, May 06, 2015 at 08:59:56AM -0400, Luiz Capitulino wrote: > On Wed, 6 May 2015 07:38:53 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, May 06, 2015 at 09:32:58AM +0200, Michael Mueller wrote: > > > On Tue, 5 May 2015 10:14:32 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Mon, Apr 27, 2015 at 04:53:18PM +0200, Michael Mueller wrote: > > > > > The HMP command info cpus now displays the CPU model name and the > > > > > backing accelerator if part of the CPUState. > > > > > > > > > > (qemu) info cpus > > > > > * CPU #0: (halted) model=2827-ga2 accel=kvm thread_id=1679 > > > > > > > > > > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> > > > > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > > > > > > > > Do we really need this? I mean: I expect the amount of CPU data we > > > > provide to QMP clients to grow a lot in the near future, but that > > > > doesn't mean HMP users need all that data to be printed by "info cpus". > > > > > > Where do you see the limit of what is worth to be shown an what not. > > > I personally use "info cpus" less then sporadic but already got a comment > > > internally on that information being worthwhile to be shown. > > > > I really don't know, but I think we shouldn't add stuff to HMP unless we > > have a good reason. For each new piece of data in HMP I would like to at > > least see the description of a real use case that justifies adding it to > > HMP and not just implementing a simple script on top of QMP. > > > > For accel info we already have "info kvm" that is not ideal but is > > enough for current use cases, isn't it? CPU model name information seems > > to be more useful, but if it is just for debugging, people can just run > > QMP query-cpus command. > > > > Luiz, what do you think? > > I don't see a problem with that. HMP is a debugging interface anyways. > Actually, I think it's a good test-case for QMP having a high-level > in-tree client (vs. qmp-shell, which is too low-level). > > If the problem is that a command is dumping too much information to > the point of hurting usability, we can split the command or add a '-a' > option or something like that. Thanks! If HMP is seen as a debugging interface, my main objections aren't valid. That said, I would prefer to keep the command output cleaner and add only the "model" field, as people can use "info kvm" for the accel info by now.
On Wed, 6 May 2015 10:33:55 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, May 06, 2015 at 08:59:56AM -0400, Luiz Capitulino wrote: > > On Wed, 6 May 2015 07:38:53 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Wed, May 06, 2015 at 09:32:58AM +0200, Michael Mueller wrote: > > > > On Tue, 5 May 2015 10:14:32 -0300 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > On Mon, Apr 27, 2015 at 04:53:18PM +0200, Michael Mueller wrote: > > > > > > The HMP command info cpus now displays the CPU model name and the > > > > > > backing accelerator if part of the CPUState. > > > > > > > > > > > > (qemu) info cpus > > > > > > * CPU #0: (halted) model=2827-ga2 accel=kvm thread_id=1679 > > > > > > > > > > > > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> > > > > > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > > > > > > > > > > Do we really need this? I mean: I expect the amount of CPU data we > > > > > provide to QMP clients to grow a lot in the near future, but that > > > > > doesn't mean HMP users need all that data to be printed by "info cpus". > > > > > > > > Where do you see the limit of what is worth to be shown an what not. > > > > I personally use "info cpus" less then sporadic but already got a comment > > > > internally on that information being worthwhile to be shown. > > > > > > I really don't know, but I think we shouldn't add stuff to HMP unless we > > > have a good reason. For each new piece of data in HMP I would like to at > > > least see the description of a real use case that justifies adding it to > > > HMP and not just implementing a simple script on top of QMP. > > > > > > For accel info we already have "info kvm" that is not ideal but is > > > enough for current use cases, isn't it? CPU model name information seems > > > to be more useful, but if it is just for debugging, people can just run > > > QMP query-cpus command. > > > > > > Luiz, what do you think? > > > > I don't see a problem with that. HMP is a debugging interface anyways. > > Actually, I think it's a good test-case for QMP having a high-level > > in-tree client (vs. qmp-shell, which is too low-level). > > > > If the problem is that a command is dumping too much information to > > the point of hurting usability, we can split the command or add a '-a' > > option or something like that. > > Thanks! If HMP is seen as a debugging interface, my main objections > aren't valid. > > That said, I would prefer to keep the command output cleaner and add > only the "model" field, as people can use "info kvm" for the accel info > by now. Ok, I buy that and will kick the accel name out again. Michael >
diff --git a/hmp.c b/hmp.c index f142d36..676d821 100644 --- a/hmp.c +++ b/hmp.c @@ -290,6 +290,13 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict) monitor_printf(mon, " (halted)"); } + if (cpu->value->has_model) { + monitor_printf(mon, " model=%s", cpu->value->model); + } + if (cpu->value->has_accel) { + monitor_printf(mon, " accel=%s", AccelId_lookup[cpu->value->accel]); + } + monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id); }